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.

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
 

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')
 

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;

cms/static/js/utils/date_utils.js

-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
     };
 });

cms/static/js/views/course_info_update.js

-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();
             }

cms/static/js/views/settings/main.js

 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':

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 {

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):

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!")