Merge pull request #301 from edx/peter-fogg/explicit-course-settings

Peter fogg/explicit course settings

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)

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

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

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'
@@ -55,6 +57,48 @@ 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()
+
+    # 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
+
+    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):
+    field_css = '#%s' % '-'.join([s.lower() for s in field.split()])
+    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):
+    """
+    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)
+
+
+@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 ##############
 def open_new_course():
     world.clear_courses()
@@ -184,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")

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 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 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 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
@@ -21,5 +24,50 @@ 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 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 press the "Save" notification button
+    When I change fields
+    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
+    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 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
+
+  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

cms/djangoapps/contentstore/features/course-settings.py

@@ -4,7 +4,7 @@
 from lettuce import world, step
 from terrain.steps import reload_the_page
 from selenium.webdriver.common.keys import Keys
-import time
+from common import type_in_codemirror
 
 from nose.tools import assert_true, assert_false, assert_equal
 
@@ -47,22 +47,11 @@ 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):
     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$')
@@ -71,8 +60,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 +106,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 +123,30 @@ 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):
+    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):
     """
@@ -155,9 +165,17 @@ def verify_date_or_time(css, date_or_time):
     assert_equal(date_or_time, world.css_find(css).first.value)
 
 
-def pause():
+def i_see_the_set_dates():
     """
-    Must sleep briefly to allow last time save to finish,
-    else refresh of browser will fail.
+    Ensure that each field has the value set in `test_and_i_set_course_dates`.
     """
-    time.sleep(float(1))
+    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)

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 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"
@@ -41,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"
 
@@ -49,5 +51,36 @@ 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 press the "Save" notification button
         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"
+        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 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

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')
@@ -99,6 +100,22 @@ 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')
+
+
+@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)

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", ->

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 = [];
-            });
-        }
     }
 });

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

cms/static/js/models/settings/course_grading_policy.js

@@ -71,24 +71,25 @@ 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.";
             }
             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 (attrs['weight']) {
-            if (!isFinite(attrs.weight) || /\D+/.test(attrs.weight)) {
+        if (_.has(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
@@ -97,19 +98,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;

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

cms/static/js/views/settings/advanced_view.js

@@ -56,9 +56,13 @@ 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 && !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));
                 }
             },
             onFocus : function(mirror) {
@@ -91,44 +95,11 @@ 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);
                 }
             }
         });
     },
-    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
@@ -138,25 +109,20 @@ 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
                 });
-            }
+            },
+            silent: true
         });
     },
-    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
         });
     },

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",
@@ -23,14 +26,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 +41,7 @@ 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.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);
@@ -61,11 +57,31 @@ 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 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.
+        _.each(['change', 'remove', 'add'],
+               function (event) {
+                   gradeCollection.on(event, function() {
+                       this.showNotificationBar();
+                       // 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);
         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));
         });
 
         // render the grade cutoffs
@@ -88,9 +104,10 @@ 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});
     },
     updateModel : function(event) {
         if (!this.selectorToField[event.currentTarget.id]) return;
@@ -100,7 +117,7 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
             break;
 
         default:
-            this.saveIfChanged(event);
+            this.setField(event);
             break;
         }
     },
@@ -220,13 +237,14 @@ 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});
     },
 
     addNewGrade: function(e) {
@@ -301,13 +319,45 @@ 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,
+            silent: 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({
     // 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",
@@ -331,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.]
@@ -342,26 +392,27 @@ 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)) {
+            // 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) != 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;
         default:
-            this.saveIfChanged(event);
+            this.setField(event);
         break;
         }
     },
     deleteModel : function(e) {
-        this.model.destroy();
         e.preventDefault();
+        this.collection.remove(this.model);
     }
-
 });

cms/static/js/views/validating_view.js

@@ -9,6 +9,11 @@ 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",
         "change textarea" : "clearValidationErrors"
@@ -20,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]);
@@ -27,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() {
@@ -36,19 +47,20 @@ 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);
     },
 
-    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);
+        this.model.isValid();
+        return newVal;
     },
     // these should perhaps go into a superclass but lack of event hash inheritance demotivates me
     inputFocus : function(event) {
@@ -67,5 +79,79 @@ CMS.Views.ValidatingView = Backbone.View.extend({
             // put error on the contained inputs
             return $(ele).find(inputElements);
         }
+    },
+
+    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: this.save_title,
+            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.model.clear({silent : true});
+                        self.confirmation.hide();
+                        self.notificationBarShowing = false;
+                    }
+                }]
+            }});
+        this.notificationBarShowing = true;
+        this.confirmation.show();
+        // Make sure the bar is in the right state
+        this.model.isValid();
+    },
+
+    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();
+        $.smoothScroll({
+            offset: 0,
+            easing: 'swing',
+            speed: 1000
+        });
+    },
+
+    saveView: function() {
+        var self = this;
+        this.model.save(
+            {},
+            {
+                success: function() {
+                    self.showSavedBar();
+                },
+                silent: true
+            }
+        );
     }
 });

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 {

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;