From 432249e9cfa37d81847064d8dbb56e3c385bb863 Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Wed, 19 Feb 2014 12:06:44 -0500
Subject: [PATCH 01/15] making mixed ms capable of front ending all
 modulestores for most operations including CRUD STUD-600

---
 .../contentstore/tests/test_contentstore.py   |   9 +-
 .../contentstore/views/component.py           |   4 +-
 cms/djangoapps/contentstore/views/item.py     |   4 +-
 cms/envs/common.py                            |   5 +-
 cms/envs/test.py                              |   2 +-
 .../xmodule/xmodule/modulestore/__init__.py   |  72 ++-
 .../xmodule/modulestore/loc_mapper_store.py   |  38 +-
 .../lib/xmodule/xmodule/modulestore/mixed.py  | 321 +++++++++--
 .../xmodule/xmodule/modulestore/mongo/base.py |  53 +-
 .../xmodule/modulestore/mongo/draft.py        |  21 +-
 .../xmodule/modulestore/split_mongo/split.py  |  48 +-
 .../xmodule/modulestore/tests/django_utils.py |   7 +-
 .../xmodule/modulestore/tests/factories.py    |   7 +-
 .../modulestore/tests/persistent_factories.py |  42 +-
 .../modulestore/tests/test_location_mapper.py |  19 +-
 .../tests/test_mixed_modulestore.py           | 503 ++++++++++--------
 .../xmodule/modulestore/tests/test_mongo.py   |   1 +
 .../lib/xmodule/xmodule/tests/test_import.py  |   4 +-
 .../xmodule/xmodule/tests/xml/factories.py    |   2 +-
 common/lib/xmodule/xmodule/x_module.py        |  16 -
 lms/djangoapps/branding/tests.py              |   2 +-
 lms/envs/acceptance.py                        |   2 +-
 lms/envs/common.py                            |   3 +-
 23 files changed, 779 insertions(+), 406 deletions(-)

diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py
index f41a5fe046..300e338cbe 100644
--- a/cms/djangoapps/contentstore/tests/test_contentstore.py
+++ b/cms/djangoapps/contentstore/tests/test_contentstore.py
@@ -1003,7 +1003,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
         # We had a bug where orphaned draft nodes caused export to fail. This is here to cover that case.
         vertical.location = mongo.draft.as_draft(vertical.location.replace(name='no_references'))
 
-        draft_store.save_xmodule(vertical)
+        draft_store.update_item(vertical, allow_not_found=True)
         orphan_vertical = draft_store.get_item(vertical.location)
         self.assertEqual(orphan_vertical.location.name, 'no_references')
 
@@ -1020,13 +1020,14 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase):
 
         # now create a new/different private (draft only) vertical
         vertical.location = mongo.draft.as_draft(Location(['i4x', 'edX', 'toy', 'vertical', 'a_private_vertical', None]))
-        draft_store.save_xmodule(vertical)
+        draft_store.update_item(vertical, allow_not_found=True)
         private_vertical = draft_store.get_item(vertical.location)
         vertical = None  # blank out b/c i destructively manipulated its location 2 lines above
 
         # add the new private to list of children
-        sequential = module_store.get_item(Location(['i4x', 'edX', 'toy',
-                                           'sequential', 'vertical_sequential', None]))
+        sequential = module_store.get_item(
+            Location('i4x', 'edX', 'toy', 'sequential', 'vertical_sequential', None)
+        )
         private_location_no_draft = private_vertical.location.replace(revision=None)
         sequential.children.append(private_location_no_draft.url())
         module_store.update_item(sequential, self.user.id)
diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py
index c1338d6230..4efc388d08 100644
--- a/cms/djangoapps/contentstore/views/component.py
+++ b/cms/djangoapps/contentstore/views/component.py
@@ -23,7 +23,7 @@ from xblock.exceptions import NoSuchHandlerError
 from xblock.fields import Scope
 from xblock.plugin import PluginMissingError
 from xblock.runtime import Mixologist
-from xmodule.x_module import prefer_xmodules
+from xmodule.modulestore import prefer_xmodules
 
 from lms.lib.xblock.runtime import unquote_slashes
 
@@ -370,6 +370,6 @@ def component_handler(request, usage_id, handler, suffix=''):
         log.info("XBlock %s attempted to access missing handler %r", descriptor, handler, exc_info=True)
         raise Http404
 
-    modulestore().save_xmodule(descriptor)
+    modulestore().update_item(descriptor)
 
     return webob_to_django_response(resp)
diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py
index d9734773b0..a0e1bfb164 100644
--- a/cms/djangoapps/contentstore/views/item.py
+++ b/cms/djangoapps/contentstore/views/item.py
@@ -521,8 +521,8 @@ def orphan_handler(request, tag=None, package_id=None, branch=None, version_guid
     if request.method == 'DELETE':
         if request.user.is_staff:
             items = modulestore().get_orphans(old_location, 'draft')
-            for item in items:
-                modulestore('draft').delete_item(item, delete_all_versions=True)
+            for itemloc in items:
+                modulestore('draft').delete_item(itemloc, delete_all_versions=True)
             return JsonResponse({'deleted': items})
         else:
             raise PermissionDenied()
diff --git a/cms/envs/common.py b/cms/envs/common.py
index 902bd59c1e..0f8d5c9e0d 100644
--- a/cms/envs/common.py
+++ b/cms/envs/common.py
@@ -34,7 +34,8 @@ from path import path
 from lms.lib.xblock.mixin import LmsBlockMixin
 from cms.lib.xblock.mixin import CmsBlockMixin
 from xmodule.modulestore.inheritance import InheritanceMixin
-from xmodule.x_module import XModuleMixin, only_xmodules
+from xmodule.modulestore import only_xmodules
+from xmodule.x_module import XModuleMixin
 from dealer.git import git
 
 ############################ FEATURE CONFIGURATION #############################
@@ -220,7 +221,7 @@ XBLOCK_SELECT_FUNCTION = only_xmodules
 # either by uncommenting them here, or adding them to your private.py
 # You should also enable the ALLOW_ALL_ADVANCED_COMPONENTS feature flag, so that
 # xblocks can be added via advanced settings
-# from xmodule.x_module import prefer_xmodules
+# from xmodule.modulestore import prefer_xmodules
 # XBLOCK_SELECT_FUNCTION = prefer_xmodules
 
 ############################ SIGNAL HANDLERS ################################
diff --git a/cms/envs/test.py b/cms/envs/test.py
index 1e1adf49e4..2f329d60fe 100644
--- a/cms/envs/test.py
+++ b/cms/envs/test.py
@@ -16,6 +16,7 @@ from .common import *
 import os
 from path import path
 from warnings import filterwarnings
+from xmodule.modulestore import prefer_xmodules
 
 # Nose Test Runner
 TEST_RUNNER = 'django_nose.NoseTestSuiteRunner'
@@ -158,7 +159,6 @@ filterwarnings('ignore', message='No request passed to the backend, unable to ra
 
 
 ################################# XBLOCK ######################################
-from xmodule.x_module import prefer_xmodules
 XBLOCK_SELECT_FUNCTION = prefer_xmodules
 
 
diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py
index 8663d6d65b..d752b6dcae 100644
--- a/common/lib/xmodule/xmodule/modulestore/__init__.py
+++ b/common/lib/xmodule/xmodule/modulestore/__init__.py
@@ -7,11 +7,15 @@ import logging
 import re
 
 from collections import namedtuple
+import collections
 
 from abc import ABCMeta, abstractmethod
+from xblock.plugin import default_select
 
 from .exceptions import InvalidLocationError, InsufficientSpecificationError
 from xmodule.errortracker import make_error_tracker
+from xblock.runtime import Mixologist
+from xblock.core import XBlock
 
 log = logging.getLogger('edx.modulestore')
 
@@ -529,9 +533,75 @@ class ModuleStoreReadBase(ModuleStoreRead):
                 return c
         return None
 
+    def update_item(self, xblock, user_id=None, 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 package_id 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=None, delete_all_versions=False, delete_children=False, 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 delete_all_versions: removes both the draft and published version of this item from
+        the course if using draft and old mongo. Split may or may not implement this.
+        :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 package_id and version_guid given and the current
+        version head != version_guid and force is not True. (only applicable to version tracking stores)
+        """
+        raise NotImplementedError
 
 class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite):
     '''
     Implement interface functionality that can be shared.
     '''
-    pass
+    def __init__(self, **kwargs):
+        super(ModuleStoreWriteBase, self).__init__(**kwargs)
+        # TODO: Don't have a runtime just to generate the appropriate mixin classes (cpennington)
+        # This is only used by partition_fields_by_scope, which is only needed because
+        # the split mongo store is used for item creation as well as item persistence
+        self.mixologist = Mixologist(self.xblock_mixins)
+
+    def partition_fields_by_scope(self, category, fields):
+        """
+        Return dictionary of {scope: {field1: val, ..}..} for the fields of this potential xblock
+
+        :param category: the xblock category
+        :param fields: the dictionary of {fieldname: value}
+        """
+        if fields is None:
+            return {}
+        cls = self.mixologist.mix(XBlock.load_class(category, select=prefer_xmodules))
+        result = collections.defaultdict(dict)
+        for field_name, value in fields.iteritems():
+            field = getattr(cls, field_name)
+            result[field.scope][field_name] = value
+        return result
+
+
+def only_xmodules(identifier, entry_points):
+    """Only use entry_points that are supplied by the xmodule package"""
+    from_xmodule = [entry_point for entry_point in entry_points if entry_point.dist.key == 'xmodule']
+
+    return default_select(identifier, from_xmodule)
+
+
+def prefer_xmodules(identifier, entry_points):
+    """Prefer entry_points from the xmodule package"""
+    from_xmodule = [entry_point for entry_point in entry_points if entry_point.dist.key == 'xmodule']
+    if from_xmodule:
+        return default_select(identifier, from_xmodule)
+    else:
+        return default_select(identifier, entry_points)
diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py
index 1f2325e811..b67454865e 100644
--- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py
+++ b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py
@@ -119,7 +119,8 @@ class LocMapperStore(object):
 
         return package_id
 
-    def translate_location(self, old_style_course_id, location, published=True, add_entry_if_missing=True):
+    def translate_location(self, old_style_course_id, location, published=True,
+                           add_entry_if_missing=True, passed_block_id=None):
         """
         Translate the given module location to a Locator. If the mapping has the run id in it, then you
         should provide old_style_course_id with that run id in it to disambiguate the mapping if there exists more
@@ -137,6 +138,8 @@ class LocMapperStore(object):
         :param add_entry_if_missing: a boolean as to whether to raise ItemNotFoundError or to create an entry if
         the course
         or block is not found in the map.
+        :param passed_block_id: what block_id to assign and save if none is found
+        (only if add_entry_if_missing)
 
         NOTE: unlike old mongo, draft branches contain the whole course; so, it applies to all category
         of locations including course.
@@ -158,7 +161,7 @@ class LocMapperStore(object):
                 self.create_map_entry(course_location)
                 entry = self.location_map.find_one(location_id)
             else:
-                raise ItemNotFoundError()
+                raise ItemNotFoundError(location)
         elif len(maps) == 1:
             entry = maps[0]
         else:
@@ -172,7 +175,9 @@ class LocMapperStore(object):
         block_id = entry['block_map'].get(self.encode_key_for_mongo(location.name))
         if block_id is None:
             if add_entry_if_missing:
-                block_id = self._add_to_block_map(location, location_id, entry['block_map'])
+                block_id = self._add_to_block_map(
+                    location, location_id, entry['block_map'], passed_block_id
+                )
             else:
                 raise ItemNotFoundError(location)
         elif isinstance(block_id, dict):
@@ -188,7 +193,7 @@ class LocMapperStore(object):
             elif add_entry_if_missing:
                 block_id = self._add_to_block_map(location, location_id, entry['block_map'])
             else:
-                raise ItemNotFoundError()
+                raise ItemNotFoundError(location)
         else:
             raise InvalidLocationError()
 
@@ -297,7 +302,7 @@ class LocMapperStore(object):
         maps = self.location_map.find(location_id)
         maps = list(maps)
         if len(maps) == 0:
-            raise ItemNotFoundError()
+            raise ItemNotFoundError(location)
         elif len(maps) == 1:
             entry = maps[0]
         else:
@@ -315,18 +320,19 @@ class LocMapperStore(object):
         else:
             return draft_course_locator
 
-    def _add_to_block_map(self, location, location_id, block_map):
+    def _add_to_block_map(self, location, location_id, block_map, block_id=None):
         '''add the given location to the block_map and persist it'''
-        if self._block_id_is_guid(location.name):
-            # This makes the ids more meaningful with a small probability of name collision.
-            # The downside is that if there's more than one course mapped to from the same org/course root
-            # the block ids will likely be out of sync and collide from an id perspective. HOWEVER,
-            # if there are few == org/course roots or their content is unrelated, this will work well.
-            block_id = self._verify_uniqueness(location.category + location.name[:3], block_map)
-        else:
-            # if 2 different category locations had same name, then they'll collide. Make the later
-            # mapped ones unique
-            block_id = self._verify_uniqueness(location.name, block_map)
+        if block_id is None:
+            if self._block_id_is_guid(location.name):
+                # This makes the ids more meaningful with a small probability of name collision.
+                # The downside is that if there's more than one course mapped to from the same org/course root
+                # the block ids will likely be out of sync and collide from an id perspective. HOWEVER,
+                # if there are few == org/course roots or their content is unrelated, this will work well.
+                block_id = self._verify_uniqueness(location.category + location.name[:3], block_map)
+            else:
+                # if 2 different category locations had same name, then they'll collide. Make the later
+                # mapped ones unique
+                block_id = self._verify_uniqueness(location.name, block_map)
         encoded_location_name = self.encode_key_for_mongo(location.name)
         block_map.setdefault(encoded_location_name, {})[location.category] = block_id
         self.location_map.update(location_id, {'$set': {'block_map': block_map}})
diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py
index 6005638796..e95dcee5aa 100644
--- a/common/lib/xmodule/xmodule/modulestore/mixed.py
+++ b/common/lib/xmodule/xmodule/modulestore/mixed.py
@@ -5,15 +5,18 @@ In this way, courses can be served up both - say - XMLModuleStore or MongoModule
 
 """
 
+import re
+from importlib import import_module
+import logging
+from xblock.fields import Reference, ReferenceList, String
+
 from . import ModuleStoreWriteBase
 from xmodule.modulestore.django import create_modulestore_instance, loc_mapper
-import logging
-from xmodule.modulestore import Location
-from xblock.fields import Reference, ReferenceList, String
-from xmodule.modulestore.locator import CourseLocator, Locator, BlockUsageLocator
+from xmodule.modulestore import Location, SPLIT_MONGO_MODULESTORE_TYPE
+from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator
 from xmodule.modulestore.exceptions import InsufficientSpecificationError, ItemNotFoundError
 from xmodule.modulestore.parsers import ALLOWED_ID_CHARS
-import re
+from uuid import uuid4
 
 log = logging.getLogger(__name__)
 
@@ -28,7 +31,8 @@ class MixedModuleStore(ModuleStoreWriteBase):
         Initialize a MixedModuleStore. Here we look into our passed in kwargs which should be a
         collection of other modulestore configuration informations
 
-        :param reference_type: either Location or Locator to indicate what type of reference this app
+        :param reference_type: either a class object such as Locator or Location or the fully
+        qualified dot-path to that class def to indicate what type of reference the app
         uses.
         """
         super(MixedModuleStore, self).__init__(**kwargs)
@@ -39,7 +43,14 @@ class MixedModuleStore(ModuleStoreWriteBase):
         if reference_type is None:
             log.warn("reference_type not specified in MixedModuleStore settings. %s",
                 "Will default temporarily to the to-be-deprecated Location.")
-        self.use_locations = (reference_type != 'Locator')
+            self.reference_type = Location
+        elif isinstance(reference_type, basestring):
+            module_path, _, class_name = reference_type.rpartition('.')
+            class_ = getattr(import_module(module_path), class_name)
+            self.reference_type = class_
+        else:
+            self.reference_type = reference_type
+
         if 'default' not in stores:
             raise Exception('Missing a default modulestore in the MixedModuleStore __init__ method.')
 
@@ -58,15 +69,24 @@ class MixedModuleStore(ModuleStoreWriteBase):
                 store['OPTIONS'],
                 i18n_service=i18n_service,
             )
+            # it would be better to have a notion of read-only rather than hardcode
+            # key name
+            if is_xml:
+                self.ensure_loc_maps_exist(key)
 
     def _get_modulestore_for_courseid(self, course_id):
         """
         For a given course_id, look in the mapping table and see if it has been pinned
         to a particular modulestore
         """
+        # TODO when this becomes a router capable of handling more than one r/w backend
+        # we'll need to generalize this to handle mappings from old Locations w/o full
+        # course_id in much the same way as loc_mapper().translate_location does.
         mapping = self.mappings.get(course_id, 'default')
         return self.modulestores[mapping]
 
+    # TODO move the location converters to a helper class which returns a converter object w/ 2
+    # methods: convert_xblock and convert_reference. Then have mixed get the converter and use it.
     def _locator_to_location(self, reference):
         """
         Convert the referenced locator to a location casting to and from a string as necessary
@@ -84,14 +104,16 @@ class MixedModuleStore(ModuleStoreWriteBase):
         stringify = isinstance(reference, basestring)
         if stringify:
             reference = Location(reference)
-        locator = loc_mapper().translate_location(course_id, reference, reference.revision == 'draft', True)
+        locator = loc_mapper().translate_location(course_id, reference, reference.revision == None, True)
         return unicode(locator) if stringify else locator
 
     def _incoming_reference_adaptor(self, store, course_id, reference):
         """
         Convert the reference to the type the persistence layer wants
         """
-        if issubclass(store.reference_type, Location if self.use_locations else Locator):
+        if reference is None:
+            return None
+        if issubclass(store.reference_type, self.reference_type):
             return reference
         if store.reference_type == Location:
             return self._locator_to_location(reference)
@@ -101,7 +123,9 @@ class MixedModuleStore(ModuleStoreWriteBase):
         """
         Convert the reference to the type the application wants
         """
-        if issubclass(store.reference_type, Location if self.use_locations else Locator):
+        if reference is None:
+            return None
+        if issubclass(store.reference_type, self.reference_type):
             return reference
         if store.reference_type == Location:
             return self._location_to_locator(course_id, reference)
@@ -111,6 +135,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
         """
         Change all reference fields in this xblock to the type expected by the receiving layer
         """
+        xblock.location = adaptor(store, course_id, xblock.location)
         for field in xblock.fields.itervalues():
             if field.is_set_on(xblock):
                 if isinstance(field, Reference):
@@ -136,7 +161,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
         Change all reference fields in this xblock to the type expected by the persistence layer
         """
         string_converter = self._get_string_converter(
-            course_id, store.reference_type, xblock.location
+            course_id, store.reference_type, xblock.scope_ids.usage_id
         )
         return self._xblock_adaptor_iterator(
             self._incoming_reference_adaptor, string_converter, store, course_id, xblock
@@ -147,7 +172,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
         Change all reference fields in this xblock to the type expected by the persistence layer
         """
         string_converter = self._get_string_converter(
-            course_id, xblock.location.__class__, xblock.location
+            course_id, xblock.scope_ids.usage_id.__class__, xblock.scope_ids.usage_id
         )
         return self._xblock_adaptor_iterator(
             self._outgoing_reference_adaptor, string_converter, store, course_id, xblock
@@ -160,9 +185,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
         Return a closure which finds and replaces all embedded links in a string field
         with the correct rewritten link for the target type
         """
-        if self.use_locations and reference_type == Location:
-            return lambda field, xblock: None
-        if not self.use_locations and issubclass(reference_type, Locator):
+        if issubclass(self.reference_type, reference_type):
             return lambda field, xblock: None
         if isinstance(from_base_addr, Location):
             def mapper(found_id):
@@ -189,6 +212,21 @@ class MixedModuleStore(ModuleStoreWriteBase):
             field.write_to(xblock, value)
         return converter
 
+    def ensure_loc_maps_exist(self, store_name):
+        """
+        Ensure location maps exist for every course in the modulestore whose
+        name is the given name (mostly used for 'xml'). It creates maps for any
+        missing ones.
+
+        NOTE: will only work if the given store is Location based. If it's not,
+        it raises NotImplementedError
+        """
+        store = self.modulestores[store_name]
+        if store.reference_type != Location:
+            raise NotImplementedError(u"Cannot create maps from %s", store.reference_type)
+        for course in store.get_courses():
+            loc_mapper().translate_location(course.location.course_id, course.location)
+
     def has_item(self, course_id, reference):
         """
         Does the course include the xblock who's id is reference?
@@ -232,9 +270,8 @@ class MixedModuleStore(ModuleStoreWriteBase):
             raise Exception("Must pass in a course_id when calling get_items()")
 
         store = self._get_modulestore_for_courseid(course_id or getattr(location, 'package_id'))
-        # translate won't work w/ missing fields so work around it
-        if store.reference_type == Location:
-            if not self.use_locations:
+        if not issubclass(self.reference_type, store.reference_type):
+            if store.reference_type == Location:
                 if getattr(location, 'block_id', False):
                     location = self._incoming_reference_adaptor(store, course_id, location)
                 else:
@@ -245,26 +282,31 @@ class MixedModuleStore(ModuleStoreWriteBase):
                         category=qualifiers.get('category', None),
                         name=None
                     )
-        else:
-            if self.use_locations:
+            else:
                 if not isinstance(location, Location):
                     location = Location(location)
                 try:
                     location.ensure_fully_specified()
                     location = loc_mapper().translate_location(
-                        course_id, location, location.revision == 'published', True
+                        course_id, location, location.revision == None, True
                     )
                 except InsufficientSpecificationError:
                     # construct the Locator by hand
                     if location.category is not None and qualifiers.get('category', False):
                         qualifiers['category'] = location.category
                     location = loc_mapper().translate_location_to_course_locator(
-                        course_id, location, location.revision == 'published'
+                        course_id, location, location.revision == None
                     )
         xblocks = store.get_items(location, course_id, depth, qualifiers)
         xblocks = [self._outgoing_xblock_adaptor(store, course_id, xblock) for xblock in xblocks]
         return xblocks
 
+    def _get_course_id_from_course_location(self, course_location):
+        """
+        Get the proper course_id based on the type of course_location
+        """
+        return getattr(course_location, 'course_id', None) or getattr(course_location, 'package_id', None)
+
     def get_courses(self):
         '''
         Returns a list containing the top level XModuleDescriptors of the courses
@@ -272,20 +314,31 @@ class MixedModuleStore(ModuleStoreWriteBase):
         '''
         courses = []
         for key in self.modulestores:
-            store_courses = self.modulestores[key].get_courses()
+            store = self.modulestores[key]
+            store_courses = store.get_courses()
             # If the store has not been labeled as 'default' then we should
             # only surface courses that have a mapping entry, for example the XMLModuleStore will
             # slurp up anything that is on disk, however, we don't want to surface those to
             # consumers *unless* there is an explicit mapping in the configuration
+            # TODO obviously this filtering only applies to filebased stores
             if key != 'default':
                 for course in store_courses:
+                    course_id = self._get_course_id_from_course_location(course.location)
                     # make sure that the courseId is mapped to the store in question
-                    if key == self.mappings.get(course.location.course_id, 'default'):
-                        courses = courses + ([course])
+                    if key == self.mappings.get(course_id, 'default'):
+                        courses.append(
+                            self._outgoing_reference_adaptor(store, course_id, course.location)
+                        )
             else:
                 # if we're the 'default' store provider, then we surface all courses hosted in
                 # that store provider
-                courses = courses + (store_courses)
+                store_courses = [
+                    self._outgoing_reference_adaptor(
+                        store, self._get_course_id_from_course_location(course.location), course.location
+                    )
+                    for course in store_courses
+                ]
+                courses = courses + store_courses
 
         return courses
 
@@ -302,7 +355,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
             # translate won't work w/ missing fields so work around it
             if store.reference_type == Location:
                 # takes the course_id: figure out if this is old or new style
-                if not self.use_locations:
+                if not issubclass(store.reference_type, self.reference_type):
                     if isinstance(course_id, basestring):
                         course_id = CourseLocator(package_id=course_id, branch='published')
                     course_location = loc_mapper().translate_locator_to_location(course_id, get_course=True)
@@ -333,8 +386,10 @@ class MixedModuleStore(ModuleStoreWriteBase):
         store = self._get_modulestore_for_courseid(course_id)
         decoded_ref = self._incoming_reference_adaptor(store, course_id, location)
         parents = store.get_parent_locations(decoded_ref, course_id)
-        return [self._outgoing_reference_adaptor(store, course_id, reference)
-                for reference in parents]
+        return [
+            self._outgoing_reference_adaptor(store, course_id, reference)
+            for reference in parents
+        ]
 
     def get_modulestore_type(self, course_id):
         """
@@ -367,16 +422,187 @@ class MixedModuleStore(ModuleStoreWriteBase):
             errs.update(store.get_errored_courses())
         return errs
 
+    def _get_course_id_from_block(self, block, store):
+        """
+        Get the course_id from the block or from asking its store. Expensive.
+        """
+        if block.course_id is not None:
+            return block.course_id
+        try:
+            course = store._get_course_for_item(block.scope_ids.usage_id)
+            if course:
+                return course.scope_ids.usage_id.course_id
+        except:  # sorry, that method just raises vanilla Exception
+            pass
+
+    def _infer_course_id_try(self, location):
+        """
+        Create, Update, Delete operations don't require a fully-specified course_id, but
+        there's no complete & sound general way to compute the course_id except via the
+        proper modulestore. This method attempts several sound but not complete methods.
+        :param location: an old style Location
+        """
+        if location.category == 'course':  # easiest case
+            return location.course_id
+        # try finding in loc_mapper
+        try:
+            locator = loc_mapper().translate_location_to_course_locator(None, location)
+            location = loc_mapper().translate_locator_to_location(locator, get_course=True)
+            return location.course_id
+        except ItemNotFoundError:
+            pass
+        # expensive query against all location-based modulestores to look for location.
+        for store in self.modulestores.itervalues():
+            if isinstance(location, store.reference_type):
+                try:
+                    block = store.get_item(location)
+                    course_id = self._get_course_id_from_block(block, store)
+                    if course_id is not None:
+                        return course_id
+                except NotImplementedError:
+                    blocks = store.get_items(location)
+                    if len(blocks) == 1:
+                        block = blocks[0]
+                        if block.course_id is not None:
+                            return block.course_id
+                except ItemNotFoundError:
+                    pass
+        # if we get here, it must be in a Locator based store, but we won't be able to find
+        # it.
+        return None
+
+    def create_course(self, course_location, user_id=None, store_name='default', **kwargs):
+        """
+        Creates and returns the course. It creates a loc map from the course_location to
+        the new one (if provided as id_root).
+
+        NOTE: course_location must be a Location not
+        a Locator until we no longer need to do loc mapping.
+
+        NOTE: unlike the other mixed modulestore methods, this does not adapt its argument
+        to the persistence store but requires its caller to know what the persistence store
+        wants for args. It does not translate any references on the way in; so, don't
+        pass children or other reference fields here.
+        It does, however, adapt the xblock on the way out to the app's
+        reference_type
+
+        :returns: course xblock
+        """
+        store = self.modulestores[store_name]
+        if not hasattr(store, 'create_course'):
+            raise NotImplementedError(u"Cannot create a course on store %s", store_name)
+        if store.get_modulestore_type(course_location.course_id) == SPLIT_MONGO_MODULESTORE_TYPE:
+            org = kwargs.pop('org', course_location.org)
+            pretty_id = kwargs.pop('pretty_id', None)
+            # TODO rename id_root to package_id for consistency. It's too confusing
+            id_root = kwargs.pop('id_root', u"{0.org}.{0.course}.{0.name}".format(course_location))
+            course = store.create_course(
+                org, pretty_id, user_id, id_root=id_root, master_branch=course_location.revision or 'published',
+                **kwargs
+            )
+            block_map = {course_location.name: {'course': course.location.block_id}}
+            # NOTE: course.location will be a Locator not == course_location
+            loc_mapper().create_map_entry(
+                course_location, course.location.package_id, block_map=block_map
+            )
+        else:  # assume mongo
+            course = store.create_course(course_location, **kwargs)
+            loc_mapper().translate_location(course_location.course_id, course_location)
+
+        return self._outgoing_xblock_adaptor(store, course_location.course_id, course)
+
+    def create_item(self, course_or_parent_loc, category, user_id=None, **kwargs):
+        """
+        Create and return the item. If parent_loc is a specific location v a course id,
+        it installs the new item as a child of the parent (if the parent_loc is a specific
+        xblock reference).
+
+        Adds an entry to the loc map using the kwarg location if provided (must be a
+        Location if provided) or block_id and category if provided.
+
+        :param course_or_parent_loc: will be translated appropriately to the course's store.
+        Can be a course_id (org/course/run), CourseLocator, Location, or BlockUsageLocator.
+        """
+        # find the store for the course
+        if self.reference_type == Location:
+            if hasattr(course_or_parent_loc, 'tag'):
+                course_id = self._infer_course_id_try(course_or_parent_loc)
+            else:
+                course_id = course_or_parent_loc
+        else:
+            course_id = course_or_parent_loc.package_id
+        store = self._get_modulestore_for_courseid(course_id)
+
+        location = kwargs.pop('location', None)
+        # invoke its create_item
+        if store.reference_type == Location:
+            # convert parent loc if it's legit
+            block_id = kwargs.pop('block_id', uuid4().hex)
+            if isinstance(course_or_parent_loc, basestring):
+                parent_loc = None
+                if location is None:
+                    locn_dict = Location.parse_course_id(course_id)
+                    locn_dict['category'] = category
+                    locn_dict['name'] = block_id
+                    location = Location(locn_dict)
+            else:
+                parent_loc = self._incoming_reference_adaptor(store, course_id, course_or_parent_loc)
+                # must have a legitimate location, compute if appropriate
+                if location is None:
+                    location = parent_loc.replace(category=category, name=block_id)
+            # do the actual creation
+            xblock = store.create_and_save_xmodule(location, **kwargs)
+            # add the loc mapping
+            loc_mapper().translate_location(course_id, location)
+            # don't forget to attach to parent
+            if parent_loc is not None and not 'detached' in xblock._class_tags:
+                parent = store.get_item(parent_loc)
+                parent.children.append(location.url())
+                store.update_item(parent)
+        else:
+            if isinstance(course_or_parent_loc, basestring): # course_id
+                old_course_id = course_or_parent_loc
+                course_or_parent_loc = loc_mapper().translate_location_to_course_locator(
+                    course_or_parent_loc, None
+                )
+            elif isinstance(course_or_parent_loc, CourseLocator):
+                old_course_loc = loc_mapper().translate_locator_to_location(
+                    course_or_parent_loc, get_course=True
+                )
+                old_course_id = old_course_loc.course_id
+            else:  # it's a Location
+                old_course_id = course_id
+                course_or_parent_loc = self._location_to_locator(course_id, course_or_parent_loc)
+            fields = kwargs.get('fields', {})
+            fields.update(kwargs.pop('metadata', {}))
+            fields.update(kwargs.pop('definition_data', {}))
+            kwargs['fields'] = fields
+            xblock = store.create_item(course_or_parent_loc, category, user_id, **kwargs)
+            if location is None:
+                locn_dict = Location.parse_course_id(old_course_id)
+                locn_dict['category'] = category
+                locn_dict['name'] = xblock.location.block_id
+                location = Location(locn_dict)
+            # map location.name to xblock.location.block_id
+            loc_mapper().translate_location(
+                old_course_id, location, passed_block_id=xblock.location.block_id
+            )
+        return xblock
+
     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.
         """
-        if self.use_locations:
-            raise NotImplementedError
-
-        locator = xblock.location
-        course_id = locator.package_id
+        if self.reference_type == Location:
+            course_id = xblock.course_id
+            if course_id is None:
+                course_id = self._infer_course_id_try(xblock.scope_ids.usage_id)
+                if course_id is None:
+                    raise ItemNotFoundError(u"Cannot find modulestore for %s", xblock.scope_ids.usage_id)
+        else:
+            locator = xblock.scope_ids.usage_id
+            course_id = locator.package_id
         store = self._get_modulestore_for_courseid(course_id)
 
         # if an xblock, convert its contents to correct addr scheme
@@ -385,13 +611,28 @@ class MixedModuleStore(ModuleStoreWriteBase):
 
         return self._outgoing_xblock_adaptor(store, course_id, xblock)
 
-    def delete_item(self, location, **kwargs):
+    def delete_item(self, location, user_id=None):
         """
         Delete the given item from persistence.
         """
-        if self.use_locations:
-            raise NotImplementedError
+        if self.reference_type == Location:
+            course_id = self._infer_course_id_try(location)
+            if course_id is None:
+                raise ItemNotFoundError(u"Cannot find modulestore for %s", location)
+        else:
+            course_id = location.package_id
+        store = self._get_modulestore_for_courseid(course_id)
+
+        decoded_ref = self._incoming_reference_adaptor(store, course_id, location)
+        return store.delete_item(decoded_ref, user_id=user_id)
+
+    def close_all_connections(self):
+        """
+        Close all db connections
+        """
+        for mstore in self.modulestores.itervalues():
+            if hasattr(mstore, 'database'):
+                mstore.database.connection.close()
+            elif hasattr(mstore, 'db'):
+                mstore.db.connection.close()
 
-        store = self._get_modulestore_for_courseid(location.package_id)
-        decoded_ref = self._incoming_reference_adaptor(store, location.package_id, location)
-        return store.delete_item(decoded_ref, **kwargs)
diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py
index 6ee52a8b01..4dcf3b6739 100644
--- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py
+++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py
@@ -625,7 +625,16 @@ class MongoModuleStore(ModuleStoreWriteBase):
         modules = self._load_items(list(items), depth)
         return modules
 
-    def create_xmodule(self, location, definition_data=None, metadata=None, system=None):
+    def create_course(self, location, definition_data=None, metadata=None, runtime=None):
+        """
+        Create a course with the given location. The location category must be 'course'.
+        """
+        if location.category != 'course':
+            raise ValueError(u"Course roots must be of category 'course': {}".format(unicode(location)))
+        return self.create_and_save_xmodule(location, definition_data, metadata, runtime)
+
+    def create_xmodule(self, location, definition_data=None, metadata=None, system=None,
+                       fields={}):
         """
         Create the new xmodule but don't save it. Returns the new module.
 
@@ -672,36 +681,18 @@ class MongoModuleStore(ModuleStoreWriteBase):
             ScopeIds(None, location.category, location, location),
             dbmodel,
         )
+        for key, value in fields.iteritems():
+            xmodule[key] = value
         # decache any pending field settings from init
         xmodule.save()
         return xmodule
 
-    def save_xmodule(self, xmodule):
-        """
-        Save the given xmodule (will either create or update based on whether id already exists).
-        Pulls out the data definition v metadata v children locally but saves it all.
-
-        :param xmodule:
-        """
-        # Save any changes to the xmodule to the MongoKeyValueStore
-        xmodule.save()
-        self.collection.save({
-            '_id': namedtuple_to_son(xmodule.location),
-            'metadata': own_metadata(xmodule),
-            'definition': {
-                'data': xmodule.get_explicitly_set_fields_by_scope(Scope.content),
-                'children': xmodule.children if xmodule.has_children else []
-            }
-        })
-        # recompute (and update) the metadata inheritance tree which is cached
-        self.refresh_cached_metadata_inheritance_tree(xmodule.location)
-        self.fire_updated_modulestore_signal(get_course_id_no_run(xmodule.location), xmodule.location)
-
-    def create_and_save_xmodule(self, location, definition_data=None, metadata=None, system=None):
+    def create_and_save_xmodule(self, location, definition_data=None, metadata=None, system=None,
+                                fields={}):
         """
         Create the new xmodule and save it. Does not return the new module because if the caller
         will insert it as a child, it's inherited metadata will completely change. The difference
-        between this and just doing create_xmodule and save_xmodule is this ensures static_tabs get
+        between this and just doing create_xmodule and update_item is this ensures static_tabs get
         pointed to by the course.
 
         :param location: a Location--must have a category
@@ -711,9 +702,9 @@ class MongoModuleStore(ModuleStoreWriteBase):
         """
         # 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.
-        new_object = self.create_xmodule(location, definition_data, metadata, system)
+        new_object = self.create_xmodule(location, definition_data, metadata, system, fields)
         location = new_object.location
-        self.save_xmodule(new_object)
+        self.update_item(new_object, allow_not_found=True)
 
         # VS[compat] cdodge: This is a hack because static_tabs also have references from the course module, so
         # if we add one then we need to also add it to the policy information (i.e. metadata)
@@ -728,9 +719,9 @@ class MongoModuleStore(ModuleStoreWriteBase):
                 'url_slug': new_object.location.name
             })
             course.tabs = existing_tabs
-            # Save any changes to the course to the MongoKeyValueStore
-            course.save()
-            self.update_item(course, '**replace_user**')
+            self.update_item(course)
+
+        return new_object
 
     def fire_updated_modulestore_signal(self, course_id, location):
         """
@@ -787,7 +778,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
         if result['n'] == 0:
             raise ItemNotFoundError(location)
 
-    def update_item(self, xblock, user, allow_not_found=False):
+    def update_item(self, xblock, user=None, allow_not_found=False):
         """
         Update the persisted version of xblock to reflect its current values.
 
@@ -861,7 +852,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
         location = Location.ensure_fully_specified(location)
         items = self.collection.find({'definition.children': location.url()},
                                      {'_id': True})
-        return [i['_id'] for i in items]
+        return [Location(i['_id']) for i in items]
 
     def get_modulestore_type(self, course_id):
         """
diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py
index 5956b00513..99a10c6e4a 100644
--- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py
+++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py
@@ -106,21 +106,6 @@ class DraftModuleStore(MongoModuleStore):
             raise InvalidVersionError(location)
         return super(DraftModuleStore, self).create_xmodule(draft_loc, definition_data, metadata, system)
 
-    def save_xmodule(self, xmodule):
-        """
-        Save the given xmodule (will either create or update based on whether id already exists).
-        Pulls out the data definition v metadata v children locally but saves it all.
-
-        :param xmodule:
-        """
-        orig_location = xmodule.location
-
-        xmodule.location = as_draft(orig_location)
-        try:
-            super(DraftModuleStore, self).save_xmodule(xmodule)
-        finally:
-            xmodule.location = orig_location
-
     def get_items(self, location, course_id=None, depth=0, qualifiers=None):
         """
         Returns a list of XModuleDescriptor instances for the items
@@ -159,7 +144,7 @@ class DraftModuleStore(MongoModuleStore):
         if draft_location.category in DIRECT_ONLY_CATEGORIES:
             raise InvalidVersionError(source_location)
         if not original:
-            raise ItemNotFoundError
+            raise ItemNotFoundError(source_location)
         original['_id'] = namedtuple_to_son(draft_location)
         try:
             self.collection.insert(original)
@@ -171,7 +156,7 @@ class DraftModuleStore(MongoModuleStore):
 
         return self._load_items([original])[0]
 
-    def update_item(self, xblock, user, allow_not_found=False):
+    def update_item(self, xblock, user=None, allow_not_found=False):
         """
         Save the current values to persisted version of the xblock
 
@@ -187,7 +172,7 @@ class DraftModuleStore(MongoModuleStore):
                 raise
 
         xblock.location = draft_loc
-        super(DraftModuleStore, self).update_item(xblock, user)
+        super(DraftModuleStore, self).update_item(xblock, user, allow_not_found)
         # don't allow locations to truly represent themselves as draft outside of this file
         xblock.location = as_published(xblock.location)
 
diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py
index 2d717edd45..9ac0533eb9 100644
--- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py
+++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py
@@ -51,14 +51,13 @@ import logging
 import re
 from importlib import import_module
 from path import path
-import collections
 import copy
 from pytz import UTC
 
 from xmodule.errortracker import null_error_tracker
-from xmodule.x_module import prefer_xmodules
 from xmodule.modulestore.locator import (
-    BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree, LocalId, Locator
+    BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree,
+    LocalId, Locator
 )
 from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError
 from xmodule.modulestore import inheritance, ModuleStoreWriteBase, Location, SPLIT_MONGO_MODULESTORE_TYPE
@@ -67,7 +66,6 @@ from ..exceptions import ItemNotFoundError
 from .definition_lazy_loader import DefinitionLazyLoader
 from .caching_descriptor_system import CachingDescriptorSystem
 from xblock.fields import Scope
-from xblock.runtime import Mixologist
 from bson.objectid import ObjectId
 from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection
 from xblock.core import XBlock
@@ -132,11 +130,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         self.render_template = render_template
         self.i18n_service = i18n_service
 
-        # TODO: Don't have a runtime just to generate the appropriate mixin classes (cpennington)
-        # This is only used by _partition_fields_by_scope, which is only needed because
-        # the split mongo store is used for item creation as well as item persistence
-        self.mixologist = Mixologist(self.xblock_mixins)
-
     def cache_items(self, system, base_block_ids, depth=0, lazy=True):
         '''
         Handles caching of items once inheritance and any other one time
@@ -759,7 +752,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         index_entry = self._get_index_if_valid(course_or_parent_locator, force, continue_version)
         structure = self._lookup_course(course_or_parent_locator)['structure']
 
-        partitioned_fields = self._partition_fields_by_scope(category, fields)
+        partitioned_fields = self.partition_fields_by_scope(category, 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)):
@@ -822,14 +815,19 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         if index_entry is not None:
             if not continue_version:
                 self._update_head(index_entry, course_or_parent_locator.branch, new_id)
-            course_parent = course_or_parent_locator.as_course_locator()
+            item_loc = BlockUsageLocator(
+                package_id=course_or_parent_locator.package_id,
+                branch=course_or_parent_locator.branch,
+                block_id=new_block_id,
+            )
         else:
-            course_parent = None
+            item_loc = BlockUsageLocator(
+                block_id=new_block_id,
+                version_guid=new_id,
+            )
 
         # reconstruct the new_item from the cache
-        return self.get_item(BlockUsageLocator(package_id=course_parent,
-                                               block_id=new_block_id,
-                                               version_guid=new_id))
+        return self.get_item(item_loc)
 
     def create_course(
         self, org, prettyid, user_id, id_root=None, fields=None,
@@ -867,7 +865,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         provide any fields overrides, see above). if not provided, will create a mostly empty course
         structure with just a category course root xblock.
         """
-        partitioned_fields = self._partition_fields_by_scope(root_category, fields)
+        partitioned_fields = self.partition_fields_by_scope(root_category, fields)
         block_fields = partitioned_fields.setdefault(Scope.settings, {})
         if Scope.children in partitioned_fields:
             block_fields.update(partitioned_fields[Scope.children])
@@ -1287,7 +1285,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         if index is None:
             raise ItemNotFoundError(package_id)
         # this is the only real delete in the system. should it do something else?
-        log.info("deleting course from split-mongo: %s", package_id)
+        log.info(u"deleting course from split-mongo: %s", package_id)
         self.db_connection.delete_course_index(index['_id'])
 
     def get_errored_courses(self):
@@ -1494,22 +1492,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         index_entry['versions'][branch] = new_id
         self.db_connection.update_course_index(index_entry)
 
-    def _partition_fields_by_scope(self, category, fields):
-        """
-        Return dictionary of {scope: {field1: val, ..}..} for the fields of this potential xblock
-
-        :param category: the xblock category
-        :param fields: the dictionary of {fieldname: value}
-        """
-        if fields is None:
-            return {}
-        cls = self.mixologist.mix(XBlock.load_class(category, select=self.xblock_select))
-        result = collections.defaultdict(dict)
-        for field_name, value in fields.iteritems():
-            field = getattr(cls, field_name)
-            result[field.scope][field_name] = value
-        return result
-
     def _filter_special_fields(self, fields):
         """
         Remove any fields which split or its kvs computes or adds but does not want persisted.
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py
index 71887629c4..ef30e96add 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py
@@ -33,7 +33,7 @@ def mixed_store_config(data_dir, mappings):
             'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
             'OPTIONS': {
                 'mappings': mappings,
-                'reference_type': 'Location',
+                'reference_type': 'xmodule.modulestore.Location',
                 'stores': {
                     'default': mongo_config['default'],
                     'xml': xml_config['default']
@@ -220,12 +220,17 @@ class ModuleStoreTestCase(TestCase):
         store = editable_modulestore()
         if hasattr(store, 'collection'):
             store.collection.drop()
+            store.db.connection.close()
+        elif hasattr(store, 'close_all_connections'):
+            store.close_all_connections()
         if contentstore().fs_files:
             db = contentstore().fs_files.database
             db.connection.drop_database(db)
+            db.connection.close()
         location_mapper = loc_mapper()
         if location_mapper.db:
             location_mapper.location_map.drop()
+            location_mapper.db.connection.close()
 
     @classmethod
     def setUpClass(cls):
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py
index 4788c8a1d1..6fe138e05c 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py
@@ -2,8 +2,7 @@ from factory import Factory, lazy_attribute_sequence, lazy_attribute
 from factory.containers import CyclicDefinitionError
 from uuid import uuid4
 
-from xmodule.modulestore import Location
-from xmodule.x_module import prefer_xmodules
+from xmodule.modulestore import Location, prefer_xmodules
 from xblock.core import XBlock
 
 
@@ -58,7 +57,7 @@ class CourseFactory(XModuleFactory):
             setattr(new_course, k, v)
 
         # Update the data in the mongo datastore
-        store.save_xmodule(new_course)
+        store.update_item(new_course)
         return new_course
 
 
@@ -159,7 +158,7 @@ class ItemFactory(XModuleFactory):
             setattr(module, attr, val)
         module.save()
 
-        store.save_xmodule(module)
+        store.update_item(module)
 
         if 'detached' not in module._class_tags:
             parent.children.append(location.url())
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py b/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py
index 3031990974..b5cce6be6c 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py
@@ -1,13 +1,23 @@
-from xmodule.modulestore.django import modulestore
 from xmodule.course_module import CourseDescriptor
 from xmodule.x_module import XModuleDescriptor
 import factory
+from factory.helpers import lazy_attribute
 
 
-# [dhm] I'm not sure why we're using factory_boy if we're not following its pattern. If anyone
-# assumes they can call build, it will completely fail, for example.
-# pylint: disable=W0232
-class PersistentCourseFactory(factory.Factory):
+class SplitFactory(factory.Factory):
+    """
+    Abstracted superclass which defines modulestore so that there's no dependency on django
+    if the caller passes modulestore in kwargs
+    """
+    @lazy_attribute
+    def modulestore(self):
+        # Delayed import so that we only depend on django if the caller
+        # hasn't provided their own modulestore
+        from xmodule.modulestore.django import modulestore
+        return modulestore('split')
+
+
+class PersistentCourseFactory(SplitFactory):
     """
     Create a new course (not a new version of a course, but a whole new index entry).
 
@@ -23,12 +33,15 @@ class PersistentCourseFactory(factory.Factory):
 
     # pylint: disable=W0613
     @classmethod
-    def _create(cls, target_class, org='testX', prettyid='999', user_id='test_user', master_branch='draft', **kwargs):
+    def _create(cls, target_class, org='testX', prettyid='999', user_id='test_user',
+                master_branch='draft', id_root=None, **kwargs):
 
+        modulestore = kwargs.pop('modulestore')
+        root_block_id = kwargs.pop('root_block_id', 'course')
         # Write the data to the mongo datastore
-        new_course = modulestore('split').create_course(
-            org, prettyid, user_id, fields=kwargs, id_root=prettyid,
-            master_branch=master_branch)
+        new_course = modulestore.create_course(
+            org, prettyid, user_id, fields=kwargs, id_root=id_root or prettyid,
+            master_branch=master_branch, root_block_id=root_block_id)
 
         return new_course
 
@@ -37,7 +50,7 @@ class PersistentCourseFactory(factory.Factory):
         raise NotImplementedError()
 
 
-class ItemFactory(factory.Factory):
+class ItemFactory(SplitFactory):
     FACTORY_FOR = XModuleDescriptor
 
     display_name = factory.LazyAttributeSequence(lambda o, n: "{} {}".format(o.category, n))
@@ -45,7 +58,8 @@ class ItemFactory(factory.Factory):
     # pylint: disable=W0613
     @classmethod
     def _create(cls, target_class, parent_location, category='chapter',
-        user_id='test_user', definition_locator=None, **kwargs):
+        user_id='test_user', block_id=None, definition_locator=None, force=False,
+        continue_version=False, **kwargs):
         """
         passes *kwargs* as the new item's field values:
 
@@ -55,8 +69,10 @@ class ItemFactory(factory.Factory):
 
         :param definition_locator (optional): the DescriptorLocator for the definition this uses or branches
         """
-        return modulestore('split').create_item(
-            parent_location, category, user_id, definition_locator, fields=kwargs
+        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
         )
 
     @classmethod
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py
index 0848e5c707..7a878cc9f1 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py
@@ -12,11 +12,11 @@ from xmodule.modulestore.loc_mapper_store import LocMapperStore
 from mock import Mock
 
 
-class TestLocationMapper(unittest.TestCase):
+class LocMapperSetupSansDjango(unittest.TestCase):
     """
-    Test the location to locator mapper
+    Create and destroy a loc mapper for each test
     """
-
+    loc_store = None
     def setUp(self):
         modulestore_options = {
             'host': 'localhost',
@@ -27,14 +27,19 @@ class TestLocationMapper(unittest.TestCase):
         cache_standin = TrivialCache()
         self.instrumented_cache = Mock(spec=cache_standin, wraps=cache_standin)
         # pylint: disable=W0142
-        TestLocationMapper.loc_store = LocMapperStore(self.instrumented_cache, **modulestore_options)
+        LocMapperSetupSansDjango.loc_store = LocMapperStore(self.instrumented_cache, **modulestore_options)
 
     def tearDown(self):
         dbref = TestLocationMapper.loc_store.db
         dbref.drop_collection(TestLocationMapper.loc_store.location_map)
         dbref.connection.close()
-        TestLocationMapper.loc_store = None
+        self.loc_store = None
 
+
+class TestLocationMapper(LocMapperSetupSansDjango):
+    """
+    Test the location to locator mapper
+    """
     def test_create_map(self):
         org = 'foo_org'
         course = 'bar_course'
@@ -125,7 +130,7 @@ class TestLocationMapper(unittest.TestCase):
         )
         test_problem_locn = Location('i4x', org, course, 'problem', 'abc123')
         # only one course matches
-        self.translate_n_check(test_problem_locn, old_style_course_id, new_style_package_id, 'problem2', 'published')
+
         # look for w/ only the Location (works b/c there's only one possible course match). Will force
         # cache as default translation for this problemid
         self.translate_n_check(test_problem_locn, None, new_style_package_id, 'problem2', 'published')
@@ -389,7 +394,7 @@ def loc_mapper():
     """
     Mocks the global location mapper.
     """
-    return TestLocationMapper.loc_store
+    return LocMapperSetupSansDjango.loc_store
 
 
 def render_to_template_mock(*_args):
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
index 1b7cbb443b..81ebd70a33 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
@@ -1,286 +1,371 @@
-# pylint: disable=E0611
-from nose.tools import assert_equals, assert_raises, assert_false, \
-    assert_true, assert_not_equals, assert_in, assert_not_in
-# pylint: enable=E0611
 import pymongo
 from uuid import uuid4
+import copy
+import ddt
+from mock import patch
 
 from xmodule.tests import DATA_DIR
-from xmodule.modulestore import Location, MONGO_MODULESTORE_TYPE, XML_MODULESTORE_TYPE
+from xmodule.modulestore import Location, MONGO_MODULESTORE_TYPE, SPLIT_MONGO_MODULESTORE_TYPE, \
+    XML_MODULESTORE_TYPE
 from xmodule.modulestore.exceptions import ItemNotFoundError
-from xmodule.modulestore.xml_importer import import_from_xml
 
-# Mixed modulestore depends on django, so we'll manually configure some django settings
-# before importing the module
+from xmodule.modulestore.locator import BlockUsageLocator
+from xmodule.modulestore.tests.test_location_mapper import LocMapperSetupSansDjango, loc_mapper
+
+# FIXME remove settings
 from django.conf import settings
-import unittest
-import copy
 if not settings.configured:
     settings.configure()
 
 from xmodule.modulestore.mixed import MixedModuleStore
 
+@ddt.ddt
+class TestMixedModuleStore(LocMapperSetupSansDjango):
+    """
+    Quasi-superclass which tests Location based apps against both split and mongo dbs (Locator and
+    Location-based dbs)
+    """
+    HOST = 'localhost'
+    PORT = 27017
+    DB = 'test_mongo_%s' % uuid4().hex[:5]
+    COLLECTION = 'modulestore'
+    FS_ROOT = DATA_DIR
+    DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor'
+    RENDER_TEMPLATE = lambda t_n, d, ctx = None, nsp = 'main': ''
+    REFERENCE_TYPE = 'xmodule.modulestore.Location'
 
-HOST = 'localhost'
-PORT = 27017
-DB = 'test_mongo_%s' % uuid4().hex[:5]
-COLLECTION = 'modulestore'
-FS_ROOT = DATA_DIR
-DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor'
-RENDER_TEMPLATE = lambda t_n, d, ctx = None, nsp = 'main': ''
+    IMPORT_COURSEID = 'MITx/999/2013_Spring'
+    XML_COURSEID1 = 'edX/toy/2012_Fall'
+    XML_COURSEID2 = 'edX/simple/2012_Fall'
 
-IMPORT_COURSEID = 'MITx/999/2013_Spring'
-XML_COURSEID1 = 'edX/toy/2012_Fall'
-XML_COURSEID2 = 'edX/simple/2012_Fall'
-
-OPTIONS = {
-    'mappings': {
-        XML_COURSEID1: 'xml',
-        XML_COURSEID2: 'xml',
-        IMPORT_COURSEID: 'default'
-    },
-    'reference_type': 'Location',
-    'stores': {
-        'xml': {
-            'ENGINE': 'xmodule.modulestore.xml.XMLModuleStore',
-            'OPTIONS': {
-                'data_dir': DATA_DIR,
-                'default_class': 'xmodule.hidden_module.HiddenDescriptor',
-            }
+    modulestore_options = {
+        'default_class': DEFAULT_CLASS,
+        'fs_root': DATA_DIR,
+        'render_template': RENDER_TEMPLATE,
+    }
+    DOC_STORE_CONFIG = {
+        'host': HOST,
+        'db': DB,
+        'collection': COLLECTION,
+    }
+    OPTIONS = {
+        'mappings': {
+            XML_COURSEID1: 'xml',
+            XML_COURSEID2: 'xml',
+            IMPORT_COURSEID: 'default'
         },
-        'default': {
-            'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',
-            'DOC_STORE_CONFIG': {
-                'host': HOST,
-                'db': DB,
-                'collection': COLLECTION,
+        'reference_type': REFERENCE_TYPE,
+        'stores': {
+            'xml': {
+                'ENGINE': 'xmodule.modulestore.xml.XMLModuleStore',
+                'OPTIONS': {
+                    'data_dir': DATA_DIR,
+                    'default_class': 'xmodule.hidden_module.HiddenDescriptor',
+                }
             },
-            'OPTIONS': {
-                'default_class': DEFAULT_CLASS,
-                'fs_root': DATA_DIR,
-                'render_template': RENDER_TEMPLATE,
+            'direct': {
+                'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',
+                'DOC_STORE_CONFIG': DOC_STORE_CONFIG,
+                'OPTIONS': modulestore_options
+            },
+            'draft': {
+                'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',
+                'DOC_STORE_CONFIG': DOC_STORE_CONFIG,
+                'OPTIONS': modulestore_options
+            },
+            'split': {
+                'ENGINE': 'xmodule.modulestore.split_mongo.SplitMongoModuleStore',
+                'DOC_STORE_CONFIG': DOC_STORE_CONFIG,
+                'OPTIONS': modulestore_options
             }
         }
     }
-}
 
+    def _compareIgnoreVersion(self, loc1, loc2, msg=None):
+        """
+        AssertEqual replacement for CourseLocator
+        """
+        if not (loc1.package_id == loc2.package_id and loc1.branch == loc2.branch and loc1.block_id == loc2.block_id):
+            self.fail(self._formatMessage(msg, u"{} != {}".format(unicode(loc1), unicode(loc2))))
 
-class TestMixedModuleStore(object):
-    '''Tests!'''
-    @classmethod
-    def setupClass(cls):
+    def setUp(self):
         """
         Set up the database for testing
         """
-        cls.connection = pymongo.MongoClient(
-            host=HOST,
-            port=PORT,
+        self.options = getattr(self, 'options', self.OPTIONS)
+        self.connection = pymongo.MongoClient(
+            host=self.HOST,
+            port=self.PORT,
             tz_aware=True,
         )
-        cls.connection.drop_database(DB)
-        cls.fake_location = Location('i4x', 'foo', 'bar', 'vertical', 'baz')
-        import_course_dict = Location.parse_course_id(IMPORT_COURSEID)
-        cls.import_org = import_course_dict['org']
-        cls.import_course = import_course_dict['course']
-        cls.import_run = import_course_dict['name']
-        # NOTE: Creating a single db for all the tests to save time.  This
-        # is ok only as long as none of the tests modify the db.
-        # If (when!) that changes, need to either reload the db, or load
-        # once and copy over to a tmp db for each test.
-        cls.store = cls.initdb()
+        self.connection.drop_database(self.DB)
+        self.addCleanup(self.connection.drop_database, self.DB)
+        self.addCleanup(self.connection.close)
+        
+        super(TestMixedModuleStore, self).setUp()
 
-    @classmethod
-    def teardownClass(cls):
-        """
-        Clear out database after test has completed
-        """
-        cls.destroy_db(cls.connection)
+        patcher = patch('xmodule.modulestore.mixed.loc_mapper', return_value=LocMapperSetupSansDjango.loc_store)
+        patcher.start()
+        self.addCleanup(patcher.stop)
+        self.addTypeEqualityFunc(BlockUsageLocator, '_compareIgnoreVersion')
 
-    @staticmethod
-    def initdb():
+    def _create_course(self, default, course_location, item_location):
         """
-        Initialize the database and import one test course into it
+        Create a course w/ one item in the persistence store using the given course & item location.
+        NOTE: course_location and item_location must be Location regardless of the app reference type in order
+        to cause the right mapping to be created.
         """
-        # connect to the db
-        _options = {}
-        _options.update(OPTIONS)
-        store = MixedModuleStore(**_options)
-
-        import_from_xml(
-            store._get_modulestore_for_courseid(IMPORT_COURSEID),
-            DATA_DIR,
-            ['toy'],
-            target_location_namespace=Location(
-                'i4x',
-                TestMixedModuleStore.import_org,
-                TestMixedModuleStore.import_course,
-                'course',
-                TestMixedModuleStore.import_run
+        if default == 'split':
+            course = self.store.create_course(course_location, store_name=default)
+            chapter = self.store.create_item(
+                # don't use course_location as it may not be the repr
+                course.location, item_location.category, location=item_location, block_id=item_location.name
             )
+        else:
+            course = self.store.create_course(
+                course_location, store_name=default, metadata={'display_name': course_location.name}
+            )
+            chapter = self.store.create_item(course_location, item_location.category, location=item_location)
+        if self.REFERENCE_TYPE == 'xmodule.modulestore.locator.CourseLocator':
+            # add_entry is false b/c this is a test that the right thing happened w/o
+            # wanting any additional side effects
+            lookup_map = loc_mapper().translate_location(
+                course_location.course_id, course_location, add_entry_if_missing=False
+            )
+            self.assertEqual(lookup_map, course.location)
+            lookup_map = loc_mapper().translate_location(
+                course_location.course_id, item_location, add_entry_if_missing=False
+            )
+            self.assertEqual(lookup_map, chapter.location)
+        else:
+            self.assertEqual(course.location, course_location)
+            self.assertEqual(chapter.location, item_location)
+
+    def initdb(self, default):
+        """
+        Initialize the database and create one test course in it
+        """
+        # set the default modulestore
+        self.options['stores']['default'] = self.options['stores'][default]
+        self.store = MixedModuleStore(**self.options)
+        self.addCleanup(self.store.close_all_connections)
+
+        def generate_location(course_id):
+            """
+            Generate the locations for the given ids
+            """
+            org, course, run = course_id.split('/')
+            return Location('i4x', org, course, 'course', run)
+
+        self.course_locations = {
+             course_id: generate_location(course_id)
+             for course_id in [self.IMPORT_COURSEID, self.XML_COURSEID1, self.XML_COURSEID2]
+        }
+        self.fake_location = Location('i4x', 'foo', 'bar', 'vertical', 'baz')
+        self.import_chapter_location = self.course_locations[self.IMPORT_COURSEID].replace(
+            category='chapter', name='Overview'
         )
+        self.xml_chapter_location = self.course_locations[self.XML_COURSEID1].replace(
+            category='chapter', name='Overview'
+        )
+        # grab old style location b4 possibly converted
+        import_location = self.course_locations[self.IMPORT_COURSEID]
+        # get Locators and set up the loc mapper if app is Locator based
+        if self.REFERENCE_TYPE == 'xmodule.modulestore.locator.CourseLocator':
+            self.fake_location = loc_mapper().translate_location('foo/bar/2012_Fall', self.fake_location)
+            self.import_chapter_location = loc_mapper().translate_location(
+                self.IMPORT_COURSEID, self.import_chapter_location
+            )
+            self.xml_chapter_location = loc_mapper().translate_location(
+                self.XML_COURSEID1, self.xml_chapter_location
+            )
+            self.course_locations = {
+                course_id: loc_mapper().translate_location(course_id, locn)
+                for course_id, locn in self.course_locations.iteritems()
+            }
 
-        return store
+        self._create_course(default, import_location, self.import_chapter_location)
 
-    @staticmethod
-    def destroy_db(connection):
-        """
-        Destroy the test db.
-        """
-        connection.drop_database(DB)
-
-    def setUp(self):
-        # make a copy for convenience
-        self.connection = TestMixedModuleStore.connection
-
-    def tearDown(self):
-        pass
-
-    def test_get_modulestore_type(self):
+    @ddt.data('direct', 'split')
+    def test_get_modulestore_type(self, default_ms):
         """
         Make sure we get back the store type we expect for given mappings
         """
-        assert_equals(self.store.get_modulestore_type(XML_COURSEID1), XML_MODULESTORE_TYPE)
-        assert_equals(self.store.get_modulestore_type(XML_COURSEID2), XML_MODULESTORE_TYPE)
-        assert_equals(self.store.get_modulestore_type(IMPORT_COURSEID), MONGO_MODULESTORE_TYPE)
+        self.initdb(default_ms)
+        self.assertEqual(self.store.get_modulestore_type(self.XML_COURSEID1), XML_MODULESTORE_TYPE)
+        self.assertEqual(self.store.get_modulestore_type(self.XML_COURSEID2), XML_MODULESTORE_TYPE)
+        mongo_ms_type = MONGO_MODULESTORE_TYPE if default_ms == 'direct' else SPLIT_MONGO_MODULESTORE_TYPE
+        self.assertEqual(self.store.get_modulestore_type(self.IMPORT_COURSEID), mongo_ms_type)
         # try an unknown mapping, it should be the 'default' store
-        assert_equals(self.store.get_modulestore_type('foo/bar/2012_Fall'), MONGO_MODULESTORE_TYPE)
+        self.assertEqual(self.store.get_modulestore_type('foo/bar/2012_Fall'), mongo_ms_type)
 
-    def test_has_item(self):
-        assert_true(self.store.has_item(
-            IMPORT_COURSEID, Location(['i4x', self.import_org, self.import_course, 'course', self.import_run])
-        ))
-        assert_true(self.store.has_item(
-            XML_COURSEID1, Location(['i4x', 'edX', 'toy', 'course', '2012_Fall'])
-        ))
+    @ddt.data('direct', 'split')
+    def test_has_item(self, default_ms):
+        self.initdb(default_ms)
+        for course_id, course_locn in self.course_locations.iteritems():
+            self.assertTrue(self.store.has_item(course_id, course_locn))
 
         # try negative cases
-        assert_false(self.store.has_item(
-            XML_COURSEID1, Location(['i4x', self.import_org, self.import_course, 'course', self.import_run])
-        ))
-        assert_false(self.store.has_item(
-            IMPORT_COURSEID, Location(['i4x', 'edX', 'toy', 'course', '2012_Fall'])
-        ))
+        self.assertFalse(self.store.has_item(self.XML_COURSEID1, self.course_locations[self.IMPORT_COURSEID]))
+        self.assertFalse(self.store.has_item(self.IMPORT_COURSEID, self.course_locations[self.XML_COURSEID1]))
 
-    def test_get_item(self):
-        with assert_raises(NotImplementedError):
+    @ddt.data('direct', 'split')
+    def test_get_item(self, default_ms):
+        self.initdb(default_ms)
+        with self.assertRaises(NotImplementedError):
             self.store.get_item(self.fake_location)
 
-    def test_get_instance(self):
-        module = self.store.get_instance(
-            IMPORT_COURSEID, Location(['i4x', self.import_org, self.import_course, 'course', self.import_run])
-        )
-        assert_not_equals(module, None)
-
-        module = self.store.get_instance(
-            XML_COURSEID1, Location(['i4x', 'edX', 'toy', 'course', '2012_Fall'])
-        )
-        assert_not_equals(module, None)
+    @ddt.data('direct', 'split')
+    def test_get_instance(self, default_ms):
+        self.initdb(default_ms)
+        for course_id, course_locn in self.course_locations.iteritems():
+            self.assertIsNotNone(self.store.get_instance(course_id, course_locn))
 
         # try negative cases
-        with assert_raises(ItemNotFoundError):
-            self.store.get_instance(
-                XML_COURSEID1, Location(['i4x', self.import_org, self.import_course, 'course', self.import_run])
-            )
+        with self.assertRaises(ItemNotFoundError):
+            self.store.get_instance(self.XML_COURSEID1, self.course_locations[self.IMPORT_COURSEID])
+        with self.assertRaises(ItemNotFoundError):
+            self.store.get_instance(self.IMPORT_COURSEID, self.course_locations[self.XML_COURSEID1])
 
-        with assert_raises(ItemNotFoundError):
-            self.store.get_instance(
-                IMPORT_COURSEID, Location(['i4x', 'edX', 'toy', 'course', '2012_Fall'])
-            )
+    @ddt.data('direct', 'split')
+    def test_get_items(self, default_ms):
+        self.initdb(default_ms)
+        for course_id, course_locn in self.course_locations.iteritems():
+            if hasattr(course_locn, 'as_course_locator'):
+                locn = course_locn.as_course_locator()
+            else:
+                locn = course_locn.replace(org=None, course=None, name=None)
+            # NOTE: use get_course if you just want the course. get_items is expensive
+            modules = self.store.get_items(locn, course_id, qualifiers={'category': 'course'})
+            self.assertEqual(len(modules), 1)
+            self.assertEqual(modules[0].location, course_locn)
 
-    def test_get_items(self):
-        # NOTE: use get_course if you just want the course. get_items only allows wildcarding of category and name
-        modules = self.store.get_items(Location('i4x', None, None, 'course', None), IMPORT_COURSEID)
-        assert_equals(len(modules), 1)
-        assert_equals(modules[0].location.course, self.import_course)
+    @ddt.data('direct', 'split')
+    def test_update_item(self, default_ms):
+        """
+        Update should fail for r/o dbs and succeed for r/w ones
+        """
+        self.initdb(default_ms)
+        # try a r/o db
+        if self.REFERENCE_TYPE == 'xmodule.modulestore.locator.CourseLocator':
+            course_id = self.course_locations[self.XML_COURSEID1]
+        else:
+            course_id = self.XML_COURSEID1
+        course = self.store.get_course(course_id)
+        # if following raised, then the test is really a noop, change it
+        self.assertFalse(course.show_calculator, "Default changed making test meaningless")
+        course.show_calculator = True
+        with self.assertRaises(NotImplementedError):
+            self.store.update_item(course, None)
+        # now do it for a r/w db
+        # get_course api's are inconsistent: one takes Locators the other an old style course id
+        if hasattr(self.course_locations[self.IMPORT_COURSEID], 'as_course_locator'):
+            locn = self.course_locations[self.IMPORT_COURSEID]
+        else:
+            locn = self.IMPORT_COURSEID
+        course = self.store.get_course(locn)
+        # if following raised, then the test is really a noop, change it
+        self.assertFalse(course.show_calculator, "Default changed making test meaningless")
+        course.show_calculator = True
+        self.store.update_item(course, None)
+        course = self.store.get_course(locn)
+        self.assertTrue(course.show_calculator)
 
-        modules = self.store.get_items(Location('i4x', None, None, 'course', None), XML_COURSEID1)
-        assert_equals(len(modules), 1)
-        assert_equals(modules[0].location.course, 'toy')
+    @ddt.data('direct', 'split')
+    def test_delete_item(self, default_ms):
+        """
+        Delete should reject on r/o db and work on r/w one
+        """
+        self.initdb(default_ms)
+        # r/o try deleting the course
+        with self.assertRaises(NotImplementedError):
+            self.store.delete_item(self.xml_chapter_location)
+        self.store.delete_item(self.import_chapter_location, '**replace_user**')
+        # verify it's gone
+        with self.assertRaises(ItemNotFoundError):
+            self.store.get_instance(self.IMPORT_COURSEID, self.import_chapter_location)
 
-        modules = self.store.get_items(Location('i4x', 'edX', 'simple', 'course', None), XML_COURSEID2)
-        assert_equals(len(modules), 1)
-        assert_equals(modules[0].location.course, 'simple')
-
-    def test_update_item(self):
-        # FIXME update
-        with assert_raises(NotImplementedError):
-            self.store.update_item(self.fake_location, '**replace_user**')
-
-    def test_delete_item(self):
-        with assert_raises(NotImplementedError):
-            self.store.delete_item(self.fake_location)
-
-    def test_get_courses(self):
+    @ddt.data('direct', 'split')
+    def test_get_courses(self, default_ms):
+        self.initdb(default_ms)
         # we should have 3 total courses aggregated
         courses = self.store.get_courses()
-        assert_equals(len(courses), 3)
-        course_ids = []
-        for course in courses:
-            course_ids.append(course.location.course_id)
-        assert_true(IMPORT_COURSEID in course_ids)
-        assert_true(XML_COURSEID1 in course_ids)
-        assert_true(XML_COURSEID2 in course_ids)
+        self.assertEqual(len(courses), 3)
+        course_ids = [course.location for course in courses]
+        self.assertIn(self.course_locations[self.IMPORT_COURSEID], course_ids)
+        self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids)
+        self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids)
 
     def test_xml_get_courses(self):
         """
         Test that the xml modulestore only loaded the courses from the maps.
         """
         courses = self.store.modulestores['xml'].get_courses()
-        assert_equals(len(courses), 2)
+        self.assertEqual(len(courses), 2)
         course_ids = [course.location.course_id for course in courses]
-        assert_in(XML_COURSEID1, course_ids)
-        assert_in(XML_COURSEID2, course_ids)
+        self.assertIn(self.XML_COURSEID1, course_ids)
+        self.assertIn(self.XML_COURSEID2, course_ids)
         # this course is in the directory from which we loaded courses but not in the map
-        assert_not_in("edX/toy/TT_2012_Fall", course_ids)
+        self.assertNotIn("edX/toy/TT_2012_Fall", course_ids)
 
-    def test_get_course(self):
-        module = self.store.get_course(IMPORT_COURSEID)
-        assert_equals(module.location.course, self.import_course)
-
-        module = self.store.get_course(XML_COURSEID1)
-        assert_equals(module.location.course, 'toy')
-
-        module = self.store.get_course(XML_COURSEID2)
-        assert_equals(module.location.course, 'simple')
+    @ddt.data('direct', 'split')
+    def test_get_course(self, default_ms):
+        self.initdb(default_ms)
+        for course_locn in self.course_locations.itervalues():
+            if hasattr(course_locn, 'as_course_locator'):
+                locn = course_locn.as_course_locator()
+            else:
+                locn = course_locn.course_id
+            # NOTE: use get_course if you just want the course. get_items is expensive
+            course = self.store.get_course(locn)
+            self.assertIsNotNone(course)
+            self.assertEqual(course.location, course_locn)
 
     # pylint: disable=E1101
-    def test_get_parent_locations(self):
+    @ddt.data('direct', 'split')
+    def test_get_parent_locations(self, default_ms):
+        self.initdb(default_ms)
         parents = self.store.get_parent_locations(
-            Location(['i4x', self.import_org, self.import_course, 'chapter', 'Overview']),
-            IMPORT_COURSEID
+            self.import_chapter_location,
+            self.IMPORT_COURSEID
         )
-        assert_equals(len(parents), 1)
-        assert_equals(Location(parents[0]).org, self.import_org)
-        assert_equals(Location(parents[0]).course, self.import_course)
-        assert_equals(Location(parents[0]).name, self.import_run)
+        self.assertEqual(len(parents), 1)
+        self.assertEqual(parents[0], self.course_locations[self.IMPORT_COURSEID])
 
         parents = self.store.get_parent_locations(
-            Location(['i4x', 'edX', 'toy', 'chapter', 'Overview']),
-            XML_COURSEID1
+            self.xml_chapter_location,
+            self.XML_COURSEID1
         )
-        assert_equals(len(parents), 1)
-        assert_equals(Location(parents[0]).org, 'edX')
-        assert_equals(Location(parents[0]).course, 'toy')
-        assert_equals(Location(parents[0]).name, '2012_Fall')
+        self.assertEqual(len(parents), 1)
+        self.assertEqual(parents[0], self.course_locations[self.XML_COURSEID1])
 
-class TestMixedMSInit(unittest.TestCase):
+@ddt.ddt
+class TestMixedUseLocator(TestMixedModuleStore):
+    """
+    Tests a mixed ms which uses Locators instead of Locations
+    """
+    REFERENCE_TYPE = 'xmodule.modulestore.locator.CourseLocator'
+
+    def setUp(self):
+        self.options = copy.copy(self.OPTIONS)
+        self.options['reference_type'] = self.REFERENCE_TYPE
+        super(TestMixedUseLocator, self).setUp()
+
+@ddt.ddt
+class TestMixedMSInit(TestMixedModuleStore):
     """
     Test initializing w/o a reference_type
     """
+    REFERENCE_TYPE = None
     def setUp(self):
-        unittest.TestCase.setUp(self)
-        options = copy.copy(OPTIONS)
-        del options['reference_type']
-        self.connection = pymongo.MongoClient(
-            host=HOST,
-            port=PORT,
-            tz_aware=True,
-        )
-        self.store = MixedModuleStore(**options)
+        self.options = copy.copy(self.OPTIONS)
+        del self.options['reference_type']
+        super(TestMixedMSInit, self).setUp()
 
-    def test_use_locations(self):
+    @ddt.data('direct', 'split')
+    def test_use_locations(self, default_ms):
         """
         Test that use_locations defaulted correctly
         """
-        self.assertTrue(self.store.use_locations)
-
+        self.initdb(default_ms)
+        self.assertEqual(self.store.reference_type, Location)
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py
index f492c321d2..33b6eaa2a4 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py
@@ -56,6 +56,7 @@ class TestMongoModuleStore(object):
     def teardownClass(cls):
         if cls.connection:
             cls.connection.drop_database(DB)
+            cls.connection.close()
 
     @staticmethod
     def initdb():
diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py
index 0f0aa80145..0a185ded87 100644
--- a/common/lib/xmodule/xmodule/tests/test_import.py
+++ b/common/lib/xmodule/xmodule/tests/test_import.py
@@ -11,10 +11,10 @@ from mock import Mock, patch
 from django.utils.timezone import UTC
 
 from xmodule.xml_module import is_pointer_tag
-from xmodule.modulestore import Location
+from xmodule.modulestore import Location, only_xmodules
 from xmodule.modulestore.xml import ImportSystem, XMLModuleStore, LocationReader
 from xmodule.modulestore.inheritance import compute_inherited_metadata
-from xmodule.x_module import XModuleMixin, only_xmodules
+from xmodule.x_module import XModuleMixin
 from xmodule.fields import Date
 from xmodule.tests import DATA_DIR
 from xmodule.modulestore.inheritance import InheritanceMixin
diff --git a/common/lib/xmodule/xmodule/tests/xml/factories.py b/common/lib/xmodule/xmodule/tests/xml/factories.py
index a66fdba2e1..e498645c2d 100644
--- a/common/lib/xmodule/xmodule/tests/xml/factories.py
+++ b/common/lib/xmodule/xmodule/tests/xml/factories.py
@@ -9,7 +9,7 @@ from factory import Factory, lazy_attribute, post_generation, Sequence
 from lxml import etree
 
 from xmodule.modulestore.inheritance import InheritanceMixin
-from xmodule.x_module import only_xmodules
+from xmodule.modulestore import only_xmodules
 
 
 class XmlImportData(object):
diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py
index 6d9ada7314..489a2c8032 100644
--- a/common/lib/xmodule/xmodule/x_module.py
+++ b/common/lib/xmodule/xmodule/x_module.py
@@ -580,22 +580,6 @@ class ResourceTemplates(object):
                     return template
 
 
-def prefer_xmodules(identifier, entry_points):
-    """Prefer entry_points from the xmodule package"""
-    from_xmodule = [entry_point for entry_point in entry_points if entry_point.dist.key == 'xmodule']
-    if from_xmodule:
-        return default_select(identifier, from_xmodule)
-    else:
-        return default_select(identifier, entry_points)
-
-
-def only_xmodules(identifier, entry_points):
-    """Only use entry_points that are supplied by the xmodule package"""
-    from_xmodule = [entry_point for entry_point in entry_points if entry_point.dist.key == 'xmodule']
-
-    return default_select(identifier, from_xmodule)
-
-
 @XBlock.needs("i18n")
 class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
     """
diff --git a/lms/djangoapps/branding/tests.py b/lms/djangoapps/branding/tests.py
index 3849e54161..c25e768567 100644
--- a/lms/djangoapps/branding/tests.py
+++ b/lms/djangoapps/branding/tests.py
@@ -31,7 +31,7 @@ class AnonymousIndexPageTest(ModuleStoreTestCase):
         self.course = CourseFactory.create()
         self.course.days_early_for_beta = 5
         self.course.enrollment_start = datetime.datetime.now(UTC) + datetime.timedelta(days=3)
-        self.store.save_xmodule(self.course)
+        self.store.update_item(self.course)
 
     @override_settings(FEATURES=FEATURES_WITH_STARTDATE)
     def test_none_user_index_access_with_startdate_fails(self):
diff --git a/lms/envs/acceptance.py b/lms/envs/acceptance.py
index 5f059782ad..eeafd783ea 100644
--- a/lms/envs/acceptance.py
+++ b/lms/envs/acceptance.py
@@ -45,7 +45,7 @@ MODULESTORE = {
         'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
         'OPTIONS': {
             'mappings': {},
-            'reference_type': 'Location',
+            'reference_type': 'xmodule.modulestore.Location',
             'stores': {
                 'default': {
                     'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',
diff --git a/lms/envs/common.py b/lms/envs/common.py
index a9d2f75726..1bf49cd9ff 100644
--- a/lms/envs/common.py
+++ b/lms/envs/common.py
@@ -34,7 +34,8 @@ from .discussionsettings import *
 
 from lms.lib.xblock.mixin import LmsBlockMixin
 from xmodule.modulestore.inheritance import InheritanceMixin
-from xmodule.x_module import XModuleMixin, only_xmodules
+from xmodule.x_module import XModuleMixin
+from xmodule.modulestore import only_xmodules
 
 ################################### FEATURES ###################################
 # The display name of the platform to be used in templates/emails/etc.

From ca0dd8390fa89b4f913ad4497564e28296837d86 Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Mon, 24 Feb 2014 11:36:34 -0500
Subject: [PATCH 02/15] Mixed no longer translates

---
 .../contentstore/tests/test_export_git.py     |   5 +-
 cms/djangoapps/contentstore/views/item.py     |   7 +-
 cms/envs/bok_choy.py                          |   2 +-
 .../xmodule/xmodule/modulestore/__init__.py   |   4 +-
 .../xmodule/xmodule/modulestore/locator.py    |  49 +-
 .../lib/xmodule/xmodule/modulestore/mixed.py  | 446 +++++-------------
 .../xmodule/xmodule/modulestore/mongo/base.py |   3 +-
 .../xmodule/modulestore/mongo/draft.py        |   4 +-
 .../xmodule/modulestore/split_mongo/split.py  |   4 +-
 .../xmodule/modulestore/tests/django_utils.py |   6 +-
 .../modulestore/tests/test_locators.py        |   5 +-
 .../tests/test_mixed_modulestore.py           | 139 +++---
 lms/envs/acceptance.py                        |   1 -
 lms/envs/cms/dev.py                           |   1 -
 14 files changed, 249 insertions(+), 427 deletions(-)

diff --git a/cms/djangoapps/contentstore/tests/test_export_git.py b/cms/djangoapps/contentstore/tests/test_export_git.py
index 6288a7ef00..3d6fef7239 100644
--- a/cms/djangoapps/contentstore/tests/test_export_git.py
+++ b/cms/djangoapps/contentstore/tests/test_export_git.py
@@ -17,6 +17,7 @@ from .utils import CourseTestCase
 import contentstore.git_export_utils as git_export_utils
 from xmodule.contentstore.django import _CONTENTSTORE
 from xmodule.modulestore.django import modulestore
+from contentstore.utils import get_modulestore
 
 TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE)
 TEST_DATA_CONTENTSTORE['DOC_STORE_CONFIG']['db'] = 'test_xcontent_%s' % uuid4().hex
@@ -70,7 +71,7 @@ class TestExportGit(CourseTestCase):
         Test failed course export response.
         """
         self.course_module.giturl = 'foobar'
-        modulestore().save_xmodule(self.course_module)
+        get_modulestore(self.course_module.location).update_item(self.course_module)
 
         response = self.client.get('{}?action=push'.format(self.test_url))
         self.assertIn('Export Failed:', response.content)
@@ -93,7 +94,7 @@ class TestExportGit(CourseTestCase):
 
         self.populateCourse()
         self.course_module.giturl = 'file://{}'.format(bare_repo_dir)
-        modulestore().save_xmodule(self.course_module)
+        get_modulestore(self.course_module.location).update_item(self.course_module)
 
         response = self.client.get('{}?action=push'.format(self.test_url))
         self.assertIn('Export Succeeded', response.content)
diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py
index a0e1bfb164..6c15a7d400 100644
--- a/cms/djangoapps/contentstore/views/item.py
+++ b/cms/djangoapps/contentstore/views/item.py
@@ -202,7 +202,12 @@ def xblock_view_handler(request, package_id, view_name, tag=None, branch=None, v
                 log.debug("unable to render studio_view for %r", component, exc_info=True)
                 fragment = Fragment(render_to_string('html_error.html', {'message': str(exc)}))
 
-            store.save_xmodule(component)
+            # change not authored by requestor but by xblocks. should not convert to draft if not
+            # already draft
+            if getattr(component, 'is_draft', False):
+                modulestore('draft').update_item(component, None)
+            else:
+                modulestore('direct').update_item(component, None)
         elif view_name == 'student_view' and component.has_children:
             # For non-leaf xblocks on the unit page, show the special rendering
             # which links to the new container page.
diff --git a/cms/envs/bok_choy.py b/cms/envs/bok_choy.py
index bc2a61a8ea..2218fed90e 100644
--- a/cms/envs/bok_choy.py
+++ b/cms/envs/bok_choy.py
@@ -16,7 +16,7 @@ os.environ['SERVICE_VARIANT'] = 'bok_choy'
 os.environ['CONFIG_ROOT'] = path(__file__).abspath().dirname()  #pylint: disable=E1120
 
 from .aws import *  # pylint: disable=W0401, W0614
-from xmodule.x_module import prefer_xmodules
+from xmodule.modulestore import prefer_xmodules
 
 
 ######################### Testing overrides ####################################
diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py
index d752b6dcae..b67480ac75 100644
--- a/common/lib/xmodule/xmodule/modulestore/__init__.py
+++ b/common/lib/xmodule/xmodule/modulestore/__init__.py
@@ -479,7 +479,9 @@ class ModuleStoreReadBase(ModuleStoreRead):
         metadata_inheritance_cache_subsystem=None, request_cache=None,
         modulestore_update_signal=None, xblock_mixins=(), xblock_select=None,
         # temporary parms to enable backward compatibility. remove once all envs migrated
-        db=None, collection=None, host=None, port=None, tz_aware=True, user=None, password=None
+        db=None, collection=None, host=None, port=None, tz_aware=True, user=None, password=None,
+        # allow lower level init args to pass harmlessly
+        ** kwargs
     ):
         '''
         Set up the error-tracking logic.
diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py
index 01c7d4155c..b76ca4861a 100644
--- a/common/lib/xmodule/xmodule/modulestore/locator.py
+++ b/common/lib/xmodule/xmodule/modulestore/locator.py
@@ -51,6 +51,12 @@ class Locator(object):
     def __eq__(self, other):
         return self.__dict__ == other.__dict__
 
+    def __hash__(self):
+        """
+        Hash on contents.
+        """
+        return hash(unicode(self))
+
     def __repr__(self):
         '''
         repr(self) returns something like this: CourseLocator("mit.eecs.6002x")
@@ -198,16 +204,16 @@ class CourseLocator(Locator):
         """
         Return a string representing this location.
         """
+        result = u""
         if self.package_id:
-            result = unicode(self.package_id)
+            result += unicode(self.package_id)
             if self.branch:
                 result += '/' + BRANCH_PREFIX + self.branch
-            return result
-        elif self.version_guid:
-            return u"{prefix}{guid}".format(prefix=VERSION_PREFIX, guid=self.version_guid)
-        else:
-            # raise InsufficientSpecificationError("missing package_id or version_guid")
-            return '<InsufficientSpecificationError: missing package_id or version_guid>'
+        if self.version_guid:
+            if self.package_id:
+                result += u"/"
+            result += u"{prefix}{guid}".format(prefix=VERSION_PREFIX, guid=self.version_guid)
+        return result
 
     def url(self):
         """
@@ -432,22 +438,29 @@ class BlockUsageLocator(CourseLocator):
 
     def version_agnostic(self):
         """
-        Returns a copy of itself.
-        If both version_guid and package_id are known, use a blank package_id in the copy.
-
         We don't care if the locator's version is not the current head; so, avoid version conflict
         by reducing info.
+        Returns a copy of itself without any version info.
+
+        :raises: ValueError if the block locator has no package_id
 
         :param block_locator:
         """
-        if self.version_guid:
-            return BlockUsageLocator(version_guid=self.version_guid,
-                                     branch=self.branch,
-                                     block_id=self.block_id)
-        else:
-            return BlockUsageLocator(package_id=self.package_id,
-                                     branch=self.branch,
-                                     block_id=self.block_id)
+        return BlockUsageLocator(package_id=self.package_id,
+                                 branch=self.branch,
+                                 block_id=self.block_id)
+
+    def course_agnostic(self):
+        """
+        We only care about the locator's version not its course.
+        Returns a copy of itself without any course info.
+
+        :raises: ValueError if the block locator has no version_guid
+
+        :param block_locator:
+        """
+        return BlockUsageLocator(version_guid=self.version_guid,
+                                 block_id=self.block_id)
 
     def set_block_id(self, new):
         """
diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py
index e95dcee5aa..663552b98c 100644
--- a/common/lib/xmodule/xmodule/modulestore/mixed.py
+++ b/common/lib/xmodule/xmodule/modulestore/mixed.py
@@ -5,58 +5,41 @@ In this way, courses can be served up both - say - XMLModuleStore or MongoModule
 
 """
 
-import re
-from importlib import import_module
 import logging
-from xblock.fields import Reference, ReferenceList, String
 
 from . import ModuleStoreWriteBase
 from xmodule.modulestore.django import create_modulestore_instance, loc_mapper
 from xmodule.modulestore import Location, SPLIT_MONGO_MODULESTORE_TYPE
-from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator
-from xmodule.modulestore.exceptions import InsufficientSpecificationError, ItemNotFoundError
-from xmodule.modulestore.parsers import ALLOWED_ID_CHARS
+from xmodule.modulestore.locator import CourseLocator
+from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError
 from uuid import uuid4
+from xmodule.modulestore.mongo.base import MongoModuleStore
+from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore
 
 log = logging.getLogger(__name__)
 
 
 class MixedModuleStore(ModuleStoreWriteBase):
     """
-    ModuleStore knows how to route requests to the right persistence ms and how to convert any
-    references in the xblocks to the type required by the app and the persistence layer.
+    ModuleStore knows how to route requests to the right persistence ms
     """
-    def __init__(self, mappings, stores, reference_type=None, i18n_service=None, **kwargs):
+    def __init__(self, mappings, stores, i18n_service=None, **kwargs):
         """
         Initialize a MixedModuleStore. Here we look into our passed in kwargs which should be a
         collection of other modulestore configuration informations
-
-        :param reference_type: either a class object such as Locator or Location or the fully
-        qualified dot-path to that class def to indicate what type of reference the app
-        uses.
         """
         super(MixedModuleStore, self).__init__(**kwargs)
 
         self.modulestores = {}
         self.mappings = mappings
-        # temporary code for transition period
-        if reference_type is None:
-            log.warn("reference_type not specified in MixedModuleStore settings. %s",
-                "Will default temporarily to the to-be-deprecated Location.")
-            self.reference_type = Location
-        elif isinstance(reference_type, basestring):
-            module_path, _, class_name = reference_type.rpartition('.')
-            class_ = getattr(import_module(module_path), class_name)
-            self.reference_type = class_
-        else:
-            self.reference_type = reference_type
 
         if 'default' not in stores:
             raise Exception('Missing a default modulestore in the MixedModuleStore __init__ method.')
 
-        for key, store in stores.items():
+        for key, store in stores.iteritems():
             is_xml = 'XMLModuleStore' in store['ENGINE']
             if is_xml:
+                # restrict xml to only load courses in mapping
                 store['OPTIONS']['course_ids'] = [
                     course_id
                     for course_id, store_key in self.mappings.iteritems()
@@ -69,8 +52,8 @@ class MixedModuleStore(ModuleStoreWriteBase):
                 store['OPTIONS'],
                 i18n_service=i18n_service,
             )
-            # it would be better to have a notion of read-only rather than hardcode
-            # key name
+            # If and when locations can identify their course, we won't need
+            # these loc maps. They're needed for figuring out which store owns these locations.
             if is_xml:
                 self.ensure_loc_maps_exist(key)
 
@@ -85,148 +68,6 @@ class MixedModuleStore(ModuleStoreWriteBase):
         mapping = self.mappings.get(course_id, 'default')
         return self.modulestores[mapping]
 
-    # TODO move the location converters to a helper class which returns a converter object w/ 2
-    # methods: convert_xblock and convert_reference. Then have mixed get the converter and use it.
-    def _locator_to_location(self, reference):
-        """
-        Convert the referenced locator to a location casting to and from a string as necessary
-        """
-        stringify = isinstance(reference, basestring)
-        if stringify:
-            reference = BlockUsageLocator(url=reference)
-        location = loc_mapper().translate_locator_to_location(reference)
-        return location.url() if stringify else location
-
-    def _location_to_locator(self, course_id, reference):
-        """
-        Convert the referenced location to a locator casting to and from a string as necessary
-        """
-        stringify = isinstance(reference, basestring)
-        if stringify:
-            reference = Location(reference)
-        locator = loc_mapper().translate_location(course_id, reference, reference.revision == None, True)
-        return unicode(locator) if stringify else locator
-
-    def _incoming_reference_adaptor(self, store, course_id, reference):
-        """
-        Convert the reference to the type the persistence layer wants
-        """
-        if reference is None:
-            return None
-        if issubclass(store.reference_type, self.reference_type):
-            return reference
-        if store.reference_type == Location:
-            return self._locator_to_location(reference)
-        return self._location_to_locator(course_id, reference)
-
-    def _outgoing_reference_adaptor(self, store, course_id, reference):
-        """
-        Convert the reference to the type the application wants
-        """
-        if reference is None:
-            return None
-        if issubclass(store.reference_type, self.reference_type):
-            return reference
-        if store.reference_type == Location:
-            return self._location_to_locator(course_id, reference)
-        return self._locator_to_location(reference)
-
-    def _xblock_adaptor_iterator(self, adaptor, string_converter, store, course_id, xblock):
-        """
-        Change all reference fields in this xblock to the type expected by the receiving layer
-        """
-        xblock.location = adaptor(store, course_id, xblock.location)
-        for field in xblock.fields.itervalues():
-            if field.is_set_on(xblock):
-                if isinstance(field, Reference):
-                    field.write_to(
-                        xblock,
-                        adaptor(store, course_id, field.read_from(xblock))
-                    )
-                elif isinstance(field, ReferenceList):
-                    field.write_to(
-                        xblock,
-                        [
-                            adaptor(store, course_id, ref)
-                            for ref in field.read_from(xblock)
-                        ]
-                    )
-                elif isinstance(field, String):
-                    # replace links within the string
-                    string_converter(field, xblock)
-        return xblock
-
-    def _incoming_xblock_adaptor(self, store, course_id, xblock):
-        """
-        Change all reference fields in this xblock to the type expected by the persistence layer
-        """
-        string_converter = self._get_string_converter(
-            course_id, store.reference_type, xblock.scope_ids.usage_id
-        )
-        return self._xblock_adaptor_iterator(
-            self._incoming_reference_adaptor, string_converter, store, course_id, xblock
-        )
-
-    def _outgoing_xblock_adaptor(self, store, course_id, xblock):
-        """
-        Change all reference fields in this xblock to the type expected by the persistence layer
-        """
-        string_converter = self._get_string_converter(
-            course_id, xblock.scope_ids.usage_id.__class__, xblock.scope_ids.usage_id
-        )
-        return self._xblock_adaptor_iterator(
-            self._outgoing_reference_adaptor, string_converter, store, course_id, xblock
-        )
-
-    CONVERT_RE = re.compile(r"/jump_to_id/({}+)".format(ALLOWED_ID_CHARS))
-
-    def _get_string_converter(self, course_id, reference_type, from_base_addr):
-        """
-        Return a closure which finds and replaces all embedded links in a string field
-        with the correct rewritten link for the target type
-        """
-        if issubclass(self.reference_type, reference_type):
-            return lambda field, xblock: None
-        if isinstance(from_base_addr, Location):
-            def mapper(found_id):
-                """
-                Convert the found id to BlockUsageLocator block_id
-                """
-                location = from_base_addr.replace(category=None, name=found_id)
-                # NOTE without category, it cannot create a new mapping if there's not one already
-                return loc_mapper().translate_location(course_id, location).block_id
-        else:
-            def mapper(found_id):
-                """
-                Convert the found id to Location block_id
-                """
-                locator = BlockUsageLocator.make_relative(from_base_addr, found_id)
-                return loc_mapper().translate_locator_to_location(locator).name
-
-        def converter(field, xblock):
-            """
-            Find all of the ids in the block and replace them w/ their mapped values
-            """
-            value = field.read_from(xblock)
-            self.CONVERT_RE.sub(mapper, value)
-            field.write_to(xblock, value)
-        return converter
-
-    def ensure_loc_maps_exist(self, store_name):
-        """
-        Ensure location maps exist for every course in the modulestore whose
-        name is the given name (mostly used for 'xml'). It creates maps for any
-        missing ones.
-
-        NOTE: will only work if the given store is Location based. If it's not,
-        it raises NotImplementedError
-        """
-        store = self.modulestores[store_name]
-        if store.reference_type != Location:
-            raise NotImplementedError(u"Cannot create maps from %s", store.reference_type)
-        for course in store.get_courses():
-            loc_mapper().translate_location(course.location.course_id, course.location)
-
     def has_item(self, course_id, reference):
         """
         Does the course include the xblock who's id is reference?
@@ -235,21 +76,19 @@ class MixedModuleStore(ModuleStoreWriteBase):
         :param reference: a Location or BlockUsageLocator
         """
         store = self._get_modulestore_for_courseid(course_id)
-        decoded_ref = self._incoming_reference_adaptor(store, course_id, reference)
-        return store.has_item(course_id, decoded_ref)
+        return store.has_item(course_id, reference)
 
     def get_item(self, location, depth=0):
         """
         This method is explicitly not implemented as we need a course_id to disambiguate
         We should be able to fix this when the data-model rearchitecting is done
         """
+        # Although we shouldn't have both get_item and get_instance imho
         raise NotImplementedError
 
     def get_instance(self, course_id, location, depth=0):
         store = self._get_modulestore_for_courseid(course_id)
-        decoded_ref = self._incoming_reference_adaptor(store, course_id, location)
-        xblock = store.get_instance(course_id, decoded_ref, depth)
-        return self._outgoing_xblock_adaptor(store, course_id, xblock)
+        return store.get_instance(course_id, location, depth)
 
     def get_items(self, location, course_id=None, depth=0, qualifiers=None):
         """
@@ -262,44 +101,15 @@ class MixedModuleStore(ModuleStoreWriteBase):
         a Locator with at least a package_id and branch but possibly no block_id.
 
         depth: An argument that some module stores may use to prefetch
-            descendents of the queried modules for more efficient results later
+            descendants of the queried modules for more efficient results later
             in the request. The depth is counted in the number of calls to
-            get_children() to cache. None indicates to cache all descendents
+            get_children() to cache. None indicates to cache all descendants
         """
         if not (course_id or hasattr(location, 'package_id')):
             raise Exception("Must pass in a course_id when calling get_items()")
 
         store = self._get_modulestore_for_courseid(course_id or getattr(location, 'package_id'))
-        if not issubclass(self.reference_type, store.reference_type):
-            if store.reference_type == Location:
-                if getattr(location, 'block_id', False):
-                    location = self._incoming_reference_adaptor(store, course_id, location)
-                else:
-                    # get the course's location
-                    location = loc_mapper().translate_locator_to_location(location, get_course=True)
-                    # now remove the unknowns
-                    location = location.replace(
-                        category=qualifiers.get('category', None),
-                        name=None
-                    )
-            else:
-                if not isinstance(location, Location):
-                    location = Location(location)
-                try:
-                    location.ensure_fully_specified()
-                    location = loc_mapper().translate_location(
-                        course_id, location, location.revision == None, True
-                    )
-                except InsufficientSpecificationError:
-                    # construct the Locator by hand
-                    if location.category is not None and qualifiers.get('category', False):
-                        qualifiers['category'] = location.category
-                    location = loc_mapper().translate_location_to_course_locator(
-                        course_id, location, location.revision == None
-                    )
-        xblocks = store.get_items(location, course_id, depth, qualifiers)
-        xblocks = [self._outgoing_xblock_adaptor(store, course_id, xblock) for xblock in xblocks]
-        return xblocks
+        return store.get_items(location, course_id, depth, qualifiers)
 
     def _get_course_id_from_course_location(self, course_location):
         """
@@ -312,35 +122,39 @@ class MixedModuleStore(ModuleStoreWriteBase):
         Returns a list containing the top level XModuleDescriptors of the courses
         in this modulestore.
         '''
-        courses = []
-        for key in self.modulestores:
-            store = self.modulestores[key]
-            store_courses = store.get_courses()
-            # If the store has not been labeled as 'default' then we should
-            # only surface courses that have a mapping entry, for example the XMLModuleStore will
-            # slurp up anything that is on disk, however, we don't want to surface those to
-            # consumers *unless* there is an explicit mapping in the configuration
-            # TODO obviously this filtering only applies to filebased stores
-            if key != 'default':
-                for course in store_courses:
-                    course_id = self._get_course_id_from_course_location(course.location)
-                    # make sure that the courseId is mapped to the store in question
-                    if key == self.mappings.get(course_id, 'default'):
-                        courses.append(
-                            self._outgoing_reference_adaptor(store, course_id, course.location)
-                        )
-            else:
-                # if we're the 'default' store provider, then we surface all courses hosted in
-                # that store provider
-                store_courses = [
-                    self._outgoing_reference_adaptor(
-                        store, self._get_course_id_from_course_location(course.location), course.location
-                    )
-                    for course in store_courses
-                ]
-                courses = courses + store_courses
+        courses = {}
+        # order the modulestores and ensure no dupes: an awkward bit of hardcoding to ensure precedence
+        # xml is in here because mappings trump discovery
+        stores = [self.modulestores['default'], self.modulestores['xml']]
+        for key, store in self.modulestores.iteritems():
+            # awkward hardcoding of knowledge that 'draft' is a dupe of 'direct'
+            if key != 'draft' and store not in stores:
+                stores.append(store)
 
-        return courses
+        has_locators = False
+        for store in stores:
+            store_courses = store.get_courses()
+            # filter out ones which were fetched from earlier stores but locations may not be ==
+            for course in store_courses:
+                course_location = unicode(course.location)
+                if course_location not in courses:
+                    if has_locators and isinstance(course.location, Location):
+                        try:
+                            # if there's no existing mapping, then the course can't have been in split
+                            course_locator = loc_mapper().translate_location(
+                                course.location.course_id, course.location, add_entry_if_missing=False
+                            )
+                            if unicode(course_locator) not in courses:
+                                courses[course_location] = course
+                        except ItemNotFoundError:
+                            courses[course_location] = course
+                    elif isinstance(course.location, CourseLocator):
+                        has_locators = True
+                        courses[course_location] = course
+                    else:
+                        courses[course_location] = course
+
+        return courses.values()
 
     def get_course(self, course_id):
         """
@@ -350,46 +164,19 @@ class MixedModuleStore(ModuleStoreWriteBase):
         :param course_id: must be either a string course_id or a CourseLocator
         """
         store = self._get_modulestore_for_courseid(
-            course_id.package_id if hasattr(course_id, 'package_id') else course_id)
+            course_id.package_id if hasattr(course_id, 'package_id') else course_id
+        )
         try:
-            # translate won't work w/ missing fields so work around it
-            if store.reference_type == Location:
-                # takes the course_id: figure out if this is old or new style
-                if not issubclass(store.reference_type, self.reference_type):
-                    if isinstance(course_id, basestring):
-                        course_id = CourseLocator(package_id=course_id, branch='published')
-                    course_location = loc_mapper().translate_locator_to_location(course_id, get_course=True)
-                    course_id = course_location.course_id
-                xblock = store.get_course(course_id)
-            else:
-                # takes a courseLocator
-                if isinstance(course_id, CourseLocator):
-                    location = course_id
-                    course_id = None  # not an old style course_id; so, don't use it further
-                elif '/' in course_id:
-                    location = loc_mapper().translate_location_to_course_locator(course_id, None, True)
-                else:
-                    location = CourseLocator(package_id=course_id, branch='published')
-                    course_id = None  # not an old style course_id; so, don't use it further
-                xblock = store.get_course(location)
+            return store.get_course(course_id)
         except ItemNotFoundError:
             return None
-        if xblock is not None:
-            return self._outgoing_xblock_adaptor(store, course_id, xblock)
-        else:
-            return None
 
     def get_parent_locations(self, location, course_id):
         """
         returns the parent locations for a given location and course_id
         """
         store = self._get_modulestore_for_courseid(course_id)
-        decoded_ref = self._incoming_reference_adaptor(store, course_id, location)
-        parents = store.get_parent_locations(decoded_ref, course_id)
-        return [
-            self._outgoing_reference_adaptor(store, course_id, reference)
-            for reference in parents
-        ]
+        return store.get_parent_locations(location, course_id)
 
     def get_modulestore_type(self, course_id):
         """
@@ -409,8 +196,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
         """
         course_id = getattr(course_location, 'course_id', getattr(course_location, 'package_id', None))
         store = self._get_modulestore_for_courseid(course_id)
-        decoded_ref = self._incoming_reference_adaptor(store, course_id, course_location)
-        return store.get_orphans(decoded_ref, branch)
+        return store.get_orphans(course_location, branch)
 
     def get_errored_courses(self):
         """
@@ -426,13 +212,16 @@ class MixedModuleStore(ModuleStoreWriteBase):
         """
         Get the course_id from the block or from asking its store. Expensive.
         """
-        if block.course_id is not None:
-            return block.course_id
+        try:
+            if block.course_id is not None:
+                return block.course_id
+        except AssertionError:  # will occur if no xmodule set
+            pass
         try:
             course = store._get_course_for_item(block.scope_ids.usage_id)
             if course:
                 return course.scope_ids.usage_id.course_id
-        except:  # sorry, that method just raises vanilla Exception
+        except:  # sorry, that method just raises vanilla Exception if it doesn't find course
             pass
 
     def _infer_course_id_try(self, location):
@@ -442,10 +231,27 @@ class MixedModuleStore(ModuleStoreWriteBase):
         proper modulestore. This method attempts several sound but not complete methods.
         :param location: an old style Location
         """
+        if isinstance(location, CourseLocator):
+            return location.package_id
+        elif isinstance(location, basestring):
+            try:
+                location = Location(location)
+            except InvalidLocationError:
+                # try to parse as a course_id
+                try:
+                    Location.parse_course_id(location)
+                    # it's already a course_id
+                    return location
+                except ValueError:
+                    # cannot interpret the location
+                    return None
+
+        # location is a Location at this point
         if location.category == 'course':  # easiest case
             return location.course_id
         # try finding in loc_mapper
         try:
+            # see if the loc mapper knows the course id (requires double translation)
             locator = loc_mapper().translate_location_to_course_locator(None, location)
             location = loc_mapper().translate_locator_to_location(locator, get_course=True)
             return location.course_id
@@ -455,16 +261,19 @@ class MixedModuleStore(ModuleStoreWriteBase):
         for store in self.modulestores.itervalues():
             if isinstance(location, store.reference_type):
                 try:
-                    block = store.get_item(location)
-                    course_id = self._get_course_id_from_block(block, store)
+                    xblock = store.get_item(location)
+                    course_id = self._get_course_id_from_block(xblock, store)
                     if course_id is not None:
                         return course_id
                 except NotImplementedError:
                     blocks = store.get_items(location)
                     if len(blocks) == 1:
                         block = blocks[0]
-                        if block.course_id is not None:
-                            return block.course_id
+                        try:
+                            if block.course_id is not None:
+                                return block.course_id
+                        except AssertionError:
+                            pass
                 except ItemNotFoundError:
                     pass
         # if we get here, it must be in a Locator based store, but we won't be able to find
@@ -481,19 +290,20 @@ class MixedModuleStore(ModuleStoreWriteBase):
 
         NOTE: unlike the other mixed modulestore methods, this does not adapt its argument
         to the persistence store but requires its caller to know what the persistence store
-        wants for args. It does not translate any references on the way in; so, don't
-        pass children or other reference fields here.
-        It does, however, adapt the xblock on the way out to the app's
-        reference_type
+        wants for args.
 
         :returns: course xblock
         """
         store = self.modulestores[store_name]
         if not hasattr(store, 'create_course'):
-            raise NotImplementedError(u"Cannot create a course on store %s", store_name)
+            raise NotImplementedError(u"Cannot create a course on store %s" % store_name)
         if store.get_modulestore_type(course_location.course_id) == SPLIT_MONGO_MODULESTORE_TYPE:
             org = kwargs.pop('org', course_location.org)
             pretty_id = kwargs.pop('pretty_id', None)
+            fields = kwargs.get('fields', {})
+            fields.update(kwargs.pop('metadata', {}))
+            fields.update(kwargs.pop('definition_data', {}))
+            kwargs['fields'] = fields
             # TODO rename id_root to package_id for consistency. It's too confusing
             id_root = kwargs.pop('id_root', u"{0.org}.{0.course}.{0.name}".format(course_location))
             course = store.create_course(
@@ -509,7 +319,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
             course = store.create_course(course_location, **kwargs)
             loc_mapper().translate_location(course_location.course_id, course_location)
 
-        return self._outgoing_xblock_adaptor(store, course_location.course_id, course)
+        return course
 
     def create_item(self, course_or_parent_loc, category, user_id=None, **kwargs):
         """
@@ -520,22 +330,17 @@ class MixedModuleStore(ModuleStoreWriteBase):
         Adds an entry to the loc map using the kwarg location if provided (must be a
         Location if provided) or block_id and category if provided.
 
-        :param course_or_parent_loc: will be translated appropriately to the course's store.
-        Can be a course_id (org/course/run), CourseLocator, Location, or BlockUsageLocator.
+        :param course_or_parent_loc: Can be a course_id (org/course/run), CourseLocator,
+        Location, or BlockUsageLocator but must be what the persistence modulestore expects
         """
         # find the store for the course
-        if self.reference_type == Location:
-            if hasattr(course_or_parent_loc, 'tag'):
-                course_id = self._infer_course_id_try(course_or_parent_loc)
-            else:
-                course_id = course_or_parent_loc
-        else:
-            course_id = course_or_parent_loc.package_id
+        course_id = self._infer_course_id_try(course_or_parent_loc)
+
         store = self._get_modulestore_for_courseid(course_id)
 
         location = kwargs.pop('location', None)
         # invoke its create_item
-        if store.reference_type == Location:
+        if isinstance(store, MongoModuleStore):
             # convert parent loc if it's legit
             block_id = kwargs.pop('block_id', uuid4().hex)
             if isinstance(course_or_parent_loc, basestring):
@@ -546,7 +351,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
                     locn_dict['name'] = block_id
                     location = Location(locn_dict)
             else:
-                parent_loc = self._incoming_reference_adaptor(store, course_id, course_or_parent_loc)
+                parent_loc = course_or_parent_loc
                 # must have a legitimate location, compute if appropriate
                 if location is None:
                     location = parent_loc.replace(category=category, name=block_id)
@@ -559,7 +364,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
                 parent = store.get_item(parent_loc)
                 parent.children.append(location.url())
                 store.update_item(parent)
-        else:
+        elif isinstance(store, SplitMongoModuleStore):
             if isinstance(course_or_parent_loc, basestring): # course_id
                 old_course_id = course_or_parent_loc
                 course_or_parent_loc = loc_mapper().translate_location_to_course_locator(
@@ -570,13 +375,15 @@ class MixedModuleStore(ModuleStoreWriteBase):
                     course_or_parent_loc, get_course=True
                 )
                 old_course_id = old_course_loc.course_id
-            else:  # it's a Location
-                old_course_id = course_id
-                course_or_parent_loc = self._location_to_locator(course_id, course_or_parent_loc)
+            else:
+                raise ValueError(u"Cannot create a child of {} in split. Wrong repr.".format(course_or_parent_loc))
+
+            # split handles all the fields in one dict not separated by scope
             fields = kwargs.get('fields', {})
             fields.update(kwargs.pop('metadata', {}))
             fields.update(kwargs.pop('definition_data', {}))
             kwargs['fields'] = fields
+
             xblock = store.create_item(course_or_parent_loc, category, user_id, **kwargs)
             if location is None:
                 locn_dict = Location.parse_course_id(old_course_id)
@@ -587,6 +394,9 @@ class MixedModuleStore(ModuleStoreWriteBase):
             loc_mapper().translate_location(
                 old_course_id, location, passed_block_id=xblock.location.block_id
             )
+        else:
+            raise NotImplementedError(u"Cannot create an item on store %s" % store)
+
         return xblock
 
     def update_item(self, xblock, user_id, allow_not_found=False):
@@ -594,37 +404,21 @@ class MixedModuleStore(ModuleStoreWriteBase):
         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.
         """
-        if self.reference_type == Location:
-            course_id = xblock.course_id
-            if course_id is None:
-                course_id = self._infer_course_id_try(xblock.scope_ids.usage_id)
-                if course_id is None:
-                    raise ItemNotFoundError(u"Cannot find modulestore for %s", xblock.scope_ids.usage_id)
-        else:
-            locator = xblock.scope_ids.usage_id
-            course_id = locator.package_id
+        course_id = self._infer_course_id_try(xblock.scope_ids.usage_id)
+        if course_id is None:
+            raise ItemNotFoundError(u"Cannot find modulestore for %s" % xblock.scope_ids.usage_id)
         store = self._get_modulestore_for_courseid(course_id)
-
-        # if an xblock, convert its contents to correct addr scheme
-        xblock = self._incoming_xblock_adaptor(store, course_id, xblock)
-        xblock = store.update_item(xblock, user_id)
-
-        return self._outgoing_xblock_adaptor(store, course_id, xblock)
+        return store.update_item(xblock, user_id)
 
     def delete_item(self, location, user_id=None):
         """
         Delete the given item from persistence.
         """
-        if self.reference_type == Location:
-            course_id = self._infer_course_id_try(location)
-            if course_id is None:
-                raise ItemNotFoundError(u"Cannot find modulestore for %s", location)
-        else:
-            course_id = location.package_id
+        course_id = self._infer_course_id_try(location)
+        if course_id is None:
+            raise ItemNotFoundError(u"Cannot find modulestore for %s" % location)
         store = self._get_modulestore_for_courseid(course_id)
-
-        decoded_ref = self._incoming_reference_adaptor(store, course_id, location)
-        return store.delete_item(decoded_ref, user_id=user_id)
+        return store.delete_item(location, user_id=user_id)
 
     def close_all_connections(self):
         """
@@ -636,3 +430,17 @@ class MixedModuleStore(ModuleStoreWriteBase):
             elif hasattr(mstore, 'db'):
                 mstore.db.connection.close()
 
+    def ensure_loc_maps_exist(self, store_name):
+        """
+        Ensure location maps exist for every course in the modulestore whose
+        name is the given name (mostly used for 'xml'). It creates maps for any
+        missing ones.
+
+        NOTE: will only work if the given store is Location based. If it's not,
+        it raises NotImplementedError
+        """
+        store = self.modulestores[store_name]
+        if store.reference_type != Location:
+            raise NotImplementedError(u"Cannot create maps from %s" % store.reference_type)
+        for course in store.get_courses():
+            loc_mapper().translate_location(course.location.course_id, course.location)
diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py
index 4dcf3b6739..b97b1e1686 100644
--- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py
+++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py
@@ -633,8 +633,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
             raise ValueError(u"Course roots must be of category 'course': {}".format(unicode(location)))
         return self.create_and_save_xmodule(location, definition_data, metadata, runtime)
 
-    def create_xmodule(self, location, definition_data=None, metadata=None, system=None,
-                       fields={}):
+    def create_xmodule(self, location, definition_data=None, metadata=None, system=None, fields={}):
         """
         Create the new xmodule but don't save it. Returns the new module.
 
diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py
index 99a10c6e4a..c8ce76e1ec 100644
--- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py
+++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py
@@ -92,7 +92,7 @@ class DraftModuleStore(MongoModuleStore):
         except ItemNotFoundError:
             return wrap_draft(super(DraftModuleStore, self).get_instance(course_id, location, depth=depth))
 
-    def create_xmodule(self, location, definition_data=None, metadata=None, system=None):
+    def create_xmodule(self, location, definition_data=None, metadata=None, system=None, fields={}):
         """
         Create the new xmodule but don't save it. Returns the new module with a draft locator
 
@@ -104,7 +104,7 @@ class DraftModuleStore(MongoModuleStore):
         draft_loc = as_draft(location)
         if draft_loc.category in DIRECT_ONLY_CATEGORIES:
             raise InvalidVersionError(location)
-        return super(DraftModuleStore, self).create_xmodule(draft_loc, definition_data, metadata, system)
+        return super(DraftModuleStore, self).create_xmodule(draft_loc, definition_data, metadata, system, fields)
 
     def get_items(self, location, course_id=None, depth=0, qualifiers=None):
         """
diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py
index 9ac0533eb9..f48a68632f 100644
--- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py
+++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py
@@ -556,8 +556,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         The block's history tracks its explicit changes but not the changes in its children.
 
         '''
-        # version_agnostic means we don't care if the head and version don't align, trust the version
-        course_struct = self._lookup_course(block_locator.version_agnostic())['structure']
+        # course_agnostic means we don't care if the head and version don't align, trust the version
+        course_struct = self._lookup_course(block_locator.course_agnostic())['structure']
         block_id = block_locator.block_id
         update_version_field = 'blocks.{}.edit_info.update_version'.format(block_id)
         all_versions_with_block = self.db_connection.find_matching_structures({'original_version': course_struct['original_version'],
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py
index ef30e96add..96603ee607 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py
@@ -33,7 +33,6 @@ def mixed_store_config(data_dir, mappings):
             'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
             'OPTIONS': {
                 'mappings': mappings,
-                'reference_type': 'xmodule.modulestore.Location',
                 'stores': {
                     'default': mongo_config['default'],
                     'xml': xml_config['default']
@@ -219,14 +218,17 @@ class ModuleStoreTestCase(TestCase):
         # even if we're using a mixed modulestore
         store = editable_modulestore()
         if hasattr(store, 'collection'):
+            connection = store.collection.database.connection
             store.collection.drop()
-            store.db.connection.close()
+            connection.close()
         elif hasattr(store, 'close_all_connections'):
             store.close_all_connections()
+
         if contentstore().fs_files:
             db = contentstore().fs_files.database
             db.connection.drop_database(db)
             db.connection.close()
+
         location_mapper = loc_mapper()
         if location_mapper.db:
             location_mapper.location_map.drop()
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py
index 047ee65dbb..cc275f91a2 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py
@@ -200,13 +200,14 @@ class LocatorTest(TestCase):
         expected_id = 'mit.eecs.6002x'
         expected_branch = 'published'
         expected_block_ref = 'HW3'
-        testobj = BlockUsageLocator(package_id=testurn)
+        testobj = BlockUsageLocator(url=testurn)
         self.check_block_locn_fields(testobj, 'test_block constructor',
                                      package_id=expected_id,
                                      branch=expected_branch,
                                      block=expected_block_ref)
         self.assertEqual(str(testobj), testurn)
         self.assertEqual(testobj.url(), 'edx://' + testurn)
+        testobj = BlockUsageLocator(url=testurn, version_guid=ObjectId())
         agnostic = testobj.version_agnostic()
         self.assertIsNone(agnostic.version_guid)
         self.check_block_locn_fields(agnostic, 'test_block constructor',
@@ -225,7 +226,7 @@ class LocatorTest(TestCase):
             block='lab2',
             version_guid=ObjectId(test_id_loc)
         )
-        agnostic = testobj.version_agnostic()
+        agnostic = testobj.course_agnostic()
         self.check_block_locn_fields(
             agnostic, 'error parsing URL with version and block',
             block='lab2',
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
index 81ebd70a33..39073e4952 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
@@ -1,24 +1,24 @@
 import pymongo
 from uuid import uuid4
-import copy
 import ddt
-from mock import patch
+from mock import patch, Mock
+from importlib import import_module
 
 from xmodule.tests import DATA_DIR
 from xmodule.modulestore import Location, MONGO_MODULESTORE_TYPE, SPLIT_MONGO_MODULESTORE_TYPE, \
     XML_MODULESTORE_TYPE
 from xmodule.modulestore.exceptions import ItemNotFoundError
 
-from xmodule.modulestore.locator import BlockUsageLocator
+from xmodule.modulestore.locator import BlockUsageLocator, CourseLocator
 from xmodule.modulestore.tests.test_location_mapper import LocMapperSetupSansDjango, loc_mapper
-
-# FIXME remove settings
+# Mixed modulestore depends on django, so we'll manually configure some django settings
+# before importing the module
 from django.conf import settings
 if not settings.configured:
     settings.configure()
-
 from xmodule.modulestore.mixed import MixedModuleStore
 
+
 @ddt.ddt
 class TestMixedModuleStore(LocMapperSetupSansDjango):
     """
@@ -32,7 +32,6 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
     FS_ROOT = DATA_DIR
     DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor'
     RENDER_TEMPLATE = lambda t_n, d, ctx = None, nsp = 'main': ''
-    REFERENCE_TYPE = 'xmodule.modulestore.Location'
 
     IMPORT_COURSEID = 'MITx/999/2013_Spring'
     XML_COURSEID1 = 'edX/toy/2012_Fall'
@@ -54,7 +53,6 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
             XML_COURSEID2: 'xml',
             IMPORT_COURSEID: 'default'
         },
-        'reference_type': REFERENCE_TYPE,
         'stores': {
             'xml': {
                 'ENGINE': 'xmodule.modulestore.xml.XMLModuleStore',
@@ -101,10 +99,13 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         self.connection.drop_database(self.DB)
         self.addCleanup(self.connection.drop_database, self.DB)
         self.addCleanup(self.connection.close)
-        
         super(TestMixedModuleStore, self).setUp()
 
-        patcher = patch('xmodule.modulestore.mixed.loc_mapper', return_value=LocMapperSetupSansDjango.loc_store)
+        patcher = patch.multiple(
+            'xmodule.modulestore.mixed',
+            loc_mapper=Mock(return_value=LocMapperSetupSansDjango.loc_store),
+            create_modulestore_instance=create_modulestore_instance,
+        )
         patcher.start()
         self.addCleanup(patcher.stop)
         self.addTypeEqualityFunc(BlockUsageLocator, '_compareIgnoreVersion')
@@ -115,28 +116,26 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         NOTE: course_location and item_location must be Location regardless of the app reference type in order
         to cause the right mapping to be created.
         """
-        if default == 'split':
-            course = self.store.create_course(course_location, store_name=default)
-            chapter = self.store.create_item(
-                # don't use course_location as it may not be the repr
-                course.location, item_location.category, location=item_location, block_id=item_location.name
-            )
-        else:
-            course = self.store.create_course(
-                course_location, store_name=default, metadata={'display_name': course_location.name}
-            )
-            chapter = self.store.create_item(course_location, item_location.category, location=item_location)
-        if self.REFERENCE_TYPE == 'xmodule.modulestore.locator.CourseLocator':
+        course = self.store.create_course(
+            course_location, store_name=default, metadata={'display_name': course_location.name}
+        )
+        chapter = self.store.create_item(
+            # don't use course_location as it may not be the repr
+            course.location, item_location.category, location=item_location, block_id=item_location.name
+        )
+        if isinstance(course.location, CourseLocator):
             # add_entry is false b/c this is a test that the right thing happened w/o
             # wanting any additional side effects
             lookup_map = loc_mapper().translate_location(
                 course_location.course_id, course_location, add_entry_if_missing=False
             )
             self.assertEqual(lookup_map, course.location)
+            self.course_locations[self.IMPORT_COURSEID] = course.location.version_agnostic()
             lookup_map = loc_mapper().translate_location(
                 course_location.course_id, item_location, add_entry_if_missing=False
             )
             self.assertEqual(lookup_map, chapter.location)
+            self.import_chapter_location = chapter.location.version_agnostic()
         else:
             self.assertEqual(course.location, course_location)
             self.assertEqual(chapter.location, item_location)
@@ -154,8 +153,10 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
             """
             Generate the locations for the given ids
             """
-            org, course, run = course_id.split('/')
-            return Location('i4x', org, course, 'course', run)
+            course_dict = Location.parse_course_id(course_id)
+            course_dict['tag'] = 'i4x'
+            course_dict['category'] = 'course'
+            return Location(course_dict)
 
         self.course_locations = {
              course_id: generate_location(course_id)
@@ -171,18 +172,8 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         # grab old style location b4 possibly converted
         import_location = self.course_locations[self.IMPORT_COURSEID]
         # get Locators and set up the loc mapper if app is Locator based
-        if self.REFERENCE_TYPE == 'xmodule.modulestore.locator.CourseLocator':
+        if default == 'split':
             self.fake_location = loc_mapper().translate_location('foo/bar/2012_Fall', self.fake_location)
-            self.import_chapter_location = loc_mapper().translate_location(
-                self.IMPORT_COURSEID, self.import_chapter_location
-            )
-            self.xml_chapter_location = loc_mapper().translate_location(
-                self.XML_COURSEID1, self.xml_chapter_location
-            )
-            self.course_locations = {
-                course_id: loc_mapper().translate_location(course_id, locn)
-                for course_id, locn in self.course_locations.iteritems()
-            }
 
         self._create_course(default, import_location, self.import_chapter_location)
 
@@ -206,8 +197,11 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
             self.assertTrue(self.store.has_item(course_id, course_locn))
 
         # try negative cases
-        self.assertFalse(self.store.has_item(self.XML_COURSEID1, self.course_locations[self.IMPORT_COURSEID]))
-        self.assertFalse(self.store.has_item(self.IMPORT_COURSEID, self.course_locations[self.XML_COURSEID1]))
+        self.assertFalse(self.store.has_item(
+                self.XML_COURSEID1,
+                self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem')
+        ))
+        self.assertFalse(self.store.has_item(self.IMPORT_COURSEID, self.fake_location))
 
     @ddt.data('direct', 'split')
     def test_get_item(self, default_ms):
@@ -223,9 +217,12 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
 
         # try negative cases
         with self.assertRaises(ItemNotFoundError):
-            self.store.get_instance(self.XML_COURSEID1, self.course_locations[self.IMPORT_COURSEID])
+            self.store.get_instance(
+                self.XML_COURSEID1,
+                self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem')
+            )
         with self.assertRaises(ItemNotFoundError):
-            self.store.get_instance(self.IMPORT_COURSEID, self.course_locations[self.XML_COURSEID1])
+            self.store.get_instance(self.IMPORT_COURSEID, self.fake_location)
 
     @ddt.data('direct', 'split')
     def test_get_items(self, default_ms):
@@ -246,11 +243,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         Update should fail for r/o dbs and succeed for r/w ones
         """
         self.initdb(default_ms)
-        # try a r/o db
-        if self.REFERENCE_TYPE == 'xmodule.modulestore.locator.CourseLocator':
-            course_id = self.course_locations[self.XML_COURSEID1]
-        else:
-            course_id = self.XML_COURSEID1
+        course_id = self.XML_COURSEID1
         course = self.store.get_course(course_id)
         # if following raised, then the test is really a noop, change it
         self.assertFalse(course.show_calculator, "Default changed making test meaningless")
@@ -288,10 +281,14 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
     @ddt.data('direct', 'split')
     def test_get_courses(self, default_ms):
         self.initdb(default_ms)
-        # we should have 3 total courses aggregated
+        # we should have 3 total courses across all stores
         courses = self.store.get_courses()
         self.assertEqual(len(courses), 3)
-        course_ids = [course.location for course in courses]
+        course_ids = [
+            course.location.version_agnostic()
+            if hasattr(course.location, 'version_agnostic') else course.location
+            for course in courses
+        ]
         self.assertIn(self.course_locations[self.IMPORT_COURSEID], course_ids)
         self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids)
         self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids)
@@ -300,6 +297,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         """
         Test that the xml modulestore only loaded the courses from the maps.
         """
+        self.initdb('direct')
         courses = self.store.modulestores['xml'].get_courses()
         self.assertEqual(len(courses), 2)
         course_ids = [course.location.course_id for course in courses]
@@ -339,33 +337,28 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         self.assertEqual(len(parents), 1)
         self.assertEqual(parents[0], self.course_locations[self.XML_COURSEID1])
 
-@ddt.ddt
-class TestMixedUseLocator(TestMixedModuleStore):
-    """
-    Tests a mixed ms which uses Locators instead of Locations
-    """
-    REFERENCE_TYPE = 'xmodule.modulestore.locator.CourseLocator'
+#=============================================================================================================
+# General utils for not using django settings
+#=============================================================================================================
 
-    def setUp(self):
-        self.options = copy.copy(self.OPTIONS)
-        self.options['reference_type'] = self.REFERENCE_TYPE
-        super(TestMixedUseLocator, self).setUp()
-
-@ddt.ddt
-class TestMixedMSInit(TestMixedModuleStore):
+def load_function(path):
     """
-    Test initializing w/o a reference_type
-    """
-    REFERENCE_TYPE = None
-    def setUp(self):
-        self.options = copy.copy(self.OPTIONS)
-        del self.options['reference_type']
-        super(TestMixedMSInit, self).setUp()
+    Load a function by name.
 
-    @ddt.data('direct', 'split')
-    def test_use_locations(self, default_ms):
-        """
-        Test that use_locations defaulted correctly
-        """
-        self.initdb(default_ms)
-        self.assertEqual(self.store.reference_type, Location)
+    path is a string of the form "path.to.module.function"
+    returns the imported python object `function` from `path.to.module`
+    """
+    module_path, _, name = path.rpartition('.')
+    return getattr(import_module(module_path), name)
+
+
+def create_modulestore_instance(engine, doc_store_config, options, i18n_service=None):
+    """
+    This will return a new instance of a modulestore given an engine and options
+    """
+    class_ = load_function(engine)
+
+    return class_(
+        doc_store_config=doc_store_config,
+        **options
+    )
diff --git a/lms/envs/acceptance.py b/lms/envs/acceptance.py
index eeafd783ea..c5cacabc3b 100644
--- a/lms/envs/acceptance.py
+++ b/lms/envs/acceptance.py
@@ -45,7 +45,6 @@ MODULESTORE = {
         'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
         'OPTIONS': {
             'mappings': {},
-            'reference_type': 'xmodule.modulestore.Location',
             'stores': {
                 'default': {
                     'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',
diff --git a/lms/envs/cms/dev.py b/lms/envs/cms/dev.py
index 57b362ce52..440e74a5c9 100644
--- a/lms/envs/cms/dev.py
+++ b/lms/envs/cms/dev.py
@@ -32,7 +32,6 @@ MODULESTORE = {
     'default': {
         'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
         'OPTIONS': {
-            'reference_type': 'Location',
             'mappings': {},
             'stores': {
                 'default': {

From c9285de4335899bcca4fc1750db8ea6ec808873e Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Wed, 26 Feb 2014 16:37:39 -0500
Subject: [PATCH 03/15] unicode correctly and understandably

---
 common/lib/xmodule/xmodule/modulestore/locator.py | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py
index b76ca4861a..2ba86da43c 100644
--- a/common/lib/xmodule/xmodule/modulestore/locator.py
+++ b/common/lib/xmodule/xmodule/modulestore/locator.py
@@ -204,16 +204,14 @@ class CourseLocator(Locator):
         """
         Return a string representing this location.
         """
-        result = u""
+        parts = []
         if self.package_id:
-            result += unicode(self.package_id)
+            parts.append(unicode(self.package_id))
             if self.branch:
-                result += '/' + BRANCH_PREFIX + self.branch
+                parts.append(u"{prefix}{branch}".format(prefix=BRANCH_PREFIX, branch=self.branch))
         if self.version_guid:
-            if self.package_id:
-                result += u"/"
-            result += u"{prefix}{guid}".format(prefix=VERSION_PREFIX, guid=self.version_guid)
-        return result
+            parts.append(u"{prefix}{guid}".format(prefix=VERSION_PREFIX, guid=self.version_guid))
+        return u"/".join(parts)
 
     def url(self):
         """

From f371c4f5ae39f334660f7e182368d5ee1329bc7c Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Wed, 26 Feb 2014 16:42:20 -0500
Subject: [PATCH 04/15] Pass user id to update_item

---
 cms/djangoapps/contentstore/views/component.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py
index 4efc388d08..f2ec685730 100644
--- a/cms/djangoapps/contentstore/views/component.py
+++ b/cms/djangoapps/contentstore/views/component.py
@@ -23,7 +23,6 @@ from xblock.exceptions import NoSuchHandlerError
 from xblock.fields import Scope
 from xblock.plugin import PluginMissingError
 from xblock.runtime import Mixologist
-from xmodule.modulestore import prefer_xmodules
 
 from lms.lib.xblock.runtime import unquote_slashes
 
@@ -370,6 +369,11 @@ def component_handler(request, usage_id, handler, suffix=''):
         log.info("XBlock %s attempted to access missing handler %r", descriptor, handler, exc_info=True)
         raise Http404
 
-    modulestore().update_item(descriptor)
+    # unintentional update to handle any side effects of handle call; so, request user didn't author
+    # the change
+    if getattr(descriptor, 'is_draft', False):
+        modulestore('draft').update_item(descriptor, None)
+    else:
+        modulestore('direct').update_item(descriptor, None)
 
     return webob_to_django_response(resp)

From 43df25d42b4b40a94b227085445d7b3741800c2d Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Thu, 27 Feb 2014 10:23:17 -0500
Subject: [PATCH 05/15] Point to prefer_xmodules new locn

---
 common/lib/xmodule/xmodule/modulestore/mixed.py | 10 +++++++---
 lms/envs/bok_choy.py                            |  2 +-
 lms/envs/common.py                              |  2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py
index 663552b98c..95d10dcb64 100644
--- a/common/lib/xmodule/xmodule/modulestore/mixed.py
+++ b/common/lib/xmodule/xmodule/modulestore/mixed.py
@@ -122,10 +122,14 @@ class MixedModuleStore(ModuleStoreWriteBase):
         Returns a list containing the top level XModuleDescriptors of the courses
         in this modulestore.
         '''
-        courses = {}
+        courses = {}  # a dictionary of stringified course locations to course objects
         # order the modulestores and ensure no dupes: an awkward bit of hardcoding to ensure precedence
         # xml is in here because mappings trump discovery
-        stores = [self.modulestores['default'], self.modulestores['xml']]
+        if self.modulestores.has_key('xml'):
+            stores = [self.modulestores['default'], self.modulestores['xml']]
+        else:
+            stores = [self.modulestores['default']]
+
         for key, store in self.modulestores.iteritems():
             # awkward hardcoding of knowledge that 'draft' is a dupe of 'direct'
             if key != 'draft' and store not in stores:
@@ -194,7 +198,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
         usually orphaned. NOTE: may include xblocks which still have references via xblocks which don't
         use children to point to their dependents.
         """
-        course_id = getattr(course_location, 'course_id', getattr(course_location, 'package_id', None))
+        course_id = self._get_course_id_from_course_location(course_location)
         store = self._get_modulestore_for_courseid(course_id)
         return store.get_orphans(course_location, branch)
 
diff --git a/lms/envs/bok_choy.py b/lms/envs/bok_choy.py
index ae8f5bacf0..afeb08d1d5 100644
--- a/lms/envs/bok_choy.py
+++ b/lms/envs/bok_choy.py
@@ -4,7 +4,7 @@ Settings for bok choy tests
 
 import os
 from path import path
-from xmodule.x_module import prefer_xmodules
+from xmodule.modulestore import prefer_xmodules
 
 
 CONFIG_ROOT = path(__file__).abspath().dirname()  #pylint: disable=E1120
diff --git a/lms/envs/common.py b/lms/envs/common.py
index 1bf49cd9ff..1908ef71d8 100644
--- a/lms/envs/common.py
+++ b/lms/envs/common.py
@@ -438,7 +438,7 @@ XBLOCK_SELECT_FUNCTION = only_xmodules
 
 # Use the following lines to allow any xblock in the LMS,
 # either by uncommenting them here, or adding them to your private.py
-# from xmodule.x_module import prefer_xmodules
+# from xmodule.modulestore import prefer_xmodules
 # XBLOCK_SELECT_FUNCTION = prefer_xmodules
 
 #################### Python sandbox ############################################

From 371ecb08e87f0ded187d5ae9f1e36bf642e3b98c Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Thu, 27 Feb 2014 17:11:05 -0500
Subject: [PATCH 06/15] Better sorting of stores for get_courses May apply to
 all store ordering

---
 .../lib/xmodule/xmodule/modulestore/mixed.py  | 54 ++++++++++++-------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py
index 95d10dcb64..92a577a686 100644
--- a/common/lib/xmodule/xmodule/modulestore/mixed.py
+++ b/common/lib/xmodule/xmodule/modulestore/mixed.py
@@ -9,8 +9,8 @@ import logging
 
 from . import ModuleStoreWriteBase
 from xmodule.modulestore.django import create_modulestore_instance, loc_mapper
-from xmodule.modulestore import Location, SPLIT_MONGO_MODULESTORE_TYPE
-from xmodule.modulestore.locator import CourseLocator
+from xmodule.modulestore import Location, SPLIT_MONGO_MODULESTORE_TYPE, XML_MODULESTORE_TYPE
+from xmodule.modulestore.locator import CourseLocator, Locator
 from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError
 from uuid import uuid4
 from xmodule.modulestore.mongo.base import MongoModuleStore
@@ -122,20 +122,13 @@ class MixedModuleStore(ModuleStoreWriteBase):
         Returns a list containing the top level XModuleDescriptors of the courses
         in this modulestore.
         '''
+        # order the modulestores and ensure no dupes (default may be a dupe of a named store)
+        # remove 'draft' as we know it's a functional dupe of 'direct' (ugly hardcoding)
+        stores = set([value for key, value in self.modulestores.iteritems() if key != 'draft'])
+        stores = sorted(stores, _compare_stores)
+
         courses = {}  # a dictionary of stringified course locations to course objects
-        # order the modulestores and ensure no dupes: an awkward bit of hardcoding to ensure precedence
-        # xml is in here because mappings trump discovery
-        if self.modulestores.has_key('xml'):
-            stores = [self.modulestores['default'], self.modulestores['xml']]
-        else:
-            stores = [self.modulestores['default']]
-
-        for key, store in self.modulestores.iteritems():
-            # awkward hardcoding of knowledge that 'draft' is a dupe of 'direct'
-            if key != 'draft' and store not in stores:
-                stores.append(store)
-
-        has_locators = False
+        has_locators = any(issubclass(CourseLocator, store.reference_type) for store in stores)
         for store in stores:
             store_courses = store.get_courses()
             # filter out ones which were fetched from earlier stores but locations may not be ==
@@ -143,6 +136,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
                 course_location = unicode(course.location)
                 if course_location not in courses:
                     if has_locators and isinstance(course.location, Location):
+                        # see if a locator version of course is in the result
                         try:
                             # if there's no existing mapping, then the course can't have been in split
                             course_locator = loc_mapper().translate_location(
@@ -152,9 +146,6 @@ class MixedModuleStore(ModuleStoreWriteBase):
                                 courses[course_location] = course
                         except ItemNotFoundError:
                             courses[course_location] = course
-                    elif isinstance(course.location, CourseLocator):
-                        has_locators = True
-                        courses[course_location] = course
                     else:
                         courses[course_location] = course
 
@@ -448,3 +439,30 @@ class MixedModuleStore(ModuleStoreWriteBase):
             raise NotImplementedError(u"Cannot create maps from %s" % store.reference_type)
         for course in store.get_courses():
             loc_mapper().translate_location(course.location.course_id, course.location)
+
+
+def _compare_stores(alpha, aleph):
+    """
+    Order stores via precedence: if a course is found in an earlier store, it shadows the later store.
+
+    xml stores take precedence b/c they only contain hardcoded mappings, then Locator-based ones,
+    then others. Locators before Locations because if some courses may be in both,
+    the ones in the Locator-based stores shadow the others.
+    """
+    if alpha.get_modulestore_type(None) == XML_MODULESTORE_TYPE:
+        if aleph.get_modulestore_type(None) == XML_MODULESTORE_TYPE:
+            return 0
+        else:
+            return -1
+    elif aleph.get_modulestore_type(None) == XML_MODULESTORE_TYPE:
+        return 1
+
+    if issubclass(alpha.reference_type, Locator):
+        if issubclass(aleph.reference_type, Locator):
+            return 0
+        else:
+            return -1
+    elif issubclass(aleph.reference_type, Locator):
+        return 1
+
+    return 0

From aab5e0441b9bef5ded7e038884e057fc8e9c2c4d Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Fri, 28 Feb 2014 15:32:01 -0500
Subject: [PATCH 07/15] Mixed modulestore handles most modulestore functions,
 cont.

---
 .../lib/xmodule/xmodule/modulestore/mixed.py  |  8 ++--
 .../xmodule/xmodule/modulestore/mongo/base.py |  2 +-
 .../tests/test_mixed_modulestore.py           | 42 +++++++------------
 3 files changed, 21 insertions(+), 31 deletions(-)

diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py
index 92a577a686..03fd14b489 100644
--- a/common/lib/xmodule/xmodule/modulestore/mixed.py
+++ b/common/lib/xmodule/xmodule/modulestore/mixed.py
@@ -125,7 +125,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
         # order the modulestores and ensure no dupes (default may be a dupe of a named store)
         # remove 'draft' as we know it's a functional dupe of 'direct' (ugly hardcoding)
         stores = set([value for key, value in self.modulestores.iteritems() if key != 'draft'])
-        stores = sorted(stores, _compare_stores)
+        stores = sorted(stores, cmp=_compare_stores)
 
         courses = {}  # a dictionary of stringified course locations to course objects
         has_locators = any(issubclass(CourseLocator, store.reference_type) for store in stores)
@@ -214,9 +214,9 @@ class MixedModuleStore(ModuleStoreWriteBase):
             pass
         try:
             course = store._get_course_for_item(block.scope_ids.usage_id)
-            if course:
+            if course is not None:
                 return course.scope_ids.usage_id.course_id
-        except:  # sorry, that method just raises vanilla Exception if it doesn't find course
+        except Exception:  # sorry, that method just raises vanilla Exception if it doesn't find course
             pass
 
     def _infer_course_id_try(self, location):
@@ -436,7 +436,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
         """
         store = self.modulestores[store_name]
         if store.reference_type != Location:
-            raise NotImplementedError(u"Cannot create maps from %s" % store.reference_type)
+            raise ValueError(u"Cannot create maps from %s" % store.reference_type)
         for course in store.get_courses():
             loc_mapper().translate_location(course.location.course_id, course.location)
 
diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py
index b97b1e1686..3fa8c4531e 100644
--- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py
+++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py
@@ -681,7 +681,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
             dbmodel,
         )
         for key, value in fields.iteritems():
-            xmodule[key] = value
+            setattr(xmodule, key, value)
         # decache any pending field settings from init
         xmodule.save()
         return xmodule
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
index 39073e4952..a82ffbb050 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
@@ -33,7 +33,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
     DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor'
     RENDER_TEMPLATE = lambda t_n, d, ctx = None, nsp = 'main': ''
 
-    IMPORT_COURSEID = 'MITx/999/2013_Spring'
+    MONGO_COURSEID = 'MITx/999/2013_Spring'
     XML_COURSEID1 = 'edX/toy/2012_Fall'
     XML_COURSEID2 = 'edX/simple/2012_Fall'
 
@@ -51,7 +51,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         'mappings': {
             XML_COURSEID1: 'xml',
             XML_COURSEID2: 'xml',
-            IMPORT_COURSEID: 'default'
+            MONGO_COURSEID: 'default'
         },
         'stores': {
             'xml': {
@@ -124,17 +124,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
             course.location, item_location.category, location=item_location, block_id=item_location.name
         )
         if isinstance(course.location, CourseLocator):
-            # add_entry is false b/c this is a test that the right thing happened w/o
-            # wanting any additional side effects
-            lookup_map = loc_mapper().translate_location(
-                course_location.course_id, course_location, add_entry_if_missing=False
-            )
-            self.assertEqual(lookup_map, course.location)
-            self.course_locations[self.IMPORT_COURSEID] = course.location.version_agnostic()
-            lookup_map = loc_mapper().translate_location(
-                course_location.course_id, item_location, add_entry_if_missing=False
-            )
-            self.assertEqual(lookup_map, chapter.location)
+            self.course_locations[self.MONGO_COURSEID] = course.location.version_agnostic()
             self.import_chapter_location = chapter.location.version_agnostic()
         else:
             self.assertEqual(course.location, course_location)
@@ -160,17 +150,17 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
 
         self.course_locations = {
              course_id: generate_location(course_id)
-             for course_id in [self.IMPORT_COURSEID, self.XML_COURSEID1, self.XML_COURSEID2]
+             for course_id in [self.MONGO_COURSEID, self.XML_COURSEID1, self.XML_COURSEID2]
         }
         self.fake_location = Location('i4x', 'foo', 'bar', 'vertical', 'baz')
-        self.import_chapter_location = self.course_locations[self.IMPORT_COURSEID].replace(
+        self.import_chapter_location = self.course_locations[self.MONGO_COURSEID].replace(
             category='chapter', name='Overview'
         )
         self.xml_chapter_location = self.course_locations[self.XML_COURSEID1].replace(
             category='chapter', name='Overview'
         )
         # grab old style location b4 possibly converted
-        import_location = self.course_locations[self.IMPORT_COURSEID]
+        import_location = self.course_locations[self.MONGO_COURSEID]
         # get Locators and set up the loc mapper if app is Locator based
         if default == 'split':
             self.fake_location = loc_mapper().translate_location('foo/bar/2012_Fall', self.fake_location)
@@ -186,7 +176,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         self.assertEqual(self.store.get_modulestore_type(self.XML_COURSEID1), XML_MODULESTORE_TYPE)
         self.assertEqual(self.store.get_modulestore_type(self.XML_COURSEID2), XML_MODULESTORE_TYPE)
         mongo_ms_type = MONGO_MODULESTORE_TYPE if default_ms == 'direct' else SPLIT_MONGO_MODULESTORE_TYPE
-        self.assertEqual(self.store.get_modulestore_type(self.IMPORT_COURSEID), mongo_ms_type)
+        self.assertEqual(self.store.get_modulestore_type(self.MONGO_COURSEID), mongo_ms_type)
         # try an unknown mapping, it should be the 'default' store
         self.assertEqual(self.store.get_modulestore_type('foo/bar/2012_Fall'), mongo_ms_type)
 
@@ -201,7 +191,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
                 self.XML_COURSEID1,
                 self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem')
         ))
-        self.assertFalse(self.store.has_item(self.IMPORT_COURSEID, self.fake_location))
+        self.assertFalse(self.store.has_item(self.MONGO_COURSEID, self.fake_location))
 
     @ddt.data('direct', 'split')
     def test_get_item(self, default_ms):
@@ -222,7 +212,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
                 self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem')
             )
         with self.assertRaises(ItemNotFoundError):
-            self.store.get_instance(self.IMPORT_COURSEID, self.fake_location)
+            self.store.get_instance(self.MONGO_COURSEID, self.fake_location)
 
     @ddt.data('direct', 'split')
     def test_get_items(self, default_ms):
@@ -252,10 +242,10 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
             self.store.update_item(course, None)
         # now do it for a r/w db
         # get_course api's are inconsistent: one takes Locators the other an old style course id
-        if hasattr(self.course_locations[self.IMPORT_COURSEID], 'as_course_locator'):
-            locn = self.course_locations[self.IMPORT_COURSEID]
+        if hasattr(self.course_locations[self.MONGO_COURSEID], 'as_course_locator'):
+            locn = self.course_locations[self.MONGO_COURSEID]
         else:
-            locn = self.IMPORT_COURSEID
+            locn = self.MONGO_COURSEID
         course = self.store.get_course(locn)
         # if following raised, then the test is really a noop, change it
         self.assertFalse(course.show_calculator, "Default changed making test meaningless")
@@ -276,7 +266,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         self.store.delete_item(self.import_chapter_location, '**replace_user**')
         # verify it's gone
         with self.assertRaises(ItemNotFoundError):
-            self.store.get_instance(self.IMPORT_COURSEID, self.import_chapter_location)
+            self.store.get_instance(self.MONGO_COURSEID, self.import_chapter_location)
 
     @ddt.data('direct', 'split')
     def test_get_courses(self, default_ms):
@@ -289,7 +279,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
             if hasattr(course.location, 'version_agnostic') else course.location
             for course in courses
         ]
-        self.assertIn(self.course_locations[self.IMPORT_COURSEID], course_ids)
+        self.assertIn(self.course_locations[self.MONGO_COURSEID], course_ids)
         self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids)
         self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids)
 
@@ -325,10 +315,10 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         self.initdb(default_ms)
         parents = self.store.get_parent_locations(
             self.import_chapter_location,
-            self.IMPORT_COURSEID
+            self.MONGO_COURSEID
         )
         self.assertEqual(len(parents), 1)
-        self.assertEqual(parents[0], self.course_locations[self.IMPORT_COURSEID])
+        self.assertEqual(parents[0], self.course_locations[self.MONGO_COURSEID])
 
         parents = self.store.get_parent_locations(
             self.xml_chapter_location,

From dcc6f54409499d776478494aa1f3d8598e94cfe6 Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Mon, 3 Mar 2014 13:15:48 -0500
Subject: [PATCH 08/15] Change get_courses default branch to draft. We need to
 figure out how apps configure what default they want or ensure they get the
 one they want

---
 common/lib/xmodule/xmodule/modulestore/split_mongo/split.py   | 4 ++--
 .../xmodule/modulestore/tests/test_split_modulestore.py       | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py
index f48a68632f..da61e061a1 100644
--- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py
+++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py
@@ -274,7 +274,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         }
         return envelope
 
-    def get_courses(self, branch='published', qualifiers=None):
+    def get_courses(self, branch='draft', qualifiers=None):
         '''
         Returns a list of course descriptors matching any given qualifiers.
 
@@ -284,7 +284,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         Note, this is to find the current head of the named branch type
         (e.g., 'draft'). To get specific versions via guid use get_course.
 
-        :param branch: the branch for which to return courses. Default value is 'published'.
+        :param branch: the branch for which to return courses. Default value is 'draft'.
         :param qualifiers: a optional dict restricting which elements should match
         '''
         if qualifiers is None:
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py
index 653b2a2f70..d2121bffa1 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py
@@ -163,8 +163,6 @@ class SplitModuleCourseTests(SplitModuleTest):
                              "children")
 
         _verify_published_course(modulestore().get_courses(branch='published'))
-        # default for branch is 'published'.
-        _verify_published_course(modulestore().get_courses())
 
     def test_search_qualifiers(self):
         # query w/ search criteria

From fc1ca7b3a27d84081293a5e5e6e208071407a239 Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Mon, 3 Mar 2014 13:16:12 -0500
Subject: [PATCH 09/15] No longer create loc maps on create_(item|course)

---
 .../xmodule/xmodule/modulestore/locator.py    |   4 -
 .../lib/xmodule/xmodule/modulestore/mixed.py  | 116 +++++++-----------
 .../xmodule/xmodule/modulestore/mongo/base.py |  15 ++-
 .../tests/test_mixed_modulestore.py           |  22 ++--
 4 files changed, 64 insertions(+), 93 deletions(-)

diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py
index 2ba86da43c..4c384a49f1 100644
--- a/common/lib/xmodule/xmodule/modulestore/locator.py
+++ b/common/lib/xmodule/xmodule/modulestore/locator.py
@@ -441,8 +441,6 @@ class BlockUsageLocator(CourseLocator):
         Returns a copy of itself without any version info.
 
         :raises: ValueError if the block locator has no package_id
-
-        :param block_locator:
         """
         return BlockUsageLocator(package_id=self.package_id,
                                  branch=self.branch,
@@ -454,8 +452,6 @@ class BlockUsageLocator(CourseLocator):
         Returns a copy of itself without any course info.
 
         :raises: ValueError if the block locator has no version_guid
-
-        :param block_locator:
         """
         return BlockUsageLocator(version_guid=self.version_guid,
                                  block_id=self.block_id)
diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py
index 03fd14b489..d4d6e5f039 100644
--- a/common/lib/xmodule/xmodule/modulestore/mixed.py
+++ b/common/lib/xmodule/xmodule/modulestore/mixed.py
@@ -207,11 +207,9 @@ class MixedModuleStore(ModuleStoreWriteBase):
         """
         Get the course_id from the block or from asking its store. Expensive.
         """
-        try:
-            if block.course_id is not None:
-                return block.course_id
-        except AssertionError:  # will occur if no xmodule set
-            pass
+        # HACK: check xmodule_runtime to avoid an assertion error
+        if block.xmodule_runtime is not None and block.course_id is not None:
+            return block.course_id
         try:
             course = store._get_course_for_item(block.scope_ids.usage_id)
             if course is not None:
@@ -264,55 +262,49 @@ class MixedModuleStore(ModuleStoreWriteBase):
                     blocks = store.get_items(location)
                     if len(blocks) == 1:
                         block = blocks[0]
-                        try:
-                            if block.course_id is not None:
-                                return block.course_id
-                        except AssertionError:
-                            pass
+                        # HACK violate abstraction to avoid assertion error
+                        if block.xmodule_runtime is not None and block.course_id is not None:
+                            return block.course_id
                 except ItemNotFoundError:
                     pass
         # if we get here, it must be in a Locator based store, but we won't be able to find
         # it.
         return None
 
-    def create_course(self, course_location, user_id=None, store_name='default', **kwargs):
+    def create_course(self, course_id, user_id=None, store_name='default', **kwargs):
         """
-        Creates and returns the course. It creates a loc map from the course_location to
-        the new one (if provided as id_root).
-
-        NOTE: course_location must be a Location not
-        a Locator until we no longer need to do loc mapping.
-
-        NOTE: unlike the other mixed modulestore methods, this does not adapt its argument
-        to the persistence store but requires its caller to know what the persistence store
-        wants for args.
+        Creates and returns the course.
 
+        :param org: the org
+        :param fields: a dict of xblock field name - value pairs for the course module.
+        :param metadata: the old way of setting fields by knowing which ones are scope.settings v scope.content
+        :param definition_data: the complement to metadata which is also a subset of fields
+        :param id_root: the split-mongo course_id starting value (see split.create_course)
+        :param pretty_id: a field split.create_course uses and may quit using
         :returns: course xblock
         """
         store = self.modulestores[store_name]
         if not hasattr(store, 'create_course'):
             raise NotImplementedError(u"Cannot create a course on store %s" % store_name)
-        if store.get_modulestore_type(course_location.course_id) == SPLIT_MONGO_MODULESTORE_TYPE:
-            org = kwargs.pop('org', course_location.org)
-            pretty_id = kwargs.pop('pretty_id', None)
-            fields = kwargs.get('fields', {})
+        if store.get_modulestore_type(course_id) == SPLIT_MONGO_MODULESTORE_TYPE:
+            id_root = kwargs.get('id_root')
+            try:
+                course_dict = Location.parse_course_id(course_id)
+                org = course_dict['org']
+                if id_root is None:
+                    id_root = "{org}.{course}.{name}".format(**course_dict)
+            except ValueError:
+                org = None
+                if id_root is None:
+                    id_root = course_id
+            org = kwargs.pop('org', org)
+            pretty_id = kwargs.pop('pretty_id', id_root)
+            fields = kwargs.pop('fields', {})
             fields.update(kwargs.pop('metadata', {}))
             fields.update(kwargs.pop('definition_data', {}))
-            kwargs['fields'] = fields
-            # TODO rename id_root to package_id for consistency. It's too confusing
-            id_root = kwargs.pop('id_root', u"{0.org}.{0.course}.{0.name}".format(course_location))
-            course = store.create_course(
-                org, pretty_id, user_id, id_root=id_root, master_branch=course_location.revision or 'published',
-                **kwargs
-            )
-            block_map = {course_location.name: {'course': course.location.block_id}}
-            # NOTE: course.location will be a Locator not == course_location
-            loc_mapper().create_map_entry(
-                course_location, course.location.package_id, block_map=block_map
-            )
+            course = store.create_course(org, pretty_id, user_id, id_root=id_root, fields=fields, **kwargs)
         else:  # assume mongo
-            course = store.create_course(course_location, **kwargs)
-            loc_mapper().translate_location(course_location.course_id, course_location)
+            course = store.create_course(course_id, **kwargs)
 
         return course
 
@@ -322,29 +314,23 @@ class MixedModuleStore(ModuleStoreWriteBase):
         it installs the new item as a child of the parent (if the parent_loc is a specific
         xblock reference).
 
-        Adds an entry to the loc map using the kwarg location if provided (must be a
-        Location if provided) or block_id and category if provided.
-
         :param course_or_parent_loc: Can be a course_id (org/course/run), CourseLocator,
         Location, or BlockUsageLocator but must be what the persistence modulestore expects
         """
         # find the store for the course
         course_id = self._infer_course_id_try(course_or_parent_loc)
+        if course_id is None:
+            raise ItemNotFoundError(u"Cannot find modulestore for %s" % course_or_parent_loc)
 
         store = self._get_modulestore_for_courseid(course_id)
 
         location = kwargs.pop('location', None)
         # invoke its create_item
         if isinstance(store, MongoModuleStore):
+            block_id = kwargs.pop('block_id', getattr(location, 'name', uuid4().hex))
             # convert parent loc if it's legit
-            block_id = kwargs.pop('block_id', uuid4().hex)
             if isinstance(course_or_parent_loc, basestring):
                 parent_loc = None
-                if location is None:
-                    locn_dict = Location.parse_course_id(course_id)
-                    locn_dict['category'] = category
-                    locn_dict['name'] = block_id
-                    location = Location(locn_dict)
             else:
                 parent_loc = course_or_parent_loc
                 # must have a legitimate location, compute if appropriate
@@ -352,8 +338,6 @@ class MixedModuleStore(ModuleStoreWriteBase):
                     location = parent_loc.replace(category=category, name=block_id)
             # do the actual creation
             xblock = store.create_and_save_xmodule(location, **kwargs)
-            # add the loc mapping
-            loc_mapper().translate_location(course_id, location)
             # don't forget to attach to parent
             if parent_loc is not None and not 'detached' in xblock._class_tags:
                 parent = store.get_item(parent_loc)
@@ -361,16 +345,10 @@ class MixedModuleStore(ModuleStoreWriteBase):
                 store.update_item(parent)
         elif isinstance(store, SplitMongoModuleStore):
             if isinstance(course_or_parent_loc, basestring): # course_id
-                old_course_id = course_or_parent_loc
                 course_or_parent_loc = loc_mapper().translate_location_to_course_locator(
                     course_or_parent_loc, None
                 )
-            elif isinstance(course_or_parent_loc, CourseLocator):
-                old_course_loc = loc_mapper().translate_locator_to_location(
-                    course_or_parent_loc, get_course=True
-                )
-                old_course_id = old_course_loc.course_id
-            else:
+            elif not isinstance(course_or_parent_loc, CourseLocator):
                 raise ValueError(u"Cannot create a child of {} in split. Wrong repr.".format(course_or_parent_loc))
 
             # split handles all the fields in one dict not separated by scope
@@ -379,16 +357,10 @@ class MixedModuleStore(ModuleStoreWriteBase):
             fields.update(kwargs.pop('definition_data', {}))
             kwargs['fields'] = fields
 
+            if not kwargs.get('block_id', False) and getattr(location, 'name', False):
+                kwargs['block_id'] = getattr(location, 'name')
+
             xblock = store.create_item(course_or_parent_loc, category, user_id, **kwargs)
-            if location is None:
-                locn_dict = Location.parse_course_id(old_course_id)
-                locn_dict['category'] = category
-                locn_dict['name'] = xblock.location.block_id
-                location = Location(locn_dict)
-            # map location.name to xblock.location.block_id
-            loc_mapper().translate_location(
-                old_course_id, location, passed_block_id=xblock.location.block_id
-            )
         else:
             raise NotImplementedError(u"Cannot create an item on store %s" % store)
 
@@ -441,7 +413,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
             loc_mapper().translate_location(course.location.course_id, course.location)
 
 
-def _compare_stores(alpha, aleph):
+def _compare_stores(left, right):
     """
     Order stores via precedence: if a course is found in an earlier store, it shadows the later store.
 
@@ -449,20 +421,20 @@ def _compare_stores(alpha, aleph):
     then others. Locators before Locations because if some courses may be in both,
     the ones in the Locator-based stores shadow the others.
     """
-    if alpha.get_modulestore_type(None) == XML_MODULESTORE_TYPE:
-        if aleph.get_modulestore_type(None) == XML_MODULESTORE_TYPE:
+    if left.get_modulestore_type(None) == XML_MODULESTORE_TYPE:
+        if right.get_modulestore_type(None) == XML_MODULESTORE_TYPE:
             return 0
         else:
             return -1
-    elif aleph.get_modulestore_type(None) == XML_MODULESTORE_TYPE:
+    elif right.get_modulestore_type(None) == XML_MODULESTORE_TYPE:
         return 1
 
-    if issubclass(alpha.reference_type, Locator):
-        if issubclass(aleph.reference_type, Locator):
+    if issubclass(left.reference_type, Locator):
+        if issubclass(right.reference_type, Locator):
             return 0
         else:
             return -1
-    elif issubclass(aleph.reference_type, Locator):
+    elif issubclass(right.reference_type, Locator):
         return 1
 
     return 0
diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py
index 3fa8c4531e..1ef0141f55 100644
--- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py
+++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py
@@ -625,12 +625,19 @@ class MongoModuleStore(ModuleStoreWriteBase):
         modules = self._load_items(list(items), depth)
         return modules
 
-    def create_course(self, location, definition_data=None, metadata=None, runtime=None):
+    def create_course(self, course_id, definition_data=None, metadata=None, runtime=None):
         """
-        Create a course with the given location. The location category must be 'course'.
+        Create a course with the given course_id.
         """
-        if location.category != 'course':
-            raise ValueError(u"Course roots must be of category 'course': {}".format(unicode(location)))
+        if isinstance(course_id, Location):
+            location = course_id
+            if location.category != 'course':
+                raise ValueError(u"Course roots must be of category 'course': {}".format(unicode(location)))
+        else:
+            course_dict = Location.parse_course_id(course_id)
+            course_dict['category'] = 'course'
+            course_dict['tag'] = 'i4x'
+            location = Location(course_dict)
         return self.create_and_save_xmodule(location, definition_data, metadata, runtime)
 
     def create_xmodule(self, location, definition_data=None, metadata=None, system=None, fields={}):
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
index a82ffbb050..80e627e49f 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
@@ -110,25 +110,23 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         self.addCleanup(patcher.stop)
         self.addTypeEqualityFunc(BlockUsageLocator, '_compareIgnoreVersion')
 
-    def _create_course(self, default, course_location, item_location):
+    def _create_course(self, default, course_id):
         """
         Create a course w/ one item in the persistence store using the given course & item location.
-        NOTE: course_location and item_location must be Location regardless of the app reference type in order
-        to cause the right mapping to be created.
         """
-        course = self.store.create_course(
-            course_location, store_name=default, metadata={'display_name': course_location.name}
-        )
+        course = self.store.create_course(course_id, store_name=default)
+        category = self.import_chapter_location.category
+        block_id = self.import_chapter_location.name
         chapter = self.store.create_item(
             # don't use course_location as it may not be the repr
-            course.location, item_location.category, location=item_location, block_id=item_location.name
+            course.location, category, location=self.import_chapter_location, block_id=block_id
         )
         if isinstance(course.location, CourseLocator):
             self.course_locations[self.MONGO_COURSEID] = course.location.version_agnostic()
             self.import_chapter_location = chapter.location.version_agnostic()
         else:
-            self.assertEqual(course.location, course_location)
-            self.assertEqual(chapter.location, item_location)
+            self.assertEqual(course.location.course_id, course_id)
+            self.assertEqual(chapter.location, self.import_chapter_location)
 
     def initdb(self, default):
         """
@@ -159,13 +157,11 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         self.xml_chapter_location = self.course_locations[self.XML_COURSEID1].replace(
             category='chapter', name='Overview'
         )
-        # grab old style location b4 possibly converted
-        import_location = self.course_locations[self.MONGO_COURSEID]
         # get Locators and set up the loc mapper if app is Locator based
         if default == 'split':
             self.fake_location = loc_mapper().translate_location('foo/bar/2012_Fall', self.fake_location)
 
-        self._create_course(default, import_location, self.import_chapter_location)
+        self._create_course(default, self.MONGO_COURSEID)
 
     @ddt.data('direct', 'split')
     def test_get_modulestore_type(self, default_ms):
@@ -273,12 +269,12 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         self.initdb(default_ms)
         # we should have 3 total courses across all stores
         courses = self.store.get_courses()
-        self.assertEqual(len(courses), 3)
         course_ids = [
             course.location.version_agnostic()
             if hasattr(course.location, 'version_agnostic') else course.location
             for course in courses
         ]
+        self.assertEqual(len(courses), 3, "Not 3 courses: {}".format(course_ids))
         self.assertIn(self.course_locations[self.MONGO_COURSEID], course_ids)
         self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids)
         self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids)

From aecc037e2da8847e47eda03ac06e1b24aefc2a76 Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Mon, 3 Mar 2014 16:52:38 -0500
Subject: [PATCH 10/15] pylint and pep8 fixes

---
 .../lib/xmodule/xmodule/modulestore/__init__.py   |  2 +-
 common/lib/xmodule/xmodule/modulestore/mixed.py   |  8 ++++----
 .../modulestore/tests/persistent_factories.py     |  4 ++--
 .../modulestore/tests/test_mixed_modulestore.py   | 15 +++++++++++----
 4 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py
index b67480ac75..e010948e05 100644
--- a/common/lib/xmodule/xmodule/modulestore/__init__.py
+++ b/common/lib/xmodule/xmodule/modulestore/__init__.py
@@ -451,7 +451,7 @@ class ModuleStoreWrite(ModuleStoreRead):
         pass
 
     @abstractmethod
-    def delete_item(self, location, user_id=None, delete_all_versions=False, delete_children=False, force=False):
+    def delete_item(self, location, user_id=None, **kwargs):
         """
         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.
diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py
index d4d6e5f039..c937093cad 100644
--- a/common/lib/xmodule/xmodule/modulestore/mixed.py
+++ b/common/lib/xmodule/xmodule/modulestore/mixed.py
@@ -344,7 +344,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
                 parent.children.append(location.url())
                 store.update_item(parent)
         elif isinstance(store, SplitMongoModuleStore):
-            if isinstance(course_or_parent_loc, basestring): # course_id
+            if isinstance(course_or_parent_loc, basestring):  # course_id
                 course_or_parent_loc = loc_mapper().translate_location_to_course_locator(
                     course_or_parent_loc, None
                 )
@@ -377,15 +377,15 @@ class MixedModuleStore(ModuleStoreWriteBase):
         store = self._get_modulestore_for_courseid(course_id)
         return store.update_item(xblock, user_id)
 
-    def delete_item(self, location, user_id=None):
+    def delete_item(self, location, user_id=None, **kwargs):
         """
-        Delete the given item from persistence.
+        Delete the given item from persistence. kwargs allow modulestore specific parameters.
         """
         course_id = self._infer_course_id_try(location)
         if course_id is None:
             raise ItemNotFoundError(u"Cannot find modulestore for %s" % location)
         store = self._get_modulestore_for_courseid(course_id)
-        return store.delete_item(location, user_id=user_id)
+        return store.delete_item(location, user_id=user_id, **kwargs)
 
     def close_all_connections(self):
         """
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py b/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py
index b5cce6be6c..77759d2431 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py
@@ -58,8 +58,8 @@ class ItemFactory(SplitFactory):
     # pylint: disable=W0613
     @classmethod
     def _create(cls, target_class, parent_location, category='chapter',
-        user_id='test_user', block_id=None, definition_locator=None, force=False,
-        continue_version=False, **kwargs):
+                user_id='test_user', block_id=None, definition_locator=None, force=False,
+                continue_version=False, **kwargs):
         """
         passes *kwargs* as the new item's field values:
 
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
index 80e627e49f..f60a250e7b 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
@@ -109,7 +109,11 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         patcher.start()
         self.addCleanup(patcher.stop)
         self.addTypeEqualityFunc(BlockUsageLocator, '_compareIgnoreVersion')
+        # define attrs which get set in initdb to quell pylint
+        self.import_chapter_location = self.store = self.fake_location = self.xml_chapter_location = None
+        self.course_locations = []
 
+    # pylint: disable=invalid-name
     def _create_course(self, default, course_id):
         """
         Create a course w/ one item in the persistence store using the given course & item location.
@@ -147,8 +151,8 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
             return Location(course_dict)
 
         self.course_locations = {
-             course_id: generate_location(course_id)
-             for course_id in [self.MONGO_COURSEID, self.XML_COURSEID1, self.XML_COURSEID2]
+            course_id: generate_location(course_id)
+            for course_id in [self.MONGO_COURSEID, self.XML_COURSEID1, self.XML_COURSEID2]
         }
         self.fake_location = Location('i4x', 'foo', 'bar', 'vertical', 'baz')
         self.import_chapter_location = self.course_locations[self.MONGO_COURSEID].replace(
@@ -184,8 +188,8 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
 
         # try negative cases
         self.assertFalse(self.store.has_item(
-                self.XML_COURSEID1,
-                self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem')
+            self.XML_COURSEID1,
+            self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem')
         ))
         self.assertFalse(self.store.has_item(self.MONGO_COURSEID, self.fake_location))
 
@@ -323,10 +327,12 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         self.assertEqual(len(parents), 1)
         self.assertEqual(parents[0], self.course_locations[self.XML_COURSEID1])
 
+
 #=============================================================================================================
 # General utils for not using django settings
 #=============================================================================================================
 
+
 def load_function(path):
     """
     Load a function by name.
@@ -338,6 +344,7 @@ def load_function(path):
     return getattr(import_module(module_path), name)
 
 
+# pylint: disable=unused-argument
 def create_modulestore_instance(engine, doc_store_config, options, i18n_service=None):
     """
     This will return a new instance of a modulestore given an engine and options

From 563b713a3f5fb6247ade6280b2838428e60c819a Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Tue, 4 Mar 2014 09:23:51 -0500
Subject: [PATCH 11/15] Test exception on read-only db

---
 .../xmodule/modulestore/tests/test_mixed_modulestore.py   | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
index f60a250e7b..a168705993 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
@@ -296,6 +296,14 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         # this course is in the directory from which we loaded courses but not in the map
         self.assertNotIn("edX/toy/TT_2012_Fall", course_ids)
 
+    def test_xml_no_write(self):
+        """
+        Test that the xml modulestore doesn't allow write ops.
+        """
+        self.initdb('direct')
+        with self.assertRaises(NotImplementedError):
+            self.store.create_course("org/course/run", store_name='xml')
+
     @ddt.data('direct', 'split')
     def test_get_course(self, default_ms):
         self.initdb(default_ms)

From e972b230640df5a4338a3ab2526a00c39157a8b3 Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Tue, 4 Mar 2014 09:34:37 -0500
Subject: [PATCH 12/15] Requesting an xmodule field in wrong app context raises
 exception rather than assertion

---
 common/lib/xmodule/xmodule/exceptions.py        |  7 +++++++
 common/lib/xmodule/xmodule/modulestore/mixed.py | 11 +++++++----
 common/lib/xmodule/xmodule/x_module.py          |  4 +++-
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/common/lib/xmodule/xmodule/exceptions.py b/common/lib/xmodule/xmodule/exceptions.py
index 48c083cbf1..a84336c469 100644
--- a/common/lib/xmodule/xmodule/exceptions.py
+++ b/common/lib/xmodule/xmodule/exceptions.py
@@ -31,3 +31,10 @@ class SerializationError(Exception):
     def __init__(self, location, msg):
         super(SerializationError, self).__init__(msg)
         self.location = location
+
+
+class UndefinedContext(Exception):
+    """
+    Tried to access an xmodule field which needs a different context (runtime) to have a value.
+    """
+    pass
diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py
index c937093cad..1adab076f3 100644
--- a/common/lib/xmodule/xmodule/modulestore/mixed.py
+++ b/common/lib/xmodule/xmodule/modulestore/mixed.py
@@ -15,6 +15,7 @@ from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationErr
 from uuid import uuid4
 from xmodule.modulestore.mongo.base import MongoModuleStore
 from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore
+from xmodule.exceptions import UndefinedContext
 
 log = logging.getLogger(__name__)
 
@@ -207,9 +208,10 @@ class MixedModuleStore(ModuleStoreWriteBase):
         """
         Get the course_id from the block or from asking its store. Expensive.
         """
-        # HACK: check xmodule_runtime to avoid an assertion error
-        if block.xmodule_runtime is not None and block.course_id is not None:
+        try:
             return block.course_id
+        except UndefinedContext:
+            pass
         try:
             course = store._get_course_for_item(block.scope_ids.usage_id)
             if course is not None:
@@ -262,9 +264,10 @@ class MixedModuleStore(ModuleStoreWriteBase):
                     blocks = store.get_items(location)
                     if len(blocks) == 1:
                         block = blocks[0]
-                        # HACK violate abstraction to avoid assertion error
-                        if block.xmodule_runtime is not None and block.course_id is not None:
+                        try:
                             return block.course_id
+                        except UndefinedContext:
+                            pass
                 except ItemNotFoundError:
                     pass
         # if we get here, it must be in a Locator based store, but we won't be able to find
diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py
index 489a2c8032..38d895b93e 100644
--- a/common/lib/xmodule/xmodule/x_module.py
+++ b/common/lib/xmodule/xmodule/x_module.py
@@ -26,6 +26,7 @@ from xmodule.errortracker import exc_info_to_str
 from xmodule.modulestore import Location
 from xmodule.modulestore.exceptions import ItemNotFoundError, InsufficientSpecificationError, InvalidLocationError
 from xmodule.modulestore.locator import BlockUsageLocator
+from xmodule.exceptions import UndefinedContext
 
 
 log = logging.getLogger(__name__)
@@ -795,7 +796,8 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
         Returns the XModule corresponding to this descriptor. Expects that the system
         already supports all of the attributes needed by xmodules
         """
-        assert self.xmodule_runtime is not None
+        if self.xmodule_runtime is None:
+            raise UndefinedContext()
         assert self.xmodule_runtime.error_descriptor_class is not None
         if self.xmodule_runtime.xmodule_instance is None:
             try:

From 71ac8441205555b6f5e178a71d4b679ec07dbc61 Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Tue, 4 Mar 2014 11:50:59 -0500
Subject: [PATCH 13/15] Add tests for get_orphans

---
 common/lib/xmodule/xmodule/modulestore/mixed.py |  4 ++++
 .../modulestore/tests/test_mixed_modulestore.py | 17 ++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py
index 1adab076f3..4b675fc146 100644
--- a/common/lib/xmodule/xmodule/modulestore/mixed.py
+++ b/common/lib/xmodule/xmodule/modulestore/mixed.py
@@ -334,6 +334,10 @@ class MixedModuleStore(ModuleStoreWriteBase):
             # convert parent loc if it's legit
             if isinstance(course_or_parent_loc, basestring):
                 parent_loc = None
+                if location is None:
+                    loc_dict = Location.parse_course_id(course_id)
+                    loc_dict['name'] = block_id
+                    location = Location(category=category, **loc_dict)
             else:
                 parent_loc = course_or_parent_loc
                 # must have a legitimate location, compute if appropriate
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
index a168705993..dac4870f51 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
@@ -317,7 +317,6 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
             self.assertIsNotNone(course)
             self.assertEqual(course.location, course_locn)
 
-    # pylint: disable=E1101
     @ddt.data('direct', 'split')
     def test_get_parent_locations(self, default_ms):
         self.initdb(default_ms)
@@ -335,6 +334,22 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         self.assertEqual(len(parents), 1)
         self.assertEqual(parents[0], self.course_locations[self.XML_COURSEID1])
 
+    @ddt.data('direct', 'split')
+    def test_get_orphans(self, default_ms):
+        self.initdb(default_ms)
+        # create an orphan
+        if default_ms == 'split':
+            course_id = self.course_locations[self.MONGO_COURSEID].as_course_locator()
+            branch = course_id.branch
+        else:
+            course_id = self.MONGO_COURSEID
+            branch = None
+        orphan = self.store.create_item(course_id, 'problem', block_id='orphan')
+        found_orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID], branch)
+        if default_ms == 'split':
+            self.assertEqual(found_orphans, [orphan.location.version_agnostic()])
+        else:
+            self.assertEqual(found_orphans, [unicode(orphan.location)])
 
 #=============================================================================================================
 # General utils for not using django settings

From 41f5159b37b7e8ee5100c795ae4079a5a3b789f7 Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Tue, 4 Mar 2014 12:55:19 -0500
Subject: [PATCH 14/15] Test special code paths in create_item

---
 .../lib/xmodule/xmodule/modulestore/mixed.py  |  6 +--
 .../tests/test_mixed_modulestore.py           | 45 +++++++++++++++----
 2 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py
index 4b675fc146..5152ea9231 100644
--- a/common/lib/xmodule/xmodule/modulestore/mixed.py
+++ b/common/lib/xmodule/xmodule/modulestore/mixed.py
@@ -353,7 +353,8 @@ class MixedModuleStore(ModuleStoreWriteBase):
         elif isinstance(store, SplitMongoModuleStore):
             if isinstance(course_or_parent_loc, basestring):  # course_id
                 course_or_parent_loc = loc_mapper().translate_location_to_course_locator(
-                    course_or_parent_loc, None
+                    # hardcode draft version until we figure out how we're handling branches from app
+                    course_or_parent_loc, None, published=False
                 )
             elif not isinstance(course_or_parent_loc, CourseLocator):
                 raise ValueError(u"Cannot create a child of {} in split. Wrong repr.".format(course_or_parent_loc))
@@ -364,9 +365,6 @@ class MixedModuleStore(ModuleStoreWriteBase):
             fields.update(kwargs.pop('definition_data', {}))
             kwargs['fields'] = fields
 
-            if not kwargs.get('block_id', False) and getattr(location, 'name', False):
-                kwargs['block_id'] = getattr(location, 'name')
-
             xblock = store.create_item(course_or_parent_loc, category, user_id, **kwargs)
         else:
             raise NotImplementedError(u"Cannot create an item on store %s" % store)
diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
index dac4870f51..d2e31cb831 100644
--- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
+++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
@@ -141,15 +141,6 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         self.store = MixedModuleStore(**self.options)
         self.addCleanup(self.store.close_all_connections)
 
-        def generate_location(course_id):
-            """
-            Generate the locations for the given ids
-            """
-            course_dict = Location.parse_course_id(course_id)
-            course_dict['tag'] = 'i4x'
-            course_dict['category'] = 'course'
-            return Location(course_dict)
-
         self.course_locations = {
             course_id: generate_location(course_id)
             for course_id in [self.MONGO_COURSEID, self.XML_COURSEID1, self.XML_COURSEID2]
@@ -351,6 +342,32 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         else:
             self.assertEqual(found_orphans, [unicode(orphan.location)])
 
+    @ddt.data('split')
+    def test_create_item_from_course_id(self, default_ms):
+        """
+        Test code paths missed by the above:
+        * passing an old-style course_id which has a loc map to split's create_item
+        """
+        self.initdb(default_ms)
+        # create loc_map entry
+        loc_mapper().translate_location(self.MONGO_COURSEID, generate_location(self.MONGO_COURSEID))
+        orphan = self.store.create_item(self.MONGO_COURSEID, 'problem', block_id='orphan')
+        self.assertEqual(
+            orphan.location.version_agnostic().as_course_locator(),
+            self.course_locations[self.MONGO_COURSEID].as_course_locator()
+        )
+
+    @ddt.data('direct')
+    def test_create_item_from_parent_location(self, default_ms):
+        """
+        Test a code path missed by the above: passing an old-style location as parent but no
+        new location for the child
+        """
+        self.initdb(default_ms)
+        self.store.create_item(self.course_locations[self.MONGO_COURSEID], 'problem', block_id='orphan')
+        orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID], None)
+        self.assertEqual(len(orphans), 0, "unexpected orphans: {}".format(orphans))
+
 #=============================================================================================================
 # General utils for not using django settings
 #=============================================================================================================
@@ -378,3 +395,13 @@ def create_modulestore_instance(engine, doc_store_config, options, i18n_service=
         doc_store_config=doc_store_config,
         **options
     )
+
+
+def generate_location(course_id):
+    """
+    Generate the locations for the given ids
+    """
+    course_dict = Location.parse_course_id(course_id)
+    course_dict['tag'] = 'i4x'
+    course_dict['category'] = 'course'
+    return Location(course_dict)

From e915a32aed1b9176ef49900e0ffbb07333a184fd Mon Sep 17 00:00:00 2001
From: Don Mitchell <dmitchell@edx.org>
Date: Wed, 5 Mar 2014 09:40:28 -0500
Subject: [PATCH 15/15] xblock save only affects draft

Fix test which caused error
---
 cms/djangoapps/contentstore/views/component.py       | 9 +++------
 cms/djangoapps/contentstore/views/item.py            | 9 +++------
 cms/djangoapps/contentstore/views/tests/test_item.py | 6 +++---
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py
index f2ec685730..6b641fc33b 100644
--- a/cms/djangoapps/contentstore/views/component.py
+++ b/cms/djangoapps/contentstore/views/component.py
@@ -26,7 +26,7 @@ from xblock.runtime import Mixologist
 
 from lms.lib.xblock.runtime import unquote_slashes
 
-from contentstore.utils import get_lms_link_for_item, compute_unit_state, UnitState
+from contentstore.utils import get_lms_link_for_item, compute_unit_state, UnitState, get_modulestore
 from contentstore.views.helpers import get_parent_xblock
 
 from models.settings.course_grading import CourseGradingModel
@@ -358,7 +358,7 @@ def component_handler(request, usage_id, handler, suffix=''):
 
     location = unquote_slashes(usage_id)
 
-    descriptor = modulestore().get_item(location)
+    descriptor = get_modulestore(location).get_item(location)
     # Let the module handle the AJAX
     req = django_to_webob_request(request)
 
@@ -371,9 +371,6 @@ def component_handler(request, usage_id, handler, suffix=''):
 
     # unintentional update to handle any side effects of handle call; so, request user didn't author
     # the change
-    if getattr(descriptor, 'is_draft', False):
-        modulestore('draft').update_item(descriptor, None)
-    else:
-        modulestore('direct').update_item(descriptor, None)
+    get_modulestore(location).update_item(descriptor, None)
 
     return webob_to_django_response(resp)
diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py
index 6c15a7d400..5a7550611f 100644
--- a/cms/djangoapps/contentstore/views/item.py
+++ b/cms/djangoapps/contentstore/views/item.py
@@ -202,12 +202,9 @@ def xblock_view_handler(request, package_id, view_name, tag=None, branch=None, v
                 log.debug("unable to render studio_view for %r", component, exc_info=True)
                 fragment = Fragment(render_to_string('html_error.html', {'message': str(exc)}))
 
-            # change not authored by requestor but by xblocks. should not convert to draft if not
-            # already draft
-            if getattr(component, 'is_draft', False):
-                modulestore('draft').update_item(component, None)
-            else:
-                modulestore('direct').update_item(component, None)
+            # change not authored by requestor but by xblocks.
+            store.update_item(component, None)
+
         elif view_name == 'student_view' and component.has_children:
             # For non-leaf xblocks on the unit page, show the special rendering
             # which links to the new container page.
diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py
index 3aa3f15ad9..cfa1088e20 100644
--- a/cms/djangoapps/contentstore/views/tests/test_item.py
+++ b/cms/djangoapps/contentstore/views/tests/test_item.py
@@ -636,11 +636,11 @@ class TestComponentHandler(TestCase):
     def setUp(self):
         self.request_factory = RequestFactory()
 
-        patcher = patch('contentstore.views.component.modulestore')
-        self.modulestore = patcher.start()
+        patcher = patch('contentstore.views.component.get_modulestore')
+        self.get_modulestore = patcher.start()
         self.addCleanup(patcher.stop)
 
-        self.descriptor = self.modulestore.return_value.get_item.return_value
+        self.descriptor = self.get_modulestore.return_value.get_item.return_value
 
         self.usage_id = 'dummy_usage_id'