split create_item and create_child out as separate methods

cms/djangoapps/contentstore/course_info_model.py

@@ -36,7 +36,7 @@ def get_course_updates(location, provided_id, user_id):
     try:
         course_updates = modulestore().get_item(location)
     except ItemNotFoundError:
-        course_updates = modulestore().create_item(user_id, location)
+        course_updates = modulestore().create_item(user_id, location.course_key, location.block_type, location.block_id)
 
     course_update_items = get_course_update_items(course_updates, provided_id)
     return _get_visible_update(course_update_items)
@@ -51,7 +51,7 @@ def update_course_updates(location, update, passed_id=None, user=None):
     try:
         course_updates = modulestore().get_item(location)
     except ItemNotFoundError:
-        course_updates = modulestore().create_item(user.id, location)
+        course_updates = modulestore().create_item(user.id, location.course_key, location.block_type, location.block_id)
 
     course_update_items = list(reversed(get_course_update_items(course_updates)))
 

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -575,7 +575,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase):
 
         location = course.id.make_usage_key('chapter', 'neuvo')
         # Ensure draft mongo store does not create drafts for things that shouldn't be draft
-        newobject = draft_store.create_item(self.user.id, location)
+        newobject = draft_store.create_item(self.user.id, location.course_key, location.block_type, location.block_id)
         self.assertFalse(getattr(newobject, 'is_draft', False))
         with self.assertRaises(InvalidVersionError):
             draft_store.convert_to_draft(location, self.user.id)
@@ -1392,12 +1392,9 @@ class ContentStoreTest(ContentStoreTestCase):
 
     def test_forum_id_generation(self):
         course = CourseFactory.create(org='edX', course='999', display_name='Robot Super Course')
-        new_component_location = course.id.make_usage_key('discussion', 'new_component')
 
         # crate a new module and add it as a child to a vertical
-        self.store.create_item(self.user.id, new_component_location)
-
-        new_discussion_item = self.store.get_item(new_component_location)
+        new_discussion_item = self.store.create_item(self.user.id, course.id, 'discussion', 'new_component')
 
         self.assertNotEquals(new_discussion_item.discussion_id, '$$GUID$$')
 

cms/djangoapps/contentstore/tests/test_crud.py

@@ -162,7 +162,7 @@ class TemplateTests(unittest.TestCase):
         self.assertIsInstance(self.split_store.get_course(id_locator), CourseDescriptor)
         # and by guid
         self.assertIsInstance(self.split_store.get_item(guid_locator), CourseDescriptor)
-        self.split_store.delete_course(id_locator, ModuleStoreEnum.UserID.test)
+        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
@@ -187,11 +187,11 @@ class TemplateTests(unittest.TestCase):
         )
         first_problem.max_attempts = 3
         first_problem.save()  # decache the above into the kvs
-        updated_problem = self.split_store.update_item(first_problem, ModuleStoreEnum.UserID.test)
+        updated_problem = self.split_store.update_item(first_problem, 'testbot')
         self.assertIsNotNone(updated_problem.previous_version)
         self.assertEqual(updated_problem.previous_version, first_problem.update_version)
         self.assertNotEqual(updated_problem.update_version, first_problem.update_version)
-        self.split_store.delete_item(updated_problem.location, ModuleStoreEnum.UserID.test, 'testbot')
+        self.split_store.delete_item(updated_problem.location, 'testbot')
 
         second_problem = persistent_factories.ItemFactory.create(
             display_name='problem 2',

cms/djangoapps/contentstore/tests/test_orphan.py

@@ -34,7 +34,13 @@ class TestOrphan(CourseTestCase):
         location = self.course.location.replace(category=category, name=name)
         store = modulestore()
         store.create_item(
-            self.user.id, location, definition_data=data, metadata=metadata, runtime=runtime
+            self.user.id,
+            location.course_key,
+            location.block_type,
+            location.block_id,
+            definition_data=data,
+            metadata=metadata,
+            runtime=runtime
         )
         if parent_name:
             # add child to parent in mongo

cms/djangoapps/contentstore/tests/utils.py

@@ -151,11 +151,11 @@ class CourseTestCase(ModuleStoreTestCase):
         self.assertEqual(self.store.compute_publish_state(draft_vertical), PublishState.draft)
 
         # create a Private (draft only) vertical
-        private_vertical = self.store.create_item(self.user.id, course_id.make_usage_key('vertical', self.PRIVATE_VERTICAL))
+        private_vertical = self.store.create_item(self.user.id, course_id, 'vertical', self.PRIVATE_VERTICAL)
         self.assertEqual(self.store.compute_publish_state(private_vertical), PublishState.private)
 
         # create a Published (no draft) vertical
-        public_vertical = self.store.create_item(self.user.id, course_id.make_usage_key('vertical', self.PUBLISHED_VERTICAL))
+        public_vertical = self.store.create_item(self.user.id, course_id, 'vertical', self.PUBLISHED_VERTICAL)
         public_vertical = self.store.publish(public_vertical.location, self.user.id)
         self.assertEqual(self.store.compute_publish_state(public_vertical), PublishState.public)
 

cms/djangoapps/contentstore/views/item.py

@@ -287,7 +287,7 @@ def _save_item(user, usage_key, data=None, children=None, metadata=None, nullout
         if usage_key.category in CREATE_IF_NOT_FOUND:
             # New module at this location, for pages that are not pre-created.
             # Used for course info handouts.
-            existing_item = store.create_item(user.id, usage_key)
+            existing_item = store.create_item(user.id, usage_key.course_key, usage_key.block_type, usage_key.block_id)
         else:
             raise
     except InvalidLocationError:
@@ -416,9 +416,11 @@ def _create_item(request):
     if display_name is not None:
         metadata['display_name'] = display_name
 
-    created_block = store.create_item(
+    created_block = store.create_child(
         request.user.id,
-        dest_usage_key,
+        usage_key,
+        dest_usage_key.block_type,
+        block_id=dest_usage_key.block_id,
         definition_data=data,
         metadata=metadata,
         runtime=parent.runtime,
@@ -437,11 +439,6 @@ def _create_item(request):
         )
         store.update_item(course, request.user.id)
 
-    # TODO replace w/ nicer accessor
-    if not 'detached' in parent.runtime.load_block_type(category)._class_tags:
-        parent.children.append(created_block.location)
-        store.update_item(parent, request.user.id)
-
     return JsonResponse({"locator": unicode(created_block.location), "courseKey": unicode(created_block.location.course_key)})
 
 
@@ -467,7 +464,9 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_
 
     dest_module = store.create_item(
         user.id,
-        dest_usage_key,
+        dest_usage_key.course_key,
+        dest_usage_key.block_type,
+        block_id=dest_usage_key.block_id,
         definition_data=source_item.get_explicitly_set_fields_by_scope(Scope.content),
         metadata=duplicate_metadata,
         runtime=source_item.runtime,
@@ -555,7 +554,7 @@ def _get_module_info(usage_key, user, rewrite_static_links=True):
     except ItemNotFoundError:
         if usage_key.category in CREATE_IF_NOT_FOUND:
             # Create a new one for certain categories only. Used for course info handouts.
-            module = store.create_item(user.id, usage_key)
+            module = store.create_item(user.id, usage_key.course_key, usage_key.block_type, block_id=usage_key.block_id)
         else:
             raise
 

cms/djangoapps/contentstore/views/tests/test_course_updates.py

@@ -130,7 +130,12 @@ class CourseUpdateTest(CourseTestCase):
         '''
         # get the updates and populate 'data' field with some data.
         location = self.course.id.make_usage_key('course_info', 'updates')
-        course_updates = modulestore().create_item(self.user.id, location)
+        course_updates = modulestore().create_item(
+            self.user.id,
+            location.course_key,
+            location.block_type,
+            block_id=location.block_id
+        )
         update_date = u"January 23, 2014"
         update_content = u"Hello world!"
         update_data = u"<ol><li><h2>" + update_date + "</h2>" + update_content + "</li></ol>"
@@ -204,7 +209,12 @@ class CourseUpdateTest(CourseTestCase):
         '''Test trying to add to a saved course_update which is not an ol.'''
         # get the updates and set to something wrong
         location = self.course.id.make_usage_key('course_info', 'updates')
-        modulestore().create_item(self.user.id, location)
+        modulestore().create_item(
+            self.user.id,
+            location.course_key,
+            location.block_type,
+            block_id=location.block_id
+        )
         course_updates = modulestore().get_item(location)
         course_updates.data = 'bad news'
         modulestore().update_item(course_updates, self.user.id)
@@ -229,8 +239,7 @@ class CourseUpdateTest(CourseTestCase):
         """
         Test that a user can successfully post on course updates and handouts of a course
         """
-        course_key = SlashSeparatedCourseKey('Org1', 'Course_1', 'Run_1')
-        course_update_url = self.create_update_url(course_key=course_key)
+        course_update_url = self.create_update_url(course_key=self.course.id)
 
         # create a course via the view handler
         self.client.ajax_post(course_update_url)

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -368,6 +368,25 @@ class ModuleStoreWrite(ModuleStoreRead):
         pass
 
     @abstractmethod
+    def create_item(self, user_id, course_key, block_type, block_id=None, fields=None, **kwargs):
+        """
+        Creates and saves a new item in a course.
+
+        Returns the newly created item.
+
+        Args:
+            user_id: ID of the user creating and saving the xmodule
+            course_key: A :class:`~opaque_keys.edx.CourseKey` identifying which course to create
+                this item in
+            block_type: The type of block to create
+            block_id: a unique identifier for the new item. If not supplied,
+                a new identifier will be generated
+            fields (dict): A dictionary specifying initial values for some or all fields
+                in the newly created block
+        """
+        pass
+
+    @abstractmethod
     def clone_course(self, source_course_id, dest_course_id, user_id):
         """
         Sets up source_course_id to point a course with the same content as the desct_course_id. This
@@ -561,54 +580,6 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite):
             result[field.scope][field_name] = value
         return result
 
-    def create_item(self, user_id, location, parent_location=None, category=None, **kwargs):
-        """
-        Creates and saves a new item.
-        Either location or (category, parent_location) or both must be provided.
-        If parent_location is provided, a new item of the given category is added as a child.
-        If location is not provided, a new item with the given category and given block_id
-          is added to the parent_location.  If the block_id is not provided, a unique name
-          is automatically generated.
-
-        Returns the newly created item.
-
-        :param user_id: ID of the user creating and saving the xmodule
-        :param location: a Location--must have a category
-        :param parent_location: optional parameter, specifying the Location of the parent item
-        :param category: optional parameter for the category of the new item
-        :param block_id: a unique identifier for the new item
-        """
-        raise NotImplementedError
-
-    def update_item(self, xblock, user_id, allow_not_found=False, force=False):
-        """
-        Update the given xblock's persisted repr. Pass the user's unique id which the persistent store
-        should save with the update if it has that ability.
-
-        :param allow_not_found: whether this method should raise an exception if the given xblock
-        has not been persisted before.
-        :param force: fork the structure and don't update the course draftVersion if there's a version
-        conflict (only applicable to version tracking and conflict detecting persistence stores)
-
-        :raises VersionConflictError: if org, course, run, and version_guid given and the current
-        version head != version_guid and force is not True. (only applicable to version tracking stores)
-        """
-        raise NotImplementedError
-
-    def delete_item(self, location, user_id, force=False):
-        """
-        Delete an item from persistence. Pass the user's unique id which the persistent store
-        should save with the update if it has that ability.
-
-        :param user_id: ID of the user deleting the item
-        :param force: fork the structure and don't update the course draftVersion if there's a version
-        conflict (only applicable to version tracking and conflict detecting persistence stores)
-
-        :raises VersionConflictError: if org, course, run, and version_guid given and the current
-        version head != version_guid and force is not True. (only applicable to version tracking stores)
-        """
-        raise NotImplementedError
-
     def clone_course(self, source_course_id, dest_course_id, user_id):
         """
         This base method just copies the assets. The lower level impls must do the actual cloning of
@@ -639,6 +610,27 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite):
             self.contentstore._drop_database()  # pylint: disable=protected-access
         super(ModuleStoreWriteBase, self)._drop_database()  # pylint: disable=protected-access
 
+    def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fields=None, **kwargs):
+        """
+        Creates and saves a new xblock that as a child of the specified block
+
+        Returns the newly created item.
+
+        Args:
+            user_id: ID of the user creating and saving the xmodule
+            parent_usage_key: a :class:`~opaque_key.edx.UsageKey` identifing the
+                block that this item should be parented under
+            block_type: The type of block to create
+            block_id: a unique identifier for the new item. If not supplied,
+                a new identifier will be generated
+            fields (dict): A dictionary specifying initial values for some or all fields
+                in the newly created block
+        """
+        item = self.create_item(user_id, parent_usage_key.course_key, block_type, block_id=block_id, fields=fields, **kwargs)
+        parent = self.get_item(parent_usage_key)
+        parent.children.append(item.location)
+        self.update_item(parent, user_id)
+
     @contextmanager
     def bulk_write_operations(self, course_id):
         """
@@ -699,18 +691,3 @@ class EdxJSONEncoder(json.JSONEncoder):
                 return obj.isoformat()
         else:
             return super(EdxJSONEncoder, self).default(obj)
-
-
-def compute_location_from_args(location=None, parent_location=None, **kwargs):
-    """
-    Returns a Location object that is generated from the given arguments, as follows:
-        If location is provided, returns it.
-        If location is not provided, returns a location with the given category and given block_id.
-        If the block_id is not provided, a unique name is automatically generated.
-    """
-    if location is None:
-        block_id = kwargs.get('block_id', kwargs.get('name', kwargs.get('display_name', uuid4().hex)))
-        category = kwargs.pop('category')
-        course_key = getattr(parent_location, 'course_key', parent_location)
-        location = course_key.make_usage_key(category, Location.clean(block_id))
-    return location

common/lib/xmodule/xmodule/modulestore/draft_and_published.py

@@ -12,8 +12,8 @@ class ModuleStoreDraftAndPublished(object):
     """
     __metaclass__ = ABCMeta
 
-    def __init__(self, **kwargs):
-        super(ModuleStoreDraftAndPublished, self).__init__(**kwargs)
+    def __init__(self, *args, **kwargs):
+        super(ModuleStoreDraftAndPublished, self).__init__(*args, **kwargs)
         self.branch_setting_func = kwargs.pop('branch_setting_func', lambda: ModuleStoreEnum.Branch.published_only)
 
     @abstractmethod

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

@@ -7,21 +7,23 @@ In this way, courses can be served up both - say - XMLModuleStore or MongoModule
 
 import logging
 from contextlib import contextmanager
-from opaque_keys import InvalidKeyError
+import itertools
 
-from . import ModuleStoreWriteBase
-from xmodule.modulestore import ModuleStoreEnum
-from xmodule.modulestore.exceptions import ItemNotFoundError
+from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
-import itertools
-from xmodule.modulestore.split_migrator import SplitMigrator
+
+from . import ModuleStoreWriteBase
+from . import ModuleStoreEnum
+from .exceptions import ItemNotFoundError
+from .draft_and_published import ModuleStoreDraftAndPublished
+from .split_migrator import SplitMigrator
 
 
 log = logging.getLogger(__name__)
 
 
-class MixedModuleStore(ModuleStoreWriteBase):
+class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
     """
     ModuleStore knows how to route requests to the right persistence ms
     """
@@ -282,7 +284,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
 
         Returns: a CourseDescriptor
         """
-        store = self._get_modulestore_for_courseid(None)
+        store = self._verify_modulestore_support(None, 'create_course')
         return store.create_course(org, course, run, user_id, **kwargs)
 
     def clone_course(self, source_course_id, dest_course_id, user_id):
@@ -312,40 +314,57 @@ class MixedModuleStore(ModuleStoreWriteBase):
                 source_course_id, user_id, dest_course_id.org, dest_course_id.course, dest_course_id.run
             )
 
-    def create_item(self, user_id, location=None, parent_location=None, **kwargs):
+    def create_item(self, user_id, course_key, block_type, block_id=None, fields=None, **kwargs):
         """
-        Creates and saves a new item.
-        Either location or (category, parent_location) or both must be provided.
-        If parent_location is provided, a new item of the given category is added as a child.
-        If location is not provided, a new item with the given category and given block_id
-          is added to the parent_location.  If the block_id is not provided, a unique name
-          is automatically generated.
+        Creates and saves a new item in a course.
 
         Returns the newly created item.
 
-        :param user_id: ID of the user creating and saving the xmodule
-        :param location: a Location--must have a category
-        :param parent_location: optional parameter, specifying the Location of the parent item
-        :param category: optional parameter for the category of the new item
-        :param block_id: a unique identifier for the new item
+        Args:
+            user_id: ID of the user creating and saving the xmodule
+            course_key: A :class:`~opaque_keys.edx.CourseKey` identifying which course to create
+                this item in
+            block_type: The typo of block to create
+            block_id: a unique identifier for the new item. If not supplied,
+                a new identifier will be generated
+            fields (dict): A dictionary specifying initial values for some or all fields
+                in the newly created block
+        """
+        modulestore = self._verify_modulestore_support(course_key, 'create_item')
+        return modulestore.create_item(user_id, course_key, block_type, block_id=block_id, fields=fields, **kwargs)
+
+    def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fields=None, **kwargs):
         """
-        location = compute_location_from_args(location, parent_location, **kwargs)
-        modulestore = self._verify_modulestore_support(location, 'create_item')
-        return modulestore.create_item(user_id, location, parent_location, **kwargs)
+        Creates and saves a new xblock that is a child of the specified block
+
+        Returns the newly created item.
+
+        Args:
+            user_id: ID of the user creating and saving the xmodule
+            parent_usage_key: a :class:`~opaque_key.edx.UsageKey` identifying the
+                block that this item should be parented under
+            block_type: The typo of block to create
+            block_id: a unique identifier for the new item. If not supplied,
+                a new identifier will be generated
+            fields (dict): A dictionary specifying initial values for some or all fields
+                in the newly created block
+        """
+        modulestore = self._verify_modulestore_support(parent_usage_key.course_key, 'create_child')
+        return modulestore.create_child(user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, **kwargs)
 
     def update_item(self, xblock, user_id, allow_not_found=False):
         """
         Update the xblock persisted to be the same as the given for all types of fields
         (content, children, and metadata) attribute the change to the given user.
         """
-        store = self._verify_modulestore_support(xblock.location, 'update_item')
+        store = self._verify_modulestore_support(xblock.location.course_key, 'update_item')
         return store.update_item(xblock, user_id, allow_not_found)
 
     def delete_item(self, location, user_id, **kwargs):
         """
         Delete the given item from persistence. kwargs allow modulestore specific parameters.
         """
-        store = self._verify_modulestore_support(location, 'delete_item')
+        store = self._verify_modulestore_support(location.course_key, 'delete_item')
         store.delete_item(location, user_id=user_id, **kwargs)
 
     def revert_to_published(self, location, user_id):
@@ -358,7 +377,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
 
         :raises InvalidVersionError: if no published version exists for the location specified
         """
-        store = self._verify_modulestore_support(location, 'revert_to_published')
+        store = self._verify_modulestore_support(location.course_key, 'revert_to_published')
         return store.revert_to_published(location, user_id)
 
     def close_all_connections(self):
@@ -388,7 +407,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
         :param runtime: if you already have an xblock from the course, the xblock.runtime value
         :param fields: a dictionary of field names and values for the new xmodule
         """
-        store = self._verify_modulestore_support(location, 'create_xmodule')
+        store = self._verify_modulestore_support(location.course_key, 'create_xmodule')
         return store.create_xmodule(location, definition_data, metadata, runtime, fields, **kwargs)
 
     def get_courses_for_wiki(self, wiki_slug):
@@ -433,7 +452,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
         Save a current draft to the underlying modulestore
         Returns the newly published item.
         """
-        store = self._verify_modulestore_support(location, 'publish')
+        store = self._verify_modulestore_support(location.course_key, 'publish')
         return store.publish(location, user_id)
 
     def unpublish(self, location, user_id):
@@ -441,7 +460,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
         Save a current draft to the underlying modulestore
         Returns the newly unpublished item.
         """
-        store = self._verify_modulestore_support(location, 'unpublish')
+        store = self._verify_modulestore_support(location.course_key, 'unpublish')
         return store.unpublish(location, user_id)
 
     def convert_to_draft(self, location, user_id):
@@ -451,18 +470,26 @@ class MixedModuleStore(ModuleStoreWriteBase):
 
         :param source: the location of the source (its revision must be None)
         """
-        store = self._verify_modulestore_support(location, 'convert_to_draft')
+        store = self._verify_modulestore_support(location.course_key, 'convert_to_draft')
         return store.convert_to_draft(location, user_id)
 
-    def _verify_modulestore_support(self, location, method):
+    def has_changes(self, usage_key):
+        """
+        Checks if the given block has unpublished changes
+        :param usage_key: the block to check
+        :return: True if the draft and published versions differ
+        """
+        store = self._verify_modulestore_support(usage_key.course_key, 'has_changes')
+        return store.has_changes(usage_key)
+
+    def _verify_modulestore_support(self, course_key, method):
         """
         Finds and returns the store that contains the course for the given location, and verifying
         that the store supports the given method.
 
         Raises NotImplementedError if the found store does not support the given method.
         """
-        course_id = location.course_key
-        store = self._get_modulestore_for_courseid(course_id)
+        store = self._get_modulestore_for_courseid(course_key)
         if hasattr(store, method):
             return store
         else:

common/lib/xmodule/xmodule/modulestore/mongo/base.py

@@ -17,6 +17,7 @@ import sys
 import logging
 import copy
 import re
+from uuid import uuid4
 
 from bson.son import SON
 from fs.osfs import OSFS
@@ -904,7 +905,12 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
                 ]))
 
         location = course_id.make_usage_key('course', course_id.run)
-        course = self.create_item(user_id, location, fields=fields, **kwargs)
+        course = self.create_xmodule(
+            location,
+            fields=fields,
+            **kwargs
+        )
+        self.update_item(course, user_id, allow_not_found=True)
 
         # clone a default 'about' overview module as well
         about_location = location.replace(
@@ -914,7 +920,9 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         overview_template = AboutDescriptor.get_template('overview.yaml')
         self.create_item(
             user_id,
-            about_location,
+            about_location.course_key,
+            about_location.block_type,
+            block_id=about_location.block_id,
             definition_data=overview_template.get('data'),
             runtime=course.system
         )
@@ -975,6 +983,52 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         xmodule.save()
         return xmodule
 
+    def create_item(self, user_id, course_key, block_type, block_id=None, **kwargs):
+        """
+        Creates and saves a new item in a course.
+
+        Returns the newly created item.
+
+        Args:
+            user_id: ID of the user creating and saving the xmodule
+            course_key: A :class:`~opaque_keys.edx.CourseKey` identifying which course to create
+                this item in
+            block_type: The typo of block to create
+            block_id: a unique identifier for the new item. If not supplied,
+                a new identifier will be generated
+        """
+        if block_id is None:
+            block_id = uuid4().hex
+
+        location = course_key.make_usage_key(block_type, block_id)
+        xblock = self.create_xmodule(location, **kwargs)
+        self.update_item(xblock, user_id, allow_not_found=True)
+
+        return xblock
+
+    def create_child(self, user_id, parent_usage_key, block_type, block_id=None, **kwargs):
+        """
+        Creates and saves a new xblock that as a child of the specified block
+
+        Returns the newly created item.
+
+        Args:
+            user_id: ID of the user creating and saving the xmodule
+            parent_usage_key: a :class:`~opaque_key.edx.UsageKey` identifing the
+                block that this item should be parented under
+            block_type: The typo of block to create
+            block_id: a unique identifier for the new item. If not supplied,
+                a new identifier will be generated
+        """
+        xblock = self.create_item(user_id, parent_usage_key.course_key, block_type, block_id=block_id, **kwargs)
+        # attach to parent if given
+        if 'detached' not in xblock._class_tags:
+            parent = self.get_item(parent_usage_key)
+            parent.children.append(xblock.location)
+            self.update_item(parent, user_id)
+
+        return xblock
+
     def _get_course_for_item(self, location, depth=0):
         '''
         for a given Xmodule, return the course that it belongs to
@@ -1065,6 +1119,9 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         except ItemNotFoundError:
             if not allow_not_found:
                 raise
+            elif not self.has_course(xblock.location.course_key):
+                raise ItemNotFoundError(xblock.location.course_key)
+
 
         return xblock
 

common/lib/xmodule/xmodule/modulestore/mongo/draft.py

@@ -11,7 +11,7 @@ import logging
 
 from opaque_keys.edx.locations import Location
 from xmodule.exceptions import InvalidVersionError
-from xmodule.modulestore import PublishState, ModuleStoreEnum, compute_location_from_args
+from xmodule.modulestore import PublishState, ModuleStoreEnum
 from xmodule.modulestore.exceptions import (
     ItemNotFoundError, DuplicateItemError, InvalidBranchSetting, DuplicateCourseError
 )
@@ -305,35 +305,6 @@ class DraftModuleStore(MongoModuleStore):
             super(DraftModuleStore, self).create_xmodule(location, definition_data, metadata, runtime, fields)
         )
 
-    def create_item(self, user_id, location=None, parent_location=None, **kwargs):
-        """
-        Creates and saves a new item.
-        Either location or (category, parent_location) or both must be provided.
-        If parent_location is provided, a new item of the given category is added as a child.
-        If location is not provided, a new item with the given category and given block_id
-          is added to the parent_location.  If the block_id is not provided, a unique name
-          is automatically generated.
-
-        Returns the newly created item.
-
-        :param user_id: ID of the user creating and saving the xmodule
-        :param location: a Location--must have a category
-        :param parent_location: optional parameter, specifying the Location of the parent item
-        :param category: optional parameter for the category of the new item
-        :param block_id: a unique identifier for the new item
-        """
-        location = compute_location_from_args(location, parent_location, **kwargs)
-        xblock = self.create_xmodule(location, **kwargs)
-        self.update_item(xblock, user_id, allow_not_found=True)
-
-        # attach to parent if given
-        if parent_location is not None and not 'detached' in xblock._class_tags:
-            parent = self.get_item(parent_location)
-            parent.children.append(location)
-            self.update_item(parent, user_id)
-
-        return xblock
-
     def get_items(self, course_key, settings=None, content=None, revision=None, **kwargs):
         """
         Performance Note: This is generally a costly operation, but useful for wildcard searches.
@@ -511,6 +482,7 @@ class DraftModuleStore(MongoModuleStore):
                 ModuleStoreEnum.RevisionOption.published_only - removes only Published versions
                 ModuleStoreEnum.RevisionOption.all - removes both Draft and Published parents
                     currently only provided by contentstore.views.item.orphan_handler
+                Otherwise, raises a ValueError.
         """
         self._verify_branch_setting(ModuleStoreEnum.Branch.draft_preferred)
         _verify_revision_is_published(location)
@@ -555,8 +527,10 @@ class DraftModuleStore(MongoModuleStore):
             as_functions = [as_draft, as_published]
         elif revision == ModuleStoreEnum.RevisionOption.published_only:
             as_functions = [as_published]
-        else:
+        elif revision is None:
             as_functions = [as_draft]
+        else:
+            raise ValueError('revision not one of None, ModuleStoreEnum.RevisionOption.published_only, or ModuleStoreEnum.RevisionOption.all')
         self._delete_subtree(location, as_functions)
 
     def _delete_subtree(self, location, as_functions):

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

@@ -85,8 +85,8 @@ class SplitMigrator(object):
                 # the 'children' field as it goes.
                 _new_module = self.split_modulestore.create_item(
                     user_id,
-                    course_version_locator.make_usage_key(module.location.category, module.location.block_id),
-                    parent_location=course_version_locator,
+                    course_version_locator,
+                    module.location.block_type,
                     block_id=module.location.block_id,
                     fields=self._get_json_fields_translate_references(
                         module, course_version_locator, new_course.location.block_id
@@ -131,7 +131,8 @@ class SplitMigrator(object):
             else:
                 # only a draft version (aka, 'private').
                 _new_module = self.split_modulestore.create_item(
-                    user_id, new_locator, parent_location=new_draft_course_loc,
+                    user_id, new_draft_course_loc,
+                    new_locator.block_type,
                     block_id=new_locator.block_id,
                     fields=self._get_json_fields_translate_references(
                         module, new_draft_course_loc, published_course_usage_key.block_id

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

@@ -65,7 +65,7 @@ from opaque_keys.edx.locator import (
 from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError, \
     DuplicateCourseError
 from xmodule.modulestore import (
-    inheritance, ModuleStoreWriteBase, ModuleStoreEnum, compute_location_from_args
+    inheritance, ModuleStoreWriteBase, ModuleStoreEnum
 )
 
 from ..exceptions import ItemNotFoundError
@@ -267,7 +267,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         :param course_version_guid: if provided, clear only this entry
         """
         if course_version_guid:
-            del self.thread_cache.course_cache[course_version_guid]
+            try:
+                del self.thread_cache.course_cache[course_version_guid]
+            except KeyError:
+                pass
         else:
             self.thread_cache.course_cache = {}
 
@@ -630,7 +633,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         Find the history of this block. Return as a VersionTree of each place the block changed (except
         deletion).
 
-        The block's history tracks its explicit changes but not the changes in its children.
+        The block's history tracks its explicit changes but not the changes in its children starting
+        from when the block was created.
 
         '''
         # course_agnostic means we don't care if the head and version don't align, trust the version
@@ -640,7 +644,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         all_versions_with_block = self.db_connection.find_matching_structures(
             {
                 'original_version': course_struct['original_version'],
-                update_version_field: {'$exists': True}
+                update_version_field: {'$exists': True},
             }
         )
         # find (all) root versions and build map {previous: {successors}..}
@@ -650,6 +654,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             block_payload = self._get_block_from_structure(version, block_id)
             if version['_id'] == block_payload['edit_info']['update_version']:
                 if block_payload['edit_info'].get('previous_version') is None:
+                    # this was when this block was created
                     possible_roots.append(block_payload['edit_info']['update_version'])
                 else:  # map previous to {update..}
                     result.setdefault(block_payload['edit_info']['previous_version'], set()).add(
@@ -763,19 +768,16 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             serial += 1
         return category + str(serial)
 
-    # 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.
-    def create_item(self, user_id, location=None, parent_location=None,
-        definition_locator=None, force=False, continue_version=False, **kwargs
+    def create_item(
+        self, user_id, course_key, block_type, block_id=None,
+        definition_locator=None, fields=None,
+        force=False, continue_version=False, **kwargs
     ):
         """
-        Add a descriptor to persistence as the last child of the optional parent_location or just as an element
-        of the course (if no parent provided). Return the resulting post saved version with populated locators.
+        Add a descriptor to persistence as an element
+        of the course. Return the resulting post saved version with populated locators.
 
-        :param course_or_parent_locator: If BlockUsageLocator, then it's assumed to be the parent.
-        If it's a CourseLocator, then it's
-        merely the containing course. If it has a version_guid and a course org + course + run + branch, this
+        :param course_key: If it has a version_guid and a course org + course + run + branch, this
         method ensures that the version is the head of the given course branch before making the change.
 
         raises InsufficientSpecificationError if there is no course locator.
@@ -815,27 +817,15 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         the course id'd by version_guid but instead in one w/ a new version_guid. Ensure in this case that you get
         the new version_guid from the locator in the returned object!
         """
-        location = compute_location_from_args(location, parent_location, **kwargs)
-
-        if not isinstance(location, (CourseLocator, BlockUsageLocator)):
-            raise ValueError(u"Cannot create item {} in split. Wrong repr.".format(location))
-
-        # convert fields into a single dict if separated by scope
-        fields = kwargs.get('fields', {})
-        fields.update(kwargs.pop('metadata', {}))
-        fields.update(kwargs.pop('definition_data', {}))
-        kwargs['fields'] = fields
-
         # find course_index entry if applicable and structures entry
-        index_entry = self._get_index_if_valid(location, force, continue_version)
-        structure = self._lookup_course(location)['structure']
+        index_entry = self._get_index_if_valid(course_key, force, continue_version)
+        structure = self._lookup_course(course_key)['structure']
 
-        category = location.block_type
-        partitioned_fields = self.partition_fields_by_scope(category, fields)
+        partitioned_fields = self.partition_fields_by_scope(block_type, fields)
         new_def_data = partitioned_fields.get(Scope.content, {})
         # persist the definition if persisted != passed
         if (definition_locator is None or isinstance(definition_locator.definition_id, LocalId)):
-            definition_locator = self.create_definition_from_data(new_def_data, category, user_id)
+            definition_locator = self.create_definition_from_data(new_def_data, block_type, user_id)
         elif new_def_data is not None:
             definition_locator, _ = self.update_definition_from_data(definition_locator, new_def_data, user_id)
 
@@ -848,22 +838,21 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         new_id = new_structure['_id']
 
         # generate usage id
-        block_id = kwargs.pop('block_id', location.block_id)
         if block_id is not None:
             if encode_key_for_mongo(block_id) in new_structure['blocks']:
                 raise DuplicateItemError(block_id, self, 'structures')
             else:
                 new_block_id = block_id
         else:
-            new_block_id = self._generate_block_id(new_structure['blocks'], category)
+            new_block_id = self._generate_block_id(new_structure['blocks'], block_type)
 
         block_fields = partitioned_fields.get(Scope.settings, {})
         if Scope.children in partitioned_fields:
             block_fields.update(partitioned_fields[Scope.children])
         self._update_block_in_structure(new_structure, new_block_id, {
-            "category": category,
+            "category": block_type,
             "definition": definition_locator.definition_id,
-            "fields": self._serialize_fields(category, block_fields),
+            "fields": self._serialize_fields(block_type, block_fields),
             'edit_info': {
                 'edited_on': datetime.datetime.now(UTC),
                 'edited_by': user_id,
@@ -872,17 +861,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             }
         })
 
-        # if given parent, add new block as child and update parent's version
-        parent = None
-        if isinstance(parent_location, BlockUsageLocator) and parent_location.block_id is not None:
-            encoded_block_id = encode_key_for_mongo(parent_location.block_id)
-            parent = new_structure['blocks'][encoded_block_id]
-            parent['fields'].setdefault('children', []).append(new_block_id)
-            if not continue_version or parent['edit_info']['update_version'] != structure['_id']:
-                parent['edit_info']['edited_on'] = datetime.datetime.now(UTC)
-                parent['edit_info']['edited_by'] = user_id
-                parent['edit_info']['previous_version'] = parent['edit_info']['update_version']
-                parent['edit_info']['update_version'] = new_id
         if continue_version:
             # db update
             self.db_connection.update_structure(new_structure)
@@ -894,22 +872,68 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         # update the index entry if appropriate
         if index_entry is not None:
             if not continue_version:
-                self._update_head(index_entry, location.branch, new_id)
+                self._update_head(index_entry, course_key.branch, new_id)
             item_loc = BlockUsageLocator(
-                location.version_agnostic(),
-                block_type=category,
+                course_key.version_agnostic(),
+                block_type=block_type,
                 block_id=new_block_id,
             )
         else:
             item_loc = BlockUsageLocator(
                 CourseLocator(version_guid=new_id),
-                block_type=category,
+                block_type=block_type,
                 block_id=new_block_id,
             )
 
         # reconstruct the new_item from the cache
         return self.get_item(item_loc)
 
+    def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fields=None, continue_version=False, **kwargs):
+        """
+        Creates and saves a new xblock that as a child of the specified block
+
+        Returns the newly created item.
+
+        Args:
+            user_id: ID of the user creating and saving the xmodule
+            parent_usage_key: a :class:`~opaque_key.edx.UsageKey` identifying the
+                block that this item should be parented under
+            block_type: The typo of block to create
+            block_id: a unique identifier for the new item. If not supplied,
+                a new identifier will be generated
+            fields (dict): A dictionary specifying initial values for some or all fields
+                in the newly created block
+        """
+        xblock = self.create_item(
+            user_id, parent_usage_key.course_key, block_type, block_id=block_id, fields=fields,
+            continue_version=continue_version,
+            **kwargs)
+
+        # don't version the structure as create_item handled that already.
+        new_structure = self._lookup_course(xblock.location.course_key)['structure']
+
+        # add new block as child and update parent's version
+        encoded_block_id = encode_key_for_mongo(parent_usage_key.block_id)
+        parent = new_structure['blocks'][encoded_block_id]
+        parent['fields'].setdefault('children', []).append(xblock.location.block_id)
+        if parent['edit_info']['update_version'] != new_structure['_id']:
+            # if the parent hadn't been previously changed in this bulk transaction, indicate that it's
+            # part of the bulk transaction
+            parent['edit_info'] = {
+                'edited_on': datetime.datetime.now(UTC),
+                'edited_by': user_id,
+                'previous_version': parent['edit_info']['update_version'],
+                'update_version': new_structure['_id'],
+            }
+
+        # db update
+        self.db_connection.update_structure(new_structure)
+            # clear cache so things get refetched and inheritance recomputed
+        self._clear_cache(new_structure['_id'])
+
+        # don't need to update the index b/c create_item did it for this version
+        return xblock
+
     def clone_course(self, source_course_id, dest_course_id, user_id):
         """
         See :meth: `.ModuleStoreWrite.clone_course` for documentation.

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

@@ -6,6 +6,7 @@ from ..exceptions import ItemNotFoundError
 from split import SplitMongoModuleStore
 from xmodule.modulestore import ModuleStoreEnum, PublishState
 from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished
+from xmodule.modulestore.draft import DIRECT_ONLY_CATEGORIES
 
 
 class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleStore):
@@ -13,13 +14,10 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
     A subclass of Split that supports a dual-branch fall-back versioning framework
         with a Draft branch that falls back to a Published branch.
     """
-    def __init__(self, **kwargs):
-        super(DraftVersioningModuleStore, self).__init__(**kwargs)
-
     def create_course(self, org, course, run, user_id, **kwargs):
         master_branch = kwargs.pop('master_branch', ModuleStoreEnum.BranchName.draft)
         return super(DraftVersioningModuleStore, self).create_course(
-            org, course, run, user_id, master_branch, **kwargs
+            org, course, run, user_id, master_branch=master_branch, **kwargs
         )
 
     def get_courses(self):
@@ -31,32 +29,81 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
     def delete_item(self, location, user_id, revision=None, **kwargs):
         """
         Delete the given item from persistence. kwargs allow modulestore specific parameters.
+
+        Args:
+            location: UsageKey of the item to be deleted
+            user_id: id of the user deleting the item
+            revision:
+                None - deletes the item and its subtree, and updates the parents per description above
+                ModuleStoreEnum.RevisionOption.published_only - removes only Published versions
+                ModuleStoreEnum.RevisionOption.all - removes both Draft and Published parents
+                    currently only provided by contentstore.views.item.orphan_handler
+                Otherwise, raises a ValueError.
         """
         if revision == ModuleStoreEnum.RevisionOption.published_only:
             branches_to_delete = [ModuleStoreEnum.BranchName.published]
         elif revision == ModuleStoreEnum.RevisionOption.all:
             branches_to_delete = [ModuleStoreEnum.BranchName.published, ModuleStoreEnum.BranchName.draft]
-        else:
+        elif revision is None:
             branches_to_delete = [ModuleStoreEnum.BranchName.draft]
+        else:
+            raise ValueError('revision not one of None, ModuleStoreEnum.RevisionOption.published_only, or ModuleStoreEnum.RevisionOption.all')
         for branch in branches_to_delete:
             SplitMongoModuleStore.delete_item(self, location.for_branch(branch), user_id, **kwargs)
 
-    def get_item(self, usage_key, depth=0, revision=None):
+    def _map_revision_to_branch(self, key, revision=None):
         """
-        Returns the item identified by usage_key and revision.
+        Maps RevisionOptions to BranchNames, inserting them into the key
         """
-        def for_branch(branch_state):
-            if usage_key.branch is not None and usage_key.branch is not branch_state:
-                raise ValueError('{} already has a branch.'.format(usage_key))
-            return usage_key.for_branch(branch_state)
         if revision == ModuleStoreEnum.RevisionOption.published_only:
-            usage_key = for_branch(ModuleStoreEnum.BranchName.published)
+            return key.for_branch(ModuleStoreEnum.BranchName.published)
         elif revision == ModuleStoreEnum.RevisionOption.draft_only:
-            usage_key = for_branch(ModuleStoreEnum.BranchName.draft)
+            return key.for_branch(ModuleStoreEnum.BranchName.draft)
+        else:
+            return key
+
+    def has_item(self, usage_key, revision=None):
+        """
+        Returns True if location exists in this ModuleStore.
+        """
+        usage_key = self._map_revision_to_branch(usage_key, revision=revision)
+        return super(DraftVersioningModuleStore, self).has_item(usage_key)
+
+    def get_item(self, usage_key, depth=0, revision=None):
+        """
+        Returns the item identified by usage_key and revision.
+        """
+        usage_key = self._map_revision_to_branch(usage_key, revision=revision)
         return super(DraftVersioningModuleStore, self).get_item(usage_key, depth=depth)
 
+    def get_items(self, course_locator, settings=None, content=None, revision=None, **kwargs):
+        """
+        Returns a list of XModuleDescriptor instances for the matching items within the course with
+        the given course_locator.
+        """
+        course_locator = self._map_revision_to_branch(course_locator, revision=revision)
+        return super(DraftVersioningModuleStore, self).get_items(
+            course_locator,
+            settings=settings,
+            content=content,
+            **kwargs
+        )
+
     def get_parent_location(self, location, revision=None, **kwargs):
-        # NAATODO - support draft_preferred
+        '''
+        Returns the given location's parent location in this course.
+        Args:
+            revision:
+                None - uses the branch setting for the revision
+                ModuleStoreEnum.RevisionOption.published_only
+                    - return only the PUBLISHED parent if it exists, else returns None
+                ModuleStoreEnum.RevisionOption.draft_preferred
+                    - return either the DRAFT or PUBLISHED parent, preferring DRAFT, if parent(s) exists,
+                        else returns None
+        '''
+        if revision == ModuleStoreEnum.RevisionOption.draft_preferred:
+            revision = ModuleStoreEnum.RevisionOption.draft_only
+        location = self._map_revision_to_branch(location, revision=revision)
         return SplitMongoModuleStore.get_parent_location(self, location, **kwargs)
 
     def has_changes(self, usage_key):
@@ -65,6 +112,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         :param usage_key: the block to check
         :return: True if the draft and published versions differ
         """
+        # TODO for better performance: lookup the courses and get the block entry, don't create the instances
         draft = self.get_item(usage_key.for_branch(ModuleStoreEnum.BranchName.draft))
         try:
             published = self.get_item(usage_key.for_branch(ModuleStoreEnum.BranchName.published))
@@ -120,20 +168,21 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
             course_structure = self._lookup_course(xblock.location.course_key.for_branch(branch))['structure']
             return self._get_block_from_structure(course_structure, xblock.location.block_id)
 
-        if xblock.location.branch is None:
-            raise ValueError(u'{} is not in a branch; so, this is nonsensical'.format(xblock.location))
         if xblock.location.branch == ModuleStoreEnum.BranchName.draft:
-            other = get_head(ModuleStoreEnum.BranchName.published)
+            try:
+                other = get_head(ModuleStoreEnum.BranchName.published)
+            except ItemNotFoundError:
+                return PublishState.private
         elif xblock.location.branch == ModuleStoreEnum.BranchName.published:
             other = get_head(ModuleStoreEnum.BranchName.draft)
         else:
-            raise ValueError(u'{} is not in a branch other than draft or published; so, this is nonsensical'.format(xblock.location))
+            raise ValueError(u'{} is in a branch other than draft or published.'.format(xblock.location))
 
         if not other:
             if xblock.location.branch == ModuleStoreEnum.BranchName.draft:
                 return PublishState.private
             else:
-                return PublishState.public  # a bit nonsensical
+                return PublishState.public
         elif xblock.update_version != other['edit_info']['update_version']:
             return PublishState.draft
         else:

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

@@ -174,7 +174,15 @@ class ItemFactory(XModuleFactory):
             if display_name is not None:
                 metadata['display_name'] = display_name
             runtime = parent.runtime if parent else None
-            store.create_item(user_id, location, metadata=metadata, definition_data=data, runtime=runtime)
+            store.create_item(
+                user_id,
+                location.course_key,
+                location.block_type,
+                block_id=location.block_id,
+                metadata=metadata,
+                definition_data=data,
+                runtime=runtime
+            )
 
             module = store.get_item(location)
 

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

@@ -4,6 +4,7 @@ from xmodule.course_module import CourseDescriptor
 from xmodule.x_module import XModuleDescriptor
 import factory
 from factory.helpers import lazy_attribute
+from opaque_keys.edx.keys import UsageKey
 # Factories don't have __init__ methods, and are self documenting
 # pylint: disable=W0232, C0111
 
@@ -73,10 +74,16 @@ class ItemFactory(SplitFactory):
         :param definition_locator (optional): the DescriptorLocator for the definition this uses or branches
         """
         modulestore = kwargs.pop('modulestore')
-        return modulestore.create_item(
-            user_id, category=category, parent_location=parent_location, defintion_locator=definition_locator,
-            force=force, continue_version=continue_version, **kwargs
-        )
+        if isinstance(parent_location, UsageKey):
+            return modulestore.create_child(
+                user_id, parent_location, category, defintion_locator=definition_locator,
+                force=force, continue_version=continue_version, **kwargs
+            )
+        else:
+            return modulestore.create_item(
+                user_id, parent_location, category, defintion_locator=definition_locator,
+                force=force, continue_version=continue_version, **kwargs
+            )
 
     @classmethod
     def _build(cls, target_class, *args, **kwargs):

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

@@ -117,24 +117,21 @@ class TestMixedModuleStore(unittest.TestCase):
         """
         Create a course w/ one item in the persistence store using the given course & item location.
         """
+        # create course
         self.course = self.store.create_course(course_key.org, course_key.course, course_key.run, self.user_id)
-        self.writable_chapter_location = self.course.id.make_usage_key('chapter', 'Overview').version_agnostic()
-        block_id = self.writable_chapter_location.name
-        chapter = self.store.create_item(
-            # don't use course_location as it may not be the repr
-            self.user_id, self.writable_chapter_location,
-            parent_location=self.course.location, block_id=block_id
-        )
         if isinstance(self.course.id, CourseLocator):
             self.course_locations[self.MONGO_COURSEID] = self.course.location.version_agnostic()
         else:
             self.assertEqual(self.course.id, course_key)
-            self.assertEqual(chapter.location, self.writable_chapter_location)
+
+        # create chapter
+        chapter = self.store.create_child(self.user_id, self.course.location, 'chapter', block_id='Overview')
+        self.writable_chapter_location = chapter.location.version_agnostic()
 
     def _create_block_hierarchy(self):
         """
         Creates a hierarchy of blocks for testing
-        Each block is assigned as a field of the class and can be easily accessed
+        Each block's (version_agnostic) location is assigned as a field of the class and can be easily accessed
         """
         BlockInfo = namedtuple('BlockInfo', 'field_name, category, display_name, sub_tree')
 
@@ -173,15 +170,13 @@ class TestMixedModuleStore(unittest.TestCase):
         ]
 
         def create_sub_tree(parent, block_info):
-            block = self.store.create_item(
-                self.user_id, parent_location=parent.location.version_agnostic(),
-                category=block_info.category, block_id=block_info.display_name
+            block = self.store.create_child(
+                self.user_id, parent.location.version_agnostic(),
+                block_info.category, block_id=block_info.display_name
             )
             for tree in block_info.sub_tree:
                 create_sub_tree(block, tree)
-            # reload the block to update its children field
-            block = self.store.get_item(block.location.version_agnostic())
-            setattr(self, block_info.field_name, block)
+            setattr(self, block_info.field_name, block.location.version_agnostic())
 
         for tree in trees:
             create_sub_tree(self.course, tree)
@@ -317,17 +312,17 @@ class TestMixedModuleStore(unittest.TestCase):
         self.store.delete_item(self.writable_chapter_location, self.user_id)
         # verify it's gone
         with self.assertRaises(ItemNotFoundError):
-            self.store.get_item(self.writable_chapter_location.version_agnostic())
+            self.store.get_item(self.writable_chapter_location)
 
         # create and delete a private vertical with private children
-        private_vert = self.store.create_item(
+        private_vert = self.store.create_child(
             # don't use course_location as it may not be the repr
-            self.user_id, parent_location=self.course_locations[self.MONGO_COURSEID],
-            category='vertical', block_id='private'
+            self.user_id, self.course_locations[self.MONGO_COURSEID],
+            'vertical', block_id='private'
         )
-        private_leaf = self.store.create_item(
+        private_leaf = self.store.create_child(
             # don't use course_location as it may not be the repr
-            self.user_id, parent_location=private_vert.location, category='html', block_id='private_leaf'
+            self.user_id, private_vert.location, 'html', block_id='private_leaf'
         )
 
         # verify pre delete state (just to verify that the test is valid)
@@ -357,18 +352,18 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertFalse(self.store.has_item(leaf_loc))
         self.assertNotIn(vert_loc, course.children)
 
-        # NAATODO enable for split after your converge merge
-        if default_ms == 'split':
-            return
+        # TODO can remove this once LMS-2869 is implemented
+        # first create a Published branch
+        self.store.publish(self.course_locations[self.MONGO_COURSEID], self.user_id)
 
         # reproduce bug STUD-1965
         # create and delete a private vertical with private children
-        private_vert = self.store.create_item(
+        private_vert = self.store.create_child(
             # don't use course_location as it may not be the repr
-            self.course_locations[self.MONGO_COURSEID], 'vertical', user_id=self.user_id, block_id='publish'
+             self.user_id, self.course_locations[self.MONGO_COURSEID], 'vertical', block_id='publish'
         )
-        private_leaf = self.store.create_item(
-            private_vert.location, 'html', user_id=self.user_id, block_id='bug_leaf'
+        private_leaf = self.store.create_child(
+            self.user_id, private_vert.location, 'html', block_id='bug_leaf'
         )
 
         self.store.publish(private_vert.location, self.user_id)
@@ -435,14 +430,13 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertEqual(parent, self.course_locations[self.XML_COURSEID1])
 
     def verify_get_parent_locations_results(self, expected_results):
-        # expected_results should be a list of (child, parent, revision)
-        for test in expected_results:
+        for child_location, parent_location, revision in expected_results:
             self.assertEqual(
-                test[1].location if test[1] else None,
-                self.store.get_parent_location(test[0].location, revision=test[2])
+                parent_location,
+                self.store.get_parent_location(child_location, revision=revision)
             )
 
-    @ddt.data('draft')
+    @ddt.data('draft', 'split')
     def test_get_parent_locations_moved_child(self, default_ms):
         self.initdb(default_ms)
         self._create_block_hierarchy()
@@ -451,30 +445,34 @@ class TestMixedModuleStore(unittest.TestCase):
         self.store.publish(self.course.location, self.user_id)
 
         # make drafts of verticals
-        self.store.convert_to_draft(self.vertical_x1a.location, self.user_id)
-        self.store.convert_to_draft(self.vertical_y1a.location, self.user_id)
+        self.store.convert_to_draft(self.vertical_x1a, self.user_id)
+        self.store.convert_to_draft(self.vertical_y1a, self.user_id)
 
         # move child problem_x1a_1 to vertical_y1a
-        child_to_move = self.problem_x1a_1
-        old_parent = self.vertical_x1a
-        new_parent = self.vertical_y1a
-        old_parent.children.remove(child_to_move.location)
-        new_parent.children.append(child_to_move.location)
+        child_to_move_location = self.problem_x1a_1
+        new_parent_location = self.vertical_y1a
+        old_parent_location = self.vertical_x1a
+
+        old_parent = self.store.get_item(old_parent_location)
+        old_parent.children.remove(child_to_move_location.replace(version_guid=old_parent.location.version_guid))
         self.store.update_item(old_parent, self.user_id)
+
+        new_parent = self.store.get_item(new_parent_location)
+        new_parent.children.append(child_to_move_location.replace(version_guid=new_parent.location.version_guid))
         self.store.update_item(new_parent, self.user_id)
 
         self.verify_get_parent_locations_results([
-            (child_to_move, new_parent, None),
-            (child_to_move, new_parent, ModuleStoreEnum.RevisionOption.draft_preferred),
-            (child_to_move, old_parent, ModuleStoreEnum.RevisionOption.published_only),
+            (child_to_move_location, new_parent_location, None),
+            (child_to_move_location, new_parent_location, ModuleStoreEnum.RevisionOption.draft_preferred),
+            (child_to_move_location, old_parent_location.for_branch(ModuleStoreEnum.BranchName.published), ModuleStoreEnum.RevisionOption.published_only),
         ])
 
         # publish the course again
         self.store.publish(self.course.location, self.user_id)
         self.verify_get_parent_locations_results([
-            (child_to_move, new_parent, None),
-            (child_to_move, new_parent, ModuleStoreEnum.RevisionOption.draft_preferred),
-            (child_to_move, new_parent, ModuleStoreEnum.RevisionOption.published_only),
+            (child_to_move_location, new_parent_location, None),
+            (child_to_move_location, new_parent_location, ModuleStoreEnum.RevisionOption.draft_preferred),
+            (child_to_move_location, new_parent_location.for_branch(ModuleStoreEnum.BranchName.published), ModuleStoreEnum.RevisionOption.published_only),
         ])
 
     @ddt.data('draft')
@@ -486,29 +484,28 @@ class TestMixedModuleStore(unittest.TestCase):
         self.store.publish(self.course.location, self.user_id)
 
         # make draft of vertical
-        self.store.convert_to_draft(self.vertical_y1a.location, self.user_id)
+        self.store.convert_to_draft(self.vertical_y1a, self.user_id)
 
         # delete child problem_y1a_1
-        child_to_delete = self.problem_y1a_1
-        old_parent = self.vertical_y1a
-        self.store.delete_item(child_to_delete.location, self.user_id)
+        child_to_delete_location = self.problem_y1a_1
+        old_parent_location = self.vertical_y1a
+        self.store.delete_item(child_to_delete_location, self.user_id)
 
         self.verify_get_parent_locations_results([
-            (child_to_delete, old_parent, None),
+            (child_to_delete_location, old_parent_location, None),
             # Note: The following could be an unexpected result, but we want to avoid an extra database call
-            (child_to_delete, old_parent, ModuleStoreEnum.RevisionOption.draft_preferred),
-            (child_to_delete, old_parent, ModuleStoreEnum.RevisionOption.published_only),
+            (child_to_delete_location, old_parent_location, ModuleStoreEnum.RevisionOption.draft_preferred),
+            (child_to_delete_location, old_parent_location, ModuleStoreEnum.RevisionOption.published_only),
         ])
 
         # publish the course again
         self.store.publish(self.course.location, self.user_id)
         self.verify_get_parent_locations_results([
-            (child_to_delete, None, None),
-            (child_to_delete, None, ModuleStoreEnum.RevisionOption.draft_preferred),
-            (child_to_delete, None, ModuleStoreEnum.RevisionOption.published_only),
+            (child_to_delete_location, None, None),
+            (child_to_delete_location, None, ModuleStoreEnum.RevisionOption.draft_preferred),
+            (child_to_delete_location, None, ModuleStoreEnum.RevisionOption.published_only),
         ])
 
-
     @ddt.data('draft')
     def test_revert_to_published_root_draft(self, default_ms):
         """
@@ -516,23 +513,25 @@ class TestMixedModuleStore(unittest.TestCase):
         """
         self.initdb(default_ms)
         self._create_block_hierarchy()
-        vertical_children_num = len(self.vertical_x1a.children)
+
+        vertical = self.store.get_item(self.vertical_x1a)
+        vertical_children_num = len(vertical.children)
 
         self.store.publish(self.course.location, self.user_id)
 
         # delete leaf problem (will make parent vertical a draft)
-        self.store.delete_item(self.problem_x1a_1.location, self.user_id)
+        self.store.delete_item(self.problem_x1a_1, self.user_id)
 
-        draft_parent = self.store.get_item(self.vertical_x1a.location)
+        draft_parent = self.store.get_item(self.vertical_x1a)
         self.assertEqual(vertical_children_num - 1, len(draft_parent.children))
         published_parent = self.store.get_item(
-            self.vertical_x1a.location,
+            self.vertical_x1a,
             revision=ModuleStoreEnum.RevisionOption.published_only
         )
         self.assertEqual(vertical_children_num, len(published_parent.children))
 
-        self.store.revert_to_published(self.vertical_x1a.location, self.user_id)
-        reverted_parent = self.store.get_item(self.vertical_x1a.location)
+        self.store.revert_to_published(self.vertical_x1a, self.user_id)
+        reverted_parent = self.store.get_item(self.vertical_x1a)
         self.assertEqual(vertical_children_num, len(published_parent.children))
         self.assertEqual(reverted_parent, published_parent)
 
@@ -545,14 +544,15 @@ class TestMixedModuleStore(unittest.TestCase):
         self._create_block_hierarchy()
         self.store.publish(self.course.location, self.user_id)
 
-        orig_display_name = self.problem_x1a_1.display_name
+        problem = self.store.get_item(self.problem_x1a_1)
+        orig_display_name = problem.display_name
 
         # Change display name of problem and update just it (so parent remains published)
-        self.problem_x1a_1.display_name = "updated before calling revert"
-        self.store.update_item(self.problem_x1a_1, self.user_id)
-        self.store.revert_to_published(self.vertical_x1a.location, self.user_id)
+        problem.display_name = "updated before calling revert"
+        self.store.update_item(problem, self.user_id)
+        self.store.revert_to_published(self.vertical_x1a, self.user_id)
 
-        reverted_problem = self.store.get_item(self.problem_x1a_1.location)
+        reverted_problem = self.store.get_item(self.problem_x1a_1)
         self.assertEqual(orig_display_name, reverted_problem.display_name)
 
     @ddt.data('draft')
@@ -564,9 +564,9 @@ class TestMixedModuleStore(unittest.TestCase):
         self._create_block_hierarchy()
         self.store.publish(self.course.location, self.user_id)
 
-        orig_vertical = self.vertical_x1a
-        self.store.revert_to_published(self.vertical_x1a.location, self.user_id)
-        reverted_vertical = self.store.get_item(self.vertical_x1a.location)
+        orig_vertical = self.store.get_item(self.vertical_x1a)
+        self.store.revert_to_published(self.vertical_x1a, self.user_id)
+        reverted_vertical = self.store.get_item(self.vertical_x1a)
         self.assertEqual(orig_vertical, reverted_vertical)
 
     @ddt.data('draft')
@@ -577,7 +577,7 @@ class TestMixedModuleStore(unittest.TestCase):
         self.initdb(default_ms)
         self._create_block_hierarchy()
         with self.assertRaises(InvalidVersionError):
-            self.store.revert_to_published(self.vertical_x1a.location, self.user_id)
+            self.store.revert_to_published(self.vertical_x1a, self.user_id)
 
     @ddt.data('draft')
     def test_revert_to_published_direct_only(self, default_ms):
@@ -586,8 +586,8 @@ class TestMixedModuleStore(unittest.TestCase):
         """
         self.initdb(default_ms)
         self._create_block_hierarchy()
-        self.store.revert_to_published(self.sequential_x1.location, self.user_id)
-        reverted_parent = self.store.get_item(self.sequential_x1.location)
+        self.store.revert_to_published(self.sequential_x1, self.user_id)
+        reverted_parent = self.store.get_item(self.sequential_x1)
         # It does not discard the child vertical, even though that child is a draft (with no published version)
         self.assertEqual(1, len(reverted_parent.children))
 
@@ -596,6 +596,9 @@ class TestMixedModuleStore(unittest.TestCase):
         self.initdb(default_ms)
         course_id = self.course_locations[self.MONGO_COURSEID].course_key
 
+        # create parented children
+        self._create_block_hierarchy()
+
         # orphans
         orphan_locations = [
             course_id.make_usage_key('chapter', 'OrphanChapter'),
@@ -612,7 +615,12 @@ class TestMixedModuleStore(unittest.TestCase):
         ]
 
         for location in (orphan_locations + detached_locations):
-            self.store.create_item(self.user_id, location)
+            self.store.create_item(
+                self.user_id,
+                location.course_key,
+                location.block_type,
+                block_id=location.block_id
+            )
 
         found_orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID].course_key)
         self.assertEqual(set(found_orphans), set(orphan_locations))
@@ -624,9 +632,11 @@ class TestMixedModuleStore(unittest.TestCase):
         new location for the child
         """
         self.initdb(default_ms)
-        self.store.create_item(
-            self.user_id, parent_location=self.course_locations[self.MONGO_COURSEID],
-            category='problem', block_id='orphan'
+        self.store.create_child(
+            self.user_id,
+            self.course_locations[self.MONGO_COURSEID],
+            'problem',
+            block_id='orphan'
         )
         orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID].course_key)
         self.assertEqual(len(orphans), 0, "unexpected orphans: {}".format(orphans))
@@ -655,32 +665,80 @@ class TestMixedModuleStore(unittest.TestCase):
         """
         self.initdb(default_ms)
         self._create_block_hierarchy()
+
         # publish
         self.store.publish(self.course.location, self.user_id)
         published_xblock = self.store.get_item(
-            self.vertical_x1a.location.replace(branch=None),
+            self.vertical_x1a,
             revision=ModuleStoreEnum.RevisionOption.published_only
         )
         self.assertIsNotNone(published_xblock)
 
         # unpublish
-        self.store.unpublish(self.vertical_x1a.location, self.user_id)
+        self.store.unpublish(self.vertical_x1a, self.user_id)
 
         with self.assertRaises(ItemNotFoundError):
             self.store.get_item(
-                self.vertical_x1a.location.replace(branch=None),
+                self.vertical_x1a,
                 revision=ModuleStoreEnum.RevisionOption.published_only
             )
 
-        draft_xblock_from_get_item = self.store.get_item(
-            self.vertical_x1a.location.replace(branch=None),
+        # make sure draft version still exists
+        draft_xblock = self.store.get_item(
+            self.vertical_x1a,
             revision=ModuleStoreEnum.RevisionOption.draft_only
         )
+        self.assertIsNotNone(draft_xblock)
+
+    @ddt.data('draft', 'split')
+    def test_compute_publish_state(self, default_ms):
+        """
+        Test the compute_publish_state method
+        """
+        self.initdb(default_ms)
+        self._create_block_hierarchy()
+
+        # TODO - Remove this call to explicitly Publish the course once LMS-2869 is implemented
+        # For now, we need this since we can't publish a child item without its course already been published
+        course_location = self.course_locations[self.MONGO_COURSEID]
+        self.store.publish(course_location, self.user_id)
+
+        # start off as Private
+        item = self.store.create_child(self.user_id, self.writable_chapter_location, 'problem', 'test_compute_publish_state')
+        item_location = item.location.version_agnostic()
+        self.assertEquals(self.store.compute_publish_state(item), PublishState.private)
+
+        # Private -> Public
+        self.store.publish(item_location, self.user_id)
+        item = self.store.get_item(item_location)
+        self.assertEquals(self.store.compute_publish_state(item), PublishState.public)
+
+        # Public -> Private
+        self.store.unpublish(item_location, self.user_id)
+        item = self.store.get_item(item_location)
+        self.assertEquals(self.store.compute_publish_state(item), PublishState.private)
+
+        # Private -> Public
+        self.store.publish(item_location, self.user_id)
+        item = self.store.get_item(item_location)
+        self.assertEquals(self.store.compute_publish_state(item), PublishState.public)
+
+        # Public -> Draft with NO changes
+        # Note: This is where Split and Mongo differ
+        self.store.convert_to_draft(item_location, self.user_id)
+        item = self.store.get_item(item_location)
         self.assertEquals(
-            published_xblock.location.version_agnostic().replace(branch=None),
-            draft_xblock_from_get_item.location.version_agnostic().replace(branch=None)
+            self.store.compute_publish_state(item),
+            PublishState.draft if default_ms == 'draft' else PublishState.public
         )
 
+        # Draft WITH changes
+        item.display_name = 'new name'
+        item = self.store.update_item(item, self.user_id)
+        self.assertTrue(self.store.has_changes(item.location))
+        self.assertEquals(self.store.compute_publish_state(item), PublishState.draft)
+
+
 #=============================================================================================================
 # General utils for not using django settings
 #=============================================================================================================

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

@@ -391,12 +391,27 @@ class TestMongoModuleStore(unittest.TestCase):
             course = self.draft_store.get_course(course_key)
             # can't use item factory as it depends on django settings
             p1ele = self.draft_store.create_item(
-                99, course.id.make_usage_key('problem', 'p1'), runtime=course.runtime)
+                99,
+                course_key,
+                'problem',
+                block_id='p1',
+                runtime=course.runtime
+            )
             p2ele = self.draft_store.create_item(
-                99, course.id.make_usage_key('problem', 'p2'), runtime=course.runtime)
+                99,
+                course_key,
+                'problem',
+                block_id='p2',
+                runtime=course.runtime
+            )
             self.refloc = course.id.make_usage_key('ref_test', 'ref_test')
             self.draft_store.create_item(
-                99,self.refloc,  runtime=course.runtime, fields={
+                99,
+                self.refloc.course_key,
+                self.refloc.block_type,
+                block_id=self.refloc.block_id,
+                runtime=course.runtime,
+                fields={
                     'reference_link': p1ele.location,
                     'reference_list': [p1ele.location, p2ele.location],
                     'reference_dict': {'p1': p1ele.location, 'p2': p2ele.location},
@@ -497,12 +512,16 @@ class TestMongoModuleStore(unittest.TestCase):
         """
         Tests that has_changes() returns false when a new xblock in a direct only category is checked
         """
-        course_location = Location('edx', 'direct', '2012_Fall', 'course', 'test_course')
-        chapter_location = Location('edx', 'direct', '2012_Fall', 'chapter', 'test_chapter')
+        course_location = Location('edX', 'toy', '2012_Fall', 'course', '2012_Fall')
+        chapter_location = Location('edX', 'toy', '2012_Fall', 'chapter', 'vertical_container')
 
         # Create dummy direct only xblocks
-        self.draft_store.create_item(self.dummy_user, course_location)
-        self.draft_store.create_item(self.dummy_user, chapter_location)
+        self.draft_store.create_item(
+            self.dummy_user,
+            chapter_location.course_key,
+            chapter_location.block_type,
+            block_id=chapter_location.block_id
+        )
 
         # Check that neither xblock has changes
         self.assertFalse(self.draft_store.has_changes(course_location))
@@ -512,10 +531,15 @@ class TestMongoModuleStore(unittest.TestCase):
         """
         Tests that has_changes() only returns true when changes are present
         """
-        location = Location('edX', 'changes', '2012_Fall', 'vertical', 'test_vertical')
+        location = Location('edX', 'toy', '2012_Fall', 'vertical', 'test_vertical')
 
         # Create a dummy component to test against
-        self.draft_store.create_item(self.dummy_user, location)
+        self.draft_store.create_item(
+            self.dummy_user,
+            location.course_key,
+            location.block_type,
+            block_id=location.block_id
+        )
 
         # Not yet published, so changes are present
         self.assertTrue(self.draft_store.has_changes(location))
@@ -538,11 +562,16 @@ class TestMongoModuleStore(unittest.TestCase):
         """
         Tests that has_changes() returns False when a published parent points to a child that doesn't exist.
         """
-        location = Location('edX', 'missing', '2012_Fall', 'sequential', 'parent')
+        location = Location('edX', 'toy', '2012_Fall', 'sequential', 'parent')
 
         # Create the parent and point it to a fake child
-        parent = self.draft_store.create_item(self.dummy_user, location)
-        parent.children += [Location('edX', 'missing', '2012_Fall', 'vertical', 'does_not_exist')]
+        parent = self.draft_store.create_item(
+            self.dummy_user,
+            location.course_key,
+            location.block_type,
+            block_id=location.block_id
+        )
+        parent.children += [Location('edX', 'toy', '2012_Fall', 'vertical', 'does_not_exist')]
         self.draft_store.update_item(parent, self.dummy_user)
 
         # Check the parent for changes should return False and not throw an exception
@@ -561,27 +590,39 @@ class TestMongoModuleStore(unittest.TestCase):
         if user_id is None:
             user_id = self.dummy_user
 
-        locations = {
-            'grandparent': Location('edX', 'tree', name, 'chapter', 'grandparent'),
-            'parent_sibling': Location('edX', 'tree', name, 'sequential', 'parent_sibling'),
-            'parent': Location('edX', 'tree', name, 'sequential', 'parent'),
-            'child_sibling': Location('edX', 'tree', name, 'vertical', 'child_sibling'),
-            'child': Location('edX', 'tree', name, 'vertical', 'child'),
-        }
+        org = 'edX'
+        course = 'tree{}'.format(name)
+        run = name
 
-        for key in locations:
-            self.draft_store.create_item(user_id, locations[key])
+        if not self.draft_store.has_course(SlashSeparatedCourseKey(org, course, run)):
+            self.draft_store.create_course(org, course, run, user_id)
 
-        grandparent = self.draft_store.get_item(locations['grandparent'])
-        grandparent.children += [locations['parent_sibling'], locations['parent']]
-        self.draft_store.update_item(grandparent, user_id=user_id)
+            locations = {
+                'grandparent': Location(org, course, run, 'chapter', 'grandparent'),
+                'parent_sibling': Location(org, course, run, 'sequential', 'parent_sibling'),
+                'parent': Location(org, course, run, 'sequential', 'parent'),
+                'child_sibling': Location(org, course, run, 'vertical', 'child_sibling'),
+                'child': Location(org, course, run, 'vertical', 'child'),
+            }
 
-        parent = self.draft_store.get_item(locations['parent'])
-        parent.children += [locations['child_sibling'], locations['child']]
-        self.draft_store.update_item(parent, user_id=user_id)
+            for key in locations:
+                self.draft_store.create_item(
+                    user_id,
+                    locations[key].course_key,
+                    locations[key].block_type,
+                    block_id=locations[key].block_id
+                )
+
+            grandparent = self.draft_store.get_item(locations['grandparent'])
+            grandparent.children += [locations['parent_sibling'], locations['parent']]
+            self.draft_store.update_item(grandparent, user_id=user_id)
 
-        self.draft_store.publish(locations['parent'], user_id)
-        self.draft_store.publish(locations['parent_sibling'], user_id)
+            parent = self.draft_store.get_item(locations['parent'])
+            parent.children += [locations['child_sibling'], locations['child']]
+            self.draft_store.update_item(parent, user_id=user_id)
+
+            self.draft_store.publish(locations['parent'], user_id)
+            self.draft_store.publish(locations['parent_sibling'], user_id)
 
         return locations
 
@@ -663,10 +704,12 @@ class TestMongoModuleStore(unittest.TestCase):
 
         # Create a new child and attach it to parent
         new_child_location = Location('edX', 'tree', 'has_changes_add_remove_child', 'vertical', 'new_child')
-        self.draft_store.create_item(self.dummy_user, new_child_location)
-        parent = self.draft_store.get_item(locations['parent'])
-        parent.children += [new_child_location]
-        self.draft_store.update_item(parent, user_id=self.dummy_user)
+        self.draft_store.create_child(
+            self.dummy_user,
+            locations['parent'],
+            new_child_location.block_type,
+            block_id=new_child_location.block_id
+        )
 
         # Verify that the ancestors now have changes
         self.assertTrue(self.draft_store.has_changes(locations['grandparent']))
@@ -685,13 +728,21 @@ class TestMongoModuleStore(unittest.TestCase):
         """
         Tests that has_changes() returns true after editing the child of a vertical (both not direct only categories).
         """
-        parent_location = Location('edX', 'test', 'non_direct_only_children', 'vertical', 'parent')
-        child_location = Location('edX', 'test', 'non_direct_only_children', 'html', 'child')
+        parent_location = Location('edX', 'toy', '2012_Fall', 'vertical', 'parent')
+        child_location = Location('edX', 'toy', '2012_Fall', 'html', 'child')
 
-        parent = self.draft_store.create_item(self.dummy_user, parent_location)
-        child = self.draft_store.create_item(self.dummy_user, child_location)
-        parent.children += [child_location]
-        self.draft_store.update_item(parent, user_id=self.dummy_user)
+        parent = self.draft_store.create_item(
+            self.dummy_user,
+            parent_location.course_key,
+            parent_location.block_type,
+            block_id=parent_location.block_id
+        )
+        child = self.draft_store.create_child(
+            self.dummy_user,
+            parent_location,
+            child_location.block_type,
+            block_id=child_location.block_id
+        )
         self.draft_store.publish(parent_location, self.dummy_user)
 
         # Verify that there are no changes
@@ -757,10 +808,15 @@ class TestMongoModuleStore(unittest.TestCase):
         """
         Tests that edited_on and edited_by are set correctly during an update
         """
-        location = Location('edX', 'editInfoTest', '2012_Fall', 'html', 'test_html')
+        location = Location('edX', 'toy', '2012_Fall', 'html', 'test_html')
 
         # Create a dummy component to test against
-        self.draft_store.create_item(self.dummy_user, location)
+        self.draft_store.create_item(
+            self.dummy_user,
+            location.course_key,
+            location.block_type,
+            block_id=location.block_id
+        )
 
         # Store the current edit time and verify that dummy_user created the component
         component = self.draft_store.get_item(location)
@@ -780,12 +836,17 @@ class TestMongoModuleStore(unittest.TestCase):
         """
         Tests that published_date and published_by are set correctly
         """
-        location = Location('edX', 'publishInfo', '2012_Fall', 'html', 'test_html')
+        location = Location('edX', 'toy', '2012_Fall', 'html', 'test_html')
         create_user = 123
         publish_user = 456
 
         # Create a dummy component to test against
-        self.draft_store.create_item(create_user, location)
+        self.draft_store.create_item(
+            create_user,
+            location.course_key,
+            location.block_type,
+            block_id=location.block_id
+        )
 
         # Store the current time, then publish
         old_time = datetime.now(UTC)

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

@@ -19,7 +19,7 @@ class TestPublish(SplitWMongoCourseBoostrapper):
         # There are 12 created items and 7 parent updates
         # create course: finds: 1 to verify uniqueness, 1 to find parents
         # sends: 1 to create course, 1 to create overview
-        with check_mongo_calls(self.draft_mongo, 5, 2):
+        with check_mongo_calls(self.draft_mongo, 6, 2):
             super(TestPublish, self)._create_course(split=False)  # 2 inserts (course and overview)
 
         # with bulk will delay all inheritance computations which won't be added into the mongo_calls

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

@@ -30,8 +30,7 @@ class TestDraftVersioningModuleStore(unittest.TestCase):
             render_template=render_to_template_mock,
             xblock_mixins=(InheritanceMixin, XModuleMixin),
         )
-        # NAATODO - uncomment once merged with drop_database PR
-        # self.addCleanup(module_store._drop_database)
+        self.addCleanup(self.module_store._drop_database)
 
         SplitModuleTest.bootstrapDB(self.module_store)
 

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

@@ -444,7 +444,7 @@ class SplitModuleTest(unittest.TestCase):
             }
         },
     }
-    
+
     @staticmethod
     def bootstrapDB(split_store):
         '''
@@ -985,7 +985,7 @@ class TestItemCrud(SplitModuleTest):
         # add minimal one w/o a parent
         category = 'sequential'
         new_module = modulestore().create_item(
-            'user123', parent_location=locator, category=category,
+            'user123', locator, category,
             fields={'display_name': 'new sequential'}
         )
         # check that course version changed and course's previous is the other one
@@ -1025,8 +1025,8 @@ class TestItemCrud(SplitModuleTest):
         )
         premod_course = modulestore().get_course(locator.course_key)
         category = 'chapter'
-        new_module = modulestore().create_item(
-            'user123', parent_location=locator, category=category,
+        new_module = modulestore().create_child(
+            'user123', locator, category,
             fields={'display_name': 'new chapter'},
             definition_locator=original.definition_locator
         )
@@ -1054,13 +1054,13 @@ class TestItemCrud(SplitModuleTest):
         )
         category = 'problem'
         new_payload = "<problem>empty</problem>"
-        new_module = modulestore().create_item(
-            'anotheruser', parent_location=locator, category=category,
+        new_module = modulestore().create_child(
+            'anotheruser', locator, category,
             fields={'display_name': 'problem 1', 'data': new_payload},
         )
         another_payload = "<problem>not empty</problem>"
-        another_module = modulestore().create_item(
-            'anotheruser', parent_location=locator, category=category,
+        another_module = modulestore().create_child(
+            'anotheruser', locator, category,
             fields={'display_name': 'problem 2', 'data': another_payload},
             definition_locator=original.definition_locator,
         )
@@ -1086,8 +1086,8 @@ class TestItemCrud(SplitModuleTest):
         course_key = CourseLocator(org='guestx', course='contender', run="run", branch=BRANCH_NAME_DRAFT)
         parent_locator = BlockUsageLocator(course_key, 'course', block_id="head345679")
         chapter_locator = BlockUsageLocator(course_key, 'chapter', block_id="foo.bar_-~:0")
-        modulestore().create_item(
-            'anotheruser', parent_location=parent_locator, category='chapter',
+        modulestore().create_child(
+            'anotheruser', parent_locator, 'chapter',
             block_id=chapter_locator.block_id,
             fields={'display_name': 'chapter 99'},
         )
@@ -1097,8 +1097,8 @@ class TestItemCrud(SplitModuleTest):
         # now try making that a parent of something
         new_payload = "<problem>empty</problem>"
         problem_locator = BlockUsageLocator(course_key, 'problem', block_id="prob.bar_-~:99a")
-        modulestore().create_item(
-            'anotheruser', parent_location=chapter_locator, category='problem',
+        modulestore().create_child(
+            'anotheruser', chapter_locator, 'problem',
             block_id=problem_locator.block_id,
             fields={'display_name': 'chapter 99', 'data': new_payload},
         )
@@ -1123,8 +1123,8 @@ class TestItemCrud(SplitModuleTest):
         versionless_course_locator = new_course_locator.version_agnostic()
 
         # positive simple case: no force, add chapter
-        new_ele = modulestore().create_item(
-            user, parent_location=new_course.location, category='chapter',
+        new_ele = modulestore().create_child(
+            user, new_course.location, 'chapter',
             fields={'display_name': 'chapter 1'},
             continue_version=True
         )
@@ -1141,40 +1141,40 @@ class TestItemCrud(SplitModuleTest):
 
         # try to create existing item
         with self.assertRaises(DuplicateItemError):
-            _fail = modulestore().create_item(
-                user, parent_location=new_course.location, category='chapter',
+            _fail = modulestore().create_child(
+                user, new_course.location, 'chapter',
                 block_id=new_ele.location.block_id,
                 fields={'display_name': 'chapter 2'},
                 continue_version=True
             )
 
         # start a new transaction
-        new_ele = modulestore().create_item(
-            user, parent_location=new_course.location, category='chapter',
+        new_ele = modulestore().create_child(
+            user, new_course.location, 'chapter',
             fields={'display_name': 'chapter 2'},
             continue_version=False
         )
         transaction_guid = new_ele.location.version_guid
         # ensure force w/ continue gives exception
         with self.assertRaises(VersionConflictError):
-            _fail = modulestore().create_item(
-                user, parent_location=new_course.location, category='chapter',
+            _fail = modulestore().create_child(
+                user, new_course.location, 'chapter',
                 fields={'display_name': 'chapter 2'},
                 force=True, continue_version=True
             )
 
         # ensure trying to continue the old one gives exception
         with self.assertRaises(VersionConflictError):
-            _fail = modulestore().create_item(
-                user, parent_location=new_course.location, category='chapter',
+            _fail = modulestore().create_child(
+                user, new_course.location, 'chapter',
                 fields={'display_name': 'chapter 3'},
                 continue_version=True
             )
 
         # add new child to old parent in continued (leave off version_guid)
         course_module_locator = new_course.location.version_agnostic()
-        new_ele = modulestore().create_item(
-            user, parent_location=course_module_locator, category='chapter',
+        new_ele = modulestore().create_child(
+            user, course_module_locator, 'chapter',
             fields={'display_name': 'chapter 4'},
             continue_version=True
         )
@@ -1283,13 +1283,13 @@ class TestItemCrud(SplitModuleTest):
         )
         category = 'problem'
         new_payload = "<problem>empty</problem>"
-        modulestore().create_item(
-            'test_update_manifold', parent_location=locator, category=category,
+        modulestore().create_child(
+            'test_update_manifold', locator, category,
             fields={'display_name': 'problem 1', 'data': new_payload},
         )
         another_payload = "<problem>not empty</problem>"
-        modulestore().create_item(
-            'test_update_manifold', parent_location=locator, category=category,
+        modulestore().create_child(
+            'test_update_manifold', locator, category,
             fields={'display_name': 'problem 2', 'data': another_payload},
             definition_locator=original.definition_locator,
         )
@@ -1368,8 +1368,8 @@ class TestItemCrud(SplitModuleTest):
         """
         if not category_queue:
             return
-        node = modulestore().create_item(
-            'deleting_user', parent_location=parent.version_agnostic(), category=category_queue[0]
+        node = modulestore().create_child(
+            'deleting_user', parent.version_agnostic(), category_queue[0]
         )
         node_loc = node.location.map_into_course(parent.course_key)
         for _ in range(4):
@@ -1433,8 +1433,8 @@ class TestCourseCreation(SplitModuleTest):
 
         # changing this course will not change the original course
         # using new_draft.location will insert the chapter under the course root
-        new_item = modulestore().create_item(
-            'leech_master', parent_location=new_draft.location, category='chapter',
+        new_item = modulestore().create_child(
+            'leech_master', new_draft.location, 'chapter',
             fields={'display_name': 'new chapter'}
         )
         new_draft_locator = new_draft_locator.course_key.version_agnostic()
@@ -1595,8 +1595,8 @@ class TestPublish(SplitModuleTest):
             source_course, dest_course, expected, [chapter2.block_id, chapter3.block_id, "problem1", "problem3_2"]
         )
         # add a child under chapter1
-        new_module = modulestore().create_item(
-            self.user_id, parent_location=chapter1, category="sequential",
+        new_module = modulestore().create_child(
+            self.user_id, chapter1, "sequential",
             fields={'display_name': 'new sequential'},
         )
         # remove chapter1 from expected b/c its pub'd version != the source anymore since source changed
@@ -1614,7 +1614,7 @@ class TestPublish(SplitModuleTest):
         )
         # ensure intentionally orphaned blocks work (e.g., course_info)
         new_module = modulestore().create_item(
-            self.user_id, parent_location=source_course, category="course_info", block_id="handouts"
+            self.user_id, source_course, "course_info", block_id="handouts"
         )
         # publish it
         modulestore().copy(self.user_id, source_course, dest_course, [new_module.location], None)

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

@@ -87,7 +87,13 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase):
         """
         location = self.old_course_key.make_usage_key(category, name)
         self.draft_mongo.create_item(
-            self.user_id, location, definition_data=data, metadata=metadata, runtime=self.runtime
+            self.user_id,
+            location.course_key,
+            location.block_type,
+            block_id=location.block_id,
+            definition_data=data,
+            metadata=metadata,
+            runtime=self.runtime
         )
         if not draft:
             self.draft_mongo.publish(location, self.user_id)
@@ -104,16 +110,28 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase):
             self.draft_mongo.update_item(parent, self.user_id)
             if not draft:
                 self.draft_mongo.publish(parent_location, self.user_id)
-            # create pointer for split
-            course_or_parent_locator = BlockUsageLocator(
-                course_key=self.split_course_key,
-                block_type=parent_category,
-                block_id=parent_name
-            )
+            # create child for split
+            if split:
+                self.split_mongo.create_child(
+                    self.user_id,
+                    BlockUsageLocator(
+                        course_key=self.split_course_key,
+                        block_type=parent_category,
+                        block_id=parent_name
+                    ),
+                    category,
+                    block_id=name,
+                    fields=fields
+                )
         else:
-            course_or_parent_locator = self.split_course_key
-        if split:
-            self.split_mongo.create_item(course_or_parent_locator, category, self.user_id, block_id=name, fields=fields)
+            if split:
+                self.split_mongo.create_item(
+                    self.user_id,
+                    self.split_course_key,
+                    category,
+                    block_id=name,
+                    fields=fields
+                )
 
     def _create_course(self, split=True):
         """

common/lib/xmodule/xmodule/split_test_module.py

@@ -578,7 +578,9 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor, StudioEditableDes
         metadata = {'display_name': group.name}
         modulestore.create_item(
             user_id,
-            dest_usage_key,
+            self.location.course_key,
+            dest_usage_key.block_type,
+            block_id=dest_usage_key.block_id,
             definition_data=None,
             metadata=metadata,
             runtime=self.system,