From 2735fdc2d4c9c6a97faaf7e12eeb45e9a3000647 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Fri, 25 Mar 2016 16:16:48 -0400 Subject: [PATCH 1/2] Course Updates date validation TNL-4115. Previously, course updates (which are intended to be posted with dates, for sorting in the LMS) could be authored in studio with a valid date, nothing, or a random string as the "date" for the update. As there is no validation for this in studio, everything succeeded with no warning. However, the LMS has problems parsing some of these values, and barfs when loaded by learners. The fix does two big things: - gracefully handles invalid dates in LMS. These updates are now treated as having a date of today, for sorting purposes. - turns on validation in studio. Now, it is not only impossible to enter invalid dates in studio, but notifications will draw the course author's eye if any invalid updates were previously saved. Test additions for this commit: Adds: - unit test for LMS parsing - Jasmine test to confirm invalid dates cannot be set by the user -also adds event to setAndValidate instead of using a global object - fix for lettuce test -It is no longer valid to enter the string "January 1, 2013" as this test had been doing. Keyed-in entries must use MM/DD/YY format. --- .../features/course-updates.feature | 2 +- .../coffee/spec/views/course_info_spec.coffee | 15 ++++ cms/static/js/models/course_update.js | 7 ++ cms/static/js/utils/date_utils.js | 75 ++++++++++++++-- cms/static/js/views/course_info_update.js | 89 ++++++++++++++++--- cms/static/js/views/settings/main.js | 57 ++---------- cms/static/sass/views/_updates.scss | 7 ++ common/lib/xmodule/xmodule/html_module.py | 12 ++- .../xmodule/xmodule/tests/test_html_module.py | 39 +++++++- 9 files changed, 231 insertions(+), 72 deletions(-) diff --git a/cms/djangoapps/contentstore/features/course-updates.feature b/cms/djangoapps/contentstore/features/course-updates.feature index e15376ca29..41a9f71fff 100644 --- a/cms/djangoapps/contentstore/features/course-updates.feature +++ b/cms/djangoapps/contentstore/features/course-updates.feature @@ -34,7 +34,7 @@ Feature: CMS.Course updates Given I have opened a new course in Studio And I go to the course updates page And I add a new update with the text "Hello" - When I edit the date to "June 1, 2013" + When I edit the date to "06/01/13" Then I should see the date "June 1, 2013" And I see a "saving" notification diff --git a/cms/static/coffee/spec/views/course_info_spec.coffee b/cms/static/coffee/spec/views/course_info_spec.coffee index 9d2087297e..a71eaa3ddf 100644 --- a/cms/static/coffee/spec/views/course_info_spec.coffee +++ b/cms/static/coffee/spec/views/course_info_spec.coffee @@ -153,6 +153,21 @@ define ["js/views/course_info_handout", "js/views/course_info_update", "js/model it "does not remove existing course info on click outside modal", -> @cancelExistingCourseInfo(false) + @testInvalidDateValue: (value) -> + @courseInfoEdit.onNew(@event) + expect(@courseInfoEdit.$el.find('.save-button').hasClass("is-disabled")).toEqual(false) + @courseInfoEdit.$el.find('input.date').val(value).trigger("change") + expect(@courseInfoEdit.$el.find('.save-button').hasClass("is-disabled")).toEqual(true) + @courseInfoEdit.$el.find('input.date').val("01/01/16").trigger("change") + expect(@courseInfoEdit.$el.find('.save-button').hasClass("is-disabled")).toEqual(false) + + it "does not allow updates to be saved with an invalid date", -> + @testInvalidDateValue("Marchtober 40, 2048") + + it "does not allow updates to be saved with a blank date", -> + @testInvalidDateValue("") + + describe "Course Updates WITH Push notification", -> courseInfoTemplate = readFixtures('course_info_update.underscore') diff --git a/cms/static/js/models/course_update.js b/cms/static/js/models/course_update.js index 137aa5d516..65907fb0fe 100644 --- a/cms/static/js/models/course_update.js +++ b/cms/static/js/models/course_update.js @@ -6,6 +6,13 @@ define(["backbone", "jquery", "jquery.ui"], function(Backbone, $) { "content" : "", "push_notification_enabled": false, "push_notification_selected" : false + }, + validate: function(attrs) { + var date_exists = (attrs.date !== null && attrs.date !== ""); + var date_is_valid_string = ($.datepicker.formatDate('MM d, yy', new Date(attrs.date)) === attrs.date); + if (!(date_exists && date_is_valid_string)) { + return {"date_required": gettext("This field must contain a valid date.")}; + } } }); return CourseUpdate; diff --git a/cms/static/js/utils/date_utils.js b/cms/static/js/utils/date_utils.js index c6ae76d00d..544ea32132 100644 --- a/cms/static/js/utils/date_utils.js +++ b/cms/static/js/utils/date_utils.js @@ -1,15 +1,77 @@ -define(["jquery", "date", "jquery.ui", "jquery.timepicker"], function($, date) { +define(["jquery", "date", "js/utils/change_on_enter", "jquery.ui", "jquery.timepicker"], +function($, date, TriggerChangeEventOnEnter) { + 'use strict'; + var setupDatePicker = function (fieldName, view, index) { + var cacheModel; + var div; + if (typeof index !== "undefined" && view.hasOwnProperty("collection")) { + cacheModel = view.collection.models[index]; + div = view.$el.find('#' + view.collectionSelector(cacheModel.cid)); + } else { + cacheModel = view.model; + div = view.$el.find('#' + view.fieldToSelectorMap[fieldName]); + } + var datefield = $(div).find("input.date"); + var timefield = $(div).find("input.time"); + var cacheview = view; + var setfield = function (event) { + var newVal = getDate(datefield, timefield), + oldTime = new Date(cacheModel.get(fieldName)).getTime(); + if (newVal) { + if (!cacheModel.has(fieldName) || oldTime !== newVal.getTime()) { + cacheview.clearValidationErrors(); + cacheview.setAndValidate(fieldName, newVal, event); + } + } + else { + // Clear date (note that this clears the time as well, as date and time are linked). + // Note also that the validation logic prevents us from clearing the start date + // (start date is required by the back end). + cacheview.clearValidationErrors(); + cacheview.setAndValidate(fieldName, null, event); + } + }; + + // instrument as date and time pickers + timefield.timepicker({'timeFormat' : 'H:i'}); + datefield.datepicker(); + + // Using the change event causes setfield to be triggered twice, but it is necessary + // to pick up when the date is typed directly in the field. + datefield.change(setfield).keyup(TriggerChangeEventOnEnter); + timefield.on('changeTime', setfield); + timefield.on('input', setfield); + + var current_date = null; + if (cacheModel) { + current_date = cacheModel.get(fieldName); + } + // timepicker doesn't let us set null, so check that we have a time + if (current_date) { + setDate(datefield, timefield, current_date); + } // but reset fields either way + else { + timefield.val(''); + datefield.val(''); + } + }; + var getDate = function (datepickerInput, timepickerInput) { // given a pair of inputs (datepicker and timepicker), return a JS Date // object that corresponds to the datetime.js that they represent. Assume // UTC timezone, NOT the timezone of the user's browser. - var date = $(datepickerInput).datepicker("getDate"); - var time = $(timepickerInput).timepicker("getTime"); + var date = $(datepickerInput).datepicker("getDate"), time = null; + if (timepickerInput.length > 0) { + time = $(timepickerInput).timepicker("getTime"); + } if(date && time) { return new Date(Date.UTC( date.getFullYear(), date.getMonth(), date.getDate(), time.getHours(), time.getMinutes() )); + } else if (date) { + return new Date(Date.UTC( + date.getFullYear(), date.getMonth(), date.getDate())); } else { return null; } @@ -21,7 +83,9 @@ define(["jquery", "date", "jquery.ui", "jquery.timepicker"], function($, date) { datetime = date.parse(datetime); if (datetime) { $(datepickerInput).datepicker("setDate", datetime); - $(timepickerInput).timepicker("setTime", datetime); + if (timepickerInput.length > 0) { + $(timepickerInput).timepicker("setTime", datetime); + } } }; @@ -58,6 +122,7 @@ define(["jquery", "date", "jquery.ui", "jquery.timepicker"], function($, date) { setDate: setDate, renderDate: renderDate, convertDateStringsToObjects: convertDateStringsToObjects, - parseDateFromString: parseDateFromString + parseDateFromString: parseDateFromString, + setupDatePicker: setupDatePicker }; }); diff --git a/cms/static/js/views/course_info_update.js b/cms/static/js/views/course_info_update.js index ae6a17f31c..7b5babc771 100644 --- a/cms/static/js/views/course_info_update.js +++ b/cms/static/js/views/course_info_update.js @@ -1,9 +1,11 @@ -define(["js/views/baseview", "codemirror", "js/models/course_update", +define(["js/views/validation", "codemirror", "js/models/course_update", "common/js/components/views/feedback_prompt", "common/js/components/views/feedback_notification", - "js/views/course_info_helper", "js/utils/modal"], - function(BaseView, CodeMirror, CourseUpdateModel, PromptView, NotificationView, CourseInfoHelper, ModalUtils) { + "js/views/course_info_helper", "js/utils/modal", "js/utils/date_utils"], + function(ValidatingView, CodeMirror, CourseUpdateModel, PromptView, NotificationView, + CourseInfoHelper, ModalUtils, DateUtils) { - var CourseInfoUpdateView = BaseView.extend({ + 'use strict'; + var CourseInfoUpdateView = ValidatingView.extend({ // collection is CourseUpdateCollection events: { @@ -19,6 +21,7 @@ define(["js/views/baseview", "codemirror", "js/models/course_update", this.render(); // when the client refetches the updates as a whole, re-render them this.listenTo(this.collection, 'reset', this.render); + this.listenTo(this.collection, 'invalid', this.handleValidationError); }, render: function () { @@ -27,22 +30,68 @@ define(["js/views/baseview", "codemirror", "js/models/course_update", // remove and then add all children $(updateEle).empty(); var self = this; - this.collection.each(function (update) { + this.collection.each(function (update, index) { try { CourseInfoHelper.changeContentToPreview( update, 'content', self.options['base_asset_url']); // push notification is always disabled for existing updates var newEle = self.template({ updateModel : update, push_notification_enabled : false }); $(updateEle).append(newEle); + DateUtils.setupDatePicker("date", self, index); + update.isValid(); } catch (e) { // ignore } }); this.$el.find(".new-update-form").hide(); - this.$el.find('.date').datepicker({ 'dateFormat': 'MM d, yy' }); return this; }, + collectionSelector: function(uid) { + return "course-update-list li[name=" + uid + "]"; + }, + + setAndValidate: function(attr, value, event) { + if (attr === 'date') { + // If the value to be set was typed, validate that entry rather than the current datepicker value + if (this.dateEntry(event).length > 0) { + value = DateUtils.parseDateFromString(this.dateEntry(event).val()); + if (value && isNaN(value.getTime())) { + value = ""; + } + } + value = $.datepicker.formatDate("MM d, yy", value); + } + var targetModel = this.collection.get(this.$currentPost.attr('name')); + var prevValue = targetModel.get(attr); + if (prevValue !== value) { + targetModel.set(attr, value); + this.validateModel(targetModel); + } + }, + + handleValidationError : function(model, error) { + var ele = this.$el.find('#course-update-list li[name=\"'+model.cid+'\"'); + $(ele).find('.message-error').remove(); + for (var field in error) { + if (error.hasOwnProperty(field)) { + $(ele).find('#update-date-'+model.cid).parent().append( + this.errorTemplate({message : error[field]}) + ); + $(ele).find('.date-display').parent().append(this.errorTemplate({message : error[field]})); + } + } + $(ele).find('.save-button').addClass('is-disabled'); + }, + + validateModel: function(model) { + if (model.isValid()) { + var ele = this.$el.find('#course-update-list li[name=\"' + model.cid + '\"'); + $(ele).find('.message-error').remove(); + $(ele).find('.save-button').removeClass('is-disabled'); + } + }, + onNew: function(event) { event.preventDefault(); var self = this; @@ -75,15 +124,15 @@ define(["js/views/baseview", "codemirror", "js/models/course_update", // Binding empty function to prevent default hideModal. }); - $('.date').datepicker('destroy'); - $('.date').datepicker({ 'dateFormat': 'MM d, yy' }); + DateUtils.setupDatePicker("date", this, 0); }, onSave: function(event) { event.preventDefault(); var targetModel = this.eventModel(event); targetModel.set({ - date : this.dateEntry(event).val(), + // translate short-form date (for input) into long form date (for display) + date : $.datepicker.formatDate("MM d, yy", new Date(this.dateEntry(event).val())), content : this.$codeMirror.getValue(), push_notification_selected : this.push_notification_selected(event) }); @@ -112,11 +161,13 @@ define(["js/views/baseview", "codemirror", "js/models/course_update", onCancel: function(event) { event.preventDefault(); - // change editor contents back to model values and hide the editor - $(this.editor(event)).hide(); - // If the model was never created (user created a new update, then pressed Cancel), - // we wish to remove it from the DOM. + // Since we're cancelling, the model should be using it's previous attributes var targetModel = this.eventModel(event); + targetModel.set(targetModel.previousAttributes()); + this.validateModel(targetModel); + // Hide the editor + $(this.editor(event)).hide(); + // targetModel will be lacking an id if it was newly created this.closeEditor(!targetModel.id); }, @@ -129,6 +180,13 @@ define(["js/views/baseview", "codemirror", "js/models/course_update", $(this.editor(event)).show(); var $textArea = this.$currentPost.find(".new-update-content").first(); var targetModel = this.eventModel(event); + // translate long-form date (for viewing) into short-form date (for input) + if (targetModel.get('date') && targetModel.isValid()) { + $(this.dateEntry(event)).val($.datepicker.formatDate("mm/dd/yy", new Date(targetModel.get('date')))); + } + else { + $(this.dateEntry(event)).val("MM/DD/YY"); + } this.$codeMirror = CourseInfoHelper.editWithCodeMirror( targetModel, 'content', self.options['base_asset_url'], $textArea.get(0)); @@ -138,6 +196,9 @@ define(["js/views/baseview", "codemirror", "js/models/course_update", self.closeEditor(false); } ); + + // Ensure validity is marked appropriately + targetModel.isValid(); }, onDelete: function(event) { @@ -189,6 +250,8 @@ define(["js/views/baseview", "codemirror", "js/models/course_update", closeEditor: function(removePost) { var targetModel = this.collection.get(this.$currentPost.attr('name')); + // If the model was never created (user created a new update, then pressed Cancel), + // we wish to remove it from the DOM. if(removePost) { this.$currentPost.remove(); } diff --git a/cms/static/js/views/settings/main.js b/cms/static/js/views/settings/main.js index 82703ac734..74aacc24c5 100644 --- a/cms/static/js/views/settings/main.js +++ b/cms/static/js/views/settings/main.js @@ -1,8 +1,8 @@ define(["js/views/validation", "codemirror", "underscore", "jquery", "jquery.ui", "js/utils/date_utils", "js/models/uploads", - "js/views/uploads", "js/utils/change_on_enter", "js/views/license", "js/models/license", + "js/views/uploads", "js/views/license", "js/models/license", "common/js/components/views/feedback_notification", "jquery.timepicker", "date", "gettext"], function(ValidatingView, CodeMirror, _, $, ui, DateUtils, FileUploadModel, - FileUploadDialog, TriggerChangeEventOnEnter, LicenseView, LicenseModel, NotificationView, + FileUploadDialog, LicenseView, LicenseModel, NotificationView, timepicker, date, gettext) { var DetailsView = ValidatingView.extend({ @@ -63,10 +63,10 @@ var DetailsView = ValidatingView.extend({ }, render: function() { - this.setupDatePicker('start_date'); - this.setupDatePicker('end_date'); - this.setupDatePicker('enrollment_start'); - this.setupDatePicker('enrollment_end'); + DateUtils.setupDatePicker('start_date', this); + DateUtils.setupDatePicker('end_date', this); + DateUtils.setupDatePicker('enrollment_start', this); + DateUtils.setupDatePicker('enrollment_end', this); this.$el.find('#' + this.fieldToSelectorMap['overview']).val(this.model.get('overview')); this.codeMirrorize(null, $('#course-overview')[0]); @@ -147,51 +147,6 @@ var DetailsView = ValidatingView.extend({ }, true)); }, - setupDatePicker: function (fieldName) { - var cacheModel = this.model; - var div = this.$el.find('#' + this.fieldToSelectorMap[fieldName]); - var datefield = $(div).find("input.date"); - var timefield = $(div).find("input.time"); - var cachethis = this; - var setfield = function () { - var newVal = DateUtils.getDate(datefield, timefield), - oldTime = new Date(cacheModel.get(fieldName)).getTime(); - if (newVal) { - if (!cacheModel.has(fieldName) || oldTime !== newVal.getTime()) { - cachethis.clearValidationErrors(); - cachethis.setAndValidate(fieldName, newVal); - } - } - else { - // Clear date (note that this clears the time as well, as date and time are linked). - // Note also that the validation logic prevents us from clearing the start date - // (start date is required by the back end). - cachethis.clearValidationErrors(); - cachethis.setAndValidate(fieldName, null); - } - }; - - // instrument as date and time pickers - timefield.timepicker({'timeFormat' : 'H:i'}); - datefield.datepicker(); - - // Using the change event causes setfield to be triggered twice, but it is necessary - // to pick up when the date is typed directly in the field. - datefield.change(setfield).keyup(TriggerChangeEventOnEnter); - timefield.on('changeTime', setfield); - timefield.on('input', setfield); - - date = this.model.get(fieldName) - // timepicker doesn't let us set null, so check that we have a time - if (date) { - DateUtils.setDate(datefield, timefield, date); - } // but reset fields either way - else { - timefield.val(''); - datefield.val(''); - } - }, - updateModel: function(event) { switch (event.currentTarget.id) { case 'course-language': diff --git a/cms/static/sass/views/_updates.scss b/cms/static/sass/views/_updates.scss index ec3c5c4f75..63512cb21f 100644 --- a/cms/static/sass/views/_updates.scss +++ b/cms/static/sass/views/_updates.scss @@ -74,6 +74,13 @@ @extend %t-strong; margin: 34px 0 11px; } + + .message-error { + @extend %t-copy-sub1; + margin-top: ($baseline/4); + margin-bottom: ($baseline/2); + color: $red; + } } .update-contents { diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 62fce42e3f..83a3ab415e 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -439,7 +439,7 @@ class CourseInfoModule(CourseInfoFields, HtmlModuleMixin): return self.data else: course_updates = [item for item in self.items if item.get('status') == self.STATUS_VISIBLE] - course_updates.sort(key=lambda item: datetime.strptime(item['date'], '%B %d, %Y'), reverse=True) + course_updates.sort(key=lambda item: CourseInfoModule.safe_parse_date(item['date']), reverse=True) context = { 'visible_updates': course_updates[:3], @@ -448,6 +448,16 @@ class CourseInfoModule(CourseInfoFields, HtmlModuleMixin): return self.system.render_template("{0}/course_updates.html".format(self.TEMPLATE_DIR), context) + @staticmethod + def safe_parse_date(date): + """ + Since this is used solely for ordering purposes, use today's date as a default + """ + try: + return datetime.strptime(date, '%B %d, %Y') + except ValueError: # occurs for ill-formatted date values + return datetime.today() + @XBlock.tag("detached") class CourseInfoDescriptor(CourseInfoFields, HtmlDescriptor): diff --git a/common/lib/xmodule/xmodule/tests/test_html_module.py b/common/lib/xmodule/xmodule/tests/test_html_module.py index b755166444..20288c2f5f 100644 --- a/common/lib/xmodule/xmodule/tests/test_html_module.py +++ b/common/lib/xmodule/xmodule/tests/test_html_module.py @@ -3,7 +3,7 @@ import unittest from mock import Mock from xblock.field_data import DictFieldData -from xmodule.html_module import HtmlModule, HtmlDescriptor +from xmodule.html_module import HtmlModule, HtmlDescriptor, CourseInfoModule from . import get_test_system, get_test_descriptor_system from opaque_keys.edx.locations import SlashSeparatedCourseKey @@ -144,3 +144,40 @@ class HtmlDescriptorIndexingTestCase(unittest.TestCase): "content": {"html_content": " This has HTML comment in it. HTML end. ", "display_name": "Text"}, "content_type": "Text" }) + + +class CourseInfoModuleTestCase(unittest.TestCase): + """ + Make sure that CourseInfoModule renders updates properly + """ + def test_updates_render(self): + """ + Tests that a course info module will render its updates, even if they are malformed. + """ + sample_update_data = [ + { + "id": i, + "date": data, + "content": "This is a very important update!", + "status": CourseInfoModule.STATUS_VISIBLE, + } for i, data in enumerate( + [ + 'January 1, 1970', + 'Marchtober 45, -1963', + 'Welcome!', + 'Date means "title", right?' + ] + ) + ] + info_module = CourseInfoModule( + Mock(), + get_test_system(), + DictFieldData({'items': sample_update_data, 'data': ""}), + Mock() + ) + + # Prior to TNL-4115, an exception would be raised when trying to parse invalid dates in this method + try: + info_module.get_html() + except ValueError: + self.fail("CourseInfoModule could not parse an invalid date!") From 29961736db9c4c2877b41c8df1982438ec48f71e Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Wed, 6 Apr 2016 14:28:04 -0400 Subject: [PATCH 2/2] Styling changes for Course Updates errors --- cms/static/js/models/course_update.js | 2 +- cms/static/sass/views/_updates.scss | 19 ++++++++++++++----- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/cms/static/js/models/course_update.js b/cms/static/js/models/course_update.js index 65907fb0fe..936ce70a9d 100644 --- a/cms/static/js/models/course_update.js +++ b/cms/static/js/models/course_update.js @@ -11,7 +11,7 @@ define(["backbone", "jquery", "jquery.ui"], function(Backbone, $) { var date_exists = (attrs.date !== null && attrs.date !== ""); var date_is_valid_string = ($.datepicker.formatDate('MM d, yy', new Date(attrs.date)) === attrs.date); if (!(date_exists && date_is_valid_string)) { - return {"date_required": gettext("This field must contain a valid date.")}; + return {"date_required": gettext("Action required: Enter a valid date.")}; } } }); diff --git a/cms/static/sass/views/_updates.scss b/cms/static/sass/views/_updates.scss index 63512cb21f..8124e4aa20 100644 --- a/cms/static/sass/views/_updates.scss +++ b/cms/static/sass/views/_updates.scss @@ -66,7 +66,6 @@ line-height: 30px; color: #646464; letter-spacing: 1px; - text-transform: uppercase; } h3 { @@ -75,11 +74,21 @@ margin: 34px 0 11px; } + .display-date { + padding-right: 15px; + } + .message-error { - @extend %t-copy-sub1; - margin-top: ($baseline/4); - margin-bottom: ($baseline/2); - color: $red; + display: inline-block; + font-weight: normal; + font-size: 1.2em; + + &:before { + content: "\f06a"; + font-family: FontAwesome; + color: #fdbc56; + padding: 5px; + } } }