Skip to content

E
edx-platform
Commit 5795ba3f by Kelketek
Options

Merge pull request #7482 from open-craft/fix_duplicate_rcb 

Fix duplication of Randomized Content Blocks
parents e469e4da e95e5e70
Showing with 232 additions and 50 deletions
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
......@@ -745,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()
......@@ -822,7 +827,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 +840,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)
......@@ -852,6 +865,52 @@ 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.
"""
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 @@ messages = json.dumps(xblock.validate().to_json())
</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 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,6 +308,25 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe
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 @@ 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')
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 @@ 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)
......
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
......@@ -21,14 +22,17 @@ class LibraryToolsService(object):
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 @@ 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,6 +127,8 @@ class LibraryToolsService(object):
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 @@ 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
......
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,14 @@ 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 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)
......@@ -937,11 +940,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 +958,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):
"""
......@@ -2170,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.
......@@ -2204,12 +2207,14 @@ 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:
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):
......@@ -2226,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:
......@@ -2250,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()
......@@ -2263,9 +2270,11 @@ 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
hashable_source_id = src_course_key.for_version(None)
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]
......@@ -2273,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,
)
......@@ -2319,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)
......
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)
......@@ -100,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.
......
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)
......
common/test/acceptance/pages/studio/container.py
......@@ -407,6 +407,20 @@ class XBlockWrapper(PageObject):
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):
"""
Returns true if this xblock has an 'edit visibility' button
......
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)
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment