Merge pull request #14676 from edx/mushtaq/fix-discard-multiple-components

Fix multiple copies of components on discard changing a move operation.

cms/djangoapps/contentstore/views/item.py

@@ -733,8 +733,8 @@ def _move_item(source_usage_key, target_parent_usage_key, user, target_index=Non
                 source_type=source_type,
                 target_parent_type=target_parent_type,
             )
-        elif source_parent.location == target_parent.location:
-            error = _('You can not move an item into the same parent.')
+        elif source_parent.location == target_parent.location or source_item.location in target_parent.children:
+            error = _('Item is already present in target location.')
         elif source_item.location == target_parent.location:
             error = _('You can not move an item into itself.')
         elif is_source_item_in_target_parents(source_item, target_parent):
@@ -761,16 +761,16 @@ def _move_item(source_usage_key, target_parent_usage_key, user, target_index=Non
         if error:
             return JsonResponse({'error': error}, status=400)
 
-        # Remove reference from old parent.
-        source_parent.children.remove(source_item.location)
-        store.update_item(source_parent, user.id)
-
         # When target_index is provided, insert xblock at target_index position, otherwise insert at the end.
         insert_at = target_index if target_index is not None else len(target_parent.children)
 
-        # Add to new parent at particular location.
-        target_parent.children.insert(insert_at, source_item.location)
-        store.update_item(target_parent, user.id)
+        store.update_item_parent(
+            item_location=source_item.location,
+            new_parent_location=target_parent.location,
+            old_parent_location=source_parent.location,
+            insert_at=insert_at,
+            user_id=user.id
+        )
 
         log.info(
             'MOVE: %s moved from %s to %s at %d index',

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

@@ -31,6 +31,7 @@ from xblock_django.models import XBlockConfiguration, XBlockStudioConfiguration,
 from xmodule.capa_module import CapaDescriptor
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.django import modulestore
+from xmodule.modulestore.exceptions import ItemNotFoundError
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, TEST_DATA_SPLIT_MODULESTORE
 from xmodule.modulestore.tests.factories import ItemFactory, LibraryFactory, check_mongo_calls, CourseFactory
 from xmodule.x_module import STUDIO_VIEW, STUDENT_VIEW
@@ -878,10 +879,20 @@ class TestMoveItem(ItemTest):
         self.assertEqual(response['move_source_locator'], unicode(source_usage_key))
         self.assertEqual(response['parent_locator'], unicode(target_usage_key))
         self.assertEqual(response['source_index'], expected_index)
+
+        # Verify parent referance has been changed now.
         new_parent_loc = self.store.get_parent_location(source_usage_key)
+        source_item = self.get_item_from_modulestore(source_usage_key)
+        self.assertEqual(source_item.parent, new_parent_loc)
         self.assertEqual(new_parent_loc, target_usage_key)
         self.assertNotEqual(parent_loc, new_parent_loc)
 
+        # Assert item is present in children list of target parent and not source parent
+        target_parent = self.get_item_from_modulestore(target_usage_key)
+        source_parent = self.get_item_from_modulestore(parent_loc)
+        self.assertIn(source_usage_key, target_parent.children)
+        self.assertNotIn(source_usage_key, source_parent.children)
+
     @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
     def test_move_component(self, store_type):
         """
@@ -988,7 +999,7 @@ class TestMoveItem(ItemTest):
         self.assertEqual(response.status_code, 400)
         response = json.loads(response.content)
 
-        self.assertEqual(response['error'], 'You can not move an item into the same parent.')
+        self.assertEqual(response['error'], 'Item is already present in target location.')
         self.assertEqual(self.store.get_parent_location(self.html_usage_key), parent_loc)
 
     def test_can_not_move_into_itself(self):
@@ -1161,6 +1172,80 @@ class TestMoveItem(ItemTest):
             insert_at
         )
 
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_move_and_discard_changes(self, store_type):
+        """
+        Verifies that discard changes operation brings moved component back to source location and removes the component
+        from target location.
+
+        Arguments:
+            store_type (ModuleStoreEnum.Type): Type of modulestore to create test course in.
+        """
+        self.setup_course(default_store=store_type)
+
+        old_parent_loc = self.store.get_parent_location(self.html_usage_key)
+
+        # Check that old_parent_loc is not yet published.
+        self.assertFalse(self.store.has_item(old_parent_loc, revision=ModuleStoreEnum.RevisionOption.published_only))
+
+        # Publish old_parent_loc unit
+        self.client.ajax_post(
+            reverse_usage_url("xblock_handler", old_parent_loc),
+            data={'publish': 'make_public'}
+        )
+
+        # Check that old_parent_loc is now published.
+        self.assertTrue(self.store.has_item(old_parent_loc, revision=ModuleStoreEnum.RevisionOption.published_only))
+        self.assertFalse(self.store.has_changes(self.store.get_item(old_parent_loc)))
+
+        # Move component html_usage_key in vert2_usage_key
+        self.assert_move_item(self.html_usage_key, self.vert2_usage_key)
+
+        # Check old_parent_loc becomes in draft mode now.
+        self.assertTrue(self.store.has_changes(self.store.get_item(old_parent_loc)))
+
+        # Now discard changes in old_parent_loc
+        self.client.ajax_post(
+            reverse_usage_url("xblock_handler", old_parent_loc),
+            data={'publish': 'discard_changes'}
+        )
+
+        # Check that old_parent_loc now is reverted to publish. Changes discarded, html_usage_key moved back.
+        self.assertTrue(self.store.has_item(old_parent_loc, revision=ModuleStoreEnum.RevisionOption.published_only))
+        self.assertFalse(self.store.has_changes(self.store.get_item(old_parent_loc)))
+
+        # Now source item should be back in the old parent.
+        source_item = self.get_item_from_modulestore(self.html_usage_key)
+        self.assertEqual(source_item.parent, old_parent_loc)
+        self.assertEqual(self.store.get_parent_location(self.html_usage_key), source_item.parent)
+
+        # Also, check that item is not present in target parent but in source parent
+        target_parent = self.get_item_from_modulestore(self.vert2_usage_key)
+        source_parent = self.get_item_from_modulestore(old_parent_loc)
+        self.assertIn(self.html_usage_key, source_parent.children)
+        self.assertNotIn(self.html_usage_key, target_parent.children)
+
+    @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split)
+    def test_move_item_not_found(self, store_type=ModuleStoreEnum.Type.mongo):
+        """
+        Test that an item not found exception raised when an item is not found when getting the item.
+
+        Arguments:
+            store_type (ModuleStoreEnum.Type): Type of modulestore to create test course in.
+        """
+        self.setup_course(default_store=store_type)
+
+        data = {
+            'move_source_locator': unicode(self.usage_key.course_key.make_usage_key('html', 'html_test')),
+            'parent_locator': unicode(self.vert2_usage_key)
+        }
+        with self.assertRaises(ItemNotFoundError):
+            self.client.patch(
+                reverse('contentstore.views.xblock_handler'),
+                json.dumps(data),
+                content_type='application/json'
+            )
+
 
 class TestDuplicateItemWithAsides(ItemTest, DuplicateHelper):
     """

common/lib/xmodule/xmodule/modulestore/draft_and_published.py

@@ -3,13 +3,17 @@ This module provides an abstraction for Module Stores that support Draft and Pub
 """
 
 import threading
+import logging
 from abc import ABCMeta, abstractmethod
 from contextlib import contextmanager
 from . import ModuleStoreEnum, BulkOperationsMixin
+from .exceptions import ItemNotFoundError
 
 # Things w/ these categories should never be marked as version=DRAFT
 DIRECT_ONLY_CATEGORIES = ['course', 'chapter', 'sequential', 'about', 'static_tab', 'course_info']
 
+log = logging.getLogger(__name__)
+
 
 class BranchSettingMixin(object):
     """
@@ -134,6 +138,61 @@ class ModuleStoreDraftAndPublished(BranchSettingMixin, BulkOperationsMixin):
                 # We remove the branch, because publishing always means copying from draft to published
                 self.signal_handler.send("course_published", course_key=course_key.for_branch(None))
 
+    def update_item_parent(self, item_location, new_parent_location, old_parent_location, user_id, insert_at=None):
+        """
+        Updates item's parent and removes it's reference from old parent.
+
+        Arguments:
+            item_location (BlockUsageLocator)    : Locator of item.
+            new_parent_location (BlockUsageLocator)  : New parent block locator.
+            old_parent_location (BlockUsageLocator)  : Old parent block locator.
+            user_id (int)   : User id.
+            insert_at (int) : Insert item at the particular index in new parent.
+
+        Returns:
+           BlockUsageLocator or None: Source item location if updated, otherwise None.
+        """
+        try:
+            source_item = self.get_item(item_location)  # pylint: disable=no-member
+            old_parent_item = self.get_item(old_parent_location)    # pylint: disable=no-member
+            new_parent_item = self.get_item(new_parent_location)    # pylint: disable=no-member
+        except ItemNotFoundError as exception:
+            log.error('Unable to find the item : %s', exception.message)
+            return
+
+        # Remove item from the list of children of old parent.
+        if source_item.location in old_parent_item.children:
+            old_parent_item.children.remove(source_item.location)
+            self.update_item(old_parent_item, user_id)  # pylint: disable=no-member
+            log.info(
+                '%s removed from %s children',
+                unicode(source_item.location),
+                unicode(old_parent_item.location)
+            )
+
+        # Add item to new parent at particular location.
+        if source_item.location not in new_parent_item.children:
+            if insert_at is not None:
+                new_parent_item.children.insert(insert_at, source_item.location)
+            else:
+                new_parent_item.children.append(source_item.location)
+            self.update_item(new_parent_item, user_id)  # pylint: disable=no-member
+            log.info(
+                '%s added to %s children',
+                unicode(source_item.location),
+                unicode(new_parent_item.location)
+            )
+
+        # Update parent attribute of the item block
+        source_item.parent = new_parent_location
+        self.update_item(source_item, user_id)  # pylint: disable=no-member
+        log.info(
+            '%s parent updated to %s',
+            unicode(source_item.location),
+            unicode(new_parent_item.location)
+        )
+        return source_item.location
+
 
 class UnsupportedRevisionError(ValueError):
     """

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

@@ -796,6 +796,15 @@ class DraftModuleStore(MongoModuleStore):
             # If 2 versions versions exist, we can assume one is a published version. Go ahead and do the delete
             # of the draft version.
             if versions_found.count() > 1:
+                # Moving a child from published parent creates a draft of the parent and moved child.
+                published_version = [
+                    version
+                    for version in versions_found
+                    if version.get('_id').get('revision') != MongoRevisionKey.draft
+                ]
+                if len(published_version) > 0:
+                    # This change makes sure that parents are updated too i.e. an item will have only one parent.
+                    self.update_parent_if_moved(root_location, published_version[0], delete_draft_only, user_id)
                 self._delete_subtree(root_location, [as_draft], draft_only=True)
             elif versions_found.count() == 1:
                 # Since this method cannot be called on something in DIRECT_ONLY_CATEGORIES and we call
@@ -809,6 +818,28 @@ class DraftModuleStore(MongoModuleStore):
 
         delete_draft_only(location)
 
+    def update_parent_if_moved(self, original_parent_location, published_version, delete_draft_only, user_id):
+        """
+        Update parent of an item if it has moved.
+
+        Arguments:
+            original_parent_location (BlockUsageLocator)  : Original parent block locator.
+            published_version (dict)   : Published version of the block.
+            delete_draft_only (function)    : A callback function to delete draft children if it was moved.
+            user_id (int)   : User id
+        """
+        for child_location in published_version.get('definition', {}).get('children', []):
+            item_location = original_parent_location.course_key.make_usage_key_from_deprecated_string(child_location)
+            try:
+                source_item = self.get_item(item_location)
+            except ItemNotFoundError:
+                log.error('Unable to find the item %s', unicode(item_location))
+                return
+
+            if source_item.parent and source_item.parent.block_id != original_parent_location.block_id:
+                if self.update_item_parent(item_location, original_parent_location, source_item.parent, user_id):
+                    delete_draft_only(Location.from_deprecated_string(child_location))
+
     def _query_children_for_cache_children(self, course_key, items):
         # first get non-draft in a round-trip
         to_process_non_drafts = super(DraftModuleStore, self)._query_children_for_cache_children(course_key, items)

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

@@ -443,7 +443,10 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
                     self._get_block_from_structure(published_course_structure, root_block_id)
                 )
                 block = self._get_block_from_structure(new_structure, root_block_id)
+                original_parent_location = location.course_key.make_usage_key(root_block_id.type, root_block_id.id)
                 for child_block_id in block.fields.get('children', []):
+                    item_location = location.course_key.make_usage_key(child_block_id.type, child_block_id.id)
+                    self.update_parent_if_moved(item_location, original_parent_location, new_structure, user_id)
                     copy_from_published(child_block_id)
 
             copy_from_published(BlockKey.from_usage_key(location))
@@ -454,6 +457,23 @@ class DraftVersioningModuleStore(SplitMongoModuleStore, ModuleStoreDraftAndPubli
             if index_entry is not None:
                 self._update_head(draft_course_key, index_entry, ModuleStoreEnum.BranchName.draft, new_structure['_id'])
 
+    def update_parent_if_moved(self, item_location, original_parent_location, course_structure, user_id):
+        """
+        Update parent of an item if it has moved.
+
+        Arguments:
+            item_location (BlockUsageLocator)    : Locator of item.
+            original_parent_location (BlockUsageLocator)  : Original parent block locator.
+            course_structure (dict)  : course structure of the course.
+            user_id (int)   : User id
+        """
+        parent_block_keys = self._get_parents_from_structure(BlockKey.from_usage_key(item_location), course_structure)
+        for block_key in parent_block_keys:
+            # Item's parent is different than its new parent - so it has moved.
+            if block_key.id != original_parent_location.block_id:
+                old_parent_location = original_parent_location.course_key.make_usage_key(block_key.type, block_key.id)
+                self.update_item_parent(item_location, original_parent_location, old_parent_location, user_id)
+
     def force_publish_course(self, course_locator, user_id, commit=False):
         """
         Helper method to forcefully publish a course,