Merge pull request #11968 from edx/efischer/refactor_date_utils

Course Updates date validation

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("Action required: Enter 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

@@ -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 {

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