Peter fogg/explicit course settings #301

peter-fogg · 2013-06-28T15:19:36Z

Change the save model for all settings pages from an asynchronous save onBlur to an explicit save with notifications. There's a bit of refactoring around this new model -- where we previously called model.save, we now call model.set and pop up a confirmation. Settings are all reverted when the user hits the cancel button on said bar.

cahrens · 2013-06-30T14:24:30Z

cms/djangoapps/contentstore/features/common.py

@@ -54,6 +56,42 @@ 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):


The "save_clicked" helper method was written specifically around what happens when you press Save in Advanced Settings. Since it is only looking for either an error message to appear or the notification-warning to disappear, it might be fine as a general method for pressing a Save or Cancel button on a notification bar. But if we are making this a generic library method, the name of the helper method "save_clicked" and the comments around it should be modified.

cahrens · 2013-06-30T15:08:02Z

Assuming we stick with the current model, you need to scroll-to-top to show the "changes saved" notification. And you need to dismiss that when a new change is made, like we did in Advanced Settings.

However, I suggest talking to @talbs to see if we can show the "changes saved" notification at the bottom like you did for assets page. Then it could be auto-dismissing, which would also address the "shift up" issue that I didn't like in Advanced Settings (when you type, field suddenly shifts up because the confirmation message disappears).

Mark wants to QA the branch, but we should get the confirmation interaction right first.

cahrens · 2013-06-30T15:15:17Z

"New Assignment Type" on Course Settings/Grading does not work. Make sure to add a test for it when fixing the bug.

I'm also concerned about the fact that the notification does not appear until you move OUT of the field you are typing in (for input fields). This is something we talked about briefly with Don and DB. I'd like you to consider a way to get the notification to appear immediately, as it does in Advanced Settings and in the course overview editor.

cahrens · 2013-06-30T15:29:39Z

cms/djangoapps/contentstore/features/common.py

+        ele._element.send_keys(Keys.ENTER)
+
+
+@step('I reset the database')


I recommend some documentation, and/or a clearer name for this method. I wasn't sure what resetting the database would do-- would it clear the database but not log you out, or do a full log out?

cahrens · 2013-06-30T15:39:30Z

If there is a validation error, I don't think we should allow the user to press Save. For example, try making the video id "foo bar". You get the validation error, but you can still press Save. And on Save, it claims your changes were saved, though they were not (refresh). Should also add a test for this case.

cahrens · 2013-06-30T15:43:48Z

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

        var self = this;
-        this.model.deleteKeys = [];
-        this.model.clear({silent : true});


The clear was unnecessary?

The model is getting fetched anyway, so clearing it doesn't make a difference.

As for deleteKeys, I'm noticing that it doesn't look like it's needed anymore -- should I take it out of the model? (@dmitchell might weigh in too)

Yes, take it out. It was from a previous version where the user typed in keys as well as values b/c we had no fixed set of keys provided by the server. Definitely out of date.

@dmitchell It looks like all the logic of the advanced model is based around the delete keys. Does it make sense to replace this with CMS.Models.Settings.Advanced = Backbone.Model.extend({});, or just do away with the advanced model entirely in favor of using a vanilla backbone model?

cahrens · 2013-06-30T16:04:38Z

Oops, didn't mean to close the pull request.

cahrens · 2013-06-30T16:09:08Z

cms/static/js/views/settings/settings_grading_view.js

@@ -100,9 +109,10 @@ CMS.Views.Settings.Grading = CMS.Views.ValidatingView.extend({
            break;

        default:
-            this.saveIfChanged(event);
+            this.setField(event);
        break;


cahrens · 2013-06-30T16:54:03Z

Reviewing the relevant columns in the table below, it appears that some of the interactions may not be what @talbs wanted. You should clarify with him.

https://edx-wiki.atlassian.net/wiki/pages/viewpage.action?pageId=6783773

In particular,

Notification while save is in progress. Does not actually state we want the confirmation at the top.
Show confirmation dialog on delete of assignment type.

cahrens · 2013-06-30T17:08:05Z

I also ran across this more explicit description of what should happen if validation fails--

"If a user chooses to save their changes, their values are validated and if valid saved. The view is refreshed with the submitted values and a confirmation alert letting the user know that their changes have been saved (type: Alert - Confirmation (with close) - Saved Changes).

If a user chooses to save their changes, their values are validated and if not valid are not saved. The view is refreshed with the submitted values and a confirmation alert letting the user know that there are errors with their edits (type: Alert - Error - Submission)."

peter-fogg · 2013-07-03T19:16:47Z

@cahrens Finally in a state for more review -- addresses comments on the last iteration, fixing bugs, and UX changes. You may want to pull the branch as new, it's been rebased onto master since the last push.

cahrens · 2013-07-09T13:12:44Z

When there is a validation error, I am still able to press the Save button (and there is no feedback about the fact that the save did not occur). For example, choose an enrollment start date after the course start date.

Also, there is a failing test on the last build:

Test Result (1 failure / +1)
CMS.Views.SystemFeedback.close button sends a .hide() message

I'll hold off on reviewing the code again until these are fixed.

cahrens · 2013-07-09T13:16:16Z

cms/djangoapps/contentstore/features/grading.py

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


You no longer need these ENTERs, right? Now the notification comes up on type events.

cahrens · 2013-07-09T13:25:55Z

Just found a validation issue.

Change Homework weight to something else (for instance, 16)
Save changes
Delete the 6 (do this quickly, ideally while the "sliding" up is still happening
You get a validation error on Homework that "There's already another assignment type with this name."

If it doesn't reproduce, refresh and start again. I was able to reproduce it about 5 times, though it doesn't happen every time.

cahrens · 2013-07-10T20:58:36Z

cms/static/js/views/settings/main_settings_view.js

-
+        this.showNotificationBar(this.save_message,
+                                 _.bind(this.saveView, this),
+                                 _.bind(this.revertView, this));
    },

    removeSyllabus: function() {


Let's remove this syllabus code. It's not used.

When all settings require an explicit save, this functionality will be shared between each view (with slight changes, e.g. click handlers.)

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.

… and reliable reverting of changes.

cahrens · 2013-07-11T19:03:38Z

cms/static/js/views/validating_view.js

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


Add period on end (consistent with save_message).

cahrens · 2013-07-11T19:48:52Z

👍

…ps up correctly.

Peter fogg/explicit course settings

changes from master to release

…dx#301)

…ssage-for-required-fields Move a message for required fields to more noticeable position (openedx#301)

remove useless text

…te (openedx#301) Learners were having questions when we would continue showing them the 'Verify Now' button if they had a submitted a verification attempt already.

cahrens reviewed Jun 30, 2013
View reviewed changes

cahrens closed this Jun 30, 2013

cahrens reopened this Jun 30, 2013

cahrens reviewed Jun 30, 2013
View reviewed changes

cahrens reviewed Jul 9, 2013
View reviewed changes

cahrens reviewed Jul 10, 2013
View reviewed changes

Peter Fogg added 6 commits July 11, 2013 09:11

Factor showNotificationBar out into ValidatingView.

164a469

When all settings require an explicit save, this functionality will be shared between each view (with slight changes, e.g. click handlers.)

Refactor and update acceptance tests around saving settings.

eba1bd0

Make sure that changing grading cutoffs triggers a save confirmation,…

3d4f926

… and reliable reverting of changes.

Remove outdated deleteKeys from advanced settings.

ec13349

Update acceptance tests for course settings.

d9716a7

cahrens reviewed Jul 11, 2013
View reviewed changes

Peter Fogg added 2 commits July 11, 2013 15:53

Fix validation in many cases, and ensure that the notification bar po…

c2f0b7d

…ps up correctly.

Fix many validation bugs and failing tests.

6e94960

peter-fogg pushed a commit that referenced this pull request Jul 11, 2013

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

5b4524d

Peter fogg/explicit course settings

peter-fogg merged commit 5b4524d into master Jul 11, 2013

peter-fogg deleted the peter-fogg/explicit-course-settings branch July 11, 2013 21:09

Kelketek referenced this pull request in open-craft/edx-platform Nov 6, 2014

Merge pull request #301 from edx-solutions/master

00033f2

changes from master to release

yokose-ks added a commit to nttks/edx-platform that referenced this pull request Oct 2, 2015

Fix translations for drop-down menus in register page (openedx#301)

eb8266c

yokose-ks added a commit to nttks/edx-platform that referenced this pull request Oct 2, 2015

Fix translations in register page (openedx#301)

0dcebe1

yokose-ks added a commit to nttks/edx-platform that referenced this pull request Oct 7, 2015

Move a message for required fields to more noticeable position (opene…

729a2dc

…dx#301)

yokose-ks added a commit to nttks/edx-platform that referenced this pull request Oct 7, 2015

Merge pull request openedx#400 from yokose-ks/develop/cypress/move-me…

efe6a04

…ssage-for-required-fields Move a message for required fields to more noticeable position (openedx#301)

CrewS pushed a commit to CrewS/edx-platform-1 that referenced this pull request Feb 18, 2019

Merge pull request openedx#301 from jauzzz/i18n

56d6a72

remove useless text

This was referenced Oct 4, 2021

[Snyk] Security upgrade karma-webpack from 2.0.9 to 2.0.10 baby636/edx-platform#1

Open

[Snyk] Security upgrade karma-webpack from 2.0.9 to 2.0.10 baby636/edx-platform#7

Open

baby636 mentioned this pull request Nov 16, 2022

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#36

Open

baby636 mentioned this pull request Nov 29, 2022

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#39

Open

baby636 mentioned this pull request Apr 16, 2024

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#63

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Peter fogg/explicit course settings #301

Peter fogg/explicit course settings #301

peter-fogg commented Jun 28, 2013

cahrens Jun 30, 2013

cahrens commented Jun 30, 2013

cahrens commented Jun 30, 2013

cahrens Jun 30, 2013

cahrens commented Jun 30, 2013

cahrens Jun 30, 2013

peter-fogg Jul 1, 2013

dmitchell Jul 1, 2013

peter-fogg Jul 1, 2013

cahrens commented Jun 30, 2013

cahrens Jun 30, 2013

cahrens commented Jun 30, 2013

cahrens commented Jun 30, 2013

peter-fogg commented Jul 3, 2013

cahrens commented Jul 9, 2013

cahrens Jul 9, 2013

cahrens commented Jul 9, 2013

cahrens Jul 10, 2013

cahrens Jul 11, 2013

cahrens commented Jul 11, 2013

		ele._element.send_keys(Keys.ENTER)


		@step('I reset the database')

Peter fogg/explicit course settings #301

Peter fogg/explicit course settings #301

Conversation

peter-fogg commented Jun 28, 2013

Choose a reason for hiding this comment

cahrens commented Jun 30, 2013

cahrens commented Jun 30, 2013

Choose a reason for hiding this comment

cahrens commented Jun 30, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens commented Jun 30, 2013

Choose a reason for hiding this comment

cahrens commented Jun 30, 2013

cahrens commented Jun 30, 2013

peter-fogg commented Jul 3, 2013

cahrens commented Jul 9, 2013

Choose a reason for hiding this comment

cahrens commented Jul 9, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens commented Jul 11, 2013