Code review changes re split import/export, create_xblock

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()

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):

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,
         )

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.

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)

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
         """

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:

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

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
 

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,

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)