Skip to content

E
edx-platform
Commit 6e949604 by Peter Fogg
Options

Fix many validation bugs and failing tests.

parent c2f0b7d4
Showing with 123 additions and 71 deletions
cms/djangoapps/contentstore/features/common.py
......@@ -68,7 +68,7 @@ def press_the_notification_button(_step, name):
error_showing = world.is_css_present('.is-shown.wrapper-notification-error')
return confirmation_dismissed or error_showing
assert_true(world.css_click(css, success_condition=button_clicked), '%s button not clicked after 5 attempts.' % name)
world.css_click(css, success_condition=button_clicked), '%s button not clicked after 5 attempts.' % name
@step('I change the "(.*)" field to "(.*)"$')
......@@ -228,6 +228,13 @@ def shows_captions(step, show_captions):
assert world.is_css_not_present('.video.closed')
@step('the save button is disabled$')
def save_button_disabled(step):
button_css = '.action-save'
disabled = 'is-disabled'
assert world.css_find(button_css)[0].has_class(disabled)
def type_in_codemirror(index, text):
world.css_click(".CodeMirror", index=index)
g = world.css_find("div.CodeMirror.CodeMirror-focused > div > textarea")
......
cms/djangoapps/contentstore/features/course-settings.feature
......@@ -65,3 +65,9 @@ Feature: Course Settings
And I change the course overview
And I press the "Save" notification button
Then I see a confirmation that my changes have been saved
Scenario: User cannot save invalid settings
Given I have opened a new course in Studio
When I select Schedule and Details
And I change the "Course Start Date" field to ""
Then the save button is disabled
cms/djangoapps/contentstore/features/grading.feature
......@@ -77,3 +77,10 @@ Feature: Course Grading
When I change assignment type "Homework" to "New Type"
And I press the "Save" notification button
Then I see a confirmation that my changes have been saved
Scenario: User cannot save invalid settings
Given I have opened a new course in Studio
And I have populated the course
And I am viewing the grading settings
When I change assignment type "Homework" to ""
Then the save button is disabled
cms/djangoapps/contentstore/features/grading.py
......@@ -60,8 +60,6 @@ def change_assignment_name(step, old_name, new_name):
for count in range(len(old_name)):
f._element.send_keys(Keys.END, Keys.BACK_SPACE)
f._element.send_keys(new_name)
# Without this, the "you've made changes" notification won't pop up
f._element.send_keys(Keys.ENTER)
@step(u'I go back to the main course page')
......@@ -94,8 +92,6 @@ def add_assignment_type(step, new_name):
name_id = '#course-grading-assignment-name'
f = world.css_find(name_id)[4]
f._element.send_keys(new_name)
# Without this, the "you've made changes" notification won't pop up
f._element.send_keys(Keys.ENTER)
@step(u'I have populated the course')
......
cms/static/coffee/spec/views/feedback_spec.coffee
......@@ -37,6 +37,10 @@ describe "CMS.Views.SystemFeedback", ->
@renderSpy = spyOn(CMS.Views.Alert.Confirmation.prototype, 'render').andCallThrough()
@showSpy = spyOn(CMS.Views.Alert.Confirmation.prototype, 'show').andCallThrough()
@hideSpy = spyOn(CMS.Views.Alert.Confirmation.prototype, 'hide').andCallThrough()
@clock = sinon.useFakeTimers()
afterEach ->
@clock.restore()
it "requires a type and an intent", ->
neither = =>
......@@ -80,8 +84,8 @@ describe "CMS.Views.SystemFeedback", ->
it "close button sends a .hide() message", ->
view = new CMS.Views.Alert.Confirmation(@options).show()
view.$(".action-close").click()
expect(@hideSpy).toHaveBeenCalled()
@clock.tick(900)
expect(view.$('.wrapper')).toBeHiding()
describe "CMS.Views.Prompt", ->
......
cms/static/js/models/settings/course_grading_policy.js
......@@ -77,18 +77,19 @@ CMS.Models.Settings.CourseGrader = Backbone.Model.extend({
}
else {
// FIXME somehow this.collection is unbound sometimes. I can't track down when
var existing = this.collection && this.collection.some(function(other) { return (other != this) && (other.get('type') == attrs['type']);}, this);
var existing = this.collection && this.collection.some(function(other) { return (other.cid != this.cid) && (other.get('type') == attrs['type']);}, this);
if (existing) {
errors.type = "There's already another assignment type with this name.";
}
}
}
if (_.has(attrs, 'weight')) {
if (!isFinite(attrs.weight) || /\D+/.test(attrs.weight)) {
var intWeight = parseInt(attrs.weight); // see if this ensures value saved is int
if (!isFinite(intWeight) || /\D+/.test(attrs.weight) || intWeight < 0 || intWeight > 100) {
errors.weight = "Please enter an integer between 0 and 100.";
}
else {
attrs.weight = parseInt(attrs.weight); // see if this ensures value saved is int
attrs.weight = intWeight;
if (this.collection && attrs.weight > 0) {
// FIXME b/c saves don't update the models if validation fails, we should
// either revert the field value to the one in the model and make them make room
......
cms/static/js/views/feedback.js
......@@ -140,7 +140,21 @@ CMS.Views.SystemFeedback = Backbone.View.extend({
CMS.Views.Alert = CMS.Views.SystemFeedback.extend({
options: $.extend({}, CMS.Views.SystemFeedback.prototype.options, {
type: "alert"
})
}),
slide_speed: 900,
show: function() {
CMS.Views.SystemFeedback.prototype.show.apply(this, arguments);
this.$el.hide();
this.$el.slideDown(this.slide_speed);
return this;
},
hide: function () {
this.$el.slideUp({
duration: this.slide_speed
});
setTimeout(_.bind(CMS.Views.SystemFeedback.prototype.hide, this, arguments),
this.slideSpeed);
}
});
CMS.Views.Notification = CMS.Views.SystemFeedback.extend({
options: $.extend({}, CMS.Views.SystemFeedback.prototype.options, {
......
cms/static/js/views/settings/main_settings_view.js
......@@ -3,10 +3,11 @@ if (!CMS.Views['Settings']) CMS.Views.Settings = {};
CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
// Model class is CMS.Models.Settings.CourseDetails
events : {
"input input" : "updateModel",
"input textarea" : "updateModel",
// Leaving change in as fallback for older browsers
"change input" : "updateModel",
"change textarea" : "updateModel",
'click .remove-course-syllabus' : "removeSyllabus",
'click .new-course-syllabus' : 'assetSyllabus',
'click .remove-course-introduction-video' : "removeVideo",
'focus #course-overview' : "codeMirrorize",
'mouseover #timezone' : "updateTime",
......@@ -28,6 +29,7 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
this.$el.find('#timezone').html("(" + dateIntrospect.getTimezone() + ")");
this.listenTo(this.model, 'invalid', this.handleValidationError);
this.listenTo(this.model, 'change', this.showNotificationBar);
this.selectorToField = _.invert(this.fieldToSelectorMap);
},
......@@ -37,25 +39,13 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
this.setupDatePicker('enrollment_start');
this.setupDatePicker('enrollment_end');
if (this.model.has('syllabus')) {
this.$el.find(this.fieldToSelectorMap['syllabus']).html(
this.fileAnchorTemplate({
fullpath : this.model.get('syllabus'),
filename: 'syllabus'}));
this.$el.find('.remove-course-syllabus').show();
}
else {
this.$el.find('#' + this.fieldToSelectorMap['syllabus']).html("");
this.$el.find('.remove-course-syllabus').hide();
}
this.$el.find('#' + this.fieldToSelectorMap['overview']).val(this.model.get('overview'));
this.codeMirrorize(null, $('#course-overview')[0]);
this.$el.find('.current-course-introduction-video iframe').attr('src', this.model.videosourceSample());
this.$el.find('#' + this.fieldToSelectorMap['intro_video']).val(this.model.get('intro_video') || '');
if (this.model.has('intro_video')) {
this.$el.find('.remove-course-introduction-video').show();
this.$el.find('#' + this.fieldToSelectorMap['intro_video']).val(this.model.get('intro_video'));
}
else this.$el.find('.remove-course-introduction-video').hide();
......@@ -68,7 +58,6 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
'end_date' : 'course-end',
'enrollment_start' : 'enrollment-start',
'enrollment_end' : 'enrollment-end',
'syllabus' : '.current-course-syllabus .doc-filename',
'overview' : 'course-overview',
'intro_video' : 'course-introduction-video',
'effort' : "course-effort"
......@@ -120,7 +109,13 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
timefield.on('changeTime', setfield);
datefield.datepicker('setDate', this.model.get(fieldName));
if (this.model.has(fieldName)) timefield.timepicker('setTime', this.model.get(fieldName));
// timepicker doesn't let us set null, so check that we have a time
if (this.model.has(fieldName)) {
timefield.timepicker('setTime', this.model.get(fieldName));
} // but reset the field either way
else {
timefield.val('');
}
},
updateModel: function(event) {
......@@ -129,34 +124,28 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
this.setField(event);
break;
// Don't make the user reload the page to check the Youtube ID.
// Wait for a second to load the video, avoiding egregious AJAX calls.
case 'course-introduction-video':
this.clearValidationErrors();
var previewsource = this.model.set_videosource($(event.currentTarget).val());
this.$el.find(".current-course-introduction-video iframe").attr("src", previewsource);
if (this.model.has('intro_video')) {
this.$el.find('.remove-course-introduction-video').show();
}
else {
this.$el.find('.remove-course-introduction-video').hide();
}
clearTimeout(this.videoTimer);
this.videoTimer = setTimeout(_.bind(function() {
this.$el.find(".current-course-introduction-video iframe").attr("src", previewsource);
if (this.model.has('intro_video')) {
this.$el.find('.remove-course-introduction-video').show();
}
else {
this.$el.find('.remove-course-introduction-video').hide();
}
}, this), 1000);
break;
default: // Everything else is handled by datepickers and CodeMirror.
break;
}
this.showNotificationBar(this.save_message,
_.bind(this.saveView, this),
_.bind(this.revertView, this));
},
removeSyllabus: function() {
if (this.model.has('syllabus')) this.setAndValidate('syllabus', null);
},
assetSyllabus : function() {
// TODO implement
},
removeVideo: function() {
removeVideo: function(event) {
event.preventDefault();
if (this.model.has('intro_video')) {
this.model.set_videosource(null);
this.$el.find(".current-course-introduction-video iframe").attr("src", "");
......@@ -185,9 +174,6 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
var newVal = mirror.getValue();
if (cachethis.model.get(field) != newVal) {
cachethis.setAndValidate(field, newVal);
cachethis.showNotificationBar(cachethis.save_message,
_.bind(cachethis.saveView, cachethis),
_.bind(cachethis.revertView, cachethis));
}
}
});
......@@ -208,7 +194,8 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
mirror.setValue(self.model.get(field));
});
},
reset: true});
reset: true,
silent: true});
},
setAndValidate: function(attr, value) {
// If we call model.set() with {validate: true}, model fields
......@@ -219,6 +206,15 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
// call validate ourselves.
this.model.set(attr, value);
this.model.isValid();
},
showNotificationBar: function() {
// We always call showNotificationBar with the same args, just
// delegate to superclass
CMS.Views.ValidatingView.prototype.showNotificationBar.call(this,
this.save_message,
_.bind(this.saveView, this),
_.bind(this.revertView, this));
}
});
cms/static/js/views/settings/settings_grading_view.js
......@@ -3,6 +3,9 @@ if (!CMS.Views['Settings']) CMS.Views.Settings = {}; // ensure the pseudo pkg ex
CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
// Model class is CMS.Models.Settings.CourseGradingPolicy
events : {
"input input" : "updateModel",
"input textarea" : "updateModel",
// Leaving change in as fallback for older browsers
"change input" : "updateModel",
"change textarea" : "updateModel",
"change span[contenteditable=true]" : "updateDesignation",
......@@ -62,7 +65,12 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
function (event) {
gradeCollection.on(event, function() {
this.showNotificationBar();
this.render();
// Since the change event gets fired every time
// we type in an input field, we don't need to
// (and really shouldn't) rerender the whole view.
if(event !== 'change') {
this.render();
}
}, this);
},
this);
......@@ -110,7 +118,7 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
default:
this.setField(event);
break;
break;
}
},
......@@ -347,6 +355,9 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
CMS.Views.Settings.GraderView = CMS.Views.ValidatingView.extend({
// Model class is CMS.Models.Settings.CourseGrader
events : {
"input input" : "updateModel",
"input textarea" : "updateModel",
// Leaving change in as fallback for older browsers
"change input" : "updateModel",
"change textarea" : "updateModel",
"click .remove-grading-data" : "deleteModel",
......@@ -370,7 +381,7 @@ CMS.Views.Settings.GraderView = CMS.Views.ValidatingView.extend({
'drop_count' : 'course-grading-assignment-droppable',
'weight' : 'course-grading-assignment-gradeweight'
},
updateModel : function(event) {
updateModel: function(event) {
// HACK to fix model sometimes losing its pointer to the collection [I think I fixed this but leaving
// this in out of paranoia. If this error ever happens, the user will get a warning that they cannot
// give 2 assignments the same name.]
......@@ -384,13 +395,14 @@ CMS.Views.Settings.GraderView = CMS.Views.ValidatingView.extend({
this.setField(event);
break;
case 'course-grading-assignment-name':
var oldName = this.model.get('type');
// Keep the original name, until we save
this.oldName = this.oldName === undefined ? this.model.get('type') : this.oldName;
// If the name has changed, alert the user to change all subsection names.
if (this.setField(event) != oldName && !_.isEmpty(oldName)) {
if (this.setField(event) != this.oldName && !_.isEmpty(this.oldName)) {
// overload the error display logic
this._cacheValidationErrors.push(event.currentTarget);
$(event.currentTarget).parent().append(
this.errorTemplate({message : 'For grading to work, you must change all "' + oldName +
this.errorTemplate({message : 'For grading to work, you must change all "' + this.oldName +
'" subsections to "' + this.model.get('type') + '".'}));
}
break;
......
cms/static/js/views/validating_view.js
......@@ -9,7 +9,10 @@ CMS.Views.ValidatingView = Backbone.View.extend({
errorTemplate : _.template('<span class="message-error"><%= message %></span>'),
save_title: gettext("You've made some changes"),
save_message: gettext("Your changes will not take effect until you save your progress."),
error_title: gettext("You've made some changes, but there are some errors"),
error_message: gettext("Please address the errors on this page first, and then save your progress."),
events : {
"change input" : "clearValidationErrors",
......@@ -22,6 +25,7 @@ CMS.Views.ValidatingView = Backbone.View.extend({
_cacheValidationErrors : [],
handleValidationError : function(model, error) {
this.clearValidationErrors();
// error is object w/ fields and error strings
for (var field in error) {
var ele = this.$el.find('#' + this.fieldToSelectorMap[field]);
......@@ -29,6 +33,11 @@ CMS.Views.ValidatingView = Backbone.View.extend({
this.getInputElements(ele).addClass('error');
$(ele).parent().append(this.errorTemplate({message : error[field]}));
}
$('.wrapper-notification-warning').addClass('wrapper-notification-warning-w-errors');
$('.action-save').addClass('is-disabled');
// TODO: (pfogg) should this text fade in/out on change?
$('#notification-warning-title').text(this.error_title);
$('#notification-warning-description').text(this.error_message);
},
clearValidationErrors : function() {
......@@ -38,6 +47,10 @@ CMS.Views.ValidatingView = Backbone.View.extend({
this.getInputElements(ele).removeClass('error');
$(ele).nextAll('.message-error').remove();
}
$('.wrapper-notification-warning').removeClass('wrapper-notification-warning-w-errors');
$('.action-save').removeClass('is-disabled');
$('#notification-warning-title').text(this.save_title);
$('#notification-warning-description').text(this.save_message);
},
setField : function(event) {
......@@ -83,7 +96,7 @@ CMS.Views.ValidatingView = Backbone.View.extend({
}
var self = this;
this.confirmation = new CMS.Views.Notification.Warning({
title: gettext("You've made some changes"),
title: this.save_title,
message: message,
actions: {
primary: {
......@@ -110,6 +123,8 @@ CMS.Views.ValidatingView = Backbone.View.extend({
}});
this.notificationBarShowing = true;
this.confirmation.show();
// Make sure the bar is in the right state
this.model.isValid();
},
showSavedBar: function(title, message) {
......
cms/static/sass/elements/_system-feedback.scss
......@@ -665,14 +665,8 @@
}
}
// alert showing/hiding
.wrapper-alert {
display: none;
&.is-shown {
display: block;
}
}
// alert showing/hiding done by jQuery
.wrapper-alert { }
// notification showing/hiding
.wrapper-notification {
......
common/static/sass/_mixins-inherited.scss
......@@ -126,7 +126,7 @@
padding: ($baseline/5) $baseline ($baseline/4);
font-weight: 700;
&.disabled {
&.disabled, &.is-disabled {
border: 1px solid $gray-l1 !important;
border-radius: 3px !important;
background: $gray-l1 !important;
......@@ -157,7 +157,7 @@
color: $white;
}
&.disabled {
&.disabled, &.is-disabled {
border: 1px solid $green-l3 !important;
background: $green-l3 !important;
color: $white !important;
......@@ -178,7 +178,7 @@
color: $white;
}
&.disabled {
&.disabled, &.is-disabled {
box-shadow: none;
border: 1px solid $blue-l3 !important;
background: $blue-l3 !important;
......@@ -199,7 +199,7 @@
color: $white;
}
&.disabled {
&.disabled, &.is-disabled {
box-shadow: none;
border: 1px solid $red-l3 !important;
background: $red-l3 !important;
......@@ -220,7 +220,7 @@
color: $white;
}
&.disabled {
&.disabled, &.is-disabled {
box-shadow: none;
border: 1px solid $pink-l3 !important;
background: $pink-l3 !important;
......@@ -242,7 +242,7 @@
color: $gray-d2;
}
&.disabled {
&.disabled, &.is-disabled {
border: 1px solid $orange-l3 !important;
background: $orange-l2 !important;
color: $gray-l1 !important;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment