From 192d701899c9751acf159745e5e670bc1467f15f Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Fri, 8 Aug 2014 10:40:27 -0400 Subject: [PATCH] Code review changes re split import/export, create_xblock --- .../contentstore/tests/test_contentstore.py | 20 ++++++------- .../contentstore/tests/test_crud.py | 4 +-- .../xmodule/xmodule/modulestore/__init__.py | 3 +- .../modulestore/draft_and_published.py | 2 +- .../lib/xmodule/xmodule/modulestore/mixed.py | 13 +++++++- .../xmodule/xmodule/modulestore/mongo/base.py | 12 ++++++-- .../xmodule/modulestore/split_mongo/split.py | 19 ++++++++---- .../modulestore/split_mongo/split_draft.py | 30 ++++++++++++------- .../test_cross_modulestore_import_export.py | 1 - .../xmodule/modulestore/xml_importer.py | 9 +++--- common/lib/xmodule/xmodule/tests/__init__.py | 16 ++++++---- 11 files changed, 84 insertions(+), 45 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index faf0404dbe..09e78efc40 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1400,30 +1400,30 @@ class ContentStoreTest(ContentStoreTestCase): # crate a new module and add it as a child to a vertical parent = verticals[0] - new_module = self.store.create_child( + new_block = self.store.create_child( self.user.id, parent.location, 'html', 'new_component' ) # flush the cache - new_module = self.store.get_item(new_module.location) + new_block = self.store.get_item(new_block.location) # check for grace period definition which should be defined at the course level - self.assertEqual(parent.graceperiod, new_module.graceperiod) - self.assertEqual(parent.start, new_module.start) - self.assertEqual(course.start, new_module.start) + self.assertEqual(parent.graceperiod, new_block.graceperiod) + self.assertEqual(parent.start, new_block.start) + self.assertEqual(course.start, new_block.start) - self.assertEqual(course.xqa_key, new_module.xqa_key) + self.assertEqual(course.xqa_key, new_block.xqa_key) # # now let's define an override at the leaf node level # - new_module.graceperiod = timedelta(1) - self.store.update_item(new_module, self.user.id) + new_block.graceperiod = timedelta(1) + self.store.update_item(new_block, self.user.id) # flush the cache and refetch - new_module = self.store.get_item(new_module.location) + new_block = self.store.get_item(new_block.location) - self.assertEqual(timedelta(1), new_module.graceperiod) + self.assertEqual(timedelta(1), new_block.graceperiod) def test_default_metadata_inheritance(self): course = CourseFactory.create() diff --git a/cms/djangoapps/contentstore/tests/test_crud.py b/cms/djangoapps/contentstore/tests/test_crud.py index 0665254211..08d67db3d3 100644 --- a/cms/djangoapps/contentstore/tests/test_crud.py +++ b/cms/djangoapps/contentstore/tests/test_crud.py @@ -163,12 +163,12 @@ class TemplateTests(unittest.TestCase): guid_locator = test_course.location.course_agnostic() # verify it can be retrieved by id self.assertIsInstance(self.split_store.get_course(id_locator), CourseDescriptor) - # and by guid -- reenable when split_draft supports getting specific versions + # and by guid -- TODO reenable when split_draft supports getting specific versions # self.assertIsInstance(self.split_store.get_item(guid_locator), CourseDescriptor) self.split_store.delete_course(id_locator, 'testbot') # test can no longer retrieve by id self.assertRaises(ItemNotFoundError, self.split_store.get_course, id_locator) - # but can by guid + # but can by guid -- same TODO as above # self.assertIsInstance(self.split_store.get_item(guid_locator), CourseDescriptor) def test_block_generations(self): diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index b22b78dac9..25de6e22dc 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -603,7 +603,8 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite): about_location.course_key, about_location.block_type, block_id=about_location.block_id, - definition_data=overview_template.get('data'), + definition_data={'data': overview_template.get('data')}, + metadata=overview_template.get('metadata'), runtime=runtime, continue_version=True, ) diff --git a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py index 3318135289..7db8ec0559 100644 --- a/common/lib/xmodule/xmodule/modulestore/draft_and_published.py +++ b/common/lib/xmodule/xmodule/modulestore/draft_and_published.py @@ -104,7 +104,7 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin): raise NotImplementedError @abstractmethod - def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None): + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None, **kwargs): """ Import the given xblock into the current branch setting: import completely overwrites any existing block of the same id. diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 961f5c634b..91b4f1076d 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -453,8 +453,10 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): return modulestore.create_child(user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, **kwargs) @strip_key - def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None): + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None, **kwargs): """ + See :py:meth `ModuleStoreDraftAndPublished.import_xblock` + Defer to the course's modulestore if it supports this method """ store = self._verify_modulestore_support(course_key, 'import_xblock') @@ -511,6 +513,15 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): def create_xblock(self, runtime, course_key, block_type, block_id=None, fields=None, **kwargs): """ Create the new xmodule but don't save it. Returns the new module. + + Args: + runtime: :py:class `xblock.runtime` from another xblock in the same course. Providing this + significantly speeds up processing (inheritance and subsequent persistence) + course_key: :py:class `opaque_keys.CourseKey` + block_type: :py:class `string`: the string identifying the xblock type + block_id: the string uniquely identifying the block within the given course + fields: :py:class `dict` field_name, value pairs for initializing the xblock fields. Values + should be the pythonic types not the json serialized ones. """ store = self._verify_modulestore_support(course_key, 'create_xblock') return store.create_xblock(runtime, course_key, block_type, block_id, fields or {}, **kwargs) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index e68b499e8b..e2592168d2 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -974,7 +974,10 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): # @Cale, should this use LocalId like we do in split? if block_id is None: - block_id = uuid4().hex # really need to make this more readable: e.g., u'{}{}'.format(block_type, uuid4().hex[:4] + if block_type == 'course': + block_id = course_key.run + else: + block_id = u'{}_{}'.format(block_type, uuid4().hex[:5]) if runtime is None: services = {} @@ -1027,7 +1030,10 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): a new identifier will be generated """ if block_id is None: - block_id = uuid4().hex + if block_type == 'course': + block_id = course_key.run + else: + block_id = u'{}_{}'.format(block_type, uuid4().hex[:5]) runtime = kwargs.pop('runtime', None) xblock = self.create_xblock(runtime, course_key, block_type, block_id, **kwargs) @@ -1058,7 +1064,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): return xblock - def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None): + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None, **kwargs): """ Simple implementation of overwriting any existing xblock """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 7e7840b391..abe9a693f0 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -534,7 +534,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): return [ BlockUsageLocator( course_key=course_key, block_type=blocks[block_id]['category'], block_id=block_id - ).version_agnostic() + ) for block_id in items ] @@ -818,7 +818,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # split handles all the fields in one dict not separated by scope fields = fields or {} fields.update(kwargs.pop('metadata', {}) or {}) - fields.update(kwargs.pop('definition_data', {}) or {}) + definition_data = kwargs.pop('definition_data', {}) + if definition_data: + if not isinstance(definition_data, dict): + definition_data = {'data': definition_data} # backward compatibility to mongo's hack + fields.update(definition_data) # find course_index entry if applicable and structures entry index_entry = self._get_index_if_valid(course_key, force, continue_version) @@ -962,10 +966,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): **kwargs ) + DEFAULT_ROOT_BLOCK_ID = 'course' def create_course( self, org, course, run, user_id, master_branch=None, fields=None, versions_dict=None, search_targets=None, root_category='course', - root_block_id='course', **kwargs + root_block_id=None, **kwargs ): """ Create a new entry in the active courses index which points to an existing or new structure. Returns @@ -1043,7 +1048,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): self.db_connection.insert_definition(definition_entry) draft_structure = self._new_structure( - user_id, root_block_id, root_category, block_fields, definition_id + user_id, + root_block_id or SplitMongoModuleStore.DEFAULT_ROOT_BLOCK_ID, + root_category, + block_fields, + definition_id ) new_id = draft_structure['_id'] @@ -1168,7 +1177,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): is_updated = self._compare_settings(settings, original_entry['fields']) # check children - if partitioned_fields[Scope.children]: + if partitioned_fields.get(Scope.children, {}): # purposely not 'is not None' serialized_children = [child.block_id for child in partitioned_fields[Scope.children]['children']] is_updated = is_updated or original_entry['fields'].get('children', []) != serialized_children if is_updated: diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index c21e52822a..a57da5c16c 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -48,9 +48,9 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS return item - def get_course(self, course_id, depth=0): + def get_course(self, course_id, depth=0, **kwargs): course_id = self._map_revision_to_branch(course_id) - return super(DraftVersioningModuleStore, self).get_course(course_id, depth=depth) + return super(DraftVersioningModuleStore, self).get_course(course_id, depth=depth, **kwargs) def get_courses(self, **kwargs): """ @@ -92,8 +92,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS force=False, continue_version=False, skip_auto_publish=False, **kwargs ): """ - Adds skip_auto_publish to behavior or parent. Skip_auto_publish basically just calls the super - and skips all of this wrapper's functionality. + See :py:meth `ModuleStoreDraftAndPublished.create_item` """ course_key = self._map_revision_to_branch(course_key) item = super(DraftVersioningModuleStore, self).create_item( @@ -210,9 +209,9 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS location = self._map_revision_to_branch(location, revision=revision) return SplitMongoModuleStore.get_parent_location(self, location, **kwargs) - def get_orphans(self, course_key): + def get_orphans(self, course_key, **kwargs): course_key = self._map_revision_to_branch(course_key) - return super(DraftVersioningModuleStore, self).get_orphans(course_key) + return super(DraftVersioningModuleStore, self).get_orphans(course_key, **kwargs) def has_changes(self, xblock): """ @@ -271,16 +270,25 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS raise NotImplementedError() def get_course_history_info(self, course_locator): + """ + See :py:meth `xmodule.modulestore.split_mongo.split.SplitMongoModuleStore.get_course_history_info` + """ course_locator = self._map_revision_to_branch(course_locator) return super(DraftVersioningModuleStore, self).get_course_history_info(course_locator) def get_course_successors(self, course_locator, version_history_depth=1): + """ + See :py:meth `xmodule.modulestore.split_mongo.split.SplitMongoModuleStore.get_course_successors` + """ course_locator = self._map_revision_to_branch(course_locator) return super(DraftVersioningModuleStore, self).get_course_successors( course_locator, version_history_depth=version_history_depth ) def get_block_generations(self, block_locator): + """ + See :py:meth `xmodule.modulestore.split_mongo.split.SplitMongoModuleStore.get_block_generations` + """ block_locator = self._map_revision_to_branch(block_locator) return super(DraftVersioningModuleStore, self).get_block_generations(block_locator) @@ -325,25 +333,25 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS """ return block['edit_info'].get('source_version', block['edit_info']['update_version']) - def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None): + def import_xblock(self, user_id, course_key, block_type, block_id, fields=None, runtime=None, **kwargs): """ Split-based modulestores need to import published blocks to both branches """ # hardcode course root block id if block_type == 'course': - block_id = 'course' + block_id = self.DEFAULT_ROOT_BLOCK_ID new_usage_key = course_key.make_usage_key(block_type, block_id) - if self.get_branch_setting(course_key) == ModuleStoreEnum.Branch.published_only: + if self.get_branch_setting() == ModuleStoreEnum.Branch.published_only: # if importing a direct only, override existing draft if block_type in DIRECT_ONLY_CATEGORIES: draft_course = course_key.for_branch(ModuleStoreEnum.BranchName.draft) with self.branch_setting(ModuleStoreEnum.Branch.draft_preferred, draft_course): draft = self.import_xblock(user_id, draft_course, block_type, block_id, fields, runtime) self._auto_publish_no_children(draft.location, block_type, user_id) - return self.get_item(new_usage_key) + return self.get_item(new_usage_key.for_branch(ModuleStoreEnum.BranchName.published)) # if new to published - elif not self.has_item(new_usage_key): + elif not self.has_item(new_usage_key.for_branch(ModuleStoreEnum.BranchName.published)): # check whether it's new to draft if not self.has_item(new_usage_key.for_branch(ModuleStoreEnum.BranchName.draft)): # add to draft too diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py index 2efaa154b3..3c407eceef 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_cross_modulestore_import_export.py @@ -18,7 +18,6 @@ import random from contextlib import contextmanager, nested from shutil import rmtree from tempfile import mkdtemp -from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.tests import CourseComparisonTest diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 3de561569a..aa2daa9d72 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -166,7 +166,7 @@ def import_from_xml( served directly by nginx, instead of going through django. create_new_course_if_not_present: If True, then a new course is created if it doesn't already exist. - Otherwise, it throws an InvalidLocationError for the course. + Otherwise, it throws an InvalidLocationError if the course does not exist. default_class, load_error_modules: are arguments for constructing the XMLModuleStore (see its doc) """ @@ -236,14 +236,15 @@ def import_from_xml( try: all_locs.remove(child.location) except KeyError: - # ContentStoreTest.test_image_import has non-tree children - # so, make this more robust + # tolerate same child occurring under 2 parents such as in + # ContentStoreTest.test_image_import pass if verbose: log.debug('importing module location {loc}'.format(loc=child.location)) _import_module_and_update_references( - child, store, + child, + store, user_id, course_key, dest_course_id, diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index dbe8decf0f..dfd1a95b7a 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -219,19 +219,22 @@ class CourseComparisonTest(unittest.TestCase): self.assertEqual(len(expected_items), len(actual_items)) actual_item_map = { - actual_course_key.make_usage_key(item.category, item.location.block_id): item + item.location.block_id: item for item in actual_items } for expected_item in expected_items: actual_item_location = actual_course_key.make_usage_key(expected_item.category, expected_item.location.block_id) + # split and old mongo use different names for the course root but we don't know which + # modulestore actual's come from here; so, assume old mongo and if that fails, assume split if expected_item.location.category == 'course': actual_item_location = actual_item_location.replace(name=actual_item_location.run) - actual_item = actual_item_map.get(actual_item_location) + actual_item = actual_item_map.get(actual_item_location.block_id) + # must be split if actual_item is None and expected_item.location.category == 'course': actual_item_location = actual_item_location.replace(name='course') - actual_item = actual_item_map.get(actual_item_location) - assert actual_item is not None + actual_item = actual_item_map.get(actual_item_location.block_id) + self.assertIsNotNone(actual_item, u'cannot find {} in {}'.format(actual_item_location, actual_item_map)) # compare fields self.assertEqual(expected_item.fields, actual_item.fields) @@ -264,14 +267,15 @@ class CourseComparisonTest(unittest.TestCase): # compare children self.assertEqual(expected_item.has_children, actual_item.has_children) if expected_item.has_children: + actual_course_key = actual_item.location.course_key.version_agnostic() expected_children = [ - course1_item_child.location.map_into_course(actual_item.location.course_key) + course1_item_child.location.map_into_course(actual_course_key) for course1_item_child in expected_item.get_children() # get_children was returning drafts for published parents :-( if expect_drafts or not getattr(course1_item_child, 'is_draft', False) ] actual_children = [ - item_child.location + item_child.location.version_agnostic() for item_child in actual_item.get_children() # get_children was returning drafts for published parents :-( if expect_drafts or not getattr(item_child, 'is_draft', False)