making mixed ms capable of front ending all modulestores for most operations including CRUD

STUD-600

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/views/component.py

@@ -23,7 +23,7 @@ from xblock.exceptions import NoSuchHandlerError
 from xblock.fields import Scope
 from xblock.plugin import PluginMissingError
 from xblock.runtime import Mixologist
-from xmodule.x_module import prefer_xmodules
+from xmodule.modulestore import prefer_xmodules
 
 from lms.lib.xblock.runtime import unquote_slashes
 
@@ -370,6 +370,6 @@ def component_handler(request, usage_id, handler, suffix=''):
         log.info("XBlock %s attempted to access missing handler %r", descriptor, handler, exc_info=True)
         raise Http404
 
-    modulestore().save_xmodule(descriptor)
+    modulestore().update_item(descriptor)
 
     return webob_to_django_response(resp)

cms/djangoapps/contentstore/views/item.py

@@ -521,8 +521,8 @@ def orphan_handler(request, tag=None, package_id=None, branch=None, version_guid
     if request.method == 'DELETE':
         if request.user.is_staff:
             items = modulestore().get_orphans(old_location, 'draft')
-            for item in items:
-                modulestore('draft').delete_item(item, delete_all_versions=True)
+            for itemloc in items:
+                modulestore('draft').delete_item(itemloc, delete_all_versions=True)
             return JsonResponse({'deleted': items})
         else:
             raise PermissionDenied()

cms/envs/common.py

@@ -34,7 +34,8 @@ from path import path
 from lms.lib.xblock.mixin import LmsBlockMixin
 from cms.lib.xblock.mixin import CmsBlockMixin
 from xmodule.modulestore.inheritance import InheritanceMixin
-from xmodule.x_module import XModuleMixin, only_xmodules
+from xmodule.modulestore import only_xmodules
+from xmodule.x_module import XModuleMixin
 from dealer.git import git
 
 ############################ FEATURE CONFIGURATION #############################
@@ -220,7 +221,7 @@ XBLOCK_SELECT_FUNCTION = only_xmodules
 # either by uncommenting them here, or adding them to your private.py
 # You should also enable the ALLOW_ALL_ADVANCED_COMPONENTS feature flag, so that
 # xblocks can be added via advanced settings
-# from xmodule.x_module import prefer_xmodules
+# from xmodule.modulestore import prefer_xmodules
 # XBLOCK_SELECT_FUNCTION = prefer_xmodules
 
 ############################ SIGNAL HANDLERS ################################

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/modulestore/__init__.py

@@ -7,11 +7,15 @@ import logging
 import re
 
 from collections import namedtuple
+import collections
 
 from abc import ABCMeta, abstractmethod
+from xblock.plugin import default_select
 
 from .exceptions import InvalidLocationError, InsufficientSpecificationError
 from xmodule.errortracker import make_error_tracker
+from xblock.runtime import Mixologist
+from xblock.core import XBlock
 
 log = logging.getLogger('edx.modulestore')
 
@@ -529,9 +533,75 @@ class ModuleStoreReadBase(ModuleStoreRead):
                 return c
         return None
 
+    def update_item(self, xblock, user_id=None, allow_not_found=False, force=False):
+        """
+        Update the given xblock's persisted repr. Pass the user's unique id which the persistent store
+        should save with the update if it has that ability.
+
+        :param allow_not_found: whether this method should raise an exception if the given xblock
+        has not been persisted before.
+        :param force: fork the structure and don't update the course draftVersion if there's a version
+        conflict (only applicable to version tracking and conflict detecting persistence stores)
+
+        :raises VersionConflictError: if package_id and version_guid given and the current
+        version head != version_guid and force is not True. (only applicable to version tracking stores)
+        """
+        raise NotImplementedError
+
+    def delete_item(self, location, user_id=None, delete_all_versions=False, delete_children=False, force=False):
+        """
+        Delete an item from persistence. Pass the user's unique id which the persistent store
+        should save with the update if it has that ability.
+
+        :param delete_all_versions: removes both the draft and published version of this item from
+        the course if using draft and old mongo. Split may or may not implement this.
+        :param force: fork the structure and don't update the course draftVersion if there's a version
+        conflict (only applicable to version tracking and conflict detecting persistence stores)
+
+        :raises VersionConflictError: if package_id and version_guid given and the current
+        version head != version_guid and force is not True. (only applicable to version tracking stores)
+        """
+        raise NotImplementedError
 
 class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite):
     '''
     Implement interface functionality that can be shared.
     '''
-    pass
+    def __init__(self, **kwargs):
+        super(ModuleStoreWriteBase, self).__init__(**kwargs)
+        # TODO: Don't have a runtime just to generate the appropriate mixin classes (cpennington)
+        # This is only used by partition_fields_by_scope, which is only needed because
+        # the split mongo store is used for item creation as well as item persistence
+        self.mixologist = Mixologist(self.xblock_mixins)
+
+    def partition_fields_by_scope(self, category, fields):
+        """
+        Return dictionary of {scope: {field1: val, ..}..} for the fields of this potential xblock
+
+        :param category: the xblock category
+        :param fields: the dictionary of {fieldname: value}
+        """
+        if fields is None:
+            return {}
+        cls = self.mixologist.mix(XBlock.load_class(category, select=prefer_xmodules))
+        result = collections.defaultdict(dict)
+        for field_name, value in fields.iteritems():
+            field = getattr(cls, field_name)
+            result[field.scope][field_name] = value
+        return result
+
+
+def only_xmodules(identifier, entry_points):
+    """Only use entry_points that are supplied by the xmodule package"""
+    from_xmodule = [entry_point for entry_point in entry_points if entry_point.dist.key == 'xmodule']
+
+    return default_select(identifier, from_xmodule)
+
+
+def prefer_xmodules(identifier, entry_points):
+    """Prefer entry_points from the xmodule package"""
+    from_xmodule = [entry_point for entry_point in entry_points if entry_point.dist.key == 'xmodule']
+    if from_xmodule:
+        return default_select(identifier, from_xmodule)
+    else:
+        return default_select(identifier, entry_points)

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/mongo/base.py

@@ -625,7 +625,16 @@ class MongoModuleStore(ModuleStoreWriteBase):
         modules = self._load_items(list(items), depth)
         return modules
 
-    def create_xmodule(self, location, definition_data=None, metadata=None, system=None):
+    def create_course(self, location, definition_data=None, metadata=None, runtime=None):
+        """
+        Create a course with the given location. The location category must be 'course'.
+        """
+        if location.category != 'course':
+            raise ValueError(u"Course roots must be of category 'course': {}".format(unicode(location)))
+        return self.create_and_save_xmodule(location, definition_data, metadata, runtime)
+
+    def create_xmodule(self, location, definition_data=None, metadata=None, system=None,
+                       fields={}):
         """
         Create the new xmodule but don't save it. Returns the new module.
 
@@ -672,36 +681,18 @@ class MongoModuleStore(ModuleStoreWriteBase):
             ScopeIds(None, location.category, location, location),
             dbmodel,
         )
+        for key, value in fields.iteritems():
+            xmodule[key] = value
         # decache any pending field settings from init
         xmodule.save()
         return xmodule
 
-    def save_xmodule(self, xmodule):
-        """
-        Save the given xmodule (will either create or update based on whether id already exists).
-        Pulls out the data definition v metadata v children locally but saves it all.
-
-        :param xmodule:
-        """
-        # Save any changes to the xmodule to the MongoKeyValueStore
-        xmodule.save()
-        self.collection.save({
-            '_id': namedtuple_to_son(xmodule.location),
-            'metadata': own_metadata(xmodule),
-            'definition': {
-                'data': xmodule.get_explicitly_set_fields_by_scope(Scope.content),
-                'children': xmodule.children if xmodule.has_children else []
-            }
-        })
-        # recompute (and update) the metadata inheritance tree which is cached
-        self.refresh_cached_metadata_inheritance_tree(xmodule.location)
-        self.fire_updated_modulestore_signal(get_course_id_no_run(xmodule.location), xmodule.location)
-
-    def create_and_save_xmodule(self, location, definition_data=None, metadata=None, system=None):
+    def create_and_save_xmodule(self, location, definition_data=None, metadata=None, system=None,
+                                fields={}):
         """
         Create the new xmodule and save it. Does not return the new module because if the caller
         will insert it as a child, it's inherited metadata will completely change. The difference
-        between this and just doing create_xmodule and save_xmodule is this ensures static_tabs get
+        between this and just doing create_xmodule and update_item is this ensures static_tabs get
         pointed to by the course.
 
         :param location: a Location--must have a category
@@ -711,9 +702,9 @@ class MongoModuleStore(ModuleStoreWriteBase):
         """
         # differs from split mongo in that I believe most of this logic should be above the persistence
         # layer but added it here to enable quick conversion. I'll need to reconcile these.
-        new_object = self.create_xmodule(location, definition_data, metadata, system)
+        new_object = self.create_xmodule(location, definition_data, metadata, system, fields)
         location = new_object.location
-        self.save_xmodule(new_object)
+        self.update_item(new_object, allow_not_found=True)
 
         # VS[compat] cdodge: This is a hack because static_tabs also have references from the course module, so
         # if we add one then we need to also add it to the policy information (i.e. metadata)
@@ -728,9 +719,9 @@ class MongoModuleStore(ModuleStoreWriteBase):
                 'url_slug': new_object.location.name
             })
             course.tabs = existing_tabs
-            # Save any changes to the course to the MongoKeyValueStore
-            course.save()
-            self.update_item(course, '**replace_user**')
+            self.update_item(course)
+
+        return new_object
 
     def fire_updated_modulestore_signal(self, course_id, location):
         """
@@ -787,7 +778,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
         if result['n'] == 0:
             raise ItemNotFoundError(location)
 
-    def update_item(self, xblock, user, allow_not_found=False):
+    def update_item(self, xblock, user=None, allow_not_found=False):
         """
         Update the persisted version of xblock to reflect its current values.
 
@@ -861,7 +852,7 @@ class MongoModuleStore(ModuleStoreWriteBase):
         location = Location.ensure_fully_specified(location)
         items = self.collection.find({'definition.children': location.url()},
                                      {'_id': True})
-        return [i['_id'] for i in items]
+        return [Location(i['_id']) for i in items]
 
     def get_modulestore_type(self, course_id):
         """

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

@@ -106,21 +106,6 @@ class DraftModuleStore(MongoModuleStore):
             raise InvalidVersionError(location)
         return super(DraftModuleStore, self).create_xmodule(draft_loc, definition_data, metadata, system)
 
-    def save_xmodule(self, xmodule):
-        """
-        Save the given xmodule (will either create or update based on whether id already exists).
-        Pulls out the data definition v metadata v children locally but saves it all.
-
-        :param xmodule:
-        """
-        orig_location = xmodule.location
-
-        xmodule.location = as_draft(orig_location)
-        try:
-            super(DraftModuleStore, self).save_xmodule(xmodule)
-        finally:
-            xmodule.location = orig_location
-
     def get_items(self, location, course_id=None, depth=0, qualifiers=None):
         """
         Returns a list of XModuleDescriptor instances for the items
@@ -159,7 +144,7 @@ class DraftModuleStore(MongoModuleStore):
         if draft_location.category in DIRECT_ONLY_CATEGORIES:
             raise InvalidVersionError(source_location)
         if not original:
-            raise ItemNotFoundError
+            raise ItemNotFoundError(source_location)
         original['_id'] = namedtuple_to_son(draft_location)
         try:
             self.collection.insert(original)
@@ -171,7 +156,7 @@ class DraftModuleStore(MongoModuleStore):
 
         return self._load_items([original])[0]
 
-    def update_item(self, xblock, user, allow_not_found=False):
+    def update_item(self, xblock, user=None, allow_not_found=False):
         """
         Save the current values to persisted version of the xblock
 
@@ -187,7 +172,7 @@ class DraftModuleStore(MongoModuleStore):
                 raise
 
         xblock.location = draft_loc
-        super(DraftModuleStore, self).update_item(xblock, user)
+        super(DraftModuleStore, self).update_item(xblock, user, allow_not_found)
         # don't allow locations to truly represent themselves as draft outside of this file
         xblock.location = as_published(xblock.location)
 

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

@@ -51,14 +51,13 @@ import logging
 import re
 from importlib import import_module
 from path import path
-import collections
 import copy
 from pytz import UTC
 
 from xmodule.errortracker import null_error_tracker
-from xmodule.x_module import prefer_xmodules
 from xmodule.modulestore.locator import (
-    BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree, LocalId, Locator
+    BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree,
+    LocalId, Locator
 )
 from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError
 from xmodule.modulestore import inheritance, ModuleStoreWriteBase, Location, SPLIT_MONGO_MODULESTORE_TYPE
@@ -67,7 +66,6 @@ from ..exceptions import ItemNotFoundError
 from .definition_lazy_loader import DefinitionLazyLoader
 from .caching_descriptor_system import CachingDescriptorSystem
 from xblock.fields import Scope
-from xblock.runtime import Mixologist
 from bson.objectid import ObjectId
 from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection
 from xblock.core import XBlock
@@ -132,11 +130,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         self.render_template = render_template
         self.i18n_service = i18n_service
 
-        # TODO: Don't have a runtime just to generate the appropriate mixin classes (cpennington)
-        # This is only used by _partition_fields_by_scope, which is only needed because
-        # the split mongo store is used for item creation as well as item persistence
-        self.mixologist = Mixologist(self.xblock_mixins)
-
     def cache_items(self, system, base_block_ids, depth=0, lazy=True):
         '''
         Handles caching of items once inheritance and any other one time
@@ -759,7 +752,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         index_entry = self._get_index_if_valid(course_or_parent_locator, force, continue_version)
         structure = self._lookup_course(course_or_parent_locator)['structure']
 
-        partitioned_fields = self._partition_fields_by_scope(category, fields)
+        partitioned_fields = self.partition_fields_by_scope(category, fields)
         new_def_data = partitioned_fields.get(Scope.content, {})
         # persist the definition if persisted != passed
         if (definition_locator is None or isinstance(definition_locator.definition_id, LocalId)):
@@ -822,14 +815,19 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         if index_entry is not None:
             if not continue_version:
                 self._update_head(index_entry, course_or_parent_locator.branch, new_id)
-            course_parent = course_or_parent_locator.as_course_locator()
+            item_loc = BlockUsageLocator(
+                package_id=course_or_parent_locator.package_id,
+                branch=course_or_parent_locator.branch,
+                block_id=new_block_id,
+            )
         else:
-            course_parent = None
+            item_loc = BlockUsageLocator(
+                block_id=new_block_id,
+                version_guid=new_id,
+            )
 
         # reconstruct the new_item from the cache
-        return self.get_item(BlockUsageLocator(package_id=course_parent,
-                                               block_id=new_block_id,
-                                               version_guid=new_id))
+        return self.get_item(item_loc)
 
     def create_course(
         self, org, prettyid, user_id, id_root=None, fields=None,
@@ -867,7 +865,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         provide any fields overrides, see above). if not provided, will create a mostly empty course
         structure with just a category course root xblock.
         """
-        partitioned_fields = self._partition_fields_by_scope(root_category, fields)
+        partitioned_fields = self.partition_fields_by_scope(root_category, fields)
         block_fields = partitioned_fields.setdefault(Scope.settings, {})
         if Scope.children in partitioned_fields:
             block_fields.update(partitioned_fields[Scope.children])
@@ -1287,7 +1285,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         if index is None:
             raise ItemNotFoundError(package_id)
         # this is the only real delete in the system. should it do something else?
-        log.info("deleting course from split-mongo: %s", package_id)
+        log.info(u"deleting course from split-mongo: %s", package_id)
         self.db_connection.delete_course_index(index['_id'])
 
     def get_errored_courses(self):
@@ -1494,22 +1492,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         index_entry['versions'][branch] = new_id
         self.db_connection.update_course_index(index_entry)
 
-    def _partition_fields_by_scope(self, category, fields):
-        """
-        Return dictionary of {scope: {field1: val, ..}..} for the fields of this potential xblock
-
-        :param category: the xblock category
-        :param fields: the dictionary of {fieldname: value}
-        """
-        if fields is None:
-            return {}
-        cls = self.mixologist.mix(XBlock.load_class(category, select=self.xblock_select))
-        result = collections.defaultdict(dict)
-        for field_name, value in fields.iteritems():
-            field = getattr(cls, field_name)
-            result[field.scope][field_name] = value
-        return result
-
     def _filter_special_fields(self, fields):
         """
         Remove any fields which split or its kvs computes or adds but does not want persisted.

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

@@ -33,7 +33,7 @@ def mixed_store_config(data_dir, mappings):
             'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
             'OPTIONS': {
                 'mappings': mappings,
-                'reference_type': 'Location',
+                'reference_type': 'xmodule.modulestore.Location',
                 'stores': {
                     'default': mongo_config['default'],
                     'xml': xml_config['default']
@@ -220,12 +220,17 @@ class ModuleStoreTestCase(TestCase):
         store = editable_modulestore()
         if hasattr(store, 'collection'):
             store.collection.drop()
+            store.db.connection.close()
+        elif hasattr(store, 'close_all_connections'):
+            store.close_all_connections()
         if contentstore().fs_files:
             db = contentstore().fs_files.database
             db.connection.drop_database(db)
+            db.connection.close()
         location_mapper = loc_mapper()
         if location_mapper.db:
             location_mapper.location_map.drop()
+            location_mapper.db.connection.close()
 
     @classmethod
     def setUpClass(cls):

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_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/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

@@ -580,22 +580,6 @@ class ResourceTemplates(object):
                     return template
 
 
-def prefer_xmodules(identifier, entry_points):
-    """Prefer entry_points from the xmodule package"""
-    from_xmodule = [entry_point for entry_point in entry_points if entry_point.dist.key == 'xmodule']
-    if from_xmodule:
-        return default_select(identifier, from_xmodule)
-    else:
-        return default_select(identifier, entry_points)
-
-
-def only_xmodules(identifier, entry_points):
-    """Only use entry_points that are supplied by the xmodule package"""
-    from_xmodule = [entry_point for entry_point in entry_points if entry_point.dist.key == 'xmodule']
-
-    return default_select(identifier, from_xmodule)
-
-
 @XBlock.needs("i18n")
 class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock):
     """

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,7 @@ MODULESTORE = {
         'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
         'OPTIONS': {
             'mappings': {},
-            'reference_type': 'Location',
+            'reference_type': 'xmodule.modulestore.Location',
             'stores': {
                 'default': {
                     'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore',

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.