Merge pull request #14625 from edx/mushtaq/bug-bash-fixes

Move Bug fixes

cms/djangoapps/contentstore/views/item.py

@@ -739,6 +739,8 @@ def _move_item(source_usage_key, target_parent_usage_key, user, target_index=Non
             error = 'You can not move an item into itself.'
         elif is_source_item_in_target_parents(source_item, target_parent):
             error = 'You can not move an item into it\'s child.'
+        elif target_parent_type == 'split_test':
+            error = 'You can not move an item directly into content experiment.'
         elif source_index is None:
             error = '{source_usage_key} not found in {parent_usage_key}.'.format(
                 source_usage_key=unicode(source_usage_key),
@@ -1119,7 +1121,8 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
     xblock_info = {
         'id': unicode(xblock.location),
         'display_name': xblock.display_name_with_default,
-        'category': xblock.category
+        'category': xblock.category,
+        'has_children': xblock.has_children
     }
     if is_concise:
         if child_info and len(child_info.get('children', [])) > 0:
@@ -1127,7 +1130,6 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
         # Groups are labelled with their internal ids, rather than with the group name. Replace id with display name.
         group_display_name = get_group_display_name(user_partitions, xblock_info['display_name'])
         xblock_info['display_name'] = group_display_name if group_display_name else xblock_info['display_name']
-        xblock_info['has_children'] = xblock.has_children
     else:
         xblock_info.update({
             'edited_on': get_default_time_display(xblock.subtree_edited_on) if xblock.subtree_edited_on else None,

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

@@ -948,17 +948,17 @@ class TestMoveItem(ItemTest):
         """
         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)
+        parent_loc = self.store.get_parent_location(self.html_usage_key)
+        response = self._move_component(self.html_usage_key, self.seq_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
+            source_type=self.html_usage_key.block_type,
+            target_type=self.seq_usage_key.block_type
         )
         self.assertEqual(expected_error, response['error'])
-        new_parent_loc = self.store.get_parent_location(self.chapter_usage_key)
+        new_parent_loc = self.store.get_parent_location(self.html_usage_key)
         self.assertEqual(new_parent_loc, parent_loc)
 
     def test_move_current_parent(self):
@@ -1039,11 +1039,23 @@ class TestMoveItem(ItemTest):
 
     def test_move_into_content_experiment_groups(self):
         """
-        Test that a component can be moved to content experiment.
+        Test that a component can be moved to content experiment groups.
         """
         split_test = self.setup_and_verify_content_experiment(0)
         self.assert_move_item(self.html_usage_key, split_test.children[0])
 
+    def test_can_not_move_into_content_experiment_level(self):
+        """
+        Test that a component can not be moved directly to content experiment level.
+        """
+        self.setup_and_verify_content_experiment(0)
+        response = self._move_component(self.html_usage_key, self.split_test_usage_key)
+        self.assertEqual(response.status_code, 400)
+        response = json.loads(response.content)
+
+        self.assertEqual(response['error'], 'You can not move an item directly into content experiment.')
+        self.assertEqual(self.store.get_parent_location(self.html_usage_key), self.vert_usage_key)
+
     def test_can_not_move_content_experiment_into_its_children(self):
         """
         Test that a content experiment can not be moved inside any of it's children.

cms/static/js/models/xblock_info.js

@@ -50,6 +50,10 @@ function(Backbone, _, str, ModuleUtils) {
              */
             'published_by': null,
             /**
+             * True if the xblock is a parentable xblock.
+             */
+            has_children: null,
+            /**
              * True if the xblock has changes.
              * Note: this is not always provided as a performance optimization. It is only provided for
              * verticals functioning as units.

cms/static/js/spec/views/pages/container_spec.js

@@ -49,6 +49,9 @@ define(['jquery', 'underscore', 'underscore.string', 'edx-ui-toolkit/js/utils/sp
 
                 afterEach(function() {
                     EditHelpers.uninstallMockXBlock();
+                    if (containerPage !== undefined) {
+                        containerPage.remove();
+                    }
                 });
 
                 respondWithHtml = function(html) {

cms/static/js/spec/views/pages/container_subviews_spec.js

@@ -35,6 +35,9 @@ define(['jquery', 'underscore', 'underscore.string', 'edx-ui-toolkit/js/utils/sp
 
             afterEach(function() {
                 delete window.course;
+                if (containerPage !== undefined) {
+                    containerPage.remove();
+                }
             });
 
             defaultXBlockInfo = {

cms/static/js/views/modals/move_xblock_modal.js

@@ -41,7 +41,6 @@ function($, Backbone, _, gettext, BaseView, XBlockViewUtils, MoveXBlockUtils, Ht
         initialize: function() {
             var self = this;
             BaseModal.prototype.initialize.call(this);
-            this.listenTo(Backbone, 'move:breadcrumbRendered', this.focusModal);
             this.sourceXBlockInfo = this.options.sourceXBlockInfo;
             this.sourceParentXBlockInfo = this.options.sourceParentXBlockInfo;
             this.targetParentXBlockInfo = null;
@@ -57,9 +56,9 @@ function($, Backbone, _, gettext, BaseView, XBlockViewUtils, MoveXBlockUtils, Ht
                 $('.breadcrumb-container').removeClass('is-hidden');
                 self.renderViews(courseOutlineInfo, ancestorInfo);
             });
-            this.movedAlertView = null;
-            this.isValidMove = false;
+            this.listenTo(Backbone, 'move:breadcrumbRendered', this.focusModal);
             this.listenTo(Backbone, 'move:enableMoveOperation', this.enableMoveOperation);
+            this.listenTo(Backbone, 'move:hideMoveModal', this.hide);
         },
 
         getTitle: function() {
@@ -137,15 +136,22 @@ function($, Backbone, _, gettext, BaseView, XBlockViewUtils, MoveXBlockUtils, Ht
             }
         },
 
-        isValidCategory: function(sourceParentType, targetParentType, targetHasChildren) {
-            var basicBlockTypes = ['course', 'chapter', 'sequential', 'vertical'];
+        isValidCategory: function(targetParentXBlockInfo) {
+            var basicBlockTypes = ['course', 'chapter', 'sequential', 'vertical'],
+                sourceParentType = this.sourceParentXBlockInfo.get('category'),
+                targetParentType = targetParentXBlockInfo.get('category'),
+                sourceParentHasChildren = this.sourceParentXBlockInfo.get('has_children'),
+                targetParentHasChildren = targetParentXBlockInfo.get('has_children');
+
             // Treat source parent component as vertical to support move child components under content experiment
             // and other similar xblocks.
-            // eslint-disable-next-line no-param-reassign
-            sourceParentType = sourceParentType === 'split_test' ? 'vertical' : sourceParentType;
+            if (sourceParentHasChildren && !_.contains(basicBlockTypes, sourceParentType)) {
+                sourceParentType = 'vertical';  // eslint-disable-line no-param-reassign
+            }
+
             // Treat target parent component as a vertical to support move to parentable target parent components.
             // Also, moving a component directly to content experiment is not allowed, we need to visit to group level.
-            if (targetHasChildren && !_.contains(basicBlockTypes, targetParentType) &&
+            if (targetParentHasChildren && !_.contains(basicBlockTypes, targetParentType) &&
                 targetParentType !== 'split_test') {
                 targetParentType = 'vertical';  // eslint-disable-line no-param-reassign
             }
@@ -153,44 +159,28 @@ function($, Backbone, _, gettext, BaseView, XBlockViewUtils, MoveXBlockUtils, Ht
         },
 
         enableMoveOperation: function(targetParentXBlockInfo) {
-            var isValidMove = false,
-                sourceParentType = this.sourceParentXBlockInfo.get('category'),
-                targetParentType = targetParentXBlockInfo.get('category'),
-                targetHasChildren = targetParentXBlockInfo.get('has_children');
+            var isValidMove = false;
 
-            if (this.isValidCategory(sourceParentType, targetParentType, targetHasChildren) &&
+            // update target parent on navigation
+            this.targetParentXBlockInfo = targetParentXBlockInfo;
+            if (this.isValidCategory(targetParentXBlockInfo) &&
                 this.sourceParentXBlockInfo.id !== targetParentXBlockInfo.id && // same parent case
                 this.sourceXBlockInfo.id !== targetParentXBlockInfo.id) { // same source item case
                 isValidMove = true;
-                this.targetParentXBlockInfo = targetParentXBlockInfo;
             }
             this.updateMoveState(isValidMove);
         },
 
         moveXBlock: function() {
-            var self = this;
-            XBlockViewUtils.moveXBlock(self.sourceXBlockInfo.id, self.targetParentXBlockInfo.id)
-            .done(function(response) {
-                // hide modal
-                self.hide();
-                // hide xblock element
-                $("li.studio-xblock-wrapper[data-locator='" + self.sourceXBlockInfo.id + "']").hide();
-                self.movedAlertView = MoveXBlockUtils.showMovedNotification(
-                    StringUtils.interpolate(
-                        gettext('Success! "{displayName}" has been moved.'),
-                        {
-                            displayName: self.sourceXBlockInfo.get('display_name')
-                        }
-                    ),
-                    {
-                        sourceDisplayName: self.sourceXBlockInfo.get('display_name'),
-                        sourceLocator: self.sourceXBlockInfo.id,
-                        sourceParentLocator: self.sourceParentXBlockInfo.id,
-                        targetParentLocator: response.parent_locator,
-                        targetIndex: response.source_index
-                    }
-                );
-            });
+            MoveXBlockUtils.moveXBlock(
+                {
+                    sourceXBlockElement: $("li.studio-xblock-wrapper[data-locator='" + this.sourceXBlockInfo.id + "']"),
+                    sourceDisplayName: this.sourceXBlockInfo.get('display_name'),
+                    sourceLocator: this.sourceXBlockInfo.id,
+                    sourceParentLocator: this.sourceParentXBlockInfo.id,
+                    targetParentLocator: this.targetParentXBlockInfo.id
+                }
+            );
         }
     });
 

cms/static/js/views/pages/container.js

@@ -2,11 +2,12 @@
  * XBlockContainerPage is used to display Studio's container page for an xblock which has children.
  * This page allows the user to understand and manipulate the xblock and its children.
  */
-define(['jquery', 'underscore', 'gettext', 'js/views/pages/base_page', 'common/js/components/utils/view_utils',
-        'js/views/container', 'js/views/xblock', 'js/views/components/add_xblock', 'js/views/modals/edit_xblock',
-        'js/views/modals/move_xblock_modal', 'js/models/xblock_info', 'js/views/xblock_string_field_editor',
-        'js/views/pages/container_subviews', 'js/views/unit_outline', 'js/views/utils/xblock_utils'],
-    function($, _, gettext, BasePage, ViewUtils, ContainerView, XBlockView, AddXBlockComponent,
+define(['jquery', 'underscore', 'backbone', 'gettext', 'js/views/pages/base_page',
+        'common/js/components/utils/view_utils', 'js/views/container', 'js/views/xblock',
+        'js/views/components/add_xblock', 'js/views/modals/edit_xblock', 'js/views/modals/move_xblock_modal',
+        'js/models/xblock_info', 'js/views/xblock_string_field_editor', 'js/views/pages/container_subviews',
+        'js/views/unit_outline', 'js/views/utils/xblock_utils'],
+    function($, _, Backbone, gettext, BasePage, ViewUtils, ContainerView, XBlockView, AddXBlockComponent,
               EditXBlockModal, MoveXBlockModal, XBlockInfo, XBlockStringFieldEditor, ContainerSubviews,
               UnitOutlineView, XBlockUtils) {
         'use strict';
@@ -81,6 +82,8 @@ define(['jquery', 'underscore', 'gettext', 'js/views/pages/base_page', 'common/j
                     });
                     this.unitOutlineView.render();
                 }
+
+                this.listenTo(Backbone, 'move:onXBlockMoved', this.onXBlockMoved);
             },
 
             getViewParameters: function() {
@@ -283,6 +286,13 @@ define(['jquery', 'underscore', 'gettext', 'js/views/pages/base_page', 'common/j
                 this.model.fetch();
             },
 
+            /*
+            After move operation is complete, updates the xblock information from server .
+             */
+            onXBlockMoved: function() {
+                this.model.fetch();
+            },
+
             onNewXBlock: function(xblockElement, scrollOffset, is_duplicate, data) {
                 ViewUtils.setScrollOffset(xblockElement, scrollOffset);
                 xblockElement.data('locator', data.locator);

cms/static/js/views/utils/move_xblock_utils.js

@@ -4,25 +4,53 @@
 define([
     'jquery',
     'underscore',
+    'backbone',
     'common/js/components/views/feedback',
     'common/js/components/views/feedback_alert',
     'js/views/utils/xblock_utils',
     'js/views/utils/move_xblock_utils',
     'edx-ui-toolkit/js/utils/string-utils'
 ],
-function($, _, Feedback, AlertView, XBlockViewUtils, MoveXBlockUtils, StringUtils) {
+function($, _, Backbone, Feedback, AlertView, XBlockViewUtils, MoveXBlockUtils, StringUtils) {
     'use strict';
-    var redirectLink, undoMoveXBlock, showMovedNotification, hideMovedNotification;
+    var redirectLink, moveXBlock, undoMoveXBlock, showMovedNotification, hideMovedNotification;
 
     redirectLink = function(link) {
         window.location.href = link;
     };
 
+    moveXBlock = function(data) {
+        XBlockViewUtils.moveXBlock(data.sourceLocator, data.targetParentLocator)
+        .done(function(response) {
+            // hide modal
+            Backbone.trigger('move:hideMoveModal');
+            // hide xblock element
+            data.sourceXBlockElement.hide();
+            showMovedNotification(
+                StringUtils.interpolate(
+                    gettext('Success! "{displayName}" has been moved.'),
+                    {
+                        displayName: data.sourceDisplayName
+                    }
+                ),
+                {
+                    sourceXBlockElement: data.sourceXBlockElement,
+                    sourceDisplayName: data.sourceDisplayName,
+                    sourceLocator: data.sourceLocator,
+                    sourceParentLocator: data.sourceParentLocator,
+                    targetParentLocator: data.targetParentLocator,
+                    targetIndex: response.source_index
+                }
+            );
+            Backbone.trigger('move:onXBlockMoved');
+        });
+    };
+
     undoMoveXBlock = function(data) {
         XBlockViewUtils.moveXBlock(data.sourceLocator, data.sourceParentLocator, data.targetIndex)
-        .done(function(response) {
+        .done(function() {
             // show XBlock element
-            $('.studio-xblock-wrapper[data-locator="' + response.move_source_locator + '"]').show();
+            data.sourceXBlockElement.show();
             showMovedNotification(
                 StringUtils.interpolate(
                     gettext('Move cancelled. "{sourceDisplayName}" has been moved back to its original location.'),
@@ -31,6 +59,7 @@ function($, _, Feedback, AlertView, XBlockViewUtils, MoveXBlockUtils, StringUtil
                     }
                 )
             );
+            Backbone.trigger('move:onXBlockMoved');
         });
     };
 
@@ -44,15 +73,10 @@ function($, _, Feedback, AlertView, XBlockViewUtils, MoveXBlockUtils, StringUtil
                     primary: {
                         text: gettext('Undo move'),
                         class: 'action-save',
-                        data: JSON.stringify({
-                            sourceDisplayName: data.sourceDisplayName,
-                            sourceLocator: data.sourceLocator,
-                            sourceParentLocator: data.sourceParentLocator,
-                            targetIndex: data.targetIndex
-                        }),
                         click: function() {
                             undoMoveXBlock(
                                 {
+                                    sourceXBlockElement: data.sourceXBlockElement,
                                     sourceDisplayName: data.sourceDisplayName,
                                     sourceLocator: data.sourceLocator,
                                     sourceParentLocator: data.sourceParentLocator,
@@ -65,9 +89,6 @@ function($, _, Feedback, AlertView, XBlockViewUtils, MoveXBlockUtils, StringUtil
                         {
                             text: gettext('Take me to the new location'),
                             class: 'action-cancel',
-                            data: JSON.stringify({
-                                targetParentLocator: data.targetParentLocator
-                            }),
                             click: function() {
                                 redirectLink('/container/' + data.targetParentLocator);
                             }
@@ -100,6 +121,8 @@ function($, _, Feedback, AlertView, XBlockViewUtils, MoveXBlockUtils, StringUtil
 
     return {
         redirectLink: redirectLink,
+        moveXBlock: moveXBlock,
+        undoMoveXBlock: undoMoveXBlock,
         showMovedNotification: showMovedNotification,
         hideMovedNotification: hideMovedNotification
     };

cms/static/js/views/utils/xblock_utils.js

@@ -272,7 +272,8 @@ define(['jquery', 'underscore', 'gettext', 'common/js/components/utils/view_util
         findXBlockInfo = function(xblockWrapperElement, defaultXBlockInfo) {
             var xblockInfo = defaultXBlockInfo,
                 xblockElement,
-                displayName;
+                displayName,
+                hasChildren;
             if (xblockWrapperElement.length > 0) {
                 xblockElement = xblockWrapperElement.find('.xblock');
                 displayName = xblockWrapperElement.find(
@@ -283,11 +284,13 @@ define(['jquery', 'underscore', 'gettext', 'common/js/components/utils/view_util
                 if (!displayName) {
                     displayName = xblockElement.find('.component-header').text().trim();
                 }
+                hasChildren = defaultXBlockInfo ? defaultXBlockInfo.get('has_children') : false;
                 xblockInfo = new XBlockInfo({
                     id: xblockWrapperElement.data('locator'),
                     courseKey: xblockWrapperElement.data('course-key'),
                     category: xblockElement.data('block-type'),
-                    display_name: displayName
+                    display_name: displayName,
+                    has_children: hasChildren
                 });
             }
             return xblockInfo;

common/test/acceptance/pages/studio/container.py

@@ -229,6 +229,18 @@ class ContainerPage(PageObject, HelpMixin):
         self.q(css='.button-view').first.click()
         self._switch_to_lms()
 
+    def verify_publish_title(self, expected_title):
+        """
+        Waits for the publish title to change to the expected value.
+        """
+        def wait_for_title_change():
+            """
+            Promise function to check publish title.
+            """
+            return (self.publish_title == expected_title, self.publish_title)
+
+        Promise(wait_for_title_change, "Publish title incorrect. Found '" + self.publish_title + "'").fulfill()
+
     def preview(self):
         """
         Clicks "Preview", which will open the draft version of the unit page in the LMS.