Duplicate children so as to not create a DAG.

2014-01-15 16:16:55 -05:00
parent 0d8d7cb393
commit 81fecf3e6c
2 changed files with 67 additions and 27 deletions
--- 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 = {
--- 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):