From cb2d78bf34d94eb019cd0dd8ffc983558e945a4b Mon Sep 17 00:00:00 2001 From: Mushtaq Ali Date: Thu, 9 Mar 2017 15:38:26 +0500 Subject: [PATCH] Add logging on move xblock Add i18n to backend error messages in case they appear to end user Remove success banner when trying to publish or discard changes on container page --- cms/djangoapps/contentstore/views/item.py | 24 +++++--- .../contentstore/views/tests/test_item.py | 18 ++++++ .../js/views/pages/container_subviews.js | 13 ++-- .../test/acceptance/pages/studio/container.py | 10 +-- .../tests/studio/test_studio_container.py | 61 ++++++++++++++++++- 5 files changed, 109 insertions(+), 17 deletions(-) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 5e902c5898..d5e2f7febf 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 28e2b5f11f..3e28f0e205 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -1126,6 +1126,24 @@ class TestMoveItem(ItemTest): 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 c3dabf3a75..0a0e028b23 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 99d1e41fa8..092346967c 100644 --- a/common/test/acceptance/pages/studio/container.py +++ b/common/test/acceptance/pages/studio/container.py @@ -286,16 +286,18 @@ class ContainerPage(PageObject, HelpMixin): """ 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 ab3ae1e928..eb74e9854e 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.pages.lms.staff_view import StaffPage 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 @@ class ProblemCategoryTabsTest(ContainerBase): @attr(shard=1) +@ddt.ddt class MoveComponentTest(ContainerBase): """ Tests of moving an XBlock to another XBlock. @@ -1220,6 +1221,30 @@ class MoveComponentTest(ContainerBase): 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 @@ class MoveComponentTest(ContainerBase): 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.