Move an item - TNL-6064

cms/djangoapps/contentstore/views/item.py

@@ -19,6 +19,8 @@ from django.views.decorators.http import require_http_methods
 from opaque_keys.edx.keys import CourseKey
 from opaque_keys.edx.locator import LibraryUsageLocator
 from pytz import UTC
+
+from xblock.core import XBlock
 from xblock.fields import Scope
 from xblock.fragment import Fragment
 from xblock_django.user_service import DjangoXBlockUserService
@@ -127,8 +129,11 @@ def xblock_handler(request, usage_key_string):
               if usage_key_string is not specified, create a new xblock instance, either by duplicating
               an existing xblock, or creating an entirely new one. The json playload can contain
               these fields:
-                :parent_locator: parent for new xblock, required for both duplicate and create new instance
+                :parent_locator: parent for new xblock, required for duplicate, move and create new instance
                 :duplicate_source_locator: if present, use this as the source for creating a duplicate copy
+                :move_source_locator: if present, use this as the source item for moving
+                :target_index: if present, use this as the target index for moving an item to a particular index
+                    otherwise target_index is calculated. It is sent back in the response.
                 :category: type of xblock, required if duplicate_source_locator is not present.
                 :display_name: name for new xblock, optional
                 :boilerplate: template name for populating fields, optional and only used
@@ -198,14 +203,26 @@ def xblock_handler(request, usage_key_string):
                 request.user,
                 request.json.get('display_name'),
             )
-
-            return JsonResponse({"locator": unicode(dest_usage_key), "courseKey": unicode(dest_usage_key.course_key)})
+            return JsonResponse({'locator': unicode(dest_usage_key), 'courseKey': unicode(dest_usage_key.course_key)})
         else:
             return _create_item(request)
+    elif request.method == 'PATCH':
+        if 'move_source_locator' in request.json:
+            move_source_usage_key = usage_key_with_run(request.json.get('move_source_locator'))
+            target_parent_usage_key = usage_key_with_run(request.json.get('parent_locator'))
+            target_index = request.json.get('target_index')
+            if (
+                    not has_studio_write_access(request.user, target_parent_usage_key.course_key) or
+                    not has_studio_read_access(request.user, target_parent_usage_key.course_key)
+            ):
+                raise PermissionDenied()
+            return _move_item(move_source_usage_key, target_parent_usage_key, request.user, target_index)
+
+        return JsonResponse({'error': 'Patch request did not recognise any parameters to handle.'}, status=400)
     else:
         return HttpResponseBadRequest(
-            "Only instance creation is supported without a usage key.",
-            content_type="text/plain"
+            'Only instance creation is supported without a usage key.',
+            content_type='text/plain'
         )
 
 
@@ -636,10 +653,109 @@ def _create_item(request):
     )
 
     return JsonResponse(
-        {"locator": unicode(created_block.location), "courseKey": unicode(created_block.location.course_key)}
+        {'locator': unicode(created_block.location), 'courseKey': unicode(created_block.location.course_key)}
     )
 
 
+def _get_source_index(source_usage_key, source_parent):
+    """
+    Get source index position of the XBlock.
+
+    Arguments:
+        source_usage_key (BlockUsageLocator): Locator of source item.
+        source_parent (XBlock): A parent of the source XBlock.
+
+    Returns:
+        source_index (int): Index position of the xblock in a parent.
+    """
+    try:
+        source_index = source_parent.children.index(source_usage_key)
+        return source_index
+    except ValueError:
+        return None
+
+
+def _move_item(source_usage_key, target_parent_usage_key, user, target_index=None):
+    """
+    Move an existing xblock as a child of the supplied target_parent_usage_key.
+
+    Arguments:
+        source_usage_key (BlockUsageLocator): Locator of source item.
+        target_parent_usage_key (BlockUsageLocator): Locator of target parent.
+        target_index (int): If provided, insert source item at provided index location in target_parent_usage_key item.
+
+    Returns:
+        JsonResponse: Information regarding move operation. It may contains error info if an invalid move operation
+            is performed.
+    """
+    # Get the list of all component type XBlocks
+    component_types = sorted(set(name for name, class_ in XBlock.load_classes()) - set(DIRECT_ONLY_CATEGORIES))
+
+    store = modulestore()
+    with store.bulk_operations(source_usage_key.course_key):
+        source_item = store.get_item(source_usage_key)
+        source_parent = source_item.get_parent()
+        target_parent = store.get_item(target_parent_usage_key)
+        source_type = source_item.category
+        target_parent_type = target_parent.category
+        error = None
+
+        # Store actual/initial index of the source item. This would be sent back with response,
+        # so that with Undo operation, it would easier to move back item to it's original/old index.
+        source_index = _get_source_index(source_usage_key, source_parent)
+
+        valid_move_type = {
+            'vertical': source_type if source_type in component_types else 'component',
+            'sequential': 'vertical',
+            'chapter': 'sequential',
+        }
+
+        if valid_move_type.get(target_parent_type, '') != source_type:
+            error = 'You can not move {source_type} into {target_parent_type}.'.format(
+                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_index is None:
+            error = '{source_usage_key} not found in {parent_usage_key}.'.format(
+                source_usage_key=unicode(source_usage_key),
+                parent_usage_key=unicode(source_parent.location)
+            )
+        else:
+            try:
+                target_index = int(target_index) if target_index is not None else None
+                if len(target_parent.children) < target_index:
+                    error = 'You can not move {source_usage_key} at an invalid index ({target_index}).'.format(
+                        source_usage_key=unicode(source_usage_key),
+                        target_index=target_index
+                    )
+            except ValueError:
+                error = 'You must provide target_index ({target_index}) as an integer.'.format(
+                    target_index=target_index
+                )
+        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)
+
+        context = {
+            'move_source_locator': unicode(source_usage_key),
+            'parent_locator': unicode(target_parent_usage_key),
+            'source_index': target_index if target_index is not None else source_index
+        }
+        return JsonResponse(context)
+
+
 def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_name=None, is_child=False):
     """
     Duplicate an existing xblock as a child of the supplied parent_usage_key.

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

@@ -14,13 +14,14 @@ from django.test.client import RequestFactory
 from django.core.urlresolvers import reverse
 from contentstore.utils import reverse_usage_url, reverse_course_url
 
+from opaque_keys import InvalidKeyError
 from openedx.core.djangoapps.self_paced.models import SelfPacedConfiguration
 from contentstore.views.component import (
     component_handler, get_component_templates
 )
 
 from contentstore.views.item import (
-    create_xblock_info, _get_module_info, ALWAYS, VisibilityState, _xblock_type_and_display_name,
+    create_xblock_info, _get_source_index, _get_module_info, ALWAYS, VisibilityState, _xblock_type_and_display_name,
     add_container_page_publishing_info
 )
 from contentstore.tests.utils import CourseTestCase
@@ -734,6 +735,235 @@ class TestDuplicateItem(ItemTest, DuplicateHelper):
         verify_name(self.seq_usage_key, self.chapter_usage_key, "customized name", display_name="customized name")
 
 
+class TestMoveItem(ItemTest):
+    """
+    Tests for move item.
+    """
+    def setUp(self):
+        """
+        Creates the test course structure to build course outline tree.
+        """
+        super(TestMoveItem, self).setUp()
+
+        # Create a parent chapter
+        chap1 = self.create_xblock(parent_usage_key=self.course.location, display_name='chapter1', category='chapter')
+        self.chapter_usage_key = self.response_usage_key(chap1)
+
+        chap2 = self.create_xblock(parent_usage_key=self.course.location, display_name='chapter2', category='chapter')
+        self.chapter2_usage_key = self.response_usage_key(chap2)
+
+        # create a sequential
+        seq1 = self.create_xblock(parent_usage_key=self.chapter_usage_key, display_name='seq1', category='sequential')
+        self.seq_usage_key = self.response_usage_key(seq1)
+
+        seq2 = self.create_xblock(parent_usage_key=self.chapter_usage_key, display_name='seq2', category='sequential')
+        self.seq2_usage_key = self.response_usage_key(seq2)
+
+        # create a vertical
+        vert1 = self.create_xblock(parent_usage_key=self.seq_usage_key, display_name='vertical1', category='vertical')
+        self.vert_usage_key = self.response_usage_key(vert1)
+
+        vert2 = self.create_xblock(parent_usage_key=self.seq_usage_key, display_name='vertical2', category='vertical')
+        self.vert2_usage_key = self.response_usage_key(vert2)
+
+        # create problem and an html component
+        problem1 = self.create_xblock(parent_usage_key=self.vert_usage_key, display_name='problem1', category='problem')
+        self.problem_usage_key = self.response_usage_key(problem1)
+
+        html1 = self.create_xblock(parent_usage_key=self.vert_usage_key, display_name='html1', category='html')
+        self.html_usage_key = self.response_usage_key(html1)
+
+    def _move_component(self, source_usage_key, target_usage_key, target_index=None):
+        """
+        Helper method to send move request and returns the response.
+
+        Arguments:
+            source_usage_key (BlockUsageLocator): Locator of source item.
+            target_usage_key (BlockUsageLocator): Locator of target parent.
+            target_index (int): If provided, insert source item at the provided index location in target_usage_key item.
+
+        Returns:
+            resp (JsonResponse): Response after the move operation is complete.
+        """
+        data = {
+            'move_source_locator': unicode(source_usage_key),
+            'parent_locator': unicode(target_usage_key)
+        }
+        if target_index is not None:
+            data['target_index'] = target_index
+
+        return self.client.patch(
+            reverse('contentstore.views.xblock_handler'),
+            json.dumps(data),
+            content_type='application/json'
+        )
+
+    def assert_move_item(self, source_usage_key, target_usage_key, target_index=None):
+        """
+        Assert move component.
+
+        Arguments:
+            source_usage_key (BlockUsageLocator): Locator of source item.
+            target_usage_key (BlockUsageLocator): Locator of target parent.
+            target_index (int): If provided, insert source item at the provided index location in target_usage_key item.
+        """
+        parent_loc = self.store.get_parent_location(source_usage_key)
+        parent = self.get_item_from_modulestore(parent_loc)
+        source_index = _get_source_index(source_usage_key, parent)
+        expected_index = target_index if target_index is not None else source_index
+        response = self._move_component(source_usage_key, target_usage_key, target_index)
+        self.assertEqual(response.status_code, 200)
+        response = json.loads(response.content)
+        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)
+        new_parent_loc = self.store.get_parent_location(source_usage_key)
+        self.assertEqual(new_parent_loc, target_usage_key)
+        self.assertNotEqual(parent_loc, new_parent_loc)
+
+    def test_move_component(self):
+        """
+        Test move component with different xblock types.
+        """
+        for source_usage_key, target_usage_key in [
+                (self.html_usage_key, self.vert2_usage_key),
+                (self.vert_usage_key, self.seq2_usage_key),
+                (self.seq_usage_key, self.chapter2_usage_key)
+        ]:
+            self.assert_move_item(source_usage_key, target_usage_key)
+
+    def test_move_source_index(self):
+        """
+        Test moving an item to a particular index.
+        """
+        parent = self.get_item_from_modulestore(self.vert_usage_key)
+        children = parent.get_children()
+        self.assertEqual(len(children), 2)
+
+        # Create a component within vert2.
+        resp = self.create_xblock(parent_usage_key=self.vert2_usage_key, display_name='html2', category='html')
+        html2_usage_key = self.response_usage_key(resp)
+
+        # Move html2_usage_key inside vert_usage_key at second position.
+        self.assert_move_item(html2_usage_key, self.vert_usage_key, 1)
+        parent = self.get_item_from_modulestore(self.vert_usage_key)
+        children = parent.get_children()
+        self.assertEqual(len(children), 3)
+        self.assertEqual(children[1].location, html2_usage_key)
+
+    def test_move_undo(self):
+        """
+        Test move a component and move it back (undo).
+        """
+        # Get the initial index of the component
+        parent = self.get_item_from_modulestore(self.vert_usage_key)
+        original_index = _get_source_index(self.html_usage_key, parent)
+
+        # Move component and verify that response contains initial index
+        response = self._move_component(self.html_usage_key, self.vert2_usage_key)
+        response = json.loads(response.content)
+        self.assertEquals(original_index, response['source_index'])
+
+        # Verify that new parent has the moved component at the last index.
+        parent = self.get_item_from_modulestore(self.vert2_usage_key)
+        self.assertEqual(self.html_usage_key, parent.children[-1])
+
+        # Verify original and new index is different now.
+        source_index = _get_source_index(self.html_usage_key, parent)
+        self.assertNotEquals(original_index, source_index)
+
+        # Undo Move to the original index, use the source index fetched from the response.
+        response = self._move_component(self.html_usage_key, self.vert_usage_key, response['source_index'])
+        response = json.loads(response.content)
+        self.assertEquals(original_index, response['source_index'])
+
+    def test_move_large_target_index(self):
+        """
+        Test moving an item at a large index would generate an error message.
+        """
+        parent = self.get_item_from_modulestore(self.vert2_usage_key)
+        parent_children_length = len(parent.children)
+        response = self._move_component(self.html_usage_key, self.vert2_usage_key, parent_children_length + 10)
+        self.assertEqual(response.status_code, 400)
+        response = json.loads(response.content)
+
+        expected_error = 'You can not move {usage_key} at an invalid index ({target_index}).'.format(
+            usage_key=self.html_usage_key,
+            target_index=parent_children_length + 10
+        )
+        self.assertEqual(expected_error, response['error'])
+        new_parent_loc = self.store.get_parent_location(self.html_usage_key)
+        self.assertEqual(new_parent_loc, self.vert_usage_key)
+
+    def test_invalid_move(self):
+        """
+        Test invalid move.
+        """
+        parent_loc = self.store.get_parent_location(self.chapter_usage_key)
+        response = self._move_component(self.chapter_usage_key, self.usage_key)
+        self.assertEqual(response.status_code, 400)
+        response = json.loads(response.content)
+
+        expected_error = 'You can not move {source_type} into {target_type}.'.format(
+            source_type=self.chapter_usage_key.block_type,
+            target_type=self.usage_key.block_type
+        )
+        self.assertEqual(expected_error, response['error'])
+        new_parent_loc = self.store.get_parent_location(self.chapter_usage_key)
+        self.assertEqual(new_parent_loc, parent_loc)
+
+    def test_move_current_parent(self):
+        """
+        Test that a component can not be moved to it's current parent.
+        """
+        parent_loc = self.store.get_parent_location(self.html_usage_key)
+        self.assertEqual(parent_loc, self.vert_usage_key)
+        response = self._move_component(self.html_usage_key, self.vert_usage_key)
+        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(self.store.get_parent_location(self.html_usage_key), parent_loc)
+
+    def test_move_invalid_source_index(self):
+        """
+        Test moving an item to an invalid index.
+        """
+        target_index = 'test_index'
+        parent_loc = self.store.get_parent_location(self.html_usage_key)
+        response = self._move_component(self.html_usage_key, self.vert2_usage_key, target_index)
+        self.assertEqual(response.status_code, 400)
+        response = json.loads(response.content)
+
+        error = 'You must provide target_index ({target_index}) as an integer.'.format(target_index=target_index)
+        self.assertEqual(response['error'], error)
+        new_parent_loc = self.store.get_parent_location(self.html_usage_key)
+        self.assertEqual(new_parent_loc, parent_loc)
+
+    def test_move_no_target_locator(self):
+        """
+        Test move an item without specifying the target location.
+        """
+        data = {'move_source_locator': unicode(self.html_usage_key)}
+        with self.assertRaises(InvalidKeyError):
+            self.client.patch(
+                reverse('contentstore.views.xblock_handler'),
+                json.dumps(data),
+                content_type='application/json'
+            )
+
+    def test_no_move_source_locator(self):
+        """
+        Test patch request without providing a move source locator.
+        """
+        response = self.client.patch(
+            reverse('contentstore.views.xblock_handler')
+        )
+        self.assertEqual(response.status_code, 400)
+        response = json.loads(response.content)
+        self.assertEqual(response['error'], 'Patch request did not recognise any parameters to handle.')
+
+
 class TestDuplicateItemWithAsides(ItemTest, DuplicateHelper):
     """
     Test the duplicate method for blocks with asides.