Merge pull request #3626 from edx/adam/fix-import-bug

fix importing bug (STUD-1599)

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

@@ -22,8 +22,21 @@ class TestImport(ModuleStoreTestCase):
     Unit tests for importing a course from command line
     """
 
-    COURSE_ID = ['EDx', '0.00x', '2013_Spring', ]
+    BASE_COURSE_ID = ['EDx', '0.00x', '2013_Spring', ]
     DIFF_RUN = ['EDx', '0.00x', '2014_Spring', ]
+    TRUNCATED_COURSE = ['EDx', '0.00', '2014_Spring', ]
+
+    def create_course_xml(self, content_dir, course_id):
+        directory = tempfile.mkdtemp(dir=content_dir)
+        os.makedirs(os.path.join(directory, "course"))
+        with open(os.path.join(directory, "course.xml"), "w+") as f:
+            f.write('<course url_name="{0[2]}" org="{0[0]}" '
+                    'course="{0[1]}"/>'.format(course_id))
+
+        with open(os.path.join(directory, "course", "{0[2]}.xml".format(course_id)), "w+") as f:
+            f.write('<course></course>')
+
+        return directory
 
     def setUp(self):
         """
@@ -34,32 +47,22 @@ class TestImport(ModuleStoreTestCase):
         self.addCleanup(shutil.rmtree, self.content_dir)
 
         # Create good course xml
-        self.good_dir = tempfile.mkdtemp(dir=self.content_dir)
-        os.makedirs(os.path.join(self.good_dir, "course"))
-        with open(os.path.join(self.good_dir, "course.xml"), "w+") as f:
-            f.write('<course url_name="{0[2]}" org="{0[0]}" '
-                    'course="{0[1]}"/>'.format(self.COURSE_ID))
-
-        with open(os.path.join(self.good_dir, "course", "{0[2]}.xml".format(self.COURSE_ID)), "w+") as f:
-            f.write('<course></course>')
+        self.good_dir = self.create_course_xml(self.content_dir, self.BASE_COURSE_ID)
 
         # Create run changed course xml
-        self.dupe_dir = tempfile.mkdtemp(dir=self.content_dir)
-        os.makedirs(os.path.join(self.dupe_dir, "course"))
-        with open(os.path.join(self.dupe_dir, "course.xml"), "w+") as f:
-            f.write('<course url_name="{0[2]}" org="{0[0]}" '
-                    'course="{0[1]}"/>'.format(self.DIFF_RUN))
+        self.dupe_dir = self.create_course_xml(self.content_dir, self.DIFF_RUN)
 
-        with open(os.path.join(self.dupe_dir, "course", "{0[2]}.xml".format(self.DIFF_RUN)), "w+") as f:
-            f.write('<course></course>')
+        # 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_COURSE)
 
     def test_forum_seed(self):
         """
         Tests that forum roles were created with import.
         """
-        self.assertFalse(are_permissions_roles_seeded('/'.join(self.COURSE_ID)))
+        self.assertFalse(are_permissions_roles_seeded('/'.join(self.BASE_COURSE_ID)))
         call_command('import', self.content_dir, self.good_dir)
-        self.assertTrue(are_permissions_roles_seeded('/'.join(self.COURSE_ID)))
+        self.assertTrue(are_permissions_roles_seeded('/'.join(self.BASE_COURSE_ID)))
 
     def test_duplicate_with_url(self):
         """
@@ -70,8 +73,24 @@ 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('/'.join(self.COURSE_ID)))
+        self.assertIsNotNone(store.get_course('/'.join(self.BASE_COURSE_ID)))
 
         # 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('/'.join(self.DIFF_RUN)))
+
+    def test_truncated_course_with_url(self):
+        """
+        Check to make sure an import only blocks true duplicates: new
+        courses with similar but not unique org/course combinations aren't
+        blocked if the original course's course starts with the new course's
+        org/course combinations (i.e. EDx/0.00x/Spring -> EDx/0.00/Spring)
+        """
+        # 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('/'.join(self.BASE_COURSE_ID)))
+
+        # 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('/'.join(self.TRUNCATED_COURSE)))

cms/djangoapps/contentstore/tests/test_import.py

@@ -67,17 +67,41 @@ class ContentStoreImportTest(ModuleStoreTestCase):
 
     def load_test_import_course(self):
         '''
-        Load the standard course used to test imports (for do_import_static=False behavior).
+        Load the standard course used to test imports
+        (for do_import_static=False behavior).
         '''
         content_store = contentstore()
         module_store = modulestore('direct')
-        import_from_xml(module_store, 'common/test/data/', ['test_import_course'], static_content_store=content_store, do_import_static=False, verbose=True)
-        course_location = CourseDescriptor.id_to_location('edX/test_import_course/2012_Fall')
+        import_from_xml(
+            module_store,
+            'common/test/data/',
+            ['test_import_course'],
+            static_content_store=content_store,
+            do_import_static=False,
+            verbose=True,
+        )
+        course_location = CourseDescriptor.id_to_location(
+            'edX/test_import_course/2012_Fall'
+        )
         course = module_store.get_item(course_location)
         self.assertIsNotNone(course)
 
         return module_store, content_store, course, course_location
 
+    def test_import_course_into_similar_namespace(self):
+        # Checks to make sure that a course with an org/course like
+        # edx/course can be imported into a namespace with an org/course
+        # like edx/course_name
+        module_store, __, __, course_location = self.load_test_import_course()
+        __, course_items = import_from_xml(
+            module_store,
+            'common/test/data',
+            ['test_import_course_2'],
+            target_location_namespace=course_location,
+            verbose=True,
+        )
+        self.assertEqual(len(course_items), 1)
+
     def test_unicode_chars_in_course_name_import(self):
         """
         # Test that importing course with unicode 'id' and 'display name' doesn't give UnicodeEncodeError

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

@@ -176,29 +176,32 @@ def import_from_xml(
                 if module.scope_ids.block_type == 'course':
                     course_data_path = path(data_dir) / module.data_dir
                     course_location = module.location
-                    course_prefix = u'{0.org}/{0.course}'.format(course_location)
+                    course_org_lower = course_location.org.lower()
+                    course_number_lower = course_location.course.lower()
 
                     # Check to see if a course with the same
                     # pseudo_course_id, but different run exists in
                     # the passed store to avoid broken courses
                     courses = store.get_courses()
                     bad_run = False
-                    for course in courses:
-                        if course.location.course_id.startswith(course_prefix):
-                            log.debug('Import is overwriting existing course')
-                            # Importing over existing course, check
-                            # that runs match or fail
-                            if course.location.name != module.location.name:
-                                log.error(
-                                    'A course with ID %s exists, and this '
-                                    'course has the same organization and '
-                                    'course number, but a different term that '
-                                    'is fully identified as %s.',
-                                    course.location.course_id,
-                                    module.location.course_id
-                                )
-                                bad_run = True
-                                break
+                    if target_location_namespace is None:
+                        for course in courses:
+                            if course.location.org.lower() == course_org_lower and \
+                            course.location.course.lower() == course_number_lower:
+                                log.debug('Import is overwriting existing course')
+                                # Importing over existing course, check
+                                # that runs match or fail
+                                if course.location.name != module.location.name:
+                                    log.error(
+                                        'A course with ID %s exists, and this '
+                                        'course has the same organization and '
+                                        'course number, but a different term that '
+                                        'is fully identified as %s.',
+                                        course.location.course_id,
+                                        module.location.course_id
+                                    )
+                                    bad_run = True
+                                    break
                     if bad_run:
                         # Skip this course, but keep trying to import courses
                         continue

common/test/data/test_import_course_2/about/end_date.html

@@ -0,0 +1 @@
+TBD

common/test/data/test_import_course_2/chapter/vertical_container.xml

@@ -0,0 +1,3 @@
+<sequential>
+    <sequential filename='vertical_sequential' slug='vertical_sequential' />
+</sequential>

common/test/data/test_import_course_2/course.xml

@@ -0,0 +1 @@
+<course org="edX" course="test_import" url_name="2014_Fall"/>

common/test/data/test_import_course_2/course/2014_Fall.xml

@@ -0,0 +1,20 @@
+<course>
+    <textbook title="Textbook" book_url="https://s3.amazonaws.com/edx-textbooks/guttag_computation_v3/"/>
+  <chapter url_name="Overview">
+    <videosequence url_name="Toy_Videos">
+      <html url_name="secret:toylab"/>
+      <html url_name="toyjumpto"/>
+      <html url_name="toyhtml"/>
+      <html url_name="nonportable"/>
+      <html url_name="nonportable_link"/>
+      <video url_name="Video_Resources" youtube_id_1_0="1bK-WdDi6Qw" display_name="Video Resources"/>
+    </videosequence>
+    <video url_name="Welcome" youtube_id_1_0="p2Q6BrNhdh8" display_name="Welcome"/>
+    <video url_name="video_123456789012" youtube_id_1_0="p2Q6BrNhdh8" display_name='Test Video'/>
+    <video url_name="video_4f66f493ac8f" youtube_id_1_0="p2Q6BrNhdh8"/>
+  </chapter>
+  <chapter url_name="secret:magic"/>
+  <chapter url_name="poll_test"/>
+  <chapter url_name="vertical_container"/>
+  <chapter url_name="handout_container"/>
+</course>

common/test/data/test_import_course_2/info/handouts.html

@@ -0,0 +1 @@
+<a href='/static/handouts/sample_handout.txt'>Sample</a>

common/test/data/test_import_course_2/policies/2012_Fall.json

@@ -0,0 +1,33 @@
+{
+    "course/2012_Fall": {
+        "graceperiod": "2 days 5 hours 59 minutes 59 seconds",
+        "start": "2015-07-17T12:00",
+        "display_name": "Toy Course",
+        "graded": "true",
+	"tabs": [
+          {"type": "courseware"},
+          {"type": "course_info", "name": "Course Info"},
+          {"type": "static_tab", "url_slug": "syllabus", "name": "Syllabus"},
+          {"type": "static_tab", "url_slug": "resources", "name": "Resources"},
+          {"type": "discussion", "name": "Discussion"},
+          {"type": "wiki", "name": "Wiki"},
+          {"type": "progress", "name": "Progress"}
+        ]
+    },
+    "chapter/Overview": {
+        "display_name": "Overview"
+    },
+    "videosequence/Toy_Videos": {
+        "display_name": "Toy Videos",
+        "format": "Lecture Sequence"
+    },
+    "html/secret:toylab": {
+        "display_name": "Toy lab"
+    },
+    "video/Video_Resources": {
+        "display_name": "Video Resources"
+    },
+    "video/Welcome": {
+        "display_name": "Welcome"
+    }
+}

common/test/data/test_import_course_2/sequential/vertical_sequential.xml

@@ -0,0 +1,4 @@
+<sequential>
+    <vertical filename="vertical_test" slug="vertical_test" />
+    <html slug="unicode">…</html>
+</sequential>

common/test/data/test_import_course_2/vertical/vertical_test.xml

@@ -0,0 +1,9 @@
+<sequential>
+    <video display_name="default" youtube_id_0_75="JMD_ifUUfsU" youtube_id_1_0="OEoXaMPEzfM" youtube_id_1_25="AKqURZnYqpk" youtube_id_1_5="DYpADpL7jAY" name="sample_video"/>
+    <video url_name="separate_file_video"/>
+    <poll_question name="T1_changemind_poll_foo_2" display_name="Change your answer" reset="false">
+        <p>Have you changed your mind?</p>
+        <answer id="yes">Yes</answer>
+        <answer id="no">No</answer>
+    </poll_question>
+</sequential>

common/test/data/test_import_course_2/video/separate_file_video.xml

@@ -0,0 +1 @@
+<video display_name="default" youtube_id_0_75="JMD_ifUUfsU" youtube_id_1_0="OEoXaMPEzfM" youtube_id_1_25="AKqURZnYqpk" youtube_id_1_5="DYpADpL7jAY" name="sample_video"/>