From 4844a50405b909d861f67b32e62da9b00b9f7155 Mon Sep 17 00:00:00 2001 From: Carson Gee Date: Mon, 31 Mar 2014 16:41:23 -0400 Subject: [PATCH 1/2] Add test in import code to make sure two overlapping courses aren't imported --- .../management/commands/import.py | 2 +- .../management/commands/tests/test_import.py | 26 +++++++++++++++++++ .../xmodule/modulestore/xml_importer.py | 25 ++++++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/management/commands/import.py b/cms/djangoapps/contentstore/management/commands/import.py index bb410105802f..724886621d88 100644 --- a/cms/djangoapps/contentstore/management/commands/import.py +++ b/cms/djangoapps/contentstore/management/commands/import.py @@ -53,5 +53,5 @@ def handle(self, *args, **options): for module in course_items: course_id = module.location.course_id if not are_permissions_roles_seeded(course_id): - self.stdout.write('Seeding forum roles for course {0}'.format(course_id)) + self.stdout.write('Seeding forum roles for course {0}\n'.format(course_id)) seed_permissions_roles(course_id) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_import.py b/cms/djangoapps/contentstore/management/commands/tests/test_import.py index 3a3cac70d11e..d34c449f1c7d 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_import.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_import.py @@ -12,6 +12,7 @@ from contentstore.tests.modulestore_config import TEST_MODULESTORE from django_comment_common.utils import are_permissions_roles_seeded +from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -22,6 +23,7 @@ class TestImport(ModuleStoreTestCase): """ COURSE_ID = ['EDx', '0.00x', '2013_Spring', ] + DIFF_TERM = ['EDx', '0.00x', '2014_Spring', ] def setUp(self): """ @@ -41,6 +43,16 @@ def setUp(self): with open(os.path.join(self.good_dir, "course", "{0[2]}.xml".format(self.COURSE_ID)), "w+") as f: f.write('') + # Create term 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_TERM)) + + with open(os.path.join(self.dupe_dir, "course", "{0[2]}.xml".format(self.DIFF_TERM)), "w+") as f: + f.write('') + def test_forum_seed(self): """ Tests that forum roles were created with import. @@ -48,3 +60,17 @@ def test_forum_seed(self): self.assertFalse(are_permissions_roles_seeded('/'.join(self.COURSE_ID))) call_command('import', self.content_dir, self.good_dir) self.assertTrue(are_permissions_roles_seeded('/'.join(self.COURSE_ID))) + + def test_duplicate_with_url(self): + """ + Check to make sure an import doesn't import courses that will + create find one duplicates + """ + # 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))) + + # 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_TERM))) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index b9dd91d09e79..74ad299523d6 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -176,6 +176,31 @@ 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) + + # Check to see if a course with the same + # pseudo_course_id, but different term exists in + # the passed store to avoid broken courses + courses = store.get_courses() + bad_term = 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 terms 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_term = True + if bad_term: + # Skip this course, but keep trying to import courses + continue log.debug('======> IMPORTING course to location {loc}'.format( loc=course_location From 13af6fc1ee917251b2dc85d12fdacad88912cf5e Mon Sep 17 00:00:00 2001 From: Carson Gee Date: Wed, 9 Apr 2014 14:10:52 -0400 Subject: [PATCH 2/2] Refactored terminology and added loop break for optimization --- .../management/commands/tests/test_import.py | 15 ++++++++------- .../xmodule/xmodule/modulestore/xml_importer.py | 11 ++++++----- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_import.py b/cms/djangoapps/contentstore/management/commands/tests/test_import.py index d34c449f1c7d..055d132f1201 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_import.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_import.py @@ -23,7 +23,7 @@ class TestImport(ModuleStoreTestCase): """ COURSE_ID = ['EDx', '0.00x', '2013_Spring', ] - DIFF_TERM = ['EDx', '0.00x', '2014_Spring', ] + DIFF_RUN = ['EDx', '0.00x', '2014_Spring', ] def setUp(self): """ @@ -43,14 +43,14 @@ def setUp(self): with open(os.path.join(self.good_dir, "course", "{0[2]}.xml".format(self.COURSE_ID)), "w+") as f: f.write('') - # Create term changed course xml + # 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_TERM)) + 'course="{0[1]}"/>'.format(self.DIFF_RUN)) - with open(os.path.join(self.dupe_dir, "course", "{0[2]}.xml".format(self.DIFF_TERM)), "w+") as f: + with open(os.path.join(self.dupe_dir, "course", "{0[2]}.xml".format(self.DIFF_RUN)), "w+") as f: f.write('') def test_forum_seed(self): @@ -63,8 +63,9 @@ def test_forum_seed(self): def test_duplicate_with_url(self): """ - Check to make sure an import doesn't import courses that will - create find one duplicates + 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) @@ -73,4 +74,4 @@ def test_duplicate_with_url(self): # 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_TERM))) + self.assertIsNone(store.get_course('/'.join(self.DIFF_RUN))) diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 74ad299523d6..cd4a694d2ed9 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -179,15 +179,15 @@ def import_from_xml( course_prefix = u'{0.org}/{0.course}'.format(course_location) # Check to see if a course with the same - # pseudo_course_id, but different term exists in + # pseudo_course_id, but different run exists in # the passed store to avoid broken courses courses = store.get_courses() - bad_term = False + 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 terms match or fail + # that runs match or fail if course.location.name != module.location.name: log.error( 'A course with ID %s exists, and this ' @@ -197,8 +197,9 @@ def import_from_xml( course.location.course_id, module.location.course_id ) - bad_term = True - if bad_term: + bad_run = True + break + if bad_run: # Skip this course, but keep trying to import courses continue