Change email dialog #22309

islemaster · 2018-05-10T17:29:24Z

Progress on the spec Email opt-in consent on account changes.

What's in this PR

This first change replaces the current email update flow on the account page with a new React dialog for changing email address.

Here's the flow for a teacher:

And for a student (we leave the email hidden on the actual account page):

Validation

Email is required (checked on client and server)

Email must be valid (checked on client and server)

Email must have changed (checked on client)

Email must be available (checked on server)

Password is required (checked on client and server)

Password must be correct (checked on server)

Error handling

An unexpected server error results in this message:

Unit-tested behaviors

ChangeEmailForm
  calls onChange
    ✔ when the email field changes
    ✔ when the password field changes
    ✔ when the email opt-in field changes
  calls onSubmit
    ✔ when the enter key is pressed in the email field
    ✔ when the enter key is pressed in the password field
    ✔ when the enter key is pressed on the opt-in field
    ✔ but not when other keys are pressed
    ✔ and not when the form is disabled
  when disabled
    ✔ the email field is disabled
    ✔ the password field is disabled
    ✔ the opt-in field is disabled
  focusOnAnError()
    ✔ does nothing if there are no validation errors
    ✔ focuses on the email field if there is an email validation error
    ✔ focuses on the password field if there is a password validation error
    ✔ focuses on the email field if there are both email and password validation errors
ChangeEmailModal
  ✔ disables everything and shows save text when saving
  ✔ shows unknown error text when an unknown error occurs
  ✔ calls handleCancel when clicking the cancel button
  validation
    ✔ checks that email is present
    ✔ checks that email is valid
    ✔ checks that email is not the same as the current email
    ✔ reports email server errors
    ✔ checks that password is present
    ✔ reports password server errors
    ✔ disables the submit button when validation errors are present
    ✔ enables the submit button form passes validation
  changes clear server errors
    ✔ on email
    ✔ on password
  onSubmitSuccess
    ✔ calls handleSubmit with the new email
  onSubmitFailure
    ✔ puts the dialog in UNKNOWN ERROR state if response has no JSON
    ✔ loads returned validation errors into dialog state

islemaster · 2018-05-11T01:27:18Z

Hey Eric. I've still got to do a little cleanup and write tests for this, but could you do an initial scan? I'm taking a somewhat unconventional approach here and maybe you can stop me from doing something dumb now before I spend a bunch of time cleaning it up. 😁

islemaster · 2018-05-11T16:52:50Z

Also, security/optics question (@poorvasingal @jeremydstone):

Email must have changed (checked on client)

For students, this check sort of lets the user manually rainbow-table attack their own hashed email, guessing email addresses until they get this validation error. This not new from a security point of view: We're already rendering the hashed email to the client's DOM, and the user must be signed in to see this view (this is the "edit my account" page). But do we think this will look like a security problem to users that don't understand how hashed emails work? Do we care?

I personally think the improved user experience outweighs the imagined security concern, but thought I should bring it up.

poorvasingal · 2018-05-11T17:47:42Z

I wonder if we need to care if the email address is the same? So basically, just end up doing a no-op if the email address is updated to the same as before? That way we can avoid this issue?

Other alternative is to not show the error until they hit the submit button.

islemaster · 2018-05-11T18:22:12Z

@breville : Tests still coming soon, but this is ready for early review. There's some ugly action-at-a-distance stuff between the HAML view and JavaScript, I'd love advice on fixing that.

breville · 2018-05-11T18:56:19Z

Oh, one other possibility is to not post via a form at all, but to use a more simple web API. While you wouldn't have the token, your server-side code can still authorize the user using CanCan. @aoby has expertise here.

ewjordan · 2018-05-11T19:50:40Z

apps/src/sites/studio/pages/devise/registrations/edit.js

@@ -61,8 +91,6 @@ $(document).ready(() => {
    if (needToConfirmEmail) {
      e.preventDefault();
      showConfirmEmailModal(onCancelModal, onSubmitModal(e));
-    } else if ($('#user_email').length) {
-      prepareEmailData();


Are we sure that everything else works fine if this is removed? I don't really remember the logic here, and when prepareEmailData() needs to be called or not.

I presume this is ok because the email fields have been removed from the form, and are now handled by the React modal.

Andrew's got it: I took a pretty close look at this. I've removed the only #user_email element on this page to replace it with the uneditable email display and "Update" link, so $('#user_email').length would always evaluate false, making this dead code.

ewjordan · 2018-05-11T19:56:56Z

dashboard/app/controllers/registrations_controller.rb

@@ -116,7 +116,11 @@ def respond_to_account_update(successfully_updated, flash_message_kind = :update
        format.any {head :no_content}
      else
        format.html {render "edit", formats: [:html]}
-        format.any {head :unprocessable_entity}
+        format.any do
+          render status: :unprocessable_entity,


The opaque erroring here has bothered me for sooooo long, but I always forgot about it when I was working nearby - thank you for setting this up!

ewjordan · 2018-05-11T20:04:38Z

As far as I'm aware, the way you've handled the HAML/JS stuff there is about as good as it can get while sticking with that method of updating (leaning on the autogenerated user update machinery). @breville's suggestion about using a Web API and handling auth through CanCan is interesting, and maybe worth looking into whether it's a better pattern for all of this stuff?

I don't really like the autogenerated Rails updaters at all, because they're opaque to debug, a bit too magical, and (especially) leave the configuration of business functionality inside a HAML view. But that may be a longer term thing to look into, depending how tough API-ifying it would be.

aoby

I only skimmed the UI part, but the Rails side LGTM. As @breville mentioned, and as I discussed offline w Brad, pulling this out into a separate API has tradeoffs: without a token, it will be arguably less secure in that it won't require updating through the UI (but will still require log in), and it will mean writing a new API controller. On the other hand, the submit logic will be cleaner, send the state directly over JSON instead of filling and submitting a hidden form. I don't have a strong preference.

aoby · 2018-05-11T20:26:19Z

apps/src/sites/studio/pages/devise/registrations/edit.js

@@ -61,8 +91,6 @@ $(document).ready(() => {
    if (needToConfirmEmail) {
      e.preventDefault();
      showConfirmEmailModal(onCancelModal, onSubmitModal(e));
-    } else if ($('#user_email').length) {
-      prepareEmailData();


I presume this is ok because the email fields have been removed from the form, and are now handled by the React modal.

aoby · 2018-05-11T20:34:36Z

apps/src/lib/ui/ChangeEmailModal.jsx

+      this._form.find('#change-email-modal_user_email').val(this.props.userAge < 13 ? '' : this.state.newEmail);
+      this._form.find('#change-email-modal_user_hashed_email').val(hashEmail(this.state.newEmail));
+      this._form.find('#change-email-modal_user_current_password').val(this.state.currentPassword);
+      // this._form.find('#user_email_opt_in').val(this.state.emailOptIn);


why commented out? Is this a future field?

Yes. The whole reason we're converting this to a dialog is so that we can explicitly ask teachers to re-opt-in to marketing email when they change their email address. I started building this out, then realized just switching to a dialog for this flow was complicated enough, so I'll be adding the opt-in as a follow-on PR. I wasn't sure it was worth extracting the work I'd done so far though.

ChangeEmailForm calls onChange ✔ when the email field changes ✔ when the password field changes ✔ when the email opt-in field changes calls onSubmit ✔ when the enter key is pressed in the email field ✔ when the enter key is pressed in the password field ✔ when the enter key is pressed on the opt-in field ✔ but not when other keys are pressed ✔ and not when the form is disabled when disabled ✔ the email field is disabled ✔ the password field is disabled ✔ the opt-in field is disabled focusOnAnError() ✔ does nothing if there are no validation errors ✔ focuses on the email field if there is an email validation error ✔ focuses on the password field if there is a password validation error ✔ focuses on the email field if there are both email and password validation errors

ChangeEmailModal ✔ disables everything and shows save text when saving ✔ disables everything and shows save text when saving ✔ calls handleCancel when clicking the cancel button validation ✔ checks that email is present ✔ checks that email is valid ✔ checks that email is not the same as the current email ✔ reports email server errors ✔ checks that password is present ✔ reports password server errors ✔ disables the submit button when validation errors are present ✔ enables the submit button form passes validation changes clear server errors ✔ on email ✔ on password

islemaster · 2018-05-12T00:50:14Z

PTAL!

I've done the additional cleanup I wanted to get to before merging this and added some unit test coverage of the React parts of this change.

I've extracted a lot of the rendering code out to a ChangeEmailForm display component, leaving ChangeEmailModal where most of the ugliness lives smaller and easier to understand.

I'm not submitting a UI test with this change because there's expected follow-on work immediately after it (the email opt-in work) and I'd like to save the end-to-end test until that's in place.

islemaster · 2018-05-14T19:04:08Z

Just discovered that this change breaks the ability to upgrade a student account to a teacher account, because it depends on the email field that this PR removes.

My immediate follow-up work involves changing that "change account type" flow 😢 and maybe I need to combine that work with this one to not break anything?

islemaster · 2018-05-14T19:34:20Z

Spoke with Poorva and we're okay with temporary disabling the ability for student accounts to upgrade to teacher accounts, for the day or two between these two features. We'll give our helpdesk a heads-up before we ship this change.

islemaster requested a review from ewjordan May 11, 2018 01:25

islemaster changed the title ~~[WIP] Change email dialog~~ Change email dialog May 11, 2018

islemaster added 14 commits May 11, 2018 09:24

Nonfunctional dialog layout with story

d9d55da

Attach ChangeEmailModal to account page

5728937

Interactable fields and client-side validation

64c8ee9

Able to successfully update email address

7598a61

Better documentation of the Rails UJS scheme

b6cb184

Remove copypasta'd comment

c25c79f

Server sends back validation errors, client handles them gracefully

907957b

Proper JSON response and handling of unknown error

249f179

Disable when saving

0b80087

Submit on enter key

a7da285

Not changing email is a validation error

4aeb229

Break out header and footer components

a73dfe9

Update email on the account page after successful submission

f5dbf38

Focus email input when dialog opens

2a9db7f

islemaster force-pushed the change-email-opt-in branch from a5546b9 to 2a9db7f Compare May 11, 2018 16:24

islemaster added 4 commits May 11, 2018 11:04

Fix id collision

8c28d54

Use 'namespace' feature

a16420b

Remove old email update code

e31b996

Formatting cleanup

865465e

islemaster requested a review from breville May 11, 2018 18:21

ewjordan reviewed May 11, 2018

View reviewed changes

aoby approved these changes May 11, 2018

View reviewed changes

islemaster added 6 commits May 11, 2018 14:42

Extract ChangeEmailForm

a97ab50

Change state format

ed93558

Test success and failure handlers

fb28643

Deduplicate hashing logic

4d146da

islemaster added 2 commits May 11, 2018 17:57

Fix test name

3fbadeb

Fix ChangeEmailModal story

29ed7e6

islemaster added 2 commits May 14, 2018 12:26

Extract and test can_change_own_user_type?

1e1b16c

Temporary constraint on changing user type

df13710

Remove UI test no longer relevant under new spec

dd54d6f

islemaster merged commit b2418b7 into staging May 15, 2018

This was referenced May 16, 2018

Fix capitalization of i18n string #22459

Merged

New "Update account type" flow #22403

Merged

islemaster deleted the change-email-opt-in branch May 16, 2018 23:18

This was referenced May 17, 2018

API support for opt-in when changing email or account type #22471

Merged

Add controls to opt-in to email when changing email or account type #22539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change email dialog #22309

Change email dialog #22309

islemaster commented May 10, 2018 •

edited

islemaster commented May 11, 2018

islemaster commented May 11, 2018 •

edited

poorvasingal commented May 11, 2018

islemaster commented May 11, 2018

breville commented May 11, 2018

ewjordan May 11, 2018

aoby May 11, 2018

islemaster May 11, 2018

ewjordan May 11, 2018

ewjordan commented May 11, 2018

aoby left a comment

aoby May 11, 2018

aoby May 11, 2018

islemaster May 11, 2018

islemaster commented May 12, 2018

islemaster commented May 14, 2018

islemaster commented May 14, 2018

Change email dialog #22309

Change email dialog #22309

Conversation

islemaster commented May 10, 2018 • edited

What's in this PR

Validation

Error handling

Unit-tested behaviors

islemaster commented May 11, 2018

islemaster commented May 11, 2018 • edited

poorvasingal commented May 11, 2018

islemaster commented May 11, 2018

breville commented May 11, 2018

ewjordan May 11, 2018

Choose a reason for hiding this comment

aoby May 11, 2018

Choose a reason for hiding this comment

islemaster May 11, 2018

Choose a reason for hiding this comment

ewjordan May 11, 2018

Choose a reason for hiding this comment

ewjordan commented May 11, 2018

aoby left a comment

Choose a reason for hiding this comment

aoby May 11, 2018

Choose a reason for hiding this comment

aoby May 11, 2018

Choose a reason for hiding this comment

islemaster May 11, 2018

Choose a reason for hiding this comment

islemaster commented May 12, 2018

islemaster commented May 14, 2018

islemaster commented May 14, 2018

islemaster commented May 10, 2018 •

edited

islemaster commented May 11, 2018 •

edited