Fix cached definition change detection

LMS-11485

cms/djangoapps/contentstore/tests/test_course_settings.py

@@ -20,11 +20,14 @@ from xmodule.fields import Date
 from .utils import CourseTestCase
 from xmodule.modulestore.django import modulestore
 from contentstore.views.component import ADVANCED_COMPONENT_POLICY_KEY
+import ddt
+from xmodule.modulestore import ModuleStoreEnum
 
 
 def get_url(course_id, handler_name='settings_handler'):
     return reverse_course_url(handler_name, course_id)
 
+
 class CourseDetailsTestCase(CourseTestCase):
     """
     Tests the first course settings page (course dates, overview, etc.).
@@ -139,7 +142,6 @@ class CourseDetailsTestCase(CourseTestCase):
             response = self.client.get_html(settings_details_url)
             self.assertNotContains(response, "Course Short Description")
 
-
     def test_regular_site_fetch(self):
         settings_details_url = get_url(self.course.id)
 
@@ -240,6 +242,7 @@ class CourseDetailsViewTest(CourseTestCase):
             self.fail(field + " included in encoding but missing from details at " + context)
 
 
+@ddt.ddt
 class CourseGradingTest(CourseTestCase):
     """
     Tests for the course settings grading page.
@@ -258,7 +261,10 @@ class CourseGradingTest(CourseTestCase):
             subgrader = CourseGradingModel.fetch_grader(self.course.id, i)
             self.assertDictEqual(grader, subgrader, str(i) + "th graders not equal")
 
-    def test_update_from_json(self):
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_update_from_json(self, store):
+        self.course = CourseFactory.create(default_store=store)
+
         test_grader = CourseGradingModel.fetch(self.course.id)
         altered_grader = CourseGradingModel.update_from_json(self.course.id, test_grader.__dict__, self.user)
         self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "Noop update")
@@ -267,6 +273,18 @@ class CourseGradingTest(CourseTestCase):
         altered_grader = CourseGradingModel.update_from_json(self.course.id, test_grader.__dict__, self.user)
         self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "Weight[0] * 2")
 
+        # test for bug LMS-11485
+        with modulestore().bulk_operations(self.course.id):
+            new_grader = test_grader.graders[0].copy()
+            new_grader['type'] += '_foo'
+            new_grader['short_label'] += '_foo'
+            new_grader['id'] = len(test_grader.graders)
+            test_grader.graders.append(new_grader)
+            # don't use altered cached def, get a fresh one
+            CourseGradingModel.update_from_json(self.course.id, test_grader.__dict__, self.user)
+            altered_grader = CourseGradingModel.fetch(self.course.id)
+            self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__)
+
         test_grader.grade_cutoffs['D'] = 0.3
         altered_grader = CourseGradingModel.update_from_json(self.course.id, test_grader.__dict__, self.user)
         self.assertDictEqual(test_grader.__dict__, altered_grader.__dict__, "cutoff add D")

cms/djangoapps/models/settings/course_grading.py

 from datetime import timedelta
 from xmodule.modulestore.django import modulestore
-from xblock.fields import Scope
 
 
 class CourseGradingModel(object):

common/lib/xmodule/xmodule/course_module.py

@@ -24,6 +24,7 @@ _ = lambda text: text
 
 DEFAULT_START_DATE = datetime(2030, 1, 1, tzinfo=UTC())
 
+
 class StringOrDate(Date):
     def from_json(self, value):
         """

common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py

 from opaque_keys.edx.locator import DefinitionLocator
+import copy
 
 
 class DefinitionLazyLoader(object):
@@ -24,4 +25,8 @@ class DefinitionLazyLoader(object):
         Fetch the definition. Note, the caller should replace this lazy
         loader pointer with the result so as not to fetch more than once
         """
-        return self.modulestore.get_definition(self.course_key, self.definition_locator.definition_id)
+        # get_definition may return a cached value perhaps from another course or code path
+        # so, we copy the result here so that updates don't cross-pollinate nor change the cached
+        # value in such a way that we can't tell that the definition's been updated.
+        definition = self.modulestore.get_definition(self.course_key, self.definition_locator.definition_id)
+        return copy.deepcopy(definition)

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -361,7 +361,6 @@ class SplitBulkWriteMixin(BulkOperationsMixin):
         definitions.extend(self.db_connection.get_definitions(list(ids)))
         return definitions
 
-
     def update_definition(self, course_key, definition):
         """
         Update a definition, respecting the current bulk operation status
@@ -1892,7 +1891,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                     # find the parents and put root in the right sequence
                     parent = self._get_parent_from_structure(BlockKey.from_usage_key(subtree_root), source_structure)
                     if parent is not None:  # may be a detached category xblock
-                        if not parent in destination_blocks:
+                        if parent not in destination_blocks:
                             raise ItemNotFoundError(parent)
                         orphans.update(
                             self._sync_children(

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

@@ -63,10 +63,17 @@ class CourseFactory(XModuleFactory):
         run = kwargs.get('run', name)
         user_id = kwargs.pop('user_id', ModuleStoreEnum.UserID.test)
 
+        # Pass the metadata just as field=value pairs
+        kwargs.update(kwargs.pop('metadata', {}))
+        default_store_override = kwargs.pop('default_store', None)
+
         with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred):
-            # Write the data to the mongo datastore
-            kwargs.update(kwargs.get('metadata', {}))
+            if default_store_override is not None:
+                with store.default_store(default_store_override):
+                    new_course = store.create_course(org, number, run, user_id, fields=kwargs)
+            else:
                 new_course = store.create_course(org, number, run, user_id, fields=kwargs)
+
             last_course.loc = new_course.location
             return new_course