Content libraries settings overrides (SOL-46)

cms/djangoapps/contentstore/tests/test_libraries.py

@@ -303,3 +303,57 @@ def test_change_after_first_sync(self):
         self.assertEqual(len(lc_block.children), 1)  # Children should not be deleted due to a bad setting.
         html_block = modulestore().get_item(lc_block.children[0])
         self.assertEqual(html_block.data, data_value)
+
+    def test_overrides(self):
+        """
+        Test that overriding block Scope.settings fields from a library in a specific course works
+        """
+        original_display_name = "An HTML Block"
+
+        # First, create a library containing an HTML block:
+        block1 = ItemFactory.create(
+            category="html",
+            parent_location=self.library.location,
+            display_name=original_display_name,  # display_name is a scope.settings field
+            user_id=self.user.id,
+            publish_item=False,
+        )
+        self.assertEqual(block1.display_name, original_display_name)
+        def_id1 = block1.definition_locator.definition_id
+
+        # Next, create two courses:
+        with modulestore().default_store(ModuleStoreEnum.Type.split):
+            course1 = CourseFactory.create()
+            course2 = CourseFactory.create()
+
+        # Add a LibraryContent block to each course:
+        lc_block1 = self._add_library_content_block(course1, self.lib_key)
+        lc_block1 = self._refresh_children(lc_block1)
+
+        # Make sure that the new child of the LibraryContent block
+        # shares its definition with block1
+        block1_course = modulestore().get_item(lc_block1.children[0])
+        self.assertEqual(block1_course.definition_locator.definition_id, def_id1)
+        self.assertEqual(block1_course.display_name, original_display_name)
+
+        # Change a settings field on lc_block1
+        block1_course.display_name = "NEW"
+        modulestore().update_item(block1_course, self.user.id)
+        block1_course = modulestore().get_item(block1_course.location)
+        self.assertEqual(block1_course.display_name, "NEW")
+
+        # Add a LibraryContent block to the second course:
+        lc_block2 = self._add_library_content_block(course2, self.lib_key)
+        lc_block2 = self._refresh_children(lc_block2)
+        block2_course = modulestore().get_item(lc_block2.children[0])
+        self.assertEqual(block2_course.display_name, original_display_name)
+
+        # Now make sure the override persists even when blocks are refreshed.
+        # Force a new library version by adding another block to the library
+        ItemFactory.create(category="problem", parent_location=self.library.location, user_id=self.user.id, publish_item=False)
+
+        block1_course = modulestore().get_item(block1_course.location)
+        block2_course = modulestore().get_item(block2_course.location)
+
+        self.assertEqual(block1_course.display_name, "NEW")
+        self.assertEqual(block2_course.display_name, original_display_name)

cms/djangoapps/contentstore/views/item.py

@@ -553,7 +553,10 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_
         category = dest_usage_key.block_type
 
         # Update the display name to indicate this is a duplicate (unless display name provided).
-        duplicate_metadata = own_metadata(source_item)
+        duplicate_metadata = {}
+        for field in source_item.fields.values():
+            if (field.scope == Scope.settings and field.is_set_on(source_item)):
+                duplicate_metadata[field.name] = field.read_from(source_item)
         if display_name is not None:
             duplicate_metadata['display_name'] = display_name
         else:
@@ -578,7 +581,8 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_
             dest_module.children = []
             for child in source_item.children:
                 dupe = _duplicate_item(dest_module.location, child, user=user)
-                dest_module.children.append(dupe)
+                if dupe not in dest_module.children:  # _duplicate_item may add the child for us.
+                    dest_module.children.append(dupe)
             store.update_item(dest_module, user.id)
 
         if 'detached' not in source_item.runtime.load_block_type(category)._class_tags:

common/lib/xmodule/xmodule/library_content_module.py

@@ -289,7 +289,7 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe
     js_module_name = "VerticalDescriptor"
 
     @XBlock.handler
-    def refresh_children(self, request, suffix, update_db=True):  # pylint: disable=unused-argument
+    def refresh_children(self, request=None, suffix=None):  # pylint: disable=unused-argument
         """
         Refresh children:
         This method is to be used when any of the libraries that this block
@@ -301,14 +301,11 @@ def refresh_children(self, request, suffix, update_db=True):  # pylint: disable=
         This method will update this block's 'source_libraries' field to store
         the version number of the libraries used, so we easily determine if
         this block is up to date or not.
-
-        If update_db is True (default), this will explicitly persist the changes
-        to the modulestore by calling update_item()
         """
         lib_tools = self.runtime.service(self, 'library_tools')
         user_service = self.runtime.service(self, 'user')
         user_id = user_service.user_id if user_service else None  # May be None when creating bok choy test fixtures
-        lib_tools.update_children(self, user_id, update_db)
+        lib_tools.update_children(self, user_id)
         return Response()
 
     def validate(self):
@@ -364,7 +361,7 @@ def editor_saved(self, user, old_metadata, old_content):
         old_source_libraries = LibraryList().from_json(old_metadata.get('source_libraries', []))
         if set(old_source_libraries) != set(self.source_libraries):
             try:
-                self.refresh_children(None, None, update_db=False)  # update_db=False since update_item() is about to be called anyways
+                self.refresh_children()
             except ValueError:
                 pass  # The validation area will display an error message, no need to do anything now.
 

common/lib/xmodule/xmodule/library_tools.py

@@ -1,9 +1,7 @@
 """
 XBlock runtime services for LibraryContentModule
 """
-import hashlib
 from opaque_keys.edx.locator import LibraryLocator
-from xblock.fields import Scope
 from xmodule.library_content_module import LibraryVersionReference
 from xmodule.modulestore.exceptions import ItemNotFoundError
 
@@ -54,7 +52,7 @@ def get_library_display_name(self, lib_key):
             return library.display_name
         return None
 
-    def update_children(self, dest_block, user_id, update_db=True):
+    def update_children(self, dest_block, user_id):
         """
         This method is to be used when any of the libraries that a LibraryContentModule
         references have been updated. It will re-fetch all matching blocks from
@@ -65,74 +63,18 @@ def update_children(self, dest_block, user_id, update_db=True):
         This method will update dest_block's 'source_libraries' field to store
         the version number of the libraries used, so we easily determine if
         dest_block is up to date or not.
-
-        If update_db is True (default), this will explicitly persist the changes
-        to the modulestore by calling update_item(). Only set update_db False if
-        you know for sure that dest_block is about to be saved to the modulestore
-        anyways. Otherwise, orphaned blocks may be created.
         """
-        root_children = []
+        new_libraries = []
+        source_blocks = []
+        for library_key, dummy in dest_block.source_libraries:
+            library = self._get_library(library_key)
+            if library is None:
+                raise ValueError("Required library not found.")
+            source_blocks.extend(library.children)  # In future, this will be filtered so only specific children are used.
+            new_libraries.append(LibraryVersionReference(library_key, library.location.library_key.version_guid))
 
         with self.store.bulk_operations(dest_block.location.course_key):
-            # Currently, ALL children are essentially deleted and then re-added
-            # in a way that preserves their block_ids (and thus should preserve
-            # student data, grades, analytics, etc.)
-            # Once course-level field overrides are implemented, this will
-            # change to a more conservative implementation.
-
-            # First, load and validate the source_libraries:
-            libraries = []
-            for library_key, old_version in dest_block.source_libraries:  # pylint: disable=unused-variable
-                library = self._get_library(library_key)
-                if library is None:
-                    raise ValueError("Required library not found.")
-                libraries.append((library_key, library))
-
-            # Next, delete all our existing children to avoid block_id conflicts when we add them:
-            for child in dest_block.children:
-                self.store.delete_item(child, user_id)
-
-            # Now add all matching children, and record the library version we use:
-            new_libraries = []
-            for library_key, library in libraries:
-
-                def copy_children_recursively(from_block):
-                    """
-                    Internal method to copy blocks from the library recursively
-                    """
-                    new_children = []
-                    for child_key in from_block.children:
-                        child = self.store.get_item(child_key, depth=9)
-                        # We compute a block_id for each matching child block found in the library.
-                        # block_ids are unique within any branch, but are not unique per-course or globally.
-                        # We need our block_ids to be consistent when content in the library is updated, so
-                        # we compute block_id as a hash of three pieces of data:
-                        unique_data = "{}:{}:{}".format(
-                            dest_block.location.block_id,  # Must not clash with other usages of the same library in this course
-                            unicode(library_key.for_version(None)).encode("utf-8"),  # The block ID below is only unique within a library, so we need this too
-                            child_key.block_id,  # Child block ID. Should not change even if the block is edited.
-                        )
-                        child_block_id = hashlib.sha1(unique_data).hexdigest()[:20]
-                        fields = {}
-                        for field in child.fields.itervalues():
-                            if field.scope == Scope.settings and field.is_set_on(child):
-                                fields[field.name] = field.read_from(child)
-                        if child.has_children:
-                            fields['children'] = copy_children_recursively(from_block=child)
-                        new_child_info = self.store.create_item(
-                            user_id,
-                            dest_block.location.course_key,
-                            child_key.block_type,
-                            block_id=child_block_id,
-                            definition_locator=child.definition_locator,
-                            runtime=dest_block.system,
-                            fields=fields,
-                        )
-                        new_children.append(new_child_info.location)
-                    return new_children
-                root_children.extend(copy_children_recursively(from_block=library))
-                new_libraries.append(LibraryVersionReference(library_key, library.location.library_key.version_guid))
             dest_block.source_libraries = new_libraries
-            dest_block.children = root_children
-            if update_db:
-                self.store.update_item(dest_block, user_id)
+            self.store.update_item(dest_block, user_id)
+            dest_block.children = self.store.inherit_copy(source_blocks, dest_block.location, user_id, copy_children=True)
+            # ^-- inherit_copy updates the children in the DB but we must also set .children here to avoid overwriting the DB again

common/lib/xmodule/xmodule/modulestore/inheritance.py

@@ -211,8 +211,8 @@ def inherit_metadata(descriptor, inherited_data):
 
 def own_metadata(module):
     """
-    Return a dictionary that contains only non-inherited field keys,
-    mapped to their serialized values
+    Return a JSON-friendly dictionary that contains only non-inherited field
+    keys, mapped to their serialized values
     """
     return module.get_explicitly_set_fields_by_scope(Scope.settings)
 
@@ -283,6 +283,8 @@ def has(self, key):
 
     def default(self, key):
         """
-        Check to see if the default should be from inheritance rather than from the field's global default
+        Check to see if the default should be from inheritance. If not
+        inheriting, this will raise KeyError which will cause the caller to use
+        the field's global default.
         """
         return self.inherited_settings[key.field_name]

common/lib/xmodule/xmodule/modulestore/mixed.py

@@ -671,6 +671,14 @@ def import_xblock(self, user_id, course_key, block_type, block_id, fields=None,
         store = self._verify_modulestore_support(course_key, 'import_xblock')
         return store.import_xblock(user_id, course_key, block_type, block_id, fields, runtime)
 
+    @strip_key
+    def inherit_copy(self, source_keys, dest_key, user_id, copy_children=True):
+        """
+        See :py:meth `SplitMongoModuleStore.inherit_copy`
+        """
+        store = self._verify_modulestore_support(dest_key, 'inherit_copy')
+        return store.inherit_copy(source_keys, dest_key, user_id, copy_children)
+
     @strip_key
     def update_item(self, xblock, user_id, allow_not_found=False, **kwargs):
         """

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

@@ -169,16 +169,17 @@ def xblock_from_json(
         if block_key is None:
             block_key = BlockKey(json_data['block_type'], LocalId())
 
+        convert_fields = lambda field: self.modulestore.convert_references_to_keys(
+            course_key, class_, field, self.course_entry.structure['blocks'],
+        )
+
         if definition_id is not None and not json_data.get('definition_loaded', False):
             definition_loader = DefinitionLazyLoader(
                 self.modulestore,
                 course_key,
                 block_key.type,
                 definition_id,
-                lambda fields: self.modulestore.convert_references_to_keys(
-                    course_key, self.load_block_type(block_key.type),
-                    fields, self.course_entry.structure['blocks'],
-                )
+                convert_fields,
             )
         else:
             definition_loader = None
@@ -193,9 +194,8 @@ def xblock_from_json(
             block_id=block_key.id,
         )
 
-        converted_fields = self.modulestore.convert_references_to_keys(
-            block_locator.course_key, class_, json_data.get('fields', {}), self.course_entry.structure['blocks'],
-        )
+        converted_fields = convert_fields(json_data.get('fields', {}))
+        converted_defaults = convert_fields(json_data.get('defaults', {}))
         if block_key in self._parent_map:
             parent_key = self._parent_map[block_key]
             parent = course_key.make_usage_key(parent_key.type, parent_key.id)
@@ -204,6 +204,7 @@ def xblock_from_json(
         kvs = SplitMongoKVS(
             definition_loader,
             converted_fields,
+            converted_defaults,
             parent=parent,
             field_decorator=kwargs.get('field_decorator')
         )

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

@@ -258,7 +258,7 @@ def get_definitions(self, definitions):
         """
         Retrieve all definitions listed in `definitions`.
         """
-        return self.definitions.find({'$in': {'_id': definitions}})
+        return self.definitions.find({'_id': {'$in': definitions}})
 
     def insert_definition(self, definition):
         """