From a6c00635b23bfaa38018ebe982a0b6691cf65220 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Wed, 25 Mar 2015 21:00:39 +0000 Subject: [PATCH 1/9] Fix issues with overrides on duplicated RCBs. --- .../contentstore/tests/test_libraries.py | 14 +++- cms/djangoapps/contentstore/views/item.py | 16 +++- .../xmodule/xmodule/library_content_module.py | 74 ++++++++++++++++--- common/lib/xmodule/xmodule/library_tools.py | 13 +++- 4 files changed, 97 insertions(+), 20 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index 0ff0bfabce..b484c74a09 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -3,6 +3,7 @@ Content library unit tests that require the CMS runtime. """ 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 @@ class TestLibraryAccess(SignalDisconnectTestMixin, LibraryTestCase): 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 @@ -822,7 +824,8 @@ class TestOverrides(LibraryTestCase): 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 +837,14 @@ class TestOverrides(LibraryTestCase): 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) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 1de8db3413..fc876a37db 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -593,17 +593,27 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_ runtime=source_item.runtime, ) + handle_children = True + handle_parenting = True + + 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. + handle_children, handle_parenting = dest_module.studio_post_duplicate(store, parent_usage_key, 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 handle_children: + 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) - if 'detached' not in source_item.runtime.load_block_type(category)._class_tags: + # pylint: disable=protected-access + if ('detached' not in source_item.runtime.load_block_type(category)._class_tags) and handle_parenting: parent = store.get_item(parent_usage_key) # If source was already a child of the parent, add duplicate immediately afterward. # Otherwise, add child to end. diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index d80ae4f82b..a8e329effc 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -306,6 +306,29 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe non_editable_fields.extend([LibraryContentFields.mode, LibraryContentFields.source_library_version]) return non_editable_fields + def get_tools(self): + """ + Grab the library tools service or raise an error. + """ + 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) + return lib_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 +343,50 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe 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') + lib_tools = self.get_tools() 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 + user_id = self.get_user_id() lib_tools.update_children(self, user_id, user_perms) return Response() + # pylint: disable=unused-argument + def studio_post_duplicate(self, store, parent_usage_key, 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. + lib_tools = self.get_tools() + user_id = self.get_user_id() + user_perms = self.runtime.service(self, 'studio_user_permissions') + # pylint: disable=no-member + lib_tools.update_children(self, user_id, user_perms, version=self.source_library_version) + + # Copy over any overridden settings the course author may have applied to the blocks. + def copy_overrides(source, dest): + """ + Copy any overrides the user has made on blocks in this library. + """ + for field_name in source.fields.keys(): + field = dest.fields[field_name] + if field.scope == Scope.settings and field.is_set_on(source): + setattr(dest, field_name, getattr(source, field_name)) + if source.has_children: + source_children = [store.get_item(source_key) for source_key in source.children] + dest_children = [store.get_item(dest_key) for dest_key in dest.children] + for source_child, dest_child in zip(source_children, dest_children): + copy_overrides(source_child, dest_child) + store.update_item(dest, user_id) + + copy_overrides(source_block, self) + + # Don't handle children. Handle parenting. + return False, True + def _validate_library_version(self, validation, lib_tools, version, library_key): """ Validates library version diff --git a/common/lib/xmodule/xmodule/library_tools.py b/common/lib/xmodule/xmodule/library_tools.py index 40afbbb549..da248457da 100644 --- a/common/lib/xmodule/xmodule/library_tools.py +++ b/common/lib/xmodule/xmodule/library_tools.py @@ -16,16 +16,21 @@ class LibraryToolsService(object): def __init__(self, modulestore): self.store = modulestore - def _get_library(self, library_key): + def _get_library(self, library_key, version=None): """ 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 + + library_key = LibraryLocator( + org=library_key.org, library=library_key.library, branch=library_key.branch, version_guid=version + ) try: return self.store.get_library(library_key, remove_version=False, remove_branch=False) @@ -102,7 +107,7 @@ class LibraryToolsService(object): """ 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,7 +128,7 @@ class LibraryToolsService(object): source_blocks = [] library_key = dest_block.source_library_key - library = self._get_library(library_key) + library = self._get_library(library_key, version=version) if library is None: raise ValueError("Requested library not found.") if user_perms and not user_perms.can_read(library_key): From 51905f0cd537ccec5b15606cf8ec6d778bd0f930 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Wed, 1 Apr 2015 18:42:10 +0000 Subject: [PATCH 2/9] Addressed notes for RCB duplication, removed management buttons from RCB container --- cms/djangoapps/contentstore/views/item.py | 9 +++-- cms/djangoapps/contentstore/views/preview.py | 4 +-- cms/templates/studio_xblock_wrapper.html | 25 ++++++++------ .../xmodule/xmodule/library_content_module.py | 34 ++++++++----------- .../xmodule/xmodule/library_root_xblock.py | 1 + common/lib/xmodule/xmodule/library_tools.py | 5 ++- common/lib/xmodule/xmodule/studio_editable.py | 1 + .../test/acceptance/pages/studio/container.py | 14 ++++++++ .../studio/test_studio_library_container.py | 18 ++++++++++ 9 files changed, 71 insertions(+), 40 deletions(-) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index fc876a37db..36e1e877fb 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -593,18 +593,17 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_ runtime=source_item.runtime, ) - handle_children = True - handle_parenting = True + 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. - handle_children, handle_parenting = dest_module.studio_post_duplicate(store, parent_usage_key, source_item) + 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 and handle_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) @@ -613,7 +612,7 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_ store.update_item(dest_module, user.id) # pylint: disable=protected-access - if ('detached' not in source_item.runtime.load_block_type(category)._class_tags) and handle_parenting: + 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. # Otherwise, add child to end. diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index e831ca7dd1..fbcd7f3bba 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/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) diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index 8ffbbc7f4d..f947bf1038 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -80,19 +80,24 @@ messages = json.dumps(xblock.validate().to_json()) % endif -
  • - - - ${_("Duplicate")} + % if can_add: +
  • + + + ${_("Duplicate")} + +
  • + % endif + % endif + % if can_add: + +
  • + + + ${_("Delete")}
  • % endif -
  • - - - ${_("Delete")} - -
  • % if is_reorderable:
  • diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index a8e329effc..bbe696fd3f 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -7,6 +7,7 @@ from lxml import etree 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 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule): '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,16 +308,12 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe non_editable_fields.extend([LibraryContentFields.mode, LibraryContentFields.source_library_version]) return non_editable_fields - def get_tools(self): + @lazy + def tools(self): """ Grab the library tools service or raise an error. """ - 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) - return lib_tools + return self.runtime.service(self, 'library_tools') def get_user_id(self): """ @@ -343,14 +341,12 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe the version number of the libraries used, so we easily determine if this block is up to date or not. """ - lib_tools = self.get_tools() user_perms = self.runtime.service(self, 'studio_user_permissions') user_id = self.get_user_id() - lib_tools.update_children(self, user_id, user_perms) + self.tools.update_children(self, user_id, user_perms) return Response() - # pylint: disable=unused-argument - def studio_post_duplicate(self, store, parent_usage_key, source_block): + 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. @@ -360,32 +356,30 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe # 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. - lib_tools = self.get_tools() user_id = self.get_user_id() user_perms = self.runtime.service(self, 'studio_user_permissions') # pylint: disable=no-member - lib_tools.update_children(self, user_id, user_perms, version=self.source_library_version) + self.tools.update_children(self, user_id, user_perms, version=self.source_library_version) # Copy over any overridden settings the course author may have applied to the blocks. def copy_overrides(source, dest): """ Copy any overrides the user has made on blocks in this library. """ - for field_name in source.fields.keys(): - field = dest.fields[field_name] + for field in source.fields.itervalues(): if field.scope == Scope.settings and field.is_set_on(source): - setattr(dest, field_name, getattr(source, field_name)) + setattr(dest, field.name, field.read_from(source)) if source.has_children: - source_children = [store.get_item(source_key) for source_key in source.children] - dest_children = [store.get_item(dest_key) for dest_key in dest.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): copy_overrides(source_child, dest_child) store.update_item(dest, user_id) copy_overrides(source_block, self) - # Don't handle children. Handle parenting. - return False, True + # Children have been handled. + return True def _validate_library_version(self, validation, lib_tools, version, library_key): """ diff --git a/common/lib/xmodule/xmodule/library_root_xblock.py b/common/lib/xmodule/xmodule/library_root_xblock.py index 9da0d9eaf9..2b6f642bf0 100644 --- a/common/lib/xmodule/xmodule/library_root_xblock.py +++ b/common/lib/xmodule/xmodule/library_root_xblock.py @@ -82,6 +82,7 @@ class LibraryRoot(XBlock): # 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) diff --git a/common/lib/xmodule/xmodule/library_tools.py b/common/lib/xmodule/xmodule/library_tools.py index da248457da..33d79619b0 100644 --- a/common/lib/xmodule/xmodule/library_tools.py +++ b/common/lib/xmodule/xmodule/library_tools.py @@ -28,9 +28,8 @@ class LibraryToolsService(object): if not isinstance(library_key, LibraryLocator): library_key = LibraryLocator.from_string(library_key) - library_key = LibraryLocator( - org=library_key.org, library=library_key.library, branch=library_key.branch, version_guid=version - ) + if version: + library_key.for_version(version) try: return self.store.get_library(library_key, remove_version=False, remove_branch=False) diff --git a/common/lib/xmodule/xmodule/studio_editable.py b/common/lib/xmodule/xmodule/studio_editable.py index 28e44d4bcc..d06c20c4fe 100644 --- a/common/lib/xmodule/xmodule/studio_editable.py +++ b/common/lib/xmodule/xmodule/studio_editable.py @@ -22,6 +22,7 @@ class StudioEditableBlock(object): for child in self.get_children(): # pylint: disable=no-member if can_reorder: context['reorderable_items'].add(child.location) + context['can_add'] = can_add rendered_child = child.render(StudioEditableModule.get_preview_view_name(child), context) fragment.add_frag_resources(rendered_child) diff --git a/common/test/acceptance/pages/studio/container.py b/common/test/acceptance/pages/studio/container.py index 3bd58bd2bb..b06b6956be 100644 --- a/common/test/acceptance/pages/studio/container.py +++ b/common/test/acceptance/pages/studio/container.py @@ -406,6 +406,20 @@ class XBlockWrapper(PageObject): def has_group_visibility_set(self): return self.q(css=self._bounded_selector('.wrapper-xblock.has-group-visibility-set')).is_present() + @property + def has_duplicate_button(self): + """ + Returns true if this xblock has a 'duplicate' button + """ + return self.q(css=self._bounded_selector('a.duplicate-button')) + + @property + def has_delete_button(self): + """ + Returns true if this xblock has a 'delete' button + """ + return self.q(css=self._bounded_selector('a.delete-button')) + @property def has_edit_visibility_button(self): """ diff --git a/common/test/acceptance/tests/studio/test_studio_library_container.py b/common/test/acceptance/tests/studio/test_studio_library_container.py index 772b9013a0..01591203d8 100644 --- a/common/test/acceptance/tests/studio/test_studio_library_container.py +++ b/common/test/acceptance/tests/studio/test_studio_library_container.py @@ -290,3 +290,21 @@ class StudioLibraryContainerTest(StudioLibraryTest, UniqueCourseTest): block.reset_field_val("Display Name") block.save_settings() self.assertEqual(block.name, name_default) + + def test_cannot_manage(self): + """ + Scenario: Given I have a library, a course and library content xblock in a course + When I go to studio unit page for library content block + And when I click the "View" link + Then I can see a preview of the blocks drawn from the library. + + And I do not see a duplicate button + And I do not see a delete button + """ + block_wrapper_unit_page = self._get_library_xblock_wrapper(self.unit_page.xblocks[0].children[0]) + container_page = block_wrapper_unit_page.go_to_container() + + for block in container_page.xblocks: + self.assertFalse(block.has_duplicate_button) + self.assertFalse(block.has_delete_button) + self.assertFalse(block.has_edit_visibility_button) From ce19c769ebf44c81d6f60ac826d17991e077c84e Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Thu, 2 Apr 2015 18:21:23 +0000 Subject: [PATCH 3/9] Fix handling of library versions in the library tools. --- common/lib/xmodule/xmodule/library_tools.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/common/lib/xmodule/xmodule/library_tools.py b/common/lib/xmodule/xmodule/library_tools.py index 33d79619b0..6aaf8e4135 100644 --- a/common/lib/xmodule/xmodule/library_tools.py +++ b/common/lib/xmodule/xmodule/library_tools.py @@ -16,7 +16,7 @@ class LibraryToolsService(object): def __init__(self, modulestore): self.store = modulestore - def _get_library(self, library_key, version=None): + 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. @@ -28,9 +28,6 @@ class LibraryToolsService(object): if not isinstance(library_key, LibraryLocator): library_key = LibraryLocator.from_string(library_key) - if version: - library_key.for_version(version) - try: return self.store.get_library(library_key, remove_version=False, remove_branch=False) except ItemNotFoundError: @@ -126,8 +123,8 @@ class LibraryToolsService(object): return source_blocks = [] - library_key = dest_block.source_library_key - library = self._get_library(library_key, version=version) + library_key = dest_block.source_library_key.for_version(version) + library = self._get_library(library_key) if library is None: raise ValueError("Requested library not found.") if user_perms and not user_perms.can_read(library_key): From fa95d323c68540c0e3018eeab688de5ab283aedd Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Thu, 2 Apr 2015 21:33:56 +0000 Subject: [PATCH 4/9] Tested version handling, modified modulestore when it didn't work. --- .../contentstore/tests/test_libraries.py | 39 +++++++++++++++++++ cms/djangoapps/contentstore/views/item.py | 2 +- common/lib/xmodule/xmodule/library_tools.py | 18 +++++++-- .../xmodule/modulestore/split_mongo/split.py | 15 +++---- .../modulestore/split_mongo/split_draft.py | 6 ++- 5 files changed, 67 insertions(+), 13 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index b484c74a09..8417cd5ea8 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -747,6 +747,9 @@ class TestOverrides(LibraryTestCase): 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() @@ -862,6 +865,42 @@ class TestOverrides(LibraryTestCase): 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. + """ + self.assertEqual(len(self.library.children), 1) + self.assertEqual(len(self.lc_block.children), 1) + + # Create an additional problem block in the library: + self.problem = ItemFactory.create( + category="problem", + parent_location=self.library.location, + user_id=self.user.id, + publish_item=False, + ) + + # Refresh our reference to the library + store = modulestore() + self.library = store.get_library(self.lib_key) + + # Refresh our reference to the block + self.lc_block = store.get_item(self.lc_block.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) + + duplicate = store.get_item( + _duplicate_item(self.course.location, self.lc_block.location, self.user) + ) + 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) + class TestIncompatibleModuleStore(LibraryTestCase): """ diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 36e1e877fb..10f2bfed56 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -612,7 +612,7 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_ store.update_item(dest_module, user.id) # pylint: disable=protected-access - if ('detached' not in source_item.runtime.load_block_type(category)._class_tags): + 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. # Otherwise, add child to end. diff --git a/common/lib/xmodule/xmodule/library_tools.py b/common/lib/xmodule/xmodule/library_tools.py index 6aaf8e4135..8fcd31b443 100644 --- a/common/lib/xmodule/xmodule/library_tools.py +++ b/common/lib/xmodule/xmodule/library_tools.py @@ -16,7 +16,7 @@ class LibraryToolsService(object): def __init__(self, modulestore): self.store = modulestore - def _get_library(self, library_key): + def _get_library(self, library_key, version=None): """ Given a library key like "library-v1:ProblemX+PR0B", return the 'library' XBlock with meta-information about the library. @@ -28,8 +28,18 @@ class LibraryToolsService(object): if not isinstance(library_key, LibraryLocator): library_key = LibraryLocator.from_string(library_key) + assert library_key.version_guid is None + + if version: + library_key = library_key.for_version(version) + try: - return self.store.get_library(library_key, remove_version=False, remove_branch=False) + library = self.store.get_library( + library_key, remove_version=False, remove_branch=False, head_validation=False + ) + if version: + assert library_key.version_guid == library.location.version_guid + return library except ItemNotFoundError: return None @@ -123,8 +133,8 @@ class LibraryToolsService(object): return source_blocks = [] - library_key = dest_block.source_library_key.for_version(version) - library = self._get_library(library_key) + library_key = dest_block.source_library_key + library = self._get_library(library_key, version=version) if library is None: raise ValueError("Requested library not found.") if user_perms and not user_perms.can_read(library_key): diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 631902c6ad..98a7692ee0 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -790,7 +790,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): else: self.request_cache.data['course_cache'] = {} - def _lookup_course(self, course_key): + def _lookup_course(self, course_key, head_validation=True): """ Decode the locator into the right series of db access. Does not return the CourseDescriptor! It returns the actual db json from @@ -799,11 +799,12 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): Semantics: if course id and branch given, then it will get that branch. If also give a version_guid, it will see if the current head of that branch == that guid. If not it raises VersionConflictError (the version now differs from what it was when you got your - reference) + reference) unless you specify head_validation = False, in which case it will return the + revision (if specified) by the course_key. :param course_key: any subclass of CourseLocator """ - if course_key.org and course_key.course and course_key.run: + if head_validation and course_key.org and course_key.course and course_key.run: if course_key.branch is None: raise InsufficientSpecificationError(course_key) @@ -937,11 +938,11 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): """ return CourseLocator(org, course, run) - def _get_structure(self, structure_id, depth, **kwargs): + def _get_structure(self, structure_id, depth, head_validation=True, **kwargs): """ Gets Course or Library by locator """ - structure_entry = self._lookup_course(structure_id) + structure_entry = self._lookup_course(structure_id, head_validation=head_validation) root = structure_entry.structure['root'] result = self._load_items(structure_entry, [root], depth, **kwargs) return result[0] @@ -955,14 +956,14 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): raise ItemNotFoundError(course_id) return self._get_structure(course_id, depth, **kwargs) - def get_library(self, library_id, depth=0, **kwargs): + def get_library(self, library_id, depth=0, head_validation=True, **kwargs): """ Gets the 'library' root block for the library identified by the locator """ if not isinstance(library_id, LibraryLocator): # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore. raise ItemNotFoundError(library_id) - return self._get_structure(library_id, depth, **kwargs) + return self._get_structure(library_id, depth, head_validation=head_validation, **kwargs) def has_course(self, course_id, ignore_case=False, **kwargs): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index 5830a64f01..64c74bde9f 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -58,7 +58,11 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli course_id = self._map_revision_to_branch(course_id) return super(DraftVersioningModuleStore, self).get_course(course_id, depth=depth, **kwargs) - def get_library(self, library_id, depth=0, **kwargs): + def get_library(self, library_id, depth=0, head_validation=True, **kwargs): + if not head_validation and library_id.version_guid: + return SplitMongoModuleStore.get_library( + self, library_id, depth=depth, head_validation=head_validation, **kwargs + ) library_id = self._map_revision_to_branch(library_id) return super(DraftVersioningModuleStore, self).get_library(library_id, depth=depth, **kwargs) From bf414cf9bbfbec2302eb6f379bb846e293ebf5a9 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Thu, 2 Apr 2015 21:46:33 +0000 Subject: [PATCH 5/9] Redid _get_library one more time, for giggles --- common/lib/xmodule/xmodule/library_tools.py | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/common/lib/xmodule/xmodule/library_tools.py b/common/lib/xmodule/xmodule/library_tools.py index 8fcd31b443..8b800e38d6 100644 --- a/common/lib/xmodule/xmodule/library_tools.py +++ b/common/lib/xmodule/xmodule/library_tools.py @@ -16,7 +16,7 @@ class LibraryToolsService(object): def __init__(self, modulestore): self.store = modulestore - def _get_library(self, library_key, version=None): + 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. @@ -28,18 +28,10 @@ class LibraryToolsService(object): if not isinstance(library_key, LibraryLocator): library_key = LibraryLocator.from_string(library_key) - assert library_key.version_guid is None - - if version: - library_key = library_key.for_version(version) - try: - library = self.store.get_library( + return self.store.get_library( library_key, remove_version=False, remove_branch=False, head_validation=False ) - if version: - assert library_key.version_guid == library.location.version_guid - return library except ItemNotFoundError: return None @@ -133,8 +125,8 @@ class LibraryToolsService(object): return source_blocks = [] - library_key = dest_block.source_library_key - library = self._get_library(library_key, version=version) + library_key = dest_block.source_library_key.for_version(version) + library = self._get_library(library_key) if library is None: raise ValueError("Requested library not found.") if user_perms and not user_perms.can_read(library_key): From 781634558e9cf757c3d7b06469720c685311fb6a Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 2 Apr 2015 18:46:31 -0700 Subject: [PATCH 6/9] Improve the test to show a failure case --- .../contentstore/tests/test_libraries.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index 8417cd5ea8..8a83dfd7cc 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -870,11 +870,15 @@ class TestOverrides(LibraryTestCase): 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: - self.problem = ItemFactory.create( + ItemFactory.create( category="problem", parent_location=self.library.location, user_id=self.user.id, @@ -882,24 +886,30 @@ class TestOverrides(LibraryTestCase): ) # Refresh our reference to the library - store = modulestore() 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): From 8d8fa0faafdcbe67b3a5b7857385046a6d394fd5 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 2 Apr 2015 18:46:57 -0700 Subject: [PATCH 7/9] Partial fix perhaps? --- common/lib/xmodule/xmodule/library_tools.py | 5 ++++- common/lib/xmodule/xmodule/modulestore/split_mongo/split.py | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/common/lib/xmodule/xmodule/library_tools.py b/common/lib/xmodule/xmodule/library_tools.py index 8b800e38d6..d6afb9226b 100644 --- a/common/lib/xmodule/xmodule/library_tools.py +++ b/common/lib/xmodule/xmodule/library_tools.py @@ -4,6 +4,7 @@ XBlock runtime services for LibraryContentModule 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 @@ -125,7 +126,9 @@ class LibraryToolsService(object): return source_blocks = [] - library_key = dest_block.source_library_key.for_version(version) + 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.") diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 98a7692ee0..c78085bc2a 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -2205,7 +2205,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # so that we can access descendant information quickly source_structures = {} for key in source_keys: - course_key = key.course_key.for_version(None) + course_key = key.course_key if course_key.branch is None: raise ItemNotFoundError("branch is required for all source keys when using copy_from_template") if course_key not in source_structures: @@ -2264,9 +2264,9 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): new_children = list() # ordered list of the new children of new_parent_block_key for usage_key in source_keys: - src_course_key = usage_key.course_key.for_version(None) + src_course_key = usage_key.course_key block_key = BlockKey(usage_key.block_type, usage_key.block_id) - source_structure = source_structures.get(src_course_key, []) + source_structure = source_structures[src_course_key] if block_key not in source_structure['blocks']: raise ItemNotFoundError(usage_key) source_block_info = source_structure['blocks'][block_key] From 5bf2c23f595e28f3b8d4c483f421a16ada927868 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Fri, 3 Apr 2015 13:51:03 +0000 Subject: [PATCH 8/9] Fixed issue with block IDs being different across library versions. --- common/lib/xmodule/xmodule/library_tools.py | 5 ++++- .../xmodule/modulestore/split_mongo/split.py | 20 +++++++++++++------ .../modulestore/split_mongo/split_draft.py | 5 ++++- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/common/lib/xmodule/xmodule/library_tools.py b/common/lib/xmodule/xmodule/library_tools.py index d6afb9226b..28a490607c 100644 --- a/common/lib/xmodule/xmodule/library_tools.py +++ b/common/lib/xmodule/xmodule/library_tools.py @@ -144,7 +144,10 @@ class LibraryToolsService(object): 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 diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index c78085bc2a..e76eca743a 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -804,6 +804,8 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): :param course_key: any subclass of CourseLocator """ + if not course_key.version_guid: + head_validation = True if head_validation and course_key.org and course_key.course and course_key.run: if course_key.branch is None: raise InsufficientSpecificationError(course_key) @@ -2171,7 +2173,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): self._update_head(destination_course, index_entry, destination_course.branch, destination_structure['_id']) @contract(source_keys="list(BlockUsageLocator)", dest_usage=BlockUsageLocator) - def copy_from_template(self, source_keys, dest_usage, user_id): + def copy_from_template(self, source_keys, dest_usage, user_id, head_validation=True): """ Flexible mechanism for inheriting content from an external course/library/etc. @@ -2210,7 +2212,9 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): raise ItemNotFoundError("branch is required for all source keys when using copy_from_template") if course_key not in source_structures: with self.bulk_operations(course_key): - source_structures[course_key] = self._lookup_course(course_key).structure + source_structures[course_key] = self._lookup_course( + course_key, head_validation=head_validation + ).structure destination_course = dest_usage.course_key with self.bulk_operations(destination_course): @@ -2227,7 +2231,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # The descendants() method used above adds the block itself, which we don't consider a descendant. orig_descendants.remove(block_key) new_descendants = self._copy_from_template( - source_structures, source_keys, dest_structure, block_key, user_id + source_structures, source_keys, dest_structure, block_key, user_id, head_validation ) # Update the edit info: @@ -2251,7 +2255,9 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): for k in dest_structure['blocks'][block_key].fields['children'] ] - def _copy_from_template(self, source_structures, source_keys, dest_structure, new_parent_block_key, user_id): + def _copy_from_template( + self, source_structures, source_keys, dest_structure, new_parent_block_key, user_id, head_validation + ): """ Internal recursive implementation of copy_from_template() @@ -2265,8 +2271,10 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): for usage_key in source_keys: src_course_key = usage_key.course_key + hashable_source_id = src_course_key.for_version(None) block_key = BlockKey(usage_key.block_type, usage_key.block_id) source_structure = source_structures[src_course_key] + if block_key not in source_structure['blocks']: raise ItemNotFoundError(usage_key) source_block_info = source_structure['blocks'][block_key] @@ -2274,7 +2282,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): # Compute a new block ID. This new block ID must be consistent when this # method is called with the same (source_key, dest_structure) pair unique_data = "{}:{}:{}".format( - unicode(src_course_key).encode("utf-8"), + unicode(hashable_source_id).encode("utf-8"), block_key.id, new_parent_block_key.id, ) @@ -2320,7 +2328,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): if children: children = [src_course_key.make_usage_key(child.type, child.id) for child in children] new_blocks |= self._copy_from_template( - source_structures, children, dest_structure, new_block_key, user_id + source_structures, children, dest_structure, new_block_key, user_id, head_validation ) new_blocks.add(new_block_key) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index 64c74bde9f..22ca4342e3 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -104,7 +104,10 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli """ source_keys = [self._map_revision_to_branch(key) for key in source_keys] dest_key = self._map_revision_to_branch(dest_key) - new_keys = super(DraftVersioningModuleStore, self).copy_from_template(source_keys, dest_key, user_id) + head_validation = kwargs.get('head_validation') + new_keys = super(DraftVersioningModuleStore, self).copy_from_template( + source_keys, dest_key, user_id, head_validation + ) if dest_key.branch == ModuleStoreEnum.BranchName.draft: # Check if any of new_keys or their descendants need to be auto-published. # We don't use _auto_publish_no_children since children may need to be published. From e95e5e70901ab3a13fa79bba50dc8110638a0a44 Mon Sep 17 00:00:00 2001 From: Jonathan Piacenti Date: Fri, 3 Apr 2015 10:40:44 -0500 Subject: [PATCH 9/9] Changed RCB recursive copy function to private function. --- .../xmodule/xmodule/library_content_module.py | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index bbe696fd3f..fb2723b880 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -343,9 +343,26 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe """ user_perms = self.runtime.service(self, 'studio_user_permissions') 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 @@ -359,24 +376,11 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe 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) - # Copy over any overridden settings the course author may have applied to the blocks. - def copy_overrides(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): - copy_overrides(source_child, dest_child) - store.update_item(dest, user_id) - - copy_overrides(source_block, self) + self._copy_overrides(store, user_id, source_block, self) # Children have been handled. return True