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.
2013-06-25 14:38:57 -04:00
parent 164a469e9d
commit 9094f1890b
10 changed files with 128 additions and 59 deletions
--- 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)
--- 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()
--- 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
--- 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))
--- 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
--- 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)
--- 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();
--- 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));
+                    }
                }
            });
        }
--- 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();
--- 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();
+                        }});
    }
 });