From 81fecf3e6ceb515b449aed8cb82f79f08f9f78d7 Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 15 Jan 2014 16:16:55 -0500 Subject: [PATCH] Duplicate children so as to not create a DAG. --- .../contentstore/tests/test_item.py | 56 +++++++++++++++---- cms/djangoapps/contentstore/views/item.py | 38 ++++++++----- 2 files changed, 67 insertions(+), 27 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_item.py b/cms/djangoapps/contentstore/tests/test_item.py index f6f31af5be..b41a371750 100644 --- a/cms/djangoapps/contentstore/tests/test_item.py +++ b/cms/djangoapps/contentstore/tests/test_item.py @@ -21,6 +21,7 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.django import loc_mapper from xmodule.modulestore.locator import BlockUsageLocator from xmodule.modulestore.exceptions import ItemNotFoundError +from xmodule.modulestore import Location class ItemTest(CourseTestCase): @@ -165,8 +166,12 @@ class TestDuplicateItem(ItemTest): def setUp(self): """ Creates the test course structure and a few components to 'duplicate'. """ super(TestDuplicateItem, self).setUp() - # create a parent sequential - resp = self.create_xblock(parent_locator=self.unicode_locator, category='sequential') + # Create a parent chapter (for testing children of children). + resp = self.create_xblock(parent_locator=self.unicode_locator, category='chapter') + self.chapter_locator = self.response_locator(resp) + + # create a sequential containing a problem and an html component + resp = self.create_xblock(parent_locator=self.chapter_locator, category='sequential') self.seq_locator = self.response_locator(resp) # create problem and an html component @@ -176,15 +181,21 @@ class TestDuplicateItem(ItemTest): resp = self.create_xblock(parent_locator=self.seq_locator, category='html') self.html_locator = self.response_locator(resp) + # Create a second sequential just (testing children of children) + self.create_xblock(parent_locator=self.chapter_locator, category='sequential2') + def test_duplicate_equality(self): """ Tests that a duplicated xblock is identical to the original, except for location and display name. """ - def verify_duplicate(source_locator, parent_locator): + def duplicate_and_verify(source_locator, parent_locator): locator = self._duplicate_item(parent_locator, source_locator) + self.assertTrue(check_equality(source_locator, locator), "Duplicated item differs from original") + + def check_equality(source_locator, duplicate_locator): original_item = self.get_item_from_modulestore(source_locator, draft=True) - duplicated_item = self.get_item_from_modulestore(locator, draft=True) + duplicated_item = self.get_item_from_modulestore(duplicate_locator, draft=True) self.assertNotEqual( original_item.location, @@ -194,11 +205,32 @@ class TestDuplicateItem(ItemTest): # Set the location and display name to be the same so we can make sure the rest of the duplicate is equal. duplicated_item.location = original_item.location duplicated_item.display_name = original_item.display_name - self.assertEqual(original_item, duplicated_item, "Duplicated item differs from original") - verify_duplicate(self.problem_locator, self.seq_locator) - verify_duplicate(self.html_locator, self.seq_locator) - verify_duplicate(self.seq_locator, self.unicode_locator) + # Children will also be duplicated, so for the purposes of testing equality, we will set + # the children to the original after recursively checking the children. + if original_item.has_children: + self.assertEqual( + len(original_item.children), + len(duplicated_item.children), + "Duplicated item differs in number of children" + ) + for i in xrange(len(original_item.children)): + source_locator = loc_mapper().translate_location( + self.course.location.course_id, Location(original_item.children[i]), False, True + ) + duplicate_locator = loc_mapper().translate_location( + self.course.location.course_id, Location(duplicated_item.children[i]), False, True + ) + if not check_equality(source_locator, duplicate_locator): + return False + duplicated_item.children = original_item.children + + return original_item == duplicated_item + + duplicate_and_verify(self.problem_locator, self.seq_locator) + duplicate_and_verify(self.html_locator, self.seq_locator) + duplicate_and_verify(self.seq_locator, self.chapter_locator) + duplicate_and_verify(self.chapter_locator, self.unicode_locator) def test_ordering(self): """ @@ -224,7 +256,7 @@ class TestDuplicateItem(ItemTest): "source item at wrong position" ) self.assertEqual( - children[source_position+1], + children[source_position + 1], self.get_old_id(locator).url(), "duplicated item not ordered after source item" ) @@ -232,7 +264,7 @@ class TestDuplicateItem(ItemTest): verify_order(self.problem_locator, self.seq_locator, 0) # 2 because duplicate of problem should be located before. verify_order(self.html_locator, self.seq_locator, 2) - verify_order(self.seq_locator, self.unicode_locator, 0) + verify_order(self.seq_locator, self.chapter_locator, 0) # Test duplicating something into a location that is not the parent of the original item. # Duplicated item should appear at the end. @@ -257,10 +289,10 @@ class TestDuplicateItem(ItemTest): verify_name(self.html_locator, self.seq_locator, "Duplicate of 'Text'") # The sequence does not have a display_name set, so None gets included as the string 'None'. - verify_name(self.seq_locator, self.unicode_locator, "Duplicate of 'None'") + verify_name(self.seq_locator, self.chapter_locator, "Duplicate of 'None'") # Now send a custom display name for the duplicate. - verify_name(self.seq_locator, self.unicode_locator, "customized name", display_name="customized name") + verify_name(self.seq_locator, self.chapter_locator, "customized name", display_name="customized name") def _duplicate_item(self, parent_locator, source_locator, display_name=None): data = { diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index fa42f40366..5f3ce37a18 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -19,6 +19,7 @@ from xmodule.modulestore.django import modulestore, loc_mapper from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.locator import BlockUsageLocator +from xmodule.modulestore import Location from xmodule.x_module import prefer_xmodules from util.json_request import expect_json, JsonResponse @@ -138,12 +139,19 @@ def xblock_handler(request, tag=None, package_id=None, branch=None, version_guid if 'duplicate_source_locator' in request.json: parent_locator = BlockUsageLocator(request.json['parent_locator']) duplicate_source_locator = BlockUsageLocator(request.json['duplicate_source_locator']) - new_locator = _duplicate_item( - parent_locator, - duplicate_source_locator, + + # _duplicate_item is dealing with locations to facilitate the recursive call for + # duplicating children. + parent_location = loc_mapper().translate_locator_to_location(parent_locator) + duplicate_source_location = loc_mapper().translate_locator_to_location(duplicate_source_locator) + dest_location = _duplicate_item( + parent_location, + duplicate_source_location, request.json.get('display_name') ) - return JsonResponse({"locator": unicode(new_locator)}) + course_location = loc_mapper().translate_locator_to_location(BlockUsageLocator(parent_locator), get_course=True) + dest_locator = loc_mapper().translate_location(course_location.course_id, dest_location, False, True) + return JsonResponse({"locator": unicode(dest_locator)}) else: return _create_item(request) else: @@ -300,13 +308,10 @@ def _create_item(request): return JsonResponse({"locator": unicode(locator)}) -def _duplicate_item(parent_locator, duplicate_source_locator, display_name): +def _duplicate_item(parent_location, duplicate_source_location, display_name=None): """ - Duplicate an existing xblock as a child of the supplied parent_locator. + Duplicate an existing xblock as a child of the supplied parent_location. """ - parent_location = loc_mapper().translate_locator_to_location(parent_locator) - duplicate_source_location = loc_mapper().translate_locator_to_location(duplicate_source_locator) - store = get_modulestore(duplicate_source_location) source_item = store.get_item(duplicate_source_location) # Change the blockID to be unique. @@ -327,9 +332,13 @@ def _duplicate_item(parent_locator, duplicate_source_locator, display_name): system=source_item.system if hasattr(source_item, 'system') else None, ) - # Children are not automatically copied over. Not all xblocks have a 'children' attribute. - if hasattr(source_item, 'children'): - get_modulestore(dest_location).update_children(dest_location, source_item.children) + # 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: + copied_children = [] + for child in source_item.children: + copied_children.append(_duplicate_item(dest_location, Location(child)).url()) + get_modulestore(dest_location).update_children(dest_location, copied_children) if category not in DETACHED_CATEGORIES: parent = get_modulestore(parent_location).get_item(parent_location) @@ -337,13 +346,12 @@ def _duplicate_item(parent_locator, duplicate_source_locator, display_name): # Otherwise, add child to end. if duplicate_source_location.url() in parent.children: source_index = parent.children.index(duplicate_source_location.url()) - parent.children.insert(source_index+1, dest_location.url()) + parent.children.insert(source_index + 1, dest_location.url()) else: parent.children.append(dest_location.url()) get_modulestore(parent_location).update_children(parent_location, parent.children) - course_location = loc_mapper().translate_locator_to_location(BlockUsageLocator(parent_locator), get_course=True) - return loc_mapper().translate_location(course_location.course_id, dest_location, False, True) + return dest_location def _delete_item_at_location(item_location, delete_children=False, delete_all_versions=False):