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

We change the data as well to check that non-overriden fields do get updated.

" + self.problem.data = new_data_value + modulestore().update_item(self.problem, self.user.id) + + self.lc_block = self._refresh_children(self.lc_block) + self.problem_in_course = modulestore().get_item(self.problem_in_course.location) + + self.assertEqual(self.problem_in_course.display_name, new_display_name) + self.assertEqual(self.problem_in_course.weight, new_weight) + self.assertEqual(self.problem_in_course.data, new_data_value) diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index 1b68e27a59..9cedc5bbff 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -323,7 +323,7 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe js_module_name = "VerticalDescriptor" @XBlock.handler - def refresh_children(self, request=None, suffix=None, update_db=True): # pylint: disable=unused-argument + def refresh_children(self, request=None, suffix=None): # pylint: disable=unused-argument """ Refresh children: This method is to be used when any of the libraries that this block @@ -335,15 +335,12 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe This method will update this block's 'source_libraries' field to store the version number of the libraries used, so we easily determine if this block is up to date or not. - - If update_db is True (default), this will explicitly persist the changes - to the modulestore by calling update_item() """ lib_tools = self.runtime.service(self, 'library_tools') user_service = self.runtime.service(self, 'user') user_perms = self.runtime.service(self, 'studio_user_permissions') user_id = user_service.user_id if user_service else None # May be None when creating bok choy test fixtures - lib_tools.update_children(self, user_id, user_perms, update_db) + lib_tools.update_children(self, user_id, user_perms) return Response() def _validate_library_version(self, validation, lib_tools, version, library_key): @@ -451,7 +448,7 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe if (set(old_source_libraries) != set(self.source_libraries) or old_metadata.get('capa_type', ANY_CAPA_TYPE_VALUE) != self.capa_type): try: - self.refresh_children(None, None, update_db=False) # update_db=False since update_item() is about to be called anyways + self.refresh_children() except ValueError: pass # The validation area will display an error message, no need to do anything now. diff --git a/common/lib/xmodule/xmodule/library_tools.py b/common/lib/xmodule/xmodule/library_tools.py index f9b9f3edab..4bdf51bdce 100644 --- a/common/lib/xmodule/xmodule/library_tools.py +++ b/common/lib/xmodule/xmodule/library_tools.py @@ -1,10 +1,8 @@ """ XBlock runtime services for LibraryContentModule """ -import hashlib from django.core.exceptions import PermissionDenied from opaque_keys.edx.locator import LibraryLocator -from xblock.fields import Scope from xmodule.library_content_module import LibraryVersionReference, ANY_CAPA_TYPE_VALUE from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.capa_module import CapaDescriptor @@ -60,7 +58,7 @@ class LibraryToolsService(object): assert isinstance(descriptor, CapaDescriptor) return capa_type in descriptor.problem_types - def update_children(self, dest_block, user_id, user_perms=None, update_db=True): + def update_children(self, dest_block, user_id, user_perms=None): """ This method is to be used when any of the libraries that a LibraryContentModule references have been updated. It will re-fetch all matching blocks from @@ -71,82 +69,28 @@ class LibraryToolsService(object): This method will update dest_block's 'source_libraries' field to store the version number of the libraries used, so we easily determine if dest_block is up to date or not. - - If update_db is True (default), this will explicitly persist the changes - to the modulestore by calling update_item(). Only set update_db False if - you know for sure that dest_block is about to be saved to the modulestore - anyways. Otherwise, orphaned blocks may be created. """ - root_children = [] if user_perms and not user_perms.can_write(dest_block.location.course_key): raise PermissionDenied() + new_libraries = [] + source_blocks = [] + for library_key, __ in dest_block.source_libraries: + library = self._get_library(library_key) + if library is None: + raise ValueError("Required library not found.") + if user_perms and not user_perms.can_read(library_key): + raise PermissionDenied() + filter_children = (dest_block.capa_type != ANY_CAPA_TYPE_VALUE) + if filter_children: + # Apply simple filtering based on CAPA problem types: + source_blocks.extend([key for key in library.children if self._filter_child(key, dest_block.capa_type)]) + else: + source_blocks.extend(library.children) + new_libraries.append(LibraryVersionReference(library_key, library.location.library_key.version_guid)) + with self.store.bulk_operations(dest_block.location.course_key): - # Currently, ALL children are essentially deleted and then re-added - # in a way that preserves their block_ids (and thus should preserve - # student data, grades, analytics, etc.) - # Once course-level field overrides are implemented, this will - # change to a more conservative implementation. - - # First, load and validate the source_libraries: - libraries = [] - for library_key, old_version in dest_block.source_libraries: # pylint: disable=unused-variable - library = self._get_library(library_key) - if library is None: - raise ValueError("Required library not found.") - if user_perms and not user_perms.can_read(library_key): - raise PermissionDenied() - libraries.append((library_key, library)) - - # Next, delete all our existing children to avoid block_id conflicts when we add them: - for child in dest_block.children: - self.store.delete_item(child, user_id) - - # Now add all matching children, and record the library version we use: - new_libraries = [] - for library_key, library in libraries: - - def copy_children_recursively(from_block, filter_problem_type=False): - """ - Internal method to copy blocks from the library recursively - """ - new_children = [] - if filter_problem_type: - filtered_children = [key for key in from_block.children if self._filter_child(key, dest_block.capa_type)] - else: - filtered_children = from_block.children - for child_key in filtered_children: - child = self.store.get_item(child_key, depth=None) - # We compute a block_id for each matching child block found in the library. - # block_ids are unique within any branch, but are not unique per-course or globally. - # We need our block_ids to be consistent when content in the library is updated, so - # we compute block_id as a hash of three pieces of data: - unique_data = "{}:{}:{}".format( - dest_block.location.block_id, # Must not clash with other usages of the same library in this course - unicode(library_key.for_version(None)).encode("utf-8"), # The block ID below is only unique within a library, so we need this too - child_key.block_id, # Child block ID. Should not change even if the block is edited. - ) - child_block_id = hashlib.sha1(unique_data).hexdigest()[:20] - fields = {} - for field in child.fields.itervalues(): - if field.scope == Scope.settings and field.is_set_on(child): - fields[field.name] = field.read_from(child) - if child.has_children: - fields['children'] = copy_children_recursively(from_block=child) - new_child_info = self.store.create_item( - user_id, - dest_block.location.course_key, - child_key.block_type, - block_id=child_block_id, - definition_locator=child.definition_locator, - runtime=dest_block.system, - fields=fields, - ) - new_children.append(new_child_info.location) - return new_children - root_children.extend(copy_children_recursively(from_block=library, filter_problem_type=True)) - new_libraries.append(LibraryVersionReference(library_key, library.location.library_key.version_guid)) dest_block.source_libraries = new_libraries - dest_block.children = root_children - if update_db: - self.store.update_item(dest_block, user_id) + self.store.update_item(dest_block, user_id) + dest_block.children = self.store.copy_from_template(source_blocks, dest_block.location, user_id) + # ^-- copy_from_template updates the children in the DB but we must also set .children here to avoid overwriting the DB again diff --git a/common/lib/xmodule/xmodule/modulestore/inheritance.py b/common/lib/xmodule/xmodule/modulestore/inheritance.py index 296fdb80ca..c0e3e1b7f1 100644 --- a/common/lib/xmodule/xmodule/modulestore/inheritance.py +++ b/common/lib/xmodule/xmodule/modulestore/inheritance.py @@ -283,6 +283,8 @@ class InheritanceKeyValueStore(KeyValueStore): def default(self, key): """ - Check to see if the default should be from inheritance rather than from the field's global default + Check to see if the default should be from inheritance. If not + inheriting, this will raise KeyError which will cause the caller to use + the field's global default. """ return self.inherited_settings[key.field_name] diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 24e68ea143..6175427659 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -676,6 +676,14 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): store = self._verify_modulestore_support(course_key, 'import_xblock') return store.import_xblock(user_id, course_key, block_type, block_id, fields, runtime) + @strip_key + def copy_from_template(self, source_keys, dest_key, user_id, **kwargs): + """ + See :py:meth `SplitMongoModuleStore.copy_from_template` + """ + store = self._verify_modulestore_support(dest_key.course_key, 'copy_from_template') + return store.copy_from_template(source_keys, dest_key, user_id) + @strip_key def update_item(self, xblock, user_id, allow_not_found=False, **kwargs): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index 3c357c8751..73a950b355 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -169,16 +169,17 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): if block_key is None: block_key = BlockKey(json_data['block_type'], LocalId()) + convert_fields = lambda field: self.modulestore.convert_references_to_keys( + course_key, class_, field, self.course_entry.structure['blocks'], + ) + if definition_id is not None and not json_data.get('definition_loaded', False): definition_loader = DefinitionLazyLoader( self.modulestore, course_key, block_key.type, definition_id, - lambda fields: self.modulestore.convert_references_to_keys( - course_key, self.load_block_type(block_key.type), - fields, self.course_entry.structure['blocks'], - ) + convert_fields, ) else: definition_loader = None @@ -193,9 +194,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): block_id=block_key.id, ) - converted_fields = self.modulestore.convert_references_to_keys( - block_locator.course_key, class_, json_data.get('fields', {}), self.course_entry.structure['blocks'], - ) + converted_fields = convert_fields(json_data.get('fields', {})) + converted_defaults = convert_fields(json_data.get('defaults', {})) if block_key in self._parent_map: parent_key = self._parent_map[block_key] parent = course_key.make_usage_key(parent_key.type, parent_key.id) @@ -204,6 +204,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): kvs = SplitMongoKVS( definition_loader, converted_fields, + converted_defaults, parent=parent, field_decorator=kwargs.get('field_decorator') ) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 5cb62bcf57..3d809f7981 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -27,6 +27,8 @@ Representation: **** 'definition': the db id of the record containing the content payload for this xblock **** 'fields': the Scope.settings and children field values ***** 'children': This is stored as a list of (block_type, block_id) pairs + **** 'defaults': Scope.settings default values copied from a template block (used e.g. when + blocks are copied from a library to a course) **** 'edit_info': dictionary: ***** 'edited_on': when was this xblock's fields last changed (will be edited_on value of update_version structure) @@ -53,6 +55,7 @@ Representation: import copy import threading import datetime +import hashlib import logging from contracts import contract, new_contract from importlib import import_module @@ -691,12 +694,9 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): for block in new_module_data.itervalues(): if block['definition'] in definitions: - converted_fields = self.convert_references_to_keys( - course_key, system.load_block_type(block['block_type']), - definitions[block['definition']].get('fields'), - system.course_entry.structure['blocks'], - ) - block['fields'].update(converted_fields) + definition = definitions[block['definition']] + # convert_fields was being done here, but it gets done later in the runtime's xblock_from_json + block['fields'].update(definition.get('fields')) block['definition_loaded'] = True system.module_data.update(new_module_data) @@ -2071,6 +2071,144 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): self.update_structure(destination_course, destination_structure) 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): + """ + Flexible mechanism for inheriting content from an external course/library/etc. + + Will copy all of the XBlocks whose keys are passed as `source_course` so that they become + children of the XBlock whose key is `dest_usage`. Any previously existing children of + `dest_usage` that haven't been replaced/updated by this copy_from_template operation will + be deleted. + + Unlike `copy()`, this does not care whether the resulting blocks are positioned similarly + in their new course/library. However, the resulting blocks will be in the same relative + order as `source_keys`. + + If any of the blocks specified already exist as children of the destination block, they + will be updated rather than duplicated or replaced. If they have Scope.settings field values + overriding inherited default values, those overrides will be preserved. + + IMPORTANT: This method does not preserve block_id - in other words, every block that is + copied will be assigned a new block_id. This is because we assume that the same source block + may be copied into one course in multiple places. However, it *is* guaranteed that every + time this method is called for the same source block and dest_usage, the same resulting + block id will be generated. + + :param source_keys: a list of BlockUsageLocators. Order is preserved. + + :param dest_usage: The BlockUsageLocator that will become the parent of an inherited copy + of all the xblocks passed in `source_keys`. + + :param user_id: The user who will get credit for making this change. + """ + # Preload the block structures for all source courses/libraries/etc. + # so that we can access descendant information quickly + source_structures = {} + for key in source_keys: + course_key = key.course_key.for_version(None) + 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 + + destination_course = dest_usage.course_key + with self.bulk_operations(destination_course): + index_entry = self.get_course_index(destination_course) + if index_entry is None: + raise ItemNotFoundError(destination_course) + dest_structure = self._lookup_course(destination_course).structure + old_dest_structure_version = dest_structure['_id'] + dest_structure = self.version_structure(destination_course, dest_structure, user_id) + + # Set of all descendent block IDs of dest_usage that are to be replaced: + block_key = BlockKey(dest_usage.block_type, dest_usage.block_id) + orig_descendants = set(self.descendants(dest_structure['blocks'], block_key, depth=None, descendent_map={})) + orig_descendants.remove(block_key) # The descendants() method used above adds the block itself, which we don't consider a descendant. + new_descendants = self._copy_from_template(source_structures, source_keys, dest_structure, block_key, user_id) + + # Update the edit info: + dest_info = dest_structure['blocks'][block_key] + + # Update the edit_info: + dest_info['edit_info']['previous_version'] = dest_info['edit_info']['update_version'] + dest_info['edit_info']['update_version'] = old_dest_structure_version + dest_info['edit_info']['edited_by'] = user_id + dest_info['edit_info']['edited_on'] = datetime.datetime.now(UTC) + + orphans = orig_descendants - new_descendants + for orphan in orphans: + del dest_structure['blocks'][orphan] + + self.update_structure(destination_course, dest_structure) + self._update_head(destination_course, index_entry, destination_course.branch, dest_structure['_id']) + # Return usage locators for all the new children: + return [destination_course.make_usage_key(*k) 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): + """ + Internal recursive implementation of copy_from_template() + + Returns the new set of BlockKeys that are the new descendants of the block with key 'block_key' + """ + # pylint: disable=no-member + # ^-- Until pylint gets namedtuple support, it will give warnings about BlockKey attributes + new_blocks = set() + + 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) + block_key = BlockKey(usage_key.block_type, usage_key.block_id) + source_structure = source_structures.get(src_course_key, []) + if block_key not in source_structure['blocks']: + raise ItemNotFoundError(usage_key) + source_block_info = source_structure['blocks'][block_key] + + # 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"), + block_key.id, + new_parent_block_key.id, + ) + new_block_id = hashlib.sha1(unique_data).hexdigest()[:20] + new_block_key = BlockKey(block_key.type, new_block_id) + + # Now clone block_key to new_block_key: + new_block_info = copy.deepcopy(source_block_info) + # Note that new_block_info now points to the same definition ID entry as source_block_info did + existing_block_info = dest_structure['blocks'].get(new_block_key, {}) + # Inherit the Scope.settings values from 'fields' to 'defaults' + new_block_info['defaults'] = new_block_info['fields'] + new_block_info['fields'] = existing_block_info.get('fields', {}) # Preserve any existing overrides + if 'children' in new_block_info['defaults']: + del new_block_info['defaults']['children'] # Will be set later + new_block_info['block_id'] = new_block_key.id + new_block_info['edit_info'] = existing_block_info.get('edit_info', {}) + new_block_info['edit_info']['previous_version'] = new_block_info['edit_info'].get('update_version', None) + new_block_info['edit_info']['update_version'] = dest_structure['_id'] + # Note we do not set 'source_version' - it's only used for copying identical blocks from draft to published as part of publishing workflow. + # Setting it to the source_block_info structure version here breaks split_draft's has_changes() method. + new_block_info['edit_info']['edited_by'] = user_id + new_block_info['edit_info']['edited_on'] = datetime.datetime.now(UTC) + dest_structure['blocks'][new_block_key] = new_block_info + + children = source_block_info['fields'].get('children') + 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) + + new_blocks.add(new_block_key) + # And add new_block_key to the list of new_parent_block_key's new children: + new_children.append(new_block_key) + + # Update the children of new_parent_block_key + dest_structure['blocks'][new_parent_block_key]['fields']['children'] = new_children + + return new_blocks + def delete_item(self, usage_locator, user_id, force=False): """ Delete the block or tree rooted at block (if delete_children) and any references w/in the course to the block 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 8870c89ee6..c651512c82 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -93,6 +93,26 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli # version_agnostic b/c of above assumption in docstring self.publish(location.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs) + def copy_from_template(self, source_keys, dest_key, user_id, **kwargs): + """ + See :py:meth `SplitMongoModuleStore.copy_from_template` + """ + 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) + 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. + with self.bulk_operations(dest_key.course_key): + keys_to_check = list(new_keys) + while keys_to_check: + usage_key = keys_to_check.pop() + if usage_key.category in DIRECT_ONLY_CATEGORIES: + self.publish(usage_key.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs) + children = getattr(self.get_item(usage_key, **kwargs), "children", []) + keys_to_check.extend(children) # e.g. if usage_key is a chapter, it may have an auto-publish sequential child + return new_keys + def update_item(self, descriptor, user_id, allow_not_found=False, force=False, **kwargs): old_descriptor_locn = descriptor.location descriptor.location = self._map_revision_to_branch(old_descriptor_locn) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py index 0cfa672143..2124732242 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py @@ -19,17 +19,20 @@ class SplitMongoKVS(InheritanceKeyValueStore): """ @contract(parent="BlockUsageLocator | None") - def __init__(self, definition, initial_values, parent, field_decorator=None): + def __init__(self, definition, initial_values, default_values, parent, field_decorator=None): """ :param definition: either a lazyloader or definition id for the definition :param initial_values: a dictionary of the locally set values + :param default_values: any Scope.settings field defaults that are set locally + (copied from a template block with copy_from_template) """ # deepcopy so that manipulations of fields does not pollute the source super(SplitMongoKVS, self).__init__(copy.deepcopy(initial_values)) self._definition = definition # either a DefinitionLazyLoader or the db id of the definition. # if the db id, then the definition is presumed to be loaded into _fields + self._defaults = default_values # a decorator function for field values (to be called when a field is accessed) if field_decorator is None: self.field_decorator = lambda x: x @@ -110,6 +113,16 @@ class SplitMongoKVS(InheritanceKeyValueStore): # if someone changes it so that they do, then change any tests of field.name in xx._field_data return key.field_name in self._fields + def default(self, key): + """ + Check to see if the default should be from the template's defaults (if any) + rather than the global default or inheritance. + """ + if self._defaults and key.field_name in self._defaults: + return self._defaults[key.field_name] + # If not, try inheriting from a parent, then use the XBlock type's normal default value: + return super(SplitMongoKVS, self).default(key) + def _load_definition(self): """ Update fields w/ the lazily loaded definitions diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py b/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py index ef8a6d4d69..323fd23301 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py @@ -10,8 +10,9 @@ from mock import patch from opaque_keys.edx.locator import LibraryLocator from xblock.fragment import Fragment from xblock.runtime import Runtime as VanillaRuntime -from xmodule.modulestore.exceptions import DuplicateCourseError -from xmodule.modulestore.tests.factories import LibraryFactory, ItemFactory, check_mongo_calls +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.exceptions import DuplicateCourseError, ItemNotFoundError +from xmodule.modulestore.tests.factories import CourseFactory, LibraryFactory, ItemFactory, check_mongo_calls from xmodule.modulestore.tests.utils import MixedSplitTestCase from xmodule.x_module import AUTHOR_VIEW @@ -217,3 +218,142 @@ class TestLibraries(MixedSplitTestCase): modulestore=self.store, ) self.assertFalse(self.store.has_published_version(block)) + + +@ddt.ddt +class TestSplitCopyTemplate(MixedSplitTestCase): + """ + Test for split's copy_from_template method. + Currently it is only used for content libraries. + However for this test, we make sure it also works when copying from course to course. + """ + @ddt.data( + LibraryFactory, + CourseFactory, + ) + def test_copy_from_template(self, source_type): + """ + Test that the behavior of copy_from_template() matches its docstring + """ + source_container = source_type.create(modulestore=self.store) # Either a library or a course + course = CourseFactory.create(modulestore=self.store) + # Add a vertical with a capa child to the source library/course: + vertical_block = ItemFactory.create( + category="vertical", + parent_location=source_container.location, + user_id=self.user_id, + publish_item=False, + modulestore=self.store, + ) + problem_library_display_name = "Problem Library Display Name" + problem_block = ItemFactory.create( + category="problem", + parent_location=vertical_block.location, + user_id=self.user_id, + publish_item=False, + modulestore=self.store, + display_name=problem_library_display_name, + markdown="Problem markdown here" + ) + + if source_type == LibraryFactory: + source_container = self.store.get_library(source_container.location.library_key, remove_version=False, remove_branch=False) + else: + source_container = self.store.get_course(source_container.location.course_key, remove_version=False, remove_branch=False) + + # Inherit the vertical and the problem from the library into the course: + source_keys = [source_container.children[0]] + new_blocks = self.store.copy_from_template(source_keys, dest_key=course.location, user_id=self.user_id) + self.assertEqual(len(new_blocks), 1) + + course = self.store.get_course(course.location.course_key) # Reload from modulestore + + self.assertEqual(len(course.children), 1) + vertical_block_course = self.store.get_item(course.children[0]) + self.assertEqual(new_blocks[0], vertical_block_course.location) + problem_block_course = self.store.get_item(vertical_block_course.children[0]) + self.assertEqual(problem_block_course.display_name, problem_library_display_name) + + # Override the display_name and weight: + new_display_name = "The Trouble with Tribbles" + new_weight = 20 + problem_block_course.display_name = new_display_name + problem_block_course.weight = new_weight + self.store.update_item(problem_block_course, self.user_id) + + # Test that "Any previously existing children of `dest_usage` that haven't been replaced/updated by this copy_from_template operation will be deleted." + extra_block = ItemFactory.create( + category="html", + parent_location=vertical_block_course.location, + user_id=self.user_id, + publish_item=False, + modulestore=self.store, + ) + + # Repeat the copy_from_template(): + new_blocks2 = self.store.copy_from_template(source_keys, dest_key=course.location, user_id=self.user_id) + self.assertEqual(new_blocks, new_blocks2) + # Reload problem_block_course: + problem_block_course = self.store.get_item(problem_block_course.location) + self.assertEqual(problem_block_course.display_name, new_display_name) + self.assertEqual(problem_block_course.weight, new_weight) + + # Ensure that extra_block was deleted: + vertical_block_course = self.store.get_item(new_blocks2[0]) + self.assertEqual(len(vertical_block_course.children), 1) + with self.assertRaises(ItemNotFoundError): + self.store.get_item(extra_block.location) + + def test_copy_from_template_auto_publish(self): + """ + Make sure that copy_from_template works with things like 'chapter' that + are always auto-published. + """ + source_course = CourseFactory.create(modulestore=self.store) + course = CourseFactory.create(modulestore=self.store) + make_block = lambda category, parent: ItemFactory.create(category=category, parent_location=parent.location, user_id=self.user_id, modulestore=self.store) + + # Populate the course: + about = make_block("about", source_course) + chapter = make_block("chapter", source_course) + sequential = make_block("sequential", chapter) + # And three blocks that are NOT auto-published: + vertical = make_block("vertical", sequential) + make_block("problem", vertical) + html = make_block("html", source_course) + + # Reload source_course since we need its branch and version to use copy_from_template: + source_course = self.store.get_course(source_course.location.course_key, remove_version=False, remove_branch=False) + + # Inherit the vertical and the problem from the library into the course: + source_keys = [block.location for block in [about, chapter, html]] + block_keys = self.store.copy_from_template(source_keys, dest_key=course.location, user_id=self.user_id) + self.assertEqual(len(block_keys), len(source_keys)) + + # Build dict of the new blocks in 'course', keyed by category (which is a unique key in our case) + new_blocks = {} + block_keys = set(block_keys) + while block_keys: + key = block_keys.pop() + block = self.store.get_item(key) + new_blocks[block.category] = block + block_keys.update(set(getattr(block, "children", []))) + + # Check that auto-publish blocks with no children are indeed published: + def published_version_exists(block): + """ Does a published version of block exist? """ + try: + self.store.get_item(block.location.for_branch(ModuleStoreEnum.BranchName.published)) + return True + except ItemNotFoundError: + return False + + # Check that the auto-publish blocks have been published: + self.assertFalse(self.store.has_changes(new_blocks["about"])) + self.assertTrue(published_version_exists(new_blocks["chapter"])) # We can't use has_changes because it includes descendants + self.assertTrue(published_version_exists(new_blocks["sequential"])) # Ditto + # Check that non-auto-publish blocks and blocks with non-auto-publish descendants show changes: + self.assertTrue(self.store.has_changes(new_blocks["html"])) + self.assertTrue(self.store.has_changes(new_blocks["problem"])) + self.assertTrue(self.store.has_changes(new_blocks["chapter"])) # Will have changes since a child block has changes. + self.assertFalse(published_version_exists(new_blocks["vertical"])) # Verify that our published_version_exists works diff --git a/common/lib/xmodule/xmodule/tests/test_library_content.py b/common/lib/xmodule/xmodule/tests/test_library_content.py index fec957d0cd..b92404df10 100644 --- a/common/lib/xmodule/xmodule/tests/test_library_content.py +++ b/common/lib/xmodule/xmodule/tests/test_library_content.py @@ -117,7 +117,8 @@ class TestLibraries(MixedSplitTestCase): # is updated, but the way we do it through a factory doesn't do that. self.assertEqual(len(self.lc_block.children), 0) # Update the LibraryContent module: - self.lc_block.refresh_children(None, None) + self.lc_block.refresh_children() + self.lc_block = self.store.get_item(self.lc_block.location) # Check that all blocks from the library are now children of the block: self.assertEqual(len(self.lc_block.children), len(self.lib_blocks)) @@ -125,7 +126,7 @@ class TestLibraries(MixedSplitTestCase): """ Test that each student sees only one block as a child of the LibraryContent block. """ - self.lc_block.refresh_children(None, None) + self.lc_block.refresh_children() self.lc_block = self.store.get_item(self.lc_block.location) self._bind_course_module(self.lc_block) # Make sure the runtime knows that the block's children vary per-user: diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 00487d4848..770d6429ad 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1250,6 +1250,7 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): # p :param xblock: :param field: """ + # pylint: disable=protected-access # in runtime b/c runtime contains app-specific xblock behavior. Studio's the only app # which needs this level of introspection right now. runtime also is 'allowed' to know # about the kvs, dbmodel, etc. @@ -1257,12 +1258,8 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime): # p result = {} result['explicitly_set'] = xblock._field_data.has(xblock, field.name) try: - block_inherited = xblock.xblock_kvs.inherited_settings - except AttributeError: # if inherited_settings doesn't exist on kvs - block_inherited = {} - if field.name in block_inherited: - result['default_value'] = block_inherited[field.name] - else: + result['default_value'] = xblock._field_data.default(xblock, field.name) + except KeyError: result['default_value'] = field.to_json(field.default) return result