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

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/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 32d3fc49b269..773bcf18c75b 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -559,7 +559,10 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_ category = dest_usage_key.block_type # Update the display name to indicate this is a duplicate (unless display name provided). - duplicate_metadata = own_metadata(source_item) + duplicate_metadata = {} # Can't use own_metadata(), b/c it converts data for JSON serialization - not suitable for setting metadata of the new block + for field in source_item.fields.values(): + if (field.scope == Scope.settings and field.is_set_on(source_item)): + duplicate_metadata[field.name] = field.read_from(source_item) if display_name is not None: duplicate_metadata['display_name'] = display_name else: @@ -584,7 +587,8 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_ dest_module.children = [] for child in source_item.children: dupe = _duplicate_item(dest_module.location, child, user=user) - dest_module.children.append(dupe) + if dupe not in dest_module.children: # _duplicate_item may add the child for us. + dest_module.children.append(dupe) store.update_item(dest_module, user.id) if 'detached' not in source_item.runtime.load_block_type(category)._class_tags: diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py index 1b68e27a59c2..9cedc5bbff40 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 @@ def refresh_children(self, request=None, suffix=None, update_db=True): # pylint This method will update this block's 'source_libraries' field to store the version number of the libraries used, so we easily determine if this block is up to date or not. - - If update_db is True (default), this will explicitly persist the changes - to the modulestore by calling update_item() """ lib_tools = self.runtime.service(self, 'library_tools') user_service = self.runtime.service(self, 'user') user_perms = self.runtime.service(self, 'studio_user_permissions') user_id = user_service.user_id if user_service else None # May be None when creating bok choy test fixtures - lib_tools.update_children(self, user_id, user_perms, update_db) + lib_tools.update_children(self, user_id, user_perms) return Response() def _validate_library_version(self, validation, lib_tools, version, library_key): @@ -451,7 +448,7 @@ def editor_saved(self, user, old_metadata, old_content): if (set(old_source_libraries) != set(self.source_libraries) or old_metadata.get('capa_type', ANY_CAPA_TYPE_VALUE) != self.capa_type): try: - self.refresh_children(None, None, update_db=False) # update_db=False since update_item() is about to be called anyways + self.refresh_children() except ValueError: pass # The validation area will display an error message, no need to do anything now. diff --git a/common/lib/xmodule/xmodule/library_tools.py b/common/lib/xmodule/xmodule/library_tools.py index f9b9f3edab83..4bdf51bdce53 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 @@ def _filter_child(self, usage_key, capa_type): assert isinstance(descriptor, CapaDescriptor) return capa_type in descriptor.problem_types - def update_children(self, dest_block, user_id, user_perms=None, update_db=True): + def update_children(self, dest_block, user_id, user_perms=None): """ This method is to be used when any of the libraries that a LibraryContentModule references have been updated. It will re-fetch all matching blocks from @@ -71,82 +69,28 @@ def update_children(self, dest_block, user_id, user_perms=None, update_db=True): This method will update dest_block's 'source_libraries' field to store the version number of the libraries used, so we easily determine if dest_block is up to date or not. - - If update_db is True (default), this will explicitly persist the changes - to the modulestore by calling update_item(). Only set update_db False if - you know for sure that dest_block is about to be saved to the modulestore - anyways. Otherwise, orphaned blocks may be created. """ - root_children = [] if user_perms and not user_perms.can_write(dest_block.location.course_key): raise PermissionDenied() - with self.store.bulk_operations(dest_block.location.course_key): - # Currently, ALL children are essentially deleted and then re-added - # in a way that preserves their block_ids (and thus should preserve - # student data, grades, analytics, etc.) - # Once course-level field overrides are implemented, this will - # change to a more conservative implementation. - - # First, load and validate the source_libraries: - libraries = [] - for library_key, old_version in dest_block.source_libraries: # pylint: disable=unused-variable - library = self._get_library(library_key) - if library is None: - raise ValueError("Required library not found.") - if user_perms and not user_perms.can_read(library_key): - raise PermissionDenied() - libraries.append((library_key, library)) + new_libraries = [] + source_blocks = [] + for library_key, __ in dest_block.source_libraries: + library = self._get_library(library_key) + if library is None: + raise ValueError("Required library not found.") + if user_perms and not user_perms.can_read(library_key): + raise PermissionDenied() + filter_children = (dest_block.capa_type != ANY_CAPA_TYPE_VALUE) + if filter_children: + # Apply simple filtering based on CAPA problem types: + source_blocks.extend([key for key in library.children if self._filter_child(key, dest_block.capa_type)]) + else: + source_blocks.extend(library.children) + new_libraries.append(LibraryVersionReference(library_key, library.location.library_key.version_guid)) - # Next, delete all our existing children to avoid block_id conflicts when we add them: - for child in dest_block.children: - self.store.delete_item(child, user_id) - - # Now add all matching children, and record the library version we use: - new_libraries = [] - for library_key, library in libraries: - - def copy_children_recursively(from_block, filter_problem_type=False): - """ - Internal method to copy blocks from the library recursively - """ - new_children = [] - if filter_problem_type: - filtered_children = [key for key in from_block.children if self._filter_child(key, dest_block.capa_type)] - else: - filtered_children = from_block.children - for child_key in filtered_children: - child = self.store.get_item(child_key, depth=None) - # We compute a block_id for each matching child block found in the library. - # block_ids are unique within any branch, but are not unique per-course or globally. - # We need our block_ids to be consistent when content in the library is updated, so - # we compute block_id as a hash of three pieces of data: - unique_data = "{}:{}:{}".format( - dest_block.location.block_id, # Must not clash with other usages of the same library in this course - unicode(library_key.for_version(None)).encode("utf-8"), # The block ID below is only unique within a library, so we need this too - child_key.block_id, # Child block ID. Should not change even if the block is edited. - ) - child_block_id = hashlib.sha1(unique_data).hexdigest()[:20] - fields = {} - for field in child.fields.itervalues(): - if field.scope == Scope.settings and field.is_set_on(child): - fields[field.name] = field.read_from(child) - if child.has_children: - fields['children'] = copy_children_recursively(from_block=child) - new_child_info = self.store.create_item( - user_id, - dest_block.location.course_key, - child_key.block_type, - block_id=child_block_id, - definition_locator=child.definition_locator, - runtime=dest_block.system, - fields=fields, - ) - new_children.append(new_child_info.location) - return new_children - root_children.extend(copy_children_recursively(from_block=library, filter_problem_type=True)) - new_libraries.append(LibraryVersionReference(library_key, library.location.library_key.version_guid)) + with self.store.bulk_operations(dest_block.location.course_key): dest_block.source_libraries = new_libraries - dest_block.children = root_children - if update_db: - self.store.update_item(dest_block, user_id) + self.store.update_item(dest_block, user_id) + dest_block.children = self.store.copy_from_template(source_blocks, dest_block.location, user_id) + # ^-- copy_from_template updates the children in the DB but we must also set .children here to avoid overwriting the DB again diff --git a/common/lib/xmodule/xmodule/modulestore/inheritance.py b/common/lib/xmodule/xmodule/modulestore/inheritance.py index 296fdb80caa6..3ec2f96dbd14 100644 --- a/common/lib/xmodule/xmodule/modulestore/inheritance.py +++ b/common/lib/xmodule/xmodule/modulestore/inheritance.py @@ -211,8 +211,8 @@ def inherit_metadata(descriptor, inherited_data): def own_metadata(module): """ - Return a dictionary that contains only non-inherited field keys, - mapped to their serialized values + Return a JSON-friendly dictionary that contains only non-inherited field + keys, mapped to their serialized values """ return module.get_explicitly_set_fields_by_scope(Scope.settings) @@ -283,6 +283,8 @@ def has(self, key): def default(self, key): """ - Check to see if the default should be from inheritance rather than from the field's global default + Check to see if the default should be from inheritance. If not + inheriting, this will raise KeyError which will cause the caller to use + the field's global default. """ return self.inherited_settings[key.field_name] diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 24e68ea14308..617542765975 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -676,6 +676,14 @@ def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, store = self._verify_modulestore_support(course_key, 'import_xblock') return store.import_xblock(user_id, course_key, block_type, block_id, fields, runtime) + @strip_key + def copy_from_template(self, source_keys, dest_key, user_id, **kwargs): + """ + See :py:meth `SplitMongoModuleStore.copy_from_template` + """ + store = self._verify_modulestore_support(dest_key.course_key, 'copy_from_template') + return store.copy_from_template(source_keys, dest_key, user_id) + @strip_key def update_item(self, xblock, user_id, allow_not_found=False, **kwargs): """ 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 3c357c87517e..73a950b3558d 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 @@ def xblock_from_json( 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 @@ def xblock_from_json( 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 @@ def xblock_from_json( 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/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index 8b4b872c4301..9535a5232d01 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -255,7 +255,7 @@ def get_definitions(self, definitions): """ Retrieve all definitions listed in `definitions`. """ - return self.definitions.find({'$in': {'_id': definitions}}) + return self.definitions.find({'_id': {'$in': definitions}}) def insert_definition(self, definition): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 5cb62bcf57be..6c375d2dfc02 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 @@ **** '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 @@ import copy import threading import datetime +import hashlib import logging from contracts import contract, new_contract from importlib import import_module @@ -670,7 +673,7 @@ def cache_items(self, system, base_block_ids, course_key, depth=0, lazy=True): new_module_data = {} for block_id in base_block_ids: new_module_data = self.descendants( - system.course_entry.structure['blocks'], + copy.deepcopy(system.course_entry.structure['blocks']), # copy or our changes like setting 'definition_loaded' will affect the active bulk operation data block_id, depth, new_module_data @@ -691,12 +694,9 @@ def cache_items(self, system, base_block_ids, course_key, depth=0, lazy=True): 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,154 @@ def copy(self, user_id, source_course, destination_course, subtree_list=None, bl 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'] + + # + # CAPA modules store their 'markdown' value (an alternate representation of their content) in Scope.settings rather than Scope.content :-/ + # markdown is a field that really should not be overridable - it fundamentally changes the content. + # capa modules also use a custom editor that always saves their markdown field to the metadata, even if it hasn't changed, which breaks our override system. + # So until capa modules are fixed, we special-case them and remove their markdown fields, forcing the inherited version to use XML only. + if usage_key.block_type == 'problem' and 'markdown' in new_block_info['defaults']: + del new_block_info['defaults']['markdown'] + # + + 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 @@ -2695,7 +2843,8 @@ def _copy_subdag(self, user_id, destination_version, block_key, source_blocks, d self._filter_blacklist(copy.copy(new_block['fields']), blacklist), new_block['definition'], destination_version, - raw=True + raw=True, + block_defaults=new_block.get('defaults') ) # introduce new edit info field for tracing where copied/published blocks came @@ -2734,7 +2883,7 @@ def _delete_if_true_orphan(self, orphan, structure): self._delete_if_true_orphan(BlockKey(*child), structure) del structure['blocks'][orphan] - def _new_block(self, user_id, category, block_fields, definition_id, new_id, raw=False): + def _new_block(self, user_id, category, block_fields, definition_id, new_id, raw=False, block_defaults=None): """ Create the core document structure for a block. @@ -2745,7 +2894,7 @@ def _new_block(self, user_id, category, block_fields, definition_id, new_id, raw """ if not raw: block_fields = self._serialize_fields(category, block_fields) - return { + document = { 'block_type': category, 'definition': definition_id, 'fields': block_fields, @@ -2756,6 +2905,9 @@ def _new_block(self, user_id, category, block_fields, definition_id, new_id, raw 'update_version': new_id } } + if block_defaults: + document['defaults'] = block_defaults + return document @contract(block_key=BlockKey) def _get_block_from_structure(self, structure, 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 12c2f4c84b0f..14edfa3b15c6 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 @@ def _auto_publish_no_children(self, location, category, user_id, **kwargs): # 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 0cfa67214330..2124732242df 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 @@ def has(self, key): # 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_split_copy_from_template.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_copy_from_template.py new file mode 100644 index 000000000000..3d73178644aa --- /dev/null +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_copy_from_template.py @@ -0,0 +1,155 @@ +""" +Tests for split's copy_from_template method. +Currently it is only used for content libraries. +However for these tests, we make sure it also works when copying from course to course. +""" +import ddt +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.exceptions import ItemNotFoundError +from xmodule.modulestore.tests.factories import CourseFactory, LibraryFactory +from xmodule.modulestore.tests.utils import MixedSplitTestCase + + +@ddt.ddt +class TestSplitCopyTemplate(MixedSplitTestCase): + """ + Test for split's copy_from_template method. + """ + @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 = self.make_block("vertical", source_container) + problem_library_display_name = "Problem Library Display Name" + problem_block = self.make_block("problem", vertical_block, 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) + + # Check that when capa modules are copied, their "markdown" fields (Scope.settings) are removed. (See note in split.py:copy_from_template()) + self.assertIsNotNone(problem_block.markdown) + self.assertIsNone(problem_block_course.markdown) + + # 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 = self.make_block("html", vertical_block_course) + + # 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_publish(self): + """ + Test that copy_from_template's "defaults" data is not lost + when blocks are published. + """ + # Create a library with a problem: + source_library = LibraryFactory.create(modulestore=self.store) + display_name_expected = "CUSTOM Library Display Name" + self.make_block("problem", source_library, display_name=display_name_expected) + # Reload source_library since we need its branch and version to use copy_from_template: + source_library = self.store.get_library(source_library.location.library_key, remove_version=False, remove_branch=False) + # And a course with a vertical: + course = CourseFactory.create(modulestore=self.store) + self.make_block("vertical", course) + + problem_key_in_course = self.store.copy_from_template(source_library.children, dest_key=course.location, user_id=self.user_id)[0] + + # We do the following twice because different methods get used inside split modulestore on first vs. subsequent publish + for __ in range(0, 2): + # Publish: + self.store.publish(problem_key_in_course, self.user_id) + # Test that the defaults values are there. + problem_published = self.store.get_item(problem_key_in_course.for_branch(ModuleStoreEnum.BranchName.published)) + self.assertEqual(problem_published.display_name, display_name_expected) + + 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) + + # Populate the course: + about = self.make_block("about", source_course) + chapter = self.make_block("chapter", source_course) + sequential = self.make_block("sequential", chapter) + # And three blocks that are NOT auto-published: + vertical = self.make_block("vertical", sequential) + self.make_block("problem", vertical) + html = self.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/modulestore/tests/utils.py b/common/lib/xmodule/xmodule/modulestore/tests/utils.py index 76c379ec0b66..986a54df9f59 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/utils.py @@ -10,6 +10,7 @@ from xmodule.modulestore.edit_info import EditInfoMixin from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.modulestore.mixed import MixedModuleStore +from xmodule.modulestore.tests.factories import ItemFactory from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST from xmodule.tests import DATA_DIR @@ -108,3 +109,17 @@ def setUp(self): ) self.addCleanup(self.store.close_all_connections) self.addCleanup(self.store._drop_database) # pylint: disable=protected-access + + def make_block(self, category, parent_block, **kwargs): + """ + Create a block of type `category` as a child of `parent_block`, in any + course or library. You can pass any field values as kwargs. + """ + extra = {"publish_item": False, "user_id": self.user_id} + extra.update(kwargs) + return ItemFactory.create( + category=category, + parent_location=parent_block.location, + modulestore=self.store, + **extra + ) diff --git a/common/lib/xmodule/xmodule/tests/test_library_content.py b/common/lib/xmodule/xmodule/tests/test_library_content.py index fec957d0cda5..b92404df10c7 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 @@ def test_lib_content_block(self): # 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 @@ def test_children_seen_by_a_user(self): """ 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 88f87f80d344..4d4362c13e4a 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1252,6 +1252,7 @@ def get_field_provenance(self, xblock, field): :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. @@ -1259,12 +1260,8 @@ def get_field_provenance(self, xblock, field): 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 diff --git a/common/test/acceptance/pages/studio/container.py b/common/test/acceptance/pages/studio/container.py index d8a760cac972..62ed331fe6a0 100644 --- a/common/test/acceptance/pages/studio/container.py +++ b/common/test/acceptance/pages/studio/container.py @@ -282,6 +282,7 @@ class XBlockWrapper(PageObject): COMPONENT_BUTTONS = { 'basic_tab': '.editor-tabs li.inner_tab_wrap:nth-child(1) > a', 'advanced_tab': '.editor-tabs li.inner_tab_wrap:nth-child(2) > a', + 'settings_tab': '.editor-modes .settings-button', 'save_settings': '.action-save', } @@ -409,6 +410,28 @@ def open_basic_tab(self): """ self._click_button('basic_tab') + def open_settings_tab(self): + """ + If editing, click on the "Settings" tab + """ + self._click_button('settings_tab') + + def set_field_val(self, field_display_name, field_value): + """ + If editing, set the value of a field. + """ + selector = '{} li.field label:contains("{}") + input'.format(self.editor_selector, field_display_name) + script = "$(arguments[0]).val(arguments[1]).change();" + self.browser.execute_script(script, selector, field_value) + + def reset_field_val(self, field_display_name): + """ + If editing, reset the value of a field to its default. + """ + scope = '{} li.field label:contains("{}")'.format(self.editor_selector, field_display_name) + script = "$(arguments[0]).siblings('.setting-clear').click();" + self.browser.execute_script(script, scope) + def set_codemirror_text(self, text, index=0): """ Set the text of a CodeMirror editor that is part of this xblock's settings. 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 ce0464374515..593243e6282c 100644 --- a/common/test/acceptance/tests/studio/test_studio_library_container.py +++ b/common/test/acceptance/tests/studio/test_studio_library_container.py @@ -244,3 +244,54 @@ def test_not_enough_children_blocks(self): expected_tpl.format(count=50, actual=len(self.library_fixture.children)), library_container.validation_warning_text ) + + def test_settings_overrides(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. + + When I edit one of the blocks to change a setting such as "display_name", + Then I can see the new setting is overriding the library version. + + When I subsequently click to refresh the content with the latest from the library, + Then I can see that the overrided version of the setting is preserved. + + When I click to edit the block and reset the setting, + then I can see that the setting's field defaults back to the library version. + """ + 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() + library_block = self._get_library_xblock_wrapper(container_page.xblocks[0]) + + self.assertFalse(library_block.has_validation_message) + self.assertEqual(len(library_block.children), 3) + + block = library_block.children[0] + self.assertIn(block.name, ("Html1", "Html2", "Html3")) + name_default = block.name + + block.edit() + new_display_name = "A new name for this HTML block" + block.set_field_val("Display Name", new_display_name) + block.save_settings() + + self.assertEqual(block.name, new_display_name) + + # Create a new block, causing a new library version: + self.library_fixture.create_xblock(self.library_fixture.library_location, XBlockFixtureDesc("html", "Html4")) + + container_page.visit() # Reload + self.assertTrue(library_block.has_validation_warning) + library_block.refresh_children() + container_page.wait_for_page() # Wait for the page to reload + + self.assertEqual(len(library_block.children), 4) + self.assertEqual(block.name, new_display_name) + + # Reset: + block.edit() + block.reset_field_val("Display Name") + block.save_settings() + self.assertEqual(block.name, name_default)