Provide implicit saves for XBlocks and XModules.

Update existing tests and provide new ones to test new paradigm.

CHANGELOG.rst

@@ -9,6 +9,8 @@ Common: Added *experimental* support for jsinput type.
 
 Common: Added setting to specify Celery Broker vhost
 
+Common: Utilize new XBlock bulk save API in LMS and CMS.
+
 Studio: Add table for tracking course creator permissions (not yet used).
 Update rake django-admin[syncdb] and rake django-admin[migrate] so they
 run for both LMS and CMS.

cms/djangoapps/contentstore/tests/test_checklists.py

@@ -46,6 +46,8 @@ class ChecklistTestCase(CourseTestCase):
         # Now delete the checklists from the course and verify they get repopulated (for courses
         # created before checklists were introduced).
         self.course.checklists = None
+        # Save the changed `checklists` to the underlying KeyValueStore before updating the modulestore
+        self.course.save()
         modulestore = get_modulestore(self.course.location)
         modulestore.update_metadata(self.course.location, own_metadata(self.course))
         self.assertEqual(self.get_persisted_checklists(), None)

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -87,6 +87,8 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         self.user.is_active = True
         # Staff has access to view all courses
         self.user.is_staff = True
+
+        # Save the data that we've just changed to the db.
         self.user.save()
 
         self.client = Client()
@@ -117,6 +119,10 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         course.advanced_modules = component_types
 
+        # Save the data that we've just changed to the underlying
+        # MongoKeyValueStore before we update the mongo datastore.
+        course.save()
+
         store.update_metadata(course.location, own_metadata(course))
 
         # just pick one vertical
@@ -239,6 +245,9 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         self.assertNotIn('graceperiod', own_metadata(html_module))
         html_module.lms.graceperiod = new_graceperiod
+        # Save the data that we've just changed to the underlying
+        # MongoKeyValueStore before we update the mongo datastore.
+        html_module.save()
         self.assertIn('graceperiod', own_metadata(html_module))
         self.assertEqual(html_module.lms.graceperiod, new_graceperiod)
 
@@ -883,6 +892,9 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         # add a bool piece of unknown metadata so we can verify we don't throw an exception
         metadata['new_metadata'] = True
 
+        # Save the data that we've just changed to the underlying
+        # MongoKeyValueStore before we update the mongo datastore.
+        course.save()
         module_store.update_metadata(location, metadata)
 
         print 'Exporting to tempdir = {0}'.format(root_dir)
@@ -1299,6 +1311,7 @@ class ContentStoreTest(ModuleStoreTestCase):
         # now let's define an override at the leaf node level
         #
         new_module.lms.graceperiod = timedelta(1)
+        new_module.save()
         module_store.update_metadata(new_module.location, own_metadata(new_module))
 
         # flush the cache and refetch

cms/djangoapps/contentstore/tests/test_course_settings.py

@@ -290,6 +290,71 @@ class CourseGradingTest(CourseTestCase):
         altered_grader = CourseGradingModel.update_grader_from_json(test_grader.course_location, test_grader.graders[1])
         self.assertDictEqual(test_grader.graders[1], altered_grader, "drop_count[1] + 2")
 
+    def test_update_cutoffs_from_json(self):
+        test_grader = CourseGradingModel.fetch(self.course.location)
+        CourseGradingModel.update_cutoffs_from_json(test_grader.course_location, test_grader.grade_cutoffs)
+        # Unlike other tests, need to actually perform a db fetch for this test since update_cutoffs_from_json
+        #  simply returns the cutoffs you send into it, rather than returning the db contents.
+        altered_grader = CourseGradingModel.fetch(self.course.location)
+        self.assertDictEqual(test_grader.grade_cutoffs, altered_grader.grade_cutoffs, "Noop update")
+
+        test_grader.grade_cutoffs['D'] = 0.3
+        CourseGradingModel.update_cutoffs_from_json(test_grader.course_location, test_grader.grade_cutoffs)
+        altered_grader = CourseGradingModel.fetch(self.course.location)
+        self.assertDictEqual(test_grader.grade_cutoffs, altered_grader.grade_cutoffs, "cutoff add D")
+
+        test_grader.grade_cutoffs['Pass'] = 0.75
+        CourseGradingModel.update_cutoffs_from_json(test_grader.course_location, test_grader.grade_cutoffs)
+        altered_grader = CourseGradingModel.fetch(self.course.location)
+        self.assertDictEqual(test_grader.grade_cutoffs, altered_grader.grade_cutoffs, "cutoff change 'Pass'")
+
+    def test_delete_grace_period(self):
+        test_grader = CourseGradingModel.fetch(self.course.location)
+        CourseGradingModel.update_grace_period_from_json(test_grader.course_location, test_grader.grace_period)
+        # update_grace_period_from_json doesn't return anything, so query the db for its contents.
+        altered_grader = CourseGradingModel.fetch(self.course.location)
+        self.assertEqual(test_grader.grace_period, altered_grader.grace_period, "Noop update")
+
+        test_grader.grace_period = {'hours': 15, 'minutes': 5, 'seconds': 30}
+        CourseGradingModel.update_grace_period_from_json(test_grader.course_location, test_grader.grace_period)
+        altered_grader = CourseGradingModel.fetch(self.course.location)
+        self.assertDictEqual(test_grader.grace_period, altered_grader.grace_period, "Adding in a grace period")
+
+        test_grader.grace_period = {'hours': 1, 'minutes': 10, 'seconds': 0}
+        # Now delete the grace period
+        CourseGradingModel.delete_grace_period(test_grader.course_location)
+        # update_grace_period_from_json doesn't return anything, so query the db for its contents.
+        altered_grader = CourseGradingModel.fetch(self.course.location)
+        # Once deleted, the grace period should simply be None
+        self.assertEqual(None, altered_grader.grace_period, "Delete grace period")
+
+    def test_update_section_grader_type(self):
+        # Get the descriptor and the section_grader_type and assert they are the default values
+        descriptor = get_modulestore(self.course.location).get_item(self.course.location)
+        section_grader_type = CourseGradingModel.get_section_grader_type(self.course.location)
+
+        self.assertEqual('Not Graded', section_grader_type['graderType'])
+        self.assertEqual(None, descriptor.lms.format)
+        self.assertEqual(False, descriptor.lms.graded)
+
+        # Change the default grader type to Homework, which should also mark the section as graded
+        CourseGradingModel.update_section_grader_type(self.course.location, {'graderType': 'Homework'})
+        descriptor = get_modulestore(self.course.location).get_item(self.course.location)
+        section_grader_type = CourseGradingModel.get_section_grader_type(self.course.location)
+
+        self.assertEqual('Homework', section_grader_type['graderType'])
+        self.assertEqual('Homework', descriptor.lms.format)
+        self.assertEqual(True, descriptor.lms.graded)
+
+        # Change the grader type back to Not Graded, which should also unmark the section as graded
+        CourseGradingModel.update_section_grader_type(self.course.location, {'graderType': 'Not Graded'})
+        descriptor = get_modulestore(self.course.location).get_item(self.course.location)
+        section_grader_type = CourseGradingModel.get_section_grader_type(self.course.location)
+
+        self.assertEqual('Not Graded', section_grader_type['graderType'])
+        self.assertEqual(None, descriptor.lms.format)
+        self.assertEqual(False, descriptor.lms.graded)
+
 
 class CourseMetadataEditingTest(CourseTestCase):
     """

cms/djangoapps/contentstore/tests/test_textbooks.py

@@ -62,6 +62,9 @@ class TextbookIndexTestCase(CourseTestCase):
             }
         ]
         self.course.pdf_textbooks = content
+        # Save the data that we've just changed to the underlying
+        # MongoKeyValueStore before we update the mongo datastore.
+        self.course.save()
         store = get_modulestore(self.course.location)
         store.update_metadata(self.course.location, own_metadata(self.course))
 
@@ -220,6 +223,9 @@ class TextbookByIdTestCase(CourseTestCase):
             'tid': 2,
         })
         self.course.pdf_textbooks = [self.textbook1, self.textbook2]
+        # Save the data that we've just changed to the underlying
+        # MongoKeyValueStore before we update the mongo datastore.
+        self.course.save()
         self.store = get_modulestore(self.course.location)
         self.store.update_metadata(self.course.location, own_metadata(self.course))
         self.url_nonexist = reverse('textbook_by_id', kwargs={

cms/djangoapps/contentstore/views/course.py

 """
 Views related to operations on course objects
 """
-#pylint: disable=W0402
 import json
 import random
-import string
+import string  # pylint: disable=W0402
 
 from django.contrib.auth.decorators import login_required
 from django_future.csrf import ensure_csrf_cookie
@@ -496,6 +495,9 @@ def textbook_index(request, org, course, name):
             if not any(tab['type'] == 'pdf_textbooks' for tab in course_module.tabs):
                 course_module.tabs.append({"type": "pdf_textbooks"})
             course_module.pdf_textbooks = textbooks
+            # Save the data that we've just changed to the underlying
+            # MongoKeyValueStore before we update the mongo datastore.
+            course_module.save()
             store.update_metadata(course_module.location, own_metadata(course_module))
             return JsonResponse(course_module.pdf_textbooks)
     else:
@@ -542,6 +544,9 @@ def create_textbook(request, org, course, name):
         tabs = course_module.tabs
         tabs.append({"type": "pdf_textbooks"})
         course_module.tabs = tabs
+    # Save the data that we've just changed to the underlying
+    # MongoKeyValueStore before we update the mongo datastore.
+    course_module.save()
     store.update_metadata(course_module.location, own_metadata(course_module))
     resp = JsonResponse(textbook, status=201)
     resp["Location"] = reverse("textbook_by_id", kwargs={
@@ -585,10 +590,13 @@ def textbook_by_id(request, org, course, name, tid):
             i = course_module.pdf_textbooks.index(textbook)
             new_textbooks = course_module.pdf_textbooks[0:i]
             new_textbooks.append(new_textbook)
-            new_textbooks.extend(course_module.pdf_textbooks[i+1:])
+            new_textbooks.extend(course_module.pdf_textbooks[i + 1:])
             course_module.pdf_textbooks = new_textbooks
         else:
             course_module.pdf_textbooks.append(new_textbook)
+        # Save the data that we've just changed to the underlying
+        # MongoKeyValueStore before we update the mongo datastore.
+        course_module.save()
         store.update_metadata(course_module.location, own_metadata(course_module))
         return JsonResponse(new_textbook, status=201)
     elif request.method == 'DELETE':
@@ -596,7 +604,8 @@ def textbook_by_id(request, org, course, name, tid):
             return JsonResponse(status=404)
         i = course_module.pdf_textbooks.index(textbook)
         new_textbooks = course_module.pdf_textbooks[0:i]
-        new_textbooks.extend(course_module.pdf_textbooks[i+1:])
+        new_textbooks.extend(course_module.pdf_textbooks[i + 1:])
         course_module.pdf_textbooks = new_textbooks
+        course_module.save()
         store.update_metadata(course_module.location, own_metadata(course_module))
         return JsonResponse()

cms/djangoapps/contentstore/views/item.py

@@ -70,6 +70,9 @@ def save_item(request):
                 delattr(existing_item, metadata_key)
             else:
                 setattr(existing_item, metadata_key, value)
+        # Save the data that we've just changed to the underlying
+        # MongoKeyValueStore before we update the mongo datastore.
+        existing_item.save()
         # commit to datastore
         store.update_metadata(item_location, own_metadata(existing_item))
 

cms/djangoapps/contentstore/views/preview.py

@@ -7,7 +7,7 @@ from django.core.urlresolvers import reverse
 from django.contrib.auth.decorators import login_required
 from mitxmako.shortcuts import render_to_response
 
-from xmodule_modifiers import replace_static_urls, wrap_xmodule
+from xmodule_modifiers import replace_static_urls, wrap_xmodule, save_module  # pylint: disable=F0401
 from xmodule.error_module import ErrorDescriptor
 from xmodule.errortracker import exc_info_to_str
 from xmodule.exceptions import NotFoundError, ProcessingError
@@ -47,6 +47,8 @@ def preview_dispatch(request, preview_id, location, dispatch=None):
     # Let the module handle the AJAX
     try:
         ajax_return = instance.handle_ajax(dispatch, request.POST)
+        # Save any module data that has changed to the underlying KeyValueStore
+        instance.save()
 
     except NotFoundError:
         log.exception("Module indicating to user that request doesn't exist")
@@ -166,6 +168,11 @@ def load_preview_module(request, preview_id, descriptor):
         course_namespace=Location([module.location.tag, module.location.org, module.location.course, None, None])
     )
 
+    module.get_html = save_module(
+        module.get_html,
+        module
+    )
+
     return module
 
 

cms/djangoapps/contentstore/views/tabs.py

@@ -76,6 +76,9 @@ def reorder_static_tabs(request):
 
     # OK, re-assemble the static tabs in the new order
     course.tabs = reordered_tabs
+    # Save the data that we've just changed to the underlying
+    # MongoKeyValueStore before we update the mongo datastore.
+    course.save()
     modulestore('direct').update_metadata(course.location, own_metadata(course))
     return HttpResponse()
 

cms/djangoapps/models/settings/course_details.py

@@ -122,6 +122,10 @@ class CourseDetails(object):
             descriptor.enrollment_end = converted
 
         if dirty:
+            # Save the data that we've just changed to the underlying
+            # MongoKeyValueStore before we update the mongo datastore.
+            descriptor.save()
+
             get_modulestore(course_location).update_metadata(course_location, own_metadata(descriptor))
 
         # NOTE: below auto writes to the db w/o verifying that any of the fields actually changed

cms/djangoapps/models/settings/course_grading.py

@@ -7,6 +7,9 @@ class CourseGradingModel(object):
     """
     Basically a DAO and Model combo for CRUD operations pertaining to grading policy.
     """
+    # Within this class, allow access to protected members of client classes.
+    # This comes up when accessing kvs data and caches during kvs saves and modulestore writes.
+    # pylint: disable=W0212
     def __init__(self, course_descriptor):
         self.course_location = course_descriptor.location
         self.graders = [CourseGradingModel.jsonize_grader(i, grader) for i, grader in enumerate(course_descriptor.raw_grader)]  # weights transformed to ints [0..100]
@@ -83,13 +86,16 @@ class CourseGradingModel(object):
         """
         course_location = Location(jsondict['course_location'])
         descriptor = get_modulestore(course_location).get_item(course_location)
-
         graders_parsed = [CourseGradingModel.parse_grader(jsonele) for jsonele in jsondict['graders']]
 
         descriptor.raw_grader = graders_parsed
         descriptor.grade_cutoffs = jsondict['grade_cutoffs']
 
+        # Save the data that we've just changed to the underlying
+        # MongoKeyValueStore before we update the mongo datastore.
+        descriptor.save()
         get_modulestore(course_location).update_item(course_location, descriptor.xblock_kvs._data)
+
         CourseGradingModel.update_grace_period_from_json(course_location, jsondict['grace_period'])
 
         return CourseGradingModel.fetch(course_location)
@@ -116,6 +122,9 @@ class CourseGradingModel(object):
         else:
             descriptor.raw_grader.append(grader)
 
+        # Save the data that we've just changed to the underlying
+        # MongoKeyValueStore before we update the mongo datastore.
+        descriptor.save()
         get_modulestore(course_location).update_item(course_location, descriptor._model_data._kvs._data)
 
         return CourseGradingModel.jsonize_grader(index, descriptor.raw_grader[index])
@@ -131,6 +140,10 @@ class CourseGradingModel(object):
 
         descriptor = get_modulestore(course_location).get_item(course_location)
         descriptor.grade_cutoffs = cutoffs
+
+        # Save the data that we've just changed to the underlying
+        # MongoKeyValueStore before we update the mongo datastore.
+        descriptor.save()
         get_modulestore(course_location).update_item(course_location, descriptor._model_data._kvs._data)
 
         return cutoffs
@@ -156,6 +169,10 @@ class CourseGradingModel(object):
 
             descriptor = get_modulestore(course_location).get_item(course_location)
             descriptor.lms.graceperiod = grace_timedelta
+
+            # Save the data that we've just changed to the underlying
+            # MongoKeyValueStore before we update the mongo datastore.
+            descriptor.save()
             get_modulestore(course_location).update_metadata(course_location, descriptor._model_data._kvs._metadata)
 
     @staticmethod
@@ -172,22 +189,11 @@ class CourseGradingModel(object):
             del descriptor.raw_grader[index]
             # force propagation to definition
             descriptor.raw_grader = descriptor.raw_grader
-            get_modulestore(course_location).update_item(course_location, descriptor._model_data._kvs._data)
 
-    # NOTE cannot delete cutoffs. May be useful to reset
-    @staticmethod
-    def delete_cutoffs(course_location, cutoffs):
-        """
-        Resets the cutoffs to the defaults
-        """
-        if not isinstance(course_location, Location):
-            course_location = Location(course_location)
-
-        descriptor = get_modulestore(course_location).get_item(course_location)
-        descriptor.grade_cutoffs = descriptor.defaut_grading_policy['GRADE_CUTOFFS']
-        get_modulestore(course_location).update_item(course_location, descriptor._model_data._kvs._data)
-
-        return descriptor.grade_cutoffs
+            # Save the data that we've just changed to the underlying
+            # MongoKeyValueStore before we update the mongo datastore.
+            descriptor.save()
+            get_modulestore(course_location).update_item(course_location, descriptor._model_data._kvs._data)
 
     @staticmethod
     def delete_grace_period(course_location):
@@ -199,6 +205,10 @@ class CourseGradingModel(object):
 
         descriptor = get_modulestore(course_location).get_item(course_location)
         del descriptor.lms.graceperiod
+
+        # Save the data that we've just changed to the underlying
+        # MongoKeyValueStore before we update the mongo datastore.
+        descriptor.save()
         get_modulestore(course_location).update_metadata(course_location, descriptor._model_data._kvs._metadata)
 
     @staticmethod
@@ -225,6 +235,9 @@ class CourseGradingModel(object):
             del descriptor.lms.format
             del descriptor.lms.graded
 
+        # Save the data that we've just changed to the underlying
+        # MongoKeyValueStore before we update the mongo datastore.
+        descriptor.save()
         get_modulestore(location).update_metadata(location, descriptor._model_data._kvs._metadata)
 
     @staticmethod

cms/djangoapps/models/settings/course_metadata.py

@@ -76,6 +76,9 @@ class CourseMetadata(object):
                 setattr(descriptor.lms, key, value)
 
         if dirty:
+            # Save the data that we've just changed to the underlying
+            # MongoKeyValueStore before we update the mongo datastore.
+            descriptor.save()
             get_modulestore(course_location).update_metadata(course_location,
                                                              own_metadata(descriptor))
 
@@ -97,6 +100,10 @@ class CourseMetadata(object):
             elif hasattr(descriptor.lms, key):
                 delattr(descriptor.lms, key)
 
+        # Save the data that we've just changed to the underlying
+        # MongoKeyValueStore before we update the mongo datastore.
+        descriptor.save()
+
         get_modulestore(course_location).update_metadata(course_location,
                                                          own_metadata(descriptor))
 

common/djangoapps/xmodule_modifiers.py

@@ -89,6 +89,21 @@ def grade_histogram(module_id):
     return grades
 
 
+def save_module(get_html, module):
+    """
+    Updates the given get_html function for the given module to save the fields
+    after rendering.
+    """
+    @wraps(get_html)
+    def _get_html():
+        """Cache the rendered output, save, then return the output."""
+        rendered_html = get_html()
+        module.save()
+        return rendered_html
+
+    return _get_html
+
+
 def add_histogram(get_html, module, user):
     """
     Updates the supplied module with a new get_html function that wraps

common/lib/xmodule/xmodule/modulestore/mongo/base.py

@@ -108,8 +108,9 @@ class MongoKeyValueStore(KeyValueStore):
     def set_many(self, update_dict):
         """set_many method. Implementations should accept an `update_dict` of
         key-value pairs, and set all the `keys` to the given `value`s."""
-        # It appears that `set` simply updates an in-memory db, rather than calling down
-        # to a real db; need to figure out if this is the case.
+        # `set` simply updates an in-memory db, rather than calling down to a real db,
+        # as mongo bulk save is handled elsewhere. A future improvement would be to pull
+        # the mongo-specific bulk save logic into this method.
         for key, value in update_dict.iteritems():
             self.set(key, value)
 
@@ -647,6 +648,8 @@ class MongoModuleStore(ModuleStoreBase):
 
         :param xmodule:
         """
+        # Save any changes to the xmodule to the MongoKeyValueStore
+        xmodule.save()
         # split mongo's persist_dag is more general and useful.
         self.collection.save({
                 '_id': xmodule.location.dict(),
@@ -691,6 +694,8 @@ class MongoModuleStore(ModuleStoreBase):
                 'url_slug': new_object.location.name
             })
             course.tabs = existing_tabs
+            # Save any changes to the course to the MongoKeyValueStore
+            course.save()
             self.update_metadata(course.location, course.xblock_kvs._metadata)
 
     def fire_updated_modulestore_signal(self, course_id, location):
@@ -797,6 +802,8 @@ class MongoModuleStore(ModuleStoreBase):
                     tab['name'] = metadata.get('display_name')
                     break
             course.tabs = existing_tabs
+            # Save the updates to the course to the MongoKeyValueStore
+            course.save()
             self.update_metadata(course.location, own_metadata(course))
 
         self._update_single_item(location, {'metadata': metadata})
@@ -819,6 +826,8 @@ class MongoModuleStore(ModuleStoreBase):
             course = self.get_course_for_item(item.location)
             existing_tabs = course.tabs or []
             course.tabs = [tab for tab in existing_tabs if tab.get('url_slug') != location.name]
+            # Save the updates to the course to the MongoKeyValueStore
+            course.save()
             self.update_metadata(course.location, own_metadata(course))
 
         # Must include this to avoid the django debug toolbar (which defines the deprecated "safe=False")

common/lib/xmodule/xmodule/modulestore/tests/django_utils.py

@@ -165,34 +165,31 @@ class ModuleStoreTestCase(TestCase):
         # Call superclass implementation
         super(ModuleStoreTestCase, self)._post_teardown()
 
+
     def assert2XX(self, status_code, msg=None):
         """
         Assert that the given value is a success status (between 200 and 299)
         """
-        if not 200 <= status_code < 300:
-            msg = self._formatMessage(msg, "%s is not a success status" % safe_repr(status_code))
-            raise self.failureExecption(msg)
+        msg = self._formatMessage(msg, "%s is not a success status" % safe_repr(status_code))
+        self.assertTrue(status_code >= 200 and status_code < 300, msg=msg)
 
     def assert3XX(self, status_code, msg=None):
         """
         Assert that the given value is a redirection status (between 300 and 399)
         """
-        if not 300 <= status_code < 400:
-            msg = self._formatMessage(msg, "%s is not a redirection status" % safe_repr(status_code))
-            raise self.failureExecption(msg)
+        msg = self._formatMessage(msg, "%s is not a redirection status" % safe_repr(status_code))
+        self.assertTrue(status_code >= 300 and status_code < 400, msg=msg)
 
     def assert4XX(self, status_code, msg=None):
         """
         Assert that the given value is a client error status (between 400 and 499)
         """
-        if not 400 <= status_code < 500:
-            msg = self._formatMessage(msg, "%s is not a client error status" % safe_repr(status_code))
-            raise self.failureExecption(msg)
+        msg = self._formatMessage(msg, "%s is not a client error status" % safe_repr(status_code))
+        self.assertTrue(status_code >= 400 and status_code < 500, msg=msg)
 
     def assert5XX(self, status_code, msg=None):
         """
         Assert that the given value is a server error status (between 500 and 599)
         """
-        if not 500 <= status_code < 600:
-            msg = self._formatMessage(msg, "%s is not a server error status" % safe_repr(status_code))
-            raise self.failureExecption(msg)
+        msg = self._formatMessage(msg, "%s is not a server error status" % safe_repr(status_code))
+        self.assertTrue(status_code >= 500 and status_code < 600, msg=msg)

common/lib/xmodule/xmodule/modulestore/tests/factories.py

@@ -53,9 +53,6 @@ class XModuleCourseFactory(Factory):
         for k, v in kwargs.iteritems():
             setattr(new_course, k, v)
 
-        # Save the data we've just created before we update mongo datastore
-        new_course.save()
-
         # Update the data in the mongo datastore
         store.save_xmodule(new_course)
         return new_course
@@ -138,7 +135,6 @@ class XModuleItemFactory(Factory):
         # replace the display name with an optional parameter passed in from the caller
         if display_name is not None:
             metadata['display_name'] = display_name
-        # note that location comes from above lazy_attribute
         store.create_and_save_xmodule(location, metadata=metadata, definition_data=data)
 
         if location.category not in DETACHED_CATEGORIES:

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -194,6 +194,10 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem):
             if hasattr(descriptor, 'children'):
                 for child in descriptor.get_children():
                     parent_tracker.add_parent(child.location, descriptor.location)
+
+            # After setting up the descriptor, save any changes that we have
+            # made to attributes on the descriptor to the underlying KeyValueStore.
+            descriptor.save()
             return descriptor
 
         render_template = lambda: ''

common/lib/xmodule/xmodule/tests/test_combined_open_ended.py

@@ -504,11 +504,13 @@ class OpenEndedModuleXmlTest(unittest.TestCase, DummyModulestore):
         See if we can load the module and save an answer
         @return:
         """
-        #Load the module
+        # Load the module
         module = self.get_module_from_location(self.problem_location, COURSE)
 
-        #Try saving an answer
+        # Try saving an answer
         module.handle_ajax("save_answer", {"student_answer": self.answer})
+        # Save our modifications to the underlying KeyValueStore so they can be persisted
+        module.save()
         task_one_json = json.loads(module.task_states[0])
         self.assertEqual(task_one_json['child_history'][0]['answer'], self.answer)
 

common/lib/xmodule/xmodule/tests/test_conditional.py

@@ -217,8 +217,11 @@ class ConditionalModuleXmlTest(unittest.TestCase):
         html = ajax['html']
         self.assertFalse(any(['This is a secret' in item for item in html]))
 
-        # now change state of the capa problem to make it completed
-        inner_get_module(Location('i4x://HarvardX/ER22x/problem/choiceprob')).attempts = 1
+        # Now change state of the capa problem to make it completed
+        inner_module = inner_get_module(Location('i4x://HarvardX/ER22x/problem/choiceprob'))
+        inner_module.attempts = 1
+        # Save our modifications to the underlying KeyValueStore so they can be persisted
+        inner_module.save()
 
         ajax = json.loads(module.handle_ajax('', ''))
         print "post-attempt ajax: ", ajax

lms/djangoapps/courseware/model_data.py

@@ -247,9 +247,10 @@ class ModelDataCache(object):
                 course_id=self.course_id,
                 student=self.user,
                 module_state_key=key.block_scope_id.url(),
-                defaults={'state': json.dumps({}),
-                          'module_type': key.block_scope_id.category,
-                          },
+                defaults={
+                    'state': json.dumps({}),
+                    'module_type': key.block_scope_id.category,
+                },
             )
         elif key.scope == Scope.content:
             field_object, _ = XModuleContentField.objects.get_or_create(
@@ -333,22 +334,10 @@ class LmsKeyValueStore(KeyValueStore):
             return json.loads(field_object.value)
 
     def set(self, key, value):
-        if key.field_name in self._descriptor_model_data:
-            raise InvalidWriteError("Not allowed to overwrite descriptor model data", key.field_name)
-
-        field_object = self._model_data_cache.find_or_create(key)
-
-        if key.scope not in self._allowed_scopes:
-            raise InvalidScopeError(key.scope)
-
-        if key.scope == Scope.user_state:
-            state = json.loads(field_object.state)
-            state[key.field_name] = value
-            field_object.state = json.dumps(state)
-        else:
-            field_object.value = json.dumps(value)
-
-        field_object.save()
+        """
+        Set a single value in the KeyValueStore
+        """
+        self.set_many({key: value})
 
     def set_many(self, kv_dict):
         """
@@ -362,23 +351,21 @@ class LmsKeyValueStore(KeyValueStore):
         # field_objects maps a field_object to a list of associated fields
         field_objects = dict()
         for field in kv_dict:
-            # check field for validity
+            # Check field for validity
             if field.field_name in self._descriptor_model_data:
                 raise InvalidWriteError("Not allowed to overwrite descriptor model data", field.field_name)
 
             if field.scope not in self._allowed_scopes:
                 raise InvalidScopeError(field.scope)
 
-            # if the field is valid
+            # If the field is valid and isn't already in the dictionary, add it.
             field_object = self._model_data_cache.find_or_create(field)
-            # if this field_object isn't already in the dictionary
-            # add it
             if field_object not in field_objects.keys():
                 field_objects[field_object] = []
-            # update the list of associated fields
+            # Update the list of associated fields
             field_objects[field_object].append(field)
 
-            # special case when scope is for the user state
+            # Special case when scope is for the user state, because this scope saves fields in a single row
             if field.scope == Scope.user_state:
                 state = json.loads(field_object.state)
                 state[field.field_name] = kv_dict[field]

lms/djangoapps/courseware/module_render.py

@@ -27,7 +27,7 @@ from xmodule.modulestore import Location
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.x_module import ModuleSystem
-from xmodule_modifiers import replace_course_urls, replace_static_urls, add_histogram, wrap_xmodule
+from xmodule_modifiers import replace_course_urls, replace_static_urls, add_histogram, wrap_xmodule, save_module  # pylint: disable=F0401
 
 import static_replace
 from psychometrics.psychoanalyze import make_psychometrics_data_update_handler
@@ -36,6 +36,8 @@ from student.models import unique_id_for_user
 from courseware.access import has_access
 from courseware.masquerade import setup_masquerade
 from courseware.model_data import LmsKeyValueStore, LmsUsage, ModelDataCache
+from xblock.runtime import KeyValueStore
+from xblock.core import Scope
 from courseware.models import StudentModule
 from util.sandboxing import can_execute_unsafe_code
 from util.json_request import JsonResponse
@@ -226,7 +228,7 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours
                                                            userid=str(user.id),
                                                            mod_id=descriptor.location.url(),
                                                            dispatch=dispatch),
-                                       )
+                                               )
         return xqueue_callback_url_prefix + relative_xqueue_callback_url
 
     # Default queuename is course-specific and is derived from the course that
@@ -234,11 +236,12 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours
     # TODO: Queuename should be derived from 'course_settings.json' of each course
     xqueue_default_queuename = descriptor.location.org + '-' + descriptor.location.course
 
-    xqueue = {'interface': xqueue_interface,
-              'construct_callback': make_xqueue_callback,
-              'default_queuename': xqueue_default_queuename.replace(' ', '_'),
-              'waittime': settings.XQUEUE_WAITTIME_BETWEEN_REQUESTS
-             }
+    xqueue = {
+        'interface': xqueue_interface,
+        'construct_callback': make_xqueue_callback,
+        'default_queuename': xqueue_default_queuename.replace(' ', '_'),
+        'waittime': settings.XQUEUE_WAITTIME_BETWEEN_REQUESTS
+    }
 
     # This is a hacky way to pass settings to the combined open ended xmodule
     # It needs an S3 interface to upload images to S3
@@ -286,18 +289,24 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours
         )
 
     def publish(event):
+        """A function that allows XModules to publish events. This only supports grade changes right now."""
         if event.get('event_name') != 'grade':
             return
 
-        student_module, created = StudentModule.objects.get_or_create(
-            course_id=course_id,
-            student=user,
-            module_type=descriptor.location.category,
-            module_state_key=descriptor.location.url(),
-            defaults={'state': '{}'},
+        usage = LmsUsage(descriptor.location, descriptor.location)
+        # Construct the key for the module
+        key = KeyValueStore.Key(
+            scope=Scope.user_state,
+            student_id=user.id,
+            block_scope_id=usage.id,
+            field_name='grade'
         )
+
+        student_module = model_data_cache.find_or_create(key)
+        # Update the grades
         student_module.grade = event.get('value')
         student_module.max_grade = event.get('max_value')
+        # Save all changes to the underlying KeyValueStore
         student_module.save()
 
         # Bin score into range and increment stats
@@ -388,9 +397,31 @@ def get_module_for_descriptor_internal(user, descriptor, model_data_cache, cours
         if has_access(user, module, 'staff', course_id):
             module.get_html = add_histogram(module.get_html, module, user)
 
+    # force the module to save after rendering
+    module.get_html = save_module(module.get_html, module)
     return module
 
 
+def find_target_student_module(request, user_id, course_id, mod_id):
+    """
+    Retrieve target StudentModule
+    """
+    user = User.objects.get(id=user_id)
+    model_data_cache = ModelDataCache.cache_for_descriptor_descendents(
+        course_id,
+        user,
+        modulestore().get_instance(course_id, mod_id),
+        depth=0,
+        select_for_update=True
+    )
+    instance = get_module(user, request, mod_id, model_data_cache, course_id, grade_bucket_type='xqueue')
+    if instance is None:
+        msg = "No module {0} for user {1}--access denied?".format(mod_id, user)
+        log.debug(msg)
+        raise Http404
+    return instance
+
+
 @csrf_exempt
 def xqueue_callback(request, course_id, userid, mod_id, dispatch):
     '''
@@ -409,20 +440,7 @@ def xqueue_callback(request, course_id, userid, mod_id, dispatch):
     if not isinstance(header, dict) or 'lms_key' not in header:
         raise Http404
 
-    # Retrieve target StudentModule
-    user = User.objects.get(id=userid)
-    model_data_cache = ModelDataCache.cache_for_descriptor_descendents(
-        course_id,
-        user,
-        modulestore().get_instance(course_id, mod_id),
-        depth=0,
-        select_for_update=True
-    )
-    instance = get_module(user, request, mod_id, model_data_cache, course_id, grade_bucket_type='xqueue')
-    if instance is None:
-        msg = "No module {0} for user {1}--access denied?".format(mod_id, user)
-        log.debug(msg)
-        raise Http404
+    instance = find_target_student_module(request, userid, course_id, mod_id)
 
     # Transfer 'queuekey' from xqueue response header to the data.
     # This is required to use the interface defined by 'handle_ajax'
@@ -433,6 +451,8 @@ def xqueue_callback(request, course_id, userid, mod_id, dispatch):
     try:
         # Can ignore the return value--not used for xqueue_callback
         instance.handle_ajax(dispatch, data)
+        # Save any state that has changed to the underlying KeyValueStore
+        instance.save()
     except:
         log.exception("error processing ajax call")
         raise
@@ -504,6 +524,8 @@ def modx_dispatch(request, dispatch, location, course_id):
     # Let the module handle the AJAX
     try:
         ajax_return = instance.handle_ajax(dispatch, data)
+        # Save any fields that have changed to the underlying KeyValueStore
+        instance.save()
 
     # If we can't find the module, respond with a 404
     except NotFoundError:

lms/djangoapps/courseware/tests/test_model_data.py

+"""
+Test for lms courseware app, module data (runtime data storage for XBlocks)
+"""
 import json
 from mock import Mock, patch
 from functools import partial
@@ -68,12 +71,17 @@ class TestDescriptorFallback(TestCase):
         self.assertRaises(InvalidWriteError, self.kvs.set, settings_key('field_b'), 'foo')
         self.assertEquals('settings', self.desc_md['field_b'])
 
+        self.assertRaises(InvalidWriteError, self.kvs.set_many, {content_key('field_a'): 'foo'})
+        self.assertEquals('content', self.desc_md['field_a'])
+
         self.assertRaises(InvalidWriteError, self.kvs.delete, content_key('field_a'))
         self.assertEquals('content', self.desc_md['field_a'])
         self.assertRaises(InvalidWriteError, self.kvs.delete, settings_key('field_b'))
         self.assertEquals('settings', self.desc_md['field_b'])
 
 
+
+
 class TestInvalidScopes(TestCase):
     def setUp(self):
         self.desc_md = {}
@@ -85,10 +93,13 @@ class TestInvalidScopes(TestCase):
         for scope in (Scope(user=True, block=BlockScope.DEFINITION),
                       Scope(user=False, block=BlockScope.TYPE),
                       Scope(user=False, block=BlockScope.ALL)):
-            self.assertRaises(InvalidScopeError, self.kvs.get, LmsKeyValueStore.Key(scope, None, None, 'field'))
-            self.assertRaises(InvalidScopeError, self.kvs.set, LmsKeyValueStore.Key(scope, None, None, 'field'), 'value')
-            self.assertRaises(InvalidScopeError, self.kvs.delete, LmsKeyValueStore.Key(scope, None, None, 'field'))
-            self.assertRaises(InvalidScopeError, self.kvs.has, LmsKeyValueStore.Key(scope, None, None, 'field'))
+            key = LmsKeyValueStore.Key(scope, None, None, 'field')
+
+            self.assertRaises(InvalidScopeError, self.kvs.get, key)
+            self.assertRaises(InvalidScopeError, self.kvs.set, key, 'value')
+            self.assertRaises(InvalidScopeError, self.kvs.delete, key)
+            self.assertRaises(InvalidScopeError, self.kvs.has, key)
+            self.assertRaises(InvalidScopeError, self.kvs.set_many, {key: 'value'})
 
 
 class TestStudentModuleStorage(TestCase):
@@ -141,7 +152,7 @@ class TestStudentModuleStorage(TestCase):
         self.assertFalse(self.kvs.has(user_state_key('not_a_field')))
 
     def construct_kv_dict(self):
-        """ construct a kv_dict that can be passed to set_many """
+        """Construct a kv_dict that can be passed to set_many"""
         key1 = user_state_key('field_a')
         key2 = user_state_key('field_b')
         new_value = 'new value'
@@ -149,7 +160,7 @@ class TestStudentModuleStorage(TestCase):
         return {key1: new_value, key2: newer_value}
 
     def test_set_many(self):
-        """Test setting many fields that are scoped to Scope.user_state """
+        "Test setting many fields that are scoped to Scope.user_state"
         kv_dict = self.construct_kv_dict()
         self.kvs.set_many(kv_dict)
 
@@ -157,7 +168,7 @@ class TestStudentModuleStorage(TestCase):
             self.assertEquals(self.kvs.get(key), kv_dict[key])
 
     def test_set_many_failure(self):
-        """Test failures when setting many fields that are scoped to Scope.user_state """
+        "Test failures when setting many fields that are scoped to Scope.user_state"
         kv_dict = self.construct_kv_dict()
         # because we're patching the underlying save, we need to ensure the
         # fields are in the cache
@@ -211,6 +222,10 @@ class StorageTestBase(object):
     A base class for that gets subclassed when testing each of the scopes.
 
     """
+    # Disable pylint warnings that arise because of the way the child classes call
+    # this base class -- pylint's static analysis can't keep up with it.
+    # pylint: disable=E1101, E1102
+
     factory = None
     scope = None
     key_factory = None
@@ -273,6 +288,7 @@ class StorageTestBase(object):
         self.assertFalse(self.kvs.has(self.key_factory('missing_field')))
 
     def construct_kv_dict(self):
+        """Construct a kv_dict that can be passed to set_many"""
         key1 = self.key_factory('existing_field')
         key2 = self.key_factory('other_existing_field')
         new_value = 'new value'

lms/djangoapps/courseware/tests/test_module_render.py

-from mock import MagicMock
+"""
+Test for lms courseware app, module render unit
+"""
+from mock import MagicMock, patch
 import json
 
 from django.http import Http404, HttpResponse
@@ -28,6 +31,20 @@ class ModuleRenderTestCase(LoginEnrollmentTestCase):
         self.location = ['i4x', 'edX', 'toy', 'chapter', 'Overview']
         self.course_id = 'edX/toy/2012_Fall'
         self.toy_course = modulestore().get_course(self.course_id)
+        self.mock_user = UserFactory()
+        self.mock_user.id = 1
+        self.request_factory = RequestFactory()
+
+        # Construct a mock module for the modulestore to return
+        self.mock_module = MagicMock()
+        self.mock_module.id = 1
+        self.dispatch = 'score_update'
+
+        # Construct a 'standard' xqueue_callback url
+        self.callback_url = reverse('xqueue_callback', kwargs=dict(course_id=self.course_id,
+                                                                   userid=str(self.mock_user.id),
+                                                                   mod_id=self.mock_module.id,
+                                                                   dispatch=self.dispatch))
 
     def test_get_module(self):
         self.assertIsNone(render.get_module('dummyuser', None,
@@ -56,7 +73,7 @@ class ModuleRenderTestCase(LoginEnrollmentTestCase):
         mock_request_3 = MagicMock()
         mock_request_3.POST.copy.return_value = {'position': 1}
         mock_request_3.FILES = False
-        mock_request_3.user = UserFactory()
+        mock_request_3.user = self.mock_user
         inputfile_2 = Stub()
         inputfile_2.size = 1
         inputfile_2.name = 'name'
@@ -87,6 +104,46 @@ class ModuleRenderTestCase(LoginEnrollmentTestCase):
             self.course_id
         )
 
+    def test_xqueue_callback_success(self):
+        """
+        Test for happy-path xqueue_callback
+        """
+        fake_key = 'fake key'
+        xqueue_header = json.dumps({'lms_key': fake_key})
+        data = {
+            'xqueue_header': xqueue_header,
+            'xqueue_body': 'hello world',
+        }
+
+        # Patch getmodule to return our mock module
+        with patch('courseware.module_render.find_target_student_module') as get_fake_module:
+            get_fake_module.return_value = self.mock_module
+            # call xqueue_callback with our mocked information
+            request = self.request_factory.post(self.callback_url, data)
+            render.xqueue_callback(request, self.course_id, self.mock_user.id, self.mock_module.id, self.dispatch)
+
+        # Verify that handle ajax is called with the correct data
+        request.POST['queuekey'] = fake_key
+        self.mock_module.handle_ajax.assert_called_once_with(self.dispatch, request.POST)
+
+    def test_xqueue_callback_missing_header_info(self):
+        data = {
+            'xqueue_header': '{}',
+            'xqueue_body': 'hello world',
+        }
+
+        with patch('courseware.module_render.find_target_student_module') as get_fake_module:
+            get_fake_module.return_value = self.mock_module
+            # Test with missing xqueue data
+            with self.assertRaises(Http404):
+                request = self.request_factory.post(self.callback_url, {})
+                render.xqueue_callback(request, self.course_id, self.mock_user.id, self.mock_module.id, self.dispatch)
+
+            # Test with missing xqueue_header
+            with self.assertRaises(Http404):
+                request = self.request_factory.post(self.callback_url, data)
+                render.xqueue_callback(request, self.course_id, self.mock_user.id, self.mock_module.id, self.dispatch)
+
     def test_get_score_bucket(self):
         self.assertEquals(render.get_score_bucket(0, 10), 'incorrect')
         self.assertEquals(render.get_score_bucket(1, 10), 'partial')

lms/djangoapps/courseware/views.py

@@ -167,6 +167,8 @@ def save_child_position(seq_module, child_name):
             # Only save if position changed
             if position != seq_module.position:
                 seq_module.position = position
+    # Save this new position to the underlying KeyValueStore
+    seq_module.save()
 
 
 def check_for_active_timelimit_module(request, course_id, course):

requirements/edx/github.txt

@@ -8,6 +8,6 @@
 -e git://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk
 
 # Our libraries:
--e git+https://github.com/edx/XBlock.git@0b71db6ee6f9b216d0dd85cd76b55f2354b93dff#egg=XBlock
+-e git+https://github.com/edx/XBlock.git@3974e999fe853a37dfa6fadf0611289434349409#egg=XBlock
 -e git+https://github.com/edx/codejail.git@0a1b468#egg=codejail
 -e git+https://github.com/edx/diff-cover.git@v0.1.3#egg=diff_cover