From 0d8d7cb393427b8581bd392a693d11c4e14bcbee Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 15 Jan 2014 14:36:40 -0500 Subject: [PATCH 1/2] Support duplicating an existing xblock to a supplied parent location. STUD-1190 --- .../contentstore/tests/test_item.py | 125 +++++++++++++++++- cms/djangoapps/contentstore/views/item.py | 70 +++++++++- 2 files changed, 187 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_item.py b/cms/djangoapps/contentstore/tests/test_item.py index acfa4b491c..f6f31af5be 100644 --- a/cms/djangoapps/contentstore/tests/test_item.py +++ b/cms/djangoapps/contentstore/tests/test_item.py @@ -4,7 +4,7 @@ import json from datetime import datetime import ddt -from mock import Mock, patch +from mock import patch from pytz import UTC from webob import Response @@ -28,9 +28,10 @@ class ItemTest(CourseTestCase): def setUp(self): super(ItemTest, self).setUp() - self.unicode_locator = unicode(loc_mapper().translate_location( + self.course_locator = loc_mapper().translate_location( self.course.location.course_id, self.course.location, False, True - )) + ) + self.unicode_locator = unicode(self.course_locator) def get_old_id(self, locator): """ @@ -157,6 +158,124 @@ class TestCreateItem(ItemTest): self.assertEqual(obj.start, datetime(2030, 1, 1, tzinfo=UTC)) +class TestDuplicateItem(ItemTest): + """ + Test the duplicate method. + """ + 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') + self.seq_locator = self.response_locator(resp) + + # create problem and an html component + resp = self.create_xblock(parent_locator=self.seq_locator, category='problem', boilerplate='multiplechoice.yaml') + self.problem_locator = self.response_locator(resp) + + resp = self.create_xblock(parent_locator=self.seq_locator, category='html') + self.html_locator = self.response_locator(resp) + + 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): + locator = self._duplicate_item(parent_locator, source_locator) + original_item = self.get_item_from_modulestore(source_locator, draft=True) + duplicated_item = self.get_item_from_modulestore(locator, draft=True) + + self.assertNotEqual( + original_item.location, + duplicated_item.location, + "Location of duplicate should be different from original" + ) + # 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) + + def test_ordering(self): + """ + Tests the a duplicated xblock appears immediately after its source + (if duplicate and source share the same parent), else at the + end of the children of the parent. + """ + def verify_order(source_locator, parent_locator, source_position=None): + locator = self._duplicate_item(parent_locator, source_locator) + parent = self.get_item_from_modulestore(parent_locator) + children = parent.children + if source_position is None: + self.assertFalse(source_locator in children, 'source item not expected in children array') + self.assertEqual( + children[len(children) - 1], + self.get_old_id(locator).url(), + "duplicated item not at end" + ) + else: + self.assertEqual( + children[source_position], + self.get_old_id(source_locator).url(), + "source item at wrong position" + ) + self.assertEqual( + children[source_position+1], + self.get_old_id(locator).url(), + "duplicated item not ordered after source item" + ) + + 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) + + # Test duplicating something into a location that is not the parent of the original item. + # Duplicated item should appear at the end. + verify_order(self.html_locator, self.unicode_locator) + + def test_display_name(self): + """ + Tests the expected display name for the duplicated xblock. + """ + def verify_name(source_locator, parent_locator, expected_name, display_name=None): + locator = self._duplicate_item(parent_locator, source_locator, display_name) + duplicated_item = self.get_item_from_modulestore(locator, draft=True) + self.assertEqual(duplicated_item.display_name, expected_name) + return locator + + # Display name comes from template. + dupe_locator = verify_name(self.problem_locator, self.seq_locator, "Duplicate of 'Multiple Choice'") + # Test dupe of dupe. + verify_name(dupe_locator, self.seq_locator, "Duplicate of 'Duplicate of 'Multiple Choice''") + + # Uses default display_name of 'Text' from HTML component. + 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'") + + # Now send a custom display name for the duplicate. + verify_name(self.seq_locator, self.unicode_locator, "customized name", display_name="customized name") + + def _duplicate_item(self, parent_locator, source_locator, display_name=None): + data = { + 'parent_locator': parent_locator, + 'duplicate_source_locator': source_locator + } + if display_name is not None: + data['display_name'] = display_name + + resp = self.client.ajax_post('/xblock', json.dumps(data)) + resp_content = json.loads(resp.content) + self.assertEqual(resp.status_code, 200) + return resp_content['locator'] + + class TestEditItem(ItemTest): """ Test xblock update. diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index d6a8ff3abb..fa42f40366 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -33,6 +33,7 @@ from .helpers import _xmodule_recurse from preview import handler_prefix, get_preview_html from edxmako.shortcuts import render_to_response, render_to_string from models.settings.course_grading import CourseGradingModel +from django.utils.translation import ugettext as _ __all__ = ['orphan_handler', 'xblock_handler'] @@ -71,12 +72,15 @@ def xblock_handler(request, tag=None, package_id=None, branch=None, version_guid :publish: can be one of three values, 'make_public, 'make_private', or 'create_draft' The JSON representation on the updated xblock (minus children) is returned. - if xblock locator is not specified, create a new xblock instance. The json playload can contain + if xblock locator is not specified, create a new xblock instance, either by duplicating + an existing xblock, or creating an entirely new one. The json playload can contain these fields: - :parent_locator: parent for new xblock, required - :category: type of xblock, required + :parent_locator: parent for new xblock, required for both duplicate and create new instance + :duplicate_source_locator: if present, use this as the source for creating a duplicate copy + :category: type of xblock, required if duplicate_source_locator is not present. :display_name: name for new xblock, optional - :boilerplate: template name for populating fields, optional + :boilerplate: template name for populating fields, optional and only used + if duplicate_source_locator is not present The locator (and old-style id) for the created xblock (minus children) is returned. """ if package_id is not None: @@ -131,7 +135,17 @@ def xblock_handler(request, tag=None, package_id=None, branch=None, version_guid publish=request.json.get('publish'), ) elif request.method in ('PUT', 'POST'): - return _create_item(request) + 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, + request.json.get('display_name') + ) + return JsonResponse({"locator": unicode(new_locator)}) + else: + return _create_item(request) else: return HttpResponseBadRequest( "Only instance creation is supported without a package_id.", @@ -286,6 +300,52 @@ def _create_item(request): return JsonResponse({"locator": unicode(locator)}) +def _duplicate_item(parent_locator, duplicate_source_locator, display_name): + """ + Duplicate an existing xblock as a child of the supplied parent_locator. + """ + 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. + dest_location = duplicate_source_location.replace(name=uuid4().hex) + category = dest_location.category + + # Update the display name to indicate this is a duplicate (unless display name provided). + duplicate_metadata = own_metadata(source_item) + if display_name is not None: + duplicate_metadata['display_name'] = display_name + else: + duplicate_metadata['display_name'] = _("Duplicate of '{0}'").format(source_item.display_name) + + get_modulestore(category).create_and_save_xmodule( + dest_location, + definition_data=source_item.data if hasattr(source_item, 'data') else None, + metadata=duplicate_metadata, + 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) + + if category not in DETACHED_CATEGORIES: + parent = get_modulestore(parent_location).get_item(parent_location) + # If source was already a child of the parent, add duplicate immediately afterward. + # 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()) + 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) + + def _delete_item_at_location(item_location, delete_children=False, delete_all_versions=False): """ Deletes the item at with the given Location. From 81fecf3e6ceb515b449aed8cb82f79f08f9f78d7 Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 15 Jan 2014 16:16:55 -0500 Subject: [PATCH 2/2] 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):