diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 3279cf1b8b..60d88027d3 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/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) diff --git a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py index b6de397a29..2f413871e7 100644 --- a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py +++ b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py @@ -3,14 +3,14 @@ Group Configuration Tests. """ import json from mock import patch -from contentstore.utils import reverse_course_url +from contentstore.utils import reverse_course_url, reverse_usage_url from contentstore.views.component import SPLIT_TEST_COMPONENT_TYPE from contentstore.views.course import GroupConfiguration from contentstore.tests.utils import CourseTestCase from util.testing import UrlResetMixin from xmodule.partitions.partitions import Group, UserPartition from xmodule.modulestore.tests.factories import ItemFactory - +from xmodule.split_test_module import ValidationMessage, ValidationMessageType GROUP_CONFIGURATION_JSON = { u'name': u'Test name', @@ -27,7 +27,7 @@ class HelperMethods(object): """ Mixin that provides useful methods for Group Configuration tests. """ - def _create_content_experiment(self, cid=None, name_suffix=''): + def _create_content_experiment(self, cid=-1, name_suffix=''): """ Create content experiment. @@ -38,12 +38,42 @@ class HelperMethods(object): parent_location=self.course.location, display_name='Test Unit {}'.format(name_suffix) ) + c0_url = self.course.id.make_usage_key("vertical", "split_test_cond0") + c1_url = self.course.id.make_usage_key("vertical", "split_test_cond1") + c2_url = self.course.id.make_usage_key("vertical", "split_test_cond2") split_test = ItemFactory.create( category='split_test', parent_location=vertical.location, user_partition_id=cid, - display_name='Test Content Experiment {}'.format(name_suffix) + display_name='Test Content Experiment {}'.format(name_suffix), + group_id_to_child={"0": c0_url, "1": c1_url, "2": c2_url} ) + ItemFactory.create( + parent_location=split_test.location, + category="vertical", + display_name="Condition 0 vertical", + location=c0_url, + ) + ItemFactory.create( + parent_location=split_test.location, + category="vertical", + display_name="Condition 1 vertical", + location=c1_url, + ) + ItemFactory.create( + parent_location=split_test.location, + category="vertical", + display_name="Condition 2 vertical", + location=c2_url, + ) + + partitions_json = [p.to_json() for p in self.course.user_partitions] + + self.client.ajax_post( + reverse_usage_url("xblock_handler", split_test.location), + data={'metadata': {'user_partitions': partitions_json}} + ) + self.save_course() return (vertical, split_test) @@ -54,7 +84,7 @@ class HelperMethods(object): partitions = [ UserPartition( i, 'Name ' + str(i), 'Description ' + str(i), [Group(0, 'Group A'), Group(1, 'Group B'), Group(2, 'Group C')] - ) for i in xrange(count) + ) for i in xrange(0, count) ] self.course.user_partitions = partitions @@ -238,7 +268,7 @@ class GroupConfigurationsDetailHandlerTestCase(UrlResetMixin, CourseTestCase, Gr Test cases for group_configurations_detail_handler. """ - ID = 000000000000 + ID = 0 @patch.dict("django.conf.settings.FEATURES", {"ENABLE_GROUP_CONFIGURATIONS": True}) def setUp(self): @@ -247,11 +277,11 @@ class GroupConfigurationsDetailHandlerTestCase(UrlResetMixin, CourseTestCase, Gr """ super(GroupConfigurationsDetailHandlerTestCase, self).setUp() - def _url(self, cid=None): + def _url(self, cid=-1): """ Return url for the handler. """ - cid = cid if cid is not None else self.ID + cid = cid if cid > 0 else self.ID return reverse_course_url( 'group_configurations_detail_handler', self.course.id, @@ -271,6 +301,7 @@ class GroupConfigurationsDetailHandlerTestCase(UrlResetMixin, CourseTestCase, Gr {u'id': 0, u'name': u'Group A', u'version': 1}, {u'id': 1, u'name': u'Group B', u'version': 1}, ], + u'usage': [], } response = self.client.put( @@ -307,7 +338,9 @@ class GroupConfigurationsDetailHandlerTestCase(UrlResetMixin, CourseTestCase, Gr {u'id': 0, u'name': u'New Group Name', u'version': 1}, {u'id': 2, u'name': u'Group C', u'version': 1}, ], + u'usage': [], } + response = self.client.put( self._url(), data=json.dumps(expected), @@ -318,8 +351,10 @@ class GroupConfigurationsDetailHandlerTestCase(UrlResetMixin, CourseTestCase, Gr content = json.loads(response.content) self.assertEqual(content, expected) self.reload_course() + # Verify that user_partitions is properly updated in the course. user_partititons = self.course.user_partitions + self.assertEqual(len(user_partititons), 1) self.assertEqual(user_partititons[0].name, u'New Test name') self.assertEqual(len(user_partititons[0].groups), 2) @@ -344,7 +379,7 @@ class GroupConfigurationsDetailHandlerTestCase(UrlResetMixin, CourseTestCase, Gr # Verify that user_partitions is properly updated in the course. user_partititons = self.course.user_partitions self.assertEqual(len(user_partititons), 1) - self.assertEqual(user_partititons[0].name, u'Name 1') + self.assertEqual(user_partititons[0].name, 'Name 1') def test_cannot_delete_used_group_configuration(self): """ @@ -366,7 +401,7 @@ class GroupConfigurationsDetailHandlerTestCase(UrlResetMixin, CourseTestCase, Gr # Verify that user_partitions is still the same. user_partititons = self.course.user_partitions self.assertEqual(len(user_partititons), 2) - self.assertEqual(user_partititons[0].name, u'Name 0') + self.assertEqual(user_partititons[0].name, 'Name 0') def test_cannot_delete_non_existent_group_configuration(self): """ @@ -383,7 +418,7 @@ class GroupConfigurationsDetailHandlerTestCase(UrlResetMixin, CourseTestCase, Gr # Verify that user_partitions is still the same. user_partititons = self.course.user_partitions self.assertEqual(len(user_partititons), 2) - self.assertEqual(user_partititons[0].name, u'Name 0') + self.assertEqual(user_partititons[0].name, 'Name 0') # pylint: disable=no-member @@ -391,11 +426,9 @@ class GroupConfigurationsUsageInfoTestCase(UrlResetMixin, CourseTestCase, Helper """ Tests for usage information of configurations. """ + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_GROUP_CONFIGURATIONS": True}) def setUp(self): - """ - Set up group configurations and split test module. - """ super(GroupConfigurationsUsageInfoTestCase, self).setUp() def test_group_configuration_not_used(self): @@ -405,16 +438,16 @@ class GroupConfigurationsUsageInfoTestCase(UrlResetMixin, CourseTestCase, Helper self._add_user_partitions() actual = GroupConfiguration.add_usage_info(self.course, self.store) expected = [{ - u'id': 0, - u'name': u'Name 0', - u'description': u'Description 0', - u'version': 1, - u'groups': [ - {u'id': 0, u'name': u'Group A', u'version': 1}, - {u'id': 1, u'name': u'Group B', u'version': 1}, - {u'id': 2, u'name': u'Group C', u'version': 1}, + 'id': 0, + 'name': 'Name 0', + 'description': 'Description 0', + 'version': 1, + 'groups': [ + {'id': 0, 'name': 'Group A', 'version': 1}, + {'id': 1, 'name': 'Group B', 'version': 1}, + {'id': 2, 'name': 'Group C', 'version': 1}, ], - u'usage': [], + 'usage': [], }] self.assertEqual(actual, expected) @@ -429,30 +462,31 @@ class GroupConfigurationsUsageInfoTestCase(UrlResetMixin, CourseTestCase, Helper actual = GroupConfiguration.add_usage_info(self.course, self.store) expected = [{ - u'id': 0, - u'name': u'Name 0', - u'description': u'Description 0', - u'version': 1, - u'groups': [ - {u'id': 0, u'name': u'Group A', u'version': 1}, - {u'id': 1, u'name': u'Group B', u'version': 1}, - {u'id': 2, u'name': u'Group C', u'version': 1}, + 'id': 0, + 'name': 'Name 0', + 'description': 'Description 0', + 'version': 1, + 'groups': [ + {'id': 0, 'name': 'Group A', 'version': 1}, + {'id': 1, 'name': 'Group B', 'version': 1}, + {'id': 2, 'name': 'Group C', 'version': 1}, ], - u'usage': [{ + 'usage': [{ 'url': '/container/i4x://MITx/999/vertical/Test_Unit_0', 'label': 'Test Unit 0 / Test Content Experiment 0', + 'validation': None, }], }, { - u'id': 1, - u'name': u'Name 1', - u'description': u'Description 1', - u'version': 1, - u'groups': [ - {u'id': 0, u'name': u'Group A', u'version': 1}, - {u'id': 1, u'name': u'Group B', u'version': 1}, - {u'id': 2, u'name': u'Group C', u'version': 1}, + 'id': 1, + 'name': 'Name 1', + 'description': 'Description 1', + 'version': 1, + 'groups': [ + {'id': 0, 'name': 'Group A', 'version': 1}, + {'id': 1, 'name': 'Group B', 'version': 1}, + {'id': 2, 'name': 'Group C', 'version': 1}, ], - u'usage': [], + 'usage': [], }] self.assertEqual(actual, expected) @@ -469,21 +503,23 @@ class GroupConfigurationsUsageInfoTestCase(UrlResetMixin, CourseTestCase, Helper actual = GroupConfiguration.add_usage_info(self.course, self.store) expected = [{ - u'id': 0, - u'name': u'Name 0', - u'description': u'Description 0', - u'version': 1, - u'groups': [ - {u'id': 0, u'name': u'Group A', u'version': 1}, - {u'id': 1, u'name': u'Group B', u'version': 1}, - {u'id': 2, u'name': u'Group C', u'version': 1}, + 'id': 0, + 'name': 'Name 0', + 'description': 'Description 0', + 'version': 1, + 'groups': [ + {'id': 0, 'name': 'Group A', 'version': 1}, + {'id': 1, 'name': 'Group B', 'version': 1}, + {'id': 2, 'name': 'Group C', 'version': 1}, ], - u'usage': [{ + 'usage': [{ 'url': '/container/i4x://MITx/999/vertical/Test_Unit_0', 'label': 'Test Unit 0 / Test Content Experiment 0', + 'validation': None, }, { 'url': '/container/i4x://MITx/999/vertical/Test_Unit_1', 'label': 'Test Unit 1 / Test Content Experiment 1', + 'validation': None, }], }] self.assertEqual(actual, expected) @@ -502,3 +538,97 @@ class GroupConfigurationsUsageInfoTestCase(UrlResetMixin, CourseTestCase, Helper self.save_course() actual = GroupConfiguration.get_usage_info(self.course, self.store) self.assertEqual(actual, {0: []}) + + +class GroupConfigurationsValidationTestCase(CourseTestCase, HelperMethods): + """ + Tests for validation in Group Configurations. + """ + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_GROUP_CONFIGURATIONS": True}) + def setUp(self): + super(GroupConfigurationsValidationTestCase, self).setUp() + + @patch('xmodule.split_test_module.SplitTestDescriptor.validation_messages') + def test_error_message_present(self, mocked_validation_messages): + """ + Tests if validation message is present. + """ + self._add_user_partitions() + split_test = self._create_content_experiment(cid=0, name_suffix='0')[1] + + mocked_validation_messages.return_value = [ + ValidationMessage( + split_test, + u"Validation message", + ValidationMessageType.error + ) + ] + group_configuration = GroupConfiguration.add_usage_info(self.course, self.store)[0] + self.assertEqual( + group_configuration['usage'][0]['validation'], + { + 'message': u'This content experiment has issues that affect content visibility.', + 'type': 'error' + } + ) + + @patch('xmodule.split_test_module.SplitTestDescriptor.validation_messages') + def test_warning_message_present(self, mocked_validation_messages): + """ + Tests if validation message is present. + """ + self._add_user_partitions() + split_test = self._create_content_experiment(cid=0, name_suffix='0')[1] + + mocked_validation_messages.return_value = [ + ValidationMessage( + split_test, + u"Validation message", + ValidationMessageType.warning + ) + ] + group_configuration = GroupConfiguration.add_usage_info(self.course, self.store)[0] + self.assertEqual( + group_configuration['usage'][0]['validation'], + { + 'message': u'This content experiment has issues that affect content visibility.', + 'type': 'warning' + } + ) + + @patch('xmodule.split_test_module.SplitTestDescriptor.validation_messages') + def test_update_usage_info(self, mocked_validation_messages): + """ + Tests if validation message is present when updating usage info. + """ + self._add_user_partitions() + split_test = self._create_content_experiment(cid=0, name_suffix='0')[1] + + mocked_validation_messages.return_value = [ + ValidationMessage( + split_test, + u"Validation message", + ValidationMessageType.warning + ) + ] + + group_configuration = GroupConfiguration.update_usage_info(self.store, self.course, self.course.user_partitions[0]) + + self.assertEqual( + group_configuration['usage'][0]['validation'], + { + 'message': u'This content experiment has issues that affect content visibility.', + 'type': 'warning' + } + ) + + @patch('xmodule.split_test_module.SplitTestDescriptor.validation_messages') + def test_update_usage_info_no_message(self, mocked_validation_messages): + """ + Tests if validation message is not present when updating usage info. + """ + self._add_user_partitions() + self._create_content_experiment(cid=0, name_suffix='0') + mocked_validation_messages.return_value = [] + group_configuration = GroupConfiguration.update_usage_info(self.store, self.course, self.course.user_partitions[0]) + self.assertEqual(group_configuration['usage'][0]['validation'], None) diff --git a/cms/static/js/spec/views/group_configuration_spec.js b/cms/static/js/spec/views/group_configuration_spec.js index 758e9cce66..3b3d75be9a 100644 --- a/cms/static/js/spec/views/group_configuration_spec.js +++ b/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() { diff --git a/cms/static/sass/views/_group-configuration.scss b/cms/static/sass/views/_group-configuration.scss index 9991f4d86f..b58509667d 100644 --- a/cms/static/sass/views/_group-configuration.scss +++ b/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; diff --git a/cms/templates/js/group-configuration-details.underscore b/cms/templates/js/group-configuration-details.underscore index 82ace5314d..929b2ce609 100644 --- a/cms/templates/js/group-configuration-details.underscore +++ b/cms/templates/js/group-configuration-details.underscore @@ -62,7 +62,19 @@
+ <% if (unit.validation.type === 'warning') { %> + + <% } else if (unit.validation.type === 'error') { %> + + <% } %> + +
+ <% } %>+ <%= gettext('This configuration is currently used in content experiments. If you make changes to the groups, you may need to edit those experiments.') %> +
+