Change Reference and ReferenceList instances on import to new namespace.

STUD-149

CHANGELOG.rst

@@ -5,6 +5,8 @@ These are notable changes in edx-platform.  This is a rolling list of changes,
 in roughly chronological order, most recent first.  Add your entries at or near
 the top.  Include a label indicating the component affected.
 
+Studo: Fix import/export bug with conditional modules. STUD-149
+
 Blades: Persist student progress in video. BLD-385.
 
 Blades: Fix for the list metadata editor that gets into a bad state where "Add"

cms/djangoapps/contentstore/tests/test_import_nostatic.py → cms/djangoapps/contentstore/tests/test_import.py

 #pylint: disable=E1101
-'''
-Tests for importing with no static
-'''
+"""
+Tests for import_from_xml using the mongo modulestore.
+"""
 
 from django.test.client import Client
 from django.test.utils import override_settings
@@ -32,7 +32,7 @@ TEST_DATA_CONTENTSTORE['DOC_STORE_CONFIG']['db'] = 'test_xcontent_%s' % uuid4().
 
 
 @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE, MODULESTORE=TEST_MODULESTORE)
-class ContentStoreImportNoStaticTest(ModuleStoreTestCase):
+class ContentStoreImportTest(ModuleStoreTestCase):
     """
     Tests that rely on the toy and test_import_course courses.
     NOTE: refactor using CourseFactory so they do not.
@@ -130,7 +130,52 @@ class ContentStoreImportNoStaticTest(ModuleStoreTestCase):
         self.assertIn('/static/', handouts.data)
 
     def test_tab_name_imports_correctly(self):
-        module_store, content_store, course, course_location = self.load_test_import_course()
+        _module_store, _content_store, course, _course_location = self.load_test_import_course()
         print "course tabs = {0}".format(course.tabs)
-        self.assertEqual(course.tabs[2]['name'],'Syllabus')
+        self.assertEqual(course.tabs[2]['name'], 'Syllabus')
 
+    def test_rewrite_reference_list(self):
+        module_store = modulestore('direct')
+        target_location = Location(['i4x', 'testX', 'conditional_copy', 'course', 'copy_run'])
+        import_from_xml(
+            module_store,
+            'common/test/data/',
+            ['conditional'],
+            target_location_namespace=target_location
+        )
+        conditional_module = module_store.get_item(
+            Location(['i4x', 'testX', 'conditional_copy', 'conditional', 'condone'])
+        )
+        self.assertIsNotNone(conditional_module)
+        self.assertListEqual(
+            [
+                u'i4x://testX/conditional_copy/problem/choiceprob',
+                u'i4x://edX/different_course/html/for_testing_import_rewrites'
+            ],
+            conditional_module.sources_list
+        )
+        self.assertListEqual(
+            [
+                u'i4x://testX/conditional_copy/html/congrats',
+                u'i4x://testX/conditional_copy/html/secret_page'
+            ],
+            conditional_module.show_tag_list
+        )
+
+    def test_rewrite_reference(self):
+        module_store = modulestore('direct')
+        target_location = Location(['i4x', 'testX', 'peergrading_copy', 'course', 'copy_run'])
+        import_from_xml(
+            module_store,
+            'common/test/data/',
+            ['open_ended'],
+            target_location_namespace=target_location
+        )
+        peergrading_module = module_store.get_item(
+            Location(['i4x', 'testX', 'peergrading_copy', 'peergrading', 'PeerGradingLinked'])
+        )
+        self.assertIsNotNone(peergrading_module)
+        self.assertEqual(
+            u'i4x://testX/peergrading_copy/combinedopenended/SampleQuestion',
+            peergrading_module.link_to_location
+        )

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -6,6 +6,7 @@ import json
 
 from .xml import XMLModuleStore, ImportSystem, ParentTracker
 from xmodule.modulestore import Location
+from xblock.fields import Scope, Reference, ReferenceList
 from xmodule.contentstore.content import StaticContent
 from .inheritance import own_metadata
 from xmodule.errortracker import make_error_tracker
@@ -424,7 +425,7 @@ def import_course_draft(
                     # aka sequential), so we have to replace the location.name
                     # with the XML filename that is part of the pack
                     fn, fileExtension = os.path.splitext(filename)
-                    descriptor.location = descriptor.location._replace(name=fn)
+                    descriptor.location = descriptor.location.replace(name=fn)
 
                     index = int(descriptor.xml_attributes['index_in_children_list'])
                     if index in drafts:
@@ -442,13 +443,13 @@ def import_course_draft(
             for descriptor in drafts[key]:
                 try:
                     def _import_module(module):
-                        module.location = module.location._replace(revision='draft')
+                        module.location = module.location.replace(revision='draft')
                         # make sure our parent has us in its list of children
                         # this is to make sure private only verticals show up
                         # in the list of children since they would have been
                         # filtered out from the non-draft store export
                         if module.location.category == 'vertical':
-                            non_draft_location = module.location._replace(revision=None)
+                            non_draft_location = module.location.replace(revision=None)
                             sequential_url = module.xml_attributes['parent_sequential_url']
                             index = int(module.xml_attributes['index_in_children_list'])
 
@@ -456,7 +457,7 @@ def import_course_draft(
 
                             # IMPORTANT: Be sure to update the sequential
                             # in the NEW namespace
-                            seq_location = seq_location._replace(
+                            seq_location = seq_location.replace(
                                 org=target_location_namespace.org,
                                 course=target_location_namespace.course
                             )
@@ -486,20 +487,21 @@ def remap_namespace(module, target_location_namespace):
     if target_location_namespace is None:
         return module
 
+    original_location = module.location
+
     # This looks a bit wonky as we need to also change the 'name' of the
     # imported course to be what the caller passed in
     if module.location.category != 'course':
-        module.location = module.location._replace(
+        module.location = module.location.replace(
             tag=target_location_namespace.tag,
             org=target_location_namespace.org,
             course=target_location_namespace.course
         )
     else:
-        original_location = module.location
         #
         # module is a course module
         #
-        module.location = module.location._replace(
+        module.location = module.location.replace(
             tag=target_location_namespace.tag,
             org=target_location_namespace.org,
             course=target_location_namespace.course,
@@ -524,22 +526,41 @@ def remap_namespace(module, target_location_namespace):
 
         module.save()
 
-    # then remap children pointers since they too will be re-namespaced
+    all_fields = module.get_explicitly_set_fields_by_scope(Scope.content)
+    all_fields.update(module.get_explicitly_set_fields_by_scope(Scope.settings))
     if hasattr(module, 'children'):
-        children_locs = module.children
-        if children_locs is not None and children_locs != []:
-            new_locs = []
-            for child in children_locs:
-                child_loc = Location(child)
-                new_child_loc = child_loc._replace(
+        all_fields['children'] = module.children
+
+    def convert_ref(reference):
+        """
+        Convert a reference to the new namespace, but only
+        if the original namespace matched the original course.
+
+        Otherwise, returns the input value.
+        """
+        new_ref = reference
+        ref = Location(reference)
+        in_original_namespace = (original_location.tag == ref.tag and
+                                 original_location.org == ref.org and
+                                 original_location.course == ref.course)
+        if in_original_namespace:
+            new_ref = ref.replace(
                 tag=target_location_namespace.tag,
                 org=target_location_namespace.org,
                 course=target_location_namespace.course
-                )
-
-                new_locs.append(new_child_loc.url())
+            ).url()
+        return new_ref
 
-            module.children = new_locs
+    for field in all_fields:
+        if isinstance(module.fields.get(field), Reference):
+            new_ref = convert_ref(getattr(module, field))
+            setattr(module, field, new_ref)
+            module.save()
+        elif isinstance(module.fields.get(field), ReferenceList):
+            references = getattr(module, field)
+            new_references = [convert_ref(reference) for reference in references]
+            setattr(module, field, new_references)
+            module.save()
 
     return module
 

common/test/data/conditional/README.md

-course for testing conditional module
+Course for testing conditional module.
+
+Note that 'i4x://edX/different_course/html/for_testing_import_rewrites' in sources
+is only present for testing that import does NOT rewrite references to
+courses that differ from the original course being imported.
+
+It is not expected that i4x://edX/different_course/html/for_testing_import_rewrites will render.
 
  

common/test/data/conditional/conditional/condone.xml

-<conditional correct="True" sources="i4x://edX/cond_test/problem/choiceprob">
+<conditional correct="True" sources="i4x://edX/cond_test/problem/choiceprob; i4x://edX/different_course/html/for_testing_import_rewrites">
     <html>Conditionally shown page</html>
     <show sources="i4x://edX/cond_test/html/congrats; i4x://edX/cond_test/html/secret_page"/>
 </conditional>