Merge pull request #4720 from edx/split/add-and-fixes

Split/add and fixes

cms/djangoapps/contentstore/features/video.py

@@ -138,7 +138,7 @@ def xml_only_video(step):
     course = world.scenario_dict['COURSE']
     store = modulestore()
 
-    parent_location = store.get_items(course.id, category='vertical')[0].location
+    parent_location = store.get_items(course.id, qualifiers={'category': 'vertical'})[0].location
 
     youtube_id = 'ABCDEFG'
     world.scenario_dict['YOUTUBE_ID'] = youtube_id

cms/djangoapps/contentstore/management/commands/check_course.py

@@ -58,7 +58,7 @@ class Command(BaseCommand):
         discussion_items = _get_discussion_items(course)
 
         # now query all discussion items via get_items() and compare with the tree-traversal
-        queried_discussion_items = store.get_items(course_key=course_key, category='discussion',)
+        queried_discussion_items = store.get_items(course_key=course_key, qualifiers={'category': 'discussion'})
 
         for item in queried_discussion_items:
             if item.location not in discussion_items:

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -28,7 +28,7 @@ from xmodule.exceptions import NotFoundError, InvalidVersionError
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.modulestore.inheritance import own_metadata
-from opaque_keys.edx.keys import UsageKey
+from opaque_keys.edx.keys import UsageKey, CourseKey
 from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation, CourseLocator
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from xmodule.modulestore.xml_exporter import export_to_xml
@@ -47,6 +47,7 @@ from student.roles import CourseCreatorRole, CourseInstructorRole
 from opaque_keys import InvalidKeyError
 from contentstore.tests.utils import get_url
 from course_action_state.models import CourseRerunState, CourseRerunUIStateManager
+from course_action_state.managers import CourseActionStateItemNotFoundError
 
 
 TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE)
@@ -94,7 +95,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase):
         store.update_item(course, self.user.id)
 
         # just pick one vertical
-        descriptor = store.get_items(course.id, category='vertical',)
+        descriptor = store.get_items(course.id, qualifiers={'category': 'vertical'})
         resp = self.client.get_html(get_url('container_handler', descriptor[0].location))
         self.assertEqual(resp.status_code, 200)
 
@@ -127,7 +128,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase):
         """Verifies the editing HTML in all the verticals in the given test course"""
         _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', [test_course_name])
 
-        items = self.store.get_items(course_items[0].id, category='vertical')
+        items = self.store.get_items(course_items[0].id, qualifiers={'category': 'vertical'})
         self._check_verticals(items)
 
     def test_edit_unit_toy(self):
@@ -289,7 +290,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase):
         _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy'])
         course_key = course_items[0].id
 
-        items = self.store.get_items(course_key, category='poll_question')
+        items = self.store.get_items(course_key, qualifiers={'category': 'poll_question'})
         found = len(items) > 0
 
         self.assertTrue(found)
@@ -643,7 +644,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase):
         filesystem = OSFS(root_dir / 'test_export')
         self.assertTrue(filesystem.exists(dirname))
 
-        items = store.get_items(course_id, category=category_name)
+        items = store.get_items(course_id, qualifiers={'category': category_name})
 
         for item in items:
             filesystem = OSFS(root_dir / ('test_export/' + dirname))
@@ -742,7 +743,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase):
         # create a new video module and add it as a child to a vertical
         # this re-creates a bug whereby since the video template doesn't have
         # anything in 'data' field, the export was blowing up
-        verticals = self.store.get_items(course_id, category='vertical')
+        verticals = self.store.get_items(course_id, qualifiers={'category': 'vertical'})
 
         self.assertGreater(len(verticals), 0)
 
@@ -768,7 +769,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase):
         import_from_xml(self.store, self.user.id, 'common/test/data/', ['word_cloud'])
         course_id = SlashSeparatedCourseKey('HarvardX', 'ER22x', '2013_Spring')
 
-        verticals = self.store.get_items(course_id, category='vertical')
+        verticals = self.store.get_items(course_id, qualifiers={'category': 'vertical'})
 
         self.assertGreater(len(verticals), 0)
 
@@ -795,7 +796,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase):
         import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy'])
         course_id = SlashSeparatedCourseKey('edX', 'toy', '2012_Fall')
 
-        verticals = self.store.get_items(course_id, category='vertical')
+        verticals = self.store.get_items(course_id, qualifiers={'category': 'vertical'})
 
         self.assertGreater(len(verticals), 0)
 
@@ -916,8 +917,10 @@ class ContentStoreToyCourseTest(ContentStoreTestCase):
 
         items = self.store.get_items(
             course_id,
-            category='sequential',
-            name='vertical_sequential'
+            qualifiers={
+                'category': 'sequential',
+                'name': 'vertical_sequential',
+            }
         )
         self.assertEqual(len(items), 1)
 
@@ -1115,8 +1118,7 @@ class ContentStoreTest(ContentStoreTestCase):
     def test_create_course_with_bad_organization(self):
         """Test new course creation - error path for bad organization name"""
         self.course_data['org'] = 'University of California, Berkeley'
-        self.assert_course_creation_failed(
-            r"(?s)Unable to create course 'Robot Super Course'.*: Invalid characters in u'University of California, Berkeley'")
+        self.assert_course_creation_failed(r"(?s)Unable to create course 'Robot Super Course'.*")
 
     def test_create_course_with_course_creation_disabled_staff(self):
         """Test new course creation -- course creation disabled, but staff access."""
@@ -1401,7 +1403,7 @@ class ContentStoreTest(ContentStoreTestCase):
         _, course_items = import_from_xml(self.store, self.user.id, 'common/test/data/', ['toy'])
 
         course = course_items[0]
-        verticals = self.store.get_items(course.id, category='vertical')
+        verticals = self.store.get_items(course.id, qualifiers={'category': 'vertical'})
 
         # let's assert on the metadata_inheritance on an existing vertical
         for vertical in verticals:
@@ -1572,31 +1574,35 @@ class RerunCourseTest(ContentStoreTestCase):
             'display_name': 'Robot Super Course',
             'run': '2013_Spring'
         }
-        self.destination_course_key = _get_course_id(self.destination_course_data)
 
-    def post_rerun_request(self, source_course_key, response_code=200):
+    def post_rerun_request(
+            self, source_course_key, destination_course_data=None, response_code=200, expect_error=False
+    ):
         """Create and send an ajax post for the rerun request"""
 
         # create data to post
         rerun_course_data = {'source_course_key': unicode(source_course_key)}
-        rerun_course_data.update(self.destination_course_data)
+        if not destination_course_data:
+            destination_course_data = self.destination_course_data
+        rerun_course_data.update(destination_course_data)
+        destination_course_key = _get_course_id(destination_course_data)
 
         # post the request
-        course_url = get_url('course_handler', self.destination_course_key, 'course_key_string')
+        course_url = get_url('course_handler', destination_course_key, 'course_key_string')
         response = self.client.ajax_post(course_url, rerun_course_data)
 
         # verify response
         self.assertEqual(response.status_code, response_code)
-        if response_code == 200:
-            self.assertNotIn('ErrMsg', parse_json(response))
+        if not expect_error:
+            json_resp = parse_json(response)
+            self.assertNotIn('ErrMsg', json_resp)
+            destination_course_key = CourseKey.from_string(json_resp['destination_course_key'])
 
-    def create_course_listing_html(self, course_key):
-        """Creates html fragment that is created for the given course_key in the course listing section"""
-        return '<a class="course-link" href="/course/{}"'.format(course_key)
+        return destination_course_key
 
     def create_unsucceeded_course_action_html(self, course_key):
         """Creates html fragment that is created for the given course_key in the unsucceeded course action section"""
-        # TODO LMS-11011 Update this once the Rerun UI is implemented.
+        # TODO Update this once the Rerun UI LMS-11011 is implemented.
         return '<div class="unsucceeded-course-action" href="/course/{}"'.format(course_key)
 
     def assertInCourseListing(self, course_key):
@@ -1605,7 +1611,7 @@ class RerunCourseTest(ContentStoreTestCase):
         and NOT in the unsucceeded course action section of the html.
         """
         course_listing_html = self.client.get_html('/course/')
-        self.assertIn(self.create_course_listing_html(course_key), course_listing_html.content)
+        self.assertIn(course_key.run, course_listing_html.content)
         self.assertNotIn(self.create_unsucceeded_course_action_html(course_key), course_listing_html.content)
 
     def assertInUnsucceededCourseActions(self, course_key):
@@ -1614,32 +1620,39 @@ class RerunCourseTest(ContentStoreTestCase):
         and NOT in the accessible course listing section of the html.
         """
         course_listing_html = self.client.get_html('/course/')
-        self.assertNotIn(self.create_course_listing_html(course_key), course_listing_html.content)
-        # TODO Uncomment this once LMS-11011 is implemented.
-        # self.assertIn(self.create_unsucceeded_course_action_html(course_key), course_listing_html.content)
+        self.assertNotIn(course_key.run, course_listing_html.content)
+        # TODO Verify the course is in the unsucceeded listing once LMS-11011 is implemented.
 
     def test_rerun_course_success(self):
-        source_course = CourseFactory.create()
-        self.post_rerun_request(source_course.id)
 
-        # Verify that the course rerun action is marked succeeded
-        rerun_state = CourseRerunState.objects.find_first(course_key=self.destination_course_key)
-        self.assertEquals(rerun_state.state, CourseRerunUIStateManager.State.SUCCEEDED)
+        source_course = CourseFactory.create()
+        destination_course_key = self.post_rerun_request(source_course.id)
+
+        # Verify the contents of the course rerun action
+        rerun_state = CourseRerunState.objects.find_first(course_key=destination_course_key)
+        expected_states = {
+            'state': CourseRerunUIStateManager.State.SUCCEEDED,
+            'source_course_key': source_course.id,
+            'course_key': destination_course_key,
+            'should_display': True,
+        }
+        for field_name, expected_value in expected_states.iteritems():
+            self.assertEquals(getattr(rerun_state, field_name), expected_value)
 
         # Verify that the creator is now enrolled in the course.
-        self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.destination_course_key))
+        self.assertTrue(CourseEnrollment.is_enrolled(self.user, destination_course_key))
 
         # Verify both courses are in the course listing section
         self.assertInCourseListing(source_course.id)
-        self.assertInCourseListing(self.destination_course_key)
+        self.assertInCourseListing(destination_course_key)
 
-    def test_rerun_course_fail(self):
+    def test_rerun_course_fail_no_source_course(self):
         existent_course_key = CourseFactory.create().id
-        non_existent_course_key = CourseLocator("org", "non_existent_course", "run")
-        self.post_rerun_request(non_existent_course_key)
+        non_existent_course_key = CourseLocator("org", "non_existent_course", "non_existent_run")
+        destination_course_key = self.post_rerun_request(non_existent_course_key)
 
         # Verify that the course rerun action is marked failed
-        rerun_state = CourseRerunState.objects.find_first(course_key=self.destination_course_key)
+        rerun_state = CourseRerunState.objects.find_first(course_key=destination_course_key)
         self.assertEquals(rerun_state.state, CourseRerunUIStateManager.State.FAILED)
         self.assertIn("Cannot find a course at", rerun_state.message)
 
@@ -1652,13 +1665,32 @@ class RerunCourseTest(ContentStoreTestCase):
         # Verify that the failed course is NOT in the course listings
         self.assertInUnsucceededCourseActions(non_existent_course_key)
 
+    def test_rerun_course_fail_duplicate_course(self):
+        existent_course_key = CourseFactory.create().id
+        destination_course_data = {
+            'org': existent_course_key.org,
+            'number': existent_course_key.course,
+            'display_name': 'existing course',
+            'run': existent_course_key.run
+        }
+        destination_course_key = self.post_rerun_request(
+            existent_course_key, destination_course_data, expect_error=True
+        )
+
+        # Verify that the course rerun action doesn't exist
+        with self.assertRaises(CourseActionStateItemNotFoundError):
+            CourseRerunState.objects.find_first(course_key=destination_course_key)
+
+        # Verify that the existing course continues to be in the course listing
+        self.assertInCourseListing(existent_course_key)
+
     def test_rerun_with_permission_denied(self):
         with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}):
             source_course = CourseFactory.create()
             auth.add_users(self.user, CourseCreatorRole(), self.user)
             self.user.is_staff = False
             self.user.save()
-            self.post_rerun_request(source_course.id, 403)
+            self.post_rerun_request(source_course.id, response_code=403, expect_error=True)
 
 
 class EntryPageTestCase(TestCase):
@@ -1705,6 +1737,6 @@ def _course_factory_create_course():
     return CourseFactory.create(org='MITx', course='999', display_name='Robot Super Course')
 
 
-def _get_course_id(course_data):
+def _get_course_id(course_data, key_class=SlashSeparatedCourseKey):
     """Returns the course ID (org/number/run)."""
-    return SlashSeparatedCourseKey(course_data['org'], course_data['number'], course_data['run'])
+    return key_class(course_data['org'], course_data['number'], course_data['run'])

cms/djangoapps/contentstore/tests/test_course_settings.py

@@ -413,7 +413,7 @@ class CourseGradingTest(CourseTestCase):
         Populate the course, grab a section, get the url for the assignment type access
         """
         self.populate_course()
-        sections = modulestore().get_items(self.course.id, category="sequential")
+        sections = modulestore().get_items(self.course.id, qualifiers={'category': "sequential"})
         # see if test makes sense
         self.assertGreater(len(sections), 0, "No sections found")
         section = sections[0]  # just take the first one

cms/djangoapps/contentstore/tests/test_import.py

@@ -14,7 +14,8 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.django import modulestore
 from xmodule.contentstore.django import contentstore
-from xmodule.modulestore.tests.factories import check_exact_number_of_calls, check_number_of_calls
+from xmodule.modulestore.exceptions import DuplicateCourseError
+from xmodule.modulestore.tests.factories import check_exact_number_of_calls, check_number_of_calls, CourseFactory
 from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation
 from xmodule.modulestore.xml_importer import import_from_xml
 from xmodule.exceptions import NotFoundError

cms/djangoapps/contentstore/tests/utils.py

@@ -190,7 +190,7 @@ class CourseTestCase(ModuleStoreTestCase):
         """
         items = self.store.get_items(
             course_id,
-            category='vertical',
+            qualifiers={'category': 'vertical'},
             revision=ModuleStoreEnum.RevisionOption.published_only
         )
         self.check_verticals(items)

cms/djangoapps/contentstore/views/assets.py

@@ -277,7 +277,7 @@ def _get_asset_json(display_name, date, location, thumbnail_location, locked):
     """
     Helper method for formatting the asset information to send to client.
     """
-    asset_url = location.to_deprecated_string()
+    asset_url = _add_slash(location.to_deprecated_string())
     external_url = settings.LMS_BASE + asset_url
     return {
         'display_name': display_name,
@@ -285,8 +285,14 @@ def _get_asset_json(display_name, date, location, thumbnail_location, locked):
         'url': asset_url,
         'external_url': external_url,
         'portable_url': StaticContent.get_static_path_from_location(location),
-        'thumbnail': thumbnail_location.to_deprecated_string() if thumbnail_location is not None else None,
+        'thumbnail': _add_slash(unicode(thumbnail_location)) if thumbnail_location else None,
         'locked': locked,
         # Needed for Backbone delete/update.
         'id': unicode(location)
     }
+
+
+def _add_slash(url):
+    if not url.startswith('/'):
+        url = '/' + url  # TODO - re-address this once LMS-11198 is tackled.
+    return url

cms/djangoapps/contentstore/views/course.py

@@ -24,9 +24,9 @@ from xmodule.contentstore.content import StaticContent
 from xmodule.tabs import PDFTextbookTabs
 from xmodule.partitions.partitions import UserPartition, Group
 
-from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError
+from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError
 from opaque_keys import InvalidKeyError
-from opaque_keys.edx.locations import Location, SlashSeparatedCourseKey
+from opaque_keys.edx.locations import Location
 
 from contentstore.course_info_model import get_course_updates, update_course_updates, delete_course_update
 from contentstore.utils import (
@@ -441,7 +441,7 @@ def _create_or_rerun_course(request):
     """
     To be called by requests that create a new destination course (i.e., create_new_course and rerun_course)
     Returns the destination course_key and overriding fields for the new course.
-    Raises InvalidLocationError and InvalidKeyError
+    Raises DuplicateCourseError and InvalidKeyError
     """
     if not auth.has_access(request.user, CourseCreatorRole()):
         raise PermissionDenied()
@@ -460,15 +460,14 @@ def _create_or_rerun_course(request):
                     status=400
                 )
 
-        course_key = SlashSeparatedCourseKey(org, number, run)
         fields = {'display_name': display_name} if display_name is not None else {}
 
         if 'source_course_key' in request.json:
-            return _rerun_course(request, course_key, fields)
+            return _rerun_course(request, org, number, run, fields)
         else:
-            return _create_new_course(request, course_key, fields)
+            return _create_new_course(request, org, number, run, fields)
 
-    except InvalidLocationError:
+    except DuplicateCourseError:
         return JsonResponse({
             'ErrMsg': _(
                 'There is already a course defined with the same '
@@ -488,24 +487,27 @@ def _create_or_rerun_course(request):
         )
 
 
-def _create_new_course(request, course_key, fields):
+def _create_new_course(request, org, number, run, fields):
     """
     Create a new course.
     Returns the URL for the course overview page.
+    Raises DuplicateCourseError if the course already exists
     """
     # Set a unique wiki_slug for newly created courses. To maintain active wiki_slugs for
     # existing xml courses this cannot be changed in CourseDescriptor.
     # # TODO get rid of defining wiki slug in this org/course/run specific way and reconcile
     # w/ xmodule.course_module.CourseDescriptor.__init__
-    wiki_slug = u"{0}.{1}.{2}".format(course_key.org, course_key.course, course_key.run)
+    wiki_slug = u"{0}.{1}.{2}".format(org, number, run)
     definition_data = {'wiki_slug': wiki_slug}
     fields.update(definition_data)
 
-    # Creating the course raises InvalidLocationError if an existing course with this org/name is found
-    new_course = modulestore().create_course(
-        course_key.org,
-        course_key.course,
-        course_key.run,
+    store = modulestore()
+    with store.default_store(settings.FEATURES.get('DEFAULT_STORE_FOR_NEW_COURSE', 'mongo')):
+        # Creating the course raises DuplicateCourseError if an existing course with this org/name is found
+        new_course = store.create_course(
+            org,
+            number,
+            run,
             request.user.id,
             fields=fields,
         )
@@ -521,7 +523,7 @@ def _create_new_course(request, course_key, fields):
     })
 
 
-def _rerun_course(request, destination_course_key, fields):
+def _rerun_course(request, org, number, run, fields):
     """
     Reruns an existing course.
     Returns the URL for the course listing page.
@@ -532,6 +534,15 @@ def _rerun_course(request, destination_course_key, fields):
     if not has_course_access(request.user, source_course_key):
         raise PermissionDenied()
 
+    # create destination course key
+    store = modulestore()
+    with store.default_store('split'):
+        destination_course_key = store.make_course_key(org, number, run)
+
+     # verify org course and run don't already exist
+    if store.has_course(destination_course_key, ignore_case=True):
+        raise DuplicateCourseError(source_course_key, destination_course_key)
+
     # Make sure user has instructor and staff access to the destination course
     # so the user can see the updated status for that course
     add_instructor(destination_course_key, request.user, request.user)
@@ -540,10 +551,13 @@ def _rerun_course(request, destination_course_key, fields):
     CourseRerunState.objects.initiated(source_course_key, destination_course_key, request.user)
 
     # Rerun the course as a new celery task
-    rerun_course.delay(source_course_key, destination_course_key, request.user.id, fields)
+    rerun_course.delay(unicode(source_course_key), unicode(destination_course_key), request.user.id, fields)
 
     # Return course listing page
-    return JsonResponse({'url': reverse_url('course_handler')})
+    return JsonResponse({
+        'url': reverse_url('course_handler'),
+        'destination_course_key': unicode(destination_course_key)
+    })
 
 
 # pylint: disable=unused-argument
@@ -1121,7 +1135,7 @@ class GroupConfiguration(object):
         }
         """
         usage_info = {}
-        descriptors = store.get_items(course.id, category='split_test')
+        descriptors = store.get_items(course.id, qualifiers={'category': 'split_test'})
         for split_test in descriptors:
             if split_test.user_partition_id not in usage_info:
                 usage_info[split_test.user_partition_id] = []

cms/djangoapps/contentstore/views/item.py

@@ -424,6 +424,11 @@ def _create_item(request):
     if display_name is not None:
         metadata['display_name'] = display_name
 
+    # TODO need to fix components that are sending definition_data as strings, instead of as dicts
+    # For now, migrate them into dicts here.
+    if isinstance(data, basestring):
+        data = {'data': data}
+
     created_block = store.create_child(
         request.user.id,
         usage_key,

cms/djangoapps/contentstore/views/tasks.py

@@ -5,17 +5,27 @@ This file contains celery tasks for contentstore views
 from celery.task import task
 from django.contrib.auth.models import User
 from xmodule.modulestore.django import modulestore
+from xmodule.modulestore.exceptions import DuplicateCourseError, ItemNotFoundError
 from course_action_state.models import CourseRerunState
 from contentstore.utils import initialize_permissions
+from opaque_keys.edx.keys import CourseKey
 
 
 @task()
-def rerun_course(source_course_key, destination_course_key, user_id, fields=None):
+def rerun_course(source_course_key_string, destination_course_key_string, user_id, fields=None):
     """
     Reruns a course in a new celery task.
     """
     try:
-        modulestore().clone_course(source_course_key, destination_course_key, user_id, fields=fields)
+        # deserialize the keys
+        source_course_key = CourseKey.from_string(source_course_key_string)
+        destination_course_key = CourseKey.from_string(destination_course_key_string)
+
+        # use the split modulestore as the store for the rerun course,
+        # as the Mongo modulestore doesn't support multiple runs of the same course.
+        store = modulestore()
+        with store.default_store('split'):
+            store.clone_course(source_course_key, destination_course_key, user_id, fields=fields)
 
         # set initial permissions for the user to access the course.
         initialize_permissions(destination_course_key, User.objects.get(id=user_id))
@@ -23,10 +33,24 @@ def rerun_course(source_course_key, destination_course_key, user_id, fields=None
         # update state: Succeeded
         CourseRerunState.objects.succeeded(course_key=destination_course_key)
 
+        return "succeeded"
+
+    except DuplicateCourseError as exc:
+        # do NOT delete the original course, only update the status
+        CourseRerunState.objects.failed(course_key=destination_course_key, exception=exc)
+
+        return "duplicate course"
+
     # catch all exceptions so we can update the state and properly cleanup the course.
     except Exception as exc:  # pylint: disable=broad-except
         # update state: Failed
         CourseRerunState.objects.failed(course_key=destination_course_key, exception=exc)
 
+        try:
             # cleanup any remnants of the course
             modulestore().delete_course(destination_course_key, user_id)
+        except ItemNotFoundError:
+            # it's possible there was an error even before the course module was created
+            pass
+
+        return "exception: " + unicode(exc)

cms/envs/bok_choy.auth.json

@@ -51,7 +51,6 @@
             "ENGINE": "xmodule.modulestore.mixed.MixedModuleStore",
             "OPTIONS": {
                 "mappings": {},
-                "reference_type": "Location",
                 "stores": [
                     {
                         "NAME": "draft",

cms/envs/common.py

@@ -106,6 +106,9 @@ FEATURES = {
 
     # Toggles Group Configuration editing functionality
     'ENABLE_GROUP_CONFIGURATIONS': os.environ.get('FEATURE_GROUP_CONFIGURATIONS'),
+
+    # Modulestore to use for new courses
+    'DEFAULT_STORE_FOR_NEW_COURSE': 'mongo',
 }
 ENABLE_JASMINE = False
 

common/djangoapps/cache_toolbox/core.py

@@ -10,6 +10,7 @@ Core methods
 
 from django.core.cache import cache
 from django.db import DEFAULT_DB_ALIAS
+from opaque_keys import InvalidKeyError
 
 from . import app_settings
 
@@ -118,9 +119,19 @@ def get_cached_content(location):
 
 
 def del_cached_content(location):
-    # delete content for the given location, as well as for content with run=None.
-    # it's possible that the content could have been cached without knowing the
-    # course_key - and so without having the run.
-    cache.delete_many(
-        [unicode(loc).encode("utf-8") for loc in [location, location.replace(run=None)]]
-    )
+    """
+    delete content for the given location, as well as for content with run=None.
+    it's possible that the content could have been cached without knowing the
+    course_key - and so without having the run.
+    """
+    def location_str(loc):
+        return unicode(loc).encode("utf-8")
+
+    locations = [location_str(location)]
+    try:
+        locations.append(location_str(location.replace(run=None)))
+    except InvalidKeyError:
+        # although deprecated keys allowed run=None, new keys don't if there is no version.
+        pass
+
+    cache.delete_many(locations)

common/djangoapps/contentserver/middleware.py

@@ -6,6 +6,7 @@ from xmodule.contentstore.django import contentstore
 from xmodule.contentstore.content import StaticContent, XASSET_LOCATION_TAG
 from xmodule.modulestore import InvalidLocationError
 from opaque_keys import InvalidKeyError
+from opaque_keys.edx.locator import AssetLocator
 from cache_toolbox.core import get_cached_content, set_cached_content
 from xmodule.exceptions import NotFoundError
 
@@ -14,8 +15,11 @@ from xmodule.exceptions import NotFoundError
 
 class StaticContentServer(object):
     def process_request(self, request):
-        # look to see if the request is prefixed with 'c4x' tag
-        if request.path.startswith('/' + XASSET_LOCATION_TAG + '/'):
+        # look to see if the request is prefixed with an asset prefix tag
+        if (
+            request.path.startswith('/' + XASSET_LOCATION_TAG + '/') or
+            request.path.startswith('/' + AssetLocator.CANONICAL_NAMESPACE)
+        ):
             try:
                 loc = StaticContent.get_location_from_path(request.path)
             except (InvalidLocationError, InvalidKeyError):

common/djangoapps/user_api/middleware.py

@@ -5,7 +5,7 @@ Adds user's tags to tracking event context.
 
 from eventtracking import tracker
 from opaque_keys import InvalidKeyError
-from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from opaque_keys.edx.keys import CourseKey
 
 from track.contexts import COURSE_REGEX
 from user_api.models import UserCourseTag
@@ -24,7 +24,7 @@ class UserTagsEventContextMiddleware(object):
         if match:
             course_id = match.group('course_id')
             try:
-                course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
+                course_key = CourseKey.from_string(course_id)
             except InvalidKeyError:
                 course_id = None
                 course_key = None

common/djangoapps/xmodule_django/models.py

@@ -2,6 +2,7 @@ from django.db import models
 from django.core.exceptions import ValidationError
 from opaque_keys.edx.locations import SlashSeparatedCourseKey, Location
 from opaque_keys.edx.keys import CourseKey, UsageKey
+from opaque_keys.edx.locator import Locator
 
 from south.modelsinspector import add_introspection_rules
 add_introspection_rules([], ["^xmodule_django\.models\.CourseKeyField"])
@@ -44,6 +45,28 @@ class NoneToEmptyQuerySet(models.query.QuerySet):
         return super(NoneToEmptyQuerySet, self)._filter_or_exclude(*args, **kwargs)
 
 
+def _strip_object(key):
+    """
+    Strips branch and version info if the given key supports those attributes.
+    """
+    if hasattr(key, 'version_agnostic') and hasattr(key, 'for_branch'):
+        return key.for_branch(None).version_agnostic()
+    else:
+        return key
+
+
+def _strip_value(value, lookup='exact'):
+    """
+    Helper function to remove the branch and version information from the given value,
+    which could be a single object or a list.
+    """
+    if lookup == 'in':
+        stripped_value = [_strip_object(el) for el in value]
+    else:
+        stripped_value = _strip_object(value)
+    return stripped_value
+
+
 class CourseKeyField(models.CharField):
     description = "A SlashSeparatedCourseKey object, saved to the DB in the form of a string"
 
@@ -69,14 +92,18 @@ class CourseKeyField(models.CharField):
         if lookup == 'isnull':
             raise TypeError('Use CourseKeyField.Empty rather than None to query for a missing CourseKeyField')
 
-        return super(CourseKeyField, self).get_prep_lookup(lookup, value)
+        return super(CourseKeyField, self).get_prep_lookup(
+            lookup,
+            # strip key before comparing
+            _strip_value(value, lookup)
+        )
 
     def get_prep_value(self, value):
         if value is self.Empty or value is None:
             return ''  # CharFields should use '' as their empty value, rather than None
 
         assert isinstance(value, CourseKey)
-        return value.to_deprecated_string()
+        return unicode(_strip_value(value))
 
     def validate(self, value, model_instance):
         """Validate Empty values, otherwise defer to the parent"""
@@ -119,14 +146,19 @@ class LocationKeyField(models.CharField):
         if lookup == 'isnull':
             raise TypeError('Use LocationKeyField.Empty rather than None to query for a missing LocationKeyField')
 
-        return super(LocationKeyField, self).get_prep_lookup(lookup, value)
+        # remove version and branch info before comparing keys
+        return super(LocationKeyField, self).get_prep_lookup(
+            lookup,
+            # strip key before comparing
+            _strip_value(value, lookup)
+        )
 
     def get_prep_value(self, value):
         if value is self.Empty:
             return ''
 
         assert isinstance(value, UsageKey)
-        return value.to_deprecated_string()
+        return unicode(_strip_value(value))
 
     def validate(self, value, model_instance):
         """Validate Empty values, otherwise defer to the parent"""

common/lib/xmodule/xmodule/contentstore/content.py

@@ -10,8 +10,9 @@ import StringIO
 from urlparse import urlparse, urlunparse, parse_qsl
 from urllib import urlencode
 
-from opaque_keys.edx.locations import AssetLocation
-from opaque_keys.edx.keys import CourseKey
+from opaque_keys.edx.locator import AssetLocator
+from opaque_keys.edx.keys import CourseKey, AssetKey
+from opaque_keys import InvalidKeyError
 from PIL import Image
 
 
@@ -52,12 +53,10 @@ class StaticContent(object):
             asset
         """
         path = path.replace('/', '_')
-        return AssetLocation(
-            course_key.org, course_key.course, course_key.run,
+        return course_key.make_asset_key(
             'asset' if not is_thumbnail else 'thumbnail',
-            AssetLocation.clean_keeping_underscores(path),
-            revision
-        )
+            AssetLocator.clean_keeping_underscores(path)
+        ).for_branch(None)
 
     def get_id(self):
         return self.location
@@ -104,16 +103,22 @@ class StaticContent(object):
             return None
 
         assert(isinstance(course_key, CourseKey))
-        return course_key.make_asset_key('asset', '').to_deprecated_string()
+        # create a dummy asset location and then strip off the last character: 'a',
+        # since the AssetLocator rejects the empty string as a legal value for the block_id.
+        return course_key.make_asset_key('asset', 'a').for_branch(None).to_deprecated_string()[:-1]
 
     @staticmethod
     def get_location_from_path(path):
         """
         Generate an AssetKey for the given path (old c4x/org/course/asset/name syntax)
         """
-        # TODO OpaqueKeys after opaque keys deprecation is working
-        # return AssetLocation.from_string(path)
-        return AssetLocation.from_deprecated_string(path)
+        try:
+            return AssetKey.from_string(path)
+        except InvalidKeyError:
+            # TODO - re-address this once LMS-11198 is tackled.
+            if path.startswith('/'):
+                # try stripping off the leading slash and try again
+                return AssetKey.from_string(path[1:])
 
     @staticmethod
     def convert_legacy_static_url_with_course_id(path, course_id):

common/lib/xmodule/xmodule/contentstore/mongo.py

@@ -94,7 +94,10 @@ class MongoContentStore(ContentStore):
                 fp = self.fs.get(content_id)
                 thumbnail_location = getattr(fp, 'thumbnail_location', None)
                 if thumbnail_location:
-                    thumbnail_location = location.course_key.make_asset_key('thumbnail', thumbnail_location[4])
+                    thumbnail_location = location.course_key.make_asset_key(
+                        'thumbnail',
+                        thumbnail_location[4]
+                    )
                 return StaticContentStream(
                     location, fp.displayname, fp.content_type, fp, last_modified_at=fp.uploadDate,
                     thumbnail_location=thumbnail_location,
@@ -105,7 +108,10 @@ class MongoContentStore(ContentStore):
                 with self.fs.get(content_id) as fp:
                     thumbnail_location = getattr(fp, 'thumbnail_location', None)
                     if thumbnail_location:
-                        thumbnail_location = location.course_key.make_asset_key('thumbnail', thumbnail_location[4])
+                        thumbnail_location = location.course_key.make_asset_key(
+                            'thumbnail',
+                            thumbnail_location[4]
+                        )
                     return StaticContent(
                         location, fp.displayname, fp.content_type, fp.read(), last_modified_at=fp.uploadDate,
                         thumbnail_location=thumbnail_location,
@@ -304,7 +310,9 @@ class MongoContentStore(ContentStore):
                 asset_id = asset_key
             else:  # add the run, since it's the last field, we're golden
                 asset_key['run'] = dest_course_key.run
-                asset_id = unicode(dest_course_key.make_asset_key(asset_key['category'], asset_key['name']))
+                asset_id = unicode(
+                    dest_course_key.make_asset_key(asset_key['category'], asset_key['name']).for_branch(None)
+                )
 
             self.fs.put(
                 source_content.read(),
@@ -347,7 +355,7 @@ class MongoContentStore(ContentStore):
             # NOTE, there's no need to state that run doesn't exist in the negative case b/c access via
             # SON requires equivalence (same keys and values in exact same order)
             dbkey['run'] = location.run
-            content_id = unicode(location)
+            content_id = unicode(location.for_branch(None))
         return content_id, dbkey
 
     def make_id_son(self, fs_entry):

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

@@ -116,7 +116,7 @@ class ModuleStoreRead(object):
         pass
 
     @abstractmethod
-    def get_item(self, usage_key, depth=0):
+    def get_item(self, usage_key, depth=0, **kwargs):
         """
         Returns an XModuleDescriptor instance for the item at location.
 
@@ -150,7 +150,7 @@ class ModuleStoreRead(object):
         pass
 
     @abstractmethod
-    def get_items(self, location, course_id=None, depth=0, qualifiers=None):
+    def get_items(self, location, course_id=None, depth=0, qualifiers=None, **kwargs):
         """
         Returns a list of XModuleDescriptor instances for the items
         that match location. Any element of location that is None is treated
@@ -228,7 +228,17 @@ class ModuleStoreRead(object):
             return criteria == target
 
     @abstractmethod
-    def get_courses(self):
+    def make_course_key(self, org, course, run):
+        """
+        Return a valid :class:`~opaque_keys.edx.keys.CourseKey` for this modulestore
+        that matches the supplied `org`, `course`, and `run`.
+
+        This key may represent a course that doesn't exist in this modulestore.
+        """
+        pass
+
+    @abstractmethod
+    def get_courses(self, **kwargs):
         '''
         Returns a list containing the top level XModuleDescriptors of the courses
         in this modulestore.
@@ -236,7 +246,7 @@ class ModuleStoreRead(object):
         pass
 
     @abstractmethod
-    def get_course(self, course_id, depth=0):
+    def get_course(self, course_id, depth=0, **kwargs):
         '''
         Look for a specific course by its id (:class:`CourseKey`).
         Returns the course descriptor, or None if not found.
@@ -244,7 +254,7 @@ class ModuleStoreRead(object):
         pass
 
     @abstractmethod
-    def has_course(self, course_id, ignore_case=False):
+    def has_course(self, course_id, ignore_case=False, **kwargs):
         '''
         Look for a specific course id.  Returns whether it exists.
         Args:
@@ -256,13 +266,14 @@ class ModuleStoreRead(object):
 
     @abstractmethod
     def get_parent_location(self, location, **kwargs):
-        '''Find the location that is the parent of this location in this
+        '''
+        Find the location that is the parent of this location in this
         course.  Needed for path_to_location().
         '''
         pass
 
     @abstractmethod
-    def get_orphans(self, course_key):
+    def get_orphans(self, course_key, **kwargs):
         """
         Get all of the xblocks in the given course which have no parents and are not of types which are
         usually orphaned. NOTE: may include xblocks which still have references via xblocks which don't
@@ -287,7 +298,7 @@ class ModuleStoreRead(object):
         pass
 
     @abstractmethod
-    def get_courses_for_wiki(self, wiki_slug):
+    def get_courses_for_wiki(self, wiki_slug, **kwargs):
         """
         Return the list of courses which use this wiki_slug
         :param wiki_slug: the course wiki root slug
@@ -325,7 +336,7 @@ class ModuleStoreWrite(ModuleStoreRead):
     __metaclass__ = ABCMeta
 
     @abstractmethod
-    def update_item(self, xblock, user_id, allow_not_found=False, force=False):
+    def update_item(self, xblock, user_id, allow_not_found=False, force=False, **kwargs):
         """
         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.
@@ -413,7 +424,7 @@ class ModuleStoreWrite(ModuleStoreRead):
         pass
 
     @abstractmethod
-    def delete_course(self, course_key, user_id):
+    def delete_course(self, course_key, user_id, **kwargs):
         """
         Deletes the course. It may be a soft or hard delete. It may or may not remove the xblock definitions
         depending on the persistence layer and how tightly bound the xblocks are to the course.
@@ -480,19 +491,19 @@ class ModuleStoreReadBase(ModuleStoreRead):
         """
         return {}
 
-    def get_course(self, course_id, depth=0):
+    def get_course(self, course_id, depth=0, **kwargs):
         """
         See ModuleStoreRead.get_course
 
         Default impl--linear search through course list
         """
         assert(isinstance(course_id, CourseKey))
-        for course in self.get_courses():
+        for course in self.get_courses(**kwargs):
             if course.id == course_id:
                 return course
         return None
 
-    def has_course(self, course_id, ignore_case=False):
+    def has_course(self, course_id, ignore_case=False, **kwargs):
         """
         Returns the course_id of the course if it was found, else None
         Args:
@@ -577,7 +588,7 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite):
             result[field.scope][field_name] = value
         return result
 
-    def clone_course(self, source_course_id, dest_course_id, user_id, fields=None):
+    def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs):
         """
         This base method just copies the assets. The lower level impls must do the actual cloning of
         content.
@@ -587,7 +598,7 @@ class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite):
             self.contentstore.copy_all_course_assets(source_course_id, dest_course_id)
         return dest_course_id
 
-    def delete_course(self, course_key, user_id):
+    def delete_course(self, course_key, user_id, **kwargs):
         """
         This base method just deletes the assets. The lower level impls must do the actual deleting of
         content.

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

@@ -8,6 +8,7 @@ In this way, courses can be served up both - say - XMLModuleStore or MongoModule
 import logging
 from contextlib import contextmanager
 import itertools
+import functools
 
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
@@ -15,7 +16,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey
 
 from . import ModuleStoreWriteBase
 from . import ModuleStoreEnum
-from .exceptions import ItemNotFoundError
+from .exceptions import ItemNotFoundError, DuplicateCourseError
 from .draft_and_published import ModuleStoreDraftAndPublished
 from .split_migrator import SplitMigrator
 
@@ -23,6 +24,69 @@ from .split_migrator import SplitMigrator
 log = logging.getLogger(__name__)
 
 
+def strip_key(func):
+    """
+    A decorator for stripping version and branch information from return values that are, or contain, UsageKeys or
+    CourseKeys.
+    Additionally, the decorated function is called with an optional 'field_decorator' parameter that can be used
+    to strip any location(-containing) fields, which are not directly returned by the function.
+
+    The behavior can be controlled by passing 'remove_version' and 'remove_branch' booleans to the decorated
+    function's kwargs.
+    """
+    @functools.wraps(func)
+    def inner(*args, **kwargs):
+        """
+        Supported kwargs:
+            remove_version - If True, calls 'version_agnostic' on all return values, including those in lists and dicts.
+            remove_branch - If True, calls 'for_branch(None)' on all return values, including those in lists and dicts.
+            Note: The 'field_decorator' parameter passed to the decorated function is a function that honors the
+            values of these kwargs.
+        """
+
+        # remove version and branch, by default
+        rem_vers = kwargs.pop('remove_version', True)
+        rem_branch = kwargs.pop('remove_branch', False)
+
+        # helper function for stripping individual values
+        def strip_key_func(val):
+            """
+            Strips the version and branch information according to the settings of rem_vers and rem_branch.
+            Recursively calls this function if the given value has a 'location' attribute.
+            """
+            retval = val
+            if rem_vers and hasattr(retval, 'version_agnostic'):
+                retval = retval.version_agnostic()
+            if rem_branch and hasattr(retval, 'for_branch'):
+                retval = retval.for_branch(None)
+            if hasattr(retval, 'location'):
+                retval.location = strip_key_func(retval.location)
+            return retval
+
+        # function for stripping both, collection of, and individual, values
+        def strip_key_collection(field_value):
+            """
+            Calls strip_key_func for each element in the given value.
+            """
+            if rem_vers or rem_branch:
+                if isinstance(field_value, list):
+                    field_value = [strip_key_func(fv) for fv in field_value]
+                elif isinstance(field_value, dict):
+                    for key, val in field_value.iteritems():
+                        field_value[key] = strip_key_func(val)
+                else:
+                    field_value = strip_key_func(field_value)
+            return field_value
+
+        # call the decorated function
+        retval = func(field_decorator=strip_key_collection, *args, **kwargs)
+
+        # strip the return value
+        return strip_key_collection(retval)
+
+    return inner
+
+
 class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
     """
     ModuleStore knows how to route requests to the right persistence ms
@@ -100,12 +164,12 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
                 return mapping
             else:
                 for store in self.modulestores:
-                    if isinstance(course_id, store.reference_type) and store.has_course(course_id):
+                    if store.has_course(course_id):
                         self.mappings[course_id] = store
                         return store
 
-        # return the first store, as the default
-        return self.modulestores[0]
+        # return the default store
+        return self.default_modulestore
 
     def _get_modulestore_by_type(self, modulestore_type):
         """
@@ -127,7 +191,6 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
             return course_key
         return store.fill_in_run(course_key)
 
-
     def has_item(self, usage_key, **kwargs):
         """
         Does the course include the xblock who's id is reference?
@@ -135,6 +198,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         store = self._get_modulestore_for_courseid(usage_key.course_key)
         return store.has_item(usage_key, **kwargs)
 
+    @strip_key
     def get_item(self, usage_key, depth=0, **kwargs):
         """
         see parent doc
@@ -142,6 +206,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         store = self._get_modulestore_for_courseid(usage_key.course_key)
         return store.get_item(usage_key, depth, **kwargs)
 
+    @strip_key
     def get_items(self, course_key, **kwargs):
         """
         Returns:
@@ -158,7 +223,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
                     and rules as kwargs below
                 content (dict): fields to look for which have content scope. Follows same syntax and
                     rules as kwargs below.
-            additional kwargs (key=value): what to look for within the course.
+                qualifiers (dict): what to look for within the course.
                     Common qualifiers are ``category`` or any field name. if the target field is a list,
                     then it searches for the given value in the list not list equivalence.
                     Substring matching pass a regex object.
@@ -173,32 +238,39 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         store = self._get_modulestore_for_courseid(course_key)
         return store.get_items(course_key, **kwargs)
 
-    def get_courses(self):
+    @strip_key
+    def get_courses(self, **kwargs):
         '''
         Returns a list containing the top level XModuleDescriptors of the courses in this modulestore.
         '''
-        courses = {}  # a dictionary of course keys to course objects
-
-        # first populate with the ones in mappings as the mapping override discovery
-        for course_id, store in self.mappings.iteritems():
-            course = store.get_course(course_id)
-            # check if the course is not None - possible if the mappings file is outdated
-            # TODO - log an error if the course is None, but move it to an initialization method to keep it less noisy
-            if course is not None:
-                courses[course_id] = course
-
+        courses = {}
         for store in self.modulestores:
-
             # filter out ones which were fetched from earlier stores but locations may not be ==
-            for course in store.get_courses():
+            for course in store.get_courses(**kwargs):
                 course_id = self._clean_course_id_for_mapping(course.id)
                 if course_id not in courses:
                     # course is indeed unique. save it in result
                     courses[course_id] = course
-
         return courses.values()
 
-    def get_course(self, course_key, depth=0):
+    def make_course_key(self, org, course, run):
+        """
+        Return a valid :class:`~opaque_keys.edx.keys.CourseKey` for this modulestore
+        that matches the supplied `org`, `course`, and `run`.
+
+        This key may represent a course that doesn't exist in this modulestore.
+        """
+        # If there is a mapping that match this org/course/run, use that
+        for course_id, store in self.mappings.iteritems():
+            candidate_key = store.make_course_key(org, course, run)
+            if candidate_key == course_id:
+                return candidate_key
+
+        # Otherwise, return the key created by the default store
+        return self.default_modulestore.make_course_key(org, course, run)
+
+    @strip_key
+    def get_course(self, course_key, depth=0, **kwargs):
         """
         returns the course module associated with the course_id. If no such course exists,
         it returns None
@@ -208,11 +280,12 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         assert(isinstance(course_key, CourseKey))
         store = self._get_modulestore_for_courseid(course_key)
         try:
-            return store.get_course(course_key, depth=depth)
+            return store.get_course(course_key, depth=depth, **kwargs)
         except ItemNotFoundError:
             return None
 
-    def has_course(self, course_id, ignore_case=False):
+    @strip_key
+    def has_course(self, course_id, ignore_case=False, **kwargs):
         """
         returns the course_id of the course if it was found, else None
         Note: we return the course_id instead of a boolean here since the found course may have
@@ -225,7 +298,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         """
         assert(isinstance(course_id, CourseKey))
         store = self._get_modulestore_for_courseid(course_id)
-        return store.has_course(course_id, ignore_case)
+        return store.has_course(course_id, ignore_case, **kwargs)
 
     def delete_course(self, course_key, user_id):
         """
@@ -235,6 +308,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         store = self._get_modulestore_for_courseid(course_key)
         return store.delete_course(course_key, user_id)
 
+    @strip_key
     def get_parent_location(self, location, **kwargs):
         """
         returns the parent locations for a given location
@@ -252,14 +326,15 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         """
         return self._get_modulestore_for_courseid(course_id).get_modulestore_type()
 
-    def get_orphans(self, course_key):
+    @strip_key
+    def get_orphans(self, course_key, **kwargs):
         """
         Get all of the xblocks in the given course which have no parents and are not of types which are
         usually orphaned. NOTE: may include xblocks which still have references via xblocks which don't
         use children to point to their dependents.
         """
         store = self._get_modulestore_for_courseid(course_key)
-        return store.get_orphans(course_key)
+        return store.get_orphans(course_key, **kwargs)
 
     def get_errored_courses(self):
         """
@@ -271,6 +346,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
             errs.update(store.get_errored_courses())
         return errs
 
+    @strip_key
     def create_course(self, org, course, run, user_id, **kwargs):
         """
         Creates and returns the course.
@@ -285,10 +361,22 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
 
         Returns: a CourseDescriptor
         """
+        # first make sure an existing course doesn't already exist in the mapping
+        course_key = self.make_course_key(org, course, run)
+        if course_key in self.mappings:
+            raise DuplicateCourseError(course_key, course_key)
+
+        # create the course
         store = self._verify_modulestore_support(None, 'create_course')
-        return store.create_course(org, course, run, user_id, **kwargs)
+        course = store.create_course(org, course, run, user_id, **kwargs)
 
-    def clone_course(self, source_course_id, dest_course_id, user_id, fields=None):
+        # add new course to the mapping
+        self.mappings[course_key] = store
+
+        return course
+
+    @strip_key
+    def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs):
         """
         See the superclass for the general documentation.
 
@@ -303,18 +391,19 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         # to have only course re-runs go to split. This code, however, uses the config'd priority
         dest_modulestore = self._get_modulestore_for_courseid(dest_course_id)
         if source_modulestore == dest_modulestore:
-            return source_modulestore.clone_course(source_course_id, dest_course_id, user_id, fields)
+            return source_modulestore.clone_course(source_course_id, dest_course_id, user_id, fields, **kwargs)
 
         # ensure super's only called once. The delegation above probably calls it; so, don't move
         # the invocation above the delegation call
-        super(MixedModuleStore, self).clone_course(source_course_id, dest_course_id, user_id, fields)
+        super(MixedModuleStore, self).clone_course(source_course_id, dest_course_id, user_id, fields, **kwargs)
 
         if dest_modulestore.get_modulestore_type() == ModuleStoreEnum.Type.split:
             split_migrator = SplitMigrator(dest_modulestore, source_modulestore)
             split_migrator.migrate_mongo_course(
-                source_course_id, user_id, dest_course_id.org, dest_course_id.course, dest_course_id.run, fields
+                source_course_id, user_id, dest_course_id.org, dest_course_id.course, dest_course_id.run, fields, **kwargs
             )
 
+    @strip_key
     def create_item(self, user_id, course_key, block_type, block_id=None, fields=None, **kwargs):
         """
         Creates and saves a new item in a course.
@@ -334,6 +423,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         modulestore = self._verify_modulestore_support(course_key, 'create_item')
         return modulestore.create_item(user_id, course_key, block_type, block_id=block_id, fields=fields, **kwargs)
 
+    @strip_key
     def create_child(self, user_id, parent_usage_key, block_type, block_id=None, fields=None, **kwargs):
         """
         Creates and saves a new xblock that is a child of the specified block
@@ -353,20 +443,22 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         modulestore = self._verify_modulestore_support(parent_usage_key.course_key, 'create_child')
         return modulestore.create_child(user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, **kwargs)
 
-    def update_item(self, xblock, user_id, allow_not_found=False):
+    @strip_key
+    def update_item(self, xblock, user_id, allow_not_found=False, **kwargs):
         """
         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.
         """
         store = self._verify_modulestore_support(xblock.location.course_key, 'update_item')
-        return store.update_item(xblock, user_id, allow_not_found)
+        return store.update_item(xblock, user_id, allow_not_found, **kwargs)
 
+    @strip_key
     def delete_item(self, location, user_id, **kwargs):
         """
         Delete the given item from persistence. kwargs allow modulestore specific parameters.
         """
         store = self._verify_modulestore_support(location.course_key, 'delete_item')
-        store.delete_item(location, user_id=user_id, **kwargs)
+        return store.delete_item(location, user_id=user_id, **kwargs)
 
     def revert_to_published(self, location, user_id):
         """
@@ -398,6 +490,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
             if hasattr(modulestore, '_drop_database'):
                 modulestore._drop_database()  # pylint: disable=protected-access
 
+    @strip_key
     def create_xmodule(self, location, definition_data=None, metadata=None, runtime=None, fields={}, **kwargs):
         """
         Create the new xmodule but don't save it. Returns the new module.
@@ -411,7 +504,8 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         store = self._verify_modulestore_support(location.course_key, 'create_xmodule')
         return store.create_xmodule(location, definition_data, metadata, runtime, fields, **kwargs)
 
-    def get_courses_for_wiki(self, wiki_slug):
+    @strip_key
+    def get_courses_for_wiki(self, wiki_slug, **kwargs):
         """
         Return the list of courses which use this wiki_slug
         :param wiki_slug: the course wiki root slug
@@ -419,7 +513,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         """
         courses = []
         for modulestore in self.modulestores:
-            courses.extend(modulestore.get_courses_for_wiki(wiki_slug))
+            courses.extend(modulestore.get_courses_for_wiki(wiki_slug, **kwargs))
         return courses
 
     def heartbeat(self):
@@ -448,21 +542,23 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         store = self._get_modulestore_for_courseid(course_id)
         return store.compute_publish_state(xblock)
 
-    def publish(self, location, user_id):
+    @strip_key
+    def publish(self, location, user_id, **kwargs):
         """
         Save a current draft to the underlying modulestore
         Returns the newly published item.
         """
         store = self._verify_modulestore_support(location.course_key, 'publish')
-        return store.publish(location, user_id)
+        return store.publish(location, user_id, **kwargs)
 
-    def unpublish(self, location, user_id):
+    @strip_key
+    def unpublish(self, location, user_id, **kwargs):
         """
         Save a current draft to the underlying modulestore
         Returns the newly unpublished item.
         """
         store = self._verify_modulestore_support(location.course_key, 'unpublish')
-        return store.unpublish(location, user_id)
+        return store.unpublish(location, user_id, **kwargs)
 
     def convert_to_draft(self, location, user_id):
         """
@@ -496,23 +592,35 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         else:
             raise NotImplementedError(u"Cannot call {} on store {}".format(method, store))
 
+    @property
+    def default_modulestore(self):
+        """
+        Return the default modulestore
+        """
+        thread_local_default_store = getattr(self.thread_cache, 'default_store', None)
+        if thread_local_default_store:
+            # return the thread-local cache, if found
+            return thread_local_default_store
+        else:
+            # else return the default store
+            return self.modulestores[0]
+
     @contextmanager
     def default_store(self, store_type):
         """
         A context manager for temporarily changing the default store in the Mixed modulestore to the given store type
         """
-        previous_store_list = self.modulestores
-        found = False
+        # find the store corresponding to the given type
+        store = next((store for store in self.modulestores if store.get_modulestore_type() == store_type), None)
+        if not store:
+            raise Exception(u"Cannot find store of type {}".format(store_type))
+
+        prev_thread_local_store = getattr(self.thread_cache, 'default_store', None)
         try:
-            for i, store in enumerate(self.modulestores):
-                if store.get_modulestore_type() == store_type:
-                    self.modulestores.insert(0, self.modulestores.pop(i))
-                    found = True
+            self.thread_cache.default_store = store
             yield
-            if not found:
-                raise Exception(u"Cannot find store of type {}".format(store_type))
         finally:
-            self.modulestores = previous_store_list
+            self.thread_cache.default_store = prev_thread_local_store
 
     @contextmanager
     def branch_setting(self, branch_setting, course_id=None):

common/lib/xmodule/xmodule/modulestore/modulestore_settings.py

@@ -3,6 +3,7 @@ This file contains helper functions for configuring module_store_setting setting
 """
 
 import warnings
+import copy
 
 
 def convert_module_store_setting_if_needed(module_store_setting):
@@ -42,7 +43,6 @@ def convert_module_store_setting_if_needed(module_store_setting):
                 "ENGINE": "xmodule.modulestore.mixed.MixedModuleStore",
                 "OPTIONS": {
                     "mappings": {},
-                    "reference_type": "Location",
                     "stores": []
                 }
             }
@@ -66,6 +66,27 @@ def convert_module_store_setting_if_needed(module_store_setting):
         )
 
         assert isinstance(module_store_setting['default']['OPTIONS']['stores'], list)
+
+    # If Split is not defined but the DraftMongoModuleStore is configured, add Split as a copy of Draft
+    mixed_stores = module_store_setting['default']['OPTIONS']['stores']
+    is_split_defined = any((store['ENGINE'].endswith('.DraftVersioningModuleStore')) for store in mixed_stores)
+    if not is_split_defined:
+        # find first setting of mongo store
+        mongo_store = next(
+            (store for store in mixed_stores if (
+                store['ENGINE'].endswith('.DraftMongoModuleStore') or store['ENGINE'].endswith('.DraftModuleStore')
+            )),
+            None
+        )
+        if mongo_store:
+            # deepcopy mongo -> split
+            split_store = copy.deepcopy(mongo_store)
+            # update the ENGINE and NAME fields
+            split_store['ENGINE'] = 'xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore'
+            split_store['NAME'] = 'split'
+            # add split to the end of the list
+            mixed_stores.append(split_store)
+
     return module_store_setting
 
 

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

@@ -37,10 +37,11 @@ from xblock.fields import Scope, ScopeIds, Reference, ReferenceList, ReferenceVa
 from xmodule.modulestore import ModuleStoreWriteBase, ModuleStoreEnum
 from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished, DIRECT_ONLY_CATEGORIES
 from opaque_keys.edx.locations import Location
-from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError, ReferentialIntegrityError
+from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError, ReferentialIntegrityError
 from xmodule.modulestore.inheritance import own_metadata, InheritanceMixin, inherit_metadata, InheritanceKeyValueStore
 from xblock.core import XBlock
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
+from opaque_keys.edx.locator import CourseLocator
 from opaque_keys.edx.keys import UsageKey, CourseKey
 from xmodule.exceptions import HeartbeatFailure
 
@@ -354,8 +355,6 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
     """
     A Mongodb backed ModuleStore
     """
-    reference_type = SlashSeparatedCourseKey
-
     # TODO (cpennington): Enable non-filesystem filestores
     # pylint: disable=C0103
     # pylint: disable=W0201
@@ -716,7 +715,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
             for item in items
         ]
 
-    def get_courses(self):
+    def get_courses(self, **kwargs):
         '''
         Returns a list of course descriptors.
         '''
@@ -751,7 +750,16 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
             raise ItemNotFoundError(location)
         return item
 
-    def get_course(self, course_key, depth=0):
+    def make_course_key(self, org, course, run):
+        """
+        Return a valid :class:`~opaque_keys.edx.keys.CourseKey` for this modulestore
+        that matches the supplied `org`, `course`, and `run`.
+
+        This key may represent a course that doesn't exist in this modulestore.
+        """
+        return CourseLocator(org, course, run, deprecated=True)
+
+    def get_course(self, course_key, depth=0, **kwargs):
         """
         Get the course with the given courseid (org/course/run)
         """
@@ -763,7 +771,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         except ItemNotFoundError:
             return None
 
-    def has_course(self, course_key, ignore_case=False):
+    def has_course(self, course_key, ignore_case=False, **kwargs):
         """
         Returns the course_id of the course if it was found, else None
         Note: we return the course_id instead of a boolean here since the found course may have
@@ -838,7 +846,15 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
             for key in ('tag', 'org', 'course', 'category', 'name', 'revision')
         ])
 
-    def get_items(self, course_id, settings=None, content=None, key_revision=MongoRevisionKey.published, **kwargs):
+    def get_items(
+            self,
+            course_id,
+            settings=None,
+            content=None,
+            key_revision=MongoRevisionKey.published,
+            qualifiers=None,
+            **kwargs
+    ):
         """
         Returns:
             list of XModuleDescriptor instances for the matching items within the course with
@@ -853,15 +869,15 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         Args:
             course_id (CourseKey): the course identifier
             settings (dict): fields to look for which have settings scope. Follows same syntax
-                and rules as kwargs below
+                and rules as qualifiers below
             content (dict): fields to look for which have content scope. Follows same syntax and
-                rules as kwargs below.
+                rules as qualifiers below.
             key_revision (str): the revision of the items you're looking for.
                 MongoRevisionKey.draft - only returns drafts
                 MongoRevisionKey.published (equates to None) - only returns published
                 If you want one of each matching xblock but preferring draft to published, call this same method
                 on the draft modulestore with ModuleStoreEnum.RevisionOption.draft_preferred.
-            kwargs (key=value): what to look for within the course.
+            qualifiers (dict): what to look for within the course.
                 Common qualifiers are ``category`` or any field name. if the target field is a list,
                 then it searches for the given value in the list not list equivalence.
                 Substring matching pass a regex object.
@@ -869,20 +885,21 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
                 This modulestore does not allow searching dates by comparison or edited_by, previous_version,
                 update_version info.
         """
+        qualifiers = qualifiers.copy() if qualifiers else {}  # copy the qualifiers (destructively manipulated here)
         query = self._course_key_to_son(course_id)
         query['_id.revision'] = key_revision
         for field in ['category', 'name']:
-            if field in kwargs:
-                query['_id.' + field] = kwargs.pop(field)
+            if field in qualifiers:
+                query['_id.' + field] = qualifiers.pop(field)
 
         for key, value in (settings or {}).iteritems():
             query['metadata.' + key] = value
         for key, value in (content or {}).iteritems():
             query['definition.data.' + key] = value
-        if 'children' in kwargs:
-            query['definition.children'] = kwargs.pop('children')
+        if 'children' in qualifiers:
+            query['definition.children'] = qualifiers.pop('children')
 
-        query.update(kwargs)
+        query.update(qualifiers)
         items = self.collection.find(
             query,
             sort=[SORT_REVISION_FAVOR_DRAFT],
@@ -919,10 +936,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         ])
         courses = self.collection.find(course_search_location, fields=('_id'))
         if courses.count() > 0:
-            raise InvalidLocationError(
-                "There are already courses with the given org and course id: {}".format([
-                    course['_id'] for course in courses
-                ]))
+            raise DuplicateCourseError(course_id, courses[0]['_id'])
 
         location = course_id.make_usage_key('course', course_id.run)
         course = self.create_xmodule(
@@ -1253,7 +1267,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         """
         return ModuleStoreEnum.Type.mongo
 
-    def get_orphans(self, course_key):
+    def get_orphans(self, course_key, **kwargs):
         """
         Return an array of all of the locations for orphans in the course.
         """
@@ -1274,7 +1288,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         item_locs -= all_reachable
         return [course_key.make_usage_key_from_deprecated_string(item_loc) for item_loc in item_locs]
 
-    def get_courses_for_wiki(self, wiki_slug):
+    def get_courses_for_wiki(self, wiki_slug, **kwargs):
         """
         Return the list of courses which use this wiki_slug
         :param wiki_slug: the course wiki root slug

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

@@ -47,7 +47,7 @@ class DraftModuleStore(MongoModuleStore):
     This module also includes functionality to promote DRAFT modules (and their children)
     to published modules.
     """
-    def get_item(self, usage_key, depth=0, revision=None):
+    def get_item(self, usage_key, depth=0, revision=None, **kwargs):
         """
         Returns an XModuleDescriptor instance for the item at usage_key.
 
@@ -155,7 +155,7 @@ class DraftModuleStore(MongoModuleStore):
         course_query = self._course_key_to_son(course_key)
         self.collection.remove(course_query, multi=True)
 
-    def clone_course(self, source_course_id, dest_course_id, user_id, fields=None):
+    def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs):
         """
         Only called if cloning within this store or if env doesn't set up mixed.
         * copy the courseware
@@ -331,11 +331,6 @@ class DraftModuleStore(MongoModuleStore):
                         returns only Published items
                     if the branch setting is ModuleStoreEnum.Branch.draft_preferred,
                         returns either Draft or Published, preferring Draft items.
-            kwargs (key=value): what to look for within the course.
-                Common qualifiers are ``category`` or any field name. if the target field is a list,
-                then it searches for the given value in the list not list equivalence.
-                Substring matching pass a regex object.
-                ``name`` is another commonly provided key (Location based stores)
         """
         def base_get_items(key_revision):
             return super(DraftModuleStore, self).get_items(course_key, key_revision=key_revision, **kwargs)
@@ -439,7 +434,7 @@ class DraftModuleStore(MongoModuleStore):
         # convert the subtree using the original item as the root
         self._breadth_first(convert_item, [location])
 
-    def update_item(self, xblock, user_id, allow_not_found=False, force=False, isPublish=False):
+    def update_item(self, xblock, user_id, allow_not_found=False, force=False, isPublish=False, **kwargs):
         """
         See superclass doc.
         In addition to the superclass's behavior, this method converts the unit to draft if it's not
@@ -616,7 +611,7 @@ class DraftModuleStore(MongoModuleStore):
         else:
             return False
 
-    def publish(self, location, user_id):
+    def publish(self, location, user_id, **kwargs):
         """
         Publish the subtree rooted at location to the live course and remove the drafts.
         Such publishing may cause the deletion of previously published but subsequently deleted
@@ -690,7 +685,7 @@ class DraftModuleStore(MongoModuleStore):
             self.collection.remove({'_id': {'$in': to_be_deleted}})
         return self.get_item(as_published(location))
 
-    def unpublish(self, location, user_id):
+    def unpublish(self, location, user_id, **kwargs):
         """
         Turn the published version into a draft, removing the published version.
 

common/lib/xmodule/xmodule/modulestore/split_migrator.py

@@ -25,7 +25,9 @@ class SplitMigrator(object):
         self.split_modulestore = split_modulestore
         self.source_modulestore = source_modulestore
 
-    def migrate_mongo_course(self, source_course_key, user_id, new_org=None, new_course=None, new_run=None, fields=None):
+    def migrate_mongo_course(
+            self, source_course_key, user_id, new_org=None, new_course=None, new_run=None, fields=None, **kwargs
+    ):
         """
         Create a new course in split_mongo representing the published and draft versions of the course from the
         original mongo store. And return the new CourseLocator
@@ -43,7 +45,7 @@ class SplitMigrator(object):
         # locations are in location, children, conditionals, course.tab
 
         # create the course: set fields to explicitly_set for each scope, id_root = new_course_locator, master_branch = 'production'
-        original_course = self.source_modulestore.get_course(source_course_key)
+        original_course = self.source_modulestore.get_course(source_course_key, **kwargs)
 
         if new_org is None:
             new_org = source_course_key.org
@@ -60,17 +62,20 @@ class SplitMigrator(object):
             new_org, new_course, new_run, user_id,
             fields=new_fields,
             master_branch=ModuleStoreEnum.BranchName.published,
+            **kwargs
         )
 
         with self.split_modulestore.bulk_write_operations(new_course.id):
-            self._copy_published_modules_to_course(new_course, original_course.location, source_course_key, user_id)
+            self._copy_published_modules_to_course(
+                new_course, original_course.location, source_course_key, user_id, **kwargs
+            )
         # create a new version for the drafts
         with self.split_modulestore.bulk_write_operations(new_course.id):
-            self._add_draft_modules_to_course(new_course.location, source_course_key, user_id)
+            self._add_draft_modules_to_course(new_course.location, source_course_key, user_id, **kwargs)
 
         return new_course.id
 
-    def _copy_published_modules_to_course(self, new_course, old_course_loc, source_course_key, user_id):
+    def _copy_published_modules_to_course(self, new_course, old_course_loc, source_course_key, user_id, **kwargs):
         """
         Copy all of the modules from the 'direct' version of the course to the new split course.
         """
@@ -79,7 +84,7 @@ class SplitMigrator(object):
         # iterate over published course elements. Wildcarding rather than descending b/c some elements are orphaned (e.g.,
         # course about pages, conditionals)
         for module in self.source_modulestore.get_items(
-            source_course_key, revision=ModuleStoreEnum.RevisionOption.published_only
+            source_course_key, revision=ModuleStoreEnum.RevisionOption.published_only, **kwargs
         ):
             # don't copy the course again.
             if module.location != old_course_loc:
@@ -95,7 +100,8 @@ class SplitMigrator(object):
                     fields=self._get_fields_translate_references(
                         module, course_version_locator, new_course.location.block_id
                     ),
-                    continue_version=True
+                    continue_version=True,
+                    **kwargs
                 )
         # after done w/ published items, add version for DRAFT pointing to the published structure
         index_info = self.split_modulestore.get_course_index_info(course_version_locator)
@@ -107,7 +113,7 @@ class SplitMigrator(object):
         # children which meant some pointers were to non-existent locations in 'direct'
         self.split_modulestore.internal_clean_children(course_version_locator)
 
-    def _add_draft_modules_to_course(self, published_course_usage_key, source_course_key, user_id):
+    def _add_draft_modules_to_course(self, published_course_usage_key, source_course_key, user_id, **kwargs):
         """
         update each draft. Create any which don't exist in published and attach to their parents.
         """
@@ -117,11 +123,13 @@ class SplitMigrator(object):
         # to prevent race conditions of grandchilden being added before their parents and thus having no parent to
         # add to
         awaiting_adoption = {}
-        for module in self.source_modulestore.get_items(source_course_key, revision=ModuleStoreEnum.RevisionOption.draft_only):
+        for module in self.source_modulestore.get_items(
+                source_course_key, revision=ModuleStoreEnum.RevisionOption.draft_only, **kwargs
+        ):
             new_locator = new_draft_course_loc.make_usage_key(module.category, module.location.block_id)
             if self.split_modulestore.has_item(new_locator):
                 # was in 'direct' so draft is a new version
-                split_module = self.split_modulestore.get_item(new_locator)
+                split_module = self.split_modulestore.get_item(new_locator, **kwargs)
                 # need to remove any no-longer-explicitly-set values and add/update any now set values.
                 for name, field in split_module.fields.iteritems():
                     if field.is_set_on(split_module) and not module.fields[name].is_set_on(module):
@@ -131,7 +139,7 @@ class SplitMigrator(object):
                 ).iteritems():
                     field.write_to(split_module, value)
 
-                _new_module = self.split_modulestore.update_item(split_module, user_id)
+                _new_module = self.split_modulestore.update_item(split_module, user_id, **kwargs)
             else:
                 # only a draft version (aka, 'private').
                 _new_module = self.split_modulestore.create_item(
@@ -140,22 +148,23 @@ class SplitMigrator(object):
                     block_id=new_locator.block_id,
                     fields=self._get_fields_translate_references(
                         module, new_draft_course_loc, published_course_usage_key.block_id
-                    )
+                    ),
+                    **kwargs
                 )
                 awaiting_adoption[module.location] = new_locator
         for draft_location, new_locator in awaiting_adoption.iteritems():
             parent_loc = self.source_modulestore.get_parent_location(
-                draft_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred
+                draft_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred, **kwargs
             )
             if parent_loc is None:
                 log.warn(u'No parent found in source course for %s', draft_location)
                 continue
-            old_parent = self.source_modulestore.get_item(parent_loc)
+            old_parent = self.source_modulestore.get_item(parent_loc, **kwargs)
             split_parent_loc = new_draft_course_loc.make_usage_key(
                 parent_loc.category,
                 parent_loc.block_id if parent_loc.category != 'course' else published_course_usage_key.block_id
             )
-            new_parent = self.split_modulestore.get_item(split_parent_loc)
+            new_parent = self.split_modulestore.get_item(split_parent_loc, **kwargs)
             # this only occurs if the parent was also awaiting adoption: skip this one, go to next
             if any(new_locator == child.version_agnostic() for child in new_parent.children):
                 continue

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

@@ -53,7 +53,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         self.default_class = default_class
         self.local_modules = {}
 
-    def _load_item(self, block_id, course_entry_override=None):
+    def _load_item(self, block_id, course_entry_override=None, **kwargs):
         if isinstance(block_id, BlockUsageLocator):
             if isinstance(block_id.block_id, LocalId):
                 try:
@@ -77,7 +77,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
                 raise ItemNotFoundError(block_id)
 
         class_ = self.load_block_type(json_data.get('category'))
-        return self.xblock_from_json(class_, block_id, json_data, course_entry_override)
+        return self.xblock_from_json(class_, block_id, json_data, course_entry_override, **kwargs)
 
     # xblock's runtime does not always pass enough contextual information to figure out
     # which named container (course x branch) or which parent is requesting an item. Because split allows
@@ -90,7 +90,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
     # low; thus, the course_entry is most likely correct. If the thread is looking at > 1 named container
     # pointing to the same structure, the access is likely to be chunky enough that the last known container
     # is the intended one when not given a course_entry_override; thus, the caching of the last branch/course id.
-    def xblock_from_json(self, class_, block_id, json_data, course_entry_override=None):
+    def xblock_from_json(self, class_, block_id, json_data, course_entry_override=None, **kwargs):
         if course_entry_override is None:
             course_entry_override = self.course_entry
         else:
@@ -126,6 +126,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
             definition,
             converted_fields,
             json_data.get('_inherited_settings'),
+            **kwargs
         )
         field_data = KvsFieldData(kvs)
 
@@ -151,6 +152,10 @@ class CachingDescriptorSystem(MakoDescriptorSystem):
         edit_info = json_data.get('edit_info', {})
         module.edited_by = edit_info.get('edited_by')
         module.edited_on = edit_info.get('edited_on')
+        module.subtree_edited_by = None  # TODO - addressed with LMS-11183
+        module.subtree_edited_on = None  # TODO - addressed with LMS-11183
+        module.published_by = None  # TODO - addressed with LMS-11184
+        module.published_date = None  # TODO - addressed with LMS-11184
         module.previous_version = edit_info.get('previous_version')
         module.update_version = edit_info.get('update_version')
         module.source_version = edit_info.get('source_version', None)

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

@@ -63,7 +63,7 @@ from xblock.fields import Scope, Reference, ReferenceList, ReferenceValueDict
 from xmodule.errortracker import null_error_tracker
 from opaque_keys.edx.locator import (
     BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree,
-    LocalId, Locator
+    LocalId,
 )
 from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError, \
     DuplicateCourseError
@@ -77,7 +77,6 @@ from .caching_descriptor_system import CachingDescriptorSystem
 from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection
 from xmodule.error_module import ErrorDescriptor
 from xmodule.modulestore.split_mongo import encode_key_for_mongo, decode_key_from_mongo
-import types
 from _collections import defaultdict
 
 
@@ -111,7 +110,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
     """
 
     SCHEMA_VERSION = 1
-    reference_type = Locator
     # a list of field names to store in course index search_targets. Note, this will
     # only record one value per key. If branches disagree, the last one set wins.
     # It won't recompute the value on operations such as update_course_index (e.g., to revert to a prev
@@ -214,7 +212,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         system.module_data.update(new_module_data)
         return system.module_data
 
-    def _load_items(self, course_entry, block_ids, depth=0, lazy=True):
+    def _load_items(self, course_entry, block_ids, depth=0, lazy=True, **kwargs):
         '''
         Load & cache the given blocks from the course. Prefetch down to the
         given depth. Load the definitions into each block if lazy is False;
@@ -248,7 +246,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                 branch=course_entry.get('branch'),
             )
             self.cache_items(system, block_ids, course_key, depth, lazy)
-        return [system.load_item(block_id, course_entry) for block_id in block_ids]
+        return [system.load_item(block_id, course_entry, **kwargs) for block_id in block_ids]
 
     def _get_cache(self, course_version_guid):
         """
@@ -333,7 +331,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         }
         return envelope
 
-    def get_courses(self, branch, qualifiers=None):
+    def get_courses(self, branch, qualifiers=None, **kwargs):
         '''
         Returns a list of course descriptors matching any given qualifiers.
 
@@ -373,12 +371,21 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                 'structure': entry,
             }
             root = entry['root']
-            course_list = self._load_items(envelope, [root], 0, lazy=True)
+            course_list = self._load_items(envelope, [root], 0, lazy=True, **kwargs)
             if not isinstance(course_list[0], ErrorDescriptor):
                 result.append(course_list[0])
         return result
 
-    def get_course(self, course_id, depth=0):
+    def make_course_key(self, org, course, run):
+        """
+        Return a valid :class:`~opaque_keys.edx.keys.CourseKey` for this modulestore
+        that matches the supplied `org`, `course`, and `run`.
+
+        This key may represent a course that doesn't exist in this modulestore.
+        """
+        return CourseLocator(org, course, run)
+
+    def get_course(self, course_id, depth=0, **kwargs):
         '''
         Gets the course descriptor for the course identified by the locator
         '''
@@ -388,10 +395,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
 
         course_entry = self._lookup_course(course_id)
         root = course_entry['structure']['root']
-        result = self._load_items(course_entry, [root], 0, lazy=True)
+        result = self._load_items(course_entry, [root], 0, lazy=True, **kwargs)
         return result[0]
 
-    def has_course(self, course_id, ignore_case=False):
+    def has_course(self, course_id, ignore_case=False, **kwargs):
         '''
         Does this course exist in this modulestore. This method does not verify that the branch &/or
         version in the course_id exists. Use get_course_index_info to check that.
@@ -423,7 +430,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
 
         return self._get_block_from_structure(course_structure, usage_key.block_id) is not None
 
-    def get_item(self, usage_key, depth=0):
+    def get_item(self, usage_key, depth=0, **kwargs):
         """
         depth (int): An argument that some module stores may use to prefetch
             descendants of the queried modules for more efficient results later
@@ -437,14 +444,14 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             raise ItemNotFoundError(usage_key)
 
         course = self._lookup_course(usage_key)
-        items = self._load_items(course, [usage_key.block_id], depth, lazy=True)
+        items = self._load_items(course, [usage_key.block_id], depth, lazy=True, **kwargs)
         if len(items) == 0:
             raise ItemNotFoundError(usage_key)
         elif len(items) > 1:
             log.debug("Found more than one item for '{}'".format(usage_key))
         return items[0]
 
-    def get_items(self, course_locator, settings=None, content=None, **kwargs):
+    def get_items(self, course_locator, settings=None, content=None, qualifiers=None, **kwargs):
         """
         Returns:
             list of XModuleDescriptor instances for the matching items within the course with
@@ -455,10 +462,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         Args:
             course_locator (CourseLocator): the course identifier
             settings (dict): fields to look for which have settings scope. Follows same syntax
-                and rules as kwargs below
+                and rules as qualifiers below
             content (dict): fields to look for which have content scope. Follows same syntax and
-                rules as kwargs below.
-            kwargs (key=value): what to look for within the course.
+                rules as qualifiers below.
+            qualifiers (dict): what to look for within the course.
                 Common qualifiers are ``category`` or any field name. if the target field is a list,
                 then it searches for the given value in the list not list equivalence.
                 For substring matching pass a regex object.
@@ -467,6 +474,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         """
         course = self._lookup_course(course_locator)
         items = []
+        qualifiers = qualifiers.copy() if qualifiers else {}  # copy the qualifiers (destructively manipulated here)
 
         def _block_matches_all(block_json):
             """
@@ -474,7 +482,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             """
             # do the checks which don't require loading any additional data
             if (
-                self._block_matches(block_json, kwargs) and
+                self._block_matches(block_json, qualifiers) and
                 self._block_matches(block_json.get('fields', {}), settings)
             ):
                 if content:
@@ -485,23 +493,23 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
 
         if settings is None:
             settings = {}
-        if 'name' in kwargs:
+        if 'name' in qualifiers:
             # odd case where we don't search just confirm
-            block_id = kwargs.pop('name')
+            block_id = qualifiers.pop('name')
             block = course['structure']['blocks'].get(block_id)
             if _block_matches_all(block):
-                return self._load_items(course, [block_id], lazy=True)
+                return self._load_items(course, [block_id], lazy=True, **kwargs)
             else:
                 return []
         # don't expect caller to know that children are in fields
-        if 'children' in kwargs:
-            settings['children'] = kwargs.pop('children')
+        if 'children' in qualifiers:
+            settings['children'] = qualifiers.pop('children')
         for block_id, value in course['structure']['blocks'].iteritems():
             if _block_matches_all(value):
                 items.append(block_id)
 
         if len(items) > 0:
-            return self._load_items(course, items, 0, lazy=True)
+            return self._load_items(course, items, 0, lazy=True, **kwargs)
         else:
             return []
 
@@ -523,7 +531,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             block_id=decode_key_from_mongo(parent_id),
         )
 
-    def get_orphans(self, course_key):
+    def get_orphans(self, course_key, **kwargs):
         """
         Return an array of all of the orphans in the course.
         """
@@ -820,6 +828,11 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         the course id'd by version_guid but instead in one w/ a new version_guid. Ensure in this case that you get
         the new version_guid from the locator in the returned object!
         """
+        # split handles all the fields in one dict not separated by scope
+        fields = fields or {}
+        fields.update(kwargs.pop('metadata', {}) or {})
+        fields.update(kwargs.pop('definition_data', {}) or {})
+
         # find course_index entry if applicable and structures entry
         index_entry = self._get_index_if_valid(course_key, force, continue_version)
         structure = self._lookup_course(course_key)['structure']
@@ -940,18 +953,20 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
         # don't need to update the index b/c create_item did it for this version
         return xblock
 
-    def clone_course(self, source_course_id, dest_course_id, user_id, fields=None):
+    def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs):
         """
         See :meth: `.ModuleStoreWrite.clone_course` for documentation.
 
         In split, other than copying the assets, this is cheap as it merely creates a new version of the
         existing course.
         """
-        super(SplitMongoModuleStore, self).clone_course(source_course_id, dest_course_id, user_id, fields)
+        super(SplitMongoModuleStore, self).clone_course(source_course_id, dest_course_id, user_id, fields, **kwargs)
         source_index = self.get_course_index_info(source_course_id)
+        if source_index is None:
+            raise ItemNotFoundError("Cannot find a course at {0}. Aborting".format(source_course_id))
         return self.create_course(
             dest_course_id.org, dest_course_id.course, dest_course_id.run, user_id, fields=fields,
-            versions_dict=source_index['versions'], search_targets=source_index['search_targets']
+            versions_dict=source_index['versions'], search_targets=source_index['search_targets'], **kwargs
         )
 
     def create_course(
@@ -1087,10 +1102,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             self._update_search_targets(index_entry, fields)
         self.db_connection.insert_course_index(index_entry)
         # expensive hack to persist default field values set in __init__ method (e.g., wiki_slug)
-        course = self.get_course(locator)
-        return self.update_item(course, user_id)
+        course = self.get_course(locator, **kwargs)
+        return self.update_item(course, user_id, **kwargs)
 
-    def update_item(self, descriptor, user_id, allow_not_found=False, force=False):
+    def update_item(self, descriptor, user_id, allow_not_found=False, force=False, **kwargs):
         """
         Save the descriptor's fields. it doesn't descend the course dag to save the children.
         Return the new descriptor (updated location).
@@ -1161,12 +1176,12 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
 
             # fetch and return the new item--fetching is unnecessary but a good qc step
             new_locator = descriptor.location.map_into_course(course_key)
-            return self.get_item(new_locator)
+            return self.get_item(new_locator, **kwargs)
         else:
             # nothing changed, just return the one sent in
             return descriptor
 
-    def create_xblock(self, runtime, category, fields=None, block_id=None, definition_id=None, parent_xblock=None):
+    def create_xblock(self, runtime, category, fields=None, block_id=None, definition_id=None, parent_xblock=None, **kwargs):
         """
         This method instantiates the correct subclass of XModuleDescriptor based
         on the contents of json_data. It does not persist it and can create one which
@@ -1193,7 +1208,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
                     if field_name in fields:
                         json_data['_inherited_settings'][field_name] = fields[field_name]
 
-        new_block = runtime.xblock_from_json(xblock_class, block_id, json_data)
+        new_block = runtime.xblock_from_json(xblock_class, block_id, json_data, **kwargs)
         if parent_xblock is not None:
             parent_xblock.children.append(new_block.scope_ids.usage_id)
             # decache pending children field settings
@@ -1844,6 +1859,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             destination_block['edit_info']['previous_version'] = previous_version
             destination_block['edit_info']['update_version'] = destination_version
             destination_block['edit_info']['edited_by'] = user_id
+            destination_block['edit_info']['edited_on'] = datetime.datetime.now(UTC)
         else:
             destination_block = self._new_block(
                 user_id, new_block['category'],
@@ -1939,7 +1955,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase):
             for entry in entries
         ]
 
-    def get_courses_for_wiki(self, wiki_slug):
+    def get_courses_for_wiki(self, wiki_slug, **kwargs):
         """
         Return the list of courses which use this wiki_slug
         :param wiki_slug: the course wiki root slug

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

@@ -48,16 +48,22 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         item = super(DraftVersioningModuleStore, self).create_course(
             org, course, run, user_id, master_branch=master_branch, **kwargs
         )
-        self._auto_publish_no_children(item.location, item.location.category, user_id)
+        self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs)
         return item
 
-    def get_courses(self):
+    def get_courses(self, **kwargs):
         """
-        Returns all the courses on the Draft branch (which is a superset of the courses on the Published branch).
+        Returns all the courses on the Draft or Published branch depending on the branch setting.
         """
-        return super(DraftVersioningModuleStore, self).get_courses(ModuleStoreEnum.BranchName.draft)
+        branch_setting = self.get_branch_setting()
+        if branch_setting == ModuleStoreEnum.Branch.draft_preferred:
+            return super(DraftVersioningModuleStore, self).get_courses(ModuleStoreEnum.BranchName.draft, **kwargs)
+        elif branch_setting == ModuleStoreEnum.Branch.published_only:
+            return super(DraftVersioningModuleStore, self).get_courses(ModuleStoreEnum.BranchName.published, **kwargs)
+        else:
+            raise InsufficientSpecificationError()
 
-    def _auto_publish_no_children(self, location, category, user_id):
+    def _auto_publish_no_children(self, location, category, user_id, **kwargs):
         """
         Publishes item if the category is DIRECT_ONLY. This assumes another method has checked that
         location points to the head of the branch and ignores the version. If you call this in any
@@ -66,16 +72,17 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         """
         if location.branch == ModuleStoreEnum.BranchName.draft and category in DIRECT_ONLY_CATEGORIES:
             # version_agnostic b/c of above assumption in docstring
-            self.publish(location.version_agnostic(), user_id, blacklist=EXCLUDE_ALL)
+            self.publish(location.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs)
 
-    def update_item(self, descriptor, user_id, allow_not_found=False, force=False):
+    def update_item(self, descriptor, user_id, allow_not_found=False, force=False, **kwargs):
         item = super(DraftVersioningModuleStore, self).update_item(
             descriptor,
             user_id,
             allow_not_found=allow_not_found,
-            force=force
+            force=force,
+            **kwargs
         )
-        self._auto_publish_no_children(item.location, item.location.category, user_id)
+        self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs)
         return item
 
     def create_item(
@@ -88,7 +95,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
             definition_locator=definition_locator, fields=fields,
             force=force, continue_version=continue_version, **kwargs
         )
-        self._auto_publish_no_children(item.location, item.location.category, user_id)
+        self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs)
         return item
 
     def create_child(
@@ -99,7 +106,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
             user_id, parent_usage_key, block_type, block_id=block_id,
             fields=fields, continue_version=continue_version, **kwargs
         )
-        self._auto_publish_no_children(parent_usage_key, item.location.category, user_id)
+        self._auto_publish_no_children(parent_usage_key, item.location.category, user_id, **kwargs)
         return item
 
     def delete_item(self, location, user_id, revision=None, **kwargs):
@@ -134,8 +141,8 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         for branch in branches_to_delete:
             branched_location = location.for_branch(branch)
             parent_loc = self.get_parent_location(branched_location)
-            SplitMongoModuleStore.delete_item(self, branched_location, user_id, **kwargs)
-            self._auto_publish_no_children(parent_loc, parent_loc.category, user_id)
+            SplitMongoModuleStore.delete_item(self, branched_location, user_id)
+            self._auto_publish_no_children(parent_loc, parent_loc.category, user_id, **kwargs)
 
     def _map_revision_to_branch(self, key, revision=None):
         """
@@ -157,25 +164,20 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         usage_key = self._map_revision_to_branch(usage_key, revision=revision)
         return super(DraftVersioningModuleStore, self).has_item(usage_key)
 
-    def get_item(self, usage_key, depth=0, revision=None):
+    def get_item(self, usage_key, depth=0, revision=None, **kwargs):
         """
         Returns the item identified by usage_key and revision.
         """
         usage_key = self._map_revision_to_branch(usage_key, revision=revision)
-        return super(DraftVersioningModuleStore, self).get_item(usage_key, depth=depth)
+        return super(DraftVersioningModuleStore, self).get_item(usage_key, depth=depth, **kwargs)
 
-    def get_items(self, course_locator, settings=None, content=None, revision=None, **kwargs):
+    def get_items(self, course_locator, revision=None, **kwargs):
         """
         Returns a list of XModuleDescriptor instances for the matching items within the course with
         the given course_locator.
         """
         course_locator = self._map_revision_to_branch(course_locator, revision=revision)
-        return super(DraftVersioningModuleStore, self).get_items(
-            course_locator,
-            settings=settings,
-            content=content,
-            **kwargs
-        )
+        return super(DraftVersioningModuleStore, self).get_items(course_locator, **kwargs)
 
     def get_parent_location(self, location, revision=None, **kwargs):
         '''
@@ -200,14 +202,18 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         :param xblock: the block to check
         :return: True if the draft and published versions differ
         """
-        # TODO for better performance: lookup the courses and get the block entry, don't create the instances
-        draft = self.get_item(xblock.location.for_branch(ModuleStoreEnum.BranchName.draft))
-        try:
-            published = self.get_item(xblock.location.for_branch(ModuleStoreEnum.BranchName.published))
-        except ItemNotFoundError:
+        def get_block(branch_name):
+            course_structure = self._lookup_course(xblock.location.for_branch(branch_name))['structure']
+            return self._get_block_from_structure(course_structure, xblock.location.block_id)
+
+        draft_block = get_block(ModuleStoreEnum.BranchName.draft)
+        published_block = get_block(ModuleStoreEnum.BranchName.published)
+        if not published_block:
             return True
 
-        return draft.update_version != published.source_version
+        # check if the draft has changed since the published was created
+        return self._get_version(draft_block) != self._get_version(published_block)
+
 
     def publish(self, location, user_id, blacklist=None, **kwargs):
         """
@@ -224,15 +230,15 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
             [location],
             blacklist=blacklist
         )
-        return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.published))
+        return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.published), **kwargs)
 
-    def unpublish(self, location, user_id):
+    def unpublish(self, location, user_id, **kwargs):
         """
         Deletes the published version of the item.
         Returns the newly unpublished item.
         """
         self.delete_item(location, user_id, revision=ModuleStoreEnum.RevisionOption.published_only)
-        return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.draft))
+        return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.draft), **kwargs)
 
     def revert_to_published(self, location, user_id):
         """
@@ -255,24 +261,13 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
             PublishState.public - published exists and is the same as draft
             PublishState.private - no published version exists
         """
-        def get_head(branch):
-            course_structure = self._lookup_course(xblock.location.course_key.for_branch(branch))['structure']
-            return self._get_block_from_structure(course_structure, xblock.location.block_id)
-
-        def get_version(block):
-            """
-            Return the version of the given database representation of a block.
-            """
-            #TODO: make this method a more generic helper
-            return block['edit_info'].get('source_version', block['edit_info']['update_version'])
-
-        draft_head = get_head(ModuleStoreEnum.BranchName.draft)
-        published_head = get_head(ModuleStoreEnum.BranchName.published)
+        draft_head = self._get_head(xblock, ModuleStoreEnum.BranchName.draft)
+        published_head = self._get_head(xblock, ModuleStoreEnum.BranchName.published)
 
         if not published_head:
             # published version does not exist
             return PublishState.private
-        elif get_version(draft_head) == get_version(published_head):
+        elif self._get_version(draft_head) == self._get_version(published_head):
             # published and draft versions are equal
             return PublishState.public
         else:
@@ -287,3 +282,13 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS
         """
         # This is a no-op in Split since a draft version of the data always remains
         pass
+
+    def _get_head(self, xblock, branch):
+        course_structure = self._lookup_course(xblock.location.course_key.for_branch(branch))['structure']
+        return self._get_block_from_structure(course_structure, xblock.location.block_id)
+
+    def _get_version(self, block):
+        """
+        Return the version of the given database representation of a block.
+        """
+        return block['edit_info'].get('source_version', block['edit_info']['update_version'])

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

@@ -15,26 +15,27 @@ class SplitMongoKVS(InheritanceKeyValueStore):
     known to the MongoModuleStore (data, children, and metadata)
     """
 
-    def __init__(self, definition, fields, inherited_settings):
+    def __init__(self, definition, initial_values, inherited_settings, **kwargs):
         """
 
         :param definition: either a lazyloader or definition id for the definition
-        :param fields: a dictionary of the locally set fields
+        :param initial_values: a dictionary of the locally set values
         :param inherited_settings: the json value of each inheritable field from above this.
             Note, local fields may override and disagree w/ this b/c this says what the value
             should be if the field is undefined.
         """
         # deepcopy so that manipulations of fields does not pollute the source
-        super(SplitMongoKVS, self).__init__(copy.deepcopy(fields), inherited_settings)
+        super(SplitMongoKVS, self).__init__(copy.deepcopy(initial_values), inherited_settings)
         self._definition = definition  # either a DefinitionLazyLoader or the db id of the definition.
         # if the db id, then the definition is presumed to be loaded into _fields
 
+        # a decorator function for field values (to be called when a field is accessed)
+        self.field_decorator = kwargs.get('field_decorator', lambda x: x)
 
-    def get(self, key):
-        # simplest case, field is directly set
-        if key.field_name in self._fields:
-            return self._fields[key.field_name]
 
+    def get(self, key):
+        # load the field, if needed
+        if key.field_name not in self._fields:
             # parent undefined in editing runtime (I think)
             if key.scope == Scope.parent:
                 # see STUD-624. Right now copies MongoKeyValueStore.get's behavior of returning None
@@ -48,13 +49,19 @@ class SplitMongoKVS(InheritanceKeyValueStore):
             elif key.scope == Scope.content:
                 if isinstance(self._definition, DefinitionLazyLoader):
                     self._load_definition()
-                if key.field_name in self._fields:
-                    return self._fields[key.field_name]
-
+                else:
                     raise KeyError()
             else:
                 raise InvalidScopeError(key)
 
+        if key.field_name in self._fields:
+            field_value = self._fields[key.field_name]
+
+            # return the "decorated" field value
+            return self.field_decorator(field_value)
+
+        return None
+
     def set(self, key, value):
         # handle any special cases
         if key.scope not in [Scope.children, Scope.settings, Scope.content]:

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

@@ -9,7 +9,8 @@ from tempfile import mkdtemp
 import path
 import shutil
 
-from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation
+from opaque_keys.edx.locator import CourseLocator, AssetLocator
+from opaque_keys.edx.keys import AssetKey
 from xmodule.tests import DATA_DIR
 from xmodule.contentstore.mongo import MongoContentStore
 from xmodule.contentstore.content import StaticContent
@@ -41,13 +42,13 @@ class TestContentstore(unittest.TestCase):
         Restores deprecated values
         """
         if cls.asset_deprecated is not None:
-            setattr(AssetLocation, 'deprecated', cls.asset_deprecated)
+            setattr(AssetLocator, 'deprecated', cls.asset_deprecated)
         else:
-            delattr(AssetLocation, 'deprecated')
+            delattr(AssetLocator, 'deprecated')
         if cls.ssck_deprecated is not None:
-            setattr(SlashSeparatedCourseKey, 'deprecated', cls.ssck_deprecated)
+            setattr(CourseLocator, 'deprecated', cls.ssck_deprecated)
         else:
-            delattr(SlashSeparatedCourseKey, 'deprecated')
+            delattr(CourseLocator, 'deprecated')
         return super(TestContentstore, cls).tearDownClass()
 
     def set_up_assets(self, deprecated):
@@ -59,11 +60,11 @@ class TestContentstore(unittest.TestCase):
         self.contentstore = MongoContentStore(HOST, DB, port=PORT)
         self.addCleanup(self.contentstore._drop_database)  # pylint: disable=protected-access
 
-        setattr(AssetLocation, 'deprecated', deprecated)
-        setattr(SlashSeparatedCourseKey, 'deprecated', deprecated)
+        setattr(AssetLocator, 'deprecated', deprecated)
+        setattr(CourseLocator, 'deprecated', deprecated)
 
-        self.course1_key = SlashSeparatedCourseKey('test', 'asset_test', '2014_07')
-        self.course2_key = SlashSeparatedCourseKey('test', 'asset_test2', '2014_07')
+        self.course1_key = CourseLocator('test', 'asset_test', '2014_07')
+        self.course2_key = CourseLocator('test', 'asset_test2', '2014_07')
 
         self.course1_files = ['contains.sh', 'picture1.jpg', 'picture2.jpg']
         self.course2_files = ['picture1.jpg', 'picture3.jpg', 'door_2.ogg']
@@ -154,13 +155,13 @@ class TestContentstore(unittest.TestCase):
         course1_assets, count = self.contentstore.get_all_content_for_course(self.course1_key)
         self.assertEqual(count, len(self.course1_files), course1_assets)
         for asset in course1_assets:
-            parsed = AssetLocation.from_deprecated_string(asset['filename'])
+            parsed = AssetKey.from_string(asset['filename'])
             self.assertIn(parsed.name, self.course1_files)
 
         course1_assets, __ = self.contentstore.get_all_content_for_course(self.course1_key, 1, 1)
         self.assertEqual(len(course1_assets), 1, course1_assets)
 
-        fake_course = SlashSeparatedCourseKey('test', 'fake', 'non')
+        fake_course = CourseLocator('test', 'fake', 'non')
         course_assets, count = self.contentstore.get_all_content_for_course(fake_course)
         self.assertEqual(count, 0)
         self.assertEqual(course_assets, [])
@@ -183,7 +184,7 @@ class TestContentstore(unittest.TestCase):
         copy_all_course_assets
         """
         self.set_up_assets(deprecated)
-        dest_course = SlashSeparatedCourseKey('test', 'destination', 'copy')
+        dest_course = CourseLocator('test', 'destination', 'copy')
         self.contentstore.copy_all_course_assets(self.course1_key, dest_course)
         for filename in self.course1_files:
             asset_key = self.course1_key.make_asset_key('asset', filename)

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

 import pymongo
 from uuid import uuid4
 import ddt
+import itertools
 from importlib import import_module
 from collections import namedtuple
 import unittest
@@ -8,7 +9,6 @@ import datetime
 from pytz import UTC
 
 from xmodule.tests import DATA_DIR
-from opaque_keys.edx.locations import Location
 from xmodule.modulestore import ModuleStoreEnum, PublishState
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.exceptions import InvalidVersionError
@@ -21,6 +21,7 @@ from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator
 from django.conf import settings
 from xmodule.modulestore.tests.factories import check_mongo_calls
 from xmodule.modulestore.search import path_to_location
+from xmodule.modulestore.exceptions import DuplicateCourseError
 if not settings.configured:
     settings.configure()
 from xmodule.modulestore.mixed import MixedModuleStore
@@ -90,7 +91,7 @@ class TestMixedModuleStore(unittest.TestCase):
         """
         AssertEqual replacement for CourseLocator
         """
-        if loc1.version_agnostic() != loc2.version_agnostic():
+        if loc1.for_branch(None) != loc2.for_branch(None):
             self.fail(self._formatMessage(msg, u"{} != {}".format(unicode(loc1), unicode(loc2))))
 
     def setUp(self):
@@ -124,13 +125,13 @@ class TestMixedModuleStore(unittest.TestCase):
         # create course
         self.course = self.store.create_course(course_key.org, course_key.course, course_key.run, self.user_id)
         if isinstance(self.course.id, CourseLocator):
-            self.course_locations[self.MONGO_COURSEID] = self.course.location.version_agnostic()
+            self.course_locations[self.MONGO_COURSEID] = self.course.location
         else:
             self.assertEqual(self.course.id, course_key)
 
         # create chapter
         chapter = self.store.create_child(self.user_id, self.course.location, 'chapter', block_id='Overview')
-        self.writable_chapter_location = chapter.location.version_agnostic()
+        self.writable_chapter_location = chapter.location
 
     def _create_block_hierarchy(self):
         """
@@ -175,13 +176,13 @@ class TestMixedModuleStore(unittest.TestCase):
 
         def create_sub_tree(parent, block_info):
             block = self.store.create_child(
-                self.user_id, parent.location.version_agnostic(),
+                self.user_id, parent.location,
                 block_info.category, block_id=block_info.display_name,
                 fields={'display_name': block_info.display_name},
             )
             for tree in block_info.sub_tree:
                 create_sub_tree(block, tree)
-            setattr(self, block_info.field_name, block.location.version_agnostic())
+            setattr(self, block_info.field_name, block.location)
 
         for tree in trees:
             create_sub_tree(self.course, tree)
@@ -192,6 +193,10 @@ class TestMixedModuleStore(unittest.TestCase):
         """
         return self.course_locations[string].course_key
 
+    def _initialize_mixed(self):
+        self.store = MixedModuleStore(None, create_modulestore_instance=create_modulestore_instance, **self.options)
+        self.addCleanup(self.store.close_all_connections)
+
     def initdb(self, default):
         """
         Initialize the database and create one test course in it
@@ -203,8 +208,7 @@ class TestMixedModuleStore(unittest.TestCase):
                 if index > 0:
                     store_configs[index], store_configs[0] = store_configs[0], store_configs[index]
                 break
-        self.store = MixedModuleStore(None, create_modulestore_instance=create_modulestore_instance, **self.options)
-        self.addCleanup(self.store.close_all_connections)
+        self._initialize_mixed()
 
         # convert to CourseKeys
         self.course_locations = {
@@ -216,12 +220,9 @@ class TestMixedModuleStore(unittest.TestCase):
             course_id: course_key.make_usage_key('course', course_key.run)
             for course_id, course_key in self.course_locations.iteritems()  # pylint: disable=maybe-no-member
         }
-        if default == 'split':
-            self.fake_location = CourseLocator(
-                'foo', 'bar', 'slowly', branch=ModuleStoreEnum.BranchName.draft
-            ).make_usage_key('vertical', 'baz')
-        else:
-            self.fake_location = Location('foo', 'bar', 'slowly', 'vertical', 'baz')
+
+        self.fake_location = self.course_locations[self.MONGO_COURSEID].course_key.make_usage_key('vertical', 'fake')
+
         self.xml_chapter_location = self.course_locations[self.XML_COURSEID1].replace(
             category='chapter', name='Overview'
         )
@@ -248,6 +249,23 @@ class TestMixedModuleStore(unittest.TestCase):
             SlashSeparatedCourseKey('foo', 'bar', '2012_Fall')), mongo_ms_type
         )
 
+    @ddt.data(*itertools.product(
+        (ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split),
+        (True, False)
+    ))
+    @ddt.unpack
+    def test_duplicate_course_error(self, default_ms, reset_mixed_mappings):
+        """
+        Make sure we get back the store type we expect for given mappings
+        """
+        self._initialize_mixed()
+        with self.store.default_store(default_ms):
+            self.store.create_course('org_x', 'course_y', 'run_z', self.user_id)
+            if reset_mixed_mappings:
+                self.store.mappings = {}
+            with self.assertRaises(DuplicateCourseError):
+                self.store.create_course('org_x', 'course_y', 'run_z', self.user_id)
+
     # split has one lookup for the course and then one for the course items
     @ddt.data(('draft', 1, 0), ('split', 2, 0))
     @ddt.unpack
@@ -308,7 +326,7 @@ class TestMixedModuleStore(unittest.TestCase):
 
         course_locn = self.course_locations[self.XML_COURSEID1]
         # NOTE: use get_course if you just want the course. get_items is expensive
-        modules = self.store.get_items(course_locn.course_key, category='course')
+        modules = self.store.get_items(course_locn.course_key, qualifiers={'category': 'course'})
         self.assertEqual(len(modules), 1)
         self.assertEqual(modules[0].location, course_locn)
 
@@ -316,7 +334,7 @@ class TestMixedModuleStore(unittest.TestCase):
         course_locn = self.course_locations[self.MONGO_COURSEID]
         with check_mongo_calls(mongo_store, max_find, max_send):
             # NOTE: use get_course if you just want the course. get_items is expensive
-            modules = self.store.get_items(course_locn.course_key, category='problem')
+            modules = self.store.get_items(course_locn.course_key, qualifiers={'category': 'problem'})
         self.assertEqual(len(modules), 6)
 
         # verify that an error is raised when the revision is not valid
@@ -368,7 +386,7 @@ class TestMixedModuleStore(unittest.TestCase):
         # Create dummy direct only xblocks
         chapter = self.store.create_item(
             self.user_id,
-            test_course.id.version_agnostic(),
+            test_course.id,
             'chapter',
             block_id='vertical_container'
         )
@@ -389,7 +407,7 @@ class TestMixedModuleStore(unittest.TestCase):
         # Create a dummy component to test against
         xblock = self.store.create_item(
             self.user_id,
-            test_course.id.version_agnostic(),
+            test_course.id,
             'vertical',
             block_id='test_vertical'
         )
@@ -504,7 +522,7 @@ class TestMixedModuleStore(unittest.TestCase):
                 revision=ModuleStoreEnum.RevisionOption.draft_preferred
             )
 
-        self.store.publish(private_vert.location.version_agnostic(), self.user_id)
+        self.store.publish(private_vert.location, self.user_id)
         private_leaf.display_name = 'change me'
         private_leaf = self.store.update_item(private_leaf, self.user_id)
         mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
@@ -512,7 +530,7 @@ class TestMixedModuleStore(unittest.TestCase):
         with check_mongo_calls(mongo_store, max_find, max_send):
             self.store.delete_item(private_leaf.location, self.user_id)
 
-    @ddt.data(('draft', 3, 0), ('split', 6, 0))
+    @ddt.data(('draft', 2, 0), ('split', 3, 0))
     @ddt.unpack
     def test_get_courses(self, default_ms, max_find, max_send):
         self.initdb(default_ms)
@@ -520,16 +538,19 @@ class TestMixedModuleStore(unittest.TestCase):
         mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
         with check_mongo_calls(mongo_store, max_find, max_send):
             courses = self.store.get_courses()
-        course_ids = [
-            course.location.version_agnostic()
-            if hasattr(course.location, 'version_agnostic') else course.location
-            for course in courses
-        ]
+        course_ids = [course.location for course in courses]
         self.assertEqual(len(courses), 3, "Not 3 courses: {}".format(course_ids))
         self.assertIn(self.course_locations[self.MONGO_COURSEID], course_ids)
         self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids)
         self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids)
 
+        with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred):
+            draft_courses = self.store.get_courses(remove_branch=True)
+        with self.store.branch_setting(ModuleStoreEnum.Branch.published_only):
+            published_courses = self.store.get_courses(remove_branch=True)
+        self.assertEquals([c.id for c in draft_courses], [c.id for c in published_courses])
+
+
     def test_xml_get_courses(self):
         """
         Test that the xml modulestore only loaded the courses from the maps.
@@ -604,7 +625,7 @@ class TestMixedModuleStore(unittest.TestCase):
         self._create_block_hierarchy()
 
         # publish the course
-        self.course = self.store.publish(self.course.location.version_agnostic(), self.user_id)
+        self.course = self.store.publish(self.course.location, self.user_id)
 
         # make drafts of verticals
         self.store.convert_to_draft(self.vertical_x1a, self.user_id)
@@ -630,7 +651,7 @@ class TestMixedModuleStore(unittest.TestCase):
         ])
 
         # publish the course again
-        self.store.publish(self.course.location.version_agnostic(), self.user_id)
+        self.store.publish(self.course.location, self.user_id)
         self.verify_get_parent_locations_results([
             (child_to_move_location, new_parent_location, None),
             (child_to_move_location, new_parent_location, ModuleStoreEnum.RevisionOption.draft_preferred),
@@ -870,7 +891,7 @@ class TestMixedModuleStore(unittest.TestCase):
         self.initdb(default_ms)
         block = self.store.create_item(
             self.user_id,
-            self.course.location.version_agnostic().course_key,
+            self.course.location.course_key,
             'problem'
         )
         self.assertEqual(self.user_id, block.edited_by)
@@ -881,7 +902,7 @@ class TestMixedModuleStore(unittest.TestCase):
         self.initdb(default_ms)
         block = self.store.create_item(
             self.user_id,
-            self.course.location.version_agnostic().course_key,
+            self.course.location.course_key,
             'problem'
         )
         self.assertEqual(self.user_id, block.subtree_edited_by)
@@ -926,7 +947,7 @@ class TestMixedModuleStore(unittest.TestCase):
         self._create_block_hierarchy()
 
         # publish
-        self.store.publish(self.course.location.version_agnostic(), self.user_id)
+        self.store.publish(self.course.location, self.user_id)
         published_xblock = self.store.get_item(
             self.vertical_x1a,
             revision=ModuleStoreEnum.RevisionOption.published_only
@@ -962,7 +983,7 @@ class TestMixedModuleStore(unittest.TestCase):
 
         # start off as Private
         item = self.store.create_child(self.user_id, self.writable_chapter_location, 'problem', 'test_compute_publish_state')
-        item_location = item.location.version_agnostic()
+        item_location = item.location
         mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
         with check_mongo_calls(mongo_store, max_find, max_send):
             self.assertEquals(self.store.compute_publish_state(item), PublishState.private)
@@ -1012,13 +1033,13 @@ class TestMixedModuleStore(unittest.TestCase):
         test_course = self.store.create_course('testx', 'GreekHero', 'test_run', self.user_id)
         self.assertEqual(self.store.compute_publish_state(test_course), PublishState.public)
 
-        test_course_key = test_course.id.version_agnostic()
+        test_course_key = test_course.id
 
         # test create_item of direct-only category to make sure we are autopublishing
         chapter = self.store.create_item(self.user_id, test_course_key, 'chapter', 'Overview')
         self.assertEqual(self.store.compute_publish_state(chapter), PublishState.public)
 
-        chapter_location = chapter.location.version_agnostic()
+        chapter_location = chapter.location
 
         # test create_child of direct-only category to make sure we are autopublishing
         sequential = self.store.create_child(self.user_id, chapter_location, 'sequential', 'Sequence')
@@ -1189,6 +1210,59 @@ class TestMixedModuleStore(unittest.TestCase):
             # there should be no published problems with the old name
             assertNumProblems(problem_original_name, 0)
 
+    def verify_default_store(self, store_type):
+        # verify default_store property
+        self.assertEquals(self.store.default_modulestore.get_modulestore_type(), store_type)
+
+        # verify internal helper method
+        store = self.store._get_modulestore_for_courseid()
+        self.assertEquals(store.get_modulestore_type(), store_type)
+
+        # verify store used for creating a course
+        try:
+            course = self.store.create_course("org", "course{}".format(uuid4().hex[:3]), "run", self.user_id)
+            self.assertEquals(course.system.modulestore.get_modulestore_type(), store_type)
+        except NotImplementedError:
+            self.assertEquals(store_type, ModuleStoreEnum.Type.xml)
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.xml)
+    def test_default_store(self, default_ms):
+        """
+        Test the default store context manager
+        """
+        # initialize the mixed modulestore
+        self._initialize_mixed()
+
+        with self.store.default_store(default_ms):
+            self.verify_default_store(default_ms)
+
+    def test_default_store_nested(self):
+        """
+        Test the default store context manager, nested within one another
+        """
+        # initialize the mixed modulestore
+        self._initialize_mixed()
+
+        with self.store.default_store(ModuleStoreEnum.Type.mongo):
+            self.verify_default_store(ModuleStoreEnum.Type.mongo)
+            with self.store.default_store(ModuleStoreEnum.Type.split):
+                self.verify_default_store(ModuleStoreEnum.Type.split)
+                with self.store.default_store(ModuleStoreEnum.Type.xml):
+                    self.verify_default_store(ModuleStoreEnum.Type.xml)
+                self.verify_default_store(ModuleStoreEnum.Type.split)
+            self.verify_default_store(ModuleStoreEnum.Type.mongo)
+
+    def test_default_store_fake(self):
+        """
+        Test the default store context manager, asking for a fake store
+        """
+        # initialize the mixed modulestore
+        self._initialize_mixed()
+
+        fake_store = "fake"
+        with self.assertRaisesRegexp(Exception, "Cannot find store of type {}".format(fake_store)):
+            with self.store.default_store(fake_store):
+                pass  # pragma: no cover
 
 #=============================================================================================================
 # General utils for not using django settings

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

@@ -45,7 +45,6 @@ class ModuleStoreSettingsMigration(TestCase):
             "ENGINE": "xmodule.modulestore.mixed.MixedModuleStore",
             "OPTIONS": {
                 "mappings": {},
-                "reference_type": "Location",
                 "stores": {
                     "an_old_mongo_store": {
                         "DOC_STORE_CONFIG": {},
@@ -77,15 +76,47 @@ class ModuleStoreSettingsMigration(TestCase):
         }
     }
 
+    ALREADY_UPDATED_MIXED_CONFIG = {
+        'default': {
+            'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
+            'OPTIONS': {
+                'mappings': {},
+                'stores': [
+                    {
+                        'NAME': 'split',
+                        'ENGINE': 'xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore',
+                        'DOC_STORE_CONFIG': {},
+                        'OPTIONS': {
+                            'default_class': 'xmodule.hidden_module.HiddenDescriptor',
+                            'fs_root': "fs_root",
+                            'render_template': 'edxmako.shortcuts.render_to_string',
+                        }
+                    },
+                    {
+                        'NAME': 'draft',
+                        'ENGINE': 'xmodule.modulestore.mongo.draft.DraftModuleStore',
+                        'DOC_STORE_CONFIG': {},
+                        'OPTIONS': {
+                            'default_class': 'xmodule.hidden_module.HiddenDescriptor',
+                            'fs_root': "fs_root",
+                            'render_template': 'edxmako.shortcuts.render_to_string',
+                        }
+                    },
+                ]
+            }
+        }
+    }
+
+
     def _get_mixed_stores(self, mixed_setting):
         """
-        Helper for accessing stores in a configuration setting for the Mixed modulestore
+        Helper for accessing stores in a configuration setting for the Mixed modulestore.
         """
         return mixed_setting["default"]["OPTIONS"]["stores"]
 
     def assertStoreValuesEqual(self, store_setting1, store_setting2):
         """
-        Tests whether the fields in the given store_settings are equal
+        Tests whether the fields in the given store_settings are equal.
         """
         store_fields = ["OPTIONS", "DOC_STORE_CONFIG"]
         for field in store_fields:
@@ -108,26 +139,56 @@ class ModuleStoreSettingsMigration(TestCase):
 
         return new_mixed_setting, new_stores[0]
 
+    def is_split_configured(self, mixed_setting):
+        """
+        Tests whether the split module store is configured in the given setting.
+        """
+        stores = self._get_mixed_stores(mixed_setting)
+        split_settings = [store for store in stores if store['ENGINE'].endswith('.DraftVersioningModuleStore')]
+        if len(split_settings):
+            # there should only be one setting for split
+            self.assertEquals(len(split_settings), 1)
+            # verify name
+            self.assertEquals(split_settings[0]['NAME'], 'split')
+            # verify split config settings equal those of mongo
+            self.assertStoreValuesEqual(
+                split_settings[0],
+                next((store for store in stores if 'DraftModuleStore' in store['ENGINE']), None)
+            )
+        return len(split_settings) > 0
+
     def test_convert_into_mixed(self):
         old_setting = self.OLD_CONFIG
-        _, new_default_store_setting = self.assertMigrated(old_setting)
+        new_mixed_setting, new_default_store_setting = self.assertMigrated(old_setting)
         self.assertStoreValuesEqual(new_default_store_setting, old_setting["default"])
         self.assertEqual(new_default_store_setting["ENGINE"], old_setting["default"]["ENGINE"])
+        self.assertFalse(self.is_split_configured(new_mixed_setting))
 
     def test_convert_from_old_mongo_to_draft_store(self):
         old_setting = self.OLD_CONFIG_WITH_DIRECT_MONGO
-        _, new_default_store_setting = self.assertMigrated(old_setting)
+        new_mixed_setting, new_default_store_setting = self.assertMigrated(old_setting)
         self.assertStoreValuesEqual(new_default_store_setting, old_setting["default"])
         self.assertEqual(new_default_store_setting["ENGINE"], "xmodule.modulestore.mongo.draft.DraftModuleStore")
+        self.assertTrue(self.is_split_configured(new_mixed_setting))
 
     def test_convert_from_dict_to_list(self):
         old_mixed_setting = self.OLD_MIXED_CONFIG_WITH_DICT
         new_mixed_setting, new_default_store_setting = self.assertMigrated(old_mixed_setting)
         self.assertEqual(new_default_store_setting["ENGINE"], "the_default_store")
+        self.assertTrue(self.is_split_configured(new_mixed_setting))
 
-        # compare each store configured in mixed
+        # exclude split when comparing old and new, since split was added as part of the migration
+        new_stores = [store for store in self._get_mixed_stores(new_mixed_setting) if store['NAME'] != 'split']
         old_stores = self._get_mixed_stores(self.OLD_MIXED_CONFIG_WITH_DICT)
-        new_stores = self._get_mixed_stores(new_mixed_setting)
+
+        # compare each store configured in mixed
         self.assertEqual(len(new_stores), len(old_stores))
-        for new_store_setting in self._get_mixed_stores(new_mixed_setting):
-            self.assertStoreValuesEqual(new_store_setting, old_stores[new_store_setting['NAME']])
+        for new_store in new_stores:
+            self.assertStoreValuesEqual(new_store, old_stores[new_store['NAME']])
+
+    def test_no_conversion(self):
+        # make sure there is no migration done on an already updated config
+        old_mixed_setting = self.ALREADY_UPDATED_MIXED_CONFIG
+        new_mixed_setting, new_default_store_setting = self.assertMigrated(old_mixed_setting)
+        self.assertTrue(self.is_split_configured(new_mixed_setting))
+        self.assertEquals(old_mixed_setting, new_mixed_setting)

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

@@ -151,7 +151,7 @@ class TestMigration(SplitWMongoCourseBoostrapper):
 
         # grab the detached items to compare they should be in both published and draft
         for category in ['conditional', 'about', 'course_info', 'static_tab']:
-            for conditional in presplit.get_items(self.old_course_key, category=category):
+            for conditional in presplit.get_items(self.old_course_key, qualifiers={'category': category}):
                 locator = new_course_key.make_usage_key(category, conditional.location.block_id)
                 self.compare_dags(presplit, conditional, self.split_mongo.get_item(locator), published)
 

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

@@ -895,18 +895,18 @@ class SplitModuleItemTests(SplitModuleTest):
         self.assertEqual(len(matches), 6)
         matches = modulestore().get_items(locator)
         self.assertEqual(len(matches), 6)
-        matches = modulestore().get_items(locator, category='chapter')
+        matches = modulestore().get_items(locator, qualifiers={'category': 'chapter'})
         self.assertEqual(len(matches), 3)
-        matches = modulestore().get_items(locator, category='garbage')
+        matches = modulestore().get_items(locator, qualifiers={'category': 'garbage'})
         self.assertEqual(len(matches), 0)
         matches = modulestore().get_items(
             locator,
-            category='chapter',
+            qualifiers={'category': 'chapter'},
             settings={'display_name': re.compile(r'Hera')},
         )
         self.assertEqual(len(matches), 2)
 
-        matches = modulestore().get_items(locator, children='chapter2')
+        matches = modulestore().get_items(locator, qualifiers={'children': 'chapter2'})
         self.assertEqual(len(matches), 1)
         self.assertEqual(matches[0].location.block_id, 'head12345')
 
@@ -1324,7 +1324,7 @@ class TestItemCrud(SplitModuleTest):
         reusable_location = course.id.version_agnostic().for_branch(BRANCH_NAME_DRAFT)
 
         # delete a leaf
-        problems = modulestore().get_items(reusable_location, category='problem')
+        problems = modulestore().get_items(reusable_location, qualifiers={'category': 'problem'})
         locn_to_del = problems[0].location
         new_course_loc = modulestore().delete_item(locn_to_del, self.user_id)
         deleted = locn_to_del.version_agnostic()
@@ -1336,7 +1336,7 @@ class TestItemCrud(SplitModuleTest):
         self.assertNotEqual(new_course_loc.version_guid, course.location.version_guid)
 
         # delete a subtree
-        nodes = modulestore().get_items(reusable_location, category='chapter')
+        nodes = modulestore().get_items(reusable_location, qualifiers={'category': 'chapter'})
         new_course_loc = modulestore().delete_item(nodes[0].location, self.user_id)
         # check subtree
 

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -24,6 +24,7 @@ from xmodule.modulestore import ModuleStoreEnum, ModuleStoreReadBase
 from xmodule.tabs import CourseTabList
 from opaque_keys.edx.keys import UsageKey
 from opaque_keys.edx.locations import SlashSeparatedCourseKey, Location
+from opaque_keys.edx.locator import CourseLocator
 
 from xblock.field_data import DictFieldData
 from xblock.runtime import DictKeyValueStore, IdGenerator
@@ -403,7 +404,6 @@ class XMLModuleStore(ModuleStoreReadBase):
             self.default_class = class_
 
         self.parent_trackers = defaultdict(ParentTracker)
-        self.reference_type = Location
 
         # All field data will be stored in an inheriting field data.
         self.field_data = inheriting_field_data(kvs=DictKeyValueStore())
@@ -700,7 +700,7 @@ class XMLModuleStore(ModuleStoreReadBase):
         """
         return usage_key in self.modules[usage_key.course_key]
 
-    def get_item(self, usage_key, depth=0):
+    def get_item(self, usage_key, depth=0, **kwargs):
         """
         Returns an XBlock instance for the item for this UsageKey.
 
@@ -717,7 +717,7 @@ class XMLModuleStore(ModuleStoreReadBase):
         except KeyError:
             raise ItemNotFoundError(usage_key)
 
-    def get_items(self, course_id, settings=None, content=None, revision=None, **kwargs):
+    def get_items(self, course_id, settings=None, content=None, revision=None, qualifiers=None, **kwargs):
         """
         Returns:
             list of XModuleDescriptor instances for the matching items within the course with
@@ -729,10 +729,10 @@ class XMLModuleStore(ModuleStoreReadBase):
         Args:
             course_id (CourseKey): the course identifier
             settings (dict): fields to look for which have settings scope. Follows same syntax
-                and rules as kwargs below
+                and rules as qualifiers below
             content (dict): fields to look for which have content scope. Follows same syntax and
-                rules as kwargs below.
-            kwargs (key=value): what to look for within the course.
+                rules as qualifiers below.
+            qualifiers (dict): what to look for within the course.
                 Common qualifiers are ``category`` or any field name. if the target field is a list,
                 then it searches for the given value in the list not list equivalence.
                 Substring matching pass a regex object.
@@ -747,8 +747,9 @@ class XMLModuleStore(ModuleStoreReadBase):
 
         items = []
 
-        category = kwargs.pop('category', None)
-        name = kwargs.pop('name', None)
+        qualifiers = qualifiers.copy() if qualifiers else {}  # copy the qualifiers (destructively manipulated here)
+        category = qualifiers.pop('category', None)
+        name = qualifiers.pop('name', None)
 
         def _block_matches_all(mod_loc, module):
             if category and mod_loc.category != category:
@@ -757,7 +758,7 @@ class XMLModuleStore(ModuleStoreReadBase):
                 return False
             return all(
                 self._block_matches(module, fields or {})
-                for fields in [settings, content, kwargs]
+                for fields in [settings, content, qualifiers]
             )
 
         for mod_loc, module in self.modules[course_id].iteritems():
@@ -766,7 +767,16 @@ class XMLModuleStore(ModuleStoreReadBase):
 
         return items
 
-    def get_courses(self, depth=0):
+    def make_course_key(self, org, course, run):
+        """
+        Return a valid :class:`~opaque_keys.edx.keys.CourseKey` for this modulestore
+        that matches the supplied `org`, `course`, and `run`.
+
+        This key may represent a course that doesn't exist in this modulestore.
+        """
+        return CourseLocator(org, course, run, deprecated=True)
+
+    def get_courses(self, depth=0, **kwargs):
         """
         Returns a list of course descriptors.  If there were errors on loading,
         some of these may be ErrorDescriptors instead.
@@ -780,7 +790,7 @@ class XMLModuleStore(ModuleStoreReadBase):
         """
         return dict((k, self.errored_courses[k].errors) for k in self.errored_courses)
 
-    def get_orphans(self, course_key):
+    def get_orphans(self, course_key, **kwargs):
         """
         Get all of the xblocks in the given course which have no parents and are not of types which are
         usually orphaned. NOTE: may include xblocks which still have references via xblocks which don't
@@ -806,7 +816,7 @@ class XMLModuleStore(ModuleStoreReadBase):
         """
         return ModuleStoreEnum.Type.xml
 
-    def get_courses_for_wiki(self, wiki_slug):
+    def get_courses_for_wiki(self, wiki_slug, **kwargs):
         """
         Return the list of courses which use this wiki_slug
         :param wiki_slug: the course wiki root slug

common/lib/xmodule/xmodule/modulestore/xml_exporter.py

@@ -106,7 +106,7 @@ def export_to_xml(modulestore, contentstore, course_key, root_dir, course_dir):
     # and index here since the XML modulestore cannot load draft modules
     draft_verticals = modulestore.get_items(
         course_key,
-        category='vertical',
+        qualifiers={'category': 'vertical'},
         revision=ModuleStoreEnum.RevisionOption.draft_only
     )
     if len(draft_verticals) > 0:
@@ -144,7 +144,7 @@ def _export_field_content(xblock_item, item_dir):
 
 
 def export_extra_content(export_fs, modulestore, course_key, category_type, dirname, file_suffix=''):
-    items = modulestore.get_items(course_key, category=category_type)
+    items = modulestore.get_items(course_key, qualifiers={'category': category_type})
 
     if len(items) > 0:
         item_dir = export_fs.makeopendir(dirname)

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

@@ -17,7 +17,7 @@ from .store_utilities import rewrite_nonportable_content_links
 import xblock
 from xmodule.tabs import CourseTabList
 from xmodule.modulestore.django import ASSET_IGNORE_REGEX
-from xmodule.modulestore.exceptions import InvalidLocationError
+from xmodule.modulestore.exceptions import DuplicateCourseError
 from xmodule.modulestore.mongo.base import MongoRevisionKey
 from xmodule.modulestore import ModuleStoreEnum
 
@@ -174,8 +174,9 @@ def import_from_xml(
             if create_new_course_if_not_present and not store.has_course(dest_course_id, ignore_case=True):
                 try:
                     store.create_course(dest_course_id.org, dest_course_id.course, dest_course_id.run, user_id)
-                except InvalidLocationError:
+                except DuplicateCourseError:
                     # course w/ same org and course exists
+                    # The Mongo modulestore checks *with* the run in has_course, but not in create_course.
                     log.debug(
                         "Skipping import of course with id, {0},"
                         "since it collides with an existing one".format(dest_course_id)

lms/djangoapps/courseware/tests/test_draft_modulestore.py

@@ -13,7 +13,7 @@ class TestDraftModuleStore(TestCase):
         store = modulestore()
 
         # fix was to allow get_items() to take the course_id parameter
-        store.get_items(SlashSeparatedCourseKey('a', 'b', 'c'), category='vertical')
+        store.get_items(SlashSeparatedCourseKey('a', 'b', 'c'), qualifiers={'category': 'vertical'})
 
         # test success is just getting through the above statement.
         # The bug was that 'course_id' argument was

lms/djangoapps/courseware/tests/tests.py

@@ -163,7 +163,7 @@ class TestDraftModuleStore(ModuleStoreTestCase):
         store = modulestore()
 
         # fix was to allow get_items() to take the course_id parameter
-        store.get_items(SlashSeparatedCourseKey('abc', 'def', 'ghi'), category='vertical')
+        store.get_items(SlashSeparatedCourseKey('abc', 'def', 'ghi'), qualifiers={'category': 'vertical'})
 
         # test success is just getting through the above statement.
         # The bug was that 'course_id' argument was

lms/djangoapps/courseware/views.py

@@ -463,7 +463,7 @@ def jump_to_id(request, course_id, module_id):
     passed in. This assumes that id is unique within the course_id namespace
     """
     course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
-    items = modulestore().get_items(course_key, name=module_id)
+    items = modulestore().get_items(course_key, qualifiers={'name': module_id})
 
     if len(items) == 0:
         raise Http404(
@@ -937,7 +937,7 @@ def get_course_lti_endpoints(request, course_id):
 
     anonymous_user = AnonymousUser()
     anonymous_user.known = False  # make these "noauth" requests like module_render.handle_xblock_callback_noauth
-    lti_descriptors = modulestore().get_items(course.id, category='lti')
+    lti_descriptors = modulestore().get_items(course.id, qualifiers={'category': 'lti'})
 
     lti_noauth_modules = [
         get_module_for_descriptor(

lms/djangoapps/django_comment_client/utils.py

@@ -57,7 +57,7 @@ def has_forum_access(uname, course_id, rolename):
 
 
 def _get_discussion_modules(course):
-    all_modules = modulestore().get_items(course.id, category='discussion')
+    all_modules = modulestore().get_items(course.id, qualifiers={'category': 'discussion'})
 
     def has_required_keys(module):
         for key in ('discussion_id', 'discussion_category', 'discussion_target'):

lms/djangoapps/open_ended_grading/views.py

@@ -92,7 +92,7 @@ def find_peer_grading_module(course):
     problem_url = ""
 
     # Get the peer grading modules currently in the course.  Explicitly specify the course id to avoid issues with different runs.
-    items = modulestore().get_items(course.id, category='peergrading')
+    items = modulestore().get_items(course.id, qualifiers={'category': 'peergrading'})
     # See if any of the modules are centralized modules (ie display info from multiple problems)
     items = [i for i in items if not getattr(i, "use_for_single_location", True)]
     # Loop through all potential peer grading modules, and find the first one that has a path to it.

lms/envs/bok_choy.auth.json

@@ -51,7 +51,6 @@
             "ENGINE": "xmodule.modulestore.mixed.MixedModuleStore",
             "OPTIONS": {
                 "mappings": {},
-                "reference_type": "Location",
                 "stores": [
                     {
                         "NAME": "draft",

lms/envs/common.py

@@ -504,7 +504,6 @@ MODULESTORE = {
         'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore',
         'OPTIONS': {
             'mappings': {},
-            'reference_type': 'Location',
             'stores': [
                 {
                     'NAME': 'draft',
@@ -524,6 +523,16 @@ MODULESTORE = {
                         'default_class': 'xmodule.hidden_module.HiddenDescriptor',
                     }
                 },
+                {
+                    'NAME': 'split',
+                    'ENGINE': 'xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore',
+                    'DOC_STORE_CONFIG': DOC_STORE_CONFIG,
+                    'OPTIONS': {
+                        'default_class': 'xmodule.hidden_module.HiddenDescriptor',
+                        'fs_root': DATA_DIR,
+                        'render_template': 'edxmako.shortcuts.render_to_string',
+                    }
+                },
             ]
         }
     }