Merge pull request #609 from edx/fix/cdodge/clone-course-needs-to-save-only-own-metadata

[STUD-608] django-admin clone_course needs to properly copy metadata as well as draft content

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -617,8 +617,26 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         }
 
         module_store = modulestore('direct')
+        draft_store = modulestore('draft')
         import_from_xml(module_store, 'common/test/data/', ['toy'])
 
+        source_course_id = 'edX/toy/2012_Fall'
+        dest_course_id = 'MITx/999/2013_Spring'
+        source_location = CourseDescriptor.id_to_location(source_course_id)
+        dest_location = CourseDescriptor.id_to_location(dest_course_id)
+
+        # get a vertical (and components in it) to put into 'draft'
+        # this is to assert that draft content is also cloned over
+        vertical = module_store.get_instance(source_course_id, Location([
+            source_location.tag, source_location.org, source_location.course, 'vertical', 'vertical_test', None]), depth=1)
+
+        draft_store.convert_to_draft(vertical.location)
+        for child in vertical.get_children():
+            draft_store.convert_to_draft(child.location)
+
+        items = module_store.get_items(Location([source_location.tag, source_location.org, source_location.course, None, None, 'draft']))
+        self.assertGreater(len(items), 0)
+
         resp = self.client.post(reverse('create_new_course'), course_data)
         self.assertEqual(resp.status_code, 200)
         data = parse_json(resp)
@@ -626,22 +644,54 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         content_store = contentstore()
 
-        source_location = CourseDescriptor.id_to_location('edX/toy/2012_Fall')
-        dest_location = CourseDescriptor.id_to_location('MITx/999/2013_Spring')
-
         clone_course(module_store, content_store, source_location, dest_location)
 
+        # first assert that all draft content got cloned as well
+        items = module_store.get_items(Location([source_location.tag, source_location.org, source_location.course, None, None, 'draft']))
+        self.assertGreater(len(items), 0)
+        clone_items = module_store.get_items(Location([dest_location.tag, dest_location.org, dest_location.course, None, None, 'draft']))
+        self.assertGreater(len(clone_items), 0)
+        self.assertEqual(len(items), len(clone_items))
+
         # now loop through all the units in the course and verify that the clone can render them, which
         # means the objects are at least present
-        items = module_store.get_items(Location(['i4x', 'edX', 'toy', 'poll_question', None]))
+        items = module_store.get_items(Location([source_location.tag, source_location.org, source_location.course, None, None]))
         self.assertGreater(len(items), 0)
-        clone_items = module_store.get_items(Location(['i4x', 'MITx', '999', 'poll_question', None]))
+        clone_items = module_store.get_items(Location([dest_location.tag, dest_location.org, dest_location.course, None, None]))
         self.assertGreater(len(clone_items), 0)
+
         for descriptor in items:
-            new_loc = descriptor.location.replace(org='MITx', course='999')
+            source_item = module_store.get_instance(source_course_id, descriptor.location)
+            if descriptor.location.category == 'course':
+                new_loc = descriptor.location.replace(org=dest_location.org, course=dest_location.course, name='2013_Spring')
+            else:
+                new_loc = descriptor.location.replace(org=dest_location.org, course=dest_location.course)
             print "Checking {0} should now also be at {1}".format(descriptor.location.url(), new_loc.url())
-            resp = self.client.get(reverse('edit_unit', kwargs={'location': new_loc.url()}))
-            self.assertEqual(resp.status_code, 200)
+            lookup_item = module_store.get_item(new_loc)
+
+            # we want to assert equality between the objects, but we know the locations
+            # differ, so just make them equal for testing purposes
+            source_item.location = new_loc
+            if hasattr(source_item, 'data') and hasattr(lookup_item, 'data'):
+                self.assertEqual(source_item.data, lookup_item.data)
+
+            # also make sure that metadata was cloned over and filtered with own_metadata, i.e. inherited
+            # values were not explicitly set
+            self.assertEqual(own_metadata(source_item), own_metadata(lookup_item))
+
+            # check that the children are as expected
+            self.assertEqual(source_item.has_children, lookup_item.has_children)
+            if source_item.has_children:
+                expected_children = []
+                for child_loc_url in source_item.children:
+                    child_loc = Location(child_loc_url)
+                    child_loc = child_loc._replace(
+                        tag=dest_location.tag,
+                        org=dest_location.org,
+                        course=dest_location.course
+                    )
+                    expected_children.append(child_loc.url())
+                self.assertEqual(expected_children, lookup_item.children)
 
     def test_illegal_draft_crud_ops(self):
         draft_store = modulestore('draft')

common/lib/xmodule/xmodule/modulestore/store_utilities.py

 from xmodule.contentstore.content import StaticContent
 from xmodule.modulestore import Location
 from xmodule.modulestore.mongo import MongoModuleStore
+from xmodule.modulestore.inheritance import own_metadata
 
 import logging
 
 
+def _clone_modules(modulestore, modules, dest_location):
+    for module in modules:
+        original_loc = Location(module.location)
+
+        if original_loc.category != 'course':
+            module.location = module.location._replace(
+                tag=dest_location.tag, org=dest_location.org, course=dest_location.course)
+        else:
+            # on the course module we also have to update the module name
+            module.location = module.location._replace(
+                tag=dest_location.tag, org=dest_location.org, course=dest_location.course, name=dest_location.name)
+
+        print "Cloning module {0} to {1}....".format(original_loc, module.location)
+
+        # NOTE: usage of the the internal module.xblock_kvs._data does not include any 'default' values for the fields
+        modulestore.update_item(module.location, module.xblock_kvs._data)
+
+        # repoint children
+        if module.has_children:
+            new_children = []
+            for child_loc_url in module.children:
+                child_loc = Location(child_loc_url)
+                child_loc = child_loc._replace(
+                    tag=dest_location.tag,
+                    org=dest_location.org,
+                    course=dest_location.course
+                )
+                new_children.append(child_loc.url())
+
+            modulestore.update_children(module.location, new_children)
+
+        # save metadata
+        modulestore.update_metadata(module.location, own_metadata(module))
+
+
 def clone_course(modulestore, contentstore, source_location, dest_location, delete_original=False):
     # first check to see if the modulestore is Mongo backed
     if not isinstance(modulestore, MongoModuleStore):
@@ -37,38 +73,10 @@ def clone_course(modulestore, contentstore, source_location, dest_location, dele
     # Get all modules under this namespace which is (tag, org, course) tuple
 
     modules = modulestore.get_items([source_location.tag, source_location.org, source_location.course, None, None, None])
+    _clone_modules(modulestore, modules, dest_location)
 
-    for module in modules:
-        original_loc = Location(module.location)
-
-        if original_loc.category != 'course':
-            module.location = module.location._replace(tag=dest_location.tag, org=dest_location.org,
-                                                       course=dest_location.course)
-        else:
-            # on the course module we also have to update the module name
-            module.location = module.location._replace(tag=dest_location.tag, org=dest_location.org,
-                                                       course=dest_location.course, name=dest_location.name)
-
-        print "Cloning module {0} to {1}....".format(original_loc, module.location)
-
-        modulestore.update_item(module.location, module._model_data._kvs._data)
-
-        # repoint children
-        if module.has_children:
-            new_children = []
-            for child_loc_url in module.children:
-                child_loc = Location(child_loc_url)
-                child_loc = child_loc._replace(
-                    tag=dest_location.tag,
-                    org=dest_location.org,
-                    course=dest_location.course
-                )
-                new_children.append(child_loc.url())
-
-            modulestore.update_children(module.location, new_children)
-
-        # save metadata
-        modulestore.update_metadata(module.location, module._model_data._kvs._metadata)
+    modules = modulestore.get_items([source_location.tag, source_location.org, source_location.course, None, None, 'draft'])
+    _clone_modules(modulestore, modules, dest_location)
 
     # now iterate through all of the assets and clone them
     # first the thumbnails