From a51a0c558e7b1032b2db00b1234f9464aae1c2a8 Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Tue, 6 May 2014 16:35:18 -0400 Subject: [PATCH] fix importing bug (STUD-1599) add target_location_namespace check --- .../management/commands/tests/test_import.py | 57 ++++++++++++------- .../contentstore/tests/test_import.py | 30 +++++++++- .../xmodule/modulestore/xml_importer.py | 37 ++++++------ .../test_import_course_2/about/end_date.html | 1 + .../chapter/vertical_container.xml | 3 + .../test/data/test_import_course_2/course.xml | 1 + .../test_import_course_2/course/2014_Fall.xml | 20 +++++++ .../test_import_course_2/info/handouts.html | 1 + .../policies/2012_Fall.json | 33 +++++++++++ .../sequential/vertical_sequential.xml | 4 ++ .../vertical/vertical_test.xml | 9 +++ .../video/separate_file_video.xml | 1 + 12 files changed, 158 insertions(+), 39 deletions(-) create mode 100644 common/test/data/test_import_course_2/about/end_date.html create mode 100644 common/test/data/test_import_course_2/chapter/vertical_container.xml create mode 100644 common/test/data/test_import_course_2/course.xml create mode 100644 common/test/data/test_import_course_2/course/2014_Fall.xml create mode 100644 common/test/data/test_import_course_2/info/handouts.html create mode 100644 common/test/data/test_import_course_2/policies/2012_Fall.json create mode 100644 common/test/data/test_import_course_2/sequential/vertical_sequential.xml create mode 100644 common/test/data/test_import_course_2/vertical/vertical_test.xml create mode 100644 common/test/data/test_import_course_2/video/separate_file_video.xml diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_import.py b/cms/djangoapps/contentstore/management/commands/tests/test_import.py index 055d132f12..5e401f2c5a 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_import.py +++ b/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(''.format(course_id)) + + with open(os.path.join(directory, "course", "{0[2]}.xml".format(course_id)), "w+") as f: + f.write('') + + 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(''.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('') + 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(''.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('') + # 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))) diff --git a/cms/djangoapps/contentstore/tests/test_import.py b/cms/djangoapps/contentstore/tests/test_import.py index 7a82020b8d..dd28b87703 100644 --- a/cms/djangoapps/contentstore/tests/test_import.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 2902d3f6f8..2beeab75a5 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/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 diff --git a/common/test/data/test_import_course_2/about/end_date.html b/common/test/data/test_import_course_2/about/end_date.html new file mode 100644 index 0000000000..a0990367ef --- /dev/null +++ b/common/test/data/test_import_course_2/about/end_date.html @@ -0,0 +1 @@ +TBD diff --git a/common/test/data/test_import_course_2/chapter/vertical_container.xml b/common/test/data/test_import_course_2/chapter/vertical_container.xml new file mode 100644 index 0000000000..886346704c --- /dev/null +++ b/common/test/data/test_import_course_2/chapter/vertical_container.xml @@ -0,0 +1,3 @@ + + + \ No newline at end of file diff --git a/common/test/data/test_import_course_2/course.xml b/common/test/data/test_import_course_2/course.xml new file mode 100644 index 0000000000..de9962da73 --- /dev/null +++ b/common/test/data/test_import_course_2/course.xml @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/common/test/data/test_import_course_2/course/2014_Fall.xml b/common/test/data/test_import_course_2/course/2014_Fall.xml new file mode 100644 index 0000000000..9b14d49dcd --- /dev/null +++ b/common/test/data/test_import_course_2/course/2014_Fall.xml @@ -0,0 +1,20 @@ + + + + + + + + + + + + + + + + diff --git a/common/test/data/test_import_course_2/info/handouts.html b/common/test/data/test_import_course_2/info/handouts.html new file mode 100644 index 0000000000..85fa34d71d --- /dev/null +++ b/common/test/data/test_import_course_2/info/handouts.html @@ -0,0 +1 @@ +Sample \ No newline at end of file diff --git a/common/test/data/test_import_course_2/policies/2012_Fall.json b/common/test/data/test_import_course_2/policies/2012_Fall.json new file mode 100644 index 0000000000..464184fac8 --- /dev/null +++ b/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" + } +} diff --git a/common/test/data/test_import_course_2/sequential/vertical_sequential.xml b/common/test/data/test_import_course_2/sequential/vertical_sequential.xml new file mode 100644 index 0000000000..695e640243 --- /dev/null +++ b/common/test/data/test_import_course_2/sequential/vertical_sequential.xml @@ -0,0 +1,4 @@ + + + … + \ No newline at end of file diff --git a/common/test/data/test_import_course_2/vertical/vertical_test.xml b/common/test/data/test_import_course_2/vertical/vertical_test.xml new file mode 100644 index 0000000000..3dd75a08c8 --- /dev/null +++ b/common/test/data/test_import_course_2/vertical/vertical_test.xml @@ -0,0 +1,9 @@ + + diff --git a/common/test/data/test_import_course_2/video/separate_file_video.xml b/common/test/data/test_import_course_2/video/separate_file_video.xml new file mode 100644 index 0000000000..b90ea9d8c4 --- /dev/null +++ b/common/test/data/test_import_course_2/video/separate_file_video.xml @@ -0,0 +1 @@ +