Merge pull request #4381 from edx/nimisha/split-converge-api

Nimisha/split converge api

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_and_save_xmodule(location, user_id)
+        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_and_save_xmodule(location, user.id)
+        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_and_save_xmodule(location, self.user.id)
+        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_and_save_xmodule(new_component_location, self.user.id)
-
-        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,16 +187,16 @@ 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)
-        updated_loc = 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',
             parent_location=BlockUsageLocator.make_relative(
-                updated_loc, block_type='problem', block_id=sub.location.block_id
+                test_course.location.version_agnostic(), block_type='problem', block_id=sub.location.block_id
             ),
             user_id='testbot', category='problem',
             data="<problem></problem>"

cms/djangoapps/contentstore/tests/test_orphan.py

@@ -33,8 +33,14 @@ class TestOrphan(CourseTestCase):
     def _create_item(self, category, name, data, metadata, parent_category, parent_name, runtime):
         location = self.course.location.replace(category=category, name=name)
         store = modulestore()
-        store.create_and_save_xmodule(
-            location, self.user.id, definition_data=data, metadata=metadata, runtime=runtime
+        store.create_item(
+            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_and_save_xmodule(course_id.make_usage_key('vertical', self.PRIVATE_VERTICAL), self.user.id)
+        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_and_save_xmodule(course_id.make_usage_key('vertical', self.PUBLISHED_VERTICAL), self.user.id)
+        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_and_save_xmodule(usage_key, user.id)
+            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_and_save_xmodule(
-        dest_usage_key,
+    created_block = store.create_child(
         request.user.id,
+        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)})
 
 
@@ -465,11 +462,11 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_
         else:
             duplicate_metadata['display_name'] = _("Duplicate of '{0}'").format(source_item.display_name)
 
-    dest_module = store.create_and_save_xmodule(
-        dest_usage_key,
-
-
+    dest_module = store.create_item(
         user.id,
+        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,
@@ -531,7 +528,7 @@ def orphan_handler(request, course_key_string):
     course_usage_key = CourseKey.from_string(course_key_string)
     if request.method == 'GET':
         if has_course_access(request.user, course_usage_key):
-            return JsonResponse(modulestore().get_orphans(course_usage_key))
+            return JsonResponse([unicode(item) for item in modulestore().get_orphans(course_usage_key)])
         else:
             raise PermissionDenied()
     if request.method == 'DELETE':
@@ -539,11 +536,9 @@ def orphan_handler(request, course_key_string):
             store = modulestore()
             items = store.get_orphans(course_usage_key)
             for itemloc in items:
-                # get_orphans returns the deprecated string format w/o revision
-                usage_key = course_usage_key.make_usage_key_from_deprecated_string(itemloc)
                 # need to delete all versions
-                store.delete_item(usage_key, request.user.id, revision=ModuleStoreEnum.RevisionOption.all)
-            return JsonResponse({'deleted': items})
+                store.delete_item(itemloc, request.user.id, revision=ModuleStoreEnum.RevisionOption.all)
+            return JsonResponse({'deleted': [unicode(item) for item in items]})
         else:
             raise PermissionDenied()
 
@@ -559,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_and_save_xmodule(usage_key, user.id)
+            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_and_save_xmodule(location, self.user.id)
+        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_and_save_xmodule(location, self.user.id)
+        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)

cms/envs/test.py

@@ -63,7 +63,7 @@ STATICFILES_DIRS += [
 MODULESTORE['default']['OPTIONS']['stores'].append(
     {
         'NAME': 'split',
-        'ENGINE': 'xmodule.modulestore.split_mongo.split.SplitMongoModuleStore',
+        'ENGINE': 'xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore',
         'DOC_STORE_CONFIG': DOC_STORE_CONFIG,
         'OPTIONS': {
             'render_template': 'edxmako.shortcuts.render_to_string',

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

@@ -7,6 +7,7 @@ import logging
 import re
 import json
 import datetime
+from uuid import uuid4
 
 from collections import namedtuple, defaultdict
 import collections
@@ -367,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
@@ -560,50 +580,6 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite):
             result[field.scope][field_name] = value
         return result
 
-    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 create_and_save_xmodule(self, location, user_id, definition_data=None, metadata=None, runtime=None, fields={}):
-        """
-        Create the new xmodule and save it.
-
-        :param location: a Location--must have a category
-        :param user_id: ID of the user creating and saving the xmodule
-        :param definition_data: can be empty. The initial definition_data for the kvs
-        :param metadata: can be empty, the initial metadata for the kvs
-        :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
-        """
-        new_object = self.create_xmodule(location, definition_data, metadata, runtime, fields)
-        self.update_item(new_object, user_id, allow_not_found=True)
-        return new_object
-
     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
@@ -634,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):
         """

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

+"""
+This module provides an abstraction for Module Stores that support Draft and Published branches.
+"""
+
+from abc import ABCMeta, abstractmethod
+from . import ModuleStoreEnum
+
+class ModuleStoreDraftAndPublished(object):
+    """
+    A mixin for a read-write database backend that supports two branches, Draft and Published, with
+    options to prefer Draft and fallback to Published.
+    """
+    __metaclass__ = ABCMeta
+
+    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
+    def delete_item(self, location, user_id, revision=None, **kwargs):
+        raise NotImplementedError
+
+    @abstractmethod
+    def get_parent_location(self, location, revision=None, **kwargs):
+        raise NotImplementedError
+
+    @abstractmethod
+    def has_changes(self, usage_key):
+        raise NotImplementedError
+
+    @abstractmethod
+    def publish(self, location, user_id):
+        raise NotImplementedError
+
+    @abstractmethod
+    def unpublish(self, location, user_id):
+        raise NotImplementedError
+
+    @abstractmethod
+    def revert_to_published(self, location, user_id):
+        raise NotImplementedError
+
+    @abstractmethod
+    def compute_publish_state(self, xblock):
+        raise NotImplementedError
+
+    @abstractmethod
+    def convert_to_draft(self, location, user_id):
+        raise NotImplementedError

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
@@ -34,6 +35,7 @@ from xblock.exceptions import InvalidScopeError
 from xblock.fields import Scope, ScopeIds, Reference, ReferenceList, ReferenceValueDict
 
 from xmodule.modulestore import ModuleStoreWriteBase, ModuleStoreEnum
+from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished
 from opaque_keys.edx.locations import Location
 from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError, ReferentialIntegrityError
 from xmodule.modulestore.inheritance import own_metadata, InheritanceMixin, inherit_metadata, InheritanceKeyValueStore
@@ -335,7 +337,7 @@ def as_published(location):
     return location.replace(revision=MongoRevisionKey.published)
 
 
-class MongoModuleStore(ModuleStoreWriteBase):
+class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
     """
     A Mongodb backed ModuleStore
     """
@@ -353,7 +355,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
         :param doc_store_config: must have a host, db, and collection entries. Other common entries: port, tz_aware.
         """
 
-        super(MongoModuleStore, self).__init__(contentstore, **kwargs)
+        super(MongoModuleStore, self).__init__(contentstore=contentstore, **kwargs)
 
         def do_connection(
             db, collection, host, port=27017, tz_aware=True, user=None, password=None, **kwargs
@@ -903,7 +905,12 @@ class MongoModuleStore(ModuleStoreWriteBase):
                 ]))
 
         location = course_id.make_usage_key('course', course_id.run)
-        course = self.create_and_save_xmodule(location, user_id, 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(
@@ -911,16 +918,18 @@ class MongoModuleStore(ModuleStoreWriteBase):
             name='overview'
         )
         overview_template = AboutDescriptor.get_template('overview.yaml')
-        self.create_and_save_xmodule(
-            about_location,
+        self.create_item(
             user_id,
+            about_location.course_key,
+            about_location.block_type,
+            block_id=about_location.block_id,
             definition_data=overview_template.get('data'),
             runtime=course.system
         )
 
         return course
 
-    def create_xmodule(self, location, definition_data=None, metadata=None, runtime=None, fields={}):
+    def create_xmodule(self, location, definition_data=None, metadata=None, runtime=None, fields={}, **kwargs):
         """
         Create the new xmodule but don't save it. Returns the new module.
 
@@ -974,6 +983,52 @@ class MongoModuleStore(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
@@ -1064,6 +1119,9 @@ class MongoModuleStore(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
 
@@ -1161,7 +1219,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
 
     def get_orphans(self, course_key):
         """
-        Return an array of all of the locations (deprecated string format) for orphans in the course.
+        Return an array of all of the locations for orphans in the course.
         """
         course_key = self.fill_in_run(course_key)
         detached_categories = [name for name, __ in XBlock.load_tagged_classes("detached")]
@@ -1178,7 +1236,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
                 )
             all_reachable = all_reachable.union(item.get('definition', {}).get('children', []))
         item_locs -= all_reachable
-        return list(item_locs)
+        return [course_key.make_usage_key_from_deprecated_string(item_loc) for item_loc in item_locs]
 
     def get_courses_for_wiki(self, wiki_slug):
         """

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

@@ -54,7 +54,6 @@ class DraftModuleStore(MongoModuleStore):
                 This should be an attribute from ModuleStoreEnum.Branch
         """
         super(DraftModuleStore, self).__init__(*args, **kwargs)
-        self.branch_setting_func = kwargs.pop('branch_setting_func', lambda: ModuleStoreEnum.Branch.published_only)
 
     def get_item(self, usage_key, depth=0, revision=None):
         """
@@ -287,7 +286,7 @@ class DraftModuleStore(MongoModuleStore):
                 else ModuleStoreEnum.RevisionOption.draft_preferred
         return super(DraftModuleStore, self).get_parent_location(location, revision, **kwargs)
 
-    def create_xmodule(self, location, definition_data=None, metadata=None, runtime=None, fields={}):
+    def create_xmodule(self, location, definition_data=None, metadata=None, runtime=None, fields={}, **kwargs):
         """
         Create the new xmodule but don't save it. Returns the new module with a draft locator if
         the category allows drafts. If the category does not allow drafts, just creates a published module.
@@ -483,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)
@@ -527,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

@@ -84,12 +84,13 @@ class SplitMigrator(object):
                 # it doesn't need the parent as the first arg. That is, it translates and populates
                 # the 'children' field as it goes.
                 _new_module = self.split_modulestore.create_item(
-                    course_version_locator, module.category, user_id,
+                    user_id,
+                    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
                     ),
-                    # TODO remove continue_version when bulk write is impl'd
                     continue_version=True
                 )
         # after done w/ published items, add version for DRAFT pointing to the published structure
@@ -130,7 +131,8 @@ class SplitMigrator(object):
             else:
                 # only a draft version (aka, 'private').
                 _new_module = self.split_modulestore.create_item(
-                    new_draft_course_loc, module.category, user_id,
+                    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_draft.py

+"""
+Module for the dual-branch fall-back Draft->Published Versioning ModuleStore
+"""
+
+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):
+    """
+    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 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=master_branch, **kwargs
+        )
+
+    def get_courses(self):
+        """
+        Returns all the courses on the Draft branch (which is a superset of the courses on the Published branch).
+        """
+        return super(DraftVersioningModuleStore, self).get_courses(ModuleStoreEnum.BranchName.draft)
+
+    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]
+        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 _map_revision_to_branch(self, key, revision=None):
+        """
+        Maps RevisionOptions to BranchNames, inserting them into the key
+        """
+        if revision == ModuleStoreEnum.RevisionOption.published_only:
+            return key.for_branch(ModuleStoreEnum.BranchName.published)
+        elif revision == ModuleStoreEnum.RevisionOption.draft_only:
+            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):
+        '''
+        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):
+        """
+        Checks if the given block has unpublished changes
+        :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))
+        except ItemNotFoundError:
+            return True
+
+        return draft.update_version != published.update_version
+
+    def publish(self, location, user_id, **kwargs):
+        """
+        Save a current draft to the underlying modulestore.
+        Returns the newly published item.
+        """
+        SplitMongoModuleStore.copy(
+            self,
+            user_id,
+            location.course_key.for_branch(ModuleStoreEnum.BranchName.draft),
+            location.course_key.for_branch(ModuleStoreEnum.BranchName.published),
+            [location],
+        )
+
+    def unpublish(self, location, user_id):
+        """
+        Deletes the published version of the item.
+        Returns the newly unpublished item.
+        """
+        self.delete_item(location, user_id, revision=ModuleStoreEnum.RevisionOption.published_only)
+        return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.draft))
+
+    def revert_to_published(self, location, user_id):
+        """
+        Reverts an item to its last published version (recursively traversing all of its descendants).
+        If no published version exists, a VersionConflictError is thrown.
+
+        If a published version exists but there is no draft version of this item or any of its descendants, this
+        method is a no-op.
+
+        :raises InvalidVersionError: if no published version exists for the location specified
+        """
+        raise NotImplementedError()
+
+    def compute_publish_state(self, xblock):
+        """
+        Returns whether this xblock is draft, public, or private.
+
+        Returns:
+            PublishState.draft - published exists and is different from draft
+            PublishState.public - published exists and is the same as draft
+            PublishState.private - no published version exists
+        """
+        # TODO figure out what to say if xblock is not from the HEAD of its branch
+        def get_head(branch):
+            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 == ModuleStoreEnum.BranchName.draft:
+            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 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
+        elif xblock.update_version != other['edit_info']['update_version']:
+            return PublishState.draft
+        else:
+            return PublishState.public
+
+    def convert_to_draft(self, location, user_id):
+        """
+        Create a copy of the source and mark its revision as draft.
+
+        :param source: the location of the source (its revision must be None)
+        """
+        # This is a no-op in Split since a draft version of the data always remains
+        pass

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_and_save_xmodule(location, user_id, 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
 
@@ -61,7 +62,7 @@ class ItemFactory(SplitFactory):
     # pylint: disable=W0613
     @classmethod
     def _create(cls, target_class, parent_location, category='chapter',
-                user_id=ModuleStoreEnum.UserID.test, block_id=None, definition_locator=None, force=False,
+                user_id=ModuleStoreEnum.UserID.test, definition_locator=None, force=False,
                 continue_version=False, **kwargs):
         """
         passes *kwargs* as the new item's field values:
@@ -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(
-            parent_location, category, user_id, definition_locator=definition_locator,
-            block_id=block_id, force=force, continue_version=continue_version, fields=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_orphan.py

-from xmodule.modulestore.tests.test_split_w_old_mongo import SplitWMongoCourseBoostrapper
-
-
-class TestOrphan(SplitWMongoCourseBoostrapper):
-    """
-    Test the orphan finding code
-    """
-
-    def _create_course(self):
-        """
-        * some detached items
-        * some attached children
-        * some orphans
-        """
-        super(TestOrphan, self)._create_course()
-
-        self._create_item('chapter', 'Chapter1', {}, {'display_name': 'Chapter 1'}, 'course', 'runid')
-        self._create_item('chapter', 'Chapter2', {}, {'display_name': 'Chapter 2'}, 'course', 'runid')
-        self._create_item('chapter', 'OrphanChapter', {}, {'display_name': 'Orphan Chapter'}, None, None)
-        self._create_item('vertical', 'Vert1', {}, {'display_name': 'Vertical 1'}, 'chapter', 'Chapter1')
-        self._create_item('vertical', 'OrphanVert', {}, {'display_name': 'Orphan Vertical'}, None, None)
-        self._create_item('html', 'Html1', "<p>Goodbye</p>", {'display_name': 'Parented Html'}, 'vertical', 'Vert1')
-        self._create_item('html', 'OrphanHtml', "<p>Hello</p>", {'display_name': 'Orphan html'}, None, None)
-        self._create_item('static_tab', 'staticuno', "<p>tab</p>", {'display_name': 'Tab uno'}, None, None)
-        self._create_item('about', 'overview', "<p>overview</p>", {}, None, None)
-        self._create_item('course_info', 'updates', "<ol><li><h2>Sep 22</h2><p>test</p></li></ol>", {}, None, None)
-
-    def test_mongo_orphan(self):
-        """
-        Test that old mongo finds the orphans
-        """
-        orphans = self.draft_mongo.get_orphans(self.old_course_key)
-        self.assertEqual(len(orphans), 3, "Wrong # {}".format(orphans))
-        location = self.old_course_key.make_usage_key('chapter', 'OrphanChapter')
-        self.assertIn(location.to_deprecated_string(), orphans)
-        location = self.old_course_key.make_usage_key('vertical', 'OrphanVert')
-        self.assertIn(location.to_deprecated_string(), orphans)
-        location = self.old_course_key.make_usage_key('html', 'OrphanHtml')
-        self.assertIn(location.to_deprecated_string(), orphans)
-
-    def test_split_orphan(self):
-        """
-        Test that split mongo finds the orphans
-        """
-        orphans = self.split_mongo.get_orphans(self.split_course_key)
-        self.assertEqual(len(orphans), 3, "Wrong # {}".format(orphans))
-        location = self.split_course_key.make_usage_key('chapter', 'OrphanChapter')
-        self.assertIn(location, orphans)
-        location = self.split_course_key.make_usage_key('vertical', 'OrphanVert')
-        self.assertIn(location, orphans)
-        location = self.split_course_key.make_usage_key('html', 'OrphanHtml')
-        self.assertIn(location, orphans)

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

+"""
+    Test split_draft modulestore
+"""
+import unittest
+import uuid
+from xmodule.modulestore.split_mongo.split_draft import DraftVersioningModuleStore
+from xmodule.modulestore import ModuleStoreEnum
+from opaque_keys.edx.locator import CourseLocator
+from xmodule.modulestore.inheritance import InheritanceMixin
+from xmodule.x_module import XModuleMixin
+from xmodule.modulestore.tests.test_split_modulestore import SplitModuleTest
+
+# pylint: disable=W0613
+def render_to_template_mock(*args):
+    pass
+
+
+class TestDraftVersioningModuleStore(unittest.TestCase):
+    def setUp(self):
+        super(TestDraftVersioningModuleStore, self).setUp()
+        self.module_store = DraftVersioningModuleStore(
+            contentstore=None,
+            doc_store_config={
+                'host': 'localhost',
+                'db': 'test_xmodule',
+                'collection': 'modulestore{0}'.format(uuid.uuid4().hex[:5]),
+            },
+            fs_root='',
+            default_class='xmodule.raw_module.RawDescriptor',
+            render_template=render_to_template_mock,
+            xblock_mixins=(InheritanceMixin, XModuleMixin),
+        )
+        self.addCleanup(self.module_store._drop_database)
+
+        SplitModuleTest.bootstrapDB(self.module_store)
+
+    def test_has_changes(self):
+        """
+        Tests that has_changes() only returns true when changes are present
+        """
+        draft_course = CourseLocator(
+            org='testx', course='GreekHero', run='run', branch=ModuleStoreEnum.BranchName.draft
+        )
+        head = draft_course.make_usage_key('course', 'head12345')
+        dummy_user = ModuleStoreEnum.UserID.test
+
+        # Not yet published, so changes are present
+        self.assertTrue(self.module_store.has_changes(head))
+
+        # Publish and verify that there are no unpublished changes
+        self.module_store.publish(head, dummy_user)
+        self.assertFalse(self.module_store.has_changes(head))
+
+        # Change the course, then check that there now are changes
+        course = self.module_store.get_item(head)
+        course.show_calculator = not course.show_calculator
+        self.module_store.update_item(course, dummy_user)
+        self.assertTrue(self.module_store.has_changes(head))
+
+        # Publish and verify again
+        self.module_store.publish(head, dummy_user)
+        self.assertFalse(self.module_store.has_changes(head))

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

@@ -86,9 +86,14 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase):
         existing draft for both the new item and the parent
         """
         location = self.old_course_key.make_usage_key(category, name)
-
-        self.draft_mongo.create_and_save_xmodule(
-            location, self.user_id, definition_data=data, metadata=metadata, runtime=self.runtime
+        self.draft_mongo.create_item(
+            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)
@@ -105,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/modulestore/tests/test_xml_importer.py

@@ -7,6 +7,7 @@ from xblock.fields import String, Scope, ScopeIds
 from xblock.runtime import Runtime, KvsFieldData, DictKeyValueStore
 from xmodule.x_module import XModuleMixin
 from opaque_keys.edx.locations import Location
+from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.inheritance import InheritanceMixin
 from xmodule.modulestore.xml_importer import _import_module_and_update_references
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
@@ -39,7 +40,7 @@ class ModuleStoreNoSettings(unittest.TestCase):
         'collection': COLLECTION,
     }
     MODULESTORE = {
-        'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',
+        'ENGINE': 'xmodule.modulestore.mongo.DraftMongoModuleStore',
         'DOC_STORE_CONFIG': DOC_STORE_CONFIG,
         'OPTIONS': modulestore_options
     }
@@ -85,6 +86,7 @@ def modulestore():
         ModuleStoreNoSettings.modulestore = class_(
             None,  # contentstore
             ModuleStoreNoSettings.MODULESTORE['DOC_STORE_CONFIG'],
+            branch_setting_func = lambda: ModuleStoreEnum.Branch.draft_preferred,
             **options
         )
 

common/lib/xmodule/xmodule/split_test_module.py

@@ -569,16 +569,18 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor, StudioEditableDes
 
         This appends the new vertical to the end of children, and updates group_id_to_child.
         A mutable modulestore is needed to call this method (will need to update after mixed
-        modulestore work, currently relies on mongo's create_and_save_xmodule method).
+        modulestore work, currently relies on mongo's create_item method).
         """
-        assert hasattr(self.system, 'modulestore') and hasattr(self.system.modulestore, 'create_and_save_xmodule'), \
+        assert hasattr(self.system, 'modulestore') and hasattr(self.system.modulestore, 'create_item'), \
             "editor_saved should only be called when a mutable modulestore is available"
         modulestore = self.system.modulestore
         dest_usage_key = self.location.replace(category="vertical", name=uuid4().hex)
         metadata = {'display_name': group.name}
-        modulestore.create_and_save_xmodule(
-            dest_usage_key,
+        modulestore.create_item(
             user_id,
+            self.location.course_key,
+            dest_usage_key.block_type,
+            block_id=dest_usage_key.block_id,
             definition_data=None,
             metadata=metadata,
             runtime=self.system,