From 164a469e9dd38e2801721e527d2d0e28df856365 Mon Sep 17 00:00:00 2001
From: Peter Fogg <pfogg@edx.org>
Date: Mon, 24 Jun 2013 13:07:09 -0400
Subject: [PATCH 1/8] Factor `showNotificationBar` out into ValidatingView.

When all settings require an explicit save, this functionality will be
shared between each view (with slight changes, e.g. click handlers.)
---
 cms/static/js/views/settings/advanced_view.js | 42 ++++---------------
 cms/static/js/views/validating_view.js        | 34 +++++++++++++++
 2 files changed, 42 insertions(+), 34 deletions(-)

diff --git a/cms/static/js/views/settings/advanced_view.js b/cms/static/js/views/settings/advanced_view.js
index 302a918de1..7be45959a6 100644
--- a/cms/static/js/views/settings/advanced_view.js
+++ b/cms/static/js/views/settings/advanced_view.js
@@ -57,8 +57,14 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({
             mode: "application/json", lineNumbers: false, lineWrapping: false,
             onChange: function(instance, changeobj) {
                 // this event's being called even when there's no change :-(
-                if (instance.getValue() !== oldValue && !self.notificationBarShowing) {
-                    self.showNotificationBar();
+                if (instance.getValue() !== oldValue) {
+                    var message = gettext("Your changes will not take effect until you save your progress. Take care with key and value formatting, as validation is not implemented.");
+                    self.showNotificationBar(message,
+                                             _.bind(self.saveView, self),
+                                             _.bind(self.revertView, self));
+                    if(self.saved) {
+                        self.saved.hide();
+                    }
                 }
             },
             onFocus : function(mirror) {
@@ -97,38 +103,6 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({
             }
         });
     },
-    showNotificationBar: function() {
-        var self = this;
-        var message = gettext("Your changes will not take effect until you save your progress. Take care with key and value formatting, as validation is not implemented.")
-        var confirm = new CMS.Views.Notification.Warning({
-            title: gettext("You've Made Some Changes"),
-            message: message,
-            actions: {
-                primary: {
-                    "text": gettext("Save Changes"),
-                    "class": "action-save",
-                    "click": function() {
-                        self.saveView();
-                        confirm.hide();
-                        self.notificationBarShowing = false;
-                    }
-                },
-                secondary: [{
-                    "text": gettext("Cancel"),
-                    "class": "action-cancel",
-                    "click": function() {
-                        self.revertView();
-                        confirm.hide();
-                        self.notificationBarShowing = false;
-                    }
-                }]
-            }});
-        this.notificationBarShowing = true;
-        confirm.show();
-        if(this.saved) {
-            this.saved.hide();
-        }
-    },
     saveView : function() {
         // TODO one last verification scan:
         //    call validateKey on each to ensure proper format
diff --git a/cms/static/js/views/validating_view.js b/cms/static/js/views/validating_view.js
index afb355ad4a..b3f78c2a87 100644
--- a/cms/static/js/views/validating_view.js
+++ b/cms/static/js/views/validating_view.js
@@ -67,5 +67,39 @@ CMS.Views.ValidatingView = Backbone.View.extend({
             // put error on the contained inputs
             return $(ele).find(inputElements);
         }
+    },
+
+    showNotificationBar: function(message, primaryClick, secondaryClick) {
+        if(this.notificationBarShowing) {
+            return;
+        }
+        var self = this;
+        this.confirmation = new CMS.Views.Notification.Warning({
+            title: gettext("You've made some changes"),
+            message: message,
+            actions: {
+                primary: {
+                    "text": gettext("Save Changes"),
+                    "class": "action-save",
+                    "click": function() {
+                        primaryClick();
+                        self.confirmation.hide();
+                        self.notificationBarShowing = false;
+                    }
+                },
+                secondary: [{
+                    "text": gettext("Cancel"),
+                    "class": "action-cancel",
+                    "click": function() {
+                        if(secondaryClick) {
+                            secondaryClick();
+                        }
+                        self.confirmation.hide();
+                        self.notificationBarShowing = false;
+                    }
+                }]
+            }});
+        this.notificationBarShowing = true;
+        this.confirmation.show();
     }
 });

From 9094f1890bd7e4353f2ad284f725b2d623b0737c Mon Sep 17 00:00:00 2001
From: Peter Fogg <pfogg@edx.org>
Date: Tue, 25 Jun 2013 14:38:57 -0400
Subject: [PATCH 2/8] Change course settings to require explicit save.

Rather than asynchronously saving when a setting is updated, we now
prompt the user to confirm their changes and only persist the data if
they hit the save button. Lettuce tests are updated to expect this
behavior and some new ones are added.
---
 CHANGELOG.rst                                 |  2 +
 .../contentstore/features/common.py           |  6 +++
 .../features/course-settings.feature          | 11 +++++
 .../contentstore/features/course-settings.py  | 27 ++++++-----
 .../contentstore/features/grading.feature     | 10 ++++
 .../contentstore/features/grading.py          | 13 +++++
 .../js/models/settings/course_details.js      |  6 +--
 .../js/views/settings/main_settings_view.js   | 47 +++++++++----------
 .../views/settings/settings_grading_view.js   | 31 ++++++++----
 cms/static/js/views/validating_view.js        | 34 ++++++++++----
 10 files changed, 128 insertions(+), 59 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 41c171c52c..4d117a9c73 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -48,6 +48,8 @@ moved to be edited as metadata.
 
 XModule: Only write out assets files if the contents have changed.
 
+Studio: Course settings are now saved explicitly.
+
 XModule: Don't delete generated xmodule asset files when compiling (for
 instance, when XModule provides a coffeescript file, don't delete
 the associated javascript)
diff --git a/cms/djangoapps/contentstore/features/common.py b/cms/djangoapps/contentstore/features/common.py
index bdf07fc5ae..a65ad3a08f 100644
--- a/cms/djangoapps/contentstore/features/common.py
+++ b/cms/djangoapps/contentstore/features/common.py
@@ -55,6 +55,12 @@ def i_have_opened_a_new_course(_step):
     open_new_course()
 
 
+@step('I (save|cancel) my changes$')
+def save_changes(step, action):
+    button_css = '.action-%s' % action
+    world.css_click(button_css)
+
+
 ####### HELPER FUNCTIONS ##############
 def open_new_course():
     world.clear_courses()
diff --git a/cms/djangoapps/contentstore/features/course-settings.feature b/cms/djangoapps/contentstore/features/course-settings.feature
index e869bfe47a..41ee99336d 100644
--- a/cms/djangoapps/contentstore/features/course-settings.feature
+++ b/cms/djangoapps/contentstore/features/course-settings.feature
@@ -5,15 +5,18 @@ Feature: Course Settings
     Given I have opened a new course in Studio
     When I select Schedule and Details
     And I set course dates
+    And I save my changes
     Then I see the set dates on refresh
 
   Scenario: User can clear previously set course dates (except start date)
     Given I have set course dates
     And I clear all the dates except start
+    And I save my changes
     Then I see cleared dates on refresh
 
   Scenario: User cannot clear the course start date
     Given I have set course dates
+    And I save my changes
     And I clear the course start date
     Then I receive a warning about course start date
     And The previously set start date is shown on refresh
@@ -21,5 +24,13 @@ Feature: Course Settings
   Scenario: User can correct the course start date warning
     Given I have tried to clear the course start
     And I have entered a new course start date
+    And I save my changes
     Then The warning about course start date goes away
     And My new course start date is shown on refresh
+
+  Scenario: Settings are only persisted when saved
+    Given I have set course dates
+    And I save my changes
+    When I change fields
+    And I cancel my changes
+    Then I do not see the new changes persisted on refresh
diff --git a/cms/djangoapps/contentstore/features/course-settings.py b/cms/djangoapps/contentstore/features/course-settings.py
index bd86fff9b7..bcf260de23 100644
--- a/cms/djangoapps/contentstore/features/course-settings.py
+++ b/cms/djangoapps/contentstore/features/course-settings.py
@@ -47,8 +47,6 @@ def test_and_i_set_course_dates(step):
     set_date_or_time(COURSE_START_TIME_CSS, DUMMY_TIME)
     set_date_or_time(ENROLLMENT_END_TIME_CSS, DUMMY_TIME)
 
-    pause()
-
 
 @step('Then I see the set dates on refresh$')
 def test_then_i_see_the_set_dates_on_refresh(step):
@@ -71,8 +69,6 @@ def test_and_i_clear_all_the_dates_except_start(step):
     set_date_or_time(ENROLLMENT_START_DATE_CSS, '')
     set_date_or_time(ENROLLMENT_END_DATE_CSS, '')
 
-    pause()
-
 
 @step('Then I see cleared dates on refresh$')
 def test_then_i_see_cleared_dates_on_refresh(step):
@@ -119,7 +115,6 @@ def test_i_have_tried_to_clear_the_course_start(step):
 @step('I have entered a new course start date$')
 def test_i_have_entered_a_new_course_start_date(step):
     set_date_or_time(COURSE_START_DATE_CSS, '12/22/2013')
-    pause()
 
 
 @step('The warning about course start date goes away$')
@@ -137,6 +132,20 @@ def test_my_new_course_start_date_is_shown_on_refresh(step):
     verify_date_or_time(COURSE_START_TIME_CSS, DUMMY_TIME)
 
 
+@step('I change fields$')
+def test_i_change_fields(step):
+    set_date_or_time(COURSE_START_DATE_CSS, '7/7/7777')
+    set_date_or_time(COURSE_END_DATE_CSS, '7/7/7777')
+    set_date_or_time(ENROLLMENT_START_DATE_CSS, '7/7/7777')
+    set_date_or_time(ENROLLMENT_END_DATE_CSS, '7/7/7777')
+
+
+@step('I do not see the new changes persisted on refresh$')
+def test_changes_not_shown_on_refresh(step):
+    reload_the_page(step)
+    step.then('Then I see the set dates on refresh')
+
+
 ############### HELPER METHODS ####################
 def set_date_or_time(css, date_or_time):
     """
@@ -153,11 +162,3 @@ def verify_date_or_time(css, date_or_time):
     Verifies date or time field.
     """
     assert_equal(date_or_time, world.css_find(css).first.value)
-
-
-def pause():
-    """
-    Must sleep briefly to allow last time save to finish,
-    else refresh of browser will fail.
-    """
-    time.sleep(float(1))
diff --git a/cms/djangoapps/contentstore/features/grading.feature b/cms/djangoapps/contentstore/features/grading.feature
index 78634cb964..3736582f39 100644
--- a/cms/djangoapps/contentstore/features/grading.feature
+++ b/cms/djangoapps/contentstore/features/grading.feature
@@ -32,6 +32,7 @@ Feature: Course Grading
         And I have populated the course
         And I am viewing the grading settings
         When I change assignment type "Homework" to "New Type"
+        And I save my changes
         And I go back to the main course page
         Then I do see the assignment name "New Type"
         And I do not see the assignment name "Homework"
@@ -49,5 +50,14 @@ Feature: Course Grading
         And I have populated the course
         And I am viewing the grading settings
         When I add a new assignment type "New Type"
+        And I save my changes
         And I go back to the main course page
         Then I do see the assignment name "New Type"
+
+    Scenario: Settings are only persisted when saved
+        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 "New Type"
+        And I cancel my changes
+        Then I do not see the changes persisted on refresh
diff --git a/cms/djangoapps/contentstore/features/grading.py b/cms/djangoapps/contentstore/features/grading.py
index dc41cda30f..ae672c78a4 100644
--- a/cms/djangoapps/contentstore/features/grading.py
+++ b/cms/djangoapps/contentstore/features/grading.py
@@ -3,6 +3,7 @@
 
 from lettuce import world, step
 from common import *
+from terrain.steps import reload_the_page
 
 
 @step(u'I am viewing the grading settings')
@@ -59,6 +60,8 @@ 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')
@@ -91,6 +94,8 @@ 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')
@@ -99,6 +104,14 @@ def populate_course(step):
     step.given('I have added a new subsection')
 
 
+@step(u'I do not see the changes persisted on refresh$')
+def changes_not_persisted(step):
+    reload_the_page(step)
+    name_id = '#course-grading-assignment-name'
+    ele = world.css_find(name_id)[0]
+    assert(ele.value == 'Homework')
+
+
 def get_type_index(name):
     name_id = '#course-grading-assignment-name'
     f = world.css_find(name_id)
diff --git a/cms/static/js/models/settings/course_details.js b/cms/static/js/models/settings/course_details.js
index b71b4e2ab2..993832f830 100644
--- a/cms/static/js/models/settings/course_details.js
+++ b/cms/static/js/models/settings/course_details.js
@@ -63,13 +63,13 @@ CMS.Models.Settings.CourseDetails = Backbone.Model.extend({
     },
 
     _videokey_illegal_chars : /[^a-zA-Z0-9_-]/g,
-    save_videosource: function(newsource) {
+    set_videosource: function(newsource) {
         // newsource either is <video youtube="speed:key, *"/> or just the "speed:key, *" string
         // returns the videosource for the preview which iss the key whose speed is closest to 1
-        if (_.isEmpty(newsource) && !_.isEmpty(this.get('intro_video'))) this.save({'intro_video': null});
+        if (_.isEmpty(newsource) && !_.isEmpty(this.get('intro_video'))) this.set({'intro_video': null}, {validate: true});
         // TODO remove all whitespace w/in string
         else {
-            if (this.get('intro_video') !== newsource) this.save('intro_video', newsource);
+            if (this.get('intro_video') !== newsource) this.set('intro_video', newsource, {validate: true});
         }
 
         return this.videosourceSample();
diff --git a/cms/static/js/views/settings/main_settings_view.js b/cms/static/js/views/settings/main_settings_view.js
index 2f6239cb59..e15bcfd9eb 100644
--- a/cms/static/js/views/settings/main_settings_view.js
+++ b/cms/static/js/views/settings/main_settings_view.js
@@ -15,6 +15,7 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
         'blur :input' : "inputUnfocus"
 
     },
+
     initialize : function() {
         this.fileAnchorTemplate = _.template('<a href="<%= fullpath %>"> <i class="icon-file"></i><%= filename %></a>');
         // fill in fields
@@ -87,7 +88,7 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
         var datefield = $(div).find("input:.date");
         var timefield = $(div).find("input:.time");
         var cachethis = this;
-        var savefield = function () {
+        var setfield = function () {
             cachethis.clearValidationErrors();
             var date = datefield.datepicker('getDate');
             if (date) {
@@ -97,14 +98,14 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
                 }
                 var newVal = new Date(date.getTime() + time * 1000);
                 if (!cacheModel.has(fieldName) || cacheModel.get(fieldName).getTime() !== newVal.getTime()) {
-                    cacheModel.save(fieldName, newVal);
+                    cacheModel.set(fieldName, newVal, {validate: true});
                 }
             }
             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).
-                cacheModel.save(fieldName, null);
+                cacheModel.set(fieldName, null, {validate: true});
             }
         };
 
@@ -112,10 +113,10 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
         timefield.timepicker({'timeFormat' : 'H:i'});
         datefield.datepicker();
 
-        // Using the change event causes savefield to be triggered twice, but it is necessary
+        // 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(savefield);
-        timefield.on('changeTime', savefield);
+        datefield.change(setfield);
+        timefield.on('changeTime', setfield);
 
         datefield.datepicker('setDate', this.model.get(fieldName));
         if (this.model.has(fieldName)) timefield.timepicker('setTime', this.model.get(fieldName));
@@ -123,22 +124,13 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
 
     updateModel: function(event) {
         switch (event.currentTarget.id) {
-        case 'course-start-date': // handled via onSelect method
-        case 'course-end-date':
-        case 'course-enrollment-start-date':
-        case 'course-enrollment-end-date':
-            break;
-
-        case 'course-overview':
-            // handled via code mirror
-            break;
-
         case 'course-effort':
-            this.saveIfChanged(event);
+            this.setField(event);
             break;
+        // Don't make the user reload the page to check the Youtube ID.
         case 'course-introduction-video':
             this.clearValidationErrors();
-            var previewsource = this.model.save_videosource($(event.currentTarget).val());
+            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();
@@ -147,15 +139,16 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
                 this.$el.find('.remove-course-introduction-video').hide();
             }
             break;
-
-        default:
+        default: // Everything else is handled by datepickers and CodeMirror.
             break;
         }
-
+        var self = this;
+        this.showNotificationBar(this.save_message,
+                                 _.bind(this.saveModel, this));
     },
 
     removeSyllabus: function() {
-        if (this.model.has('syllabus'))	this.model.save({'syllabus': null});
+        if (this.model.has('syllabus'))	this.model.set({'syllabus': null});
     },
 
     assetSyllabus : function() {
@@ -164,7 +157,7 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
 
     removeVideo: function() {
         if (this.model.has('intro_video')) {
-            this.model.save_videosource(null);
+            this.model.set_videosource(null);
             this.$el.find(".current-course-introduction-video iframe").attr("src", "");
             this.$el.find('#' + this.fieldToSelectorMap['intro_video']).val("");
             this.$el.find('.remove-course-introduction-video').hide();
@@ -185,11 +178,15 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
             var field = this.selectorToField[thisTarget.id];
             this.codeMirrors[thisTarget.id] = CodeMirror.fromTextArea(thisTarget, {
                 mode: "text/html", lineNumbers: true, lineWrapping: true,
-                onBlur: function (mirror) {
+                onChange: function (mirror) {
                     mirror.save();
                     cachethis.clearValidationErrors();
                     var newVal = mirror.getValue();
-                    if (cachethis.model.get(field) != newVal) cachethis.model.save(field, newVal);
+                    if (cachethis.model.get(field) != newVal) {
+                        cachethis.model.set(field, newVal);
+                        cachethis.showNotificationBar(cachethis.save_message,
+                                                      _.bind(cachethis.saveModel, cachethis));
+                    }
                 }
             });
         }
diff --git a/cms/static/js/views/settings/settings_grading_view.js b/cms/static/js/views/settings/settings_grading_view.js
index b41c667121..c997c15f6f 100644
--- a/cms/static/js/views/settings/settings_grading_view.js
+++ b/cms/static/js/views/settings/settings_grading_view.js
@@ -88,9 +88,12 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
         'grace_period' : 'course-grading-graceperiod'
     },
     setGracePeriod : function(event) {
-        event.data.clearValidationErrors();
-        var newVal = event.data.model.dateToGracePeriod($(event.currentTarget).timepicker('getTime'));
-        if (event.data.model.get('grace_period') != newVal) event.data.model.save('grace_period', newVal);
+        var self = event.data;
+        self.clearValidationErrors();
+        var newVal = self.model.dateToGracePeriod($(event.currentTarget).timepicker('getTime'));
+        self.model.set('grace_period', newVal, {validate: true});
+        self.showNotificationBar(self.save_message,
+                                 _.bind(self.saveModel, self));
     },
     updateModel : function(event) {
         if (!this.selectorToField[event.currentTarget.id]) return;
@@ -100,9 +103,12 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
             break;
 
         default:
-            this.saveIfChanged(event);
+            this.setField(event);
         break;
         }
+        var self = this;
+        this.showNotificationBar(this.save_message,
+                                 _.bind(self.saveModel, self));
     },
 
     // Grade sliders attributes and methods
@@ -220,13 +226,16 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
     },
 
     saveCutoffs: function() {
-        this.model.save('grade_cutoffs',
+        this.model.set('grade_cutoffs',
                 _.reduce(this.descendingCutoffs,
                         function(object, cutoff) {
                     object[cutoff['designation']] = cutoff['cutoff'] / 100.0;
                     return object;
                 },
-                {}));
+                {}),
+                {validate: true});
+        this.showNotificationBar(this.save_message,
+                                 _.bind(this.saveModel, this));
     },
 
     addNewGrade: function(e) {
@@ -342,11 +351,12 @@ CMS.Views.Settings.GraderView = CMS.Views.ValidatingView.extend({
         switch (event.currentTarget.id) {
         case 'course-grading-assignment-totalassignments':
             this.$el.find('#course-grading-assignment-droppable').attr('max', $(event.currentTarget).val());
-            this.saveIfChanged(event);
+            this.setField(event);
             break;
         case 'course-grading-assignment-name':
             var oldName = this.model.get('type');
-            if (this.saveIfChanged(event) && !_.isEmpty(oldName)) {
+            // If the name has changed, alert the user to change all subsection names.
+            if (this.setField(event) != oldName && !_.isEmpty(oldName)) {
                 // overload the error display logic
                 this._cacheValidationErrors.push(event.currentTarget);
                 $(event.currentTarget).parent().append(
@@ -355,9 +365,12 @@ CMS.Views.Settings.GraderView = CMS.Views.ValidatingView.extend({
             }
             break;
         default:
-            this.saveIfChanged(event);
+            this.setField(event);
         break;
         }
+        var self = this;
+        this.showNotificationBar(this.save_message,
+                                 _.bind(this.saveModel, this));
     },
     deleteModel : function(e) {
         this.model.destroy();
diff --git a/cms/static/js/views/validating_view.js b/cms/static/js/views/validating_view.js
index b3f78c2a87..e9c5678a93 100644
--- a/cms/static/js/views/validating_view.js
+++ b/cms/static/js/views/validating_view.js
@@ -9,6 +9,8 @@ CMS.Views.ValidatingView = Backbone.View.extend({
 
     errorTemplate : _.template('<span class="message-error"><%= message %></span>'),
 
+    save_message: gettext("Your changes will not take effect until you save your progress."),
+
     events : {
         "change input" : "clearValidationErrors",
         "change textarea" : "clearValidationErrors"
@@ -38,17 +40,13 @@ CMS.Views.ValidatingView = Backbone.View.extend({
         }
     },
 
-    saveIfChanged : function(event) {
-        // returns true if the value changed and was thus sent to server
+    setField : function(event) {
+        // Set model field and return the new value.
+        this.clearValidationErrors();
         var field = this.selectorToField[event.currentTarget.id];
-        var currentVal = this.model.get(field);
         var newVal = $(event.currentTarget).val();
-        this.clearValidationErrors(); // curr = new if user reverts manually
-        if (currentVal != newVal) {
-            this.model.save(field, newVal);
-            return true;
-        }
-        else return false;
+        this.model.set(field, newVal, {validate: true});
+        return newVal;
     },
     // these should perhaps go into a superclass but lack of event hash inheritance demotivates me
     inputFocus : function(event) {
@@ -101,5 +99,23 @@ CMS.Views.ValidatingView = Backbone.View.extend({
             }});
         this.notificationBarShowing = true;
         this.confirmation.show();
+    },
+
+    showSavedBar: function(title, message) {
+        var defaultTitle = gettext('Your changes have been saved.');
+        this.saved = new CMS.Views.Alert.Confirmation({
+            title: title || defaultTitle,
+            message: message,
+            closeIcon: false
+        });
+        this.saved.show();
+    },
+
+    saveModel: function() {
+        var self = this;
+        this.model.save({},
+                        {success: function() {
+                            self.showSavedBar();
+                        }});
     }
 });

From eba1bd097230c1f5f58af851505a87dc429a22f5 Mon Sep 17 00:00:00 2001
From: Peter Fogg <pfogg@edx.org>
Date: Wed, 26 Jun 2013 12:49:23 -0400
Subject: [PATCH 3/8] Refactor and update acceptance tests around saving
 settings.

---
 .../features/advanced-settings.feature        |  6 +++
 .../features/advanced-settings.py             | 18 +--------
 .../contentstore/features/common.py           | 40 +++++++++++++++++--
 .../features/course-settings.feature          | 30 +++++++++++---
 .../contentstore/features/course-settings.py  |  1 +
 .../contentstore/features/grading.feature     | 24 +++++++++--
 6 files changed, 90 insertions(+), 29 deletions(-)

diff --git a/cms/djangoapps/contentstore/features/advanced-settings.feature b/cms/djangoapps/contentstore/features/advanced-settings.feature
index 13600f2086..514eb8898e 100644
--- a/cms/djangoapps/contentstore/features/advanced-settings.feature
+++ b/cms/djangoapps/contentstore/features/advanced-settings.feature
@@ -46,3 +46,9 @@ Feature: Advanced (manual) course policy
     Then it is displayed as a string
     And I reload the page
     Then it is displayed as a string
+
+  Scenario: Confirmation is shown on save
+    Given I am on the Advanced Course Settings page in Studio
+    When I edit the value of a policy key
+    And I press the "Save" notification button
+    Then I see a confirmation that my changes have been saved
diff --git a/cms/djangoapps/contentstore/features/advanced-settings.py b/cms/djangoapps/contentstore/features/advanced-settings.py
index 37d5c12ecc..c08216c8e6 100644
--- a/cms/djangoapps/contentstore/features/advanced-settings.py
+++ b/cms/djangoapps/contentstore/features/advanced-settings.py
@@ -2,8 +2,8 @@
 #pylint: disable=W0621
 
 from lettuce import world, step
-from nose.tools import assert_false, assert_equal, assert_regexp_matches
-from common import type_in_codemirror
+from nose.tools import assert_false, assert_equal, assert_regexp_matches, assert_true
+from common import type_in_codemirror, press_the_notification_button
 
 KEY_CSS = '.key input.policy-key'
 VALUE_CSS = 'textarea.json'
@@ -25,20 +25,6 @@ def i_am_on_advanced_course_settings(step):
     step.given('I select the Advanced Settings')
 
 
-@step(u'I press the "([^"]*)" notification button$')
-def press_the_notification_button(step, name):
-    css = 'a.action-%s' % name.lower()
-
-    # Save was clicked if either the save notification bar is gone, or we have a error notification
-    # overlaying it (expected in the case of typing Object into display_name).
-    def save_clicked():
-        confirmation_dismissed = world.is_css_not_present('.is-shown.wrapper-notification-warning')
-        error_showing = world.is_css_present('.is-shown.wrapper-notification-error')
-        return confirmation_dismissed or error_showing
-
-    world.css_click(css, success_condition=save_clicked)
-
-
 @step(u'I edit the value of a policy key$')
 def edit_the_value_of_a_policy_key(step):
     type_in_codemirror(get_index_of(DISPLAY_NAME_KEY), 'X')
diff --git a/cms/djangoapps/contentstore/features/common.py b/cms/djangoapps/contentstore/features/common.py
index a65ad3a08f..d7ac27089e 100644
--- a/cms/djangoapps/contentstore/features/common.py
+++ b/cms/djangoapps/contentstore/features/common.py
@@ -12,6 +12,8 @@ import time
 from logging import getLogger
 logger = getLogger(__name__)
 
+from terrain.browser import reset_data
+
 _COURSE_NAME = 'Robot Super Course'
 _COURSE_NUM = '999'
 _COURSE_ORG = 'MITx'
@@ -54,11 +56,41 @@ def i_press_the_category_delete_icon(_step, category):
 def i_have_opened_a_new_course(_step):
     open_new_course()
 
+@step(u'I press the "([^"]*)" notification button$')
+def press_the_notification_button(step, name):
+    css = 'a.action-%s' % name.lower()
 
-@step('I (save|cancel) my changes$')
-def save_changes(step, action):
-    button_css = '.action-%s' % action
-    world.css_click(button_css)
+    # Save was clicked if either the save notification bar is gone, or we have a error notification
+    # overlaying it (expected in the case of typing Object into display_name).
+    def save_clicked():
+        confirmation_dismissed = world.is_css_not_present('.is-shown.wrapper-notification-warning')
+        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=save_clicked), 'Save button not clicked after 5 attempts.')
+
+
+@step('I change the "(.*)" field to "(.*)"$')
+def i_change_field_to_value(step, field, value):
+    # Special casing this because we should type into CodeMirror
+    field_css = '#%s' % '-'.join([s.lower() for s in field.split()])
+    if field_css == '#course-overview':
+        type_in_codemirror(0, value)
+    else:
+        ele = world.css_find(field_css).first
+        ele.fill(value)
+        ele._element.send_keys(Keys.ENTER)
+
+
+@step('I reset the database')
+def reset_the_db(step):
+    reset_data(None)
+
+
+@step('I see a confirmation that my changes have been saved')
+def i_see_a_confirmation(step):
+    confirmation_css = '#alert-confirmation'
+    assert world.is_css_present(confirmation_css)
 
 
 ####### HELPER FUNCTIONS ##############
diff --git a/cms/djangoapps/contentstore/features/course-settings.feature b/cms/djangoapps/contentstore/features/course-settings.feature
index 41ee99336d..9307cbe719 100644
--- a/cms/djangoapps/contentstore/features/course-settings.feature
+++ b/cms/djangoapps/contentstore/features/course-settings.feature
@@ -5,18 +5,18 @@ Feature: Course Settings
     Given I have opened a new course in Studio
     When I select Schedule and Details
     And I set course dates
-    And I save my changes
+    And I press the "Save" notification button
     Then I see the set dates on refresh
 
   Scenario: User can clear previously set course dates (except start date)
     Given I have set course dates
     And I clear all the dates except start
-    And I save my changes
+    And I press the "Save" notification button
     Then I see cleared dates on refresh
 
   Scenario: User cannot clear the course start date
     Given I have set course dates
-    And I save my changes
+    And I press the "Save" notification button
     And I clear the course start date
     Then I receive a warning about course start date
     And The previously set start date is shown on refresh
@@ -24,13 +24,31 @@ Feature: Course Settings
   Scenario: User can correct the course start date warning
     Given I have tried to clear the course start
     And I have entered a new course start date
-    And I save my changes
+    And I press the "Save" notification button
     Then The warning about course start date goes away
     And My new course start date is shown on refresh
 
   Scenario: Settings are only persisted when saved
     Given I have set course dates
-    And I save my changes
+    And I press the "Save" notification button
     When I change fields
-    And I cancel my changes
+    And I press the "Cancel" notification button
     Then I do not see the new changes persisted on refresh
+
+  Scenario: Confirmation is shown on save
+    Given I have opened a new course in Studio
+    When I select Schedule and Details
+    And I change the "<field>" field to "<value>"
+    And I press the "Save" notification button
+    Then I see a confirmation that my changes have been saved
+    # Lettuce hooks don't get called between each example, so we need
+    # to run the before.each_scenario hook manually to avoid database
+    # errors.
+    And I reset the database
+
+  Examples:
+    | field                     | value             |
+    | Course Start Time         | 11:00             |
+    | Course Overview           | <h1>Overview</h1> |
+    | Course Introduction Video | 4r7wHMg5Yjg       |
+    | Course Effort             | 200:00            |
diff --git a/cms/djangoapps/contentstore/features/course-settings.py b/cms/djangoapps/contentstore/features/course-settings.py
index bcf260de23..85a11ec9ec 100644
--- a/cms/djangoapps/contentstore/features/course-settings.py
+++ b/cms/djangoapps/contentstore/features/course-settings.py
@@ -4,6 +4,7 @@
 from lettuce import world, step
 from terrain.steps import reload_the_page
 from selenium.webdriver.common.keys import Keys
+from common import type_in_codemirror
 import time
 
 from nose.tools import assert_true, assert_false, assert_equal
diff --git a/cms/djangoapps/contentstore/features/grading.feature b/cms/djangoapps/contentstore/features/grading.feature
index 3736582f39..1526c17d9f 100644
--- a/cms/djangoapps/contentstore/features/grading.feature
+++ b/cms/djangoapps/contentstore/features/grading.feature
@@ -32,7 +32,7 @@ Feature: Course Grading
         And I have populated the course
         And I am viewing the grading settings
         When I change assignment type "Homework" to "New Type"
-        And I save my changes
+        And I press the "Save" notification button
         And I go back to the main course page
         Then I do see the assignment name "New Type"
         And I do not see the assignment name "Homework"
@@ -42,6 +42,7 @@ Feature: Course Grading
         And I have populated the course
         And I am viewing the grading settings
         When I delete the assignment type "Homework"
+        And I press the "Save" notification button
         And I go back to the main course page
         Then I do not see the assignment name "Homework"
 
@@ -50,7 +51,7 @@ Feature: Course Grading
         And I have populated the course
         And I am viewing the grading settings
         When I add a new assignment type "New Type"
-        And I save my changes
+        And I press the "Save" notification button
         And I go back to the main course page
         Then I do see the assignment name "New Type"
 
@@ -59,5 +60,22 @@ Feature: Course Grading
         And I have populated the course
         And I am viewing the grading settings
         When I change assignment type "Homework" to "New Type"
-        And I cancel my changes
+        And I press the "Cancel" notification button
         Then I do not see the changes persisted on refresh
+
+    Scenario: Confirmation is shown on save
+        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 the "<field>" field to "<value>"
+        And I press the "Save" notification button
+        Then I see a confirmation that my changes have been saved
+        # Lettuce hooks don't get called between each example, so we need
+        # to run the before.each_scenario hook manually to avoid database
+        # errors.
+        And I reset the database
+
+    Examples:
+        | field                          | value               |
+        | Course Grading Graceperiod     | 1:00                |
+        | Course Grading Assignment Name | New Assignment Name |

From 3d4f9268dafda7df0decd049cb24eea9deec8364 Mon Sep 17 00:00:00 2001
From: Peter Fogg <pfogg@edx.org>
Date: Fri, 28 Jun 2013 10:42:40 -0400
Subject: [PATCH 4/8] Make sure that changing grading cutoffs triggers a save
 confirmation, and reliable reverting of changes.

---
 cms/static/js/views/settings/advanced_view.js | 19 +++--
 .../js/views/settings/main_settings_view.js   | 24 ++++++-
 .../views/settings/settings_grading_view.js   | 71 +++++++++++++------
 cms/static/js/views/validating_view.js        |  7 +-
 4 files changed, 84 insertions(+), 37 deletions(-)

diff --git a/cms/static/js/views/settings/advanced_view.js b/cms/static/js/views/settings/advanced_view.js
index 7be45959a6..65aed2a3c8 100644
--- a/cms/static/js/views/settings/advanced_view.js
+++ b/cms/static/js/views/settings/advanced_view.js
@@ -61,7 +61,10 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({
                     var message = gettext("Your changes will not take effect until you save your progress. Take care with key and value formatting, as validation is not implemented.");
                     self.showNotificationBar(message,
                                              _.bind(self.saveView, self),
-                                             _.bind(self.revertView, self));
+                                             function() {
+                                                 self.model.deleteKeys = [];
+                                                 self.revertView();
+                                             });
                     if(self.saved) {
                         self.saved.hide();
                     }
@@ -112,25 +115,19 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({
             {
             success : function() {
                 self.render();
+                var title = gettext("Your policy changes have been saved.");
                 var message = gettext("Please note that validation of your policy key and value pairs is not currently in place yet. If you are having difficulties, please review your policy pairs.");
-                self.saved = new CMS.Views.Alert.Confirmation({
-                    title: gettext("Your policy changes have been saved."),
-                    message: message,
-                    closeIcon: false
-                });
-                self.saved.show();
+                self.showSavedBar(title, message);
                 analytics.track('Saved Advanced Settings', {
                     'course': course_location_analytics
                 });
             }
         });
     },
-    revertView : function() {
+    revertView: function() {
         var self = this;
-        this.model.deleteKeys = [];
-        this.model.clear({silent : true});
         this.model.fetch({
-            success : function() { self.render(); },
+            success: function() { self.render(); },
             reset: true
         });
     },
diff --git a/cms/static/js/views/settings/main_settings_view.js b/cms/static/js/views/settings/main_settings_view.js
index e15bcfd9eb..27194dba3b 100644
--- a/cms/static/js/views/settings/main_settings_view.js
+++ b/cms/static/js/views/settings/main_settings_view.js
@@ -144,7 +144,8 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
         }
         var self = this;
         this.showNotificationBar(this.save_message,
-                                 _.bind(this.saveModel, this));
+                                 _.bind(this.saveView, this),
+                                 _.bind(this.revertView, this));
     },
 
     removeSyllabus: function() {
@@ -185,12 +186,29 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
                     if (cachethis.model.get(field) != newVal) {
                         cachethis.model.set(field, newVal);
                         cachethis.showNotificationBar(cachethis.save_message,
-                                                      _.bind(cachethis.saveModel, cachethis));
+                                                      _.bind(cachethis.saveView, cachethis),
+                                                      _.bind(cachethis.revertView, cachethis));
                     }
                 }
             });
         }
-    }
+    },
 
+    revertView: function() {
+        // Make sure that the CodeMirror instance has the correct
+        // data from its corresponding textarea
+        var self = this;
+        this.model.fetch({
+            success: function() {
+                self.render();
+                _.each(self.codeMirrors,
+                       function(mirror) {
+                           var ele = mirror.getTextArea();
+                           var field = self.selectorToField[ele.id];
+                           mirror.setValue(self.model.get(field));
+                       });
+            },
+            reset: true});
+    }
 });
 
diff --git a/cms/static/js/views/settings/settings_grading_view.js b/cms/static/js/views/settings/settings_grading_view.js
index c997c15f6f..e5e5c52086 100644
--- a/cms/static/js/views/settings/settings_grading_view.js
+++ b/cms/static/js/views/settings/settings_grading_view.js
@@ -23,14 +23,7 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
                 '<% if (removable) {%><a href="#" class="remove-button">remove</a><% ;} %>' +
         '</li>');
 
-        // Instrument grading scale
-        // convert cutoffs to inversely ordered list
-        var modelCutoffs = this.model.get('grade_cutoffs');
-        for (var cutoff in modelCutoffs) {
-            this.descendingCutoffs.push({designation: cutoff, cutoff: Math.round(modelCutoffs[cutoff] * 100)});
-        }
-        this.descendingCutoffs = _.sortBy(this.descendingCutoffs,
-                function (gradeEle) { return -gradeEle['cutoff']; });
+        this.setupCutoffs();
 
         // Instrument grace period
         this.$el.find('#course-grading-graceperiod').timepicker();
@@ -45,7 +38,6 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
         }
         );
         this.listenTo(this.model, 'invalid', this.handleValidationError);
-        this.model.get('graders').on('remove', this.render, this);
         this.model.get('graders').on('reset', this.render, this);
         this.model.get('graders').on('add', this.render, this);
         this.selectorToField = _.invert(this.fieldToSelectorMap);
@@ -61,11 +53,25 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
         // Undo the double invocation error. At some point, fix the double invocation
         $(gradelist).empty();
         var gradeCollection = this.model.get('graders');
+        // We need to bind the 'remove' event here (rather than in
+        // initialize), or else we can only press the delete button
+        // once due to the graders collection changing when we cancel
+        // our changes.
+        gradeCollection.on('remove', function() {
+            this.showNotificationBar();
+            this.render();
+        }, this);
         gradeCollection.each(function(gradeModel) {
             $(gradelist).append(self.template({model : gradeModel }));
             var newEle = gradelist.children().last();
             var newView = new CMS.Views.Settings.GraderView({el: newEle,
                 model : gradeModel, collection : gradeCollection });
+            // Listen in order to rerender when the 'cancel' button is
+            // pressed
+            self.listenTo(newView, 'revert', _.bind(self.render, self));
+            self.listenTo(gradeModel, 'change', function() {
+                self.showNotificationBar();
+            });
         });
 
         // render the grade cutoffs
@@ -83,6 +89,7 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
     addAssignmentType : function(e) {
         e.preventDefault();
         this.model.get('graders').push({});
+        this.showNotificationBar();
     },
     fieldToSelectorMap : {
         'grace_period' : 'course-grading-graceperiod'
@@ -92,8 +99,7 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
         self.clearValidationErrors();
         var newVal = self.model.dateToGracePeriod($(event.currentTarget).timepicker('getTime'));
         self.model.set('grace_period', newVal, {validate: true});
-        self.showNotificationBar(self.save_message,
-                                 _.bind(self.saveModel, self));
+        self.showNotificationBar();
     },
     updateModel : function(event) {
         if (!this.selectorToField[event.currentTarget.id]) return;
@@ -106,9 +112,7 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
             this.setField(event);
         break;
         }
-        var self = this;
-        this.showNotificationBar(this.save_message,
-                                 _.bind(self.saveModel, self));
+        this.showNotificationBar();
     },
 
     // Grade sliders attributes and methods
@@ -234,8 +238,7 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
                 },
                 {}),
                 {validate: true});
-        this.showNotificationBar(this.save_message,
-                                 _.bind(this.saveModel, this));
+        this.showNotificationBar();
     },
 
     addNewGrade: function(e) {
@@ -310,8 +313,36 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
     },
     setTopGradeLabel: function() {
         this.$el.find('.grades .letter-grade').first().html(this.descendingCutoffs[0]['designation']);
+    },
+    setupCutoffs: function() {
+        // Instrument grading scale
+        // convert cutoffs to inversely ordered list
+        var modelCutoffs = this.model.get('grade_cutoffs');
+        for (var cutoff in modelCutoffs) {
+            this.descendingCutoffs.push({designation: cutoff, cutoff: Math.round(modelCutoffs[cutoff] * 100)});
+        }
+        this.descendingCutoffs = _.sortBy(this.descendingCutoffs,
+                                          function (gradeEle) { return -gradeEle['cutoff']; });
+    },
+    revertView: function() {
+        var self = this;
+        this.model.fetch({
+            success: function() {
+                self.descendingCutoffs = [];
+                self.setupCutoffs();
+                self.render();
+                self.renderCutoffBar();
+            },
+            reset: true});
+    },
+    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.Views.Settings.GraderView = CMS.Views.ValidatingView.extend({
@@ -368,13 +399,9 @@ CMS.Views.Settings.GraderView = CMS.Views.ValidatingView.extend({
             this.setField(event);
         break;
         }
-        var self = this;
-        this.showNotificationBar(this.save_message,
-                                 _.bind(this.saveModel, this));
     },
     deleteModel : function(e) {
-        this.model.destroy();
         e.preventDefault();
+        this.collection.remove(this.model);
     }
-
 });
diff --git a/cms/static/js/views/validating_view.js b/cms/static/js/views/validating_view.js
index e9c5678a93..ede4a0b123 100644
--- a/cms/static/js/views/validating_view.js
+++ b/cms/static/js/views/validating_view.js
@@ -92,6 +92,11 @@ CMS.Views.ValidatingView = Backbone.View.extend({
                         if(secondaryClick) {
                             secondaryClick();
                         }
+                        self.model.clear({silent : true});
+                        /*self.model.fetch({
+                            success : function() { self.render(); },
+                            reset: true
+                        });*/
                         self.confirmation.hide();
                         self.notificationBarShowing = false;
                     }
@@ -111,7 +116,7 @@ CMS.Views.ValidatingView = Backbone.View.extend({
         this.saved.show();
     },
 
-    saveModel: function() {
+    saveView: function() {
         var self = this;
         this.model.save({},
                         {success: function() {

From ec13349ae37adc543152726bfee16a2e83bab34b Mon Sep 17 00:00:00 2001
From: Peter Fogg <pfogg@edx.org>
Date: Tue, 2 Jul 2013 09:05:48 -0400
Subject: [PATCH 5/8] Remove outdated deleteKeys from advanced settings.

---
 cms/static/js/models/settings/advanced.js     | 26 -------------------
 cms/static/js/views/settings/advanced_view.js |  8 +-----
 2 files changed, 1 insertion(+), 33 deletions(-)

diff --git a/cms/static/js/models/settings/advanced.js b/cms/static/js/models/settings/advanced.js
index fee89296a6..95fc64f2a0 100644
--- a/cms/static/js/models/settings/advanced.js
+++ b/cms/static/js/models/settings/advanced.js
@@ -5,8 +5,6 @@ CMS.Models.Settings.Advanced = Backbone.Model.extend({
     defaults: {
         // the properties are whatever the user types in (in addition to whatever comes originally from the server)
     },
-    // which keys to send as the deleted keys on next save
-    deleteKeys : [],
 
     validate: function (attrs) {
         // Keys can no longer be edited. We are currently not validating values.
@@ -18,32 +16,8 @@ CMS.Models.Settings.Advanced = Backbone.Model.extend({
         // add saveSuccess to the success
         var success = options.success;
         options.success = function(model, resp, options) {
-          model.afterSave(model);
           if (success) success(model, resp, options);
         };
         Backbone.Model.prototype.save.call(this, attrs, options);
-    },
-
-    afterSave : function(self) {
-        // remove deleted attrs
-        if (!_.isEmpty(self.deleteKeys)) {
-            // remove the to be deleted keys from the returned model
-            _.each(self.deleteKeys, function(key) { self.unset(key); });
-            // not able to do via backbone since we're not destroying the model
-            $.ajax({
-                url : self.url,
-                // json to and fro
-                contentType : "application/json",
-                dataType : "json",
-                // delete
-                type : 'DELETE',
-                // data
-                data : JSON.stringify({ deleteKeys : self.deleteKeys})
-            })
-            .done(function(data, status, error) {
-                // clear deleteKeys on success
-                self.deleteKeys = [];
-            });
-        }
     }
 });
diff --git a/cms/static/js/views/settings/advanced_view.js b/cms/static/js/views/settings/advanced_view.js
index 65aed2a3c8..961d9d010b 100644
--- a/cms/static/js/views/settings/advanced_view.js
+++ b/cms/static/js/views/settings/advanced_view.js
@@ -61,13 +61,7 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({
                     var message = gettext("Your changes will not take effect until you save your progress. Take care with key and value formatting, as validation is not implemented.");
                     self.showNotificationBar(message,
                                              _.bind(self.saveView, self),
-                                             function() {
-                                                 self.model.deleteKeys = [];
-                                                 self.revertView();
-                                             });
-                    if(self.saved) {
-                        self.saved.hide();
-                    }
+                                             _.bind(self.revertView, self));
                 }
             },
             onFocus : function(mirror) {

From d9716a72494a6a3933bc9202a429c99371b4cb60 Mon Sep 17 00:00:00 2001
From: Peter Fogg <pfogg@edx.org>
Date: Mon, 1 Jul 2013 12:32:37 -0400
Subject: [PATCH 6/8] Update acceptance tests for course settings.

---
 .../contentstore/features/common.py           | 34 +++++++++-------
 .../features/course-settings.feature          | 17 +++++++-
 .../contentstore/features/course-settings.py  | 40 +++++++++++++------
 .../contentstore/features/grading.feature     | 20 +++++-----
 .../contentstore/features/grading.py          |  8 ++++
 5 files changed, 80 insertions(+), 39 deletions(-)

diff --git a/cms/djangoapps/contentstore/features/common.py b/cms/djangoapps/contentstore/features/common.py
index d7ac27089e..37daebabd9 100644
--- a/cms/djangoapps/contentstore/features/common.py
+++ b/cms/djangoapps/contentstore/features/common.py
@@ -56,34 +56,40 @@ def i_press_the_category_delete_icon(_step, category):
 def i_have_opened_a_new_course(_step):
     open_new_course()
 
+
 @step(u'I press the "([^"]*)" notification button$')
-def press_the_notification_button(step, name):
+def press_the_notification_button(_step, name):
     css = 'a.action-%s' % name.lower()
 
-    # Save was clicked if either the save notification bar is gone, or we have a error notification
-    # overlaying it (expected in the case of typing Object into display_name).
-    def save_clicked():
+    # The button was clicked if either the notification bar is gone,
+    # or we see an error overlaying it (expected for invalid inputs).
+    def button_clicked():
         confirmation_dismissed = world.is_css_not_present('.is-shown.wrapper-notification-warning')
         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=save_clicked), 'Save button not clicked after 5 attempts.')
+    assert_true(world.css_click(css, success_condition=button_clicked), '%s button not clicked after 5 attempts.' % name)
 
 
 @step('I change the "(.*)" field to "(.*)"$')
-def i_change_field_to_value(step, field, value):
-    # Special casing this because we should type into CodeMirror
+def i_change_field_to_value(_step, field, value):
     field_css = '#%s' % '-'.join([s.lower() for s in field.split()])
-    if field_css == '#course-overview':
-        type_in_codemirror(0, value)
-    else:
-        ele = world.css_find(field_css).first
-        ele.fill(value)
-        ele._element.send_keys(Keys.ENTER)
+    ele = world.css_find(field_css).first
+    ele.fill(value)
+    ele._element.send_keys(Keys.ENTER)
 
 
 @step('I reset the database')
-def reset_the_db(step):
+def reset_the_db(_step):
+    """
+    When running Lettuce tests using examples (i.e. "Confirmation is
+    shown on save" in course-settings.feature), the normal hooks
+    aren't called between examples. reset_data should run before each
+    scenario to flush the test database. When this doesn't happen we
+    get errors due to trying to insert a non-unique entry. So instead,
+    we delete the database manually. This has the effect of removing
+    any users and courses that have been created during the test run.
+    """
     reset_data(None)
 
 
diff --git a/cms/djangoapps/contentstore/features/course-settings.feature b/cms/djangoapps/contentstore/features/course-settings.feature
index 9307cbe719..cafedd6d97 100644
--- a/cms/djangoapps/contentstore/features/course-settings.feature
+++ b/cms/djangoapps/contentstore/features/course-settings.feature
@@ -32,9 +32,15 @@ Feature: Course Settings
     Given I have set course dates
     And I press the "Save" notification button
     When I change fields
-    And I press the "Cancel" notification button
     Then I do not see the new changes persisted on refresh
 
+  Scenario: Settings are reset on cancel
+    Given I have set course dates
+    And I press the "Save" notification button
+    When I change fields
+    And I press the "Cancel" notification button
+    Then I do not see the changes
+
   Scenario: Confirmation is shown on save
     Given I have opened a new course in Studio
     When I select Schedule and Details
@@ -49,6 +55,13 @@ Feature: Course Settings
   Examples:
     | field                     | value             |
     | Course Start Time         | 11:00             |
-    | Course Overview           | <h1>Overview</h1> |
     | Course Introduction Video | 4r7wHMg5Yjg       |
     | Course Effort             | 200:00            |
+
+  # Special case because we have to type in code mirror
+  Scenario: Changes in Course Overview show a confirmation
+    Given I have opened a new course in Studio
+    When I select Schedule and Details
+    And I change the course overview
+    And I press the "Save" notification button
+    Then I see a confirmation that my changes have been saved
diff --git a/cms/djangoapps/contentstore/features/course-settings.py b/cms/djangoapps/contentstore/features/course-settings.py
index 85a11ec9ec..53a3fa7870 100644
--- a/cms/djangoapps/contentstore/features/course-settings.py
+++ b/cms/djangoapps/contentstore/features/course-settings.py
@@ -5,7 +5,6 @@ from lettuce import world, step
 from terrain.steps import reload_the_page
 from selenium.webdriver.common.keys import Keys
 from common import type_in_codemirror
-import time
 
 from nose.tools import assert_true, assert_false, assert_equal
 
@@ -52,16 +51,7 @@ def test_and_i_set_course_dates(step):
 @step('Then I see the set dates on refresh$')
 def test_then_i_see_the_set_dates_on_refresh(step):
     reload_the_page(step)
-    verify_date_or_time(COURSE_START_DATE_CSS, '12/20/2013')
-    verify_date_or_time(COURSE_END_DATE_CSS, '12/26/2013')
-    verify_date_or_time(ENROLLMENT_START_DATE_CSS, '12/01/2013')
-    verify_date_or_time(ENROLLMENT_END_DATE_CSS, '12/10/2013')
-
-    verify_date_or_time(COURSE_START_TIME_CSS, DUMMY_TIME)
-    # Unset times get set to 12 AM once the corresponding date has been set.
-    verify_date_or_time(COURSE_END_TIME_CSS, DEFAULT_TIME)
-    verify_date_or_time(ENROLLMENT_START_TIME_CSS, DEFAULT_TIME)
-    verify_date_or_time(ENROLLMENT_END_TIME_CSS, DUMMY_TIME)
+    i_see_the_set_dates()
 
 
 @step('And I clear all the dates except start$')
@@ -143,10 +133,20 @@ def test_i_change_fields(step):
 
 @step('I do not see the new changes persisted on refresh$')
 def test_changes_not_shown_on_refresh(step):
-    reload_the_page(step)
     step.then('Then I see the set dates on refresh')
 
 
+@step('I do not see the changes')
+def test_i_do_not_see_changes(_step):
+    i_see_the_set_dates()
+
+
+@step('I change the course overview')
+def test_change_course_overview(_step):
+    type_in_codemirror(0, "<h1>Overview</h1>")
+
+
+
 ############### HELPER METHODS ####################
 def set_date_or_time(css, date_or_time):
     """
@@ -163,3 +163,19 @@ def verify_date_or_time(css, date_or_time):
     Verifies date or time field.
     """
     assert_equal(date_or_time, world.css_find(css).first.value)
+
+
+def i_see_the_set_dates():
+    """
+    Ensure that each field has the value set in `test_and_i_set_course_dates`.
+    """
+    verify_date_or_time(COURSE_START_DATE_CSS, '12/20/2013')
+    verify_date_or_time(COURSE_END_DATE_CSS, '12/26/2013')
+    verify_date_or_time(ENROLLMENT_START_DATE_CSS, '12/01/2013')
+    verify_date_or_time(ENROLLMENT_END_DATE_CSS, '12/10/2013')
+
+    verify_date_or_time(COURSE_START_TIME_CSS, DUMMY_TIME)
+    # Unset times get set to 12 AM once the corresponding date has been set.
+    verify_date_or_time(COURSE_END_TIME_CSS, DEFAULT_TIME)
+    verify_date_or_time(ENROLLMENT_START_TIME_CSS, DEFAULT_TIME)
+    verify_date_or_time(ENROLLMENT_END_TIME_CSS, DUMMY_TIME)
diff --git a/cms/djangoapps/contentstore/features/grading.feature b/cms/djangoapps/contentstore/features/grading.feature
index 1526c17d9f..303cf5db9d 100644
--- a/cms/djangoapps/contentstore/features/grading.feature
+++ b/cms/djangoapps/contentstore/features/grading.feature
@@ -60,22 +60,20 @@ Feature: Course Grading
         And I have populated the course
         And I am viewing the grading settings
         When I change assignment type "Homework" to "New Type"
-        And I press the "Cancel" notification button
         Then I do not see the changes persisted on refresh
 
+    Scenario: Settings are reset on cancel
+        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 "New Type"
+        And I press the "Cancel" notification button
+        Then I see the assignment type "Homework"
+
     Scenario: Confirmation is shown on save
         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 the "<field>" field to "<value>"
+        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
-        # Lettuce hooks don't get called between each example, so we need
-        # to run the before.each_scenario hook manually to avoid database
-        # errors.
-        And I reset the database
-
-    Examples:
-        | field                          | value               |
-        | Course Grading Graceperiod     | 1:00                |
-        | Course Grading Assignment Name | New Assignment Name |
diff --git a/cms/djangoapps/contentstore/features/grading.py b/cms/djangoapps/contentstore/features/grading.py
index ae672c78a4..504dcf7ef3 100644
--- a/cms/djangoapps/contentstore/features/grading.py
+++ b/cms/djangoapps/contentstore/features/grading.py
@@ -112,6 +112,14 @@ def changes_not_persisted(step):
     assert(ele.value == 'Homework')
 
 
+@step(u'I see the assignment type "(.*)"$')
+def i_see_the_assignment_type(_step, name):
+      assignment_css = '#course-grading-assignment-name'
+      assignments = world.css_find(assignment_css)
+      types = [ele['value'] for ele in assignments]
+      assert name in types
+
+
 def get_type_index(name):
     name_id = '#course-grading-assignment-name'
     f = world.css_find(name_id)

From c2f0b7d4e9d06cff8a748282abdcf867d57c2f91 Mon Sep 17 00:00:00 2001
From: Peter Fogg <pfogg@edx.org>
Date: Tue, 2 Jul 2013 11:46:26 -0400
Subject: [PATCH 7/8] Fix validation in many cases, and ensure that the
 notification bar pops up correctly.

---
 .../models/settings/course_grading_policy.js  | 10 +++---
 cms/static/js/views/settings/advanced_view.js |  7 ++--
 .../js/views/settings/main_settings_view.js   | 22 ++++++++----
 .../views/settings/settings_grading_view.js   | 25 +++++++-------
 cms/static/js/views/validating_view.js        | 34 ++++++++++++++-----
 5 files changed, 62 insertions(+), 36 deletions(-)

diff --git a/cms/static/js/models/settings/course_grading_policy.js b/cms/static/js/models/settings/course_grading_policy.js
index 8162145003..c4cc326e92 100644
--- a/cms/static/js/models/settings/course_grading_policy.js
+++ b/cms/static/js/models/settings/course_grading_policy.js
@@ -71,7 +71,7 @@ CMS.Models.Settings.CourseGrader = Backbone.Model.extend({
     },
     validate : function(attrs) {
         var errors = {};
-        if (attrs['type']) {
+        if (_.has(attrs, 'type')) {
             if (_.isEmpty(attrs['type'])) {
                 errors.type = "The assignment type must have a name.";
             }
@@ -83,7 +83,7 @@ CMS.Models.Settings.CourseGrader = Backbone.Model.extend({
                 }
             }
         }
-        if (attrs['weight']) {
+        if (_.has(attrs, 'weight')) {
             if (!isFinite(attrs.weight) || /\D+/.test(attrs.weight)) {
                 errors.weight = "Please enter an integer between 0 and 100.";
             }
@@ -97,19 +97,19 @@ CMS.Models.Settings.CourseGrader = Backbone.Model.extend({
 //                  errors.weight = "The weights cannot add to more than 100.";
                 }
             }}
-        if (attrs['min_count']) {
+        if (_.has(attrs, 'min_count')) {
             if (!isFinite(attrs.min_count) || /\D+/.test(attrs.min_count)) {
                 errors.min_count = "Please enter an integer.";
             }
             else attrs.min_count = parseInt(attrs.min_count);
         }
-        if (attrs['drop_count']) {
+        if (_.has(attrs, 'drop_count')) {
             if (!isFinite(attrs.drop_count) || /\D+/.test(attrs.drop_count)) {
                 errors.drop_count = "Please enter an integer.";
             }
             else attrs.drop_count = parseInt(attrs.drop_count);
         }
-        if (attrs['min_count'] && attrs['drop_count'] && attrs.drop_count > attrs.min_count) {
+        if (_.has(attrs, 'min_count') && _.has(attrs, 'drop_count') && attrs.drop_count > attrs.min_count) {
             errors.drop_count = "Cannot drop more " + attrs.type + " than will assigned.";
         }
         if (!_.isEmpty(errors)) return errors;
diff --git a/cms/static/js/views/settings/advanced_view.js b/cms/static/js/views/settings/advanced_view.js
index 961d9d010b..90e84adf2b 100644
--- a/cms/static/js/views/settings/advanced_view.js
+++ b/cms/static/js/views/settings/advanced_view.js
@@ -56,6 +56,7 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({
         CodeMirror.fromTextArea(textarea, {
             mode: "application/json", lineNumbers: false, lineWrapping: false,
             onChange: function(instance, changeobj) {
+                instance.save()
                 // this event's being called even when there's no change :-(
                 if (instance.getValue() !== oldValue) {
                     var message = gettext("Your changes will not take effect until you save your progress. Take care with key and value formatting, as validation is not implemented.");
@@ -94,8 +95,7 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({
                     }
                 }
                 if (JSONValue !== undefined) {
-                    self.clearValidationErrors();
-                    self.model.set(key, JSONValue, {validate: true});
+                    self.model.set(key, JSONValue);
                 }
             }
         });
@@ -115,7 +115,8 @@ CMS.Views.Settings.Advanced = CMS.Views.ValidatingView.extend({
                 analytics.track('Saved Advanced Settings', {
                     'course': course_location_analytics
                 });
-            }
+            },
+            silent: true
         });
     },
     revertView: function() {
diff --git a/cms/static/js/views/settings/main_settings_view.js b/cms/static/js/views/settings/main_settings_view.js
index 27194dba3b..c378ba2eaf 100644
--- a/cms/static/js/views/settings/main_settings_view.js
+++ b/cms/static/js/views/settings/main_settings_view.js
@@ -89,7 +89,6 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
         var timefield = $(div).find("input:.time");
         var cachethis = this;
         var setfield = function () {
-            cachethis.clearValidationErrors();
             var date = datefield.datepicker('getDate');
             if (date) {
                 var time = timefield.timepicker("getSecondsFromMidnight");
@@ -98,14 +97,16 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
                 }
                 var newVal = new Date(date.getTime() + time * 1000);
                 if (!cacheModel.has(fieldName) || cacheModel.get(fieldName).getTime() !== newVal.getTime()) {
-                    cacheModel.set(fieldName, newVal, {validate: true});
+                    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).
-                cacheModel.set(fieldName, null, {validate: true});
+                cachethis.clearValidationErrors();
+                cachethis.setAndValidate(fieldName, null);
             }
         };
 
@@ -142,14 +143,13 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
         default: // Everything else is handled by datepickers and CodeMirror.
             break;
         }
-        var self = this;
         this.showNotificationBar(this.save_message,
                                  _.bind(this.saveView, this),
                                  _.bind(this.revertView, this));
     },
 
     removeSyllabus: function() {
-        if (this.model.has('syllabus'))	this.model.set({'syllabus': null});
+        if (this.model.has('syllabus'))	this.setAndValidate('syllabus', null);
     },
 
     assetSyllabus : function() {
@@ -184,7 +184,7 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
                     cachethis.clearValidationErrors();
                     var newVal = mirror.getValue();
                     if (cachethis.model.get(field) != newVal) {
-                        cachethis.model.set(field, newVal);
+                        cachethis.setAndValidate(field, newVal);
                         cachethis.showNotificationBar(cachethis.save_message,
                                                       _.bind(cachethis.saveView, cachethis),
                                                       _.bind(cachethis.revertView, cachethis));
@@ -209,6 +209,16 @@ CMS.Views.Settings.Details = CMS.Views.ValidatingView.extend({
                        });
             },
             reset: true});
+    },
+    setAndValidate: function(attr, value) {
+        // If we call model.set() with {validate: true}, model fields
+        // will not be set if validation fails. This puts the UI and
+        // the model in an inconsistent state, and causes us to not
+        // see the right validation errors the next time validate() is
+        // called on the model. So we set *without* validating, then
+        // call validate ourselves.
+        this.model.set(attr, value);
+        this.model.isValid();
     }
 });
 
diff --git a/cms/static/js/views/settings/settings_grading_view.js b/cms/static/js/views/settings/settings_grading_view.js
index e5e5c52086..7329140fa6 100644
--- a/cms/static/js/views/settings/settings_grading_view.js
+++ b/cms/static/js/views/settings/settings_grading_view.js
@@ -38,6 +38,7 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
         }
         );
         this.listenTo(this.model, 'invalid', this.handleValidationError);
+        this.listenTo(this.model, 'change', this.showNotificationBar);
         this.model.get('graders').on('reset', this.render, this);
         this.model.get('graders').on('add', this.render, this);
         this.selectorToField = _.invert(this.fieldToSelectorMap);
@@ -53,14 +54,18 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
         // Undo the double invocation error. At some point, fix the double invocation
         $(gradelist).empty();
         var gradeCollection = this.model.get('graders');
-        // We need to bind the 'remove' event here (rather than in
+        // We need to bind these events here (rather than in
         // initialize), or else we can only press the delete button
         // once due to the graders collection changing when we cancel
         // our changes.
-        gradeCollection.on('remove', function() {
-            this.showNotificationBar();
-            this.render();
-        }, this);
+        _.each(['change', 'remove', 'add'],
+               function (event) {
+                   gradeCollection.on(event, function() {
+                       this.showNotificationBar();
+                       this.render();
+                   }, this);
+               },
+               this);
         gradeCollection.each(function(gradeModel) {
             $(gradelist).append(self.template({model : gradeModel }));
             var newEle = gradelist.children().last();
@@ -69,9 +74,6 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
             // Listen in order to rerender when the 'cancel' button is
             // pressed
             self.listenTo(newView, 'revert', _.bind(self.render, self));
-            self.listenTo(gradeModel, 'change', function() {
-                self.showNotificationBar();
-            });
         });
 
         // render the grade cutoffs
@@ -89,7 +91,6 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
     addAssignmentType : function(e) {
         e.preventDefault();
         this.model.get('graders').push({});
-        this.showNotificationBar();
     },
     fieldToSelectorMap : {
         'grace_period' : 'course-grading-graceperiod'
@@ -99,7 +100,6 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
         self.clearValidationErrors();
         var newVal = self.model.dateToGracePeriod($(event.currentTarget).timepicker('getTime'));
         self.model.set('grace_period', newVal, {validate: true});
-        self.showNotificationBar();
     },
     updateModel : function(event) {
         if (!this.selectorToField[event.currentTarget.id]) return;
@@ -112,7 +112,6 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
             this.setField(event);
         break;
         }
-        this.showNotificationBar();
     },
 
     // Grade sliders attributes and methods
@@ -238,7 +237,6 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
                 },
                 {}),
                 {validate: true});
-        this.showNotificationBar();
     },
 
     addNewGrade: function(e) {
@@ -333,7 +331,8 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
                 self.render();
                 self.renderCutoffBar();
             },
-            reset: true});
+            reset: true,
+            silent: true});
     },
     showNotificationBar: function() {
         // We always call showNotificationBar with the same args, just
diff --git a/cms/static/js/views/validating_view.js b/cms/static/js/views/validating_view.js
index ede4a0b123..bd47d07808 100644
--- a/cms/static/js/views/validating_view.js
+++ b/cms/static/js/views/validating_view.js
@@ -45,7 +45,8 @@ CMS.Views.ValidatingView = Backbone.View.extend({
         this.clearValidationErrors();
         var field = this.selectorToField[event.currentTarget.id];
         var newVal = $(event.currentTarget).val();
-        this.model.set(field, newVal, {validate: true});
+        this.model.set(field, newVal);
+        this.model.isValid();
         return newVal;
     },
     // these should perhaps go into a superclass but lack of event hash inheritance demotivates me
@@ -68,9 +69,18 @@ CMS.Views.ValidatingView = Backbone.View.extend({
     },
 
     showNotificationBar: function(message, primaryClick, secondaryClick) {
+        // Show a notification with message. primaryClick is called on
+        // pressing the save button, and secondaryClick (if it's
+        // passed, which it may not be) will be called on
+        // cancel. Takes care of hiding the notification bar at the
+        // appropriate times.
         if(this.notificationBarShowing) {
             return;
         }
+        // If we've already saved something, hide the alert.
+        if(this.saved) {
+            this.saved.hide();
+        }
         var self = this;
         this.confirmation = new CMS.Views.Notification.Warning({
             title: gettext("You've made some changes"),
@@ -93,10 +103,6 @@ CMS.Views.ValidatingView = Backbone.View.extend({
                             secondaryClick();
                         }
                         self.model.clear({silent : true});
-                        /*self.model.fetch({
-                            success : function() { self.render(); },
-                            reset: true
-                        });*/
                         self.confirmation.hide();
                         self.notificationBarShowing = false;
                     }
@@ -114,13 +120,23 @@ CMS.Views.ValidatingView = Backbone.View.extend({
             closeIcon: false
         });
         this.saved.show();
+        $.smoothScroll({
+            offset: 0,
+            easing: 'swing',
+            speed: 1000
+        });
     },
 
     saveView: function() {
         var self = this;
-        this.model.save({},
-                        {success: function() {
-                            self.showSavedBar();
-                        }});
+        this.model.save(
+            {},
+            {
+                success: function() {
+                    self.showSavedBar();
+                },
+                silent: true
+            }
+        );
     }
 });

From 6e949604885c0a2e6a9f8ec7f22e62ef16d1b680 Mon Sep 17 00:00:00 2001
From: Peter Fogg <pfogg@edx.org>
Date: Tue, 2 Jul 2013 16:50:06 -0400
Subject: [PATCH 8/8] Fix many validation bugs and failing tests.

---
 .../contentstore/features/common.py           |  9 ++-
 .../features/course-settings.feature          |  6 ++
 .../contentstore/features/grading.feature     |  7 ++
 .../contentstore/features/grading.py          |  4 -
 .../coffee/spec/views/feedback_spec.coffee    |  6 +-
 .../models/settings/course_grading_policy.js  |  7 +-
 cms/static/js/views/feedback.js               | 16 +++-
 .../js/views/settings/main_settings_view.js   | 76 +++++++++----------
 .../views/settings/settings_grading_view.js   | 24 ++++--
 cms/static/js/views/validating_view.js        | 17 ++++-
 .../sass/elements/_system-feedback.scss       | 10 +--
 common/static/sass/_mixins-inherited.scss     | 12 +--
 12 files changed, 123 insertions(+), 71 deletions(-)

diff --git a/cms/djangoapps/contentstore/features/common.py b/cms/djangoapps/contentstore/features/common.py
index 37daebabd9..5a8e02f168 100644
--- a/cms/djangoapps/contentstore/features/common.py
+++ b/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")
diff --git a/cms/djangoapps/contentstore/features/course-settings.feature b/cms/djangoapps/contentstore/features/course-settings.feature
index cafedd6d97..5c79dc7ee3 100644
--- a/cms/djangoapps/contentstore/features/course-settings.feature
+++ b/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
diff --git a/cms/djangoapps/contentstore/features/grading.feature b/cms/djangoapps/contentstore/features/grading.feature
index 303cf5db9d..b01d762d73 100644
--- a/cms/djangoapps/contentstore/features/grading.feature
+++ b/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
diff --git a/cms/djangoapps/contentstore/features/grading.py b/cms/djangoapps/contentstore/features/grading.py
index 504dcf7ef3..e75d8f23ad 100644
--- a/cms/djangoapps/contentstore/features/grading.py
+++ b/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')
diff --git a/cms/static/coffee/spec/views/feedback_spec.coffee b/cms/static/coffee/spec/views/feedback_spec.coffee
index 059dd48ef7..1925f890e0 100644
--- a/cms/static/coffee/spec/views/feedback_spec.coffee
+++ b/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", ->
diff --git a/cms/static/js/models/settings/course_grading_policy.js b/cms/static/js/models/settings/course_grading_policy.js
index c4cc326e92..3014b39e82 100644
--- a/cms/static/js/models/settings/course_grading_policy.js
+++ b/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
diff --git a/cms/static/js/views/feedback.js b/cms/static/js/views/feedback.js
index 5b884ad88d..4595a6cab4 100644
--- a/cms/static/js/views/feedback.js
+++ b/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, {
diff --git a/cms/static/js/views/settings/main_settings_view.js b/cms/static/js/views/settings/main_settings_view.js
index c378ba2eaf..5550c550c0 100644
--- a/cms/static/js/views/settings/main_settings_view.js
+++ b/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));
     }
 });
 
diff --git a/cms/static/js/views/settings/settings_grading_view.js b/cms/static/js/views/settings/settings_grading_view.js
index 7329140fa6..f05261d67f 100644
--- a/cms/static/js/views/settings/settings_grading_view.js
+++ b/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;
diff --git a/cms/static/js/views/validating_view.js b/cms/static/js/views/validating_view.js
index bd47d07808..09b723e617 100644
--- a/cms/static/js/views/validating_view.js
+++ b/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) {
diff --git a/cms/static/sass/elements/_system-feedback.scss b/cms/static/sass/elements/_system-feedback.scss
index 68e7ed6aeb..29c77bbc5e 100644
--- a/cms/static/sass/elements/_system-feedback.scss
+++ b/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 {
diff --git a/common/static/sass/_mixins-inherited.scss b/common/static/sass/_mixins-inherited.scss
index 82813153fa..5a834fa256 100644
--- a/common/static/sass/_mixins-inherited.scss
+++ b/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;