diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py index 215bb9d9aa..306ccabd2b 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py +++ b/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( diff --git a/cms/djangoapps/contentstore/tests/test_crud.py b/cms/djangoapps/contentstore/tests/test_crud.py index 078cc7eefb..0e420d3ee8 100644 --- a/cms/djangoapps/contentstore/tests/test_crud.py +++ b/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') diff --git a/common/lib/xmodule/xmodule/modulestore/django.py b/common/lib/xmodule/xmodule/modulestore/django.py index 5e17a59183..abefeae84f 100644 --- a/common/lib/xmodule/xmodule/modulestore/django.py +++ b/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: diff --git a/common/lib/xmodule/xmodule/modulestore/exceptions.py b/common/lib/xmodule/xmodule/modulestore/exceptions.py index 1498429c2e..47d810f06d 100644 --- a/common/lib/xmodule/xmodule/modulestore/exceptions.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index a3757b42d4..ccde9b3c9c 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/modulestore/split_migrator.py b/common/lib/xmodule/xmodule/modulestore/split_migrator.py index e377e7355c..516e37607a 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_migrator.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index fcebb0c0f3..360028cead 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/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): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 96603ee607..07c23f8f42 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py b/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py index 77759d2431..34e3a89b22 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py b/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py index bd199cc02b..525bbab496 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index d2121bffa1..d52886b893 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/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):