diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 5e902c5898b3..d5e2f7febf6f 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -729,20 +729,20 @@ def _move_item(source_usage_key, target_parent_usage_key, user, target_index=Non if (valid_move_type.get(target_parent_type, '') != source_type and target_parent_type not in parent_component_types): - error = 'You can not move {source_type} into {target_parent_type}.'.format( + 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.' + error = _('You can not move an item into the same parent.') elif source_item.location == target_parent.location: - error = 'You can not move an item into itself.' + 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.' + 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.' + 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( + 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) ) @@ -750,12 +750,12 @@ def _move_item(source_usage_key, target_parent_usage_key, user, target_index=Non 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( + 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( + error = _('You must provide target_index ({target_index}) as an integer.').format( target_index=target_index ) if error: @@ -772,6 +772,14 @@ def _move_item(source_usage_key, target_parent_usage_key, user, target_index=Non target_parent.children.insert(insert_at, source_item.location) store.update_item(target_parent, user.id) + log.info( + 'MOVE: %s moved from %s to %s at %d index', + unicode(source_usage_key), + unicode(source_parent.location), + unicode(target_parent_usage_key), + insert_at + ) + context = { 'move_source_locator': unicode(source_usage_key), 'parent_locator': unicode(target_parent_usage_key), diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 28e2b5f11f25..3e28f0e20595 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -1126,6 +1126,24 @@ def test_no_move_source_locator(self): response = json.loads(response.content) self.assertEqual(response['error'], 'Patch request did not recognise any parameters to handle.') + @patch('contentstore.views.item.log') + def test_move_logging(self, mock_logger): + """ + Test logging when an item is successfully moved. + + Arguments: + mock_logger (object): A mock logger object. + """ + insert_at = 0 + self.assert_move_item(self.html_usage_key, self.vert2_usage_key, insert_at) + mock_logger.info.assert_called_with( + 'MOVE: %s moved from %s to %s at %d index', + unicode(self.html_usage_key), + unicode(self.vert_usage_key), + unicode(self.vert2_usage_key), + insert_at + ) + class TestDuplicateItemWithAsides(ItemTest, DuplicateHelper): """ diff --git a/cms/static/js/views/pages/container_subviews.js b/cms/static/js/views/pages/container_subviews.js index c3dabf3a754d..0a0e028b23ce 100644 --- a/cms/static/js/views/pages/container_subviews.js +++ b/cms/static/js/views/pages/container_subviews.js @@ -2,10 +2,11 @@ * Subviews (usually small side panels) for XBlockContainerPage. */ define(['jquery', 'underscore', 'gettext', 'js/views/baseview', 'common/js/components/utils/view_utils', - 'js/views/utils/xblock_utils'], - function($, _, gettext, BaseView, ViewUtils, XBlockViewUtils) { - var VisibilityState = XBlockViewUtils.VisibilityState, - disabledCss = 'is-disabled'; + 'js/views/utils/xblock_utils', 'js/views/utils/move_xblock_utils'], + function($, _, gettext, BaseView, ViewUtils, XBlockViewUtils, MoveXBlockUtils) { + 'use strict'; + + var disabledCss = 'is-disabled'; /** * A view that refreshes the view when certain values in the XBlockInfo have changed @@ -132,6 +133,8 @@ define(['jquery', 'underscore', 'gettext', 'js/views/baseview', 'common/js/compo return xblockInfo.save({publish: 'make_public'}, {patch: true}); }).always(function() { xblockInfo.set('publish', null); + // Hide any move notification if present. + MoveXBlockUtils.hideMovedNotification(); }).done(function() { xblockInfo.fetch(); }); @@ -151,6 +154,8 @@ define(['jquery', 'underscore', 'gettext', 'js/views/baseview', 'common/js/compo return xblockInfo.save({publish: 'discard_changes'}, {patch: true}); }).always(function() { xblockInfo.set('publish', null); + // Hide any move notification if present. + MoveXBlockUtils.hideMovedNotification(); }).done(function() { renderPage(); }); diff --git a/common/test/acceptance/pages/studio/container.py b/common/test/acceptance/pages/studio/container.py index 99d1e41fa892..092346967ccf 100644 --- a/common/test/acceptance/pages/studio/container.py +++ b/common/test/acceptance/pages/studio/container.py @@ -286,16 +286,18 @@ def edit(self): """ return _click_edit(self, '.edit-button', '.xblock-studio_view') - def verify_confirmation_message(self, message): + def verify_confirmation_message(self, message, verify_hidden=False): """ - Verify for confirmation message. + Verify for confirmation message is present or hidden. """ def _verify_message(): """ promise function to check confirmation message state """ text = self.q(css='#page-alert .alert.confirmation #alert-confirmation-title').text - return text and message in text[0] + return text and message not in text[0] if verify_hidden else text and message in text[0] - self.wait_for(_verify_message, description='confirmation message present') + self.wait_for(_verify_message, description='confirmation message {status}'.format( + status='hidden' if verify_hidden else 'present' + )) def click_undo_move_link(self): """ diff --git a/common/test/acceptance/tests/studio/test_studio_container.py b/common/test/acceptance/tests/studio/test_studio_container.py index ab3ae1e9283c..eb74e9854e3a 100644 --- a/common/test/acceptance/tests/studio/test_studio_container.py +++ b/common/test/acceptance/tests/studio/test_studio_container.py @@ -17,7 +17,7 @@ from common.test.acceptance.tests.helpers import create_user_partition_json import datetime -from bok_choy.promise import Promise, EmptyPromise +import ddt from base_studio_test import ContainerBase from xmodule.partitions.partitions import Group @@ -1131,6 +1131,7 @@ def test_common_problem_types_tab(self): @attr(shard=1) +@ddt.ddt class MoveComponentTest(ContainerBase): """ Tests of moving an XBlock to another XBlock. @@ -1220,6 +1221,30 @@ def verify_move_opertions(self, unit_page, source_component, operation, componen component_display_names_after_operation ) + def verify_state_change(self, unit_page, operation): + """ + Verify that after state change, confirmation message is hidden. + + Arguments: + unit_page (Object) Unit container page. + operation (String) Publish or discard changes operation. + """ + # Verify unit in draft state now + self.container.verify_publish_title(self.DRAFT_STATUS) + + # Now click publish/discard button + if operation == 'publish': + unit_page.publish_action.click() + else: + unit_page.discard_changes() + + # Now verify success message is hidden + self.container.verify_publish_title(self.PUBLISHED_LIVE_STATUS) + self.container.verify_confirmation_message( + message=self.message_move.format(display_name=self.source_component_display_name), + verify_hidden=True + ) + def test_move_component_successfully(self): """ Test if we can move a component successfully. @@ -1268,6 +1293,40 @@ def test_undo_move_component_successfully(self): component_display_names_after_operation=['HTML 11', 'HTML 12'] ) + @ddt.data('publish', 'discard') + def test_publish_discard_changes_afer_move(self, operation): + """ + Test if success banner is hidden when we discard changes or publish the unit after a move operation. + + Given I am a staff user + And I go to unit page in first section + And I open the move modal + And I navigate to unit in second section + And I see move button is enabled + When I click on the move button + Then I see move operation success message + And When I click on publish or discard changes button + Then I see move operation success message is hidden. + """ + unit_page = self.go_to_unit_page(unit_name='Test Unit 1') + components = unit_page.displayed_children + self.assertEqual(len(components), 2) + + components[0].open_move_modal() + self.move_modal_view.navigate_to_category(self.source_xblock_category, self.navigation_options) + self.assertEqual(self.move_modal_view.is_move_button_enabled, True) + + # Verify unit is in published state before move operation + self.container.verify_publish_title(self.PUBLISHED_LIVE_STATUS) + + self.move_modal_view.click_move_button() + self.container.verify_confirmation_message( + self.message_move.format(display_name=self.source_component_display_name) + ) + self.assertEqual(len(unit_page.displayed_children), 1) + + self.verify_state_change(unit_page, operation) + def test_content_experiment(self): """ Test if we can move a component of content experiment successfully.