diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index ab261dee96..a8018020fb 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 90a49f940c..ff5af379d8 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/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,14 +1627,19 @@ 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) + new_block = runtime.xblock_from_json( + xblock_class, course_key, block_id, json_data, inherited_settings, **kwargs + ) for field_name, value in fields.iteritems(): setattr(new_block, field_name, value) @@ -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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index 1dd81e863a..35e892d6ea 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -1564,6 +1564,36 @@ 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) + class TestPublish(SplitModuleTest): """ @@ -1732,7 +1762,7 @@ class TestSchema(SplitModuleTest): ) -#=========================================== +# =========================================== def modulestore(): """ Mock the django dependent global modulestore function to disentangle tests from django