From 08ce09bde73dcf2aef25e2a2cb334e59e8c8c22f Mon Sep 17 00:00:00 2001 From: cahrens Date: Thu, 23 Oct 2014 17:21:48 -0400 Subject: [PATCH] Display validation messages for any xblock on the container page. TNL-683 --- CHANGELOG.rst | 2 + cms/djangoapps/contentstore/views/course.py | 4 +- .../views/tests/test_group_configurations.py | 114 ++++----- .../contentstore/views/tests/test_item.py | 6 +- cms/static/coffee/spec/main.coffee | 2 + cms/static/js/models/xblock_validation.js | 46 ++++ .../js/spec/models/xblock_validation_spec.js | 152 ++++++++++++ .../js/spec/views/group_configuration_spec.js | 4 +- cms/static/js/spec/views/xblock_spec.js | 8 + .../js/spec/views/xblock_validation_spec.js | 132 +++++++++++ cms/static/js/views/pages/container.js | 46 ++-- cms/static/js/views/xblock.js | 12 + cms/static/js/views/xblock_validation.js | 76 ++++++ .../js/group-configuration-details.underscore | 2 +- cms/templates/js/mock/mock-xblock.underscore | 3 + .../js/xblock-validation-messages.underscore | 49 ++++ cms/templates/studio_xblock_wrapper.html | 72 ++++-- .../public/js/split_test_author_view.js | 26 ++- .../lib/xmodule/xmodule/split_test_module.py | 186 +++++++-------- .../xmodule/tests/test_split_test_module.py | 125 +++++----- .../xmodule/xmodule/tests/test_validation.py | 218 ++++++++++++++++++ common/lib/xmodule/xmodule/validation.py | 128 ++++++++++ .../test/acceptance/pages/studio/container.py | 5 +- .../courseware/tests/test_model_data.py | 2 +- lms/templates/split_test_author_view.html | 61 +---- requirements/edx/github.txt | 2 +- 26 files changed, 1146 insertions(+), 337 deletions(-) create mode 100644 cms/static/js/models/xblock_validation.js create mode 100644 cms/static/js/spec/models/xblock_validation_spec.js create mode 100644 cms/static/js/spec/views/xblock_validation_spec.js create mode 100644 cms/static/js/views/xblock_validation.js create mode 100644 cms/templates/js/xblock-validation-messages.underscore create mode 100644 common/lib/xmodule/xmodule/tests/test_validation.py create mode 100644 common/lib/xmodule/xmodule/validation.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e2b7eb2983..fb5aa94e5c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,6 +7,8 @@ the top. Include a label indicating the component affected. Common: Add configurable reset button to units +Studio: Add support xblock validation messages on Studio unit/container page. TNL-683 + LMS: Support adding cohorts from the instructor dashboard. TNL-162 LMS: Support adding students to a cohort via the instructor dashboard. TNL-163 diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 79056f749d..77f6c95b58 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -1290,10 +1290,12 @@ class GroupConfiguration(object): 'container_handler', course.location.course_key.make_usage_key(unit.location.block_type, unit.location.name) ) + + validation_summary = split_test.general_validation_message() usage_info[split_test.user_partition_id].append({ 'label': '{} / {}'.format(unit.display_name, split_test.display_name), 'url': unit_url, - 'validation': split_test.general_validation_message, + 'validation': validation_summary.to_json() if validation_summary else None, }) return usage_info diff --git a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py index ca122a58f7..7d03c23c0f 100644 --- a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py +++ b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py @@ -9,7 +9,7 @@ from contentstore.views.course import GroupConfiguration from contentstore.tests.utils import CourseTestCase from xmodule.partitions.partitions import Group, UserPartition from xmodule.modulestore.tests.factories import ItemFactory -from xmodule.split_test_module import ValidationMessage, ValidationMessageType +from xmodule.validation import StudioValidation, StudioValidationMessage from xmodule.modulestore.django import modulestore from xmodule.modulestore import ModuleStoreEnum @@ -541,87 +541,75 @@ class GroupConfigurationsValidationTestCase(CourseTestCase, HelperMethods): def setUp(self): super(GroupConfigurationsValidationTestCase, self).setUp() - @patch('xmodule.split_test_module.SplitTestDescriptor.validation_messages') - def test_error_message_present(self, mocked_validation_messages): + @patch('xmodule.split_test_module.SplitTestDescriptor.validate_split_test') + def verify_validation_add_usage_info(self, expected_result, mocked_message, mocked_validation_messages): """ - Tests if validation message is present. + Helper method for testing validation information present after add_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.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' - } - ) + validation = StudioValidation(split_test.location) + validation.add(mocked_message) + mocked_validation_messages.return_value = validation - @patch('xmodule.split_test_module.SplitTestDescriptor.validation_messages') - def test_warning_message_present(self, mocked_validation_messages): + group_configuration = GroupConfiguration.add_usage_info(self.course, self.store)[0] + self.assertEqual(expected_result.to_json(), group_configuration['usage'][0]['validation']) + + def test_error_message_present(self): """ - Tests if validation message is present. + Tests if validation message is present (error case). + """ + mocked_message = StudioValidationMessage(StudioValidationMessage.ERROR, u"Validation message") + expected_result = StudioValidationMessage( + StudioValidationMessage.ERROR, u"This content experiment has issues that affect content visibility." + ) + self.verify_validation_add_usage_info(expected_result, mocked_message) # pylint: disable=no-value-for-parameter + + def test_warning_message_present(self): + """ + Tests if validation message is present (warning case). + """ + mocked_message = StudioValidationMessage(StudioValidationMessage.WARNING, u"Validation message") + expected_result = StudioValidationMessage( + StudioValidationMessage.WARNING, u"This content experiment has issues that affect content visibility." + ) + self.verify_validation_add_usage_info(expected_result, mocked_message) # pylint: disable=no-value-for-parameter + + @patch('xmodule.split_test_module.SplitTestDescriptor.validate_split_test') + def verify_validation_update_usage_info(self, expected_result, mocked_message, mocked_validation_messages): + """ + Helper method for testing validation information present after update_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.add_usage_info(self.course, self.store)[0] + validation = StudioValidation(split_test.location) + if mocked_message is not None: + validation.add(mocked_message) + mocked_validation_messages.return_value = validation + + 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' - } + expected_result.to_json() if expected_result is not None else None, + group_configuration['usage'][0]['validation'] ) - @patch('xmodule.split_test_module.SplitTestDescriptor.validation_messages') - def test_update_usage_info(self, mocked_validation_messages): + def test_update_usage_info(self): """ 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' - } + mocked_message = StudioValidationMessage(StudioValidationMessage.WARNING, u"Validation message") + expected_result = StudioValidationMessage( + StudioValidationMessage.WARNING, u"This content experiment has issues that affect content visibility." ) + # pylint: disable=no-value-for-parameter + self.verify_validation_update_usage_info(expected_result, mocked_message) - @patch('xmodule.split_test_module.SplitTestDescriptor.validation_messages') - def test_update_usage_info_no_message(self, mocked_validation_messages): + def test_update_usage_info_no_message(self): """ 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) + self.verify_validation_update_usage_info(None, None) # pylint: disable=no-value-for-parameter diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index c765f1da55..a1296a9495 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -130,8 +130,10 @@ class GetItemTest(ItemTest): root_usage_key = self._create_vertical() html, __ = self._get_container_preview(root_usage_key) - # Verify that the Studio wrapper is not added - self.assertNotIn('wrapper-xblock', html) + # XBlock messages are added by the Studio wrapper. + self.assertIn('wrapper-xblock-message', html) + # Make sure that "wrapper-xblock" does not appear by itself (without -message at end). + self.assertNotRegexpMatches(html, r'wrapper-xblock[^-]+') # Verify that the header and article tags are still added self.assertIn('
', html) diff --git a/cms/static/coffee/spec/main.coffee b/cms/static/coffee/spec/main.coffee index 10a277b28c..45ff7269d6 100644 --- a/cms/static/coffee/spec/main.coffee +++ b/cms/static/coffee/spec/main.coffee @@ -213,6 +213,7 @@ define([ "js/spec/models/component_template_spec", "js/spec/models/explicit_url_spec", "js/spec/models/xblock_info_spec", + "js/spec/models/xblock_validation_spec", "js/spec/utils/drag_and_drop_spec", "js/spec/utils/handle_iframe_binding_spec", @@ -228,6 +229,7 @@ define([ "js/spec/views/xblock_spec", "js/spec/views/xblock_editor_spec", "js/spec/views/xblock_string_field_editor_spec", + "js/spec/views/xblock_validation_spec", "js/spec/views/utils/view_utils_spec", diff --git a/cms/static/js/models/xblock_validation.js b/cms/static/js/models/xblock_validation.js new file mode 100644 index 0000000000..f48f593afd --- /dev/null +++ b/cms/static/js/models/xblock_validation.js @@ -0,0 +1,46 @@ +define(["backbone", "gettext", "underscore"], function (Backbone, gettext, _) { + /** + * Model for xblock validation messages as displayed in Studio. + */ + var XBlockValidationModel = Backbone.Model.extend({ + defaults: { + summary: {}, + messages: [], + empty: true, + xblock_id: null + }, + + WARNING : "warning", + ERROR: "error", + NOT_CONFIGURED: "not-configured", + + parse: function(response) { + if (!response.empty) { + var summary = "summary" in response ? response.summary : {}; + var messages = "messages" in response ? response.messages : []; + if (!(_.has(summary, "text")) || !summary.text) { + summary.text = gettext("This component has validation issues."); + } + if (!(_.has(summary, "type")) || !summary.type) { + summary.type = this.WARNING; + // Possible types are ERROR, WARNING, and NOT_CONFIGURED. NOT_CONFIGURED is treated as a warning. + _.find(messages, function (message) { + if (message.type === this.ERROR) { + summary.type = this.ERROR; + return true; + } + return false; + }, this); + } + response.summary = summary; + if (response.showSummaryOnly) { + messages = []; + } + response.messages = messages; + } + + return response; + } + }); + return XBlockValidationModel; +}); diff --git a/cms/static/js/spec/models/xblock_validation_spec.js b/cms/static/js/spec/models/xblock_validation_spec.js new file mode 100644 index 0000000000..5b1fc5e9b1 --- /dev/null +++ b/cms/static/js/spec/models/xblock_validation_spec.js @@ -0,0 +1,152 @@ +define(['js/models/xblock_validation'], + function(XBlockValidationModel) { + var verifyModel; + + verifyModel = function(model, expected_empty, expected_summary, expected_messages, expected_xblock_id) { + expect(model.get("empty")).toBe(expected_empty); + expect(model.get("summary")).toEqual(expected_summary); + expect(model.get("messages")).toEqual(expected_messages); + expect(model.get("xblock_id")).toBe(expected_xblock_id); + }; + + describe('XBlockValidationModel', function() { + it('handles empty variable', function() { + verifyModel(new XBlockValidationModel({parse: true}), true, {}, [], null); + verifyModel(new XBlockValidationModel({"empty": true}, {parse: true}), true, {}, [], null); + + // It is assumed that the "empty" state on the JSON object passed in is correct + // (no attempt is made to correct other variables based on empty==true). + verifyModel( + new XBlockValidationModel( + {"empty": true, "messages": [{"text": "Bad JSON case"}], "xblock_id": "id"}, + {parse: true} + ), + true, + {}, + [{"text": "Bad JSON case"}], "id" + ); + }); + + it('creates a summary if not defined', function() { + // Single warning message. + verifyModel( + new XBlockValidationModel({ + "empty": false, + "xblock_id": "id" + }, {parse: true}), + false, + {"text": "This component has validation issues.", "type": "warning"}, + [], + "id" + ); + // Two messages that compute to a "warning" state in the summary. + verifyModel( + new XBlockValidationModel({ + "empty": false, + "messages": [{"text": "one", "type": "not-configured"}, {"text": "two", "type": "warning"}], + "xblock_id": "id" + }, {parse: true}), + false, + {"text": "This component has validation issues.", "type": "warning"}, + [{"text": "one", "type": "not-configured"}, {"text": "two", "type": "warning"}], + "id" + ); + // Two messages, with one of them "error", resulting in an "error" state in the summary. + verifyModel( + new XBlockValidationModel({ + "empty": false, + "messages": [{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}], + "xblock_id": "id" + }, {parse: true}), + false, + {"text": "This component has validation issues.", "type": "error"}, + [{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}], + "id" + ); + }); + + it('respects summary properties that are defined', function() { + // Summary already present (both text and type), no messages. + verifyModel( + new XBlockValidationModel({ + "empty": false, + "xblock_id": "id", + "summary": {"text": "my summary", "type": "custom type"} + }, {parse: true}), + false, + {"text": "my summary", "type": "custom type"}, + [], + "id" + ); + // Summary text present, but not type (will get default value of warning). + verifyModel( + new XBlockValidationModel({ + "empty": false, + "xblock_id": "id", + "summary": {"text": "my summary"} + }, {parse: true}), + false, + {"text": "my summary", "type": "warning"}, + [], + "id" + ); + // Summary type present, but not text. + verifyModel( + new XBlockValidationModel({ + "empty": false, + "summary": {"type": "custom type"}, + "messages": [{"text": "one", "type": "not-configured"}, {"text": "two", "type": "warning"}], + "xblock_id": "id" + }, {parse: true}), + false, + {"text": "This component has validation issues.", "type": "custom type"}, + [{"text": "one", "type": "not-configured"}, {"text": "two", "type": "warning"}], + "id" + ); + // Summary text present, type will be computed as error. + verifyModel( + new XBlockValidationModel({ + "empty": false, + "summary": {"text": "my summary"}, + "messages": [{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}], + "xblock_id": "id" + }, {parse: true}), + false, + {"text": "my summary", "type": "error"}, + [{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}], + "id" + ); + }); + + it('clears messages if showSummaryOnly is true', function() { + verifyModel( + new XBlockValidationModel({ + "empty": false, + "xblock_id": "id", + "summary": {"text": "my summary"}, + "messages": [{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}], + "showSummaryOnly": true + }, {parse: true}), + false, + {"text": "my summary", "type": "error"}, + [], + "id" + ); + + verifyModel( + new XBlockValidationModel({ + "empty": false, + "xblock_id": "id", + "summary": {"text": "my summary"}, + "messages": [{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}], + "showSummaryOnly": false + }, {parse: true}), + false, + {"text": "my summary", "type": "error"}, + [{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}], + "id" + ); + }); + }); + } +); diff --git a/cms/static/js/spec/views/group_configuration_spec.js b/cms/static/js/spec/views/group_configuration_spec.js index 9bc0ea3c9e..6a7abbf041 100644 --- a/cms/static/js/spec/views/group_configuration_spec.js +++ b/cms/static/js/spec/views/group_configuration_spec.js @@ -215,7 +215,7 @@ define([ 'label': 'label1', 'url': 'url1', 'validation': { - 'message': "Warning message", + 'text': "Warning message", 'type': 'warning' } } @@ -233,7 +233,7 @@ define([ 'label': 'label1', 'url': 'url1', 'validation': { - 'message': "Error message", + 'text': "Error message", 'type': 'error' } } diff --git a/cms/static/js/spec/views/xblock_spec.js b/cms/static/js/spec/views/xblock_spec.js index 64d8e31392..feb4746aac 100644 --- a/cms/static/js/spec/views/xblock_spec.js +++ b/cms/static/js/spec/views/xblock_spec.js @@ -102,6 +102,14 @@ define([ "jquery", "js/common_helpers/ajax_helpers", "URI", "js/views/xblock", " ]); expect(promise.isRejected()).toBe(true); }); + + it('Triggers an event to the runtime when a notification-action-button is clicked', function () { + var notifySpy = spyOn(xblockView, "notifyRuntime").andCallThrough(); + + postXBlockRequest(AjaxHelpers.requests(this), []); + xblockView.$el.find(".notification-action-button").click(); + expect(notifySpy).toHaveBeenCalledWith("add-missing-groups", model.get("id")); + }) }); }); }); diff --git a/cms/static/js/spec/views/xblock_validation_spec.js b/cms/static/js/spec/views/xblock_validation_spec.js new file mode 100644 index 0000000000..66e579f109 --- /dev/null +++ b/cms/static/js/spec/views/xblock_validation_spec.js @@ -0,0 +1,132 @@ +define(['jquery', 'js/models/xblock_validation', 'js/views/xblock_validation', 'js/common_helpers/template_helpers'], + function($, XBlockValidationModel, XBlockValidationView, TemplateHelpers) { + + beforeEach(function () { + TemplateHelpers.installTemplate('xblock-validation-messages'); + }); + + describe('XBlockValidationView helper methods', function() { + var model, view; + + beforeEach(function () { + model = new XBlockValidationModel({parse: true}); + view = new XBlockValidationView({model: model}); + view.render(); + }); + + it('has a getIcon method', function() { + var getIcon = view.getIcon.bind(view); + + expect(getIcon(model.WARNING)).toBe('icon-warning-sign'); + expect(getIcon(model.NOT_CONFIGURED)).toBe('icon-warning-sign'); + expect(getIcon(model.ERROR)).toBe('icon-exclamation-sign'); + expect(getIcon("unknown")).toBeNull(); + }); + + it('has a getDisplayName method', function() { + var getDisplayName = view.getDisplayName.bind(view); + + expect(getDisplayName(model.WARNING)).toBe("Warning"); + expect(getDisplayName(model.NOT_CONFIGURED)).toBe("Warning"); + expect(getDisplayName(model.ERROR)).toBe("Error"); + expect(getDisplayName("unknown")).toBeNull(); + }); + + it('can add additional classes', function() { + var noContainerContent = "no-container-content", notConfiguredModel, nonRootView, rootView; + + expect(view.getAdditionalClasses()).toBe(""); + expect(view.$('.validation')).not.toHaveClass(noContainerContent); + + notConfiguredModel = new XBlockValidationModel({ + "empty": false, "summary": {"text": "Not configured", "type": model.NOT_CONFIGURED}, + "xblock_id": "id" + }, + {parse: true} + ); + nonRootView = new XBlockValidationView({model: notConfiguredModel}); + nonRootView.render(); + expect(nonRootView.getAdditionalClasses()).toBe(""); + expect(view.$('.validation')).not.toHaveClass(noContainerContent); + + rootView = new XBlockValidationView({model: notConfiguredModel, root: true}); + rootView.render(); + expect(rootView.getAdditionalClasses()).toBe(noContainerContent); + expect(rootView.$('.validation')).toHaveClass(noContainerContent); + }); + + }); + + describe('XBlockValidationView rendering', function() { + var model, view; + + beforeEach(function () { + model = new XBlockValidationModel({ + "empty": false, + "summary": { + "text": "Summary message", "type": "error", + "action_label": "Summary Action", "action_class": "edit-button" + }, + "messages": [ + { + "text": "First message", "type": "warning", + "action_label": "First Message Action", "action_runtime_event": "fix-up" + }, + {"text": "Second message", "type": "error"} + ], + "xblock_id": "id" + }); + view = new XBlockValidationView({model: model}); + view.render(); + }); + + it('renders summary and detailed messages types', function() { + var details; + + expect(view.$('.xblock-message')).toHaveClass("has-errors"); + details = view.$('.xblock-message-item'); + expect(details.length).toBe(2); + expect(details[0]).toHaveClass("warning"); + expect(details[1]).toHaveClass("error"); + }); + + it('renders summary and detailed messages text', function() { + var details; + + expect(view.$('.xblock-message').text()).toContain("Summary message"); + + details = view.$('.xblock-message-item'); + expect(details.length).toBe(2); + expect($(details[0]).text()).toContain("Warning"); + expect($(details[0]).text()).toContain("First message"); + expect($(details[1]).text()).toContain("Error"); + expect($(details[1]).text()).toContain("Second message"); + }); + + it('renders action info', function() { + expect(view.$('a.edit-button .action-button-text').text()).toContain("Summary Action"); + + expect(view.$('a.notification-action-button .action-button-text').text()). + toContain("First Message Action"); + expect(view.$('a.notification-action-button').data("notification-action")).toBe("fix-up"); + }); + + it('renders a summary only', function() { + var summaryOnlyModel = new XBlockValidationModel({ + "empty": false, + "summary": {"text": "Summary message", "type": "warning"}, + "xblock_id": "id" + }), summaryOnlyView, details; + + summaryOnlyView = new XBlockValidationView({model: summaryOnlyModel}); + summaryOnlyView.render(); + + expect(summaryOnlyView.$('.xblock-message')).toHaveClass("has-warnings"); + expect(view.$('.xblock-message').text()).toContain("Summary message"); + + details = summaryOnlyView.$('.xblock-message-item'); + expect(details.length).toBe(0); + }); + }); + } +); diff --git a/cms/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index bce220ee2b..7a62e53591 100644 --- a/cms/static/js/views/pages/container.js +++ b/cms/static/js/views/pages/container.js @@ -13,6 +13,12 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views var XBlockContainerPage = BasePage.extend({ // takes XBlockInfo as a model + events: { + "click .edit-button": "editXBlock", + "click .duplicate-button": "duplicateXBlock", + "click .delete-button": "deleteXBlock" + }, + options: { collapsedClass: 'is-collapsed' }, @@ -81,12 +87,6 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views // Hide both blocks until we know which one to show xblockView.$el.addClass(hiddenCss); - if (!options || !options.refresh) { - // Add actions to any top level buttons, e.g. "Edit" of the container itself. - // Do not add the actions on "refresh" though, as the handlers are already registered. - self.addButtonActions(this.$el); - } - // Render the xblock xblockView.render({ done: function() { @@ -119,7 +119,6 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views }, onXBlockRefresh: function(xblockView) { - this.addButtonActions(xblockView.$el); this.xblockView.refresh(); // Update publish and last modified information from the server. this.model.fetch(); @@ -137,25 +136,12 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views }); }, - addButtonActions: function(element) { - var self = this; - element.find('.edit-button').click(function(event) { - event.preventDefault(); - self.editComponent(self.findXBlockElement(event.target)); - }); - element.find('.duplicate-button').click(function(event) { - event.preventDefault(); - self.duplicateComponent(self.findXBlockElement(event.target)); - }); - element.find('.delete-button').click(function(event) { - event.preventDefault(); - self.deleteComponent(self.findXBlockElement(event.target)); - }); - }, - - editComponent: function(xblockElement) { - var self = this, + editXBlock: function(event) { + var xblockElement = this.findXBlockElement(event.target), + self = this, modal = new EditXBlockModal({ }); + event.preventDefault(); + modal.edit(xblockElement, this.model, { refresh: function() { self.refreshXBlock(xblockElement); @@ -163,6 +149,16 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views }); }, + duplicateXBlock: function(event) { + event.preventDefault(); + this.duplicateComponent(this.findXBlockElement(event.target)); + }, + + deleteXBlock: function(event) { + event.preventDefault(); + this.deleteComponent(this.findXBlockElement(event.target)); + }, + createPlaceholderElement: function() { return $("
", { class: "studio-xblock-wrapper" }); }, diff --git a/cms/static/js/views/xblock.js b/cms/static/js/views/xblock.js index 78d1f08b84..797cc8cb45 100644 --- a/cms/static/js/views/xblock.js +++ b/cms/static/js/views/xblock.js @@ -4,6 +4,10 @@ define(["jquery", "underscore", "js/views/baseview", "xblock/runtime.v1"], var XBlockView = BaseView.extend({ // takes XBlockInfo as a model + events: { + "click .notification-action-button": "fireNotificationActionEvent" + }, + initialize: function() { BaseView.prototype.initialize.call(this); this.view = this.options.view; @@ -195,6 +199,14 @@ define(["jquery", "underscore", "js/views/baseview", "xblock/runtime.v1"], } // Return an already resolved promise for synchronous updates return $.Deferred().resolve().promise(); + }, + + fireNotificationActionEvent: function(event) { + var eventName = $(event.currentTarget).data("notification-action"); + if (eventName) { + event.preventDefault(); + this.notifyRuntime(eventName, this.model.get("id")); + } } }); diff --git a/cms/static/js/views/xblock_validation.js b/cms/static/js/views/xblock_validation.js new file mode 100644 index 0000000000..4febbc734b --- /dev/null +++ b/cms/static/js/views/xblock_validation.js @@ -0,0 +1,76 @@ +define(["jquery", "underscore", "js/views/baseview", "gettext"], + function ($, _, BaseView, gettext) { + /** + * View for xblock validation messages as displayed in Studio. + */ + var XBlockValidationView = BaseView.extend({ + + // Takes XBlockValidationModel as a model + initialize: function(options) { + BaseView.prototype.initialize.call(this); + this.template = this.loadTemplate('xblock-validation-messages'); + this.root = options.root; + }, + + render: function () { + this.$el.html(this.template({ + validation: this.model, + additionalClasses: this.getAdditionalClasses(), + getIcon: this.getIcon.bind(this), + getDisplayName: this.getDisplayName.bind(this) + })); + return this; + }, + + /** + * Returns the icon css class based on the message type. + * @param messageType + * @returns string representation of css class that will render the correct icon, or null if unknown type + */ + getIcon: function (messageType) { + if (messageType === this.model.ERROR) { + return 'icon-exclamation-sign'; + } + else if (messageType === this.model.WARNING || messageType === this.model.NOT_CONFIGURED) { + return 'icon-warning-sign'; + } + return null; + }, + + /** + * Returns a display name for a message (useful for screen readers), based on the message type. + * @param messageType + * @returns string display name (translated) + */ + getDisplayName: function (messageType) { + if (messageType === this.model.WARNING || messageType === this.model.NOT_CONFIGURED) { + // Translators: This message will be added to the front of messages of type warning, + // e.g. "Warning: this component has not been configured yet". + return gettext("Warning"); + } + else if (messageType === this.model.ERROR) { + // Translators: This message will be added to the front of messages of type error, + // e.g. "Error: required field is missing". + return gettext("Error"); + } + return null; + }, + + /** + * Returns additional css classes that can be added to HTML containing the validation messages. + * Useful for rendering NOT_CONFIGURED in a special way. + * + * @returns string of additional css classes (or empty string) + */ + getAdditionalClasses: function () { + if (this.root && this.model.get("summary").type === this.model.NOT_CONFIGURED && + this.model.get("messages").length === 0) { + + return "no-container-content"; + } + return ""; + } + }); + + return XBlockValidationView; + }); diff --git a/cms/templates/js/group-configuration-details.underscore b/cms/templates/js/group-configuration-details.underscore index 929b2ce609..fc8042bd40 100644 --- a/cms/templates/js/group-configuration-details.underscore +++ b/cms/templates/js/group-configuration-details.underscore @@ -71,7 +71,7 @@ <% } %> - <%= unit.validation.message %> + <%= unit.validation.text %>

<% } %> diff --git a/cms/templates/js/mock/mock-xblock.underscore b/cms/templates/js/mock/mock-xblock.underscore index 652aa17001..386b4b8bf8 100644 --- a/cms/templates/js/mock/mock-xblock.underscore +++ b/cms/templates/js/mock/mock-xblock.underscore @@ -9,6 +9,9 @@ + + Add Missing Groups +
diff --git a/cms/templates/js/xblock-validation-messages.underscore b/cms/templates/js/xblock-validation-messages.underscore new file mode 100644 index 0000000000..716f619312 --- /dev/null +++ b/cms/templates/js/xblock-validation-messages.underscore @@ -0,0 +1,49 @@ +<% +var summaryMessage = validation.get("summary"); +var aggregateMessageType = summaryMessage.type; +var aggregateValidationClass = aggregateMessageType === "error"? "has-errors" : "has-warnings"; +%> +
+

+ <%- summaryMessage.text %> + <% if (summaryMessage.action_class) { %> + + <%- summaryMessage.action_label %> + + <% } else if (summaryMessage.action_runtime_event) {%> + + <%- summaryMessage.action_label %> + + <% } %> +

+ <% var detailedMessages = validation.get("messages"); %> + <% if (detailedMessages.length > 0) { %> +
    + <% for (var i = 0; i < detailedMessages.length; i++) { %> + <% + var message = detailedMessages[i]; + var messageType = message.type + var messageTypeDisplayName = getDisplayName(messageType) + %> +
  • + + <% if (messageTypeDisplayName) { %> + <%- messageTypeDisplayName %>: + <% } %> + <%- message.text %> + + <% if (message.action_class) { %> + + <%- message.action_label %> + + <% } else if (message.action_runtime_event) {%> + + <%- message.action_label %> + + <% } %> + +
    • + <% } %> +
+ <% } %> +
diff --git a/cms/templates/studio_xblock_wrapper.html b/cms/templates/studio_xblock_wrapper.html index 1f578f405c..1573773561 100644 --- a/cms/templates/studio_xblock_wrapper.html +++ b/cms/templates/studio_xblock_wrapper.html @@ -1,16 +1,51 @@ + <%! from django.utils.translation import ugettext as _ from contentstore.views.helpers import xblock_studio_url +import json %> - <% xblock_url = xblock_studio_url(xblock) show_inline = xblock.has_children and not xblock_url section_class = "level-nesting" if show_inline else "level-element" collapsible_class = "is-collapsible" if xblock.has_children else "" label = xblock.display_name or xblock.scope_ids.block_type +messages = json.dumps(xblock.validate().to_json()) %> +<%namespace name='static' file='static_content.html'/> + +<%block name="header_extras"> + + + + + + % if not is_root: % if is_reorderable:
  • @@ -64,21 +99,29 @@ label = xblock.display_name or xblock.scope_ids.block_type - % if xblock_url and not is_root: -
    -
    ${_('This block contains multiple components.')}
    - -
    + % if not is_root: +
    + % if xblock_url: +
    +
    ${_('This block contains multiple components.')}
    + +
    + % endif % endif + +% if is_root: +
    +% endif + % if is_root or not xblock_url:
    ${content} @@ -86,7 +129,6 @@ label = xblock.display_name or xblock.scope_ids.block_type % else:
    ${content} -
    % endif % if not is_root: diff --git a/common/lib/xmodule/xmodule/public/js/split_test_author_view.js b/common/lib/xmodule/xmodule/public/js/split_test_author_view.js index 8c4cc866f3..5c229fb5dd 100644 --- a/common/lib/xmodule/xmodule/public/js/split_test_author_view.js +++ b/common/lib/xmodule/xmodule/public/js/split_test_author_view.js @@ -1,25 +1,27 @@ /* JavaScript for editing operations that can be done on the split test author view. */ window.SplitTestAuthorView = function (runtime, element) { var $element = $(element); + var splitTestLocator = $element.closest('.studio-xblock-wrapper').data('locator'); - $element.find('.add-missing-groups-button').click(function () { - runtime.notify('save', { - state: 'start', - element: element, - message: gettext('Creating missing groups…') - }); - $.post(runtime.handlerUrl(element, 'add_missing_groups')).done(function() { + runtime.listenTo("add-missing-groups", function (parentLocator) { + if (splitTestLocator === parentLocator) { runtime.notify('save', { - state: 'end', - element: element + state: 'start', + element: element, + message: gettext('Creating missing groups…') }); - }); + $.post(runtime.handlerUrl(element, 'add_missing_groups')).done(function() { + runtime.notify('save', { + state: 'end', + element: element + }); + }); + } }); // Listen to delete events so that the view can refresh when the last inactive group is removed. runtime.listenTo('deleted-child', function(parentLocator) { - var splitTestLocator = $element.closest('.studio-xblock-wrapper').data('locator'), - inactiveGroups = $element.find('.is-inactive .studio-xblock-wrapper'); + var inactiveGroups = $element.find('.is-inactive .studio-xblock-wrapper'); if (splitTestLocator === parentLocator && inactiveGroups.length === 0) { runtime.refreshXBlock($element); } diff --git a/common/lib/xmodule/xmodule/split_test_module.py b/common/lib/xmodule/xmodule/split_test_module.py index 63f23a824e..890afb75dc 100644 --- a/common/lib/xmodule/xmodule/split_test_module.py +++ b/common/lib/xmodule/xmodule/split_test_module.py @@ -12,6 +12,7 @@ from xmodule.progress import Progress from xmodule.seq_module import SequenceDescriptor from xmodule.studio_editable import StudioEditableModule, StudioEditableDescriptor from xmodule.x_module import XModule, module_attr, STUDENT_VIEW +from xmodule.validation import StudioValidation, StudioValidationMessage from xmodule.modulestore.inheritance import UserPartitionList from lxml import etree @@ -28,48 +29,6 @@ _ = lambda text: text DEFAULT_GROUP_NAME = _(u'Group ID {group_id}') -class ValidationMessageType(object): - """ - The type for a validation message -- currently 'information', 'warning' or 'error'. - """ - information = 'information' - warning = 'warning' - error = 'error' - - @staticmethod - def display_name(message_type): - """ - Returns the display name for the specified validation message type. - """ - if message_type == ValidationMessageType.warning: - # Translators: This message will be added to the front of messages of type warning, - # e.g. "Warning: this component has not been configured yet". - return _(u"Warning") - elif message_type == ValidationMessageType.error: - # Translators: This message will be added to the front of messages of type error, - # e.g. "Error: required field is missing". - return _(u"Error") - else: - return None - - -# TODO: move this into the xblock repo once it has a formal validation contract -class ValidationMessage(object): - """ - Represents a single validation message for an xblock. - """ - def __init__(self, xblock, message_text, message_type, action_class=None, action_label=None): - assert isinstance(message_text, unicode) - self.xblock = xblock - self.message_text = message_text - self.message_type = message_type - self.action_class = action_class - self.action_label = action_label - - def __unicode__(self): - return self.message_text - - class SplitTestFields(object): """Fields needed for split test module""" has_children = True @@ -231,6 +190,13 @@ class SplitTestModule(SplitTestFields, XModule, StudioEditableModule): return None return partitions_service.get_user_group_for_partition(self.user_partition_id) + @property + def is_configured(self): + """ + Returns true if the split_test instance is associated with a UserPartition. + """ + return self.descriptor.is_configured + def _staff_view(self, context): """ Render the staff view for a split test module. @@ -283,7 +249,6 @@ class SplitTestModule(SplitTestFields, XModule, StudioEditableModule): """ fragment = Fragment() root_xblock = context.get('root_xblock') - is_configured = not self.user_partition_id == SplitTestFields.no_partition_selected['value'] is_root = root_xblock and root_xblock.location == self.location active_groups_preview = None inactive_groups_preview = None @@ -300,7 +265,7 @@ class SplitTestModule(SplitTestFields, XModule, StudioEditableModule): fragment.add_content(self.system.render_template('split_test_author_view.html', { 'split_test': self, 'is_root': is_root, - 'is_configured': is_configured, + 'is_configured': self.is_configured, 'active_groups_preview': active_groups_preview, 'inactive_groups_preview': inactive_groups_preview, 'group_configuration_url': self.descriptor.group_configuration_url, @@ -320,7 +285,7 @@ class SplitTestModule(SplitTestFields, XModule, StudioEditableModule): active_child = self.system.get_module(active_child_descriptor) rendered_child = active_child.render(StudioEditableModule.get_preview_view_name(active_child), context) if active_child.category == 'vertical': - group_name, group_id = self.get_data_for_vertical(active_child) + group_name, group_id = self.get_data_for_vertical(active_child) if group_name: rendered_child.content = rendered_child.content.replace( DEFAULT_GROUP_NAME.format(group_id=group_id), @@ -384,6 +349,13 @@ class SplitTestModule(SplitTestFields, XModule, StudioEditableModule): return (group.name, group.id) return (None, None) + def validate(self): + """ + Message for either error or warning validation message/s. + + Returns message and type. Priority given to error type message. + """ + return self.descriptor.validate() @XBlock.needs('user_tags') # pylint: disable=abstract-method @XBlock.wants('partitions') @@ -544,46 +516,94 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor, StudioEditableDes return active_children, inactive_children - def validation_messages(self): + @property + def is_configured(self): """ - Returns a list of validation messages describing the current state of the block. Each message - includes a message type indicating whether the message represents information, a warning or an error. + Returns true if the split_test instance is associated with a UserPartition. + """ + return not self.user_partition_id == SplitTestFields.no_partition_selected['value'] + + def validate(self): + """ + Validates the state of this split_test instance. This is the override of the general XBlock method, + and it will also ask its superclass to validate. + """ + validation = super(SplitTestDescriptor, self).validate() + split_test_validation = self.validate_split_test() + + if split_test_validation: + return validation + + validation = StudioValidation.copy(validation) + if validation and (not self.is_configured and len(split_test_validation.messages) == 1): + validation.summary = split_test_validation.messages[0] + else: + validation.summary = self.general_validation_message(split_test_validation) + validation.add_messages(split_test_validation) + + return validation + + def validate_split_test(self): + """ + Returns a StudioValidation object describing the current state of the split_test_module + (not including superclass validation messages). """ _ = self.runtime.service(self, "i18n").ugettext # pylint: disable=redefined-outer-name - messages = [] + split_validation = StudioValidation(self.location) if self.user_partition_id < 0: - messages.append(ValidationMessage( - self, - _(u"The experiment is not associated with a group configuration."), - ValidationMessageType.warning, - 'edit-button', - _(u"Select a Group Configuration") - )) + split_validation.add( + StudioValidationMessage( + StudioValidationMessage.NOT_CONFIGURED, + _(u"The experiment is not associated with a group configuration."), + action_class='edit-button', + action_label=_(u"Select a Group Configuration") + ) + ) else: user_partition = self.get_selected_partition() if not user_partition: - messages.append(ValidationMessage( - self, - _(u"The experiment uses a deleted group configuration. Select a valid group configuration or delete this experiment."), - ValidationMessageType.error - )) + split_validation.add( + StudioValidationMessage( + StudioValidationMessage.ERROR, + _(u"The experiment uses a deleted group configuration. Select a valid group configuration or delete this experiment.") + ) + ) else: [active_children, inactive_children] = self.active_and_inactive_children() if len(active_children) < len(user_partition.groups): - messages.append(ValidationMessage( - self, - _(u"The experiment does not contain all of the groups in the configuration."), - ValidationMessageType.error, - 'add-missing-groups-button', - _(u"Add Missing Groups") - )) + split_validation.add( + StudioValidationMessage( + StudioValidationMessage.ERROR, + _(u"The experiment does not contain all of the groups in the configuration."), + action_runtime_event='add-missing-groups', + action_label=_(u"Add Missing Groups") + ) + ) if len(inactive_children) > 0: - messages.append(ValidationMessage( - self, - _(u"The experiment has an inactive group. Move content into active groups, then delete the inactive group."), - ValidationMessageType.warning - )) - return messages + split_validation.add( + StudioValidationMessage( + StudioValidationMessage.WARNING, + _(u"The experiment has an inactive group. Move content into active groups, then delete the inactive group.") + ) + ) + return split_validation + + def general_validation_message(self, validation=None): + """ + Returns just a summary message about whether or not this split_test instance has + validation issues (not including superclass validation messages). If the split_test instance + validates correctly, this method returns None. + """ + if validation is None: + validation = self.validate_split_test() + + if not validation: + has_error = any(message.type == StudioValidationMessage.ERROR for message in validation.messages) + return StudioValidationMessage( + StudioValidationMessage.ERROR if has_error else StudioValidationMessage.WARNING, + _(u"This content experiment has issues that affect content visibility.") + ) + return None @XBlock.handler def add_missing_groups(self, request, suffix=''): # pylint: disable=unused-argument @@ -603,7 +623,7 @@ class SplitTestDescriptor(SplitTestFields, SequenceDescriptor, StudioEditableDes changed = True if changed: - # TODO user.id - to be fixed by Publishing team + # user.id - to be fixed by Publishing team self.system.modulestore.update_item(self, None) return Response() @@ -648,19 +668,3 @@ 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 diff --git a/common/lib/xmodule/xmodule/tests/test_split_test_module.py b/common/lib/xmodule/xmodule/tests/test_split_test_module.py index a7484d902e..b82310c845 100644 --- a/common/lib/xmodule/xmodule/tests/test_split_test_module.py +++ b/common/lib/xmodule/xmodule/tests/test_split_test_module.py @@ -10,7 +10,8 @@ from xmodule.tests.xml import factories as xml from xmodule.tests.xml import XModuleXmlImportTest from xmodule.tests import get_test_system from xmodule.x_module import AUTHOR_VIEW, STUDENT_VIEW -from xmodule.split_test_module import SplitTestDescriptor, SplitTestFields, ValidationMessageType +from xmodule.validation import StudioValidationMessage +from xmodule.split_test_module import SplitTestDescriptor, SplitTestFields from xmodule.partitions.partitions import Group, UserPartition from xmodule.partitions.test_partitions import StaticPartitionService, MemoryUserTagsService @@ -320,14 +321,6 @@ class SplitTestModuleStudioTest(SplitTestModuleTest): self.assertEqual(active_children, []) self.assertEqual(inactive_children, children) - def test_validation_message_types(self): - """ - Test the behavior of validation message types. - """ - self.assertEqual(ValidationMessageType.display_name(ValidationMessageType.error), u"Error") - self.assertEqual(ValidationMessageType.display_name(ValidationMessageType.warning), u"Warning") - self.assertIsNone(ValidationMessageType.display_name(ValidationMessageType.information)) - def test_validation_messages(self): """ Test the validation messages produced for different split test configurations. @@ -335,122 +328,128 @@ class SplitTestModuleStudioTest(SplitTestModuleTest): split_test_module = self.split_test_module def verify_validation_message(message, expected_message, expected_message_type, - expected_action_class=None, expected_action_label=None): + expected_action_class=None, expected_action_label=None, + expected_action_runtime_event=None): """ Verify that the validation message has the expected validation message and type. """ - self.assertEqual(unicode(message), expected_message) - self.assertEqual(message.message_type, expected_message_type) - self.assertEqual(message.action_class, expected_action_class) - self.assertEqual(message.action_label, expected_action_label) + self.assertEqual(message.text, expected_message) + self.assertEqual(message.type, expected_message_type) + if expected_action_class: + self.assertEqual(message.action_class, expected_action_class) + else: + self.assertFalse(hasattr(message, "action_class")) + if expected_action_label: + self.assertEqual(message.action_label, expected_action_label) + else: + self.assertFalse(hasattr(message, "action_label")) + if expected_action_runtime_event: + self.assertEqual(message.action_runtime_event, expected_action_runtime_event) + else: + self.assertFalse(hasattr(message, "action_runtime_event")) - def verify_general_validation_message(general_validation, expected_message, expected_message_type): + def verify_summary_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) + self.assertEqual(general_validation.text, 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() - self.assertEqual(len(messages), 1) + validation = split_test_module.validate() + self.assertEqual(len(validation.messages), 0) verify_validation_message( - messages[0], + validation.summary, u"The experiment is not associated with a group configuration.", - ValidationMessageType.warning, + StudioValidationMessage.NOT_CONFIGURED, '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 split_test_module.user_partitions = [ UserPartition(0, 'first_partition', 'First Partition', [Group("0", 'alpha'), Group("1", 'beta')]) ] - messages = split_test_module.validation_messages() - self.assertEqual(len(messages), 0) - self.assertIsNone(split_test_module.general_validation_message, None) + validation = split_test_module.validate_split_test() + self.assertTrue(validation) + 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 = [ UserPartition(0, 'first_partition', 'First Partition', [Group("0", 'alpha'), Group("1", 'beta'), Group("2", 'gamma')]) ] - messages = split_test_module.validation_messages() - self.assertEqual(len(messages), 1) + validation = split_test_module.validate() + self.assertEqual(len(validation.messages), 1) verify_validation_message( - messages[0], + validation.messages[0], u"The experiment does not contain all of the groups in the configuration.", - ValidationMessageType.error, - 'add-missing-groups-button', - u"Add Missing Groups" + StudioValidationMessage.ERROR, + expected_action_runtime_event='add-missing-groups', + expected_action_label=u"Add Missing Groups" ) - verify_general_validation_message( - split_test_module.general_validation_message, + verify_summary_message( + validation.summary, u"This content experiment has issues that affect content visibility.", - ValidationMessageType.error + StudioValidationMessage.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', [Group("0", 'alpha')]) ] - messages = split_test_module.validation_messages() - self.assertEqual(len(messages), 1) + validation = split_test_module.validate() + self.assertEqual(len(validation.messages), 1) verify_validation_message( - messages[0], + validation.messages[0], u"The experiment has an inactive group. Move content into active groups, then delete the inactive group.", - ValidationMessageType.warning + StudioValidationMessage.WARNING ) - verify_general_validation_message( - split_test_module.general_validation_message, + verify_summary_message( + validation.summary, u"This content experiment has issues that affect content visibility.", - ValidationMessageType.warning + StudioValidationMessage.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', [Group("0", 'alpha'), Group("2", 'gamma')]) ] - messages = split_test_module.validation_messages() - self.assertEqual(len(messages), 2) + validation = split_test_module.validate() + self.assertEqual(len(validation.messages), 2) verify_validation_message( - messages[0], + validation.messages[0], u"The experiment does not contain all of the groups in the configuration.", - ValidationMessageType.error, - 'add-missing-groups-button', - u"Add Missing Groups" + StudioValidationMessage.ERROR, + expected_action_runtime_event='add-missing-groups', + expected_action_label=u"Add Missing Groups" ) verify_validation_message( - messages[1], + validation.messages[1], u"The experiment has an inactive group. Move content into active groups, then delete the inactive group.", - ValidationMessageType.warning + StudioValidationMessage.WARNING ) # With two messages of type error and warning priority given to error. - verify_general_validation_message( - split_test_module.general_validation_message, + verify_summary_message( + validation.summary, u"This content experiment has issues that affect content visibility.", - ValidationMessageType.error + StudioValidationMessage.ERROR ) # Verify the messages for a split test referring to a non-existent user partition split_test_module.user_partition_id = 2 - messages = split_test_module.validation_messages() - self.assertEqual(len(messages), 1) + validation = split_test_module.validate() + self.assertEqual(len(validation.messages), 1) verify_validation_message( - messages[0], + validation.messages[0], u"The experiment uses a deleted group configuration. " u"Select a valid group configuration or delete this experiment.", - ValidationMessageType.error + StudioValidationMessage.ERROR ) - verify_general_validation_message( - split_test_module.general_validation_message, + verify_summary_message( + validation.summary, u"This content experiment has issues that affect content visibility.", - ValidationMessageType.error + StudioValidationMessage.ERROR ) diff --git a/common/lib/xmodule/xmodule/tests/test_validation.py b/common/lib/xmodule/xmodule/tests/test_validation.py new file mode 100644 index 0000000000..f3e6b95b50 --- /dev/null +++ b/common/lib/xmodule/xmodule/tests/test_validation.py @@ -0,0 +1,218 @@ +""" +Test xblock/validation.py +""" + +import unittest +from xblock.test.tools import assert_raises + +from xmodule.validation import StudioValidationMessage, StudioValidation +from xblock.validation import Validation, ValidationMessage + + +class StudioValidationMessageTest(unittest.TestCase): + """ + Tests for `ValidationMessage` + """ + + def test_bad_parameters(self): + """ + Test that `TypeError`s are thrown for bad input parameters. + """ + with assert_raises(TypeError): + StudioValidationMessage("unknown type", u"Unknown type info") + + with assert_raises(TypeError): + StudioValidationMessage(StudioValidationMessage.WARNING, u"bad warning", action_class=0) + + with assert_raises(TypeError): + StudioValidationMessage(StudioValidationMessage.WARNING, u"bad warning", action_runtime_event=0) + + with assert_raises(TypeError): + StudioValidationMessage(StudioValidationMessage.WARNING, u"bad warning", action_label="Non-unicode string") + + def test_to_json(self): + """ + Test the `to_json` method. + """ + self.assertEqual( + { + "type": StudioValidationMessage.NOT_CONFIGURED, + "text": u"Not Configured message", + "action_label": u"Action label" + }, + StudioValidationMessage( + StudioValidationMessage.NOT_CONFIGURED, u"Not Configured message", action_label=u"Action label" + ).to_json() + ) + + self.assertEqual( + { + "type": StudioValidationMessage.WARNING, + "text": u"Warning message", + "action_class": "class-for-action" + }, + StudioValidationMessage( + StudioValidationMessage.WARNING, u"Warning message", action_class="class-for-action" + ).to_json() + ) + + self.assertEqual( + { + "type": StudioValidationMessage.ERROR, + "text": u"Error message", + "action_runtime_event": "do-fix-up" + }, + StudioValidationMessage( + StudioValidationMessage.ERROR, u"Error message", action_runtime_event="do-fix-up" + ).to_json() + ) + + +class StudioValidationTest(unittest.TestCase): + """ + Tests for `StudioValidation` class. + """ + def test_copy(self): + validation = Validation("id") + validation.add(ValidationMessage(ValidationMessage.ERROR, u"Error message")) + + studio_validation = StudioValidation.copy(validation) + self.assertIsInstance(studio_validation, StudioValidation) + self.assertFalse(studio_validation) + self.assertEqual(1, len(studio_validation.messages)) + expected = { + "type": StudioValidationMessage.ERROR, + "text": u"Error message" + } + self.assertEqual(expected, studio_validation.messages[0].to_json()) + self.assertIsNone(studio_validation.summary) + + def test_copy_studio_validation(self): + validation = StudioValidation("id") + validation.add( + StudioValidationMessage(StudioValidationMessage.WARNING, u"Warning message", action_label=u"Action Label") + ) + + validation_copy = StudioValidation.copy(validation) + self.assertFalse(validation_copy) + self.assertEqual(1, len(validation_copy.messages)) + expected = { + "type": StudioValidationMessage.WARNING, + "text": u"Warning message", + "action_label": u"Action Label" + } + self.assertEqual(expected, validation_copy.messages[0].to_json()) + + def test_copy_errors(self): + with assert_raises(TypeError): + StudioValidation.copy("foo") + + def test_empty(self): + """ + Test that `empty` return True iff there are no messages and no summary. + Also test the "bool" property of `Validation`. + """ + validation = StudioValidation("id") + self.assertTrue(validation.empty) + self.assertTrue(validation) + + validation.add(StudioValidationMessage(StudioValidationMessage.ERROR, u"Error message")) + self.assertFalse(validation.empty) + self.assertFalse(validation) + + validation_with_summary = StudioValidation("id") + validation_with_summary.set_summary( + StudioValidationMessage(StudioValidationMessage.NOT_CONFIGURED, u"Summary message") + ) + self.assertFalse(validation.empty) + self.assertFalse(validation) + + def test_add_messages(self): + """ + Test the behavior of calling `add_messages` with combination of `StudioValidation` instances. + """ + validation_1 = StudioValidation("id") + validation_1.set_summary(StudioValidationMessage(StudioValidationMessage.WARNING, u"Summary message")) + validation_1.add(StudioValidationMessage(StudioValidationMessage.ERROR, u"Error message")) + + validation_2 = StudioValidation("id") + validation_2.set_summary(StudioValidationMessage(StudioValidationMessage.ERROR, u"Summary 2 message")) + validation_2.add(StudioValidationMessage(StudioValidationMessage.NOT_CONFIGURED, u"Not configured")) + + validation_1.add_messages(validation_2) + self.assertEqual(2, len(validation_1.messages)) + + self.assertEqual(StudioValidationMessage.ERROR, validation_1.messages[0].type) + self.assertEqual(u"Error message", validation_1.messages[0].text) + + self.assertEqual(StudioValidationMessage.NOT_CONFIGURED, validation_1.messages[1].type) + self.assertEqual(u"Not configured", validation_1.messages[1].text) + + self.assertEqual(StudioValidationMessage.WARNING, validation_1.summary.type) + self.assertEqual(u"Summary message", validation_1.summary.text) + + def test_set_summary_accepts_validation_message(self): + """ + Test that `set_summary` accepts a ValidationMessage. + """ + validation = StudioValidation("id") + validation.set_summary(ValidationMessage(ValidationMessage.WARNING, u"Summary message")) + self.assertEqual(ValidationMessage.WARNING, validation.summary.type) + self.assertEqual(u"Summary message", validation.summary.text) + + def test_set_summary_errors(self): + """ + Test that `set_summary` errors if argument is not a ValidationMessage. + """ + with assert_raises(TypeError): + StudioValidation("id").set_summary("foo") + + def test_to_json(self): + """ + Test the ability to serialize a `StudioValidation` instance. + """ + validation = StudioValidation("id") + expected = { + "xblock_id": "id", + "messages": [], + "empty": True + } + self.assertEqual(expected, validation.to_json()) + + validation.add( + StudioValidationMessage( + StudioValidationMessage.ERROR, + u"Error message", + action_label=u"Action label", + action_class="edit-button" + ) + ) + validation.add( + StudioValidationMessage( + StudioValidationMessage.NOT_CONFIGURED, + u"Not configured message", + action_label=u"Action label", + action_runtime_event="make groups" + ) + ) + validation.set_summary( + StudioValidationMessage( + StudioValidationMessage.WARNING, + u"Summary message", + action_label=u"Summary label", + action_runtime_event="fix everything" + ) + ) + + # Note: it is important to test all the expected strings here because the client-side model depends on them + # (for instance, "warning" vs. using the xblock constant ValidationMessageTypes.WARNING). + expected = { + "xblock_id": "id", + "messages": [ + {"type": "error", "text": u"Error message", "action_label": u"Action label", "action_class": "edit-button"}, + {"type": "not-configured", "text": u"Not configured message", "action_label": u"Action label", "action_runtime_event": "make groups"} + ], + "summary": {"type": "warning", "text": u"Summary message", "action_label": u"Summary label", "action_runtime_event": "fix everything"}, + "empty": False + } + self.assertEqual(expected, validation.to_json()) diff --git a/common/lib/xmodule/xmodule/validation.py b/common/lib/xmodule/xmodule/validation.py new file mode 100644 index 0000000000..2b6d4fce59 --- /dev/null +++ b/common/lib/xmodule/xmodule/validation.py @@ -0,0 +1,128 @@ +""" +Extension of XBlock Validation class to include information for presentation in Studio. +""" +from xblock.validation import Validation, ValidationMessage + + +class StudioValidationMessage(ValidationMessage): + """ + A message containing validation information about an xblock, extended to provide Studio-specific fields. + """ + + # A special message type indicating that the xblock is not yet configured. This message may be rendered + # in a different way within Studio. + NOT_CONFIGURED = "not-configured" + + TYPES = [ValidationMessage.WARNING, ValidationMessage.ERROR, NOT_CONFIGURED] + + def __init__(self, message_type, message_text, action_label=None, action_class=None, action_runtime_event=None): + """ + Create a new message. + + Args: + message_type (str): The type associated with this message. Most be `WARNING` or `ERROR`. + message_text (unicode): The textual message. + action_label (unicode): Text to show on a "fix-up" action (optional). If present, either `action_class` + or `action_runtime_event` should be specified. + action_class (str): A class to link to the "fix-up" action (optional). A click handler must be added + for this class, unless it is "edit-button", "duplicate-button", or "delete-button" (which are all + handled in general for xblock instances. + action_runtime_event (str): An event name to be triggered on the xblock client-side runtime when + the "fix-up" action is clicked (optional). + """ + super(StudioValidationMessage, self).__init__(message_type, message_text) + if action_label is not None: + if not isinstance(action_label, unicode): + raise TypeError("Action label must be unicode.") + self.action_label = action_label + if action_class is not None: + if not isinstance(action_class, basestring): + raise TypeError("Action class must be a string.") + self.action_class = action_class + if action_runtime_event is not None: + if not isinstance(action_runtime_event, basestring): + raise TypeError("Action runtime event must be a string.") + self.action_runtime_event = action_runtime_event + + def to_json(self): + """ + Convert to a json-serializable representation. + + Returns: + dict: A dict representation that is json-serializable. + """ + serialized = super(StudioValidationMessage, self).to_json() + if hasattr(self, "action_label"): + serialized["action_label"] = self.action_label + if hasattr(self, "action_class"): + serialized["action_class"] = self.action_class + if hasattr(self, "action_runtime_event"): + serialized["action_runtime_event"] = self.action_runtime_event + return serialized + + +class StudioValidation(Validation): + """ + Extends `Validation` to add Studio-specific summary message. + """ + + @classmethod + def copy(cls, validation): + """ + Copies the `Validation` object to a `StudioValidation` object. This is a shallow copy. + + Args: + validation (Validation): A `Validation` object to be converted to a `StudioValidation` instance. + + Returns: + StudioValidation: A `StudioValidation` instance populated with the messages from supplied + `Validation` object + """ + if not isinstance(validation, Validation): + raise TypeError("Copy must be called with a Validation instance") + studio_validation = cls(validation.xblock_id) + studio_validation.messages = validation.messages + return studio_validation + + def __init__(self, xblock_id): + """ + Create a `StudioValidation` instance. + + Args: + xblock_id (object): An identification object that must support conversion to unicode. + """ + super(StudioValidation, self).__init__(xblock_id) + self.summary = None + + def set_summary(self, message): + """ + Sets a summary message on this instance. The summary is optional. + + Args: + message (ValidationMessage): A validation message to set as this instance's summary. + """ + if not isinstance(message, ValidationMessage): + raise TypeError("Argument must of type ValidationMessage") + self.summary = message + + @property + def empty(self): + """ + Is this object empty (contains no messages and no summary)? + + Returns: + bool: True iff this instance has no validation issues and therefore has no messages or summary. + """ + return super(StudioValidation, self).empty and not self.summary + + def to_json(self): + """ + Convert to a json-serializable representation. + + Returns: + dict: A dict representation that is json-serializable. + """ + serialized = super(StudioValidation, self).to_json() + if self.summary: + serialized["summary"] = self.summary.to_json() + return serialized diff --git a/common/test/acceptance/pages/studio/container.py b/common/test/acceptance/pages/studio/container.py index 6eac9f26d4..878a0e0bbf 100644 --- a/common/test/acceptance/pages/studio/container.py +++ b/common/test/acceptance/pages/studio/container.py @@ -16,6 +16,7 @@ class ContainerPage(PageObject): NAME_SELECTOR = '.page-header-title' NAME_INPUT_SELECTOR = '.page-header .xblock-field-input' NAME_FIELD_WRAPPER_SELECTOR = '.page-header .wrapper-xblock-field' + ADD_MISSING_GROUPS_SELECTOR = '.notification-action-button[data-notification-action="add-missing-groups"]' def __init__(self, browser, locator): super(ContainerPage, self).__init__(browser) @@ -246,7 +247,7 @@ class ContainerPage(PageObject): Click the "add missing groups" link. Note that this does an ajax call. """ - self.q(css='.add-missing-groups-button').first.click() + self.q(css=self.ADD_MISSING_GROUPS_SELECTOR).first.click() self.wait_for_ajax() # Wait until all xblocks rendered. @@ -256,7 +257,7 @@ class ContainerPage(PageObject): """ Returns True if the "add missing groups" button is present. """ - return self.q(css='.add-missing-groups-button').present + return self.q(css=self.ADD_MISSING_GROUPS_SELECTOR).present def get_xblock_information_message(self): """ diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index b9bc5b2f6a..185806411f 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -18,7 +18,7 @@ from courseware.tests.factories import StudentPrefsFactory, StudentInfoFactory from xblock.fields import Scope, BlockScope, ScopeIds from django.test import TestCase from django.db import DatabaseError -from xblock.core import KeyValueMultiSaveError +from xblock.exceptions import KeyValueMultiSaveError def mock_field(scope, name): diff --git a/lms/templates/split_test_author_view.html b/lms/templates/split_test_author_view.html index ece87663c8..ede487e199 100644 --- a/lms/templates/split_test_author_view.html +++ b/lms/templates/split_test_author_view.html @@ -1,20 +1,13 @@ <%! from django.utils.translation import ugettext as _ %> -<%! from xmodule.split_test_module import ValidationMessageType %> <% split_test = context.get('split_test') user_partition = split_test.descriptor.get_selected_partition() -messages = split_test.descriptor.validation_messages() show_link = group_configuration_url is not None %> -% if is_root and not is_configured: -
    -% else: -
    -% endif - -% if user_partition: +% if is_configured: +

    @@ -24,56 +17,8 @@ show_link = group_configuration_url is not None

    -% endif -% if len(messages) > 0: - <% - general_validation = split_test.descriptor.general_validation_message - def get_validation_icon(validation_type): - if validation_type == ValidationMessageType.error: - return 'icon-exclamation-sign' - elif validation_type == ValidationMessageType.warning: - return 'icon-warning-sign' - return None - - aggregate_validation_class = 'has-errors' if general_validation['type']==ValidationMessageType.error else ' has-warnings' - %> -
    - % if is_configured: -

    - ${general_validation['message']} -

    - % endif - % if is_root or not is_configured: -
      - % for message in messages: - <% - message_type = message.message_type - message_type_display_name = ValidationMessageType.display_name(message_type) if message_type else None - %> -
    • - % if not is_configured: - - % endif - - % if message_type_display_name: - ${message_type_display_name}: - % endif - ${unicode(message)} - - % if message.action_class: - - ${message.action_label} - - % endif - -
      • - % endfor -
    - % endif -
    -% endif -
    +% endif % if is_root:
    diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index e86c3bbdee..e50060288a 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -20,7 +20,7 @@ -e git+https://github.com/pmitros/django-pyfs.git@d175715e0fe3367ec0f1ee429c242d603f6e8b10#egg=djpyfs # Our libraries: --e git+https://github.com/edx/XBlock.git@246811773c67a84fdb17614a8e9f7ec7b1890574#egg=XBlock +-e git+https://github.com/edx/XBlock.git@2029af2a4b524310847decfb34ef39da8a30dc4e#egg=XBlock -e git+https://github.com/edx/codejail.git@66dd5a45e5072666ff9a70c768576e9ffd1daa4b#egg=codejail -e git+https://github.com/edx/diff-cover.git@v0.7.1#egg=diff_cover -e git+https://github.com/edx/js-test-tool.git@v0.1.5#egg=js_test_tool