Merge pull request #4977 from edx/split/paver

Split/paver LMS-11211

cms/djangoapps/contentstore/views/access.py

@@ -20,7 +20,8 @@ def has_course_access(user, course_key, role=CourseStaffRole):
         return True
     if OrgStaffRole(org=course_key.org).has_user(user):
         return True
-    return auth.has_access(user, role(course_key))
+    # temporary to ensure we give universal access given a course until we impl branch specific perms
+    return auth.has_access(user, role(course_key.for_branch(None)))
 
 
 def get_user_role(user, course_id):

cms/djangoapps/contentstore/views/course.py

@@ -567,7 +567,11 @@ def _create_new_course(request, org, number, run, fields):
     fields.update(definition_data)
 
     store = modulestore()
-    with store.default_store(settings.FEATURES.get('DEFAULT_STORE_FOR_NEW_COURSE', 'mongo')):
+    store_for_new_course = (
+        settings.FEATURES.get('DEFAULT_STORE_FOR_NEW_COURSE') or
+        store.default_modulestore.get_modulestore_type()
+    )
+    with store.default_store(store_for_new_course):
         # Creating the course raises DuplicateCourseError if an existing course with this org/name is found
         new_course = store.create_course(
             org,
@@ -584,7 +588,8 @@ def _create_new_course(request, org, number, run, fields):
     initialize_permissions(new_course.id, request.user)
 
     return JsonResponse({
-        'url': reverse_course_url('course_handler', new_course.id)
+        'url': reverse_course_url('course_handler', new_course.id),
+        'course_key': unicode(new_course.id),
     })
 
 

cms/djangoapps/contentstore/views/item.py

@@ -154,7 +154,7 @@ def xblock_handler(request, usage_key_string):
                 request.user,
                 _get_xblock(usage_key, request.user),
                 data=request.json.get('data'),
-                children=request.json.get('children'),
+                children_strings=request.json.get('children'),
                 metadata=request.json.get('metadata'),
                 nullout=request.json.get('nullout'),
                 grader_type=request.json.get('graderType'),
@@ -301,7 +301,23 @@ def xblock_outline_handler(request, usage_key_string):
         return Http404
 
 
-def _save_xblock(user, xblock, data=None, children=None, metadata=None, nullout=None,
+def _update_with_callback(xblock, user, old_metadata=None, old_content=None):
+    """
+    Updates the xblock in the modulestore.
+    But before doing so, it calls the xblock's editor_saved callback function.
+    """
+    if callable(getattr(xblock, "editor_saved", None)):
+        if old_metadata is None:
+            old_metadata = own_metadata(xblock)
+        if old_content is None:
+            old_content = xblock.get_explicitly_set_fields_by_scope(Scope.content)
+        xblock.editor_saved(user, old_metadata, old_content)
+
+    # Update after the callback so any changes made in the callback will get persisted.
+    modulestore().update_item(xblock, user.id)
+
+
+def _save_xblock(user, xblock, data=None, children_strings=None, metadata=None, nullout=None,
                  grader_type=None, publish=None):
     """
     Saves xblock w/ its fields. Has special processing for grader_type, publish, and nullout and Nones in metadata.
@@ -309,94 +325,127 @@ def _save_xblock(user, xblock, data=None, children=None, metadata=None, nullout=
     to default).
     """
     store = modulestore()
+    # Perform all xblock changes within a (single-versioned) transaction
+    with store.bulk_operations(xblock.location.course_key):
 
-    # Don't allow updating an xblock and discarding changes in a single operation (unsupported by UI).
+        # Don't allow updating an xblock and discarding changes in a single operation (unsupported by UI).
-    if publish == "discard_changes":
+        if publish == "discard_changes":
-        store.revert_to_published(xblock.location, user.id)
+            store.revert_to_published(xblock.location, user.id)
-        # Returning the same sort of result that we do for other save operations. In the future,
+            # Returning the same sort of result that we do for other save operations. In the future,
-        # we may want to return the full XBlockInfo.
+            # we may want to return the full XBlockInfo.
-        return JsonResponse({'id': unicode(xblock.location)})
+            return JsonResponse({'id': unicode(xblock.location)})
 
-    old_metadata = own_metadata(xblock)
+        old_metadata = own_metadata(xblock)
-    old_content = xblock.get_explicitly_set_fields_by_scope(Scope.content)
+        old_content = xblock.get_explicitly_set_fields_by_scope(Scope.content)
 
-    if data:
+        if data:
-        # TODO Allow any scope.content fields not just "data" (exactly like the get below this)
+            # TODO Allow any scope.content fields not just "data" (exactly like the get below this)
-        xblock.data = data
+            xblock.data = data
-    else:
+        else:
-        data = old_content['data'] if 'data' in old_content else None
+            data = old_content['data'] if 'data' in old_content else None
 
-    if children is not None:
+        if children_strings is not None:
-        children_usage_keys = []
+            children = []
-        for child in children:
+            for child_string in children_strings:
-            child_usage_key = usage_key_with_run(child)
+                children.append(usage_key_with_run(child_string))
-            children_usage_keys.append(child_usage_key)
-        xblock.children = children_usage_keys
 
-    # also commit any metadata which might have been passed along
+            # if new children have been added, remove them from their old parents
-    if nullout is not None or metadata is not None:
+            new_children = set(children) - set(xblock.children)
-        # the postback is not the complete metadata, as there's system metadata which is
+            for new_child in new_children:
-        # not presented to the end-user for editing. So let's use the original (existing_item) and
+                old_parent_location = store.get_parent_location(new_child)
-        # 'apply' the submitted metadata, so we don't end up deleting system metadata.
+                if old_parent_location:
-        if nullout is not None:
+                    old_parent = store.get_item(old_parent_location)
-            for metadata_key in nullout:
+                    old_parent.children.remove(new_child)
-                setattr(xblock, metadata_key, None)
+                    _update_with_callback(old_parent, user)
-
-        # update existing metadata with submitted metadata (which can be partial)
-        # IMPORTANT NOTE: if the client passed 'null' (None) for a piece of metadata that means 'remove it'. If
-        # the intent is to make it None, use the nullout field
-        if metadata is not None:
-            for metadata_key, value in metadata.items():
-                field = xblock.fields[metadata_key]
-
-                if value is None:
-                    field.delete_from(xblock)
                 else:
-                    try:
+                    # the Studio UI currently doesn't present orphaned children, so assume this is an error
-                        value = field.from_json(value)
+                    return JsonResponse({"error": "Invalid data, possibly caused by concurrent authors."}, 400)
-                    except ValueError:
-                        return JsonResponse({"error": "Invalid data"}, 400)
-                    field.write_to(xblock, value)
 
-    if callable(getattr(xblock, "editor_saved", None)):
+            # make sure there are no old children that became orphans
-        xblock.editor_saved(user, old_metadata, old_content)
+            # In a single-author (no-conflict) scenario, all children in the persisted list on the server should be
+            # present in the updated list.  If there are any children that have been dropped as part of this update,
+            # then that would be an error.
+            #
+            # We can be even more restrictive in a multi-author (conflict), by returning an error whenever
+            # len(old_children) > 0. However, that conflict can still be "merged" if the dropped child had been
+            # re-parented. Hence, the check for the parent in the any statement below.
+            #
+            # Note that this multi-author conflict error should not occur in modulestores (such as Split) that support
+            # atomic write transactions.  In Split, if there was another author who moved one of the "old_children"
+            # into another parent, then that child would have been deleted from this parent on the server. However,
+            # this is error could occur in modulestores (such as Draft) that do not support atomic write-transactions
+            old_children = set(xblock.children) - set(children)
+            if any(
+                    store.get_parent_location(old_child) == xblock.location
+                    for old_child in old_children
+            ):
+                # since children are moved as part of a single transaction, orphans should not be created
+                return JsonResponse({"error": "Invalid data, possibly caused by concurrent authors."}, 400)
 
-    # commit to datastore
+            # set the children on the xblock
-    store.update_item(xblock, user.id)
+            xblock.children = children
 
-    # for static tabs, their containing course also records their display name
+        # also commit any metadata which might have been passed along
-    if xblock.location.category == 'static_tab':
+        if nullout is not None or metadata is not None:
-        course = store.get_course(xblock.location.course_key)
+            # the postback is not the complete metadata, as there's system metadata which is
-        # find the course's reference to this tab and update the name.
+            # not presented to the end-user for editing. So let's use the original (existing_item) and
-        static_tab = CourseTabList.get_tab_by_slug(course.tabs, xblock.location.name)
+            # 'apply' the submitted metadata, so we don't end up deleting system metadata.
-        # only update if changed
+            if nullout is not None:
-        if static_tab and static_tab['name'] != xblock.display_name:
+                for metadata_key in nullout:
-            static_tab['name'] = xblock.display_name
+                    setattr(xblock, metadata_key, None)
-            store.update_item(course, user.id)
 
-    result = {
+            # update existing metadata with submitted metadata (which can be partial)
-        'id': unicode(xblock.location),
+            # IMPORTANT NOTE: if the client passed 'null' (None) for a piece of metadata that means 'remove it'. If
-        'data': data,
+            # the intent is to make it None, use the nullout field
-        'metadata': own_metadata(xblock)
+            if metadata is not None:
-    }
+                for metadata_key, value in metadata.items():
+                    field = xblock.fields[metadata_key]
 
-    if grader_type is not None:
+                    if value is None:
-        result.update(CourseGradingModel.update_section_grader_type(xblock, grader_type, user))
+                        field.delete_from(xblock)
+                    else:
+                        try:
+                            value = field.from_json(value)
+                        except ValueError:
+                            return JsonResponse({"error": "Invalid data"}, 400)
+                        field.write_to(xblock, value)
 
-    # If publish is set to 'republish' and this item is not in direct only categories and has previously been published,
+        # update the xblock and call any xblock callbacks
-    # then this item should be republished. This is used by staff locking to ensure that changing the draft
+        _update_with_callback(xblock, user, old_metadata, old_content)
-    # value of the staff lock will also update the published version, but only at the unit level.
-    if publish == 'republish' and xblock.category not in DIRECT_ONLY_CATEGORIES:
-        if modulestore().has_published_version(xblock):
-            publish = 'make_public'
 
-    # Make public after updating the xblock, in case the caller asked for both an update and a publish.
+        # for static tabs, their containing course also records their display name
-    # Used by Bok Choy tests and by republishing of staff locks.
+        if xblock.location.category == 'static_tab':
-    if publish == 'make_public':
+            course = store.get_course(xblock.location.course_key)
-        modulestore().publish(xblock.location, user.id)
+            # find the course's reference to this tab and update the name.
+            static_tab = CourseTabList.get_tab_by_slug(course.tabs, xblock.location.name)
+            # only update if changed
+            if static_tab and static_tab['name'] != xblock.display_name:
+                static_tab['name'] = xblock.display_name
+                store.update_item(course, user.id)
 
-    # Note that children aren't being returned until we have a use case.
+        result = {
-    return JsonResponse(result, encoder=EdxJSONEncoder)
+            'id': unicode(xblock.location),
+            'data': data,
+            'metadata': own_metadata(xblock)
+        }
+
+        if grader_type is not None:
+            result.update(CourseGradingModel.update_section_grader_type(xblock, grader_type, user))
+
+        # If publish is set to 'republish' and this item is not in direct only categories and has previously been published,
+        # then this item should be republished. This is used by staff locking to ensure that changing the draft
+        # value of the staff lock will also update the published version, but only at the unit level.
+        if publish == 'republish' and xblock.category not in DIRECT_ONLY_CATEGORIES:
+            if modulestore().has_published_version(xblock):
+                publish = 'make_public'
+
+        # Make public after updating the xblock, in case the caller asked for both an update and a publish.
+        # Used by Bok Choy tests and by republishing of staff locks.
+        if publish == 'make_public':
+            modulestore().publish(xblock.location, user.id)
+
+        # Note that children aren't being returned until we have a use case.
+        return JsonResponse(result, encoder=EdxJSONEncoder)
 
 
 @login_required

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

@@ -480,10 +480,16 @@ class TestEditItem(ItemTest):
         display_name = 'chapter created'
         resp = self.create_xblock(display_name=display_name, category='chapter')
         chap_usage_key = self.response_usage_key(resp)
+
+        # create 2 sequentials
         resp = self.create_xblock(parent_usage_key=chap_usage_key, category='sequential')
         self.seq_usage_key = self.response_usage_key(resp)
         self.seq_update_url = reverse_usage_url("xblock_handler", self.seq_usage_key)
 
+        resp = self.create_xblock(parent_usage_key=chap_usage_key, category='sequential')
+        self.seq2_usage_key = self.response_usage_key(resp)
+        self.seq2_update_url = reverse_usage_url("xblock_handler", self.seq2_usage_key)
+
         # create problem w/ boilerplate
         template_id = 'multiplechoice.yaml'
         resp = self.create_xblock(parent_usage_key=self.seq_usage_key, category='problem', boilerplate=template_id)
@@ -557,11 +563,8 @@ class TestEditItem(ItemTest):
         self.assertIn(chapter2_usage_key, course.children)
 
         # Remove one child from the course.
-        resp = self.client.ajax_post(
+        resp = self.client.delete(reverse_usage_url("xblock_handler", chapter1_usage_key))
-            self.course_update_url,
+        self.assertEqual(resp.status_code, 204)
-            data={'children': [unicode(chapter2_usage_key)]}
-        )
-        self.assertEqual(resp.status_code, 200)
 
         # Verify that the child is removed.
         course = self.get_item_from_modulestore(self.usage_key)
@@ -597,6 +600,79 @@ class TestEditItem(ItemTest):
         self.assertEqual(unit1_usage_key, children[2])
         self.assertEqual(unit2_usage_key, children[1])
 
+    def test_move_parented_child(self):
+        """
+        Test moving a child from one Section to another
+        """
+        unit_1_key = self.response_usage_key(
+            self.create_xblock(parent_usage_key=self.seq_usage_key, category='vertical', display_name='unit 1')
+        )
+        unit_2_key = self.response_usage_key(
+            self.create_xblock(parent_usage_key=self.seq2_usage_key, category='vertical', display_name='unit 2')
+        )
+
+        # move unit 1 from sequential1 to sequential2
+        resp = self.client.ajax_post(
+            self.seq2_update_url,
+            data={'children': [unicode(unit_1_key), unicode(unit_2_key)]}
+        )
+        self.assertEqual(resp.status_code, 200)
+
+        # verify children
+        self.assertListEqual(
+            self.get_item_from_modulestore(self.seq2_usage_key).children,
+            [unit_1_key, unit_2_key],
+        )
+        self.assertListEqual(
+            self.get_item_from_modulestore(self.seq_usage_key).children,
+            [self.problem_usage_key],  # problem child created in setUp
+        )
+
+    def test_move_orphaned_child_error(self):
+        """
+        Test moving an orphan returns an error
+        """
+        unit_1_key = self.store.create_item(self.user.id, self.course_key, 'vertical', 'unit1').location
+
+        # adding orphaned unit 1 should return an error
+        resp = self.client.ajax_post(
+            self.seq2_update_url,
+            data={'children': [unicode(unit_1_key)]}
+        )
+        self.assertEqual(resp.status_code, 400)
+        self.assertIn("Invalid data, possibly caused by concurrent authors", resp.content)
+
+        # verify children
+        self.assertListEqual(
+            self.get_item_from_modulestore(self.seq2_usage_key).children,
+            []
+        )
+
+    def test_move_child_creates_orphan_error(self):
+        """
+        Test creating an orphan returns an error
+        """
+        unit_1_key = self.response_usage_key(
+            self.create_xblock(parent_usage_key=self.seq2_usage_key, category='vertical', display_name='unit 1')
+        )
+        unit_2_key = self.response_usage_key(
+            self.create_xblock(parent_usage_key=self.seq2_usage_key, category='vertical', display_name='unit 2')
+        )
+
+        # remove unit 2 should return an error
+        resp = self.client.ajax_post(
+            self.seq2_update_url,
+            data={'children': [unicode(unit_1_key)]}
+        )
+        self.assertEqual(resp.status_code, 400)
+        self.assertIn("Invalid data, possibly caused by concurrent authors", resp.content)
+
+        # verify children
+        self.assertListEqual(
+            self.get_item_from_modulestore(self.seq2_usage_key).children,
+            [unit_1_key, unit_2_key]
+        )
+
     def _is_location_published(self, location):
         """
         Returns whether or not the item with given location has a published version.
@@ -954,44 +1030,6 @@ class TestEditSplitModule(ItemTest):
         self.assertEqual(2, len(split_test.children))
         self.assertEqual(initial_group_id_to_child, split_test.group_id_to_child)
 
-    def test_delete_children(self):
-        """
-        Test that deleting a child in the group_id_to_child map updates the map.
-
-        Also test that deleting a child not in the group_id_to_child_map behaves properly.
-        """
-        # Set to first group configuration.
-        self._update_partition_id(0)
-        split_test = self._assert_children(2)
-        vertical_1_usage_key = split_test.children[1]
-
-        # Add an extra child to the split_test
-        resp = self.create_xblock(category='html', parent_usage_key=self.split_test_usage_key)
-        extra_child_usage_key = self.response_usage_key(resp)
-        self._assert_children(3)
-
-        # Remove the first child (which is part of the group configuration).
-        resp = self.client.ajax_post(
-            self.split_test_update_url,
-            data={'children': [unicode(vertical_1_usage_key), unicode(extra_child_usage_key)]}
-        )
-        self.assertEqual(resp.status_code, 200)
-        split_test = self._assert_children(2)
-
-        # Check that group_id_to_child was updated appropriately
-        group_id_to_child = split_test.group_id_to_child
-        self.assertEqual(1, len(group_id_to_child))
-        self.assertEqual(vertical_1_usage_key, group_id_to_child['1'])
-
-        # Remove the "extra" child and make sure that group_id_to_child did not change.
-        resp = self.client.ajax_post(
-            self.split_test_update_url,
-            data={'children': [unicode(vertical_1_usage_key)]}
-        )
-        self.assertEqual(resp.status_code, 200)
-        split_test = self._assert_children(1)
-        self.assertEqual(group_id_to_child, split_test.group_id_to_child)
-
     def test_add_groups(self):
         """
         Test the "fix up behavior" when groups are missing (after a group is added to a group configuration).

cms/envs/acceptance.py

@@ -50,7 +50,8 @@ update_module_store_settings(
     module_store_options={
         'default_class': 'xmodule.raw_module.RawDescriptor',
         'fs_root': TEST_ROOT / "data",
-    }
+    },
+    default_store=os.environ.get('DEFAULT_STORE', 'draft'),
 )
 
 CONTENTSTORE = {

cms/envs/bok_choy.py

@@ -36,6 +36,7 @@ update_module_store_settings(
     xml_store_options={
         'data_dir': (TEST_ROOT / "data").abspath(),
     },
+    default_store=os.environ.get('DEFAULT_STORE', 'draft'),
 )
 
 # Enable django-pipeline and staticfiles

cms/envs/common.py

@@ -106,7 +106,7 @@ FEATURES = {
     'ADVANCED_SECURITY': False,
 
     # Modulestore to use for new courses
-    'DEFAULT_STORE_FOR_NEW_COURSE': 'mongo',
+    'DEFAULT_STORE_FOR_NEW_COURSE': None,
 }
 ENABLE_JASMINE = False
 

cms/static/js/spec/utils/drag_and_drop_spec.js

@@ -323,18 +323,15 @@ define(["js/utils/drag_and_drop", "js/views/feedback_notification", "js/spec_hel
                     }, null, {
                         clientX: $('#unit-1').offset().left
                     });
-                    expect(requests.length).toEqual(2);
+                    expect(requests.length).toEqual(1);
                     expect(this.savingSpies.constructor).toHaveBeenCalled();
                     expect(this.savingSpies.show).toHaveBeenCalled();
                     expect(this.savingSpies.hide).not.toHaveBeenCalled();
                     savingOptions = this.savingSpies.constructor.mostRecentCall.args[0];
                     expect(savingOptions.title).toMatch(/Saving/);
                     expect($('#unit-1')).toHaveClass('was-dropped');
-                    expect(requests[0].requestBody).toEqual('{"children":["second-unit-id","third-unit-id"]}');
+                    expect(requests[0].requestBody).toEqual('{"children":["fourth-unit-id","first-unit-id"]}');
                     requests[0].respond(200);
-                    expect(this.savingSpies.hide).not.toHaveBeenCalled();
-                    expect(requests[1].requestBody).toEqual('{"children":["fourth-unit-id","first-unit-id"]}');
-                    requests[1].respond(200);
                     expect(this.savingSpies.hide).toHaveBeenCalled();
                     this.clock.tick(1001);
                     expect($('#unit-1')).not.toHaveClass('was-dropped');

cms/static/js/utils/drag_and_drop.js

@@ -288,17 +288,6 @@ define(["jquery", "jquery.ui", "underscore", "gettext", "js/views/feedback_notif
                     if (_.isFunction(refresh)) { refresh(collapsed); }
 
                 };
-                // If the parent has changed, update the children of the old parent.
-                if (newParentLocator !== oldParentLocator) {
-                    // Find the old parent element.
-                    oldParentEle = $(parentSelector).filter(function () {
-                        return $(this).data('locator') === oldParentLocator;
-                    });
-                    this.saveItem(oldParentEle, childrenSelector, function () {
-                        element.data('parent', newParentLocator);
-                        refreshParent(oldParentEle);
-                    });
-                }
                 saving = new NotificationView.Mini({
                     title: gettext('Saving…')
                 });
@@ -310,7 +299,18 @@ define(["jquery", "jquery.ui", "underscore", "gettext", "js/views/feedback_notif
                 }, 1000);
                 this.saveItem(newParentEle, childrenSelector, function () {
                     saving.hide();
+
+                    // Refresh new parent.
                     refreshParent(newParentEle);
+
+                    // Refresh old parent.
+                    if (newParentLocator !== oldParentLocator) {
+                        oldParentEle = $(parentSelector).filter(function () {
+                            return $(this).data('locator') === oldParentLocator;
+                        });
+                        refreshParent(oldParentEle);
+                        element.data('parent', newParentLocator);
+                    }
                 });
             },
 

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

@@ -34,6 +34,7 @@ def convert_module_store_setting_if_needed(module_store_setting):
     if module_store_setting is None:
         return None
 
+    # Convert to Mixed, if needed
     if module_store_setting['default']['ENGINE'] != 'xmodule.modulestore.mixed.MixedModuleStore':
         warnings.warn("Direct access to a modulestore is deprecated. Please use MixedModuleStore.", DeprecationWarning)
 
@@ -54,7 +55,8 @@ def convert_module_store_setting_if_needed(module_store_setting):
         )
         module_store_setting = new_module_store_setting
 
-    elif isinstance(module_store_setting['default']['OPTIONS']['stores'], dict):
+    # Convert from dict, if needed
+    elif isinstance(get_mixed_stores(module_store_setting), dict):
         warnings.warn(
             "Using a dict for the Stores option in the MixedModuleStore is deprecated.  Please use a list instead.",
             DeprecationWarning
@@ -62,13 +64,13 @@ def convert_module_store_setting_if_needed(module_store_setting):
 
         # convert old-style (unordered) dict to (an ordered) list
         module_store_setting['default']['OPTIONS']['stores'] = convert_old_stores_into_list(
-            module_store_setting['default']['OPTIONS']['stores']
+            get_mixed_stores(module_store_setting)
         )
+        assert isinstance(get_mixed_stores(module_store_setting), list)
 
-        assert isinstance(module_store_setting['default']['OPTIONS']['stores'], list)
+    # Add Split, if needed
-
     # 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']
+    mixed_stores = get_mixed_stores(module_store_setting)
     is_split_defined = any((store['ENGINE'].endswith('.DraftVersioningModuleStore')) for store in mixed_stores)
     if not is_split_defined:
         # find first setting of mongo store
@@ -95,10 +97,14 @@ def update_module_store_settings(
         doc_store_settings=None,
         module_store_options=None,
         xml_store_options=None,
+        default_store=None,
 ):
     """
     Updates the settings for each store defined in the given module_store_setting settings
     with the given doc store configuration and options, overwriting existing keys.
+
+    If default_store is specified, the given default store is moved to the top of the
+    list of stores.
     """
     for store in module_store_setting['default']['OPTIONS']['stores']:
         if store['NAME'] == 'xml':
@@ -106,3 +112,20 @@ def update_module_store_settings(
         else:
             module_store_options and store['OPTIONS'].update(module_store_options)
             doc_store_settings and store['DOC_STORE_CONFIG'].update(doc_store_settings)
+
+    if default_store:
+        mixed_stores = get_mixed_stores(module_store_setting)
+        for store in mixed_stores:
+            if store['NAME'] == default_store:
+                # move the found store to the top of the list
+                mixed_stores.remove(store)
+                mixed_stores.insert(0, store)
+                return
+        raise Exception("Could not find setting for requested default store: {}".format(default_store))
+
+
+def get_mixed_stores(mixed_setting):
+    """
+    Helper for accessing stores in a configuration setting for the Mixed modulestore.
+    """
+    return mixed_setting["default"]["OPTIONS"]["stores"]

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

@@ -2101,7 +2101,8 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
 
     def _serialize_fields(self, category, fields):
         """
-        Convert any references to their serialized form.
+        Convert any references to their serialized form. Handle some references already being unicoded
+        because the client passed them that way and nothing above this layer did the necessary deserialization.
 
         Remove any fields which split or its kvs computes or adds but does not want persisted.
 
@@ -2111,17 +2112,26 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         xblock_class = XBlock.load_class(category, self.default_class)
         xblock_class = self.mixologist.mix(xblock_class)
 
+        def reference_block_id(reference):
+            """
+            Handle client possibly setting field to strings rather than keys to get the block_id
+            """
+            # perhaps replace by fixing the views or Field Reference*.from_json to return a Key
+            if isinstance(reference, basestring):
+                reference = BlockUsageLocator.from_string(reference)
+            return BlockKey.from_usage_key(reference)
+
         for field_name, value in fields.iteritems():
             if value is not None:
                 if isinstance(xblock_class.fields[field_name], Reference):
-                    fields[field_name] = BlockKey.from_usage_key(value)
+                    fields[field_name] = reference_block_id(value)
                 elif isinstance(xblock_class.fields[field_name], ReferenceList):
                     fields[field_name] = [
-                        BlockKey.from_usage_key(ele) for ele in value
+                        reference_block_id(ele) for ele in value
                     ]
                 elif isinstance(xblock_class.fields[field_name], ReferenceValueDict):
                     for key, subvalue in value.iteritems():
-                        value[key] = BlockKey.from_usage_key(subvalue)
+                        value[key] = reference_block_id(subvalue)
                 # should this recurse down dicts and lists just in case they contain datetime?
                 elif not isinstance(value, datetime.datetime):  # don't convert datetimes!
                     fields[field_name] = xblock_class.fields[field_name].to_json(value)

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

@@ -2,10 +2,16 @@
 Tests for testing the modulestore settings migration code.
 """
 import copy
+import ddt
 from unittest import TestCase
-from xmodule.modulestore.modulestore_settings import convert_module_store_setting_if_needed
+from xmodule.modulestore.modulestore_settings import (
+    convert_module_store_setting_if_needed,
+    update_module_store_settings,
+    get_mixed_stores,
+)
 
 
+@ddt.ddt
 class ModuleStoreSettingsMigration(TestCase):
     """
     Tests for the migration code for the module store settings
@@ -108,12 +114,6 @@ class ModuleStoreSettingsMigration(TestCase):
     }
 
 
-    def _get_mixed_stores(self, mixed_setting):
-        """
-        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.
@@ -134,7 +134,7 @@ class ModuleStoreSettingsMigration(TestCase):
         self.assertEqual(new_mixed_setting["default"]["ENGINE"], "xmodule.modulestore.mixed.MixedModuleStore")
 
         # check whether the stores are in an ordered list
-        new_stores = self._get_mixed_stores(new_mixed_setting)
+        new_stores = get_mixed_stores(new_mixed_setting)
         self.assertIsInstance(new_stores, list)
 
         return new_mixed_setting, new_stores[0]
@@ -143,7 +143,7 @@ class ModuleStoreSettingsMigration(TestCase):
         """
         Tests whether the split module store is configured in the given setting.
         """
-        stores = self._get_mixed_stores(mixed_setting)
+        stores = 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
@@ -178,8 +178,8 @@ class ModuleStoreSettingsMigration(TestCase):
         self.assertTrue(self.is_split_configured(new_mixed_setting))
 
         # 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']
+        new_stores = [store for store in get_mixed_stores(new_mixed_setting) if store['NAME'] != 'split']
-        old_stores = self._get_mixed_stores(self.OLD_MIXED_CONFIG_WITH_DICT)
+        old_stores = get_mixed_stores(self.OLD_MIXED_CONFIG_WITH_DICT)
 
         # compare each store configured in mixed
         self.assertEqual(len(new_stores), len(old_stores))
@@ -192,3 +192,14 @@ class ModuleStoreSettingsMigration(TestCase):
         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)
+
+    @ddt.data('draft', 'split')
+    def test_update_settings(self, default_store):
+        mixed_setting = self.ALREADY_UPDATED_MIXED_CONFIG
+        update_module_store_settings(mixed_setting, default_store=default_store)
+        self.assertTrue(get_mixed_stores(mixed_setting)[0]['NAME'] == default_store)
+
+    def test_update_settings_error(self):
+        mixed_setting = self.ALREADY_UPDATED_MIXED_CONFIG
+        with self.assertRaises(Exception):
+            update_module_store_settings(mixed_setting, default_store='non-existent store')

common/test/acceptance/fixtures/course.py

@@ -11,6 +11,7 @@ from textwrap import dedent
 from collections import namedtuple
 from path import path
 from lazy import lazy
+from opaque_keys.edx.keys import CourseKey
 
 from . import STUDIO_BASE_URL
 
@@ -204,6 +205,7 @@ class CourseFixture(StudioApiFixture):
         self.children = []
         self._assets = []
         self._advanced_settings = {}
+        self._course_key = None
 
     def __str__(self):
         """
@@ -263,19 +265,17 @@ class CourseFixture(StudioApiFixture):
 
         return self
 
-    @property
-    def _course_key(self):
-        """
-        Return the locator string for the course.
-        """
-        return "{org}/{number}/{run}".format(**self._course_dict)
-
     @property
     def _course_location(self):
         """
         Return the locator string for the course.
         """
-        return "i4x://{org}/{number}/course/{run}".format(**self._course_dict)
+        course_key = CourseKey.from_string(self._course_key)
+        if getattr(course_key, 'deprecated', False):
+            block_id = self._course_dict['run']
+        else:
+            block_id = 'course'
+        return unicode(course_key.make_usage_key('course', block_id))
 
     @property
     def _assets_url(self):
@@ -289,7 +289,8 @@ class CourseFixture(StudioApiFixture):
         """
         Return the locator string for the course handouts
         """
-        return "i4x://{org}/{number}/course_info/handouts".format(**self._course_dict)
+        course_key = CourseKey.from_string(self._course_key)
+        return unicode(course_key.make_usage_key('course_info', 'handouts'))
 
     def _create_course(self):
         """
@@ -315,7 +316,9 @@ class CourseFixture(StudioApiFixture):
         if err is not None:
             raise CourseFixtureError("Could not create course {0}.  Error message: '{1}'".format(self, err))
 
-        if not response.ok:
+        if response.ok:
+            self._course_key = response.json()['course_key']
+        else:
             raise CourseFixtureError(
                 "Could not create course {0}.  Status was {1}".format(
                     self._course_dict, response.status_code))

common/test/acceptance/tests/helpers.py

@@ -5,8 +5,10 @@ import json
 import unittest
 import functools
 import requests
+import os
 from path import path
 from bok_choy.web_app_test import WebAppTest
+from opaque_keys.edx.locator import CourseLocator
 
 
 def skip_if_browser(browser):
@@ -170,8 +172,6 @@ class UniqueCourseTest(WebAppTest):
     Test that provides a unique course ID.
     """
 
-    COURSE_ID_SEPARATOR = "/"
-
     def __init__(self, *args, **kwargs):
         """
         Create a unique course ID.
@@ -190,11 +190,18 @@ class UniqueCourseTest(WebAppTest):
 
     @property
     def course_id(self):
-        return self.COURSE_ID_SEPARATOR.join([
+        """
+        Returns the serialized course_key for the test
+        """
+        # TODO - is there a better way to make this agnostic to the underlying default module store?
+        default_store = os.environ.get('DEFAULT_STORE', 'draft')
+        course_key = CourseLocator(
             self.course_info['org'],
             self.course_info['number'],
-            self.course_info['run']
+            self.course_info['run'],
-        ])
+            deprecated=(default_store == 'draft')
+        )
+        return unicode(course_key)
 
 
 class YouTubeConfigError(Exception):

docs/en_us/internal/testing.md

@@ -274,6 +274,14 @@ To put a debugging breakpoint in a test use:
 
     from nose.tools import set_trace; set_trace()
 
+By default, all bokchoy tests are run with the 'split' ModuleStore.
+To override the modulestore that is used, use the default_store option.  The currently supported stores are:
+    'split' (xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore) and
+    'draft' (xmodule.modulestore.mongo.DraftMongoModuleStore).
+For example:
+
+    paver test_bokchoy --default_store='draft'
+
 
 ### Running Lettuce Acceptance Tests
 
@@ -309,6 +317,14 @@ To start the debugger on failure, add the `--pdb` option to extra_args:
 To run tests faster by not collecting static files, you can use
 `paver test_acceptance -s lms --fasttest` and `paver test_acceptance -s cms --fasttest`.
 
+By default, all acceptance tests are run with the 'draft' ModuleStore.
+To override the modulestore that is used, use the default_store option. Currently, the possible stores for acceptance tests are:
+    'split' (xmodule.modulestore.split_mongo.split_draft.DraftVersioningModuleStore) and
+    'draft' (xmodule.modulestore.mongo.DraftMongoModuleStore).
+For example:
+    paver test_acceptance --default_store='draft'
+Note, however, all acceptance tests currently do not pass with 'split'.
+
 Acceptance tests will run on a randomized port and can be run in the background of paver cms and lms or unit tests.
 To specify the port, change the LETTUCE_SERVER_PORT constant in cms/envs/acceptance.py and lms/envs/acceptance.py
 as well as the port listed in cms/djangoapps/contentstore/feature/upload.py

lms/envs/acceptance.py

@@ -50,7 +50,8 @@ update_module_store_settings(
     },
     module_store_options={
         'fs_root': TEST_ROOT / "data",
-    }
+    },
+    default_store=os.environ.get('DEFAULT_STORE', 'draft'),
 )
 CONTENTSTORE = {
     'ENGINE': 'xmodule.contentstore.mongo.MongoContentStore',

lms/envs/bok_choy.py

@@ -39,6 +39,7 @@ update_module_store_settings(
     xml_store_options={
         'data_dir': (TEST_ROOT / "data").abspath(),
     },
+    default_store=os.environ.get('DEFAULT_STORE', 'draft'),
 )
 
 # Configure the LMS to use our stub XQueue implementation

lms/templates/courseware/course_about.html

@@ -102,7 +102,7 @@
           location.href = xhr.responseText;
         }
       } else if (xhr.status == 403) {
-        location.href = "${reverse('register_user')}?course_id=${course.id.to_deprecated_string()}&enrollment_action=enroll";
+          location.href = "${reverse('register_user')}?course_id=${course.id | u}&enrollment_action=enroll";
       } else {
         $('#register_error').html(
             (xhr.responseText ? xhr.responseText : 'An error occurred. Please try again later.')

pavelib/acceptance_test.py

@@ -19,6 +19,7 @@ __test__ = False  # do not collect
 )
 @cmdopts([
     ("system=", "s", "System to act on"),
+    ("default_store=", "m", "Default modulestore to use for course creation"),
     ("fasttest", "a", "Run without collectstatic"),
     ("extra_args=", "e", "adds as extra args to the test command"),
     make_option("--verbose", action="store_const", const=2, dest="verbosity"),
@@ -32,6 +33,7 @@ def test_acceptance(options):
     opts = {
         'fasttest': getattr(options, 'fasttest', False),
         'system': getattr(options, 'system', None),
+        'default_store': getattr(options, 'default_store', None),
         'verbosity': getattr(options, 'verbosity', 3),
         'extra_args': getattr(options, 'extra_args', ''),
     }
@@ -42,6 +44,12 @@ def test_acceptance(options):
             'No system specified, running tests for both cms and lms.'
         )
         print(msg)
+    if opts['default_store'] not in ['draft', 'split']:
+        msg = colorize(
+            'red',
+            'No modulestore specified, running tests for both draft and split.'
+        )
+        print(msg)
 
     suite = AcceptanceTestSuite('{} acceptance'.format(opts['system']), **opts)
     suite.run()

pavelib/bok_choy.py

@@ -22,6 +22,7 @@ __test__ = False  # do not collect
     ('test_spec=', 't', 'Specific test to run'),
     ('fasttest', 'a', 'Skip some setup'),
     ('extra_args=', 'e', 'adds as extra args to the test command'),
+    ('default_store=', 's', 'Default modulestore'),
     make_option("--verbose", action="store_const", const=2, dest="verbosity"),
     make_option("-q", "--quiet", action="store_const", const=0, dest="verbosity"),
     make_option("-v", "--verbosity", action="count", dest="verbosity"),
@@ -45,13 +46,12 @@ def test_bokchoy(options):
     opts = {
         'test_spec': getattr(options, 'test_spec', None),
         'fasttest': getattr(options, 'fasttest', False),
+        'default_store': getattr(options, 'default_store', None),
         'verbosity': getattr(options, 'verbosity', 2),
         'extra_args': getattr(options, 'extra_args', ''),
         'test_dir': 'tests',
     }
-
+    run_bokchoy(**opts)
-    test_suite = BokChoyTestSuite('bok-choy', **opts)
-    test_suite.run()
 
 
 @task
@@ -60,6 +60,7 @@ def test_bokchoy(options):
     ('test_spec=', 't', 'Specific test to run'),
     ('fasttest', 'a', 'Skip some setup'),
     ('imports_dir=', 'd', 'Directory containing (un-archived) courses to be imported'),
+    ('default_store=', 's', 'Default modulestore'),
     make_option("--verbose", action="store_const", const=2, dest="verbosity"),
     make_option("-q", "--quiet", action="store_const", const=0, dest="verbosity"),
     make_option("-v", "--verbosity", action="count", dest="verbosity"),
@@ -72,13 +73,33 @@ def perf_report_bokchoy(options):
         'test_spec': getattr(options, 'test_spec', None),
         'fasttest': getattr(options, 'fasttest', False),
         'imports_dir': getattr(options, 'imports_dir', None),
+        'default_store': getattr(options, 'default_store', None),
         'verbosity': getattr(options, 'verbosity', 2),
         'test_dir': 'performance',
         'ptests': True,
     }
+    run_bokchoy(**opts)
 
-    test_suite = BokChoyTestSuite('bok-choy', **opts)
+
-    test_suite.run()
+def run_bokchoy(**opts):
+    """
+    Runs BokChoyTestSuite with the given options.
+    If a default store is not specified, runs the test suite for 'split' as the default store.
+    """
+    if opts['default_store'] not in ['draft', 'split']:
+        msg = colorize(
+            'red',
+            'No modulestore specified, running tests for split.'
+        )
+        print(msg)
+        stores = ['split']
+    else:
+        stores = [opts['default_store']]
+
+    for store in stores:
+        opts['default_store'] = store
+        test_suite = BokChoyTestSuite('bok-choy', **opts)
+        test_suite.run()
 
 
 @task

pavelib/utils/test/bokchoy_utils.py

@@ -18,7 +18,7 @@ except ImportError:
 __test__ = False  # do not collect
 
 
-def start_servers():
+def start_servers(default_store):
     """
     Start the servers we will run tests on, returns PIDs for servers.
     """
@@ -33,9 +33,11 @@ def start_servers():
     for service, info in Env.BOK_CHOY_SERVERS.iteritems():
         address = "0.0.0.0:{}".format(info['port'])
         cmd = (
+            "DEFAULT_STORE={default_store} "
             "coverage run --rcfile={coveragerc} -m "
             "manage {service} --settings bok_choy runserver "
             "{address} --traceback --noreload".format(
+                default_store=default_store,
                 coveragerc=Env.BOK_CHOY_COVERAGERC,
                 service=service,
                 address=address,

pavelib/utils/test/suites/acceptance_suite.py

@@ -17,7 +17,8 @@ class AcceptanceTest(TestSuite):
         super(AcceptanceTest, self).__init__(*args, **kwargs)
         self.report_dir = Env.REPORT_DIR / 'acceptance'
         self.fasttest = kwargs.get('fasttest', False)
-        self.system = kwargs.get('system', None)
+        self.system = kwargs.get('system')
+        self.default_store = kwargs.get('default_store')
         self.extra_args = kwargs.get('extra_args', '')
 
     def __enter__(self):
@@ -35,9 +36,10 @@ class AcceptanceTest(TestSuite):
         report_file = self.report_dir /  "{}.xml".format(self.system)
         report_args = "--with-xunit --xunit-file {}".format(report_file)
 
-        cmd = ( 
+        cmd = (
-            "./manage.py {system} --settings acceptance harvest --traceback "
+            "DEFAULT_STORE={default_store} ./manage.py {system} --settings acceptance harvest --traceback "
             "--debug-mode --verbosity {verbosity} {report_args} {extra_args}".format(
+                default_store=self.default_store,
                 system=self.system,
                 verbosity=self.verbosity,
                 report_args=report_args,
@@ -65,24 +67,31 @@ class AcceptanceTestSuite(TestSuite):
         self.root = 'acceptance'
         self.db = Env.REPO_ROOT / 'test_root/db/test_edx.db'
         self.db_cache = Env.REPO_ROOT / 'common/test/db_cache/lettuce.db'
-        self.system = kwargs.get('system', None)
         self.fasttest = kwargs.get('fasttest', False)
 
-        if self.system:
+        if kwargs.get('system'):
-            self.subsuites = [
+            systems = [kwargs['system']]
-                AcceptanceTest('{} acceptance'.format(self.system), **kwargs),
-            ]
         else:
-            kwargs['system'] = 'lms'
+            systems = ['lms', 'cms']
-            lms = AcceptanceTest('lms acceptance', **kwargs)
+
-            kwargs['system'] = 'cms'
+        if kwargs.get('default_store'):
-            cms = AcceptanceTest('cms acceptance', **kwargs)
+            stores = [kwargs['default_store']]
-            self.subsuites = [lms, cms]
+        else:
+            # TODO fix Acceptance tests with Split (LMS-11300)
+            # stores = ['split', 'draft']
+            stores = ['draft']
+
+        self.subsuites = []
+        for system in systems:
+            for default_store in stores:
+                kwargs['system'] = system
+                kwargs['default_store'] = default_store
+                self.subsuites.append(AcceptanceTest('{} acceptance using {}'.format(system, default_store), **kwargs))
 
     def __enter__(self):
         super(AcceptanceTestSuite, self).__enter__()
-        test_utils.clean_test_files() 
+        test_utils.clean_test_files()
-        
+
         if not self.fasttest:
             self._setup_acceptance_db()
 
@@ -104,7 +113,7 @@ class AcceptanceTestSuite(TestSuite):
         if self.db.isfile():
             # Since we are using SQLLite, we can reset the database by deleting it on disk.
             self.db.remove()
-    
+
         if self.db_cache.isfile():
             # To speed up migrations, we check for a cached database file and start from that.
             # The cached database file should be checked into the repo

pavelib/utils/test/suites/bokchoy_suite.py

@@ -28,6 +28,7 @@ class BokChoyTestSuite(TestSuite):
         self.cache = Env.BOK_CHOY_CACHE
         self.fasttest = kwargs.get('fasttest', False)
         self.test_spec = kwargs.get('test_spec', None)
+        self.default_store = kwargs.get('default_store')
         self.verbosity = kwargs.get('verbosity', 2)
         self.extra_args = kwargs.get('extra_args', '')
         self.ptests = kwargs.get('ptests', False)
@@ -64,17 +65,26 @@ class BokChoyTestSuite(TestSuite):
         self.cache.flush_all()
 
         sh(
-            "./manage.py lms --settings bok_choy loaddata --traceback"
+            "DEFAULT_STORE={default_store}"
-            " common/test/db_fixtures/*.json"
+            " ./manage.py lms --settings bok_choy loaddata --traceback"
+            " common/test/db_fixtures/*.json".format(
+                default_store=self.default_store,
+            )
         )
 
         if self.imports_dir:
-            sh("./manage.py cms --settings=bok_choy import {}".format(self.imports_dir))
+            sh(
+                "DEFAULT_STORE={default_store}"
+                " ./manage.py cms --settings=bok_choy import {import_dir}".format(
+                    default_store=self.default_store,
+                    import_dir=self.imports_dir
+                )
+            )
 
         # Ensure the test servers are available
         msg = colorize('green', "Starting test servers...")
         print(msg)
-        bokchoy_utils.start_servers()
+        bokchoy_utils.start_servers(self.default_store)
 
         msg = colorize('green', "Waiting for servers to start...")
         print(msg)
@@ -101,6 +111,7 @@ class BokChoyTestSuite(TestSuite):
         # Construct the nosetests command, specifying where to save
         # screenshots and XUnit XML reports
         cmd = [
+            "DEFAULT_STORE={}".format(self.default_store),
             "SCREENSHOT_DIR='{}'".format(self.log_dir),
             "HAR_DIR='{}'".format(self.har_dir),
             "SELENIUM_DRIVER_LOG_DIR='{}'".format(self.log_dir),