LMS-11373 Fix Studio drag and drop race condition.

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,6 +325,8 @@ 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).
         if publish == "discard_changes":
@@ -326,12 +344,46 @@ def _save_xblock(user, xblock, data=None, children=None, metadata=None, nullout=
         else:
             data = old_content['data'] if 'data' in old_content else None
 
-    if children is not None:
-        children_usage_keys = []
-        for child in children:
-            child_usage_key = usage_key_with_run(child)
-            children_usage_keys.append(child_usage_key)
-        xblock.children = children_usage_keys
+        if children_strings is not None:
+            children = []
+            for child_string in children_strings:
+                children.append(usage_key_with_run(child_string))
+
+            # if new children have been added, remove them from their old parents
+            new_children = set(children) - set(xblock.children)
+            for new_child in new_children:
+                old_parent_location = store.get_parent_location(new_child)
+                if old_parent_location:
+                    old_parent = store.get_item(old_parent_location)
+                    old_parent.children.remove(new_child)
+                    _update_with_callback(old_parent, user)
+                else:
+                    # the Studio UI currently doesn't present orphaned children, so assume this is an error
+                    return JsonResponse({"error": "Invalid data, possibly caused by concurrent authors."}, 400)
+
+            # make sure there are no old children that became orphans
+            # 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)
+
+            # set the children on the xblock
+            xblock.children = children
 
         # also commit any metadata which might have been passed along
         if nullout is not None or metadata is not None:
@@ -358,11 +410,8 @@ def _save_xblock(user, xblock, data=None, children=None, metadata=None, nullout=
                             return JsonResponse({"error": "Invalid data"}, 400)
                         field.write_to(xblock, value)
 
-    if callable(getattr(xblock, "editor_saved", None)):
-        xblock.editor_saved(user, old_metadata, old_content)
-
-    # commit to datastore
-    store.update_item(xblock, user.id)
+        # update the xblock and call any xblock callbacks
+        _update_with_callback(xblock, user, old_metadata, old_content)
 
         # for static tabs, their containing course also records their display name
         if xblock.location.category == 'static_tab':

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(
-            self.course_update_url,
-            data={'children': [unicode(chapter2_usage_key)]}
-        )
-        self.assertEqual(resp.status_code, 200)
+        resp = self.client.delete(reverse_usage_url("xblock_handler", chapter1_usage_key))
+        self.assertEqual(resp.status_code, 204)
 
         # 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/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);
+                    }
                 });
             },