Replace create_xmodule with create_xblock

Refactor all callers (often replacing with more appropriate fn)

Also, added for_branch_setting method to old mongo and removed some
branch verification tests

Conflicts:
	common/lib/xmodule/xmodule/modulestore/mongo/draft.py
	common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py
	common/lib/xmodule/xmodule/modulestore/xml_importer.py

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -1412,17 +1412,14 @@ class ContentStoreTest(ContentStoreTestCase):
 
         self.assertGreater(len(verticals), 0)
 
-        new_component_location = course.id.make_usage_key('html', 'new_component')
-
         # crate a new module and add it as a child to a vertical
-        new_object = self.store.create_xmodule(new_component_location)
-        self.store.update_item(new_object, self.user.id, allow_not_found=True)
         parent = verticals[0]
-        parent.children.append(new_component_location)
-        self.store.update_item(parent, self.user.id)
+        new_module = self.store.create_child(
+            self.user.id, parent.location, 'html', 'new_component'
+        )
 
         # flush the cache
-        new_module = self.store.get_item(new_component_location)
+        new_module = self.store.get_item(new_module.location)
 
         # check for grace period definition which should be defined at the course level
         self.assertEqual(parent.graceperiod, new_module.graceperiod)
@@ -1438,7 +1435,7 @@ class ContentStoreTest(ContentStoreTestCase):
         self.store.update_item(new_module, self.user.id)
 
         # flush the cache and refetch
-        new_module = self.store.get_item(new_component_location)
+        new_module = self.store.get_item(new_module.location)
 
         self.assertEqual(timedelta(1), new_module.graceperiod)
 

cms/djangoapps/contentstore/tests/test_crud.py

@@ -89,7 +89,8 @@ class TemplateTests(unittest.TestCase):
         )
 
         test_chapter = self.split_store.create_xblock(
-            test_course.system, 'chapter', {'display_name': 'chapter n'}, parent_xblock=test_course
+            test_course.system, test_course.id, 'chapter', fields={'display_name': 'chapter n'},
+            parent_xblock=test_course
         )
         self.assertIsInstance(test_chapter, SequenceDescriptor)
         self.assertEqual(test_chapter.display_name, 'chapter n')
@@ -98,7 +99,8 @@ class TemplateTests(unittest.TestCase):
         # test w/ a definition (e.g., a problem)
         test_def_content = '<problem>boo</problem>'
         test_problem = self.split_store.create_xblock(
-            test_course.system, 'problem', {'data': test_def_content}, parent_xblock=test_chapter
+            test_course.system, test_course.id, 'problem', fields={'data': test_def_content},
+            parent_xblock=test_chapter
         )
         self.assertIsInstance(test_problem, CapaDescriptor)
         self.assertEqual(test_problem.data, test_def_content)
@@ -115,13 +117,14 @@ class TemplateTests(unittest.TestCase):
             display_name='fun test course', user_id='testbot'
         )
         test_chapter = self.split_store.create_xblock(
-            test_course.system, 'chapter', {'display_name': 'chapter n'}, parent_xblock=test_course
+            test_course.system, test_course.id, 'chapter', fields={'display_name': 'chapter n'},
+            parent_xblock=test_course
         )
         self.assertEqual(test_chapter.display_name, 'chapter n')
         test_def_content = '<problem>boo</problem>'
         # create child
         new_block = self.split_store.create_xblock(
-            test_course.system,
+            test_course.system, test_course.id,
             'problem',
             fields={
                 'data': test_def_content,

cms/djangoapps/contentstore/tests/test_utils.py

@@ -15,6 +15,7 @@ from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from opaque_keys.edx.locations import SlashSeparatedCourseKey, Location
 
 from xmodule.modulestore.django import modulestore
+from opaque_keys.edx.locator import CourseLocator
 
 
 class LMSLinksTestCase(TestCase):
@@ -249,16 +250,15 @@ class XBlockVisibilityTestCase(TestCase):
 
     def _create_xblock_with_start_date(self, name, start_date, publish=False, visible_to_staff_only=False):
         """Helper to create an xblock with a start date, optionally publishing it"""
-        location = Location('edX', 'visibility', '2012_Fall', 'vertical', name)
+        course_key = CourseLocator('edX', 'visibility', '2012_Fall')
 
-        vertical = modulestore().create_xmodule(location)
-        vertical.start = start_date
-        if visible_to_staff_only:
-            vertical.visible_to_staff_only = visible_to_staff_only
-        modulestore().update_item(vertical, self.dummy_user, allow_not_found=True)
+        vertical = modulestore().create_item(
+            self.dummy_user, course_key, 'vertical', name, 
+            fields={'start': start_date, 'visible_to_staff_only': visible_to_staff_only}
+        )
 
         if publish:
-            modulestore().publish(location, self.dummy_user)
+            modulestore().publish(vertical.location, self.dummy_user)
 
         return vertical
 

cms/djangoapps/models/settings/course_details.py

@@ -91,7 +91,7 @@ class CourseDetails(object):
             try:
                 about_item = store.get_item(temploc)
             except ItemNotFoundError:
-                about_item = store.create_xmodule(temploc, runtime=course.runtime)
+                about_item = store.create_xblock(course.runtime, course.id, 'about', about_key)
             about_item.data = data
             store.update_item(about_item, user.id)
 

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

@@ -491,18 +491,12 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
                 modulestore._drop_database()  # pylint: disable=protected-access
 
     @strip_key
-    def create_xmodule(self, location, definition_data=None, metadata=None, runtime=None, fields={}, **kwargs):
+    def create_xblock(self, runtime, course_key, block_type, block_id=None, fields=None, **kwargs):
         """
         Create the new xmodule but don't save it. Returns the new module.
-
-        :param location: a Location--must have a category
-        :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
         """
-        store = self._verify_modulestore_support(location.course_key, 'create_xmodule')
-        return store.create_xmodule(location, definition_data, metadata, runtime, fields, **kwargs)
+        store = self._verify_modulestore_support(course_key, 'create_xblock')
+        return store.create_xblock(runtime, course_key, block_type, block_id, fields or {}, **kwargs)
 
     @strip_key
     def get_courses_for_wiki(self, wiki_slug, **kwargs):

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

@@ -476,6 +476,16 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
 
         return course_key.replace(run=self._course_run_cache[cache_key])
 
+    def for_branch_setting(self, location):
+        """
+        Returns the Location that is for the current branch setting.
+        """
+        if location.category in DIRECT_ONLY_CATEGORIES:
+            return location.replace(revision=MongoRevisionKey.published)
+        if self.get_branch_setting() == ModuleStoreEnum.Branch.draft_preferred:
+            return location.replace(revision=MongoRevisionKey.draft)
+        return location.replace(revision=MongoRevisionKey.published)
+
     def _compute_metadata_inheritance_tree(self, course_id):
         '''
         TODO (cdodge) This method can be deleted when the 'split module store' work has been completed
@@ -938,19 +948,11 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         if courses.count() > 0:
             raise DuplicateCourseError(course_id, courses[0]['_id'])
 
-        location = course_id.make_usage_key('course', course_id.run)
-        course = self.create_xmodule(
-            location,
-            fields=fields,
-            **kwargs
-        )
-        self.update_item(course, user_id, allow_not_found=True)
+        course = self.create_item(user_id, course_id, 'course', course_id.run, fields=fields, **kwargs)
 
         # clone a default 'about' overview module as well
-        about_location = location.replace(
-            category='about',
-            name='overview'
-        )
+        about_location = course_id.make_usage_key('about', 'overview')
+
         overview_template = AboutDescriptor.get_template('overview.yaml')
         self.create_item(
             user_id,
@@ -963,25 +965,26 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
 
         return course
 
-    def create_xmodule(self, location, definition_data=None, metadata=None, runtime=None, fields={}, **kwargs):
+    def create_xblock(
+        self, runtime, course_key, block_type, block_id=None, fields=None,
+        metadata=None, definition_data=None, **kwargs
+    ):
         """
-        Create the new xmodule but don't save it. Returns the new module.
+        Create the new xblock but don't save it. Returns the new module.
 
-        :param location: a Location--must have a category
-        :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
         """
-        location = location.replace(run=self.fill_in_run(location.course_key).run)
-        # differs from split mongo in that I believe most of this logic should be above the persistence
-        # layer but added it here to enable quick conversion. I'll need to reconcile these.
         if metadata is None:
             metadata = {}
 
         if definition_data is None:
             definition_data = {}
 
+        # @Cale, should this use LocalId like we do in split?
+        if block_id is None:
+            block_id = uuid4().hex  # really need to make this more readable: e.g., u'{}{}'.format(block_type, uuid4().hex[:4]
+
         if runtime is None:
             services = {}
             if self.i18n_service:
@@ -990,7 +993,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
             runtime = CachingDescriptorSystem(
                 modulestore=self,
                 module_data={},
-                course_key=location.course_key,
+                course_key=course_key,
                 default_class=self.default_class,
                 resources_fs=None,
                 error_tracker=self.error_tracker,
@@ -1000,14 +1003,15 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
                 select=self.xblock_select,
                 services=services,
             )
-        xblock_class = runtime.load_block_type(location.category)
-        dbmodel = self._create_new_field_data(location.category, location, definition_data, metadata)
+        xblock_class = runtime.load_block_type(block_type)
+        location = course_key.make_usage_key(block_type, block_id)
+        dbmodel = self._create_new_field_data(block_type, location, definition_data, metadata)
         xmodule = runtime.construct_xblock_from_class(
             xblock_class,
             # We're loading a descriptor, so student_id is meaningless
             # We also don't have separate notions of definition and usage ids yet,
             # so we use the location for both.
-            ScopeIds(None, location.category, location, location),
+            ScopeIds(None, block_type, location, location),
             dbmodel,
         )
         if fields is not None:
@@ -1034,8 +1038,8 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         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)
+        runtime = kwargs.pop('runtime', None)
+        xblock = self.create_xblock(runtime, course_key, block_type, block_id, **kwargs)
         xblock = self.update_item(xblock, user_id, allow_not_found=True)
 
         return xblock

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

@@ -12,9 +12,7 @@ import logging
 from opaque_keys.edx.locations import Location
 from xmodule.exceptions import InvalidVersionError
 from xmodule.modulestore import PublishState, ModuleStoreEnum
-from xmodule.modulestore.exceptions import (
-    ItemNotFoundError, DuplicateItemError, InvalidBranchSetting, DuplicateCourseError
-)
+from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateItemError, DuplicateCourseError
 from xmodule.modulestore.mongo.base import (
     MongoModuleStore, MongoRevisionKey, as_draft, as_published,
     SORT_REVISION_FAVOR_DRAFT
@@ -292,7 +290,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={}, **kwargs):
+    def create_xblock(self, runtime, course_key, block_type, block_id=None, fields=None, **kwargs):
         """
         Create the new xmodule but don't save it. Returns the new module with a draft locator if
         the category allows drafts. If the category does not allow drafts, just creates a published module.
@@ -303,13 +301,11 @@ class DraftModuleStore(MongoModuleStore):
         :param runtime: if you already have an xmodule from the course, the xmodule.runtime value
         :param fields: a dictionary of field names and values for the new xmodule
         """
-        self._verify_branch_setting(ModuleStoreEnum.Branch.draft_preferred)
-
-        if location.category not in DIRECT_ONLY_CATEGORIES:
-            location = as_draft(location)
-        return wrap_draft(
-            super(DraftModuleStore, self).create_xmodule(location, definition_data, metadata, runtime, fields)
+        new_block = super(DraftModuleStore, self).create_xblock(
+            runtime, course_key, block_type, block_id, fields, **kwargs
         )
+        new_block.location = self.for_branch_setting(new_block.location)
+        return wrap_draft(new_block)
 
     def get_items(self, course_key, revision=None, **kwargs):
         """
@@ -395,7 +391,7 @@ class DraftModuleStore(MongoModuleStore):
             DuplicateItemError: if the source or any of its descendants already has a draft copy. Only
                 useful for unpublish b/c we don't want unpublish to overwrite any existing drafts.
         """
-        # verify input conditions
+        # verify input conditions: can only convert to draft branch; so, verify that's the setting
         self._verify_branch_setting(ModuleStoreEnum.Branch.draft_preferred)
         _verify_revision_is_published(location)
 
@@ -440,13 +436,12 @@ class DraftModuleStore(MongoModuleStore):
         In addition to the superclass's behavior, this method converts the unit to draft if it's not
         direct-only and not already draft.
         """
-        self._verify_branch_setting(ModuleStoreEnum.Branch.draft_preferred)
+        draft_loc = self.for_branch_setting(xblock.location)
 
-        # if the xblock is direct-only, update the PUBLISHED version
-        if xblock.location.category in DIRECT_ONLY_CATEGORIES:
+        # if the revision is published, defer to base
+        if draft_loc.revision == MongoRevisionKey.published:
             return super(DraftModuleStore, self).update_item(xblock, user_id, allow_not_found)
 
-        draft_loc = as_draft(xblock.location)
         if not super(DraftModuleStore, self).has_item(draft_loc):
             try:
                 # ignore any descendants which are already draft

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

@@ -78,6 +78,7 @@ from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection
 from xmodule.error_module import ErrorDescriptor
 from xmodule.modulestore.split_mongo import encode_key_for_mongo, decode_key_from_mongo
 from _collections import defaultdict
+from types import NoneType
 
 
 log = logging.getLogger(__name__)
@@ -1120,6 +1121,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         The implementation tries to detect which, if any changes, actually need to be saved and thus won't version
         the definition, structure, nor course if they didn't change.
         """
+        if allow_not_found and isinstance(descriptor.location.block_id, (LocalId, NoneType)):
+            return self.persist_xblock_dag(descriptor, user_id, force)
+
         original_structure = self._lookup_course(descriptor.location)['structure']
         index_entry = self._get_index_if_valid(descriptor.location, force)
 
@@ -1181,7 +1185,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             # nothing changed, just return the one sent in
             return descriptor
 
-    def create_xblock(self, runtime, category, fields=None, block_id=None, definition_id=None, parent_xblock=None, **kwargs):
+    def create_xblock(
+            self, runtime, course_key, block_type, block_id=None, fields=None,
+            definition_id=None, parent_xblock=None, **kwargs
+    ):
         """
         This method instantiates the correct subclass of XModuleDescriptor based
         on the contents of json_data. It does not persist it and can create one which
@@ -1190,13 +1197,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         parent_xblock is used to compute inherited metadata as well as to append the new xblock.
 
         json_data:
-        - 'category': the xmodule category
+        - 'block_type': the xmodule block_type
         - 'fields': a dict of locally set fields (not inherited) in json format not pythonic typed format!
         - 'definition': the object id of the existing definition
         """
-        xblock_class = runtime.load_block_type(category)
+        xblock_class = runtime.load_block_type(block_type)
         json_data = {
-            'category': category,
+            'category': block_type,
             'fields': fields or {},
         }
         if definition_id is not None:

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

@@ -3,7 +3,7 @@ from factory.containers import CyclicDefinitionError
 from uuid import uuid4
 
 from xmodule.modulestore import prefer_xmodules, ModuleStoreEnum
-from opaque_keys.edx.locations import Location
+from opaque_keys.edx.locations import Location, SlashSeparatedCourseKey
 from opaque_keys.edx.keys import UsageKey
 from xblock.core import XBlock
 from xmodule.tabs import StaticTab
@@ -55,20 +55,14 @@ class CourseFactory(XModuleFactory):
         run = kwargs.get('run', name)
         user_id = kwargs.pop('user_id', ModuleStoreEnum.UserID.test)
 
-        location = Location(org, number, run, 'course', name)
-
         with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred):
             # Write the data to the mongo datastore
-            new_course = store.create_xmodule(location, metadata=kwargs.get('metadata', None))
-
-            # The rest of kwargs become attributes on the course:
-            for k, v in kwargs.iteritems():
-                setattr(new_course, k, v)
-
-            # Save the attributes we just set
-            new_course.save()
-            # Update the data in the mongo datastore
-            store.update_item(new_course, user_id)
+            kwargs.update(kwargs.get('metadata', {}))
+            course_key = SlashSeparatedCourseKey(org, number, run)
+            # TODO - We really should call create_course here.  However, since create_course verifies there are no
+            # duplicates, this breaks several tests that do not clean up properly in between tests.
+            new_course = store.create_xblock(None, course_key, 'course', block_id=run, fields=kwargs)
+            store.update_item(new_course, user_id, allow_not_found=True)
             return new_course
 
 

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

@@ -9,7 +9,6 @@ from path import path
 import re
 import random
 
-from xblock.fields import Scope
 from xmodule.course_module import CourseDescriptor
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.exceptions import (
@@ -268,7 +267,7 @@ class SplitModuleTest(unittest.TestCase):
                         "category": "problem",
                         "fields": {
                             "display_name": "Problem 3.1",
-                            "graceperiod": "4 hours 0 minutes 0 seconds"
+                            "graceperiod": _time_delta_field.from_json("4 hours 0 minutes 0 seconds"),
                         },
                     },
                     {
@@ -486,7 +485,7 @@ class SplitModuleTest(unittest.TestCase):
                         parent = split_store.get_item(block_usage)
                     block_id = LocalId(spec['id'])
                     child = split_store.create_xblock(
-                        course.runtime, spec['category'], spec['fields'], block_id, parent_xblock=parent
+                        course.runtime, course.id, spec['category'], block_id, spec['fields'], parent_xblock=parent
                     )
                     new_ele_dict[spec['id']] = child
                 course = split_store.persist_xblock_dag(course, revision['user_id'])

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -345,8 +345,10 @@ def _import_module_and_update_references(
     # Move the module to a new course
     new_usage_key = module.scope_ids.usage_id.map_into_course(dest_course_id)
     if new_usage_key.category == 'course':
-        new_usage_key = new_usage_key.replace(name=dest_course_id.run)
-    new_module = store.create_xmodule(new_usage_key, runtime=runtime)
+        block_id = dest_course_id.run
+    else:
+        block_id = module.location.block_id
+    new_module = store.create_xblock(runtime, dest_course_id, new_usage_key.category, block_id)
 
     def _convert_reference_fields_to_new_namespace(reference):
         """