Skip to content

E
edx-platform
Commit 2ca86a8d by Oleg Marshev
Options

Merge pull request #4735 from edx/oleg/validate-content-experiments 

BLD-1125: Validate content experiments after managing a group.
parents 6dd9d9ec a94e9424
Showing with 416 additions and 42 deletions
cms/djangoapps/contentstore/views/course.py
......@@ -106,7 +106,7 @@ def course_notifications_handler(request, course_key_string=None, action_state_i
Handle incoming requests for notifications in a RESTful way.
course_key_string and action_state_id must both be set; else a HttpBadResponseRequest is returned.
For each of these operations, the requesting user must have access to the course;
else a PermissionDenied error is returned.
......@@ -1123,20 +1123,49 @@ class GroupConfiguration(object):
@staticmethod
def get_usage_info(course, store):
"""
Get all units names and their urls that have experiments and associated
with configurations.
Get usage information for all Group Configurations.
"""
split_tests = store.get_items(course.id, qualifiers={'category': 'split_test'})
return GroupConfiguration._get_usage_info(store, course, split_tests)
@staticmethod
def add_usage_info(course, store):
"""
Add usage information to group configurations jsons in course.
Returns json of group configurations updated with usage information.
"""
usage_info = GroupConfiguration.get_usage_info(course, store)
configurations = []
for partition in course.user_partitions:
configuration = partition.to_json()
configuration['usage'] = usage_info.get(partition.id, [])
configurations.append(configuration)
return configurations
@staticmethod
def _get_usage_info(store, course, split_tests):
"""
Returns all units names, their urls and validation messages.
Returns:
{'user_partition_id':
[
{'label': 'Unit Name / Experiment Name', 'url': 'url_to_unit_1'},
{'label': 'Another Unit Name / Another Experiment Name', 'url': 'url_to_unit_1'}
{
'label': 'Unit 1 / Experiment 1',
'url': 'url_to_unit_1',
'validation': {'message': 'a validation message', 'type': 'warning'}
},
{
'label': 'Unit 2 / Experiment 2',
'url': 'url_to_unit_2',
'validation': {'message': 'another validation message', 'type': 'error'}
}
],
}
"""
usage_info = {}
descriptors = store.get_items(course.id, qualifiers={'category': 'split_test'})
for split_test in descriptors:
for split_test in split_tests:
if split_test.user_partition_id not in usage_info:
usage_info[split_test.user_partition_id] = []
......@@ -1157,24 +1186,28 @@ class GroupConfiguration(object):
)
usage_info[split_test.user_partition_id].append({
'label': '{} / {}'.format(unit.display_name, split_test.display_name),
'url': unit_url
'url': unit_url,
'validation': split_test.general_validation_message,
})
return usage_info
@staticmethod
def add_usage_info(course, store):
def update_usage_info(store, course, configuration):
"""
Add usage information to group configurations json.
Update usage information for particular Group Configuration.
Returns json of group configurations updated with usage information.
Returns json of particular group configuration updated with usage information.
"""
usage_info = GroupConfiguration.get_usage_info(course, store)
configurations = []
for partition in course.user_partitions:
configuration = partition.to_json()
configuration['usage'] = usage_info.get(partition.id, [])
configurations.append(configuration)
return configurations
# Get all Experiments that use particular Group Configuration in course.
split_tests = store.get_items(
course.id,
category='split_test',
content={'user_partition_id': configuration.id}
)
configuration_json = configuration.to_json()
usage_information = GroupConfiguration._get_usage_info(store, course, split_tests)
configuration_json['usage'] = usage_information.get(configuration.id, [])
return configuration_json
@require_http_methods(("GET", "POST"))
......@@ -1262,7 +1295,8 @@ def group_configurations_detail_handler(request, course_key_string, group_config
else:
course.user_partitions.append(new_configuration)
store.update_item(course, request.user.id)
return JsonResponse(new_configuration.to_json(), status=201)
configuration = GroupConfiguration.update_usage_info(store, course, new_configuration)
return JsonResponse(configuration, status=201)
elif request.method == "DELETE":
if not configuration:
return JsonResponse(status=404)
......
cms/static/js/spec/views/group_configuration_spec.js
......@@ -33,7 +33,12 @@ define([
usageText: '.group-configuration-usage-text',
usageTextAnchor: '.group-configuration-usage-text > a',
usageUnit: '.group-configuration-usage-unit',
usageUnitAnchor: '.group-configuration-usage-unit > a',
usageUnitAnchor: '.group-configuration-usage-unit a',
usageUnitMessage: '.group-configuration-validation-message',
usageUnitWarningIcon: '.group-configuration-usage-unit i.icon-warning-sign',
usageUnitErrorIcon: '.group-configuration-usage-unit i.icon-exclamation-sign',
warningMessage: '.group-configuration-validation-text',
warningIcon: '.wrapper-group-configuration-validation > i',
note: '.wrapper-delete-button'
};
......@@ -162,12 +167,10 @@ define([
it('should show non-empty usage appropriately', function() {
var usageUnitAnchors;
this.model.set('usage',
[
{'label': 'label1', 'url': 'url1'},
{'label': 'label2', 'url': 'url2'}
]
);
this.model.set('usage', [
{'label': 'label1', 'url': 'url1'},
{'label': 'label2', 'url': 'url2'}
]);
this.model.set('showGroups', false);
this.view.$('.show-groups').click();
......@@ -205,6 +208,55 @@ define([
expect(this.view.$(SELECTORS.usageCount))
.toContainText('Used in 2 units');
});
it('should show validation warning icon and message appropriately', function() {
this.model.set('usage', [
{
'label': 'label1',
'url': 'url1',
'validation': {
'message': "Warning message",
'type': 'warning'
}
}
]);
this.model.set('showGroups', false);
this.view.$('.show-groups').click();
expect(this.view.$(SELECTORS.usageUnitMessage)).toContainText('Warning message');
expect(this.view.$(SELECTORS.usageUnitWarningIcon)).toExist();
});
it('should show validation error icon and message appropriately', function() {
this.model.set('usage', [
{
'label': 'label1',
'url': 'url1',
'validation': {
'message': "Error message",
'type': 'error'
}
}
]);
this.model.set('showGroups', false);
this.view.$('.show-groups').click();
expect(this.view.$(SELECTORS.usageUnitMessage)).toContainText('Error message');
expect(this.view.$(SELECTORS.usageUnitErrorIcon)).toExist();
});
it('should hide validation icons and messages appropriately', function() {
this.model.set('usage', [
{'label': 'label1', 'url': 'url1'},
{'label': 'label2', 'url': 'url2'}
]);
this.model.set('showGroups', true);
this.view.$('.hide-groups').click();
expect(this.view.$(SELECTORS.usageUnitMessage)).not.toExist();
expect(this.view.$(SELECTORS.usageUnitWarningIcon)).not.toExist();
expect(this.view.$(SELECTORS.usageUnitErrorIcon)).not.toExist();
});
});
describe('GroupConfigurationEdit', function() {
......@@ -418,6 +470,24 @@ define([
);
expect(this.view.$('.delete')).toHaveClass('is-disabled');
});
it('contains warning message if it is in use', function () {
this.model.set('usage', [ {'label': 'label1', 'url': 'url1'} ]);
this.view.render();
expect(this.view.$(SELECTORS.warningMessage)).toContainText(
'This configuration is currently used in content ' +
'experiments. If you make changes to the groups, you may ' +
'need to edit those experiments.'
);
expect(this.view.$(SELECTORS.warningIcon)).toExist();
});
it('does not contain warning message if it is not in use', function () {
this.model.set('usage', []);
this.view.render();
expect(this.view.$(SELECTORS.warningMessage)).not.toExist();
expect(this.view.$(SELECTORS.warningIcon)).not.toExist();
});
});
describe('GroupConfigurationsList', function() {
......
cms/static/sass/views/_group-configuration.scss
......@@ -190,21 +190,51 @@
.wrapper-group-configuration-usages {
@extend %t-copy-sub1;
background-color: #f8f8f8;
box-shadow: 0 2px 2px 0 $shadow inset;
padding: $baseline ($baseline*1.5) $baseline ($baseline*2.5);
color: $gray-l1;
.group-configuration-usage {
color: $gray-l1;
margin-left: $baseline;
.group-configuration-usage-unit {
padding: ($baseline/4) 0;
a {
font-weight: 600;
}
.icon-warning-sign {
margin: ($baseline/4) ($baseline/2) 0 ($baseline*1.5);
color: $orange;
}
.icon-exclamation-sign {
margin: ($baseline/4) ($baseline/2) 0 ($baseline*1.5);
color: $red-l2;
}
}
}
}
}
.wrapper-group-configuration-validation {
@extend %t-copy-sub1;
background-color: #f8f8f8;
margin-top: $baseline;
padding: $baseline ($baseline*1.5) $baseline ($baseline*1.5);
.icon-warning-sign {
margin: ($baseline/2) $baseline 0 0;
color: $orange;
float: left;
}
.group-configuration-validation-text {
overflow: auto;
}
}
.group-configuration-edit {
@include box-sizing(border-box);
border-radius: 2px;
......
cms/templates/js/group-configuration-details.underscore
......@@ -62,7 +62,19 @@
<ol class="group-configuration-usage">
<% _.each(usage, function(unit) { %>
<li class="group-configuration-usage-unit">
<a href=<%= unit.url %> ><%= unit.label %></a>
<p><a href=<%= unit.url %> ><%= unit.label %></a></p>
<% if (unit.validation) { %>
<p>
<% if (unit.validation.type === 'warning') { %>
<i class="icon-warning-sign"></i>
<% } else if (unit.validation.type === 'error') { %>
<i class="icon-exclamation-sign"></i>
<% } %>
<span class="group-configuration-validation-message">
<%= unit.validation.message %>
</span>
</p>
<% } %>
</li>
<% }) %>
</ol>
......
cms/templates/js/group-configuration-edit.underscore
......@@ -32,6 +32,14 @@
<ol class="groups list-input enum"></ol>
<button class="action action-add-group"><i class="icon-plus"></i> <%= gettext("Add another group") %></button>
</fieldset>
<% if (!_.isEmpty(usage)) { %>
<div class="wrapper-group-configuration-validation">
<i class="icon-warning-sign"></i>
<p class="group-configuration-validation-text">
<%= gettext('This configuration is currently used in content experiments. If you make changes to the groups, you may need to edit those experiments.') %>
</p>
</div>
<% } %>
</div>
<div class="actions">
<button class="action action-primary" type="submit"><% if (isNew) { print(gettext("Create")) } else { print(gettext("Save")) } %></button>
......
common/lib/xmodule/xmodule/split_test_module.py
......@@ -607,3 +607,19 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor, StudioEditableDes
)
self.children.append(dest_usage_key) # pylint: disable=no-member
self.group_id_to_child[unicode(group.id)] = dest_usage_key
@property
def general_validation_message(self):
"""
Message for either error or warning validation message/s.
Returns message and type. Priority given to error type message.
"""
validation_messages = self.validation_messages()
if validation_messages:
has_error = any(message.message_type == ValidationMessageType.error for message in validation_messages)
return {
'message': _(u"This content experiment has issues that affect content visibility."),
'type': ValidationMessageType.error if has_error else ValidationMessageType.warning,
}
return None
common/lib/xmodule/xmodule/tests/test_split_test_module.py
......@@ -344,6 +344,13 @@ class SplitTestModuleStudioTest(SplitTestModuleTest):
self.assertEqual(message.action_class, expected_action_class)
self.assertEqual(message.action_label, expected_action_label)
def verify_general_validation_message(general_validation, expected_message, expected_message_type):
"""
Verify that the general validation message has the expected validation message and type.
"""
self.assertEqual(unicode(general_validation['message']), expected_message)
self.assertEqual(general_validation['type'], expected_message_type)
# Verify the messages for an unconfigured user partition
split_test_module.user_partition_id = -1
messages = split_test_module.validation_messages()
......@@ -355,6 +362,11 @@ class SplitTestModuleStudioTest(SplitTestModuleTest):
'edit-button',
u"Select a Group Configuration",
)
verify_general_validation_message(
split_test_module.general_validation_message,
u"This content experiment has issues that affect content visibility.",
ValidationMessageType.warning
)
# Verify the messages for a correctly configured split_test
split_test_module.user_partition_id = 0
......@@ -363,6 +375,7 @@ class SplitTestModuleStudioTest(SplitTestModuleTest):
]
messages = split_test_module.validation_messages()
self.assertEqual(len(messages), 0)
self.assertIsNone(split_test_module.general_validation_message, None)
# Verify the messages for a split test with too few groups
split_test_module.user_partitions = [
......@@ -378,7 +391,11 @@ class SplitTestModuleStudioTest(SplitTestModuleTest):
'add-missing-groups-button',
u"Add Missing Groups"
)
verify_general_validation_message(
split_test_module.general_validation_message,
u"This content experiment has issues that affect content visibility.",
ValidationMessageType.error
)
# Verify the messages for a split test with children that are not associated with any group
split_test_module.user_partitions = [
UserPartition(0, 'first_partition', 'First Partition',
......@@ -391,7 +408,11 @@ class SplitTestModuleStudioTest(SplitTestModuleTest):
u"The experiment has an inactive group. Move content into active groups, then delete the inactive group.",
ValidationMessageType.warning
)
verify_general_validation_message(
split_test_module.general_validation_message,
u"This content experiment has issues that affect content visibility.",
ValidationMessageType.warning
)
# Verify the messages for a split test with both missing and inactive children
split_test_module.user_partitions = [
UserPartition(0, 'first_partition', 'First Partition',
......@@ -411,6 +432,12 @@ class SplitTestModuleStudioTest(SplitTestModuleTest):
u"The experiment has an inactive group. Move content into active groups, then delete the inactive group.",
ValidationMessageType.warning
)
# With two messages of type error and warning priority given to error.
verify_general_validation_message(
split_test_module.general_validation_message,
u"This content experiment has issues that affect content visibility.",
ValidationMessageType.error
)
# Verify the messages for a split test referring to a non-existent user partition
split_test_module.user_partition_id = 2
......@@ -422,3 +449,8 @@ class SplitTestModuleStudioTest(SplitTestModuleTest):
u"Select a valid group configuration or delete this experiment.",
ValidationMessageType.error
)
verify_general_validation_message(
split_test_module.general_validation_message,
u"This content experiment has issues that affect content visibility.",
ValidationMessageType.error
)
common/test/acceptance/pages/studio/settings_group_configurations.py
......@@ -30,6 +30,14 @@ class GroupConfigurationsPage(CoursePage):
"""
self.q(css=".new-button").first.click()
@property
def no_group_configuration_message_is_present(self):
return self.q(css='.wrapper-content .no-group-configurations-content').present
@property
def no_group_configuration_message_text(self):
return self.q(css='.wrapper-content .no-group-configurations-content').text[0]
class GroupConfiguration(object):
"""
......@@ -198,6 +206,34 @@ class GroupConfiguration(object):
"""
return self.find_css('.wrapper-delete-button').first.attrs('data-tooltip')[0]
@property
def details_error_icon_is_present(self):
return self.find_css('.wrapper-group-configuration-usages .icon-exclamation-sign').present
@property
def details_warning_icon_is_present(self):
return self.find_css('.wrapper-group-configuration-usages .icon-warning-sign').present
@property
def details_message_is_present(self):
return self.find_css('.wrapper-group-configuration-usages .group-configuration-validation-message').present
@property
def details_message_text(self):
return self.find_css('.wrapper-group-configuration-usages .group-configuration-validation-message').text[0]
@property
def edit_warning_icon_is_present(self):
return self.find_css('.wrapper-group-configuration-validation .icon-warning-sign').present
@property
def edit_warning_message_is_present(self):
return self.find_css('.wrapper-group-configuration-validation .group-configuration-validation-text').present
@property
def edit_warning_message_text(self):
return self.find_css('.wrapper-group-configuration-validation .group-configuration-validation-text').text[0]
def __repr__(self):
return "<{}:{}>".format(self.__class__.__name__, self.name)
......
common/test/acceptance/tests/test_studio_split_test.py
......@@ -301,6 +301,33 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin):
)
)
def create_group_configuration_experiment(self, groups, associate_experiment):
"""
Creates a Group Configuration containing a list of groups.
Optionally creates a Content Experiment and associates it with previous Group Configuration.
"""
# Create a new group configurations
self.course_fixture._update_xblock(self.course_fixture._course_location, {
"metadata": {
u"user_partitions": [
UserPartition(0, "Name", "Description.", groups).to_json(),
],
},
})
if associate_experiment:
# Assign newly created group configuration to experiment
vertical = self.course_fixture.get_nested_xblocks(category="vertical")[0]
self.course_fixture.create_xblock(
vertical.locator,
XBlockFixtureDesc('split_test', 'Test Content Experiment', metadata={'user_partition_id': 0})
)
# Go to the Group Configuration Page
self.page.visit()
config = self.page.group_configurations[0]
return config
def test_no_group_configurations_added(self):
"""
Scenario: Ensure that message telling me to create a new group configuration is
......@@ -311,11 +338,10 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin):
And "Create new Group Configuration" button is available
"""
self.page.visit()
css = ".wrapper-content .no-group-configurations-content"
self.assertTrue(self.page.q(css=css).present)
self.assertTrue(self.page.no_group_configuration_message_is_present)
self.assertIn(
"You haven't created any group configurations yet.",
self.page.q(css=css).text[0]
self.page.no_group_configuration_message_text
)
def test_group_configurations_have_correct_data(self):
......@@ -784,3 +810,115 @@ class GroupConfigurationsTest(ContainerBase, SplitTestMixin):
group_configuration_link_name,
self.page.group_configurations[1].name
)
def test_details_error_validation_message(self):
"""
Scenario: When a Content Experiment uses a Group Configuration, ensure
that an error validation message appears if necessary.
Given I have a course with a Group Configuration containing two Groups
And a Content Experiment is assigned to that Group Configuration
When I go to the Group Configuration Page
Then I do not see a error icon and message in the Group Configuration details view.
When I add a Group
Then I see an error icon and message in the Group Configuration details view
"""
# Create group configuration and associated experiment
config = self.create_group_configuration_experiment([Group("0", "Group A"), Group("1", "Group B")], True)
# Display details view
config.toggle()
# Check that error icon and message are not present
self.assertFalse(config.details_error_icon_is_present)
self.assertFalse(config.details_message_is_present)
# Add a group
config.toggle()
config.edit()
config.add_group()
config.save()
# Display details view
config.toggle()
# Check that error icon and message are present
self.assertTrue(config.details_error_icon_is_present)
self.assertTrue(config.details_message_is_present)
self.assertIn(
"This content experiment has issues that affect content visibility.",
config.details_message_text
)
def test_details_warning_validation_message(self):
"""
Scenario: When a Content Experiment uses a Group Configuration, ensure
that a warning validation message appears if necessary.
Given I have a course with a Group Configuration containing three Groups
And a Content Experiment is assigned to that Group Configuration
When I go to the Group Configuration Page
Then I do not see a warning icon and message in the Group Configuration details view.
When I remove a Group
Then I see a warning icon and message in the Group Configuration details view
"""
# Create group configuration and associated experiment
config = self.create_group_configuration_experiment([Group("0", "Group A"), Group("1", "Group B"), Group("2", "Group C")], True)
# Display details view
config.toggle()
# Check that warning icon and message are not present
self.assertFalse(config.details_warning_icon_is_present)
self.assertFalse(config.details_message_is_present)
# Remove a group
config.toggle()
config.edit()
config.groups[2].remove()
config.save()
# Display details view
config.toggle()
# Check that warning icon and message are present
self.assertTrue(config.details_warning_icon_is_present)
self.assertTrue(config.details_message_is_present)
self.assertIn(
"This content experiment has issues that affect content visibility.",
config.details_message_text
)
def test_edit_warning_message_empty_usage(self):
"""
Scenario: When a Group Configuration is not used, ensure that there are no warning icon and message.
Given I have a course with a Group Configuration containing two Groups
When I edit the Group Configuration
Then I do not see a warning icon and message
"""
# Create a group configuration with no associated experiment and display edit view
config = self.create_group_configuration_experiment([Group("0", "Group A"), Group("1", "Group B")], False)
config.edit()
# Check that warning icon and message are not present
self.assertFalse(config.edit_warning_icon_is_present)
self.assertFalse(config.edit_warning_message_is_present)
def test_edit_warning_message_non_empty_usage(self):
"""
Scenario: When a Group Configuration is used, ensure that there are a warning icon and message.
Given I have a course with a Group Configuration containing two Groups
When I edit the Group Configuration
Then I see a warning icon and message
"""
# Create a group configuration with an associated experiment and display edit view
config = self.create_group_configuration_experiment([Group("0", "Group A"), Group("1", "Group B")], True)
config.edit()
# Check that warning icon and message are present
self.assertTrue(config.edit_warning_icon_is_present)
self.assertTrue(config.edit_warning_message_is_present)
self.assertIn(
"This configuration is currently used in content experiments. If you make changes to the groups, you may need to edit those experiments.",
config.edit_warning_message_text
)
lms/templates/split_test_author_view.html
......@@ -27,22 +27,20 @@ show_link = settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS') and group_confi
% endif
% if len(messages) > 0:
<%
general_validation = split_test.descriptor.general_validation_message
def get_validation_icon(validation_type):
if validation_type == 'error':
if validation_type == ValidationMessageType.error:
return 'icon-exclamation-sign'
elif validation_type == 'warning':
elif validation_type == ValidationMessageType.warning:
return 'icon-warning-sign'
return None
error_messages = (message for message in messages if message.message_type==ValidationMessageType.error)
has_errors = next(error_messages, False)
aggregate_validation_class = 'has-errors' if has_errors else 'has-warnings'
aggregate_validation_type = 'error' if has_errors else 'warning'
aggregate_validation_class = 'has-errors' if general_validation['type']==ValidationMessageType.error else ' has-warnings'
%>
<div class="xblock-message validation ${aggregate_validation_class}">
% if is_configured:
<p class="${aggregate_validation_type}"><i class="${get_validation_icon(aggregate_validation_type)}"></i>
${_("This content experiment has issues that affect content visibility.")}
<p class="${general_validation['type']}"><i class="${get_validation_icon(general_validation['type'])}"></i>
${general_validation['message']}
</p>
% endif
% if is_root or not is_configured:
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment