Merge pull request #416 from edx/sarina/xblock-bulk-save-interface

Add XBlock bulk saves to LMS/CMS

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,23 +189,12 @@ 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']
+            # 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 descriptor.grade_cutoffs
-
     @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

@@ -105,6 +105,15 @@ class MongoKeyValueStore(KeyValueStore):
         else:
             raise InvalidScopeError(key.scope)
 
+    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."""
+        # `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)
+
     def delete(self, key):
         if key.scope == Scope.children:
             self._children = []
@@ -639,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(),
@@ -683,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):
@@ -789,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})
@@ -811,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)
+        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)
+        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)
+        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)
+        self.assertTrue(status_code >= 500 and status_code < 600, msg=msg)

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

@@ -135,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

@@ -12,9 +12,14 @@ from .models import (
     XModuleStudentPrefsField,
     XModuleStudentInfoField
 )
+import logging
+
+from django.db import DatabaseError
 
 from xblock.runtime import KeyValueStore, InvalidScopeError
-from xblock.core import Scope
+from xblock.core import KeyValueMultiSaveError, Scope
+
+log = logging.getLogger(__name__)
 
 
 class InvalidWriteError(Exception):
@@ -242,7 +247,8 @@ class ModelDataCache(object):
                 course_id=self.course_id,
                 student=self.user,
                 module_state_key=key.block_scope_id.url(),
-                defaults={'state': json.dumps({}),
+                defaults={
+                    'state': json.dumps({}),
                     'module_type': key.block_scope_id.category,
                 },
             )
@@ -328,22 +334,57 @@ 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)
+        """
+        Set a single value in the KeyValueStore
+        """
+        self.set_many({key: value})
 
-        field_object = self._model_data_cache.find_or_create(key)
+    def set_many(self, kv_dict):
+        """
+        Provide a bulk save mechanism.
 
-        if key.scope not in self._allowed_scopes:
-            raise InvalidScopeError(key.scope)
+        `kv_dict`: A dictionary of dirty fields that maps
+          xblock.DbModel._key : value
 
-        if key.scope == Scope.user_state:
+        """
+        saved_fields = []
+        # field_objects maps a field_object to a list of associated fields
+        field_objects = dict()
+        for field in kv_dict:
+            # 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 and isn't already in the dictionary, add it.
+            field_object = self._model_data_cache.find_or_create(field)
+            if field_object not in field_objects.keys():
+                field_objects[field_object] = []
+            # Update the list of associated fields
+            field_objects[field_object].append(field)
+
+            # 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[key.field_name] = value
+                state[field.field_name] = kv_dict[field]
                 field_object.state = json.dumps(state)
             else:
-            field_object.value = json.dumps(value)
+            # The remaining scopes save fields on different rows, so
+            # we don't have to worry about conflicts
+                field_object.value = json.dumps(kv_dict[field])
 
+        for field_object in field_objects:
+            try:
+                # Save the field object that we made above
                 field_object.save()
+                # If save is successful on this scope, add the saved fields to
+                # the list of successful saves
+                saved_fields.extend([field.field_name for field in field_objects[field_object]])
+            except DatabaseError:
+                log.error('Error saving fields %r', field_objects[field_object])
+                raise KeyValueMultiSaveError(saved_fields)
 
     def delete(self, key):
         if key.field_name in self._descriptor_model_data:

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
@@ -234,7 +236,8 @@ 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,
+    xqueue = {
+        'interface': xqueue_interface,
         'construct_callback': make_xqueue_callback,
         'default_queuename': xqueue_default_queuename.replace(' ', '_'),
         'waittime': settings.XQUEUE_WAITTIME_BETWEEN_REQUESTS
@@ -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
+from mock import Mock, patch
 from functools import partial
 
 from courseware.model_data import LmsKeyValueStore, InvalidWriteError
@@ -15,6 +18,8 @@ from courseware.tests.factories import StudentPrefsFactory, StudentInfoFactory
 from xblock.core import Scope, BlockScope
 from xmodule.modulestore import Location
 from django.test import TestCase
+from django.db import DatabaseError
+from xblock.core import KeyValueMultiSaveError
 
 
 def mock_field(scope, name):
@@ -66,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 = {}
@@ -83,17 +93,20 @@ 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):
 
     def setUp(self):
         self.desc_md = {}
-        student_module = StudentModuleFactory(state=json.dumps({'a_field': 'a_value'}))
+        student_module = StudentModuleFactory(state=json.dumps({'a_field': 'a_value', 'b_field': 'b_value'}))
         self.user = student_module.student
         self.mdc = ModelDataCache([mock_descriptor([mock_field(Scope.user_state, 'a_field')])], course_id, self.user)
         self.kvs = LmsKeyValueStore(self.desc_md, self.mdc)
@@ -110,13 +123,13 @@ class TestStudentModuleStorage(TestCase):
         "Test that setting an existing user_state field changes the value"
         self.kvs.set(user_state_key('a_field'), 'new_value')
         self.assertEquals(1, StudentModule.objects.all().count())
-        self.assertEquals({'a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state))
+        self.assertEquals({'b_field': 'b_value', 'a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state))
 
     def test_set_missing_field(self):
         "Test that setting a new user_state field changes the value"
         self.kvs.set(user_state_key('not_a_field'), 'new_value')
         self.assertEquals(1, StudentModule.objects.all().count())
-        self.assertEquals({'a_field': 'a_value', 'not_a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state))
+        self.assertEquals({'b_field': 'b_value', 'a_field': 'a_value', 'not_a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state))
 
     def test_delete_existing_field(self):
         "Test that deleting an existing field removes it from the StudentModule"
@@ -128,7 +141,7 @@ class TestStudentModuleStorage(TestCase):
         "Test that deleting a missing field from an existing StudentModule raises a KeyError"
         self.assertRaises(KeyError, self.kvs.delete, user_state_key('not_a_field'))
         self.assertEquals(1, StudentModule.objects.all().count())
-        self.assertEquals({'a_field': 'a_value'}, json.loads(StudentModule.objects.all()[0].state))
+        self.assertEquals({'b_field': 'b_value', 'a_field': 'a_value'}, json.loads(StudentModule.objects.all()[0].state))
 
     def test_has_existing_field(self):
         "Test that `has` returns True for existing fields in StudentModules"
@@ -138,6 +151,35 @@ class TestStudentModuleStorage(TestCase):
         "Test that `has` returns False for missing fields in StudentModule"
         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"""
+        key1 = user_state_key('field_a')
+        key2 = user_state_key('field_b')
+        new_value = 'new value'
+        newer_value = 'newer value'
+        return {key1: new_value, key2: newer_value}
+
+    def test_set_many(self):
+        "Test setting many fields that are scoped to Scope.user_state"
+        kv_dict = self.construct_kv_dict()
+        self.kvs.set_many(kv_dict)
+
+        for key in kv_dict:
+            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"
+        kv_dict = self.construct_kv_dict()
+        # because we're patching the underlying save, we need to ensure the
+        # fields are in the cache
+        for key in kv_dict:
+            self.kvs.set(key, 'test_value')
+
+        with patch('django.db.models.Model.save', side_effect=DatabaseError):
+            with self.assertRaises(KeyValueMultiSaveError) as exception_context:
+                self.kvs.set_many(kv_dict)
+        self.assertEquals(len(exception_context.exception.saved_field_names), 0)
+
 
 class TestMissingStudentModule(TestCase):
     def setUp(self):
@@ -176,6 +218,14 @@ class TestMissingStudentModule(TestCase):
 
 
 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
@@ -188,7 +238,10 @@ class StorageTestBase(object):
         else:
             self.user = UserFactory.create()
         self.desc_md = {}
-        self.mdc = ModelDataCache([mock_descriptor([mock_field(self.scope, 'existing_field')])], course_id, self.user)
+        self.mock_descriptor = mock_descriptor([
+            mock_field(self.scope, 'existing_field'),
+            mock_field(self.scope, 'other_existing_field')])
+        self.mdc = ModelDataCache([self.mock_descriptor], course_id, self.user)
         self.kvs = LmsKeyValueStore(self.desc_md, self.mdc)
 
     def test_set_and_get_existing_field(self):
@@ -234,6 +287,38 @@ class StorageTestBase(object):
         "Test that `has` return False for an existing Storage Field"
         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'
+        newer_value = 'newer value'
+        return {key1: new_value, key2: newer_value}
+
+    def test_set_many(self):
+        """Test that setting many regular fields at the same time works"""
+        kv_dict = self.construct_kv_dict()
+
+        self.kvs.set_many(kv_dict)
+        for key in kv_dict:
+            self.assertEquals(self.kvs.get(key), kv_dict[key])
+
+    def test_set_many_failure(self):
+        """Test that setting many regular fields with a DB error """
+        kv_dict = self.construct_kv_dict()
+        for key in kv_dict:
+            self.kvs.set(key, 'test value')
+
+        with patch('django.db.models.Model.save', side_effect=[None, DatabaseError]):
+            with self.assertRaises(KeyValueMultiSaveError) as exception_context:
+                self.kvs.set_many(kv_dict)
+
+        exception = exception_context.exception
+        self.assertEquals(len(exception.saved_field_names), 1)
+        self.assertEquals(exception.saved_field_names[0], 'existing_field')
+
+
+
 
 class TestSettingsStorage(StorageTestBase, TestCase):
     factory = SettingsFactory

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@4d8735e883#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