Merge pull request #2738 from edx/dbarch/mixed_ms_router

Dbarch/mixed ms router

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)

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)

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
@@ -365,7 +365,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)
 
@@ -376,6 +376,8 @@ 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)
+    # unintentional update to handle any side effects of handle call; so, request user didn't author
+    # the change
+    get_modulestore(location).update_item(descriptor, None)
 
     return webob_to_django_response(resp)

cms/djangoapps/contentstore/views/item.py

@@ -202,7 +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)}))
 
-            store.save_xmodule(component)
+            # 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.
@@ -519,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()

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

@@ -661,11 +661,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'
 

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

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 #############################
@@ -229,7 +230,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 ################################

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
 
 

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

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')
 
@@ -447,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.
@@ -475,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.
@@ -529,9 +535,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)

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

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,14 @@ class CourseLocator(Locator):
         """
         Return a string representing this location.
         """
+        parts = []
         if self.package_id:
-            result = unicode(self.package_id)
+            parts.append(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>'
+                parts.append(u"{prefix}{branch}".format(prefix=BRANCH_PREFIX, branch=self.branch))
+        if self.version_guid:
+            parts.append(u"{prefix}{guid}".format(prefix=VERSION_PREFIX, guid=self.version_guid))
+        return u"/".join(parts)
 
     def url(self):
         """
@@ -432,22 +436,25 @@ 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.
 
-        :param block_locator:
+        :raises: ValueError if the block locator has no package_id
         """
-        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
+        """
+        return BlockUsageLocator(version_guid=self.version_guid,
+                                 block_id=self.block_id)
 
     def set_block_id(self, new):
         """

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

@@ -5,47 +5,42 @@ In this way, courses can be served up both - say - XMLModuleStore or MongoModule
 
 """
 
+import logging
+
 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.exceptions import InsufficientSpecificationError, ItemNotFoundError
-from xmodule.modulestore.parsers import ALLOWED_ID_CHARS
-import re
+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
+from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore
+from xmodule.exceptions import UndefinedContext
 
 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 Location or Locator to indicate what type of reference this 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.use_locations = (reference_type != 'Locator')
+
         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()
@@ -58,137 +53,22 @@ class MixedModuleStore(ModuleStoreWriteBase):
                 store['OPTIONS'],
                 i18n_service=i18n_service,
             )
+            # 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)
 
     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]
 
-    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 == 'draft', 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):
-            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 issubclass(store.reference_type, Location if self.use_locations else Locator):
-            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
-        """
-        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.location
-        )
-        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.location.__class__, xblock.location
-        )
-        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 self.use_locations and reference_type == Location:
-            return lambda field, xblock: None
-        if not self.use_locations and issubclass(reference_type, Locator):
-            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 has_item(self, course_id, reference):
         """
         Does the course include the xblock who's id is reference?
@@ -197,21 +77,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):
         """
@@ -224,70 +102,55 @@ 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'))
-        # translate won't work w/ missing fields so work around it
-        if store.reference_type == Location:
-            if not self.use_locations:
-                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 self.use_locations:
-                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
-                    )
-                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'
-                    )
-        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):
+        """
+        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
         in this modulestore.
         '''
-        courses = []
-        for key in self.modulestores:
-            store_courses = self.modulestores[key].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
-            if key != 'default':
-                for course in store_courses:
-                    # 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])
-            else:
-                # if we're the 'default' store provider, then we surface all courses hosted in
-                # that store provider
-                courses = courses + (store_courses)
-
-        return courses
+        # 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, 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)
+        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):
+                        # 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(
+                                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
+                    else:
+                        courses[course_location] = course
+
+        return courses.values()
 
     def get_course(self, course_id):
         """
@@ -297,44 +160,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 self.use_locations:
-                    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):
         """
@@ -352,10 +190,9 @@ 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)
-        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):
         """
@@ -367,31 +204,242 @@ 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.
+        """
+        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:
+                return course.scope_ids.usage_id.course_id
+        except Exception:  # sorry, that method just raises vanilla Exception if it doesn't find course
+            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 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
+        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:
+                    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]
+                        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
+        # it.
+        return None
+
+    def create_course(self, course_id, user_id=None, store_name='default', **kwargs):
+        """
+        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_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', {}))
+            course = store.create_course(org, pretty_id, user_id, id_root=id_root, fields=fields, **kwargs)
+        else:  # assume mongo
+            course = store.create_course(course_id, **kwargs)
+
+        return 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).
+
+        :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
+            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
+                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)
+            # 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)
+        elif isinstance(store, SplitMongoModuleStore):
+            if isinstance(course_or_parent_loc, basestring):  # course_id
+                course_or_parent_loc = loc_mapper().translate_location_to_course_locator(
+                    # 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))
+
+            # 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)
+        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):
         """
         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
+        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)
+        return store.update_item(xblock, user_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)
+    def delete_item(self, location, user_id=None, **kwargs):
+        """
+        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, **kwargs)
 
-        return self._outgoing_xblock_adaptor(store, course_id, xblock)
+    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()
 
-    def delete_item(self, location, **kwargs):
+    def ensure_loc_maps_exist(self, store_name):
         """
-        Delete the given item from persistence.
+        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
         """
-        if self.use_locations:
-            raise NotImplementedError
+        store = self.modulestores[store_name]
+        if store.reference_type != Location:
+            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)
+
+
+def _compare_stores(left, right):
+    """
+    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 left.get_modulestore_type(None) == XML_MODULESTORE_TYPE:
+        if right.get_modulestore_type(None) == XML_MODULESTORE_TYPE:
+            return 0
+        else:
+            return -1
+    elif right.get_modulestore_type(None) == XML_MODULESTORE_TYPE:
+        return 1
+
+    if issubclass(left.reference_type, Locator):
+        if issubclass(right.reference_type, Locator):
+            return 0
+        else:
+            return -1
+    elif issubclass(right.reference_type, Locator):
+        return 1
 
-        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)
+    return 0

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

@@ -625,7 +625,22 @@ 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, course_id, definition_data=None, metadata=None, runtime=None):
+        """
+        Create a course with the given course_id.
+        """
+        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={}):
         """
         Create the new xmodule but don't save it. Returns the new module.
 
@@ -672,36 +687,18 @@ class MongoModuleStore(ModuleStoreWriteBase):
             ScopeIds(None, location.category, location, location),
             dbmodel,
         )
+        for key, value in fields.iteritems():
+            setattr(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 +708,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 +725,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 +784,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 +858,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):
         """

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,22 +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)
-
-    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
+        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):
         """
@@ -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)
 

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
@@ -281,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.
 
@@ -291,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:
@@ -563,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'],
@@ -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.

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': 'Location',
                 'stores': {
                     'default': mongo_config['default'],
                     'xml': xml_config['default']
@@ -219,13 +218,21 @@ 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()
+            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):

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

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

-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

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

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

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

-# 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 ddt
+from mock import patch, Mock
+from importlib import import_module
 
 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
 
+from xmodule.modulestore.locator import BlockUsageLocator, CourseLocator
+from xmodule.modulestore.tests.test_location_mapper import LocMapperSetupSansDjango, loc_mapper
 # Mixed modulestore depends on django, so we'll manually configure some django settings
 # before importing the module
 from django.conf import settings
-import unittest
-import copy
 if not settings.configured:
     settings.configure()
-
 from xmodule.modulestore.mixed import MixedModuleStore
 
 
-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'
-
-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',
-            }
+@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': ''
+
+    MONGO_COURSEID = 'MITx/999/2013_Spring'
+    XML_COURSEID1 = 'edX/toy/2012_Fall'
+    XML_COURSEID2 = 'edX/simple/2012_Fall'
+
+    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',
+            MONGO_COURSEID: 'default'
         },
-        'default': {
-            'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',
-            'DOC_STORE_CONFIG': {
-                'host': HOST,
-                'db': DB,
-                'collection': COLLECTION,
+        'stores': {
+            'xml': {
+                'ENGINE': 'xmodule.modulestore.xml.XMLModuleStore',
+                'OPTIONS': {
+                    'data_dir': DATA_DIR,
+                    'default_class': 'xmodule.hidden_module.HiddenDescriptor',
+                }
+            },
+            'direct': {
+                'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',
+                'DOC_STORE_CONFIG': DOC_STORE_CONFIG,
+                'OPTIONS': modulestore_options
             },
-            'OPTIONS': {
-                'default_class': DEFAULT_CLASS,
-                'fs_root': DATA_DIR,
-                'render_template': RENDER_TEMPLATE,
+            '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()
-
-    @classmethod
-    def teardownClass(cls):
-        """
-        Clear out database after test has completed
-        """
-        cls.destroy_db(cls.connection)
-
-    @staticmethod
-    def initdb():
+        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.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')
+        # 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):
         """
-        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.
         """
-        # 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
-            )
+        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, category, location=self.import_chapter_location, block_id=block_id
         )
-
-        return store
-
-    @staticmethod
-    def destroy_db(connection):
+        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_id, course_id)
+            self.assertEqual(chapter.location, self.import_chapter_location)
+
+    def initdb(self, default):
         """
-        Destroy the test db.
+        Initialize the database and create one test course in it
         """
-        connection.drop_database(DB)
-
-    def setUp(self):
-        # make a copy for convenience
-        self.connection = TestMixedModuleStore.connection
+        # set the default modulestore
+        self.options['stores']['default'] = self.options['stores'][default]
+        self.store = MixedModuleStore(**self.options)
+        self.addCleanup(self.store.close_all_connections)
+
+        self.course_locations = {
+            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(
+            category='chapter', name='Overview'
+        )
+        self.xml_chapter_location = self.course_locations[self.XML_COURSEID1].replace(
+            category='chapter', name='Overview'
+        )
+        # 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)
 
-    def tearDown(self):
-        pass
+        self._create_course(default, self.MONGO_COURSEID)
 
-    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.MONGO_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.XML_COURSEID1].replace(name='not_findable', category='problem')
         ))
+        self.assertFalse(self.store.has_item(self.MONGO_COURSEID, self.fake_location))
 
-    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 assert_raises(ItemNotFoundError):
+        with self.assertRaises(ItemNotFoundError):
             self.store.get_instance(
-                IMPORT_COURSEID, Location(['i4x', 'edX', 'toy', 'course', '2012_Fall'])
+                self.XML_COURSEID1,
+                self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem')
             )
-
-    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)
-
-        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')
-
-        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):
-        # we should have 3 total courses aggregated
+        with self.assertRaises(ItemNotFoundError):
+            self.store.get_instance(self.MONGO_COURSEID, self.fake_location)
+
+    @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)
+
+    @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)
+        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.MONGO_COURSEID], 'as_course_locator'):
+            locn = self.course_locations[self.MONGO_COURSEID]
+        else:
+            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")
+        course.show_calculator = True
+        self.store.update_item(course, None)
+        course = self.store.get_course(locn)
+        self.assertTrue(course.show_calculator)
+
+    @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.MONGO_COURSEID, self.import_chapter_location)
+
+    @ddt.data('direct', 'split')
+    def test_get_courses(self, default_ms):
+        self.initdb(default_ms)
+        # we should have 3 total courses across all stores
         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)
+        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)
 
     def test_xml_get_courses(self):
         """
         Test that the xml modulestore only loaded the courses from the maps.
         """
+        self.initdb('direct')
         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)
-
-    def test_get_course(self):
-        module = self.store.get_course(IMPORT_COURSEID)
-        assert_equals(module.location.course, self.import_course)
+        self.assertNotIn("edX/toy/TT_2012_Fall", course_ids)
 
-        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')
-
-    # pylint: disable=E1101
-    def test_get_parent_locations(self):
+    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)
+        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)
+
+    @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.MONGO_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.MONGO_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')
-
-class TestMixedMSInit(unittest.TestCase):
-    """
-    Test initializing w/o a reference_type
-    """
-    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.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)])
+
+    @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()
         )
-        self.store = MixedModuleStore(**options)
 
-    def test_use_locations(self):
+    @ddt.data('direct')
+    def test_create_item_from_parent_location(self, default_ms):
         """
-        Test that use_locations defaulted correctly
+        Test a code path missed by the above: passing an old-style location as parent but no
+        new location for the child
         """
-        self.assertTrue(self.store.use_locations)
+        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
+#=============================================================================================================
+
+
+def load_function(path):
+    """
+    Load a function by name.
+
+    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)
 
+
+# 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
+    """
+    class_ = load_function(engine)
+
+    return class_(
+        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)

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

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

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

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

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__)
@@ -580,22 +581,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):
     """
@@ -811,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:

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

lms/envs/acceptance.py

@@ -45,7 +45,6 @@ MODULESTORE = {
         'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
         'OPTIONS': {
             'mappings': {},
-            'reference_type': 'Location',
             'stores': {
                 'default': {
                     'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',

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

lms/envs/cms/dev.py

@@ -32,7 +32,6 @@ MODULESTORE = {
     'default': {
         'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
         'OPTIONS': {
-            'reference_type': 'Location',
             'mappings': {},
             'stores': {
                 'default': {

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.
@@ -440,7 +441,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 ############################################