Content libraries settings overrides (SOL-46)

cms/djangoapps/contentstore/tests/test_libraries.py

@@ -683,3 +683,130 @@ def test_refresh_library_content_permissions(self, library_role, course_role, ex
         self._bind_module(lc_block, user=self.non_staff_user)  # We must use the CMS's module system in order to get permissions checks.
         lc_block = self._refresh_children(lc_block, status_code_expected=200 if expected_result else 403)
         self.assertEqual(len(lc_block.children), 1 if expected_result else 0)
+
+
+class TestOverrides(LibraryTestCase):
+    """
+    Test that overriding block Scope.settings fields from a library in a specific course works
+    """
+    def setUp(self):
+        super(TestOverrides, self).setUp()
+        self.original_display_name = "A Problem Block"
+        self.original_weight = 1
+
+        # Create a problem block in the library:
+        self.problem = ItemFactory.create(
+            category="problem",
+            parent_location=self.library.location,
+            display_name=self.original_display_name,  # display_name is a Scope.settings field
+            weight=self.original_weight,  # weight is also a Scope.settings field
+            user_id=self.user.id,
+            publish_item=False,
+        )
+
+        # Also create a course:
+        with modulestore().default_store(ModuleStoreEnum.Type.split):
+            self.course = CourseFactory.create()
+
+        # Add a LibraryContent block to the course:
+        self.lc_block = self._add_library_content_block(self.course, self.lib_key)
+        self.lc_block = self._refresh_children(self.lc_block)
+        self.problem_in_course = modulestore().get_item(self.lc_block.children[0])
+
+    def test_overrides(self):
+        """
+        Test that we can override Scope.settings values in a course.
+        """
+        new_display_name = "Modified Problem Title"
+        new_weight = 10
+        self.problem_in_course.display_name = new_display_name
+        self.problem_in_course.weight = new_weight
+        modulestore().update_item(self.problem_in_course, self.user.id)
+
+        # Add a second LibraryContent block to the course, with no override:
+        lc_block2 = self._add_library_content_block(self.course, self.lib_key)
+        lc_block2 = self._refresh_children(lc_block2)
+        # Re-load the two problem blocks - one with and one without an override:
+        self.problem_in_course = modulestore().get_item(self.lc_block.children[0])
+        problem2_in_course = modulestore().get_item(lc_block2.children[0])
+
+        self.assertEqual(self.problem_in_course.display_name, new_display_name)
+        self.assertEqual(self.problem_in_course.weight, new_weight)
+
+        self.assertEqual(problem2_in_course.display_name, self.original_display_name)
+        self.assertEqual(problem2_in_course.weight, self.original_weight)
+
+    def test_reset_override(self):
+        """
+        If we override a setting and then reset it, we should get the library value.
+        """
+        new_display_name = "Modified Problem Title"
+        new_weight = 10
+        self.problem_in_course.display_name = new_display_name
+        self.problem_in_course.weight = new_weight
+        modulestore().update_item(self.problem_in_course, self.user.id)
+        self.problem_in_course = modulestore().get_item(self.problem_in_course.location)
+
+        self.assertEqual(self.problem_in_course.display_name, new_display_name)
+        self.assertEqual(self.problem_in_course.weight, new_weight)
+
+        # Reset:
+        for field_name in ["display_name", "weight"]:
+            self.problem_in_course.fields[field_name].delete_from(self.problem_in_course)
+
+        # Save, reload, and verify:
+        modulestore().update_item(self.problem_in_course, self.user.id)
+        self.problem_in_course = modulestore().get_item(self.problem_in_course.location)
+
+        self.assertEqual(self.problem_in_course.display_name, self.original_display_name)
+        self.assertEqual(self.problem_in_course.weight, self.original_weight)
+
+    def test_consistent_definitions(self):
+        """
+        Make sure that the new child of the LibraryContent block
+        shares its definition with the original (self.problem).
+
+        This test is specific to split mongo.
+        """
+        definition_id = self.problem.definition_locator.definition_id
+        self.assertEqual(self.problem_in_course.definition_locator.definition_id, definition_id)
+
+        # Now even if we change some Scope.settings fields and refresh, the definition should be unchanged
+        self.problem.weight = 20
+        self.problem.display_name = "NEW"
+        modulestore().update_item(self.problem, self.user.id)
+        self.lc_block = self._refresh_children(self.lc_block)
+        self.problem_in_course = modulestore().get_item(self.problem_in_course.location)
+
+        self.assertEqual(self.problem.definition_locator.definition_id, definition_id)
+        self.assertEqual(self.problem_in_course.definition_locator.definition_id, definition_id)
+
+    def test_persistent_overrides(self):
+        """
+        Test that when we override Scope.settings values in a course,
+        the override values persist even when the block is refreshed
+        with updated blocks from the library.
+        """
+        new_display_name = "Modified Problem Title"
+        new_weight = 15
+        self.problem_in_course.display_name = new_display_name
+        self.problem_in_course.weight = new_weight
+
+        modulestore().update_item(self.problem_in_course, self.user.id)
+        self.problem_in_course = modulestore().get_item(self.problem_in_course.location)
+        self.assertEqual(self.problem_in_course.display_name, new_display_name)
+        self.assertEqual(self.problem_in_course.weight, new_weight)
+
+        # Change the settings in the library version:
+        self.problem.display_name = "X"
+        self.problem.weight = 99
+        new_data_value = "<problem><p>We change the data as well to check that non-overriden fields do get updated.</p></problem>"
+        self.problem.data = new_data_value
+        modulestore().update_item(self.problem, self.user.id)
+
+        self.lc_block = self._refresh_children(self.lc_block)
+        self.problem_in_course = modulestore().get_item(self.problem_in_course.location)
+
+        self.assertEqual(self.problem_in_course.display_name, new_display_name)
+        self.assertEqual(self.problem_in_course.weight, new_weight)
+        self.assertEqual(self.problem_in_course.data, new_data_value)

cms/djangoapps/contentstore/views/item.py

@@ -559,7 +559,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 = {}  # Can't use own_metadata(), b/c it converts data for JSON serialization - not suitable for setting metadata of the new block
+        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:
@@ -584,7 +587,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

@@ -323,7 +323,7 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe
     js_module_name = "VerticalDescriptor"
 
     @XBlock.handler
-    def refresh_children(self, request=None, suffix=None, 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
@@ -335,15 +335,12 @@ def refresh_children(self, request=None, suffix=None, update_db=True):  # pylint
         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_perms = self.runtime.service(self, 'studio_user_permissions')
         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, user_perms, update_db)
+        lib_tools.update_children(self, user_id, user_perms)
         return Response()
 
     def _validate_library_version(self, validation, lib_tools, version, library_key):
@@ -451,7 +448,7 @@ def editor_saved(self, user, old_metadata, old_content):
         if (set(old_source_libraries) != set(self.source_libraries) or
                 old_metadata.get('capa_type', ANY_CAPA_TYPE_VALUE) != self.capa_type):
             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,10 +1,8 @@
 """
 XBlock runtime services for LibraryContentModule
 """
-import hashlib
 from django.core.exceptions import PermissionDenied
 from opaque_keys.edx.locator import LibraryLocator
-from xblock.fields import Scope
 from xmodule.library_content_module import LibraryVersionReference, ANY_CAPA_TYPE_VALUE
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.capa_module import CapaDescriptor
@@ -60,7 +58,7 @@ def _filter_child(self, usage_key, capa_type):
         assert isinstance(descriptor, CapaDescriptor)
         return capa_type in descriptor.problem_types
 
-    def update_children(self, dest_block, user_id, user_perms=None, update_db=True):
+    def update_children(self, dest_block, user_id, user_perms=None):
         """
         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
@@ -71,82 +69,28 @@ def update_children(self, dest_block, user_id, user_perms=None, 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 = []
         if user_perms and not user_perms.can_write(dest_block.location.course_key):
             raise PermissionDenied()
 
-        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.")
-                if user_perms and not user_perms.can_read(library_key):
-                    raise PermissionDenied()
-                libraries.append((library_key, library))
+        new_libraries = []
+        source_blocks = []
+        for library_key, __ in dest_block.source_libraries:
+            library = self._get_library(library_key)
+            if library is None:
+                raise ValueError("Required library not found.")
+            if user_perms and not user_perms.can_read(library_key):
+                raise PermissionDenied()
+            filter_children = (dest_block.capa_type != ANY_CAPA_TYPE_VALUE)
+            if filter_children:
+                # Apply simple filtering based on CAPA problem types:
+                source_blocks.extend([key for key in library.children if self._filter_child(key, dest_block.capa_type)])
+            else:
+                source_blocks.extend(library.children)
+            new_libraries.append(LibraryVersionReference(library_key, library.location.library_key.version_guid))
 
-            # 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, filter_problem_type=False):
-                    """
-                    Internal method to copy blocks from the library recursively
-                    """
-                    new_children = []
-                    if filter_problem_type:
-                        filtered_children = [key for key in from_block.children if self._filter_child(key, dest_block.capa_type)]
-                    else:
-                        filtered_children = from_block.children
-                    for child_key in filtered_children:
-                        child = self.store.get_item(child_key, depth=None)
-                        # 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, filter_problem_type=True))
-                new_libraries.append(LibraryVersionReference(library_key, library.location.library_key.version_guid))
+        with self.store.bulk_operations(dest_block.location.course_key):
             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.copy_from_template(source_blocks, dest_block.location, user_id)
+            # ^-- copy_from_template 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

@@ -676,6 +676,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 copy_from_template(self, source_keys, dest_key, user_id, **kwargs):
+        """
+        See :py:meth `SplitMongoModuleStore.copy_from_template`
+        """
+        store = self._verify_modulestore_support(dest_key.course_key, 'copy_from_template')
+        return store.copy_from_template(source_keys, dest_key, user_id)
+
     @strip_key
     def update_item(self, xblock, user_id, allow_not_found=False, **kwargs):
         """