Merge pull request #2849 from edx/dhm/split_course_id

Make course_id a required arg to split.create_course

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

@@ -10,7 +10,7 @@ from contentstore.management.commands.migrate_to_split import Command
 from contentstore.tests.modulestore_config import TEST_MODULESTORE
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
-from xmodule.modulestore.django import modulestore, loc_mapper
+from xmodule.modulestore.django import modulestore, loc_mapper, clear_existing_modulestores
 from xmodule.modulestore.locator import CourseLocator
 # pylint: disable=E1101
 
@@ -56,6 +56,8 @@ class TestMigrateToSplit(ModuleStoreTestCase):
         password = 'foo'
         self.user = User.objects.create_user(uname, email, password)
         self.course = CourseFactory()
+        self.addCleanup(ModuleStoreTestCase.drop_mongo_collections, 'split')
+        self.addCleanup(clear_existing_modulestores)
 
     def test_user_email(self):
         call_command(

cms/djangoapps/contentstore/tests/test_crud.py

@@ -8,10 +8,11 @@ from xmodule.modulestore.django import modulestore, loc_mapper, clear_existing_m
 from xmodule.seq_module import SequenceDescriptor
 from xmodule.capa_module import CapaDescriptor
 from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator, LocalId
-from xmodule.modulestore.exceptions import ItemNotFoundError
+from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError
 from xmodule.html_module import HtmlDescriptor
 from xmodule.modulestore import inheritance
 from xblock.core import XBlock
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 
 class TemplateTests(unittest.TestCase):
@@ -20,7 +21,9 @@ class TemplateTests(unittest.TestCase):
     """
 
     def setUp(self):
-        clear_existing_modulestores()
+        clear_existing_modulestores()  # redundant w/ cleanup but someone was getting errors
+        self.addCleanup(ModuleStoreTestCase.drop_mongo_collections, 'split')
+        self.addCleanup(clear_existing_modulestores)
 
     def test_get_templates(self):
         found = templates.all_templates()
@@ -53,8 +56,10 @@ class TemplateTests(unittest.TestCase):
         self.assertIsNotNone(HtmlDescriptor.get_template('announcement.yaml'))
 
     def test_factories(self):
-        test_course = persistent_factories.PersistentCourseFactory.create(org='testx', prettyid='tempcourse',
-            display_name='fun test course', user_id='testbot')
+        test_course = persistent_factories.PersistentCourseFactory.create(
+            course_id='testx.tempcourse', org='testx', prettyid='tempcourse',
+            display_name='fun test course', user_id='testbot'
+        )
         self.assertIsInstance(test_course, CourseDescriptor)
         self.assertEqual(test_course.display_name, 'fun test course')
         index_info = modulestore('split').get_course_index_info(test_course.location)
@@ -68,12 +73,20 @@ class TemplateTests(unittest.TestCase):
         test_course = modulestore('split').get_course(test_chapter.location)
         self.assertIn(test_chapter.location.block_id, test_course.children)
 
+        with self.assertRaises(DuplicateCourseError):
+            persistent_factories.PersistentCourseFactory.create(
+                course_id='testx.tempcourse', org='testx', prettyid='tempcourse',
+                display_name='fun test course', user_id='testbot'
+            )
+
     def test_temporary_xblocks(self):
         """
         Test using load_from_json to create non persisted xblocks
         """
-        test_course = persistent_factories.PersistentCourseFactory.create(org='testx', prettyid='tempcourse',
-            display_name='fun test course', user_id='testbot')
+        test_course = persistent_factories.PersistentCourseFactory.create(
+            course_id='testx.tempcourse', org='testx', prettyid='tempcourse',
+            display_name='fun test course', user_id='testbot'
+        )
 
         test_chapter = self.load_from_json({'category': 'chapter',
             'fields': {'display_name': 'chapter n'}},
@@ -98,7 +111,7 @@ class TemplateTests(unittest.TestCase):
         try saving temporary xblocks
         """
         test_course = persistent_factories.PersistentCourseFactory.create(
-            org='testx', prettyid='tempcourse',
+            course_id='testx.tempcourse', org='testx', prettyid='tempcourse',
             display_name='fun test course', user_id='testbot')
         test_chapter = self.load_from_json({'category': 'chapter',
             'fields': {'display_name': 'chapter n'}},
@@ -135,7 +148,7 @@ class TemplateTests(unittest.TestCase):
 
     def test_delete_course(self):
         test_course = persistent_factories.PersistentCourseFactory.create(
-            org='testx',
+            course_id='edu.harvard.history.doomed', org='testx',
             prettyid='edu.harvard.history.doomed',
             display_name='doomed test course',
             user_id='testbot')
@@ -159,7 +172,7 @@ class TemplateTests(unittest.TestCase):
         Test get_block_generations
         """
         test_course = persistent_factories.PersistentCourseFactory.create(
-            org='testx',
+            course_id='edu.harvard.history.hist101', org='testx',
             prettyid='edu.harvard.history.hist101',
             display_name='history test course',
             user_id='testbot')

common/lib/xmodule/xmodule/modulestore/django.py

@@ -184,7 +184,7 @@ def editable_modulestore(name='default'):
 
     # At this point, we either have the ability to create
     # items in the store, or we do not.
-    if hasattr(store, 'create_xmodule'):
+    if hasattr(store, 'create_course'):
         return store
 
     else:

common/lib/xmodule/xmodule/modulestore/exceptions.py

@@ -46,3 +46,16 @@ class VersionConflictError(Exception):
         super(VersionConflictError, self).__init__()
         self.requestedLocation = requestedLocation
         self.currentHeadVersionGuid = currentHeadVersionGuid
+
+
+class DuplicateCourseError(Exception):
+    """
+    An attempt to create a course whose id duplicates an existing course's
+    """
+    def __init__(self, course_id, existing_entry):
+        """
+        existing_entry will have the who, when, and other properties of the existing entry
+        """
+        super(DuplicateCourseError, self).__init__()
+        self.course_id = course_id
+        self.existing_entry = existing_entry

common/lib/xmodule/xmodule/modulestore/mixed.py

@@ -282,7 +282,6 @@ class MixedModuleStore(ModuleStoreWriteBase):
         :param fields: a dict of xblock field name - value pairs for the course module.
         :param metadata: the old way of setting fields by knowing which ones are scope.settings v scope.content
         :param definition_data: the complement to metadata which is also a subset of fields
-        :param id_root: the split-mongo course_id starting value (see split.create_course)
         :param pretty_id: a field split.create_course uses and may quit using
         :returns: course xblock
         """
@@ -290,22 +289,19 @@ class MixedModuleStore(ModuleStoreWriteBase):
         if not hasattr(store, 'create_course'):
             raise NotImplementedError(u"Cannot create a course on store %s" % store_name)
         if store.get_modulestore_type(course_id) == SPLIT_MONGO_MODULESTORE_TYPE:
-            id_root = kwargs.get('id_root')
             try:
                 course_dict = Location.parse_course_id(course_id)
                 org = course_dict['org']
-                if id_root is None:
-                    id_root = "{org}.{course}.{name}".format(**course_dict)
+                course_id = "{org}.{course}.{name}".format(**course_dict)
             except ValueError:
                 org = None
-                if id_root is None:
-                    id_root = course_id
+
             org = kwargs.pop('org', org)
-            pretty_id = kwargs.pop('pretty_id', id_root)
+            pretty_id = kwargs.pop('pretty_id', course_id)
             fields = kwargs.pop('fields', {})
             fields.update(kwargs.pop('metadata', {}))
             fields.update(kwargs.pop('definition_data', {}))
-            course = store.create_course(org, pretty_id, user_id, id_root=id_root, fields=fields, **kwargs)
+            course = store.create_course(course_id, org, pretty_id, user_id, fields=fields, **kwargs)
         else:  # assume mongo
             course = store.create_course(course_id, **kwargs)
 

common/lib/xmodule/xmodule/modulestore/split_migrator.py

@@ -47,8 +47,8 @@ class SplitMigrator(object):
         original_course = self.direct_modulestore.get_item(course_location)
         new_course_root_locator = self.loc_mapper.translate_location(old_course_id, course_location)
         new_course = self.split_modulestore.create_course(
-            course_location.org, original_course.display_name,
-            user.id, id_root=new_package_id,
+            new_package_id, course_location.org, original_course.display_name,
+            user.id,
             fields=self._get_json_fields_translate_children(original_course, old_course_id, True),
             root_block_id=new_course_root_locator.block_id,
             master_branch=new_course_root_locator.branch

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -59,7 +59,8 @@ from xmodule.modulestore.locator import (
     BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree,
     LocalId, Locator
 )
-from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError
+from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError, \
+    DuplicateCourseError
 from xmodule.modulestore import inheritance, ModuleStoreWriteBase, Location, SPLIT_MONGO_MODULESTORE_TYPE
 
 from ..exceptions import ItemNotFoundError
@@ -675,26 +676,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             serial += 1
         return category + str(serial)
 
-    def _generate_package_id(self, id_root):
-        """
-        Generate a somewhat readable course id unique w/in this db using the id_root
-        :param course_blocks: the current list of blocks.
-        :param category:
-        """
-        existing_uses = self.db_connection.find_matching_course_indexes({"_id": {"$regex": id_root}})
-        if existing_uses.count() > 0:
-            max_found = 0
-            matcher = re.compile(id_root + r'(\d+)')
-            for entry in existing_uses:
-                serial = re.search(matcher, entry['_id'])
-                if serial is not None and serial.groups > 0:
-                    value = int(serial.group(1))
-                    if value > max_found:
-                        max_found = value
-            return id_root + str(max_found + 1)
-        else:
-            return id_root
-
     # DHM: Should I rewrite this to take a new xblock instance rather than to construct it? That is, require the
     # caller to use XModuleDescriptor.load_from_json thus reducing similar code and making the object creation and
     # validation behavior a responsibility of the model layer rather than the persistence layer.
@@ -830,7 +811,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         return self.get_item(item_loc)
 
     def create_course(
-        self, org, prettyid, user_id, id_root=None, fields=None,
+        self, course_id, org, prettyid, user_id, fields=None,
         master_branch='draft', versions_dict=None, root_category='course',
         root_block_id='course'
     ):
@@ -838,8 +819,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         Create a new entry in the active courses index which points to an existing or new structure. Returns
         the course root of the resulting entry (the location has the course id)
 
-        id_root: allows the caller to specify the package_id. It's a root in that, if it's already taken,
-        this method will append things to the root to make it unique. (defaults to org)
+        course_id: If it's already taken, this method will raise DuplicateCourseError
 
         fields: if scope.settings fields provided, will set the fields of the root course object in the
         new course. If both
@@ -865,6 +845,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         provide any fields overrides, see above). if not provided, will create a mostly empty course
         structure with just a category course root xblock.
         """
+        # check course_id's uniqueness
+        index = self.db_connection.get_course_index(course_id)
+        if index is not None:
+            raise DuplicateCourseError(course_id, index)
+
         partitioned_fields = self.partition_fields_by_scope(root_category, fields)
         block_fields = partitioned_fields.setdefault(Scope.settings, {})
         if Scope.children in partitioned_fields:
@@ -929,20 +914,15 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                 self.db_connection.insert_structure(draft_structure)
                 versions_dict[master_branch] = new_id
 
-        # create the index entry
-        if id_root is None:
-            id_root = org
-        new_id = self._generate_package_id(id_root)
-
         index_entry = {
-            '_id': new_id,
+            '_id': course_id,
             'org': org,
             'prettyid': prettyid,
             'edited_by': user_id,
             'edited_on': datetime.datetime.now(UTC),
             'versions': versions_dict}
         self.db_connection.insert_course_index(index_entry)
-        return self.get_course(CourseLocator(package_id=new_id, branch=master_branch))
+        return self.get_course(CourseLocator(package_id=course_id, branch=master_branch))
 
     def update_item(self, descriptor, user_id, allow_not_found=False, force=False):
         """

common/lib/xmodule/xmodule/modulestore/tests/django_utils.py

@@ -209,20 +209,24 @@ class ModuleStoreTestCase(TestCase):
         return updated_course
 
     @staticmethod
-    def drop_mongo_collections():
+    def drop_mongo_collections(store_name='default'):
         """
         If using a Mongo-backed modulestore & contentstore, drop the collections.
         """
 
         # This will return the mongo-backed modulestore
         # even if we're using a mixed modulestore
-        store = editable_modulestore()
+        store = editable_modulestore(store_name)
         if hasattr(store, 'collection'):
             connection = store.collection.database.connection
             store.collection.drop()
             connection.close()
         elif hasattr(store, 'close_all_connections'):
             store.close_all_connections()
+        elif hasattr(store, 'db'):
+            connection = store.db.connection
+            connection.drop_database(store.db.name)
+            connection.close()
 
         if contentstore().fs_files:
             db = contentstore().fs_files.database

common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py

@@ -33,15 +33,16 @@ class PersistentCourseFactory(SplitFactory):
 
     # pylint: disable=W0613
     @classmethod
-    def _create(cls, target_class, org='testX', prettyid='999', user_id='test_user',
-                master_branch='draft', id_root=None, **kwargs):
+    def _create(cls, target_class, course_id='testX.999', org='testX', prettyid='999', user_id='test_user',
+                master_branch='draft', **kwargs):
 
         modulestore = kwargs.pop('modulestore')
         root_block_id = kwargs.pop('root_block_id', 'course')
         # Write the data to the mongo datastore
         new_course = modulestore.create_course(
-            org, prettyid, user_id, fields=kwargs, id_root=id_root or prettyid,
-            master_branch=master_branch, root_block_id=root_block_id)
+            course_id, org, prettyid, user_id, fields=kwargs,
+            master_branch=master_branch, root_block_id=root_block_id
+        )
 
         return new_course
 

common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py

@@ -114,7 +114,7 @@ class TestOrphan(unittest.TestCase):
         fields.update(data)
         # split requires the course to be created separately from creating items
         self.split_mongo.create_course(
-            'test_org', 'my course', self.userid, self.split_package_id, fields=fields, root_block_id='runid'
+            self.split_package_id, 'test_org', 'my course', self.userid, fields=fields, root_block_id='runid'
         )
         self.course_location = Location('i4x', 'test_org', 'test_course', 'course', 'runid')
         self.old_mongo.create_and_save_xmodule(self.course_location, data, metadata)

common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py

@@ -12,7 +12,7 @@ from importlib import import_module
 from xblock.fields import Scope
 from xmodule.course_module import CourseDescriptor
 from xmodule.modulestore.exceptions import InsufficientSpecificationError, ItemNotFoundError, VersionConflictError, \
-    DuplicateItemError
+    DuplicateItemError, DuplicateCourseError
 from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator, VersionTree, DefinitionLocator
 from xmodule.modulestore.inheritance import InheritanceMixin
 from xmodule.x_module import XModuleMixin
@@ -652,7 +652,7 @@ class TestItemCrud(SplitModuleTest):
         """
         # start transaction w/ simple creation
         user = random.getrandbits(32)
-        new_course = modulestore().create_course('test_org', 'test_transaction', user)
+        new_course = modulestore().create_course('test_org.test_transaction', 'test_org', 'test_transaction', user)
         new_course_locator = new_course.location.as_course_locator()
         index_history_info = modulestore().get_course_history_info(new_course.location)
         course_block_prev_version = new_course.previous_version
@@ -901,7 +901,7 @@ class TestItemCrud(SplitModuleTest):
         check_subtree(nodes[0])
 
     def create_course_for_deletion(self):
-        course = modulestore().create_course('nihilx', 'deletion', 'deleting_user')
+        course = modulestore().create_course('nihilx.deletion', 'nihilx', 'deletion', 'deleting_user')
         root = BlockUsageLocator(
             package_id=course.location.package_id,
             block_id=course.location.block_id,
@@ -929,7 +929,7 @@ class TestCourseCreation(SplitModuleTest):
         """
         # Oddly getting differences of 200nsec
         pre_time = datetime.datetime.now(UTC) - datetime.timedelta(milliseconds=1)
-        new_course = modulestore().create_course('test_org', 'test_course', 'create_user')
+        new_course = modulestore().create_course('test_org.test_course', 'test_org', 'test_course', 'create_user')
         new_locator = new_course.location
         # check index entry
         index_info = modulestore().get_course_index_info(new_locator)
@@ -963,14 +963,14 @@ class TestCourseCreation(SplitModuleTest):
         original_locator = CourseLocator(package_id="wonderful", branch='draft')
         original_index = modulestore().get_course_index_info(original_locator)
         new_draft = modulestore().create_course(
-            'leech', 'best_course', 'leech_master', id_root='best',
+            'best', 'leech', 'best_course', 'leech_master',
             versions_dict=original_index['versions'])
         new_draft_locator = new_draft.location
-        self.assertRegexpMatches(new_draft_locator.package_id, r'best.*')
+        self.assertRegexpMatches(new_draft_locator.package_id, 'best')
         # the edited_by and other meta fields on the new course will be the original author not this one
         self.assertEqual(new_draft.edited_by, 'test@edx.org')
         self.assertLess(new_draft.edited_on, pre_time)
-        self.assertEqual(new_draft.location.version_guid, original_index['versions']['draft'])
+        self.assertEqual(new_draft_locator.version_guid, original_index['versions']['draft'])
         # however the edited_by and other meta fields on course_index will be this one
         new_index = modulestore().get_course_index_info(new_draft_locator)
         self.assertGreaterEqual(new_index["edited_on"], pre_time)
@@ -1028,16 +1028,16 @@ class TestCourseCreation(SplitModuleTest):
         fields['grading_policy']['GRADE_CUTOFFS'] = {'A': .9, 'B': .8, 'C': .65}
         fields['display_name'] = 'Derivative'
         new_draft = modulestore().create_course(
-            'leech', 'derivative', 'leech_master', id_root='counter',
+            'counter', 'leech', 'derivative', 'leech_master',
             versions_dict={'draft': original_index['versions']['draft']},
             fields=fields
         )
         new_draft_locator = new_draft.location
-        self.assertRegexpMatches(new_draft_locator.package_id, r'counter.*')
+        self.assertRegexpMatches(new_draft_locator.package_id, 'counter')
         # the edited_by and other meta fields on the new course will be the original author not this one
         self.assertEqual(new_draft.edited_by, 'leech_master')
         self.assertGreaterEqual(new_draft.edited_on, pre_time)
-        self.assertNotEqual(new_draft.location.version_guid, original_index['versions']['draft'])
+        self.assertNotEqual(new_draft_locator.version_guid, original_index['versions']['draft'])
         # however the edited_by and other meta fields on course_index will be this one
         new_index = modulestore().get_course_index_info(new_draft_locator)
         self.assertGreaterEqual(new_index["edited_on"], pre_time)
@@ -1086,7 +1086,7 @@ class TestCourseCreation(SplitModuleTest):
         """
         user = random.getrandbits(32)
         new_course = modulestore().create_course(
-            'test_org', 'test_transaction', user,
+            'test_org.test_transaction', 'test_org', 'test_transaction', user,
             root_block_id='top', root_category='chapter'
         )
         self.assertEqual(new_course.location.block_id, 'top')
@@ -1100,6 +1100,14 @@ class TestCourseCreation(SplitModuleTest):
         self.assertIn('top', db_structure['blocks'])
         self.assertEqual(db_structure['blocks']['top']['category'], 'chapter')
 
+    def test_create_id_dupe(self):
+        """
+        Test create_course rejects duplicate id
+        """
+        user = random.getrandbits(32)
+        courses = modulestore().get_courses()
+        with self.assertRaises(DuplicateCourseError):
+            modulestore().create_course(courses[0].location.package_id, 'org', 'pretty', user)
 
 
 class TestInheritance(SplitModuleTest):