Mixed no longer translates

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/item.py

@@ -202,7 +202,12 @@ def xblock_view_handler(request, package_id, view_name, tag=None, branch=None, v
                 log.debug("unable to render studio_view for %r", component, exc_info=True)
                 fragment = Fragment(render_to_string('html_error.html', {'message': str(exc)}))
 
-            store.save_xmodule(component)
+            # change not authored by requestor but by xblocks. should not convert to draft if not
+            # already draft
+            if getattr(component, 'is_draft', False):
+                modulestore('draft').update_item(component, None)
+            else:
+                modulestore('direct').update_item(component, None)
         elif view_name == 'student_view' and component.has_children:
             # For non-leaf xblocks on the unit page, show the special rendering
             # which links to the new container page.

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

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

@@ -479,7 +479,9 @@ class ModuleStoreReadBase(ModuleStoreRead):
         metadata_inheritance_cache_subsystem=None, request_cache=None,
         modulestore_update_signal=None, xblock_mixins=(), xblock_select=None,
         # temporary parms to enable backward compatibility. remove once all envs migrated
-        db=None, collection=None, host=None, port=None, tz_aware=True, user=None, password=None
+        db=None, collection=None, host=None, port=None, tz_aware=True, user=None, password=None,
+        # allow lower level init args to pass harmlessly
+        ** kwargs
     ):
         '''
         Set up the error-tracking logic.

common/lib/xmodule/xmodule/modulestore/locator.py

@@ -51,6 +51,12 @@ class Locator(object):
     def __eq__(self, other):
         return self.__dict__ == other.__dict__
 
+    def __hash__(self):
+        """
+        Hash on contents.
+        """
+        return hash(unicode(self))
+
     def __repr__(self):
         '''
         repr(self) returns something like this: CourseLocator("mit.eecs.6002x")
@@ -198,16 +204,16 @@ class CourseLocator(Locator):
         """
         Return a string representing this location.
         """
+        result = u""
         if self.package_id:
-            result = unicode(self.package_id)
+            result += unicode(self.package_id)
             if self.branch:
                 result += '/' + BRANCH_PREFIX + self.branch
-            return result
-        elif self.version_guid:
-            return u"{prefix}{guid}".format(prefix=VERSION_PREFIX, guid=self.version_guid)
-        else:
-            # raise InsufficientSpecificationError("missing package_id or version_guid")
-            return '<InsufficientSpecificationError: missing package_id or version_guid>'
+        if self.version_guid:
+            if self.package_id:
+                result += u"/"
+            result += u"{prefix}{guid}".format(prefix=VERSION_PREFIX, guid=self.version_guid)
+        return result
 
     def url(self):
         """
@@ -432,22 +438,29 @@ class BlockUsageLocator(CourseLocator):
 
     def version_agnostic(self):
         """
-        Returns a copy of itself.
-        If both version_guid and package_id are known, use a blank package_id in the copy.
-
         We don't care if the locator's version is not the current head; so, avoid version conflict
         by reducing info.
+        Returns a copy of itself without any version info.
+
+        :raises: ValueError if the block locator has no package_id
 
         :param block_locator:
         """
-        if self.version_guid:
-            return BlockUsageLocator(version_guid=self.version_guid,
-                                     branch=self.branch,
-                                     block_id=self.block_id)
-        else:
-            return BlockUsageLocator(package_id=self.package_id,
-                                     branch=self.branch,
-                                     block_id=self.block_id)
+        return BlockUsageLocator(package_id=self.package_id,
+                                 branch=self.branch,
+                                 block_id=self.block_id)
+
+    def course_agnostic(self):
+        """
+        We only care about the locator's version not its course.
+        Returns a copy of itself without any course info.
+
+        :raises: ValueError if the block locator has no version_guid
+
+        :param block_locator:
+        """
+        return BlockUsageLocator(version_guid=self.version_guid,
+                                 block_id=self.block_id)
 
     def set_block_id(self, new):
         """

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

@@ -5,58 +5,41 @@ In this way, courses can be served up both - say - XMLModuleStore or MongoModule
 
 """
 
-import re
-from importlib import import_module
 import logging
-from xblock.fields import Reference, ReferenceList, String
 
 from . import ModuleStoreWriteBase
 from xmodule.modulestore.django import create_modulestore_instance, loc_mapper
 from xmodule.modulestore import Location, SPLIT_MONGO_MODULESTORE_TYPE
-from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator
-from xmodule.modulestore.exceptions import InsufficientSpecificationError, ItemNotFoundError
-from xmodule.modulestore.parsers import ALLOWED_ID_CHARS
+from xmodule.modulestore.locator import CourseLocator
+from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError
 from uuid import uuid4
+from xmodule.modulestore.mongo.base import MongoModuleStore
+from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore
 
 log = logging.getLogger(__name__)
 
 
 class MixedModuleStore(ModuleStoreWriteBase):
     """
-    ModuleStore knows how to route requests to the right persistence ms and how to convert any
-    references in the xblocks to the type required by the app and the persistence layer.
+    ModuleStore knows how to route requests to the right persistence ms
     """
-    def __init__(self, mappings, stores, reference_type=None, i18n_service=None, **kwargs):
+    def __init__(self, mappings, stores, i18n_service=None, **kwargs):
         """
         Initialize a MixedModuleStore. Here we look into our passed in kwargs which should be a
         collection of other modulestore configuration informations
-
-        :param reference_type: either a class object such as Locator or Location or the fully
-        qualified dot-path to that class def to indicate what type of reference the app
-        uses.
         """
         super(MixedModuleStore, self).__init__(**kwargs)
 
         self.modulestores = {}
         self.mappings = mappings
-        # temporary code for transition period
-        if reference_type is None:
-            log.warn("reference_type not specified in MixedModuleStore settings. %s",
-                "Will default temporarily to the to-be-deprecated Location.")
-            self.reference_type = Location
-        elif isinstance(reference_type, basestring):
-            module_path, _, class_name = reference_type.rpartition('.')
-            class_ = getattr(import_module(module_path), class_name)
-            self.reference_type = class_
-        else:
-            self.reference_type = reference_type
 
         if 'default' not in stores:
             raise Exception('Missing a default modulestore in the MixedModuleStore __init__ method.')
 
-        for key, store in stores.items():
+        for key, store in stores.iteritems():
             is_xml = 'XMLModuleStore' in store['ENGINE']
             if is_xml:
+                # restrict xml to only load courses in mapping
                 store['OPTIONS']['course_ids'] = [
                     course_id
                     for course_id, store_key in self.mappings.iteritems()
@@ -69,8 +52,8 @@ class MixedModuleStore(ModuleStoreWriteBase):
                 store['OPTIONS'],
                 i18n_service=i18n_service,
             )
-            # it would be better to have a notion of read-only rather than hardcode
-            # key name
+            # If and when locations can identify their course, we won't need
+            # these loc maps. They're needed for figuring out which store owns these locations.
             if is_xml:
                 self.ensure_loc_maps_exist(key)
 
@@ -85,148 +68,6 @@ class MixedModuleStore(ModuleStoreWriteBase):
         mapping = self.mappings.get(course_id, 'default')
         return self.modulestores[mapping]
 
-    # TODO move the location converters to a helper class which returns a converter object w/ 2
-    # methods: convert_xblock and convert_reference. Then have mixed get the converter and use it.
-    def _locator_to_location(self, reference):
-        """
-        Convert the referenced locator to a location casting to and from a string as necessary
-        """
-        stringify = isinstance(reference, basestring)
-        if stringify:
-            reference = BlockUsageLocator(url=reference)
-        location = loc_mapper().translate_locator_to_location(reference)
-        return location.url() if stringify else location
-
-    def _location_to_locator(self, course_id, reference):
-        """
-        Convert the referenced location to a locator casting to and from a string as necessary
-        """
-        stringify = isinstance(reference, basestring)
-        if stringify:
-            reference = Location(reference)
-        locator = loc_mapper().translate_location(course_id, reference, reference.revision == None, True)
-        return unicode(locator) if stringify else locator
-
-    def _incoming_reference_adaptor(self, store, course_id, reference):
-        """
-        Convert the reference to the type the persistence layer wants
-        """
-        if reference is None:
-            return None
-        if issubclass(store.reference_type, self.reference_type):
-            return reference
-        if store.reference_type == Location:
-            return self._locator_to_location(reference)
-        return self._location_to_locator(course_id, reference)
-
-    def _outgoing_reference_adaptor(self, store, course_id, reference):
-        """
-        Convert the reference to the type the application wants
-        """
-        if reference is None:
-            return None
-        if issubclass(store.reference_type, self.reference_type):
-            return reference
-        if store.reference_type == Location:
-            return self._location_to_locator(course_id, reference)
-        return self._locator_to_location(reference)
-
-    def _xblock_adaptor_iterator(self, adaptor, string_converter, store, course_id, xblock):
-        """
-        Change all reference fields in this xblock to the type expected by the receiving layer
-        """
-        xblock.location = adaptor(store, course_id, xblock.location)
-        for field in xblock.fields.itervalues():
-            if field.is_set_on(xblock):
-                if isinstance(field, Reference):
-                    field.write_to(
-                        xblock,
-                        adaptor(store, course_id, field.read_from(xblock))
-                    )
-                elif isinstance(field, ReferenceList):
-                    field.write_to(
-                        xblock,
-                        [
-                            adaptor(store, course_id, ref)
-                            for ref in field.read_from(xblock)
-                        ]
-                    )
-                elif isinstance(field, String):
-                    # replace links within the string
-                    string_converter(field, xblock)
-        return xblock
-
-    def _incoming_xblock_adaptor(self, store, course_id, xblock):
-        """
-        Change all reference fields in this xblock to the type expected by the persistence layer
-        """
-        string_converter = self._get_string_converter(
-            course_id, store.reference_type, xblock.scope_ids.usage_id
-        )
-        return self._xblock_adaptor_iterator(
-            self._incoming_reference_adaptor, string_converter, store, course_id, xblock
-        )
-
-    def _outgoing_xblock_adaptor(self, store, course_id, xblock):
-        """
-        Change all reference fields in this xblock to the type expected by the persistence layer
-        """
-        string_converter = self._get_string_converter(
-            course_id, xblock.scope_ids.usage_id.__class__, xblock.scope_ids.usage_id
-        )
-        return self._xblock_adaptor_iterator(
-            self._outgoing_reference_adaptor, string_converter, store, course_id, xblock
-        )
-
-    CONVERT_RE = re.compile(r"/jump_to_id/({}+)".format(ALLOWED_ID_CHARS))
-
-    def _get_string_converter(self, course_id, reference_type, from_base_addr):
-        """
-        Return a closure which finds and replaces all embedded links in a string field
-        with the correct rewritten link for the target type
-        """
-        if issubclass(self.reference_type, reference_type):
-            return lambda field, xblock: None
-        if isinstance(from_base_addr, Location):
-            def mapper(found_id):
-                """
-                Convert the found id to BlockUsageLocator block_id
-                """
-                location = from_base_addr.replace(category=None, name=found_id)
-                # NOTE without category, it cannot create a new mapping if there's not one already
-                return loc_mapper().translate_location(course_id, location).block_id
-        else:
-            def mapper(found_id):
-                """
-                Convert the found id to Location block_id
-                """
-                locator = BlockUsageLocator.make_relative(from_base_addr, found_id)
-                return loc_mapper().translate_locator_to_location(locator).name
-
-        def converter(field, xblock):
-            """
-            Find all of the ids in the block and replace them w/ their mapped values
-            """
-            value = field.read_from(xblock)
-            self.CONVERT_RE.sub(mapper, value)
-            field.write_to(xblock, value)
-        return converter
-
-    def ensure_loc_maps_exist(self, store_name):
-        """
-        Ensure location maps exist for every course in the modulestore whose
-        name is the given name (mostly used for 'xml'). It creates maps for any
-        missing ones.
-
-        NOTE: will only work if the given store is Location based. If it's not,
-        it raises NotImplementedError
-        """
-        store = self.modulestores[store_name]
-        if store.reference_type != Location:
-            raise NotImplementedError(u"Cannot create maps from %s", store.reference_type)
-        for course in store.get_courses():
-            loc_mapper().translate_location(course.location.course_id, course.location)
-
     def has_item(self, course_id, reference):
         """
         Does the course include the xblock who's id is reference?
@@ -235,21 +76,19 @@ class MixedModuleStore(ModuleStoreWriteBase):
         :param reference: a Location or BlockUsageLocator
         """
         store = self._get_modulestore_for_courseid(course_id)
-        decoded_ref = self._incoming_reference_adaptor(store, course_id, reference)
-        return store.has_item(course_id, decoded_ref)
+        return store.has_item(course_id, reference)
 
     def get_item(self, location, depth=0):
         """
         This method is explicitly not implemented as we need a course_id to disambiguate
         We should be able to fix this when the data-model rearchitecting is done
         """
+        # Although we shouldn't have both get_item and get_instance imho
         raise NotImplementedError
 
     def get_instance(self, course_id, location, depth=0):
         store = self._get_modulestore_for_courseid(course_id)
-        decoded_ref = self._incoming_reference_adaptor(store, course_id, location)
-        xblock = store.get_instance(course_id, decoded_ref, depth)
-        return self._outgoing_xblock_adaptor(store, course_id, xblock)
+        return store.get_instance(course_id, location, depth)
 
     def get_items(self, location, course_id=None, depth=0, qualifiers=None):
         """
@@ -262,44 +101,15 @@ class MixedModuleStore(ModuleStoreWriteBase):
         a Locator with at least a package_id and branch but possibly no block_id.
 
         depth: An argument that some module stores may use to prefetch
-            descendents of the queried modules for more efficient results later
+            descendants of the queried modules for more efficient results later
             in the request. The depth is counted in the number of calls to
-            get_children() to cache. None indicates to cache all descendents
+            get_children() to cache. None indicates to cache all descendants
         """
         if not (course_id or hasattr(location, 'package_id')):
             raise Exception("Must pass in a course_id when calling get_items()")
 
         store = self._get_modulestore_for_courseid(course_id or getattr(location, 'package_id'))
-        if not issubclass(self.reference_type, store.reference_type):
-            if store.reference_type == Location:
-                if getattr(location, 'block_id', False):
-                    location = self._incoming_reference_adaptor(store, course_id, location)
-                else:
-                    # get the course's location
-                    location = loc_mapper().translate_locator_to_location(location, get_course=True)
-                    # now remove the unknowns
-                    location = location.replace(
-                        category=qualifiers.get('category', None),
-                        name=None
-                    )
-            else:
-                if not isinstance(location, Location):
-                    location = Location(location)
-                try:
-                    location.ensure_fully_specified()
-                    location = loc_mapper().translate_location(
-                        course_id, location, location.revision == None, True
-                    )
-                except InsufficientSpecificationError:
-                    # construct the Locator by hand
-                    if location.category is not None and qualifiers.get('category', False):
-                        qualifiers['category'] = location.category
-                    location = loc_mapper().translate_location_to_course_locator(
-                        course_id, location, location.revision == None
-                    )
-        xblocks = store.get_items(location, course_id, depth, qualifiers)
-        xblocks = [self._outgoing_xblock_adaptor(store, course_id, xblock) for xblock in xblocks]
-        return xblocks
+        return store.get_items(location, course_id, depth, qualifiers)
 
     def _get_course_id_from_course_location(self, course_location):
         """
@@ -312,35 +122,39 @@ class MixedModuleStore(ModuleStoreWriteBase):
         Returns a list containing the top level XModuleDescriptors of the courses
         in this modulestore.
         '''
-        courses = []
-        for key in self.modulestores:
-            store = self.modulestores[key]
+        courses = {}
+        # order the modulestores and ensure no dupes: an awkward bit of hardcoding to ensure precedence
+        # xml is in here because mappings trump discovery
+        stores = [self.modulestores['default'], self.modulestores['xml']]
+        for key, store in self.modulestores.iteritems():
+            # awkward hardcoding of knowledge that 'draft' is a dupe of 'direct'
+            if key != 'draft' and store not in stores:
+                stores.append(store)
+
+        has_locators = False
+        for store in stores:
             store_courses = store.get_courses()
-            # If the store has not been labeled as 'default' then we should
-            # only surface courses that have a mapping entry, for example the XMLModuleStore will
-            # slurp up anything that is on disk, however, we don't want to surface those to
-            # consumers *unless* there is an explicit mapping in the configuration
-            # TODO obviously this filtering only applies to filebased stores
-            if key != 'default':
-                for course in store_courses:
-                    course_id = self._get_course_id_from_course_location(course.location)
-                    # make sure that the courseId is mapped to the store in question
-                    if key == self.mappings.get(course_id, 'default'):
-                        courses.append(
-                            self._outgoing_reference_adaptor(store, course_id, course.location)
-                        )
-            else:
-                # if we're the 'default' store provider, then we surface all courses hosted in
-                # that store provider
-                store_courses = [
-                    self._outgoing_reference_adaptor(
-                        store, self._get_course_id_from_course_location(course.location), course.location
-                    )
-                    for course in store_courses
-                ]
-                courses = courses + store_courses
-
-        return courses
+            # filter out ones which were fetched from earlier stores but locations may not be ==
+            for course in store_courses:
+                course_location = unicode(course.location)
+                if course_location not in courses:
+                    if has_locators and isinstance(course.location, Location):
+                        try:
+                            # if there's no existing mapping, then the course can't have been in split
+                            course_locator = loc_mapper().translate_location(
+                                course.location.course_id, course.location, add_entry_if_missing=False
+                            )
+                            if unicode(course_locator) not in courses:
+                                courses[course_location] = course
+                        except ItemNotFoundError:
+                            courses[course_location] = course
+                    elif isinstance(course.location, CourseLocator):
+                        has_locators = True
+                        courses[course_location] = course
+                    else:
+                        courses[course_location] = course
+
+        return courses.values()
 
     def get_course(self, course_id):
         """
@@ -350,46 +164,19 @@ class MixedModuleStore(ModuleStoreWriteBase):
         :param course_id: must be either a string course_id or a CourseLocator
         """
         store = self._get_modulestore_for_courseid(
-            course_id.package_id if hasattr(course_id, 'package_id') else course_id)
+            course_id.package_id if hasattr(course_id, 'package_id') else course_id
+        )
         try:
-            # translate won't work w/ missing fields so work around it
-            if store.reference_type == Location:
-                # takes the course_id: figure out if this is old or new style
-                if not issubclass(store.reference_type, self.reference_type):
-                    if isinstance(course_id, basestring):
-                        course_id = CourseLocator(package_id=course_id, branch='published')
-                    course_location = loc_mapper().translate_locator_to_location(course_id, get_course=True)
-                    course_id = course_location.course_id
-                xblock = store.get_course(course_id)
-            else:
-                # takes a courseLocator
-                if isinstance(course_id, CourseLocator):
-                    location = course_id
-                    course_id = None  # not an old style course_id; so, don't use it further
-                elif '/' in course_id:
-                    location = loc_mapper().translate_location_to_course_locator(course_id, None, True)
-                else:
-                    location = CourseLocator(package_id=course_id, branch='published')
-                    course_id = None  # not an old style course_id; so, don't use it further
-                xblock = store.get_course(location)
+            return store.get_course(course_id)
         except ItemNotFoundError:
             return None
-        if xblock is not None:
-            return self._outgoing_xblock_adaptor(store, course_id, xblock)
-        else:
-            return None
 
     def get_parent_locations(self, location, course_id):
         """
         returns the parent locations for a given location and course_id
         """
         store = self._get_modulestore_for_courseid(course_id)
-        decoded_ref = self._incoming_reference_adaptor(store, course_id, location)
-        parents = store.get_parent_locations(decoded_ref, course_id)
-        return [
-            self._outgoing_reference_adaptor(store, course_id, reference)
-            for reference in parents
-        ]
+        return store.get_parent_locations(location, course_id)
 
     def get_modulestore_type(self, course_id):
         """
@@ -409,8 +196,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
         """
         course_id = getattr(course_location, 'course_id', getattr(course_location, 'package_id', None))
         store = self._get_modulestore_for_courseid(course_id)
-        decoded_ref = self._incoming_reference_adaptor(store, course_id, course_location)
-        return store.get_orphans(decoded_ref, branch)
+        return store.get_orphans(course_location, branch)
 
     def get_errored_courses(self):
         """
@@ -426,13 +212,16 @@ class MixedModuleStore(ModuleStoreWriteBase):
         """
         Get the course_id from the block or from asking its store. Expensive.
         """
-        if block.course_id is not None:
-            return block.course_id
+        try:
+            if block.course_id is not None:
+                return block.course_id
+        except AssertionError:  # will occur if no xmodule set
+            pass
         try:
             course = store._get_course_for_item(block.scope_ids.usage_id)
             if course:
                 return course.scope_ids.usage_id.course_id
-        except:  # sorry, that method just raises vanilla Exception
+        except:  # sorry, that method just raises vanilla Exception if it doesn't find course
             pass
 
     def _infer_course_id_try(self, location):
@@ -442,10 +231,27 @@ class MixedModuleStore(ModuleStoreWriteBase):
         proper modulestore. This method attempts several sound but not complete methods.
         :param location: an old style Location
         """
+        if isinstance(location, CourseLocator):
+            return location.package_id
+        elif isinstance(location, basestring):
+            try:
+                location = Location(location)
+            except InvalidLocationError:
+                # try to parse as a course_id
+                try:
+                    Location.parse_course_id(location)
+                    # it's already a course_id
+                    return location
+                except ValueError:
+                    # cannot interpret the location
+                    return None
+
+        # location is a Location at this point
         if location.category == 'course':  # easiest case
             return location.course_id
         # try finding in loc_mapper
         try:
+            # see if the loc mapper knows the course id (requires double translation)
             locator = loc_mapper().translate_location_to_course_locator(None, location)
             location = loc_mapper().translate_locator_to_location(locator, get_course=True)
             return location.course_id
@@ -455,16 +261,19 @@ class MixedModuleStore(ModuleStoreWriteBase):
         for store in self.modulestores.itervalues():
             if isinstance(location, store.reference_type):
                 try:
-                    block = store.get_item(location)
-                    course_id = self._get_course_id_from_block(block, store)
+                    xblock = store.get_item(location)
+                    course_id = self._get_course_id_from_block(xblock, store)
                     if course_id is not None:
                         return course_id
                 except NotImplementedError:
                     blocks = store.get_items(location)
                     if len(blocks) == 1:
                         block = blocks[0]
-                        if block.course_id is not None:
-                            return block.course_id
+                        try:
+                            if block.course_id is not None:
+                                return block.course_id
+                        except AssertionError:
+                            pass
                 except ItemNotFoundError:
                     pass
         # if we get here, it must be in a Locator based store, but we won't be able to find
@@ -481,19 +290,20 @@ class MixedModuleStore(ModuleStoreWriteBase):
 
         NOTE: unlike the other mixed modulestore methods, this does not adapt its argument
         to the persistence store but requires its caller to know what the persistence store
-        wants for args. It does not translate any references on the way in; so, don't
-        pass children or other reference fields here.
-        It does, however, adapt the xblock on the way out to the app's
-        reference_type
+        wants for args.
 
         :returns: course xblock
         """
         store = self.modulestores[store_name]
         if not hasattr(store, 'create_course'):
-            raise NotImplementedError(u"Cannot create a course on store %s", store_name)
+            raise NotImplementedError(u"Cannot create a course on store %s" % store_name)
         if store.get_modulestore_type(course_location.course_id) == SPLIT_MONGO_MODULESTORE_TYPE:
             org = kwargs.pop('org', course_location.org)
             pretty_id = kwargs.pop('pretty_id', None)
+            fields = kwargs.get('fields', {})
+            fields.update(kwargs.pop('metadata', {}))
+            fields.update(kwargs.pop('definition_data', {}))
+            kwargs['fields'] = fields
             # TODO rename id_root to package_id for consistency. It's too confusing
             id_root = kwargs.pop('id_root', u"{0.org}.{0.course}.{0.name}".format(course_location))
             course = store.create_course(
@@ -509,7 +319,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
             course = store.create_course(course_location, **kwargs)
             loc_mapper().translate_location(course_location.course_id, course_location)
 
-        return self._outgoing_xblock_adaptor(store, course_location.course_id, course)
+        return course
 
     def create_item(self, course_or_parent_loc, category, user_id=None, **kwargs):
         """
@@ -520,22 +330,17 @@ class MixedModuleStore(ModuleStoreWriteBase):
         Adds an entry to the loc map using the kwarg location if provided (must be a
         Location if provided) or block_id and category if provided.
 
-        :param course_or_parent_loc: will be translated appropriately to the course's store.
-        Can be a course_id (org/course/run), CourseLocator, Location, or BlockUsageLocator.
+        :param course_or_parent_loc: Can be a course_id (org/course/run), CourseLocator,
+        Location, or BlockUsageLocator but must be what the persistence modulestore expects
         """
         # find the store for the course
-        if self.reference_type == Location:
-            if hasattr(course_or_parent_loc, 'tag'):
-                course_id = self._infer_course_id_try(course_or_parent_loc)
-            else:
-                course_id = course_or_parent_loc
-        else:
-            course_id = course_or_parent_loc.package_id
+        course_id = self._infer_course_id_try(course_or_parent_loc)
+
         store = self._get_modulestore_for_courseid(course_id)
 
         location = kwargs.pop('location', None)
         # invoke its create_item
-        if store.reference_type == Location:
+        if isinstance(store, MongoModuleStore):
             # convert parent loc if it's legit
             block_id = kwargs.pop('block_id', uuid4().hex)
             if isinstance(course_or_parent_loc, basestring):
@@ -546,7 +351,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
                     locn_dict['name'] = block_id
                     location = Location(locn_dict)
             else:
-                parent_loc = self._incoming_reference_adaptor(store, course_id, course_or_parent_loc)
+                parent_loc = course_or_parent_loc
                 # must have a legitimate location, compute if appropriate
                 if location is None:
                     location = parent_loc.replace(category=category, name=block_id)
@@ -559,7 +364,7 @@ class MixedModuleStore(ModuleStoreWriteBase):
                 parent = store.get_item(parent_loc)
                 parent.children.append(location.url())
                 store.update_item(parent)
-        else:
+        elif isinstance(store, SplitMongoModuleStore):
             if isinstance(course_or_parent_loc, basestring): # course_id
                 old_course_id = course_or_parent_loc
                 course_or_parent_loc = loc_mapper().translate_location_to_course_locator(
@@ -570,13 +375,15 @@ class MixedModuleStore(ModuleStoreWriteBase):
                     course_or_parent_loc, get_course=True
                 )
                 old_course_id = old_course_loc.course_id
-            else:  # it's a Location
-                old_course_id = course_id
-                course_or_parent_loc = self._location_to_locator(course_id, course_or_parent_loc)
+            else:
+                raise ValueError(u"Cannot create a child of {} in split. Wrong repr.".format(course_or_parent_loc))
+
+            # split handles all the fields in one dict not separated by scope
             fields = kwargs.get('fields', {})
             fields.update(kwargs.pop('metadata', {}))
             fields.update(kwargs.pop('definition_data', {}))
             kwargs['fields'] = fields
+
             xblock = store.create_item(course_or_parent_loc, category, user_id, **kwargs)
             if location is None:
                 locn_dict = Location.parse_course_id(old_course_id)
@@ -587,6 +394,9 @@ class MixedModuleStore(ModuleStoreWriteBase):
             loc_mapper().translate_location(
                 old_course_id, location, passed_block_id=xblock.location.block_id
             )
+        else:
+            raise NotImplementedError(u"Cannot create an item on store %s" % store)
+
         return xblock
 
     def update_item(self, xblock, user_id, allow_not_found=False):
@@ -594,37 +404,21 @@ class MixedModuleStore(ModuleStoreWriteBase):
         Update the xblock persisted to be the same as the given for all types of fields
         (content, children, and metadata) attribute the change to the given user.
         """
-        if self.reference_type == Location:
-            course_id = xblock.course_id
-            if course_id is None:
-                course_id = self._infer_course_id_try(xblock.scope_ids.usage_id)
-                if course_id is None:
-                    raise ItemNotFoundError(u"Cannot find modulestore for %s", xblock.scope_ids.usage_id)
-        else:
-            locator = xblock.scope_ids.usage_id
-            course_id = locator.package_id
+        course_id = self._infer_course_id_try(xblock.scope_ids.usage_id)
+        if course_id is None:
+            raise ItemNotFoundError(u"Cannot find modulestore for %s" % xblock.scope_ids.usage_id)
         store = self._get_modulestore_for_courseid(course_id)
-
-        # if an xblock, convert its contents to correct addr scheme
-        xblock = self._incoming_xblock_adaptor(store, course_id, xblock)
-        xblock = store.update_item(xblock, user_id)
-
-        return self._outgoing_xblock_adaptor(store, course_id, xblock)
+        return store.update_item(xblock, user_id)
 
     def delete_item(self, location, user_id=None):
         """
         Delete the given item from persistence.
         """
-        if self.reference_type == Location:
-            course_id = self._infer_course_id_try(location)
-            if course_id is None:
-                raise ItemNotFoundError(u"Cannot find modulestore for %s", location)
-        else:
-            course_id = location.package_id
+        course_id = self._infer_course_id_try(location)
+        if course_id is None:
+            raise ItemNotFoundError(u"Cannot find modulestore for %s" % location)
         store = self._get_modulestore_for_courseid(course_id)
-
-        decoded_ref = self._incoming_reference_adaptor(store, course_id, location)
-        return store.delete_item(decoded_ref, user_id=user_id)
+        return store.delete_item(location, user_id=user_id)
 
     def close_all_connections(self):
         """
@@ -636,3 +430,17 @@ class MixedModuleStore(ModuleStoreWriteBase):
             elif hasattr(mstore, 'db'):
                 mstore.db.connection.close()
 
+    def ensure_loc_maps_exist(self, store_name):
+        """
+        Ensure location maps exist for every course in the modulestore whose
+        name is the given name (mostly used for 'xml'). It creates maps for any
+        missing ones.
+
+        NOTE: will only work if the given store is Location based. If it's not,
+        it raises NotImplementedError
+        """
+        store = self.modulestores[store_name]
+        if store.reference_type != Location:
+            raise NotImplementedError(u"Cannot create maps from %s" % store.reference_type)
+        for course in store.get_courses():
+            loc_mapper().translate_location(course.location.course_id, course.location)

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

@@ -633,8 +633,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
             raise ValueError(u"Course roots must be of category 'course': {}".format(unicode(location)))
         return self.create_and_save_xmodule(location, definition_data, metadata, runtime)
 
-    def create_xmodule(self, location, definition_data=None, metadata=None, system=None,
-                       fields={}):
+    def create_xmodule(self, location, definition_data=None, metadata=None, system=None, fields={}):
         """
         Create the new xmodule but don't save it. Returns the new module.
 

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

@@ -92,7 +92,7 @@ class DraftModuleStore(MongoModuleStore):
         except ItemNotFoundError:
             return wrap_draft(super(DraftModuleStore, self).get_instance(course_id, location, depth=depth))
 
-    def create_xmodule(self, location, definition_data=None, metadata=None, system=None):
+    def create_xmodule(self, location, definition_data=None, metadata=None, system=None, fields={}):
         """
         Create the new xmodule but don't save it. Returns the new module with a draft locator
 
@@ -104,7 +104,7 @@ class DraftModuleStore(MongoModuleStore):
         draft_loc = as_draft(location)
         if draft_loc.category in DIRECT_ONLY_CATEGORIES:
             raise InvalidVersionError(location)
-        return super(DraftModuleStore, self).create_xmodule(draft_loc, definition_data, metadata, system)
+        return super(DraftModuleStore, self).create_xmodule(draft_loc, definition_data, metadata, system, fields)
 
     def get_items(self, location, course_id=None, depth=0, qualifiers=None):
         """

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

@@ -556,8 +556,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         The block's history tracks its explicit changes but not the changes in its children.
 
         '''
-        # version_agnostic means we don't care if the head and version don't align, trust the version
-        course_struct = self._lookup_course(block_locator.version_agnostic())['structure']
+        # course_agnostic means we don't care if the head and version don't align, trust the version
+        course_struct = self._lookup_course(block_locator.course_agnostic())['structure']
         block_id = block_locator.block_id
         update_version_field = 'blocks.{}.edit_info.update_version'.format(block_id)
         all_versions_with_block = self.db_connection.find_matching_structures({'original_version': course_struct['original_version'],

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

@@ -33,7 +33,6 @@ def mixed_store_config(data_dir, mappings):
             'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
             'OPTIONS': {
                 'mappings': mappings,
-                'reference_type': 'xmodule.modulestore.Location',
                 'stores': {
                     'default': mongo_config['default'],
                     'xml': xml_config['default']
@@ -219,14 +218,17 @@ class ModuleStoreTestCase(TestCase):
         # even if we're using a mixed modulestore
         store = editable_modulestore()
         if hasattr(store, 'collection'):
+            connection = store.collection.database.connection
             store.collection.drop()
-            store.db.connection.close()
+            connection.close()
         elif hasattr(store, 'close_all_connections'):
             store.close_all_connections()
+
         if contentstore().fs_files:
             db = contentstore().fs_files.database
             db.connection.drop_database(db)
             db.connection.close()
+
         location_mapper = loc_mapper()
         if location_mapper.db:
             location_mapper.location_map.drop()

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

 import pymongo
 from uuid import uuid4
-import copy
 import ddt
-from mock import patch
+from mock import patch, Mock
+from importlib import import_module
 
 from xmodule.tests import DATA_DIR
 from xmodule.modulestore import Location, MONGO_MODULESTORE_TYPE, SPLIT_MONGO_MODULESTORE_TYPE, \
     XML_MODULESTORE_TYPE
 from xmodule.modulestore.exceptions import ItemNotFoundError
 
-from xmodule.modulestore.locator import BlockUsageLocator
+from xmodule.modulestore.locator import BlockUsageLocator, CourseLocator
 from xmodule.modulestore.tests.test_location_mapper import LocMapperSetupSansDjango, loc_mapper
-
-# FIXME remove settings
+# Mixed modulestore depends on django, so we'll manually configure some django settings
+# before importing the module
 from django.conf import settings
 if not settings.configured:
     settings.configure()
-
 from xmodule.modulestore.mixed import MixedModuleStore
 
+
 @ddt.ddt
 class TestMixedModuleStore(LocMapperSetupSansDjango):
     """
@@ -32,7 +32,6 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
     FS_ROOT = DATA_DIR
     DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor'
     RENDER_TEMPLATE = lambda t_n, d, ctx = None, nsp = 'main': ''
-    REFERENCE_TYPE = 'xmodule.modulestore.Location'
 
     IMPORT_COURSEID = 'MITx/999/2013_Spring'
     XML_COURSEID1 = 'edX/toy/2012_Fall'
@@ -54,7 +53,6 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
             XML_COURSEID2: 'xml',
             IMPORT_COURSEID: 'default'
         },
-        'reference_type': REFERENCE_TYPE,
         'stores': {
             'xml': {
                 'ENGINE': 'xmodule.modulestore.xml.XMLModuleStore',
@@ -101,10 +99,13 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         self.connection.drop_database(self.DB)
         self.addCleanup(self.connection.drop_database, self.DB)
         self.addCleanup(self.connection.close)
-        
         super(TestMixedModuleStore, self).setUp()
 
-        patcher = patch('xmodule.modulestore.mixed.loc_mapper', return_value=LocMapperSetupSansDjango.loc_store)
+        patcher = patch.multiple(
+            'xmodule.modulestore.mixed',
+            loc_mapper=Mock(return_value=LocMapperSetupSansDjango.loc_store),
+            create_modulestore_instance=create_modulestore_instance,
+        )
         patcher.start()
         self.addCleanup(patcher.stop)
         self.addTypeEqualityFunc(BlockUsageLocator, '_compareIgnoreVersion')
@@ -115,28 +116,26 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         NOTE: course_location and item_location must be Location regardless of the app reference type in order
         to cause the right mapping to be created.
         """
-        if default == 'split':
-            course = self.store.create_course(course_location, store_name=default)
-            chapter = self.store.create_item(
-                # don't use course_location as it may not be the repr
-                course.location, item_location.category, location=item_location, block_id=item_location.name
-            )
-        else:
-            course = self.store.create_course(
-                course_location, store_name=default, metadata={'display_name': course_location.name}
-            )
-            chapter = self.store.create_item(course_location, item_location.category, location=item_location)
-        if self.REFERENCE_TYPE == 'xmodule.modulestore.locator.CourseLocator':
+        course = self.store.create_course(
+            course_location, store_name=default, metadata={'display_name': course_location.name}
+        )
+        chapter = self.store.create_item(
+            # don't use course_location as it may not be the repr
+            course.location, item_location.category, location=item_location, block_id=item_location.name
+        )
+        if isinstance(course.location, CourseLocator):
             # add_entry is false b/c this is a test that the right thing happened w/o
             # wanting any additional side effects
             lookup_map = loc_mapper().translate_location(
                 course_location.course_id, course_location, add_entry_if_missing=False
             )
             self.assertEqual(lookup_map, course.location)
+            self.course_locations[self.IMPORT_COURSEID] = course.location.version_agnostic()
             lookup_map = loc_mapper().translate_location(
                 course_location.course_id, item_location, add_entry_if_missing=False
             )
             self.assertEqual(lookup_map, chapter.location)
+            self.import_chapter_location = chapter.location.version_agnostic()
         else:
             self.assertEqual(course.location, course_location)
             self.assertEqual(chapter.location, item_location)
@@ -154,8 +153,10 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
             """
             Generate the locations for the given ids
             """
-            org, course, run = course_id.split('/')
-            return Location('i4x', org, course, 'course', run)
+            course_dict = Location.parse_course_id(course_id)
+            course_dict['tag'] = 'i4x'
+            course_dict['category'] = 'course'
+            return Location(course_dict)
 
         self.course_locations = {
              course_id: generate_location(course_id)
@@ -171,18 +172,8 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         # grab old style location b4 possibly converted
         import_location = self.course_locations[self.IMPORT_COURSEID]
         # get Locators and set up the loc mapper if app is Locator based
-        if self.REFERENCE_TYPE == 'xmodule.modulestore.locator.CourseLocator':
+        if default == 'split':
             self.fake_location = loc_mapper().translate_location('foo/bar/2012_Fall', self.fake_location)
-            self.import_chapter_location = loc_mapper().translate_location(
-                self.IMPORT_COURSEID, self.import_chapter_location
-            )
-            self.xml_chapter_location = loc_mapper().translate_location(
-                self.XML_COURSEID1, self.xml_chapter_location
-            )
-            self.course_locations = {
-                course_id: loc_mapper().translate_location(course_id, locn)
-                for course_id, locn in self.course_locations.iteritems()
-            }
 
         self._create_course(default, import_location, self.import_chapter_location)
 
@@ -206,8 +197,11 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
             self.assertTrue(self.store.has_item(course_id, course_locn))
 
         # try negative cases
-        self.assertFalse(self.store.has_item(self.XML_COURSEID1, self.course_locations[self.IMPORT_COURSEID]))
-        self.assertFalse(self.store.has_item(self.IMPORT_COURSEID, self.course_locations[self.XML_COURSEID1]))
+        self.assertFalse(self.store.has_item(
+                self.XML_COURSEID1,
+                self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem')
+        ))
+        self.assertFalse(self.store.has_item(self.IMPORT_COURSEID, self.fake_location))
 
     @ddt.data('direct', 'split')
     def test_get_item(self, default_ms):
@@ -223,9 +217,12 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
 
         # try negative cases
         with self.assertRaises(ItemNotFoundError):
-            self.store.get_instance(self.XML_COURSEID1, self.course_locations[self.IMPORT_COURSEID])
+            self.store.get_instance(
+                self.XML_COURSEID1,
+                self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem')
+            )
         with self.assertRaises(ItemNotFoundError):
-            self.store.get_instance(self.IMPORT_COURSEID, self.course_locations[self.XML_COURSEID1])
+            self.store.get_instance(self.IMPORT_COURSEID, self.fake_location)
 
     @ddt.data('direct', 'split')
     def test_get_items(self, default_ms):
@@ -246,11 +243,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         Update should fail for r/o dbs and succeed for r/w ones
         """
         self.initdb(default_ms)
-        # try a r/o db
-        if self.REFERENCE_TYPE == 'xmodule.modulestore.locator.CourseLocator':
-            course_id = self.course_locations[self.XML_COURSEID1]
-        else:
-            course_id = self.XML_COURSEID1
+        course_id = self.XML_COURSEID1
         course = self.store.get_course(course_id)
         # if following raised, then the test is really a noop, change it
         self.assertFalse(course.show_calculator, "Default changed making test meaningless")
@@ -288,10 +281,14 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
     @ddt.data('direct', 'split')
     def test_get_courses(self, default_ms):
         self.initdb(default_ms)
-        # we should have 3 total courses aggregated
+        # we should have 3 total courses across all stores
         courses = self.store.get_courses()
         self.assertEqual(len(courses), 3)
-        course_ids = [course.location for course in courses]
+        course_ids = [
+            course.location.version_agnostic()
+            if hasattr(course.location, 'version_agnostic') else course.location
+            for course in courses
+        ]
         self.assertIn(self.course_locations[self.IMPORT_COURSEID], course_ids)
         self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids)
         self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids)
@@ -300,6 +297,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         """
         Test that the xml modulestore only loaded the courses from the maps.
         """
+        self.initdb('direct')
         courses = self.store.modulestores['xml'].get_courses()
         self.assertEqual(len(courses), 2)
         course_ids = [course.location.course_id for course in courses]
@@ -339,33 +337,28 @@ class TestMixedModuleStore(LocMapperSetupSansDjango):
         self.assertEqual(len(parents), 1)
         self.assertEqual(parents[0], self.course_locations[self.XML_COURSEID1])
 
-@ddt.ddt
-class TestMixedUseLocator(TestMixedModuleStore):
+#=============================================================================================================
+# General utils for not using django settings
+#=============================================================================================================
+
+def load_function(path):
     """
-    Tests a mixed ms which uses Locators instead of Locations
+    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`
     """
-    REFERENCE_TYPE = 'xmodule.modulestore.locator.CourseLocator'
+    module_path, _, name = path.rpartition('.')
+    return getattr(import_module(module_path), name)
 
-    def setUp(self):
-        self.options = copy.copy(self.OPTIONS)
-        self.options['reference_type'] = self.REFERENCE_TYPE
-        super(TestMixedUseLocator, self).setUp()
 
-@ddt.ddt
-class TestMixedMSInit(TestMixedModuleStore):
+def create_modulestore_instance(engine, doc_store_config, options, i18n_service=None):
     """
-    Test initializing w/o a reference_type
+    This will return a new instance of a modulestore given an engine and options
     """
-    REFERENCE_TYPE = None
-    def setUp(self):
-        self.options = copy.copy(self.OPTIONS)
-        del self.options['reference_type']
-        super(TestMixedMSInit, self).setUp()
+    class_ = load_function(engine)
 
-    @ddt.data('direct', 'split')
-    def test_use_locations(self, default_ms):
-        """
-        Test that use_locations defaulted correctly
-        """
-        self.initdb(default_ms)
-        self.assertEqual(self.store.reference_type, Location)
+    return class_(
+        doc_store_config=doc_store_config,
+        **options
+    )

lms/envs/acceptance.py

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

lms/envs/cms/dev.py

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