From bf0c4f51859278e531c07b522feff473da94eda8 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Mon, 15 Sep 2014 13:27:29 -0400 Subject: [PATCH] Don't let destructive loc change leak out to callers. Also, use the return from update_item rather than assume it was just side effecting. LMS-11361 --- cms/djangoapps/contentstore/views/item.py | 8 ++++---- cms/djangoapps/contentstore/views/tests/test_item.py | 2 -- .../xmodule/modulestore/split_mongo/split_draft.py | 4 +++- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 7d043d840b..8e80d7231d 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -314,7 +314,7 @@ def _update_with_callback(xblock, user, old_metadata=None, old_content=None): 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) + return modulestore().update_item(xblock, user.id) def _save_xblock(user, xblock, data=None, children_strings=None, metadata=None, nullout=None, @@ -356,7 +356,7 @@ def _save_xblock(user, xblock, data=None, children_strings=None, metadata=None, if old_parent_location: old_parent = store.get_item(old_parent_location) old_parent.children.remove(new_child) - _update_with_callback(old_parent, user) + old_parent = _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) @@ -411,7 +411,7 @@ def _save_xblock(user, xblock, data=None, children_strings=None, metadata=None, field.write_to(xblock, value) # update the xblock and call any xblock callbacks - _update_with_callback(xblock, user, old_metadata, old_content) + xblock = _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': @@ -550,7 +550,7 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_ dest_module.children.append(dupe) store.update_item(dest_module, user.id) - if not 'detached' in source_item.runtime.load_block_type(category)._class_tags: + if 'detached' not in source_item.runtime.load_block_type(category)._class_tags: parent = store.get_item(parent_usage_key) # If source was already a child of the parent, add duplicate immediately afterward. # Otherwise, add child to end. diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 79ef4b16a9..5d4c73d724 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -117,7 +117,6 @@ class GetItemTest(ItemTest): with check_mongo_calls(problem_queries): self.client.get(reverse_usage_url('xblock_handler', self.populated_usage_keys['problem'][-1])) - def test_get_vertical(self): # Add a vertical resp = self.create_xblock(category='vertical') @@ -197,7 +196,6 @@ class GetItemTest(ItemTest): self.assertIn('Announcement', html) self.assertIn('Zooming', html) - def test_split_test_edited(self): """ Test that rename of a group changes display name of child vertical. diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index a7ad349773..3936fb7055 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -89,7 +89,8 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS self.publish(location.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs) def update_item(self, descriptor, user_id, allow_not_found=False, force=False, **kwargs): - descriptor.location = self._map_revision_to_branch(descriptor.location) + old_descriptor_locn = descriptor.location + descriptor.location = self._map_revision_to_branch(old_descriptor_locn) with self.bulk_operations(descriptor.location.course_key): item = super(DraftVersioningModuleStore, self).update_item( descriptor, @@ -99,6 +100,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS **kwargs ) self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs) + descriptor.location = old_descriptor_locn return item def create_item(