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..936ce70a9d 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("Action required: Enter 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..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 { @@ -74,6 +73,23 @@ @extend %t-strong; margin: 34px 0 11px; } + + .display-date { + padding-right: 15px; + } + + .message-error { + display: inline-block; + font-weight: normal; + font-size: 1.2em; + + &:before { + content: "\f06a"; + font-family: FontAwesome; + color: #fdbc56; + padding: 5px; + } + } } .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!")