Skip to content
This repository has been archived by the owner. It is now read-only.

feat: Add change password, reset password, and new sync device notification emails. #26

Merged
merged 1 commit into from Jun 17, 2015

Conversation

<table border="0" cellpadding="0" cellspacing="0" width="70%" style="border-collapse:collapse !important;mso-table-rspace:0pt;mso-table-lspace:0pt;-ms-text-size-adjust:100%;-webkit-text-size-adjust:100%;">
<tr>
<td valign="top" class="bodyContent" mc:edit="body_content" style="text-align:center;padding-left:20px;padding-bottom:30px;padding-right:20px;padding-top:30px;font-family:Helvetica;mso-table-rspace:0pt;mso-table-lspace:0pt;-ms-text-size-adjust:100%;-webkit-text-size-adjust:100%;color:#8A9BA8;line-height:13px;font-size:11px;">
{{{t "This is an automated email; if you didn't change the password to your Firefox Account, you should reset it immediately at <a href='https://accounts.firefox.com/reset_password?email=%(encodedEmail)s'>https://accounts.firefox.com/reset_password</a>."}}}

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Feb 22, 2015
Author Member

https://accounts.firefox.com/reset_password?email=%(encodedEmail)s

Maybe a better solution is to pass in the entire link, that way it can be customized per deployment and there is less for localizers to worry about.

text: localized.text,
html: localized.html,
headers: {
'Content-Language': translator.language

This comment has been minimized.

@pdehaan

pdehaan Feb 24, 2015
Contributor

Should this have an 'X-Link': link as well?

(I'm guessing "no" since I dont see a values = { link: ... })

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Feb 25, 2015
Author Member

@pdehaan - your guess is correct, this email does not send a link. I should document that.

@@ -0,0 +1,5 @@
{{t "Password Change Successfully" }}

This comment has been minimized.

@pdehaan

pdehaan Mar 17, 2015
Contributor

"Change" or "Changed" (past tense)?
If it's "Change" we may need more words.
If it's "Changed" then it's probably fine and matches the "successfully changed" text on line 2 below.


I didn't see that string in the HTML version of this email, so I can't compare.

@jrgm
Copy link
Contributor

@jrgm jrgm commented Apr 14, 2015

So, is this ready for review. It has a WIP label on it.

@shane-tomlinson shane-tomlinson force-pushed the shane-tomlinson:issue-24-initial-notifications branch from 65a3ad4 to 3a3dc9d May 20, 2015
@shane-tomlinson shane-tomlinson force-pushed the shane-tomlinson:issue-24-initial-notifications branch 2 times, most recently from ed5a8fe to 1c94226 Jun 9, 2015
@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Jun 9, 2015

I'm calling this ready. Goes with mozilla/fxa-auth-server#876

This can merge before the auth server work.

@rfk - r?

@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Jun 10, 2015

Here are screenshots of the emails:

new sync device

screen shot 2015-06-10 at 10 07 04

password reset

screen shot 2015-06-10 at 10 07 18

password changed

screen shot 2015-06-10 at 10 07 27

}
}
return this.send(email)
}

This comment has been minimized.

@rfk

rfk Jun 10, 2015
Member

There's a lot of structural dupliction in these functions. @shane-tomlinson do you think there's scope for factoring out a common helper here? (Maybe in a follow-up PR, but worth thinking about)

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Jun 10, 2015
Author Member

@shane-tomlinson do you think there's scope for factoring out a common helper here?

I thought the same thing as I was copy-pasting. ;)

This comment has been minimized.

{{t "Your Firefox Account password has been successfully reset." }}

{{t "This is an automated email; if you didn’t reset the password to your Firefox Account, you should reset it immediately at %(resetLink)s" }}
{{t "For more information, please visit https://support.mozilla.org" }}

This comment has been minimized.

@rfk

rfk Jun 10, 2015
Member

I'm not sure the top-level SUMO page will be much help, should we consider a dedicted article for this?

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Jun 10, 2015
Author Member

We should, all of the previous emails suffer from the same problem.

this.mailer = nodemailer.createTransport('SMTP', options)
this.sender = config.sender
this.verificationUrl = config.verificationUrl
this.initiatePasswordResetUrl = config.initiatePasswordResetUrl

This comment has been minimized.

@rfk

rfk Jun 10, 2015
Member

This should be added to config.js for completeness

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Jun 10, 2015
Author Member

done

@rfk
Copy link
Member

@rfk rfk commented Jun 10, 2015

I guess we should get @ryanfeeley to sign off on the emails here as well, unless that's covered in a previous issue?

@rfk rfk assigned ryanfeeley and unassigned rfk Jun 10, 2015
@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Jun 10, 2015

@ryanfeeley - Can you have a look at the screen shots I pasted? I find the password reset one a bit strange. The user receives a notification email at the address they just verified when they reset their password.

Another thing I noticed:
bash
➜ templates git:(issue-24-initial-notifications) ✗ grep support.mozilla.org *.txt
new_sync_device.txt:{{t "For more information, please visit https://support.mozilla.org"}}
password_changed.txt:{{t "For more information, please visit https://support.mozilla.org" }}
password_reset.txt:{{t "For more information, please visit https://support.mozilla.org" }}
reset.txt:{{t "This is an automated email; if you received it in error, no action is required."}} {{t "For more information, please visit https://support.mozilla.org"}}
unlock.txt:{{t "This is an automated email; if you received it in error, no action is required."}} {{t "For more information, please visit https://support.mozilla.org"}}
verify.txt:{{t "This is an automated email; if you received it in error, no action is required."}} {{t "For more information, please visit https://support.mozilla.org"}}
```
If you look at the first three (the new emails), we only have text about visiting sumo. The last 3 all say "This is an automated email;..." Should we add the automated email text to the new emails?

Ignore all of this. The automated email bit is done on a different line.

@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Jun 10, 2015

@rfk and @ryanfeeley - I also find the new sync device a bit strange, esp considering we do not offer the user any way to revoke access to the new device, nor do we give them any information about the device.

Should we try to present some sensible device name based on UA string? I'm fine if something like this is a follow on PR, it just seems odd to send a notification email that provides little useful information and no way of revoking access.

@shane-tomlinson shane-tomlinson force-pushed the shane-tomlinson:issue-24-initial-notifications branch from 1c94226 to e02f7fd Jun 10, 2015
@ryanfeeley
Copy link
Contributor

@ryanfeeley ryanfeeley commented Jun 15, 2015

@shane-tomlinson Looks good, although try to follow the new template set by @johngruen with slightly different font sizing and the Firefox logo above used in the new verification email.

Because changing the password disconnects all clients, perhaps for the new sync devices we should say:

h1. Account Activity Notice

p.A new device is now syncing with all of the devices connected to your Firefox Account.

small.This is an automated email; if you didn’t add a new device to your Firefox Account, you should change your password immediately at [link]. For more information, please visit https://support.mozilla.org

@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Jun 15, 2015

@shane-tomlinson Looks good, although try to follow the new template set by @johngruen with slightly different font sizing and the Firefox logo above used in the new verification email.

It looks like @johngruen's changes have not yet landed. I'm of the opinion, first in wins. The other has to update. 🎱

Because changing the password disconnects all clients, perhaps for the new sync devices we should say:

The text here looks the same except for the addition of the link in the footer?

@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Jun 15, 2015

Thanks @ryanfeeley! I forgot to link to you above.

@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Jun 15, 2015

From @johngruen in IRC:

[21:51:54] stomlinson: i think we should get your emails merged, and my email merged, and then follow up by installing some tooling in the auth mailer to make everything totally DRY

@shane-tomlinson shane-tomlinson force-pushed the shane-tomlinson:issue-24-initial-notifications branch from e02f7fd to 1384972 Jun 16, 2015
@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Jun 16, 2015

@ryanfeeley - updated the text.

@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Jun 16, 2015

@ryanfeeley - I have a couple of comments after further testing.

  • Some of the emails titles contain Firefox Accounts, some do not.
    • Neither "Reset your password" nor "Verify your account" mention Firefox Accounts. I feel like we should add "Firefox Accounts" to these titles, e.g., "Reset your Firefox Accounts password"

screen shot 2015-06-16 at 10 47 09

  • The main header of "Reset your password" use sentence casing whereas the new emails capitalize the first letter of each word.

screen shot 2015-06-16 at 10 51 57

  • For the "Password Changed Successfully" and "Password Reset Successfully" emails, we swap the order of (Changed|Reset) & Successfully between the title and the body. We use the <verb> successfully pattern within the content server, should we swap the order in the body text too?

screen shot 2015-06-16 at 10 54 43

screen shot 2015-06-16 at 10 42 32

  • Should we add a "Your Firefox Account has been deleted" email (possibly for a separate PR)?
@johngruen
Copy link
Contributor

@johngruen johngruen commented Jun 16, 2015

@shane-tomlinson: checked out all the emails using the email on acid testing service. Everything looks right and the links are all working correctly!

@ryanfeeley
Copy link
Contributor

@ryanfeeley ryanfeeley commented Jun 16, 2015

@shane-tomlinson Good idea. I would keep it singular (account, not accounts)

e.g.
"Reset your Firefox Account password"
"Verify your Firefox Account"

"reset succesfully" in heading is fine to contrast "succesfully reset" in the body (if one way doesn't sink in by scanning the text, he other will)

Avoid using "Your". I think deletion message is fine like that (right under Firefox Account heading).

@shane-tomlinson shane-tomlinson force-pushed the shane-tomlinson:issue-24-initial-notifications branch from 1384972 to 0f04878 Jun 16, 2015
@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Jun 16, 2015

From IRC:

[21:20:59]  <rfeeley>   it's okay
[21:21:12]  <rfeeley>   your is fine
[21:21:20]  <rfeeley>   don't sweat it
...
[21:22:50]  <stomlinson>    Ah, one final thing - sentence case vs capitalise the first letter of every word?
[21:23:01]  <stomlinson>    for the main header
[21:23:35]  <rfeeley>   sentence case
@ryanfeeley
Copy link
Contributor

@ryanfeeley ryanfeeley commented Jun 16, 2015

image01

@shane-tomlinson shane-tomlinson force-pushed the shane-tomlinson:issue-24-initial-notifications branch from 0f04878 to 064bfca Jun 16, 2015
@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Jun 16, 2015

Here are all of the updated screens. We now have a symmetry that I can deal with. Thanks @ryanfeeley!

Original emails (with updated titles)

screen shot 2015-06-16 at 21 36 12

screen shot 2015-06-16 at 21 36 05

screen shot 2015-06-16 at 21 46 10

New emails

screen shot 2015-06-16 at 21 35 40

screen shot 2015-06-16 at 21 35 33

screen shot 2015-06-16 at 21 35 58

Titles

screen shot 2015-06-16 at 21 46 25

@ryanfeeley
Copy link
Contributor

@ryanfeeley ryanfeeley commented Jun 17, 2015

Ship it!

@shane-tomlinson
Copy link
Member Author

@shane-tomlinson shane-tomlinson commented Jun 17, 2015

Thanks @ryanfeeley!

@rfk - mind if I issue the refactor PR (See shane-tomlinson#2) separately, after this one? I have asked @zaach to hold off on the string extraction to slip this in, and don't want to hold up any longer than necessary, and honestly, I'm not completely happy with my cleanup.

If that seems reasonable, could you do an r?

@rfk
Copy link
Member

@rfk rfk commented Jun 17, 2015

SGTM, r+

rfk added a commit that referenced this pull request Jun 17, 2015
…tions

feat: Add change password, reset password, and new sync device notification emails.
@rfk rfk merged commit 8a5d7ef into mozilla:master Jun 17, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shane-tomlinson shane-tomlinson deleted the shane-tomlinson:issue-24-initial-notifications branch Jun 21, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants