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
17 changes: 17 additions & 0 deletions cms/djangoapps/contentstore/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,23 @@ def reverse_usage_url(handler_name, usage_key, kwargs=None):
return reverse_url(handler_name, 'usage_key_string', usage_key, kwargs)


def get_group_display_name(user_partitions, xblock_display_name):
Copy link

@cahrens cahrens Mar 16, 2017

Choose a reason for hiding this comment

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

@mushtaqak @muzaffaryousaf This is causing a problem for me on a PR I'm working on, and it would appear to be a general bug due to the implementation below. group['id'] is an integer; it can be any integer. xblock_display_name can contain any text. It is quite possible that a xblock's display name contains the integer of a group['id'], without them being related in any way.

As a concrete example, I am adding a new UserPartition based on enrollment track. The group IDs related to this partition will be small number (1, 2, 3, etc.). It's quite possible that an xblock's display name will contain those integers, and in fact, I hit failing tests that pointed out this issue (thank goodness for tests!).

Can you guys look into this tomorrow? I won't be able to merge my PR (#14682) until this is changed. For tracking purposes, I have created https://openedx.atlassian.net/browse/TNL-6731.

"""
Get the group name if matching group xblock is found.

Arguments:
user_partitions (Dict): Locator of source item.
xblock_display_name (String): Display name of group xblock.

Returns:
group name (String): Group name of the matching group.
"""
for user_partition in user_partitions:
for group in user_partition['groups']:
if str(group['id']) in xblock_display_name:
return group['name']


def get_user_partition_info(xblock, schemes=None, course=None):
"""
Retrieve user partition information for an XBlock for display in editors.
Expand Down
39 changes: 33 additions & 6 deletions cms/djangoapps/contentstore/views/item.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
from contentstore.utils import (
find_release_date_source, find_staff_lock_source, is_currently_visible_to_students,
ancestor_has_staff_lock, has_children_visible_to_specific_content_groups,
get_user_partition_info,
get_user_partition_info, get_group_display_name,
)
from contentstore.views.helpers import is_unit, xblock_studio_url, xblock_primary_child_category, \
xblock_type_display_name, get_parent_xblock, create_xblock, usage_key_with_run
Expand Down Expand Up @@ -675,6 +675,21 @@ def _get_source_index(source_usage_key, source_parent):
return None


def is_source_item_in_target_parents(source_item, target_parent):
"""
Returns True if source item is found in target parents otherwise False.

Arguments:
source_item (XBlock): Source Xblock.
target_parent (XBlock): Target XBlock.
"""
target_ancestors = _create_xblock_ancestor_info(target_parent, is_concise=True)['ancestors']
for target_ancestor in target_ancestors:
if unicode(source_item.location) == target_ancestor['id']:
return True
return False


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.
Expand All @@ -688,8 +703,11 @@ def _move_item(source_usage_key, target_parent_usage_key, user, target_index=Non
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))
# Get the list of all parentable component type XBlocks.
parent_component_types = list(
set(name for name, class_ in XBlock.load_classes() if getattr(class_, 'has_children', False)) -
set(DIRECT_ONLY_CATEGORIES)
)

store = modulestore()
with store.bulk_operations(source_usage_key.course_key):
Expand All @@ -705,18 +723,22 @@ def _move_item(source_usage_key, target_parent_usage_key, user, target_index=Non
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:
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(
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_item.location == target_parent.location:
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 source_index is None:
error = '{source_usage_key} not found in {parent_usage_key}.'.format(
source_usage_key=unicode(source_usage_key),
Expand Down Expand Up @@ -1093,6 +1115,7 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
# a percent value out of 100, e.g. "58%" means "58/100".
pct_sign=_('%'))

user_partitions = get_user_partition_info(xblock, course=course)
xblock_info = {
'id': unicode(xblock.location),
'display_name': xblock.display_name_with_default,
Expand All @@ -1101,6 +1124,10 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
if is_concise:
if child_info and len(child_info.get('children', [])) > 0:
xblock_info['child_info'] = child_info
# 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,
Expand All @@ -1121,7 +1148,7 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F
'actions': xblock_actions,
'explanatory_message': explanatory_message,
'group_access': xblock.group_access,
'user_partitions': get_user_partition_info(xblock, course=course),
'user_partitions': user_partitions,
})

if xblock.category == 'sequential':
Expand Down
1 change: 1 addition & 0 deletions cms/djangoapps/contentstore/views/preview.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False):
'can_edit': context.get('can_edit', True),
'can_edit_visibility': context.get('can_edit_visibility', True),
'can_add': context.get('can_add', True),
'can_move': context.get('can_move', True)

Choose a reason for hiding this comment

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

I'd like to see test coverage for this case.

}
html = render_to_string('studio_xblock_wrapper.html', template_context)
frag = wrap_fragment(frag, html)
Expand Down
44 changes: 42 additions & 2 deletions cms/djangoapps/contentstore/views/tests/test_container_page.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@

import contentstore.views.component as views
from contentstore.views.tests.utils import StudioPageTestCase
from contentstore.tests.test_libraries import LibraryTestCase
from xmodule.modulestore import ModuleStoreEnum
from xmodule.modulestore.django import modulestore
from xmodule.modulestore.tests.factories import ItemFactory
from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory


class ContainerPageTestCase(StudioPageTestCase):
class ContainerPageTestCase(StudioPageTestCase, LibraryTestCase):
"""
Unit tests for the container page.
"""
Expand Down Expand Up @@ -128,6 +130,44 @@ def test_public_container_preview_html(self):
self.validate_preview_html(published_child_container, self.container_view)
self.validate_preview_html(published_child_vertical, self.reorderable_child_view)

def test_library_page_preview_html(self):
"""
Verify that a library xblock's container (library page) preview returns the expected HTML.
"""
# Add some content to library.
self._add_simple_content_block()
self.validate_preview_html(self.library, self.container_view, can_reorder=False, can_move=False)

def test_library_content_preview_html(self):
"""
Verify that a library content block container page preview returns the expected HTML.
"""
# Library content block is only supported in split courses.
with modulestore().default_store(ModuleStoreEnum.Type.split):
course = CourseFactory.create()

# Add some content to library
self._add_simple_content_block()

# Create a library content block
lc_block = self._add_library_content_block(course, self.lib_key)
self.assertEqual(len(lc_block.children), 0)

# Refresh children to be reflected in lc_block
lc_block = self._refresh_children(lc_block)
self.assertEqual(len(lc_block.children), 1)

self.validate_preview_html(
lc_block,
self.container_view,
can_add=False,
can_reorder=False,
can_move=False,
can_edit=True,
can_duplicate=False,
can_delete=False
)

def test_draft_container_preview_html(self):
"""
Verify that a draft xblock's container preview returns the expected HTML.
Expand Down
Loading