Merge pull request #6936 from edx/christina/tnl-1362

If existing course is found on import make sure dest_course_id is correc...

cms/djangoapps/contentstore/management/commands/tests/test_import.py

@@ -12,7 +12,7 @@ from django.core.management import call_command
 from django_comment_common.utils import are_permissions_roles_seeded
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
-from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from xmodule.modulestore import ModuleStoreEnum
 
 
 class TestImport(ModuleStoreTestCase):
@@ -20,10 +20,6 @@ class TestImport(ModuleStoreTestCase):
     Unit tests for importing a course from command line
     """
 
-    BASE_COURSE_KEY = SlashSeparatedCourseKey(u'edX', u'test_import_course', u'2013_Spring')
-    DIFF_KEY = SlashSeparatedCourseKey(u'edX', u'test_import_course', u'2014_Spring')
-    TRUNCATED_KEY = SlashSeparatedCourseKey(u'edX', u'test_import', u'2014_Spring')
-
     def create_course_xml(self, content_dir, course_id):
         directory = tempfile.mkdtemp(dir=content_dir)
         os.makedirs(os.path.join(directory, "course"))
@@ -32,7 +28,7 @@ class TestImport(ModuleStoreTestCase):
                     'course="{0.course}"/>'.format(course_id))
 
         with open(os.path.join(directory, "course", "{0.run}.xml".format(course_id)), "w+") as f:
-            f.write('<course></course>')
+            f.write('<course><chapter name="Test Chapter"></chapter></course>')
 
         return directory
 
@@ -44,38 +40,23 @@ class TestImport(ModuleStoreTestCase):
         self.content_dir = path(tempfile.mkdtemp())
         self.addCleanup(shutil.rmtree, self.content_dir)
 
-        # Create good course xml
-        self.good_dir = self.create_course_xml(self.content_dir, self.BASE_COURSE_KEY)
+        self.base_course_key = self.store.make_course_key(u'edX', u'test_import_course', u'2013_Spring')
+        self.truncated_key = self.store.make_course_key(u'edX', u'test_import', u'2014_Spring')
 
-        # Create run changed course xml
-        self.dupe_dir = self.create_course_xml(self.content_dir, self.DIFF_KEY)
+        # Create good course xml
+        self.good_dir = self.create_course_xml(self.content_dir, self.base_course_key)
 
         # Create course XML where TRUNCATED_COURSE.org == BASE_COURSE_ID.org
         # and BASE_COURSE_ID.startswith(TRUNCATED_COURSE.course)
-        self.course_dir = self.create_course_xml(self.content_dir, self.TRUNCATED_KEY)
+        self.course_dir = self.create_course_xml(self.content_dir, self.truncated_key)
 
     def test_forum_seed(self):
         """
         Tests that forum roles were created with import.
         """
-        self.assertFalse(are_permissions_roles_seeded(self.BASE_COURSE_KEY))
+        self.assertFalse(are_permissions_roles_seeded(self.base_course_key))
         call_command('import', self.content_dir, self.good_dir)
-        self.assertTrue(are_permissions_roles_seeded(self.BASE_COURSE_KEY))
-
-    def test_duplicate_with_url(self):
-        """
-        Check to make sure an import doesn't import courses that have the
-        same org and course, but they have different runs in order to
-        prevent modulestore "findone" exceptions on deletion
-        """
-        # Load up base course and verify it is available
-        call_command('import', self.content_dir, self.good_dir)
-        store = modulestore()
-        self.assertIsNotNone(store.get_course(self.BASE_COURSE_KEY))
-
-        # Now load up duped course and verify it doesn't load
-        call_command('import', self.content_dir, self.dupe_dir)
-        self.assertIsNone(store.get_course(self.DIFF_KEY))
+        self.assertTrue(are_permissions_roles_seeded(self.base_course_key))
 
     def test_truncated_course_with_url(self):
         """
@@ -87,8 +68,28 @@ class TestImport(ModuleStoreTestCase):
         # Load up base course and verify it is available
         call_command('import', self.content_dir, self.good_dir)
         store = modulestore()
-        self.assertIsNotNone(store.get_course(self.BASE_COURSE_KEY))
+        self.assertIsNotNone(store.get_course(self.base_course_key))
 
         # Now load up the course with a similar course_id and verify it loads
         call_command('import', self.content_dir, self.course_dir)
-        self.assertIsNotNone(store.get_course(self.TRUNCATED_KEY))
+        self.assertIsNotNone(store.get_course(self.truncated_key))
+
+    def test_existing_course_with_different_modulestore(self):
+        """
+        Checks that a course that originally existed in old mongo can be re-imported when
+        split is the default modulestore.
+        """
+        with modulestore().default_store(ModuleStoreEnum.Type.mongo):
+            call_command('import', self.content_dir, self.good_dir)
+
+        # Clear out the modulestore mappings, else when the next import command goes to create a destination
+        # course_key, it will find the existing course and return the mongo course_key. To reproduce TNL-1362,
+        # the destination course_key needs to be the one for split modulestore.
+        modulestore().mappings = {}
+
+        with modulestore().default_store(ModuleStoreEnum.Type.split):
+            call_command('import', self.content_dir, self.good_dir)
+            course = modulestore().get_course(self.base_course_key)
+            # With the bug, this fails because the chapter's course_key is the split mongo form,
+            # while the course's course_key is the old mongo form.
+            self.assertEqual(unicode(course.location.course_key), unicode(course.children[0].course_key))

cms/djangoapps/contentstore/tests/test_import.py

@@ -83,21 +83,24 @@ class ContentStoreImportTest(ModuleStoreTestCase):
         """
         # Test that importing course with unicode 'id' and 'display name' doesn't give UnicodeEncodeError
         """
-        module_store = modulestore()
-        course_id = SlashSeparatedCourseKey(u'Юникода', u'unicode_course', u'échantillon')
-        import_from_xml(
-            module_store,
-            self.user.id,
-            TEST_DATA_DIR,
-            ['2014_Uni'],
-            target_course_id=course_id
-        )
+        # Test with the split modulestore because store.has_course fails in old mongo with unicode characters.
+        with modulestore().default_store(ModuleStoreEnum.Type.split):
+            module_store = modulestore()
+            course_id = module_store.make_course_key(u'Юникода', u'unicode_course', u'échantillon')
+            import_from_xml(
+                module_store,
+                self.user.id,
+                TEST_DATA_DIR,
+                ['2014_Uni'],
+                target_course_id=course_id,
+                create_course_if_not_present=True
+            )
 
-        course = module_store.get_course(course_id)
-        self.assertIsNotNone(course)
+            course = module_store.get_course(course_id)
+            self.assertIsNotNone(course)
 
-        # test that course 'display_name' same as imported course 'display_name'
-        self.assertEqual(course.display_name, u"Φυσικά το όνομα Unicode")
+            # test that course 'display_name' same as imported course 'display_name'
+            self.assertEqual(course.display_name, u"Φυσικά το όνομα Unicode")
 
     def test_static_import(self):
         '''

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

@@ -195,11 +195,18 @@ def import_from_xml(
         if target_course_id is not None:
             dest_course_id = target_course_id
         else:
+            # Note that dest_course_id will be in the format for the default modulestore.
             dest_course_id = store.make_course_key(course_key.org, course_key.course, course_key.run)
 
+        existing_course_id = store.has_course(dest_course_id, ignore_case=True)
+        # store.has_course will return the course_key in the format for the modulestore in which it was found.
+        # This may be different from dest_course_id, so correct to the format found.
+        if existing_course_id:
+            dest_course_id = existing_course_id
+
         runtime = None
         # Creates a new course if it doesn't already exist
-        if create_course_if_not_present and not store.has_course(dest_course_id, ignore_case=True):
+        if create_course_if_not_present and not existing_course_id:
             try:
                 new_course = store.create_course(dest_course_id.org, dest_course_id.course, dest_course_id.run, user_id)
                 runtime = new_course.runtime