Merge pull request #5109 from edx/split/inherited_settings

Cache inherited settings outside of structure

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

@@ -30,7 +30,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         modulestore: the module store that can be used to retrieve additional
         modules
 
-        course_entry: the originally fetched enveloped course_structure w/ branch and course id info.
+        course_entry: the originally fetched enveloped course_structure w/ branch and course id info
+        plus a dictionary of cached inherited_settings indexed by (block_type, block_id) tuple.
         Callers to _load_item provide an override but that function ignores the provided structure and
         only looks at the branch and course id
 
@@ -47,7 +48,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         super(CachingDescriptorSystem, self).__init__(
             field_data=None,
             load_item=self._load_item,
-            resources_fs = OSFS(root),
+            resources_fs=OSFS(root),
             **kwargs
         )
         self.modulestore = modulestore
@@ -57,9 +58,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         # Compute inheritance
         modulestore.inherit_settings(
             course_entry['structure'].get('blocks', {}),
-            course_entry['structure'].get('blocks', {}).get(
-                encode_key_for_mongo(course_entry['structure'].get('root'))
-            )
+            encode_key_for_mongo(course_entry['structure'].get('root')),
+            course_entry.setdefault('inherited_settings', {}),
         )
         self.default_class = default_class
         self.local_modules = {}
@@ -93,7 +93,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         json_data = self.get_module_data(block_id, course_key)
 
         class_ = self.load_block_type(json_data.get('category'))
-        new_item = self.xblock_from_json(class_, course_key, block_id, json_data, course_entry_override, **kwargs)
+        # pass None for inherited_settings to signal that it should get the settings from cache
+        new_item = self.xblock_from_json(class_, course_key, block_id, json_data, None, course_entry_override, **kwargs)
         return new_item
 
     def get_module_data(self, block_id, course_key):
@@ -124,7 +125,9 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
     # low; thus, the course_entry is most likely correct. If the thread is looking at > 1 named container
     # pointing to the same structure, the access is likely to be chunky enough that the last known container
     # is the intended one when not given a course_entry_override; thus, the caching of the last branch/course id.
-    def xblock_from_json(self, class_, course_key, block_id, json_data, course_entry_override=None, **kwargs):
+    def xblock_from_json(
+        self, class_, course_key, block_id, json_data, inherited_settings, course_entry_override=None, **kwargs
+    ):
         if course_entry_override is None:
             course_entry_override = self.course_entry
         else:
@@ -136,6 +139,13 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
 
         definition_id = json_data.get('definition')
         block_type = json_data['category']
+        if block_id is not None:
+            if inherited_settings is None:
+                # see if there's a value in course_entry
+                if (block_type, block_id) in self.course_entry['inherited_settings']:
+                    inherited_settings = self.course_entry['inherited_settings'][(block_type, block_id)]
+            elif (block_type, block_id) not in self.course_entry['inherited_settings']:
+                self.course_entry['inherited_settings'][(block_type, block_id)] = inherited_settings
 
         if definition_id is not None and not json_data.get('definition_loaded', False):
             definition_loader = DefinitionLazyLoader(
@@ -168,7 +178,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
         kvs = SplitMongoKVS(
             definition_loader,
             converted_fields,
-            json_data.get('_inherited_settings'),
+            inherited_settings,
             **kwargs
         )
         field_data = KvsFieldData(kvs)

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

@@ -57,7 +57,6 @@ from path import path
 import copy
 from pytz import UTC
 from bson.objectid import ObjectId
-from pymongo.errors import DuplicateKeyError
 
 from xblock.core import XBlock
 from xblock.fields import Scope, Reference, ReferenceList, ReferenceValueDict
@@ -1628,15 +1627,20 @@ class SplitMongoModuleStore(BulkWriteMixin, ModuleStoreWriteBase):
         }
         if definition_id is not None:
             json_data['definition'] = definition_id
-        if parent_xblock is not None:
-            json_data['_inherited_settings'] = parent_xblock.xblock_kvs.inherited_settings.copy()
+        if parent_xblock is None:
+            # If no parent, then nothing to inherit.
+            inherited_settings = {}
+        else:
+            inherited_settings = parent_xblock.xblock_kvs.inherited_settings.copy()
             if fields is not None:
                 for field_name in inheritance.InheritanceMixin.fields:
                     if field_name in fields:
-                        json_data['_inherited_settings'][field_name] = fields[field_name]
+                        inherited_settings[field_name] = fields[field_name]
 
-        new_block = runtime.xblock_from_json(xblock_class, course_key, block_id, json_data, **kwargs)
-        for field_name, value in fields.iteritems():
+        new_block = runtime.xblock_from_json(
+            xblock_class, course_key, block_id, json_data, inherited_settings, **kwargs
+        )
+        for field_name, value in (fields or {}).iteritems():
             setattr(new_block, field_name, value)
 
         if parent_xblock is not None:
@@ -1936,12 +1940,14 @@ class SplitMongoModuleStore(BulkWriteMixin, ModuleStoreWriteBase):
         # in case the course is later restored.
         # super(SplitMongoModuleStore, self).delete_course(course_key, user_id)
 
-    def inherit_settings(self, block_map, block_json, inheriting_settings=None):
+    def inherit_settings(self, block_map, block_id, inherited_settings_map, inheriting_settings=None):
         """
         Updates block_json with any inheritable setting set by an ancestor and recurses to children.
         """
-        if block_json is None:
+        encoded_key = encode_key_for_mongo(block_id)
+        if encoded_key not in block_map:
             return
+        block_json = block_map[encoded_key]
 
         if inheriting_settings is None:
             inheriting_settings = {}
@@ -1949,11 +1955,10 @@ class SplitMongoModuleStore(BulkWriteMixin, ModuleStoreWriteBase):
         # the currently passed down values take precedence over any previously cached ones
         # NOTE: this should show the values which all fields would have if inherited: i.e.,
         # not set to the locally defined value but to value set by nearest ancestor who sets it
-        # ALSO NOTE: no xblock should ever define a _inherited_settings field as it will collide w/ this logic.
-        block_json.setdefault('_inherited_settings', {}).update(inheriting_settings)
+        inherited_settings_map.setdefault((block_json['category'], block_id), {}).update(inheriting_settings)
 
         # update the inheriting w/ what should pass to children
-        inheriting_settings = block_json['_inherited_settings'].copy()
+        inheriting_settings = inherited_settings_map[(block_json['category'], block_id)].copy()
         block_fields = block_json['fields']
         for field_name in inheritance.InheritanceMixin.fields:
             if field_name in block_fields:
@@ -1962,7 +1967,7 @@ class SplitMongoModuleStore(BulkWriteMixin, ModuleStoreWriteBase):
         for child in block_fields.get('children', []):
             try:
                 child = encode_key_for_mongo(child)
-                self.inherit_settings(block_map, block_map[child], inheriting_settings)
+                self.inherit_settings(block_map, child, inherited_settings_map, inheriting_settings)
             except KeyError:
                 # here's where we need logic for looking up in other structures when we allow cross pointers
                 # but it's also getting this during course creation if creating top down w/ children set or

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

@@ -1564,6 +1564,56 @@ class TestInheritance(SplitModuleTest):
         # overridden
         self.assertEqual(node.graceperiod, datetime.timedelta(hours=4))
 
+    def test_inheritance_not_saved(self):
+        """
+        Was saving inherited settings with updated blocks causing inheritance to be sticky
+        """
+        # set on parent, retrieve child, verify setting
+        chapter = modulestore().get_item(
+            BlockUsageLocator(
+                CourseLocator(org='testx', course='GreekHero', run="run", branch=BRANCH_NAME_DRAFT), 'chapter', 'chapter3'
+            )
+        )
+        problem = modulestore().get_item(
+            BlockUsageLocator(
+                CourseLocator(org='testx', course='GreekHero', run="run", branch=BRANCH_NAME_DRAFT), 'problem', 'problem3_2'
+            )
+        )
+        self.assertFalse(problem.visible_to_staff_only)
+
+        chapter.visible_to_staff_only = True
+        modulestore().update_item(chapter, self.user_id)
+        problem = modulestore().get_item(problem.location.version_agnostic())
+        self.assertTrue(problem.visible_to_staff_only)
+
+        # unset on parent, retrieve child, verify unset
+        chapter = modulestore().get_item(chapter.location.version_agnostic())
+        chapter.fields['visible_to_staff_only'].delete_from(chapter)
+        modulestore().update_item(chapter, self.user_id)
+
+        problem = modulestore().get_item(problem.location.version_agnostic())
+        self.assertFalse(problem.visible_to_staff_only)
+
+    def test_dynamic_inheritance(self):
+        """
+        Test inheritance for create_item with and without a parent pointer
+        """
+        course_key = CourseLocator(org='testx', course='GreekHero', run="run", branch=BRANCH_NAME_DRAFT)
+        chapter = modulestore().get_item(BlockUsageLocator(course_key, 'chapter', 'chapter3'))
+
+        chapter.visible_to_staff_only = True
+        orphan_problem = modulestore().create_item(self.user_id, course_key, 'problem')
+        self.assertFalse(orphan_problem.visible_to_staff_only)
+        parented_problem = modulestore().create_child(self.user_id, chapter.location.version_agnostic(), 'problem')
+        # FIXME LMS-11376
+#         self.assertTrue(parented_problem.visible_to_staff_only)
+
+        orphan_problem = modulestore().create_xblock(chapter.runtime, course_key, 'problem')
+        self.assertFalse(orphan_problem.visible_to_staff_only)
+        parented_problem = modulestore().create_xblock(chapter.runtime, course_key, 'problem', parent_xblock=chapter)
+        # FIXME LMS-11376
+#         self.assertTrue(parented_problem.visible_to_staff_only)
+
 
 class TestPublish(SplitModuleTest):
     """
@@ -1732,7 +1782,7 @@ class TestSchema(SplitModuleTest):
             )
 
 
-#===========================================
+# ===========================================
 def modulestore():
     """
     Mock the django dependent global modulestore function to disentangle tests from django