diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index bc1b44c03754..1ef0635bc13c 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -303,3 +303,57 @@ def test_change_after_first_sync(self): self.assertEqual(len(lc_block.children), 1) # Children should not be deleted due to a bad setting. html_block = modulestore().get_item(lc_block.children[0]) self.assertEqual(html_block.data, data_value) + + def test_overrides(self): + """ + Test that overriding block Scope.settings fields from a library in a specific course works + """ + original_display_name = "An HTML Block" + + # First, create a library containing an HTML block: + block1 = ItemFactory.create( + category="html", + parent_location=self.library.location, + display_name=original_display_name, # display_name is a scope.settings field + user_id=self.user.id, + publish_item=False, + ) + self.assertEqual(block1.display_name, original_display_name) + def_id1 = block1.definition_locator.definition_id + + # Next, create two courses: + with modulestore().default_store(ModuleStoreEnum.Type.split): + course1 = CourseFactory.create() + course2 = CourseFactory.create() + + # Add a LibraryContent block to each course: + lc_block1 = self._add_library_content_block(course1, self.lib_key) + lc_block1 = self._refresh_children(lc_block1) + + # Make sure that the new child of the LibraryContent block + # shares its definition with block1 + block1_course = modulestore().get_item(lc_block1.children[0]) + self.assertEqual(block1_course.definition_locator.definition_id, def_id1) + self.assertEqual(block1_course.display_name, original_display_name) + + # Change a settings field on lc_block1 + block1_course.display_name = "NEW" + modulestore().update_item(block1_course, self.user.id) + block1_course = modulestore().get_item(block1_course.location) + self.assertEqual(block1_course.display_name, "NEW") + + # Add a LibraryContent block to the second course: + lc_block2 = self._add_library_content_block(course2, self.lib_key) + lc_block2 = self._refresh_children(lc_block2) + block2_course = modulestore().get_item(lc_block2.children[0]) + self.assertEqual(block2_course.display_name, original_display_name) + + # Now make sure the override persists even when blocks are refreshed. + # Force a new library version by adding another block to the library + ItemFactory.create(category="problem", parent_location=self.library.location, user_id=self.user.id, publish_item=False) + + block1_course = modulestore().get_item(block1_course.location) + block2_course = modulestore().get_item(block2_course.location) + + self.assertEqual(block1_course.display_name, "NEW") + self.assertEqual(block2_course.display_name, original_display_name) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 7b893486eedd..d68e3d9bcacf 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -553,7 +553,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 = {} + 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: @@ -578,7 +581,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 8571540f7589..261bb382147c 100644 --- a/common/lib/xmodule/xmodule/library_content_module.py +++ b/common/lib/xmodule/xmodule/library_content_module.py @@ -289,7 +289,7 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe js_module_name = "VerticalDescriptor" @XBlock.handler - def refresh_children(self, request, suffix, 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 @@ -301,14 +301,11 @@ def refresh_children(self, request, suffix, update_db=True): # pylint: disable= 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_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, update_db) + lib_tools.update_children(self, user_id) return Response() def validate(self): @@ -364,7 +361,7 @@ def editor_saved(self, user, old_metadata, old_content): old_source_libraries = LibraryList().from_json(old_metadata.get('source_libraries', [])) if set(old_source_libraries) != set(self.source_libraries): 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 0ff50d81a911..568b8f0fe2e2 100644 --- a/common/lib/xmodule/xmodule/library_tools.py +++ b/common/lib/xmodule/xmodule/library_tools.py @@ -1,9 +1,7 @@ """ XBlock runtime services for LibraryContentModule """ -import hashlib from opaque_keys.edx.locator import LibraryLocator -from xblock.fields import Scope from xmodule.library_content_module import LibraryVersionReference from xmodule.modulestore.exceptions import ItemNotFoundError @@ -54,7 +52,7 @@ def get_library_display_name(self, lib_key): return library.display_name return None - def update_children(self, dest_block, user_id, update_db=True): + def update_children(self, dest_block, user_id): """ 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 @@ -65,74 +63,18 @@ def update_children(self, dest_block, user_id, 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 = [] + new_libraries = [] + source_blocks = [] + for library_key, dummy in dest_block.source_libraries: + library = self._get_library(library_key) + if library is None: + raise ValueError("Required library not found.") + source_blocks.extend(library.children) # In future, this will be filtered so only specific children are used. + 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.") - 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): - """ - Internal method to copy blocks from the library recursively - """ - new_children = [] - for child_key in from_block.children: - child = self.store.get_item(child_key, depth=9) - # 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)) - 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.inherit_copy(source_blocks, dest_block.location, user_id, copy_children=True) + # ^-- inherit_copy 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 0ce363c4837c..0b6b563d9aab 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -671,6 +671,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 inherit_copy(self, source_keys, dest_key, user_id, copy_children=True): + """ + See :py:meth `SplitMongoModuleStore.inherit_copy` + """ + store = self._verify_modulestore_support(dest_key, 'inherit_copy') + return store.inherit_copy(source_keys, dest_key, user_id, copy_children) + @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 015ecdd16489..cd9fb3a7eacf 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -258,7 +258,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 aa0dc8eb7427..1e2fb1062783 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -27,6 +27,7 @@ **** '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 inherited from another block **** 'edit_info': dictionary: ***** 'edited_on': when was this xblock's fields last changed (will be edited_on value of update_version structure) @@ -53,6 +54,7 @@ import copy import threading import datetime +import hashlib import logging from contracts import contract, new_contract from importlib import import_module @@ -644,7 +646,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 @@ -665,12 +667,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) @@ -2045,6 +2044,157 @@ 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 inherit_copy(self, source_keys, dest_usage, user_id, copy_children=True): + """ + 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 inherit_copy 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. + + :param copy_children: If true, all descendants of each XBlock will be copied recursively. + """ + if copy_children: + # 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.course_key.for_version(None) + if course.branch is None: + raise ItemNotFoundError("branch is required for all source keys when using inherit_copy") + if course not in source_structures: + with self.bulk_operations(course): + source_structures[course] = self._lookup_course(course).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([block for block in 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._inherit_copy(source_structures, source_keys, dest_structure, block_key, user_id, recurse=copy_children) + + # 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 _inherit_copy(self, source_structures, source_keys, dest_structure, new_parent_block_key, user_id, recurse): + """ + Internal recursive implementation of inherit_copy() + + 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 recurse and children: + children = [src_course_key.make_usage_key(child.type, child.id) for child in children] + new_blocks |= self._inherit_copy(source_structures, children, dest_structure, new_block_key, user_id, True) + + 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 12c2f4c84b0f..4cf8d3b0e141 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,14 @@ 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 inherit_copy(self, source_keys, dest_key, user_id, copy_children=True): + """ + See :py:meth `SplitMongoModuleStore.inherit_copy` + """ + source_keys = [self._map_revision_to_branch(key) for key in source_keys] + dest_key = self._map_revision_to_branch(dest_key) + return super(DraftVersioningModuleStore, self).inherit_copy(source_keys, dest_key, user_id, copy_children) + 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..cfd5425a9b64 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,19 @@ 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 fields that are set locally (inherited from another block) """ # 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 +112,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 definition's defaults + 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/tests/test_library_content.py b/common/lib/xmodule/xmodule/tests/test_library_content.py index 2b52386e3740..1704ed3fd7f8 100644 --- a/common/lib/xmodule/xmodule/tests/test_library_content.py +++ b/common/lib/xmodule/xmodule/tests/test_library_content.py @@ -89,7 +89,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)) @@ -97,7 +98,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 5b9672115322..5489b40d452e 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1265,6 +1265,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. @@ -1272,12 +1273,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 d7a592c79fce..c7aedf2c9ae6 100644 --- a/common/test/acceptance/tests/studio/test_studio_library_container.py +++ b/common/test/acceptance/tests/studio/test_studio_library_container.py @@ -162,3 +162,54 @@ def test_out_of_date_message(self): self.assertFalse(library_block.has_validation_message) #self.assertIn("4 matching components", library_block.author_content) # Removed this assert until a summary message is added back to the author view (SOL-192) + + 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)