diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index 91dc14983b86..1547c25f91ab 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -16,7 +16,7 @@ from cms.djangoapps.contentstore.tests.utils import AjaxEnabledTestClient, parse_json from cms.djangoapps.contentstore.utils import reverse_library_url, reverse_url, reverse_usage_url -from cms.djangoapps.contentstore.views.item import _duplicate_item +from cms.djangoapps.contentstore.views.item import duplicate_block from cms.djangoapps.contentstore.views.preview import _load_preview_module from cms.djangoapps.contentstore.views.tests.test_library import LIBRARY_REST_URL from cms.djangoapps.course_creators.views import add_user_with_status_granted @@ -947,7 +947,7 @@ def test_persistent_overrides(self, duplicate): if duplicate: # Check that this also works when the RCB is duplicated. self.lc_block = modulestore().get_item( - _duplicate_item(self.course.location, self.lc_block.location, self.user) + duplicate_block(self.course.location, self.lc_block.location, self.user) ) self.problem_in_course = modulestore().get_item(self.lc_block.children[0]) else: @@ -1006,7 +1006,7 @@ def test_duplicated_version(self): # Duplicate self.lc_block: duplicate = store.get_item( - _duplicate_item(self.course.location, self.lc_block.location, self.user) + duplicate_block(self.course.location, self.lc_block.location, self.user) ) # The duplicate should have identical children to the original: self.assertEqual(len(duplicate.children), 1) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 4364134aa4dc..77f2a344d7f8 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -239,11 +239,11 @@ def xblock_handler(request, usage_key_string=None): status=400 ) - dest_usage_key = _duplicate_item( + dest_usage_key = duplicate_block( parent_usage_key, duplicate_source_usage_key, request.user, - request.json.get('display_name'), + display_name=request.json.get('display_name'), ) return JsonResponse({ 'locator': str(dest_usage_key), @@ -873,47 +873,88 @@ def _move_item(source_usage_key, target_parent_usage_key, user, target_index=Non return JsonResponse(context) -def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_name=None, is_child=False): +def gather_block_attributes(source_item, display_name=None, is_child=False): + """ + Gather all the attributes of the source block that need to be copied over to a new or updated block. + """ + # Update the display name to indicate this is a duplicate (unless display name provided). + # Can't use own_metadata(), b/c it converts data for JSON serialization - + # not suitable for setting metadata of the new block + 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 is_child: + display_name = display_name or source_item.display_name or source_item.category + + if display_name is not None: + duplicate_metadata['display_name'] = display_name + else: + if source_item.display_name is None: + duplicate_metadata['display_name'] = _("Duplicate of {0}").format(source_item.category) + else: + duplicate_metadata['display_name'] = _("Duplicate of '{0}'").format(source_item.display_name) + + asides_to_create = [] + for aside in source_item.runtime.get_asides(source_item): + for field in aside.fields.values(): + if field.scope in (Scope.settings, Scope.content,) and field.is_set_on(aside): + asides_to_create.append(aside) + break + + for aside in asides_to_create: + for field in aside.fields.values(): + if field.scope not in (Scope.settings, Scope.content,): + field.delete_from(aside) + return duplicate_metadata, asides_to_create + + +def update_from_source(*, source_block, destination_block, user_id): + """ + Update a block to have all the settings and attributes of another source. + + Copies over all attributes and settings of a source block to a destination + block. Blocks must be the same type. This function does not modify or duplicate + children. + """ + duplicate_metadata, asides = gather_block_attributes(source_block, display_name=source_block.display_name) + for key, value in duplicate_metadata.items(): + setattr(destination_block, key, value) + for key, value in source_block.get_explicitly_set_fields_by_scope(Scope.content).items(): + setattr(destination_block, key, value) + modulestore().update_item( + destination_block, + user_id, + metadata=duplicate_metadata, + asides=asides, + ) + + +def duplicate_block( + parent_usage_key, + duplicate_source_usage_key, + user, + dest_usage_key=None, + display_name=None, + shallow=False, + is_child=False +): """ Duplicate an existing xblock as a child of the supplied parent_usage_key. """ store = modulestore() with store.bulk_operations(duplicate_source_usage_key.course_key): source_item = store.get_item(duplicate_source_usage_key) - # Change the blockID to be unique. - dest_usage_key = source_item.location.replace(name=uuid4().hex) - category = dest_usage_key.block_type - - # Update the display name to indicate this is a duplicate (unless display name provided). - # Can't use own_metadata(), b/c it converts data for JSON serialization - - # not suitable for setting metadata of the new block - 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 is_child: - display_name = display_name or source_item.display_name or source_item.category - - if display_name is not None: - duplicate_metadata['display_name'] = display_name - else: - if source_item.display_name is None: - duplicate_metadata['display_name'] = _("Duplicate of {0}").format(source_item.category) - else: - duplicate_metadata['display_name'] = _("Duplicate of '{0}'").format(source_item.display_name) + if not dest_usage_key: + # Change the blockID to be unique. + dest_usage_key = source_item.location.replace(name=uuid4().hex) - asides_to_create = [] - for aside in source_item.runtime.get_asides(source_item): - for field in aside.fields.values(): - if field.scope in (Scope.settings, Scope.content,) and field.is_set_on(aside): - asides_to_create.append(aside) - break + category = dest_usage_key.block_type - for aside in asides_to_create: - for field in aside.fields.values(): - if field.scope not in (Scope.settings, Scope.content,): - field.delete_from(aside) + duplicate_metadata, asides_to_create = gather_block_attributes( + source_item, display_name=display_name, is_child=is_child, + ) dest_module = store.create_item( user.id, @@ -937,11 +978,11 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_ # Children are not automatically copied over (and not all xblocks have a 'children' attribute). # Because DAGs are not fully supported, we need to actually duplicate each child as well. - if source_item.has_children and not children_handled: + if source_item.has_children and not shallow and not children_handled: dest_module.children = dest_module.children or [] for child in source_item.children: - dupe = _duplicate_item(dest_module.location, child, user=user, is_child=True) - if dupe not in dest_module.children: # _duplicate_item may add the child for us. + dupe = duplicate_block(dest_module.location, child, user=user, is_child=True) + if dupe not in dest_module.children: # _duplicate_block may add the child for us. dest_module.children.append(dupe) store.update_item(dest_module, user.id) diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index b648fd8f8683..09d0a51f3919 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -68,7 +68,7 @@ _get_source_index, _xblock_type_and_display_name, add_container_page_publishing_info, - create_xblock_info, + create_xblock_info, duplicate_block, update_from_source, ) @@ -780,6 +780,29 @@ def verify_name(source_usage_key, parent_usage_key, expected_name, display_name= # Now send a custom display name for the duplicate. verify_name(self.seq_usage_key, self.chapter_usage_key, "customized name", display_name="customized name") + def test_shallow_duplicate(self): + """ + Test that shallow_duplicate creates a new block. + """ + source_course = CourseFactory() + user = UserFactory.create() + source_chapter = ItemFactory(parent=source_course, category='chapter', display_name='Source Chapter') + ItemFactory(parent=source_chapter, category='html', display_name='Child') + # Refresh. + source_chapter = self.store.get_item(source_chapter.location) + self.assertEqual(len(source_chapter.get_children()), 1) + destination_course = CourseFactory() + destination_location = duplicate_block( + parent_usage_key=destination_course.location, duplicate_source_usage_key=source_chapter.location, + user=user, + display_name=source_chapter.display_name, + shallow=True, + ) + # Refresh here, too, just to be sure. + destination_chapter = self.store.get_item(destination_location) + self.assertEqual(len(destination_chapter.get_children()), 0) + self.assertEqual(destination_chapter.display_name, 'Source Chapter') + @ddt.ddt class TestMoveItem(ItemTest): @@ -3587,3 +3610,111 @@ def test_creator_show_delete_button_with_waffle(self): ) self.assertFalse(xblock_info['show_delete_button']) + + +@patch('xmodule.modulestore.split_mongo.caching_descriptor_system.CachingDescriptorSystem.applicable_aside_types', + lambda self, block: ['test_aside']) +class TestUpdateFromSource(ModuleStoreTestCase): + """ + Test update_from_source. + """ + + def setUp(self): + """ + Set up the runtime for tests. + """ + super().setUp() + key_store = DictKeyValueStore() + field_data = KvsFieldData(key_store) + self.runtime = TestRuntime(services={'field-data': field_data}) + + def create_source_block(self, course): + """ + Create a chapter with all the fixings. + """ + source_block = ItemFactory( + parent=course, + category='course_info', + display_name='Source Block', + metadata={'due': datetime(2010, 11, 22, 4, 0, tzinfo=UTC)}, + ) + + def_id = self.runtime.id_generator.create_definition('html') + usage_id = self.runtime.id_generator.create_usage(def_id) + + aside = AsideTest(scope_ids=ScopeIds('user', 'html', def_id, usage_id), runtime=self.runtime) + aside.field11 = 'html_new_value1' + + # The data attribute is handled in a special manner and should be updated. + source_block.data = '
test
' + # This field is set on the content scope (definition_data), which should be updated. + source_block.items = ['test', 'beep'] + + self.store.update_item(source_block, self.user.id, asides=[aside]) + + # quick sanity checks + source_block = self.store.get_item(source_block.location) + self.assertEqual(source_block.due, datetime(2010, 11, 22, 4, 0, tzinfo=UTC)) + self.assertEqual(source_block.display_name, 'Source Block') + self.assertEqual(source_block.runtime.get_asides(source_block)[0].field11, 'html_new_value1') + self.assertEqual(source_block.data, '
test
') + self.assertEqual(source_block.items, ['test', 'beep']) + + return source_block + + def check_updated(self, source_block, destination_key): + """ + Check that the destination block has been updated to match our source block. + """ + revised = self.store.get_item(destination_key) + self.assertEqual(source_block.display_name, revised.display_name) + self.assertEqual(source_block.due, revised.due) + self.assertEqual(revised.data, source_block.data) + self.assertEqual(revised.items, source_block.items) + + self.assertEqual( + revised.runtime.get_asides(revised)[0].field11, + source_block.runtime.get_asides(source_block)[0].field11, + ) + + @XBlockAside.register_temp_plugin(AsideTest, 'test_aside') + def test_update_from_source(self): + """ + Test that update_from_source updates the destination block. + """ + course = CourseFactory() + user = UserFactory.create() + + source_block = self.create_source_block(course) + + destination_block = ItemFactory(parent=course, category='course_info', display_name='Destination Problem') + update_from_source(source_block=source_block, destination_block=destination_block, user_id=user.id) + self.check_updated(source_block, destination_block.location) + + @XBlockAside.register_temp_plugin(AsideTest, 'test_aside') + def test_update_clobbers(self): + """ + Verify that our update clobbers everything. + """ + course = CourseFactory() + user = UserFactory.create() + + source_block = self.create_source_block(course) + + destination_block = ItemFactory( + parent=course, + category='course_info', + display_name='Destination Chapter', + metadata={'due': datetime(2025, 10, 21, 6, 5, tzinfo=UTC)}, + ) + + def_id = self.runtime.id_generator.create_definition('html') + usage_id = self.runtime.id_generator.create_usage(def_id) + aside = AsideTest(scope_ids=ScopeIds('user', 'html', def_id, usage_id), runtime=self.runtime) + aside.field11 = 'Other stuff' + destination_block.data = '
other stuff
' + destination_block.items = ['other stuff', 'boop'] + self.store.update_item(destination_block, user.id, asides=[aside]) + + update_from_source(source_block=source_block, destination_block=destination_block, user_id=user.id) + self.check_updated(source_block, destination_block.location) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 7878b85d076f..31cf347e3642 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -57,7 +57,6 @@ import copy import datetime -import hashlib import logging from collections import defaultdict from importlib import import_module @@ -102,7 +101,7 @@ ) from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope from xmodule.modulestore.split_mongo.mongo_connection import DuplicateKeyError, DjangoFlexPersistenceBackend -from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES +from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES, derived_key from xmodule.partitions.partitions_service import PartitionService from ..exceptions import ItemNotFoundError @@ -2428,7 +2427,6 @@ def _copy_from_template( for usage_key in source_keys: src_course_key = usage_key.course_key - hashable_source_id = src_course_key.for_version(None) block_key = BlockKey(usage_key.block_type, usage_key.block_id) source_structure = source_structures[src_course_key] @@ -2436,15 +2434,7 @@ def _copy_from_template( 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( - str(hashable_source_id).encode("utf-8"), - block_key.id, - new_parent_block_key.id, - ) - new_block_id = hashlib.sha1(unique_data.encode('utf-8')).hexdigest()[:20] - new_block_key = BlockKey(block_key.type, new_block_id) + new_block_key = derived_key(src_course_key, block_key, new_parent_block_key) # Now clone block_key to new_block_key: new_block_info = copy.deepcopy(source_block_info) diff --git a/common/lib/xmodule/xmodule/modulestore/store_utilities.py b/common/lib/xmodule/xmodule/modulestore/store_utilities.py index da71999c0104..cfa2451779d3 100644 --- a/common/lib/xmodule/xmodule/modulestore/store_utilities.py +++ b/common/lib/xmodule/xmodule/modulestore/store_utilities.py @@ -1,5 +1,5 @@ # lint-amnesty, pylint: disable=missing-module-docstring - +import hashlib import logging import re import uuid @@ -7,6 +7,8 @@ from xblock.core import XBlock +from xmodule.modulestore.split_mongo import BlockKey + DETACHED_XBLOCK_TYPES = {name for name, __ in XBlock.load_tagged_classes("detached")} @@ -104,3 +106,26 @@ def get_draft_subtree_roots(draft_nodes): for draft_node in draft_nodes: if draft_node.parent_url not in urls: yield draft_node + + +def derived_key(courselike_source_key, block_key, parent): + """ + Return a new reproducible block ID for a given root, source block, and destination parent. + + When recursively copying a block structure, we need to generate new block IDs for the + blocks. We don't want to use the exact same IDs as we might copy blocks multiple times. + However, we do want to be able to reproduce the same IDs when copying the same block + so that if we ever need to re-copy the block from its source (that is, to update it with + upstream changes) we don't affect any data tied to the ID, such as grades. + """ + hashable_source_id = courselike_source_key.for_version(None) + + # 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( + str(hashable_source_id).encode("utf-8"), + block_key.id, + parent.id, + ) + new_block_id = hashlib.sha1(unique_data.encode('utf-8')).hexdigest()[:20] + return BlockKey(block_key.type, new_block_id) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_store_utilities.py b/common/lib/xmodule/xmodule/modulestore/tests/test_store_utilities.py index fdc76655f770..a15999c60745 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_store_utilities.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_store_utilities.py @@ -4,11 +4,15 @@ import unittest +from unittest import TestCase from unittest.mock import Mock import ddt -from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots +from opaque_keys.edx.keys import CourseKey + +from xmodule.modulestore.split_mongo import BlockKey +from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots, derived_key @ddt.ddt @@ -82,3 +86,43 @@ def test_get_draft_subtree_roots(self, node_arguments_list, expected_roots_urls) subtree_roots_urls = [root.url for root in get_draft_subtree_roots(module_nodes)] # check that we return the expected urls assert set(subtree_roots_urls) == set(expected_roots_urls) + + +mock_block = Mock() +mock_block.id = CourseKey.from_string('course-v1:Beeper+B33P+BOOP') + + +derived_key_scenarios = [ + { + 'courselike_source_key': CourseKey.from_string('course-v1:edX+DemoX+Demo_Course'), + 'block_key': BlockKey('chapter', 'interactive_demonstrations'), + 'parent': mock_block, + 'expected': BlockKey( + 'chapter', '5793ec64e25ed870a7dd', + ), + }, + { + 'courselike_source_key': CourseKey.from_string('course-v1:edX+DemoX+Demo_Course'), + 'block_key': BlockKey('chapter', 'interactive_demonstrations'), + 'parent': BlockKey( + 'chapter', 'thingy', + ), + 'expected': BlockKey( + 'chapter', '599792a5622d85aa41e6', + ), + } +] + + +@ddt.ddt +class TestDerivedKey(TestCase): + """ + Test reproducible block ID generation. + """ + @ddt.data(*derived_key_scenarios) + @ddt.unpack + def test_derived_key(self, courselike_source_key, block_key, parent, expected): + """ + Test that derived_key returns the expected value. + """ + self.assertEqual(derived_key(courselike_source_key, block_key, parent), expected)