Skip to content

Commit 08ce09b

Browse files
author
cahrens
committed
Display validation messages for any xblock on the container page.
TNL-683
1 parent cb5e90f commit 08ce09b

26 files changed

+1145
-336
lines changed

‎CHANGELOG.rst

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ the top. Include a label indicating the component affected.
77

88
Common: Add configurable reset button to units
99

10+
Studio: Add support xblock validation messages on Studio unit/container page. TNL-683
11+
1012
LMS: Support adding cohorts from the instructor dashboard. TNL-162
1113

1214
LMS: Support adding students to a cohort via the instructor dashboard. TNL-163

‎cms/djangoapps/contentstore/views/course.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1290,10 +1290,12 @@ def _get_usage_info(store, course, split_tests):
12901290
'container_handler',
12911291
course.location.course_key.make_usage_key(unit.location.block_type, unit.location.name)
12921292
)
1293+
1294+
validation_summary = split_test.general_validation_message()
12931295
usage_info[split_test.user_partition_id].append({
12941296
'label': '{} / {}'.format(unit.display_name, split_test.display_name),
12951297
'url': unit_url,
1296-
'validation': split_test.general_validation_message,
1298+
'validation': validation_summary.to_json() if validation_summary else None,
12971299
})
12981300
return usage_info
12991301

‎cms/djangoapps/contentstore/views/tests/test_group_configurations.py

Lines changed: 49 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
from contentstore.tests.utils import CourseTestCase
1010
from xmodule.partitions.partitions import Group, UserPartition
1111
from xmodule.modulestore.tests.factories import ItemFactory
12-
from xmodule.split_test_module import ValidationMessage, ValidationMessageType
12+
from xmodule.validation import StudioValidation, StudioValidationMessage
1313
from xmodule.modulestore.django import modulestore
1414
from xmodule.modulestore import ModuleStoreEnum
1515

@@ -541,87 +541,75 @@ class GroupConfigurationsValidationTestCase(CourseTestCase, HelperMethods):
541541
def setUp(self):
542542
super(GroupConfigurationsValidationTestCase, self).setUp()
543543

544-
@patch('xmodule.split_test_module.SplitTestDescriptor.validation_messages')
545-
def test_error_message_present(self, mocked_validation_messages):
544+
@patch('xmodule.split_test_module.SplitTestDescriptor.validate_split_test')
545+
def verify_validation_add_usage_info(self, expected_result, mocked_message, mocked_validation_messages):
546546
"""
547-
Tests if validation message is present.
547+
Helper method for testing validation information present after add_usage_info.
548548
"""
549549
self._add_user_partitions()
550550
split_test = self._create_content_experiment(cid=0, name_suffix='0')[1]
551551

552-
mocked_validation_messages.return_value = [
553-
ValidationMessage(
554-
split_test,
555-
u"Validation message",
556-
ValidationMessageType.error
557-
)
558-
]
552+
validation = StudioValidation(split_test.location)
553+
validation.add(mocked_message)
554+
mocked_validation_messages.return_value = validation
555+
559556
group_configuration = GroupConfiguration.add_usage_info(self.course, self.store)[0]
560-
self.assertEqual(
561-
group_configuration['usage'][0]['validation'],
562-
{
563-
'message': u'This content experiment has issues that affect content visibility.',
564-
'type': 'error'
565-
}
566-
)
557+
self.assertEqual(expected_result.to_json(), group_configuration['usage'][0]['validation'])
567558

568-
@patch('xmodule.split_test_module.SplitTestDescriptor.validation_messages')
569-
def test_warning_message_present(self, mocked_validation_messages):
559+
def test_error_message_present(self):
570560
"""
571-
Tests if validation message is present.
561+
Tests if validation message is present (error case).
572562
"""
573-
self._add_user_partitions()
574-
split_test = self._create_content_experiment(cid=0, name_suffix='0')[1]
563+
mocked_message = StudioValidationMessage(StudioValidationMessage.ERROR, u"Validation message")
564+
expected_result = StudioValidationMessage(
565+
StudioValidationMessage.ERROR, u"This content experiment has issues that affect content visibility."
566+
)
567+
self.verify_validation_add_usage_info(expected_result, mocked_message) # pylint: disable=no-value-for-parameter
575568

576-
mocked_validation_messages.return_value = [
577-
ValidationMessage(
578-
split_test,
579-
u"Validation message",
580-
ValidationMessageType.warning
581-
)
582-
]
583-
group_configuration = GroupConfiguration.add_usage_info(self.course, self.store)[0]
584-
self.assertEqual(
585-
group_configuration['usage'][0]['validation'],
586-
{
587-
'message': u'This content experiment has issues that affect content visibility.',
588-
'type': 'warning'
589-
}
569+
def test_warning_message_present(self):
570+
"""
571+
Tests if validation message is present (warning case).
572+
"""
573+
mocked_message = StudioValidationMessage(StudioValidationMessage.WARNING, u"Validation message")
574+
expected_result = StudioValidationMessage(
575+
StudioValidationMessage.WARNING, u"This content experiment has issues that affect content visibility."
590576
)
577+
self.verify_validation_add_usage_info(expected_result, mocked_message) # pylint: disable=no-value-for-parameter
591578

592-
@patch('xmodule.split_test_module.SplitTestDescriptor.validation_messages')
593-
def test_update_usage_info(self, mocked_validation_messages):
579+
@patch('xmodule.split_test_module.SplitTestDescriptor.validate_split_test')
580+
def verify_validation_update_usage_info(self, expected_result, mocked_message, mocked_validation_messages):
594581
"""
595-
Tests if validation message is present when updating usage info.
582+
Helper method for testing validation information present after update_usage_info.
596583
"""
597584
self._add_user_partitions()
598585
split_test = self._create_content_experiment(cid=0, name_suffix='0')[1]
599586

600-
mocked_validation_messages.return_value = [
601-
ValidationMessage(
602-
split_test,
603-
u"Validation message",
604-
ValidationMessageType.warning
605-
)
606-
]
607-
608-
group_configuration = GroupConfiguration.update_usage_info(self.store, self.course, self.course.user_partitions[0])
587+
validation = StudioValidation(split_test.location)
588+
if mocked_message is not None:
589+
validation.add(mocked_message)
590+
mocked_validation_messages.return_value = validation
609591

592+
group_configuration = GroupConfiguration.update_usage_info(
593+
self.store, self.course, self.course.user_partitions[0]
594+
)
610595
self.assertEqual(
611-
group_configuration['usage'][0]['validation'],
612-
{
613-
'message': u'This content experiment has issues that affect content visibility.',
614-
'type': 'warning'
615-
}
596+
expected_result.to_json() if expected_result is not None else None,
597+
group_configuration['usage'][0]['validation']
616598
)
617599

618-
@patch('xmodule.split_test_module.SplitTestDescriptor.validation_messages')
619-
def test_update_usage_info_no_message(self, mocked_validation_messages):
600+
def test_update_usage_info(self):
601+
"""
602+
Tests if validation message is present when updating usage info.
603+
"""
604+
mocked_message = StudioValidationMessage(StudioValidationMessage.WARNING, u"Validation message")
605+
expected_result = StudioValidationMessage(
606+
StudioValidationMessage.WARNING, u"This content experiment has issues that affect content visibility."
607+
)
608+
# pylint: disable=no-value-for-parameter
609+
self.verify_validation_update_usage_info(expected_result, mocked_message)
610+
611+
def test_update_usage_info_no_message(self):
620612
"""
621613
Tests if validation message is not present when updating usage info.
622614
"""
623-
self._add_user_partitions()
624-
self._create_content_experiment(cid=0, name_suffix='0')
625-
mocked_validation_messages.return_value = []
626-
group_configuration = GroupConfiguration.update_usage_info(self.store, self.course, self.course.user_partitions[0])
627-
self.assertEqual(group_configuration['usage'][0]['validation'], None)
615+
self.verify_validation_update_usage_info(None, None) # pylint: disable=no-value-for-parameter

‎cms/djangoapps/contentstore/views/tests/test_item.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,10 @@ def test_get_empty_container_fragment(self):
130130
root_usage_key = self._create_vertical()
131131
html, __ = self._get_container_preview(root_usage_key)
132132

133-
# Verify that the Studio wrapper is not added
134-
self.assertNotIn('wrapper-xblock', html)
133+
# XBlock messages are added by the Studio wrapper.
134+
self.assertIn('wrapper-xblock-message', html)
135+
# Make sure that "wrapper-xblock" does not appear by itself (without -message at end).
136+
self.assertNotRegexpMatches(html, r'wrapper-xblock[^-]+')
135137

136138
# Verify that the header and article tags are still added
137139
self.assertIn('<header class="xblock-header xblock-header-vertical">', html)

‎cms/static/coffee/spec/main.coffee

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,7 @@ define([
213213
"js/spec/models/component_template_spec",
214214
"js/spec/models/explicit_url_spec",
215215
"js/spec/models/xblock_info_spec",
216+
"js/spec/models/xblock_validation_spec",
216217

217218
"js/spec/utils/drag_and_drop_spec",
218219
"js/spec/utils/handle_iframe_binding_spec",
@@ -228,6 +229,7 @@ define([
228229
"js/spec/views/xblock_spec",
229230
"js/spec/views/xblock_editor_spec",
230231
"js/spec/views/xblock_string_field_editor_spec",
232+
"js/spec/views/xblock_validation_spec",
231233

232234
"js/spec/views/utils/view_utils_spec",
233235

‎cms/static/js/models/xblock_validation.js

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
define(["backbone", "gettext", "underscore"], function (Backbone, gettext, _) {
2+
/**
3+
* Model for xblock validation messages as displayed in Studio.
4+
*/
5+
var XBlockValidationModel = Backbone.Model.extend({
6+
defaults: {
7+
summary: {},
8+
messages: [],
9+
empty: true,
10+
xblock_id: null
11+
},
12+
13+
WARNING : "warning",
14+
ERROR: "error",
15+
NOT_CONFIGURED: "not-configured",
16+
17+
parse: function(response) {
18+
if (!response.empty) {
19+
var summary = "summary" in response ? response.summary : {};
20+
var messages = "messages" in response ? response.messages : [];
21+
if (!(_.has(summary, "text")) || !summary.text) {
22+
summary.text = gettext("This component has validation issues.");
23+
}
24+
if (!(_.has(summary, "type")) || !summary.type) {
25+
summary.type = this.WARNING;
26+
// Possible types are ERROR, WARNING, and NOT_CONFIGURED. NOT_CONFIGURED is treated as a warning.
27+
_.find(messages, function (message) {
28+
if (message.type === this.ERROR) {
29+
summary.type = this.ERROR;
30+
return true;
31+
}
32+
return false;
33+
}, this);
34+
}
35+
response.summary = summary;
36+
if (response.showSummaryOnly) {
37+
messages = [];
38+
}
39+
response.messages = messages;
40+
}
41+
42+
return response;
43+
}
44+
});
45+
return XBlockValidationModel;
46+
});

‎cms/static/js/spec/models/xblock_validation_spec.js

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
define(['js/models/xblock_validation'],
2+
function(XBlockValidationModel) {
3+
var verifyModel;
4+
5+
verifyModel = function(model, expected_empty, expected_summary, expected_messages, expected_xblock_id) {
6+
expect(model.get("empty")).toBe(expected_empty);
7+
expect(model.get("summary")).toEqual(expected_summary);
8+
expect(model.get("messages")).toEqual(expected_messages);
9+
expect(model.get("xblock_id")).toBe(expected_xblock_id);
10+
};
11+
12+
describe('XBlockValidationModel', function() {
13+
it('handles empty variable', function() {
14+
verifyModel(new XBlockValidationModel({parse: true}), true, {}, [], null);
15+
verifyModel(new XBlockValidationModel({"empty": true}, {parse: true}), true, {}, [], null);
16+
17+
// It is assumed that the "empty" state on the JSON object passed in is correct
18+
// (no attempt is made to correct other variables based on empty==true).
19+
verifyModel(
20+
new XBlockValidationModel(
21+
{"empty": true, "messages": [{"text": "Bad JSON case"}], "xblock_id": "id"},
22+
{parse: true}
23+
),
24+
true,
25+
{},
26+
[{"text": "Bad JSON case"}], "id"
27+
);
28+
});
29+
30+
it('creates a summary if not defined', function() {
31+
// Single warning message.
32+
verifyModel(
33+
new XBlockValidationModel({
34+
"empty": false,
35+
"xblock_id": "id"
36+
}, {parse: true}),
37+
false,
38+
{"text": "This component has validation issues.", "type": "warning"},
39+
[],
40+
"id"
41+
);
42+
// Two messages that compute to a "warning" state in the summary.
43+
verifyModel(
44+
new XBlockValidationModel({
45+
"empty": false,
46+
"messages": [{"text": "one", "type": "not-configured"}, {"text": "two", "type": "warning"}],
47+
"xblock_id": "id"
48+
}, {parse: true}),
49+
false,
50+
{"text": "This component has validation issues.", "type": "warning"},
51+
[{"text": "one", "type": "not-configured"}, {"text": "two", "type": "warning"}],
52+
"id"
53+
);
54+
// Two messages, with one of them "error", resulting in an "error" state in the summary.
55+
verifyModel(
56+
new XBlockValidationModel({
57+
"empty": false,
58+
"messages": [{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}],
59+
"xblock_id": "id"
60+
}, {parse: true}),
61+
false,
62+
{"text": "This component has validation issues.", "type": "error"},
63+
[{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}],
64+
"id"
65+
);
66+
});
67+
68+
it('respects summary properties that are defined', function() {
69+
// Summary already present (both text and type), no messages.
70+
verifyModel(
71+
new XBlockValidationModel({
72+
"empty": false,
73+
"xblock_id": "id",
74+
"summary": {"text": "my summary", "type": "custom type"}
75+
}, {parse: true}),
76+
false,
77+
{"text": "my summary", "type": "custom type"},
78+
[],
79+
"id"
80+
);
81+
// Summary text present, but not type (will get default value of warning).
82+
verifyModel(
83+
new XBlockValidationModel({
84+
"empty": false,
85+
"xblock_id": "id",
86+
"summary": {"text": "my summary"}
87+
}, {parse: true}),
88+
false,
89+
{"text": "my summary", "type": "warning"},
90+
[],
91+
"id"
92+
);
93+
// Summary type present, but not text.
94+
verifyModel(
95+
new XBlockValidationModel({
96+
"empty": false,
97+
"summary": {"type": "custom type"},
98+
"messages": [{"text": "one", "type": "not-configured"}, {"text": "two", "type": "warning"}],
99+
"xblock_id": "id"
100+
}, {parse: true}),
101+
false,
102+
{"text": "This component has validation issues.", "type": "custom type"},
103+
[{"text": "one", "type": "not-configured"}, {"text": "two", "type": "warning"}],
104+
"id"
105+
);
106+
// Summary text present, type will be computed as error.
107+
verifyModel(
108+
new XBlockValidationModel({
109+
"empty": false,
110+
"summary": {"text": "my summary"},
111+
"messages": [{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}],
112+
"xblock_id": "id"
113+
}, {parse: true}),
114+
false,
115+
{"text": "my summary", "type": "error"},
116+
[{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}],
117+
"id"
118+
);
119+
});
120+
121+
it('clears messages if showSummaryOnly is true', function() {
122+
verifyModel(
123+
new XBlockValidationModel({
124+
"empty": false,
125+
"xblock_id": "id",
126+
"summary": {"text": "my summary"},
127+
"messages": [{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}],
128+
"showSummaryOnly": true
129+
}, {parse: true}),
130+
false,
131+
{"text": "my summary", "type": "error"},
132+
[],
133+
"id"
134+
);
135+
136+
verifyModel(
137+
new XBlockValidationModel({
138+
"empty": false,
139+
"xblock_id": "id",
140+
"summary": {"text": "my summary"},
141+
"messages": [{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}],
142+
"showSummaryOnly": false
143+
}, {parse: true}),
144+
false,
145+
{"text": "my summary", "type": "error"},
146+
[{"text": "one", "type": "warning"}, {"text": "two", "type": "error"}],
147+
"id"
148+
);
149+
});
150+
});
151+
}
152+
);

0 commit comments

Comments
 (0)