Fix duplication of Randomized Content Blocks

cms/djangoapps/contentstore/tests/test_libraries.py

@@ -3,6 +3,7 @@
 """
 from contentstore.tests.utils import AjaxEnabledTestClient, parse_json
 from contentstore.utils import reverse_url, reverse_usage_url, reverse_library_url
+from contentstore.views.item import _duplicate_item
 from contentstore.views.preview import _load_preview_module
 from contentstore.views.tests.test_library import LIBRARY_REST_URL
 import ddt
@@ -726,6 +727,7 @@ def test_refresh_library_content_permissions(self, library_role, course_role, ex
         self.assertEqual(len(lc_block.children), 1 if expected_result else 0)
 
 
+@ddt.ddt
 class TestOverrides(LibraryTestCase):
     """
     Test that overriding block Scope.settings fields from a library in a specific course works
@@ -745,6 +747,9 @@ def setUp(self):
             publish_item=False,
         )
 
+        # Refresh library now that we've added something.
+        self.library = modulestore().get_library(self.lib_key)
+
         # Also create a course:
         with modulestore().default_store(ModuleStoreEnum.Type.split):
             self.course = CourseFactory.create()
@@ -822,7 +827,8 @@ def test_consistent_definitions(self):
         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):
+    @ddt.data(False, True)
+    def test_persistent_overrides(self, duplicate):
         """
         Test that when we override Scope.settings values in a course,
         the override values persist even when the block is refreshed
@@ -834,7 +840,14 @@ def test_persistent_overrides(self):
         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)
+        if duplicate:
+            # Check that this also works when the RCB is duplicated.
+            self.lc_block = modulestore().get_item(
+                _duplicate_item(self.course.location, self.lc_block.location, self.user)
+            )
+            self.problem_in_course = modulestore().get_item(self.lc_block.children[0])
+        else:
+            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)
 
@@ -852,6 +865,52 @@ def test_persistent_overrides(self):
         self.assertEqual(self.problem_in_course.weight, new_weight)
         self.assertEqual(self.problem_in_course.data, new_data_value)
 
+    def test_duplicated_version(self):
+        """
+        Test that if a library is updated, and the content block is duplicated,
+        the new block will use the old library version and not the new one.
+        """
+        store = modulestore()
+        self.assertEqual(len(self.library.children), 1)
+        self.assertEqual(len(self.lc_block.children), 1)
+
+        # Edit the only problem in the library:
+        self.problem.display_name = "--changed in library--"
+        store.update_item(self.problem, self.user.id)
+        # Create an additional problem block in the library:
+        ItemFactory.create(
+            category="problem",
+            parent_location=self.library.location,
+            user_id=self.user.id,
+            publish_item=False,
+        )
+
+        # Refresh our reference to the library
+        self.library = store.get_library(self.lib_key)
+
+        # Refresh our reference to the block
+        self.lc_block = store.get_item(self.lc_block.location)
+        self.problem_in_course = store.get_item(self.problem_in_course.location)
+
+        # The library has changed...
+        self.assertEqual(len(self.library.children), 2)
+
+        # But the block hasn't.
+        self.assertEqual(len(self.lc_block.children), 1)
+        self.assertEqual(self.problem_in_course.location, self.lc_block.children[0])
+        self.assertEqual(self.problem_in_course.display_name, self.original_display_name)
+
+        # Duplicate self.lc_block:
+        duplicate = store.get_item(
+            _duplicate_item(self.course.location, self.lc_block.location, self.user)
+        )
+        # The duplicate should have identical children to the original:
+        self.assertEqual(len(duplicate.children), 1)
+        self.assertTrue(self.lc_block.source_library_version)
+        self.assertEqual(self.lc_block.source_library_version, duplicate.source_library_version)
+        problem2_in_course = store.get_item(duplicate.children[0])
+        self.assertEqual(problem2_in_course.display_name, self.original_display_name)
+
 
 class TestIncompatibleModuleStore(LibraryTestCase):
     """

cms/djangoapps/contentstore/views/item.py

@@ -593,16 +593,25 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_
             runtime=source_item.runtime,
         )
 
+        children_handled = False
+
+        if hasattr(dest_module, 'studio_post_duplicate'):
+            # Allow an XBlock to do anything fancy it may need to when duplicated from another block.
+            # These blocks may handle their own children or parenting if needed. Let them return booleans to
+            # let us know if we need to handle these or not.
+            children_handled = dest_module.studio_post_duplicate(store, source_item)
+
         # Children are not automatically copied over (and not all xblocks have a 'children' attribute).
         # Because DAGs are not fully supported, we need to actually duplicate each child as well.
-        if source_item.has_children:
-            dest_module.children = []
+        if source_item.has_children and not children_handled:
+            dest_module.children = dest_module.children or []
             for child in source_item.children:
                 dupe = _duplicate_item(dest_module.location, child, user=user)
                 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)
 
+        # pylint: disable=protected-access
         if 'detached' not in source_item.runtime.load_block_type(category)._class_tags:
             parent = store.get_item(parent_usage_key)
             # If source was already a child of the parent, add duplicate immediately afterward.

cms/djangoapps/contentstore/views/preview.py

@@ -240,7 +240,6 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False):
     # Only add the Studio wrapper when on the container page. The "Pages" page will remain as is for now.
     if not context.get('is_pages_view', None) and view in PREVIEW_VIEWS:
         root_xblock = context.get('root_xblock')
-        can_edit_visibility = not isinstance(xblock.location, LibraryUsageLocator)
         is_root = root_xblock and xblock.location == root_xblock.location
         is_reorderable = _is_xblock_reorderable(xblock, context)
         template_context = {
@@ -251,7 +250,8 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False):
             'is_root': is_root,
             'is_reorderable': is_reorderable,
             'can_edit': context.get('can_edit', True),
-            'can_edit_visibility': can_edit_visibility,
+            'can_edit_visibility': context.get('can_edit_visibility', True),
+            'can_add': context.get('can_add', True),
         }
         html = render_to_string('studio_xblock_wrapper.html', template_context)
         frag = wrap_fragment(frag, html)

cms/templates/studio_xblock_wrapper.html

@@ -80,19 +80,24 @@
                                     </a>
                                 </li>
                             % endif
-                            <li class="action-item action-duplicate">
-                                <a href="#" data-tooltip="${_("Duplicate")}" class="duplicate-button action-button">
-                                <i class="icon fa fa-copy"></i>
-                                <span class="sr">${_("Duplicate")}</span>
+                            % if can_add:
+                                <li class="action-item action-duplicate">
+                                    <a href="#" data-tooltip="${_("Duplicate")}" class="duplicate-button action-button">
+                                    <i class="icon fa fa-copy"></i>
+                                    <span class="sr">${_("Duplicate")}</span>
+                                    </a>
+                                </li>
+                            % endif
+                        % endif
+                        % if can_add:
+                            <!-- If we can add, we can delete. -->
+                            <li class="action-item action-delete">
+                                <a href="#" data-tooltip="${_("Delete")}" class="delete-button action-button">
+                                <i class="icon fa fa-trash-o"></i>
+                                <span class="sr">${_("Delete")}</span>
                                 </a>
                             </li>
                         % endif
-                        <li class="action-item action-delete">
-                            <a href="#" data-tooltip="${_("Delete")}" class="delete-button action-button">
-                            <i class="icon fa fa-trash-o"></i>
-                            <span class="sr">${_("Delete")}</span>
-                            </a>
-                        </li>
                         % if is_reorderable:
                             <li class="action-item action-drag">
                                 <span data-tooltip="${_('Drag to reorder')}" class="drag-handle action"></span>

common/lib/xmodule/xmodule/library_content_module.py

@@ -7,6 +7,7 @@
 from copy import copy
 from capa.responsetypes import registry
 from gettext import ngettext
+from lazy import lazy
 
 from .mako_module import MakoModuleDescriptor
 from opaque_keys.edx.locator import LibraryLocator
@@ -269,6 +270,7 @@ def author_view(self, context):
                     'max_count': self.max_count,
                     'display_name': self.display_name or self.url_name,
                 }))
+                context['can_edit_visibility'] = False
                 self.render_children(context, fragment, can_reorder=False, can_add=False)
         # else: When shown on a unit page, don't show any sort of preview -
         # just the status of this block in the validation area.
@@ -306,6 +308,25 @@ def non_editable_metadata_fields(self):
         non_editable_fields.extend([LibraryContentFields.mode, LibraryContentFields.source_library_version])
         return non_editable_fields
 
+    @lazy
+    def tools(self):
+        """
+        Grab the library tools service or raise an error.
+        """
+        return self.runtime.service(self, 'library_tools')
+
+    def get_user_id(self):
+        """
+        Get the ID of the current user.
+        """
+        user_service = self.runtime.service(self, 'user')
+        if user_service:
+            # May be None when creating bok choy test fixtures
+            user_id = user_service.get_current_user().opt_attrs.get('edx-platform.user_id', None)
+        else:
+            user_id = None
+        return user_id
+
     @XBlock.handler
     def refresh_children(self, request=None, suffix=None):  # pylint: disable=unused-argument
         """
@@ -320,21 +341,50 @@ def refresh_children(self, request=None, suffix=None):  # pylint: disable=unused
         the version number of the libraries used, so we easily determine if
         this block is up to date or not.
         """
-        lib_tools = self.runtime.service(self, 'library_tools')
-        if not lib_tools:
-            # This error is diagnostic. The user won't see it, but it may be helpful
-            # during debugging.
-            return Response(_(u"Course does not support Library tools."), status=400)
-        user_service = self.runtime.service(self, 'user')
         user_perms = self.runtime.service(self, 'studio_user_permissions')
-        if user_service:
-            # May be None when creating bok choy test fixtures
-            user_id = user_service.get_current_user().opt_attrs.get('edx-platform.user_id', None)
-        else:
-            user_id = None
-        lib_tools.update_children(self, user_id, user_perms)
+        user_id = self.get_user_id()
+        if not self.tools:
+            return Response("Library Tools unavailable in current runtime.", status=400)
+        self.tools.update_children(self, user_id, user_perms)
         return Response()
 
+    # Copy over any overridden settings the course author may have applied to the blocks.
+    def _copy_overrides(self, store, user_id, source, dest):
+        """
+        Copy any overrides the user has made on blocks in this library.
+        """
+        for field in source.fields.itervalues():
+            if field.scope == Scope.settings and field.is_set_on(source):
+                setattr(dest, field.name, field.read_from(source))
+        if source.has_children:
+            source_children = [self.runtime.get_block(source_key) for source_key in source.children]
+            dest_children = [self.runtime.get_block(dest_key) for dest_key in dest.children]
+            for source_child, dest_child in zip(source_children, dest_children):
+                self._copy_overrides(store, user_id, source_child, dest_child)
+        store.update_item(dest, user_id)
+
+    def studio_post_duplicate(self, store, source_block):
+        """
+        Used by the studio after basic duplication of a source block. We handle the children
+        ourselves, because we have to properly reference the library upstream and set the overrides.
+
+        Otherwise we'll end up losing data on the next refresh.
+        """
+        # The first task will be to refresh our copy of the library to generate the children.
+        # We must do this at the currently set version of the library block. Otherwise we may not have
+        # exactly the same children-- someone may be duplicating an out of date block, after all.
+        user_id = self.get_user_id()
+        user_perms = self.runtime.service(self, 'studio_user_permissions')
+        # pylint: disable=no-member
+        if not self.tools:
+            raise RuntimeError("Library tools unavailable, duplication will not be sane!")
+        self.tools.update_children(self, user_id, user_perms, version=self.source_library_version)
+
+        self._copy_overrides(store, user_id, source_block, self)
+
+        # Children have been handled.
+        return True
+
     def _validate_library_version(self, validation, lib_tools, version, library_key):
         """
         Validates library version

common/lib/xmodule/xmodule/library_root_xblock.py

@@ -82,6 +82,7 @@ def render_children(self, context, fragment, can_reorder=False, can_add=False):
             # Children must have a separate context from the library itself. Make a copy.
             child_context = context.copy()
             child_context['show_preview'] = self.show_children_previews
+            child_context['can_edit_visibility'] = False
             child = self.runtime.get_block(child_key)
             child_view_name = StudioEditableModule.get_preview_view_name(child)
 

common/lib/xmodule/xmodule/library_tools.py

@@ -4,6 +4,7 @@
 from django.core.exceptions import PermissionDenied
 from opaque_keys.edx.locator import LibraryLocator
 from xmodule.library_content_module import ANY_CAPA_TYPE_VALUE
+from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.capa_module import CapaDescriptor
 
@@ -21,14 +22,17 @@ def _get_library(self, library_key):
         Given a library key like "library-v1:ProblemX+PR0B", return the
         'library' XBlock with meta-information about the library.
 
+        A specific version may be specified.
+
         Returns None on error.
         """
         if not isinstance(library_key, LibraryLocator):
             library_key = LibraryLocator.from_string(library_key)
-        assert library_key.version_guid is None
 
         try:
-            return self.store.get_library(library_key, remove_version=False, remove_branch=False)
+            return self.store.get_library(
+                library_key, remove_version=False, remove_branch=False, head_validation=False
+            )
         except ItemNotFoundError:
             return None
 
@@ -102,7 +106,7 @@ def can_use_library_content(self, block):
         """
         return self.store.check_supports(block.location.course_key, 'copy_from_template')
 
-    def update_children(self, dest_block, user_id, user_perms=None):
+    def update_children(self, dest_block, user_id, user_perms=None, version=None):
         """
         This method is to be used when the library that a LibraryContentModule
         references has been updated. It will re-fetch all matching blocks from
@@ -123,6 +127,8 @@ def update_children(self, dest_block, user_id, user_perms=None):
 
         source_blocks = []
         library_key = dest_block.source_library_key
+        if version:
+            library_key = library_key.replace(branch=ModuleStoreEnum.BranchName.library, version_guid=version)
         library = self._get_library(library_key)
         if library is None:
             raise ValueError("Requested library not found.")
@@ -138,7 +144,10 @@ def update_children(self, dest_block, user_id, user_perms=None):
         with self.store.bulk_operations(dest_block.location.course_key):
             dest_block.source_library_version = unicode(library.location.library_key.version_guid)
             self.store.update_item(dest_block, user_id)
-            dest_block.children = self.store.copy_from_template(source_blocks, dest_block.location, user_id)
+            head_validation = not version
+            dest_block.children = self.store.copy_from_template(
+                source_blocks, dest_block.location, user_id, head_validation=head_validation
+            )
             # ^-- copy_from_template updates the children in the DB
             # but we must also set .children here to avoid overwriting the DB again