Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions cms/djangoapps/contentstore/views/item.py
Original file line number Diff line number Diff line change
Expand Up @@ -729,33 +729,33 @@ 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)
)
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(
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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mushtaqak My only concern is that some of the above messages are not user friendly. We can release the changes now but we should create a new ticket to improve the log messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

target_index=target_index
)
if error:
Expand All @@ -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),
Expand Down
18 changes: 18 additions & 0 deletions cms/djangoapps/contentstore/views/tests/test_item.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand Down
13 changes: 9 additions & 4 deletions cms/static/js/views/pages/container_subviews.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VisibilityState is not used in the file


var disabledCss = 'is-disabled';

/**
* A view that refreshes the view when certain values in the XBlockInfo have changed
Expand Down Expand Up @@ -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();
});
Expand All @@ -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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add coverage for this action ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will see where we can add this, may be in bokchoy.

}).done(function() {
renderPage();
});
Expand Down
10 changes: 6 additions & 4 deletions common/test/acceptance/pages/studio/container.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand Down
61 changes: 60 additions & 1 deletion common/test/acceptance/tests/studio/test_studio_container.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operation argument is missing docs

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.
Expand Down Expand Up @@ -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.
Expand Down