From 542b146f2fa1875cf22c232ce47d3ad2e85b1f8c Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 6 Mar 2014 11:56:35 -0500 Subject: [PATCH] Split: add schema version and drop pretty_id (was placeholder looking for purpose) STUD-1359 --- .../contentstore/tests/test_crud.py | 18 +++++++------- cms/djangoapps/contentstore/views/course.py | 2 +- .../xmodule/xmodule/modulestore/locator.py | 3 +-- .../lib/xmodule/xmodule/modulestore/mixed.py | 4 +--- .../xmodule/modulestore/split_migrator.py | 2 +- .../xmodule/modulestore/split_mongo/split.py | 24 ++++++++++++------- .../modulestore/tests/persistent_factories.py | 5 ++-- .../xmodule/modulestore/tests/test_orphan.py | 2 +- .../tests/test_split_modulestore.py | 21 +++++----------- .../data/splitmongo_json/active_versions.json | 3 --- 10 files changed, 38 insertions(+), 46 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_crud.py b/cms/djangoapps/contentstore/tests/test_crud.py index 0e420d3ee8..f843c71758 100644 --- a/cms/djangoapps/contentstore/tests/test_crud.py +++ b/cms/djangoapps/contentstore/tests/test_crud.py @@ -57,14 +57,14 @@ class TemplateTests(unittest.TestCase): def test_factories(self): test_course = persistent_factories.PersistentCourseFactory.create( - course_id='testx.tempcourse', org='testx', prettyid='tempcourse', + course_id='testx.tempcourse', org='testx', 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) self.assertEqual(index_info['org'], 'testx') - self.assertEqual(index_info['prettyid'], 'tempcourse') + self.assertEqual(index_info['_id'], 'testx.tempcourse') test_chapter = persistent_factories.ItemFactory.create(display_name='chapter 1', parent_location=test_course.location) @@ -75,7 +75,7 @@ class TemplateTests(unittest.TestCase): with self.assertRaises(DuplicateCourseError): persistent_factories.PersistentCourseFactory.create( - course_id='testx.tempcourse', org='testx', prettyid='tempcourse', + course_id='testx.tempcourse', org='testx', display_name='fun test course', user_id='testbot' ) @@ -84,7 +84,7 @@ class TemplateTests(unittest.TestCase): Test using load_from_json to create non persisted xblocks """ test_course = persistent_factories.PersistentCourseFactory.create( - course_id='testx.tempcourse', org='testx', prettyid='tempcourse', + course_id='testx.tempcourse', org='testx', display_name='fun test course', user_id='testbot' ) @@ -111,8 +111,9 @@ class TemplateTests(unittest.TestCase): try saving temporary xblocks """ test_course = persistent_factories.PersistentCourseFactory.create( - course_id='testx.tempcourse', org='testx', prettyid='tempcourse', - display_name='fun test course', user_id='testbot') + course_id='testx.tempcourse', org='testx', + display_name='fun test course', user_id='testbot' + ) test_chapter = self.load_from_json({'category': 'chapter', 'fields': {'display_name': 'chapter n'}}, test_course.system, parent_xblock=test_course) @@ -149,7 +150,6 @@ class TemplateTests(unittest.TestCase): def test_delete_course(self): test_course = persistent_factories.PersistentCourseFactory.create( course_id='edu.harvard.history.doomed', org='testx', - prettyid='edu.harvard.history.doomed', display_name='doomed test course', user_id='testbot') persistent_factories.ItemFactory.create(display_name='chapter 1', @@ -173,9 +173,9 @@ class TemplateTests(unittest.TestCase): """ test_course = persistent_factories.PersistentCourseFactory.create( course_id='edu.harvard.history.hist101', org='testx', - prettyid='edu.harvard.history.hist101', display_name='history test course', - user_id='testbot') + user_id='testbot' + ) chapter = persistent_factories.ItemFactory.create(display_name='chapter 1', parent_location=test_course.location, user_id='testbot') sub = persistent_factories.ItemFactory.create(display_name='subsection 1', diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index af7f1324de..6da30a1555 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -97,7 +97,7 @@ def course_handler(request, tag=None, package_id=None, branch=None, version_guid index entry. PUT json: update this course (index entry not xblock) such as repointing head, changing display name, org, - package_id, prettyid. Return same json as above. + package_id. Return same json as above. DELETE json: delete this branch from this course (leaving off /branch/draft would imply delete the course) """ diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index 4c384a49f1..5ed8f0689a 100644 --- a/common/lib/xmodule/xmodule/modulestore/locator.py +++ b/common/lib/xmodule/xmodule/modulestore/locator.py @@ -358,8 +358,7 @@ class CourseLocator(Locator): Generate a discussion group id based on course To make compatible with old Location object functionality. I don't believe this behavior fits at this - place, but I have no way to override. If this is really needed, it should probably use the pretty_id to seed - the name although that's mutable. We should also clearly define the purpose and restrictions of this + place, but I have no way to override. We should clearly define the purpose and restrictions of this (e.g., I'm assuming periods are fine). """ return self.package_id diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index ccde9b3c9c..1304a902b4 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 pretty_id: a field split.create_course uses and may quit using :returns: course xblock """ store = self.modulestores[store_name] @@ -297,11 +296,10 @@ class MixedModuleStore(ModuleStoreWriteBase): org = None org = kwargs.pop('org', org) - 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(course_id, org, pretty_id, user_id, fields=fields, **kwargs) + course = store.create_course(course_id, org, 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 516e37607a..80eba98029 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/split_migrator.py @@ -47,7 +47,7 @@ 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( - new_package_id, course_location.org, original_course.display_name, + new_package_id, course_location.org, user.id, fields=self._get_json_fields_translate_children(original_course, old_course_id, True), root_block_id=new_course_root_locator.block_id, diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 360028cead..8785b00015 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -5,7 +5,6 @@ Representation: * course_index: a dictionary: ** '_id': package_id (e.g., myu.mydept.mycourse.myrun), ** 'org': the org's id. Only used for searching not identity, - ** 'prettyid': a vague to-be-determined field probably more useful to storing searchable tags, ** 'edited_by': user_id of user who created the original entry, ** 'edited_on': the datetime of the original creation, ** 'versions': versions_dict: {branch_id: structure_id, ...} @@ -99,6 +98,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): A Mongodb backed ModuleStore supporting versions, inheritance, and sharing. """ + + SCHEMA_VERSION = 1 reference_type = Locator def __init__(self, doc_store_config, fs_root, render_template, default_class=None, @@ -468,7 +469,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): heads. This function is primarily for test verification but may serve some more general purpose. :param course_locator: must have a package_id set - :return {'org': , 'prettyid': , + :return {'org': string, versions: {'draft': the head draft version id, 'published': the head published version id if any, }, @@ -618,7 +619,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): "edited_on": datetime.datetime.now(UTC), "previous_version": None, "original_version": new_id, - } + }, + 'schema_version': self.SCHEMA_VERSION, } self.db_connection.insert_definition(document) definition_locator = DefinitionLocator(new_id) @@ -654,6 +656,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): old_definition['edit_info']['edited_on'] = datetime.datetime.now(UTC) # previous version id old_definition['edit_info']['previous_version'] = definition_locator.definition_id + old_definition['schema_version'] = self.SCHEMA_VERSION self.db_connection.insert_definition(old_definition) return DefinitionLocator(old_definition['_id']), True else: @@ -811,7 +814,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): return self.get_item(item_loc) def create_course( - self, course_id, org, prettyid, user_id, fields=None, + self, course_id, org, user_id, fields=None, master_branch='draft', versions_dict=None, root_category='course', root_block_id='course' ): @@ -870,7 +873,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 'edited_on': datetime.datetime.now(UTC), 'previous_version': None, 'original_version': definition_id, - } + }, + 'schema_version': self.SCHEMA_VERSION, } self.db_connection.insert_definition(definition_entry) @@ -904,6 +908,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): definition['edit_info']['edited_by'] = user_id definition['edit_info']['edited_on'] = datetime.datetime.now(UTC) definition['_id'] = ObjectId() + definition['schema_version'] = self.SCHEMA_VERSION self.db_connection.insert_definition(definition) root_block['definition'] = definition['_id'] root_block['edit_info']['edited_on'] = datetime.datetime.now(UTC) @@ -917,10 +922,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): index_entry = { '_id': course_id, 'org': org, - 'prettyid': prettyid, 'edited_by': user_id, 'edited_on': datetime.datetime.now(UTC), - 'versions': versions_dict} + 'versions': versions_dict, + 'schema_version': self.SCHEMA_VERSION, + } self.db_connection.insert_course_index(index_entry) return self.get_course(CourseLocator(package_id=course_id, branch=master_branch)) @@ -1448,6 +1454,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): new_structure['previous_version'] = structure['_id'] new_structure['edited_by'] = user_id new_structure['edited_on'] = datetime.datetime.now(UTC) + new_structure['schema_version'] = self.SCHEMA_VERSION return new_structure def _find_local_root(self, element_to_find, possibility, tree): @@ -1508,7 +1515,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 'original_version': new_id, 'edited_by': user_id, 'edited_on': datetime.datetime.now(UTC), - 'blocks': blocks + 'blocks': blocks, + 'schema_version': self.SCHEMA_VERSION, } def _get_parents_from_structure(self, block_id, structure): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py b/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py index 34e3a89b22..9f6675f033 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py @@ -24,7 +24,6 @@ class PersistentCourseFactory(SplitFactory): keywords: any xblock field plus (note, the below are filtered out; so, if they become legitimate xblock fields, they won't be settable via this factory) * org: defaults to textX - * prettyid: defaults to 999 * master_branch: (optional) defaults to 'draft' * user_id: (optional) defaults to 'test_user' * display_name (xblock field): will default to 'Robot Super Course' unless provided @@ -33,14 +32,14 @@ class PersistentCourseFactory(SplitFactory): # pylint: disable=W0613 @classmethod - def _create(cls, target_class, course_id='testX.999', org='testX', prettyid='999', user_id='test_user', + def _create(cls, target_class, course_id='testX.999', org='testX', 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( - course_id, org, prettyid, user_id, fields=kwargs, + course_id, org, user_id, fields=kwargs, master_branch=master_branch, root_block_id=root_block_id ) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py b/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py index 525bbab496..6202d9779d 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( - self.split_package_id, 'test_org', 'my course', self.userid, fields=fields, root_block_id='runid' + self.split_package_id, 'test_org', 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 d52886b893..82336faa36 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -176,12 +176,6 @@ class SplitModuleCourseTests(SplitModuleTest): qualifiers={'edited_on': {"$lt": datetime.datetime(2013, 3, 28, 15)}}) self.assertEqual(len(courses), 2) - courses = modulestore().get_courses( - branch='draft', - qualifiers={'org': 'testx', "prettyid": "test_course"}) - self.assertEqual(len(courses), 1) - self.assertIsNotNone(self.findByIdInResult(courses, "head12345")) - def test_get_course(self): ''' Test the various calling forms for get_course @@ -652,7 +646,7 @@ class TestItemCrud(SplitModuleTest): """ # start transaction w/ simple creation user = random.getrandbits(32) - new_course = modulestore().create_course('test_org.test_transaction', 'test_org', 'test_transaction', user) + new_course = modulestore().create_course('test_org.test_transaction', 'test_org', 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 +895,7 @@ class TestItemCrud(SplitModuleTest): check_subtree(nodes[0]) def create_course_for_deletion(self): - course = modulestore().create_course('nihilx.deletion', 'nihilx', 'deletion', 'deleting_user') + course = modulestore().create_course('nihilx.deletion', 'nihilx', 'deleting_user') root = BlockUsageLocator( package_id=course.location.package_id, block_id=course.location.block_id, @@ -929,12 +923,11 @@ 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', 'test_org', 'test_course', 'create_user') + new_course = modulestore().create_course('test_org.test_course', 'test_org', 'create_user') new_locator = new_course.location # check index entry index_info = modulestore().get_course_index_info(new_locator) self.assertEqual(index_info['org'], 'test_org') - self.assertEqual(index_info['prettyid'], 'test_course') self.assertGreaterEqual(index_info["edited_on"], pre_time) self.assertLessEqual(index_info["edited_on"], datetime.datetime.now(UTC)) self.assertEqual(index_info['edited_by'], 'create_user') @@ -963,7 +956,7 @@ 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( - 'best', 'leech', 'best_course', 'leech_master', + 'best', 'leech', 'leech_master', versions_dict=original_index['versions']) new_draft_locator = new_draft.location self.assertRegexpMatches(new_draft_locator.package_id, 'best') @@ -1028,7 +1021,7 @@ class TestCourseCreation(SplitModuleTest): fields['grading_policy']['GRADE_CUTOFFS'] = {'A': .9, 'B': .8, 'C': .65} fields['display_name'] = 'Derivative' new_draft = modulestore().create_course( - 'counter', 'leech', 'derivative', 'leech_master', + 'counter', 'leech', 'leech_master', versions_dict={'draft': original_index['versions']['draft']}, fields=fields ) @@ -1061,11 +1054,9 @@ class TestCourseCreation(SplitModuleTest): self.assertEqual(course_info['org'], 'funkyU') course_info['org'] = 'moreFunky' - course_info['prettyid'] = 'Ancient Greek Demagods' modulestore().update_course_index(course_info) course_info = modulestore().get_course_index_info(locator) self.assertEqual(course_info['org'], 'moreFunky') - self.assertEqual(course_info['prettyid'], 'Ancient Greek Demagods') # an allowed but not necessarily recommended way to revert the draft version versions = course_info['versions'] @@ -1086,7 +1077,7 @@ class TestCourseCreation(SplitModuleTest): """ user = random.getrandbits(32) new_course = modulestore().create_course( - 'test_org.test_transaction', 'test_org', 'test_transaction', user, + 'test_org.test_transaction', 'test_org', user, root_block_id='top', root_category='chapter' ) self.assertEqual(new_course.location.block_id, 'top') diff --git a/common/test/data/splitmongo_json/active_versions.json b/common/test/data/splitmongo_json/active_versions.json index b41440e0e7..e84f4659bd 100644 --- a/common/test/data/splitmongo_json/active_versions.json +++ b/common/test/data/splitmongo_json/active_versions.json @@ -1,6 +1,5 @@ [{"_id" : "GreekHero", "org" : "testx", - "prettyid" : "test_course", "versions" : { "draft" : { "$oid" : "1d00000000000000dddd0000" } }, @@ -9,7 +8,6 @@ {"_id" : "wonderful", "org" : "testx", - "prettyid" : "another_course", "versions" : { "draft" : { "$oid" : "1d00000000000000dddd2222" }, "published" : { "$oid" : "1d00000000000000eeee0000" } @@ -19,7 +17,6 @@ {"_id" : "contender", "org" : "guestx", - "prettyid" : "test_course", "versions" : { "draft" : { "$oid" : "1d00000000000000dddd5555" }}, "edited_on" : {"$date" : 1364491313238},