From 2930e29a010ead8878e2e22355fdf7123dbb28a9 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Mon, 8 Sep 2014 16:06:02 -0400 Subject: [PATCH] LMS-11373 Fix Studio drag and drop race condition. --- cms/djangoapps/contentstore/views/item.py | 201 +++++++++++------- .../contentstore/views/tests/test_item.py | 124 +++++++---- .../js/spec/utils/drag_and_drop_spec.js | 7 +- cms/static/js/utils/drag_and_drop.js | 22 +- 4 files changed, 219 insertions(+), 135 deletions(-) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index e7aa4d55cc..7d043d840b 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/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). - if publish == "discard_changes": - store.revert_to_published(xblock.location, user.id) - # Returning the same sort of result that we do for other save operations. In the future, - # we may want to return the full XBlockInfo. - return JsonResponse({'id': unicode(xblock.location)}) + # Don't allow updating an xblock and discarding changes in a single operation (unsupported by UI). + if publish == "discard_changes": + store.revert_to_published(xblock.location, user.id) + # Returning the same sort of result that we do for other save operations. In the future, + # we may want to return the full XBlockInfo. + return JsonResponse({'id': unicode(xblock.location)}) - old_metadata = own_metadata(xblock) - old_content = xblock.get_explicitly_set_fields_by_scope(Scope.content) + old_metadata = own_metadata(xblock) + old_content = xblock.get_explicitly_set_fields_by_scope(Scope.content) - if data: - # TODO Allow any scope.content fields not just "data" (exactly like the get below this) - xblock.data = data - else: - data = old_content['data'] if 'data' in old_content else None + if data: + # TODO Allow any scope.content fields not just "data" (exactly like the get below this) + xblock.data = data + 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)) - # also commit any metadata which might have been passed along - if nullout is not None or metadata is not None: - # the postback is not the complete metadata, as there's system metadata which is - # not presented to the end-user for editing. So let's use the original (existing_item) and - # 'apply' the submitted metadata, so we don't end up deleting system metadata. - if nullout is not None: - for metadata_key in nullout: - setattr(xblock, metadata_key, None) - - # 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) + # 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: - try: - value = field.from_json(value) - except ValueError: - return JsonResponse({"error": "Invalid data"}, 400) - field.write_to(xblock, value) + # 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) - if callable(getattr(xblock, "editor_saved", None)): - xblock.editor_saved(user, old_metadata, old_content) + # 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) - # commit to datastore - store.update_item(xblock, user.id) + # set the children on the xblock + xblock.children = children - # for static tabs, their containing course also records their display name - if xblock.location.category == 'static_tab': - course = store.get_course(xblock.location.course_key) - # 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) + # also commit any metadata which might have been passed along + if nullout is not None or metadata is not None: + # the postback is not the complete metadata, as there's system metadata which is + # not presented to the end-user for editing. So let's use the original (existing_item) and + # 'apply' the submitted metadata, so we don't end up deleting system metadata. + if nullout is not None: + for metadata_key in nullout: + setattr(xblock, metadata_key, None) - result = { - 'id': unicode(xblock.location), - 'data': data, - 'metadata': own_metadata(xblock) - } + # 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 grader_type is not None: - result.update(CourseGradingModel.update_section_grader_type(xblock, grader_type, user)) + if value is None: + 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, - # 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' + # update the xblock and call any xblock callbacks + _update_with_callback(xblock, user, old_metadata, old_content) - # 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) + # for static tabs, their containing course also records their display name + if xblock.location.category == 'static_tab': + course = store.get_course(xblock.location.course_key) + # 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. - return JsonResponse(result, encoder=EdxJSONEncoder) + result = { + '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 diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 53bb73fe90..79ef4b16a9 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/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). diff --git a/cms/static/js/spec/utils/drag_and_drop_spec.js b/cms/static/js/spec/utils/drag_and_drop_spec.js index 93e4b23340..1eea1f920a 100644 --- a/cms/static/js/spec/utils/drag_and_drop_spec.js +++ b/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'); diff --git a/cms/static/js/utils/drag_and_drop.js b/cms/static/js/utils/drag_and_drop.js index 8a7ef8d3aa..23c3404559 100644 --- a/cms/static/js/utils/drag_and_drop.js +++ b/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); + } }); },