feat(email): reinstate account verification reminder emails #2990

philbooth · 2019-03-26T12:43:28Z

Adds logic for creating, deleting and processing account verification reminders, which was removed because of MySQL-related complications in #2283. The implementation here depends on Redis sorted sets instead of MySQL, which were added to fxa-shared in mozilla/fxa-shared#65.

The code here works and has tests, but it's still marked WIP because I haven't written the script for the fxa-admin node yet. Opening in case anyone wants to cast an eye over it or offer feedback while I finish that off.

Also note there is an unresolved question about the copy for the emails, since they still mention "a few days ago..." and "a week ago..." but the actual intervals are now 1 day and 5 days. Presumably we're going to fix the copy rather than the intervals.

shane-tomlinson · 2019-03-26T13:23:42Z

\o/

davismtl · 2019-03-26T18:26:53Z

lib/senders/templates/verification_reminder_first.html

+<tr style="page-break-before: always">
+  <td valign="top">
+    <h1 style="font-family: sans-serif; font-size: 21px; line-height: 29px; font-weight: normal; margin: 0 0 11px 0; text-align: center;">{{t "Hello again."}}</h1>
+    <p class="primary" style="font-family: sans-serif; font-size: 14px; line-height: 21px; font-weight: normal; margin: 0 0 21px 0; text-align: center;">{{{t "A few days ago you created a Firefox Account, but never verified it. A verified account lets you access your tabs, bookmarks, passwords and history on any device connected to it. Simply confirm this email address to activate your account."}}}</p>


I recommend this copy instead of the current paragraph:

You recently created a Firefox Account but haven't confirmed your email yet. Confirm your email so that you can start using Firefox Sync, Pocket, Lockbox, Firefox Send, Monitor and more.

Make sure "Confirm your email" is in bold. This is the main call to action that matches the button.

CC @ryanfeeley
The copy was very Sync specific. I've made it shorter and a bit more generic. I'm also trying to eliminate synonyms that were used interchangeably (i.e. verify, confirm and activate). This will make things less confusing.

@davismtl, regarding the part in bold, that is going to cause problems for localisers. Word order etc are obviously completely different depending on language, so we can't just split the sentence up and assume that the bold part is always supposed to be before the non-bold part. In some languages it will be in the middle or at the end or possibly even split apart in more than one place.

If you can rework the copy so that we're breaking things up into full sentences, that would work, or if you want to keep that copy and lose the bold, that's fine too.

(alternatively, @shane-tomlinson / @vbudhram, if there are clever markup tricks we can employ to work round this, let me know)

(for now I've just ditched the bold, but someone shout at me if I should do something else)

The template here renders the string using a {{{t, so you should be able to use a <b> or <strong> HTML element within the string. Ugly, but I think it should work? Localizers don't mind HTML as long as there's not too much.

As mentioned in IRC, I don't think that's an improvement. Going to merge this as is, but you're welcome to change it as a follow-up if you feel strongly about it.

lib/senders/templates/verification_reminder_first.html

lib/senders/templates/verification_reminder_first.txt

lib/senders/templates/verification_reminder_second.html

vbudhram

Thanks @philbooth, this looks good overall, I just had a few questions/comments.

lib/metrics/amplitude.js

vbudhram · 2019-03-26T19:07:49Z

lib/routes/emails.js

@@ -14,7 +14,9 @@ const validators = require('./validators');

 const HEX_STRING = validators.HEX_STRING;

-module.exports = (log, db, mailer, config, customs, push) => {
+module.exports = (log, db, mailer, config, customs, push, verificationReminders) => {
+  const REMINDER_PATTERN = new RegExp(`^(?:${verificationReminders.keys.join('|')})$`);


You could export the regex directly from verificationReminders or put into the validators lib.

I'm minded to keep this where it is because it's local to this module and conceptually it seems like the right place. Exporting it from lib/verification-reminders pollutes that module with knowledge about the API surface and putting it in the validators lib would force that to import lib/verification-reminders.

lib/routes/emails.js

lib/senders/email.js

lib/routes/emails.js

lib/senders/templates/verification_reminder_first.html

lib/verification-reminders.js

vbudhram · 2019-03-26T19:41:22Z

test/local/routes/emails.js

  });
  const route = getRoute(accountRoutes, '/recovery_email/verify_code');
+
+  afterEach(() => {


Nice cleanup 👍

lib/verification-reminders.js

shane-tomlinson

Overall, this looks good @philbooth, I left a few comments about areas I find confusing.

There is one case I'm wondering how we'll handle, the case of a deleted account. This can happen if a user signs up, never verifies, then signs up again with the same email address. The original account will be deleted. I imagine it should be fine since you won't be able to get the email address for the uid, but want to point out that it's possible and should be accounted for in the script that does the sending.

lib/senders/email.js

lib/senders/templates/_versions.json

lib/verification-reminders.js

test/local/devices.js

test/local/metrics/amplitude.js

philbooth · 2019-03-28T13:23:25Z

Removing the WIP from this and calling it ready for review.

I've been using these steps to test the script locally end-to-end:

Sign up for an account. Don't verify.

In your terminal, set environment variables to reduce the verification reminder intervals. For instance, if you're an enlightened soul who likes fish:

set -x VERIFICATION_REMINDERS_FIRST_INTERVAL '10 seconds'
set -x VERIFICATION_REMINDERS_SECOND_INTERVAL '1 minute'

Or if you prefer ye olde-fashionede wayse:

export VERIFICATION_REMINDERS_FIRST_INTERVAL='10 seconds'
export VERIFICATION_REMINDERS_SECOND_INTERVAL='1 minute'

After your chosen interval has elapsed, run:
```
node scripts/verification-reminders
```

You should then see the reminder emails turn up in your inbox (or the link will show up in the mailer log if you're doing it that way).

This is what the first email looks like rendered:

And this is the second one:

@mozilla/fxa-devs r?

philbooth · 2019-03-28T14:06:51Z

Just realised the ViewAction thing isn't showing up for me, so I'm digging into that now:

philbooth · 2019-03-28T14:54:54Z

...the ViewAction thing isn't showing up...

I double-checked the markup against the docs, ran it through the validator, still no luck. The only thing occurring to me at the moment is whether it might be related to the MIME-encoding we use in the email service. I've noticed the emails where ViewActions work for me are all 7bit encoding, whereas we use base64.

I can have a play with the email service at some point to see if that fixes it but regardless, that won't be in this PR.

philbooth · 2019-03-28T16:14:23Z

The only thing occurring to me at the moment is whether it might be related to the MIME-encoding we use in the email service.

Another possibility that occurs to me is it could be because the emails are being sent from a dev.lcip address in my test setup. Maybe in prod, with accounts@firefox.com and DMARC and all that, it will start working?

philbooth · 2019-03-28T16:18:48Z

Another thing I just noticed, our emails have <html>, <head> and <body> tags, but other emails where I see this working (e.g. GitHub) just go straight into the content of the body. Should we be doing that too?

philbooth · 2019-03-28T16:42:10Z

Putting a shipit on this because it needs to land tomorrow if it's to go out on train 134.

davismtl · 2019-03-28T17:57:00Z

@philbooth can you just make the following text in the body bold?

Confirm this email in the first email

and

Confirm this email address to activate your account in bold for the second email

philbooth · 2019-03-28T18:01:30Z

@davismtl, see the note and question above about bold text and localization:

#2990 (comment)

davismtl · 2019-03-28T19:20:17Z

@philbooth ahh, that makes sense. Thanks for clarifying.

How about we make the sentences that start with "Confirm" their own paragraphs so that the call to actions stand out rather than being part of the larger paragraph?

philbooth · 2019-03-28T19:22:40Z

How about we make the sentences that start with "Confirm" their own paragraphs so that the call to actions stand out rather than being part of the larger paragraph?

Yep, that works, will update the PR.

vbudhram

@philbooth This test locally and it runs pretty well 👍🏽. I have just the one question about including service name but I think that can be done in a follow up issue.

vbudhram · 2019-03-28T19:32:33Z

lib/senders/templates/verification_reminder_first.html

+
+<body style="-ms-text-size-adjust: 100%; -webkit-text-size-adjust: 100%; margin: 0; padding: 0;">
+
+<script type="application/ld+json">


vbudhram · 2019-03-28T19:46:47Z

lib/routes/account.js

@@ -279,6 +279,8 @@ module.exports = (log, db, mailer, Password, config, customs, signinUtils, push)
                    tokenVerificationId: tokenVerificationId
                  });
                }
+
+                return verificationReminders.create(account.uid);


This might be a can of worms, but should we store the verification link with the redirectTo, service, etc? If we do this, then it will be possible for those users to get linked to the right verification page for the service.

I think it's a valid suggestion but the code I'm reinstating didn't behave that way and it's too late in the train to implement it now. The choice is to ship on train 134 without that or delay the feature for two weeks.

philbooth · 2019-03-28T20:17:32Z

Updated screenshots with the "Confirm your email" sentences starting fresh paragraphs:

These changes now have been addressed and an r+ was given in Slack.

philbooth added WIP waffle:active train-134 labels Mar 26, 2019

philbooth self-assigned this Mar 26, 2019

philbooth mentioned this pull request Mar 26, 2019

Re-enable verification reminder emails #2939

Closed

philbooth force-pushed the pb/2939-verification-reminders branch 3 times, most recently from e8a7942 to 1f7d55d Compare March 26, 2019 15:00

davismtl suggested changes Mar 26, 2019

View reviewed changes

davismtl previously requested changes Mar 26, 2019

View reviewed changes

vbudhram reviewed Mar 26, 2019

View reviewed changes

clouserw added this to the Train 134 milestone Mar 26, 2019

philbooth force-pushed the pb/2939-verification-reminders branch from 1f7d55d to a9898d4 Compare March 27, 2019 07:20

shane-tomlinson reviewed Mar 27, 2019

View reviewed changes

philbooth force-pushed the pb/2939-verification-reminders branch 2 times, most recently from a792d8d to 5e536d4 Compare March 27, 2019 07:53

This was referenced Mar 27, 2019

feat(redis): implement atomic zpoprangebyscore method for sorted sets mozilla/fxa-shared#67

Merged

fix(redis): fix broken zpoprangebyscore argument-marshalling mozilla/fxa-shared#69

Merged

feat(email): reinstate account verification reminder emails

7bd920e

philbooth force-pushed the pb/2939-verification-reminders branch 2 times, most recently from 473cd0e to e9bcdec Compare March 28, 2019 12:36

feat(scripts): add a script to process verification reminders

b42276f

philbooth force-pushed the pb/2939-verification-reminders branch 3 times, most recently from ddcc65a to 28094c0 Compare March 28, 2019 13:05

philbooth added waffle:review and removed WIP waffle:active labels Mar 28, 2019

philbooth requested review from davismtl and a team March 28, 2019 13:24

philbooth force-pushed the pb/2939-verification-reminders branch 2 times, most recently from cbf9e6c to d3b26f4 Compare March 28, 2019 14:38

philbooth added the shipit label Mar 28, 2019

philbooth force-pushed the pb/2939-verification-reminders branch 2 times, most recently from 0e53262 to d8acc65 Compare March 28, 2019 20:00

fix(email): use new copy for verification reminder emails

0ab024a

philbooth force-pushed the pb/2939-verification-reminders branch from d8acc65 to 042547a Compare March 28, 2019 20:03

feat(email): add view actions to verification reminder emails

a326c28

philbooth force-pushed the pb/2939-verification-reminders branch from 042547a to a326c28 Compare March 28, 2019 20:10

vbudhram approved these changes Mar 28, 2019

View reviewed changes

philbooth merged commit e566a18 into master Mar 29, 2019

ghost removed train-134 waffle:review labels Mar 29, 2019

philbooth deleted the pb/2939-verification-reminders branch March 29, 2019 05:51

shane-tomlinson added the train-134:qa-needed label Apr 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(email): reinstate account verification reminder emails #2990

feat(email): reinstate account verification reminder emails #2990

philbooth commented Mar 26, 2019 •

edited

Loading

shane-tomlinson commented Mar 26, 2019

davismtl Mar 26, 2019

philbooth Mar 28, 2019

philbooth Mar 28, 2019

shane-tomlinson Mar 28, 2019

philbooth Mar 29, 2019

vbudhram left a comment

vbudhram Mar 26, 2019

philbooth Mar 28, 2019

vbudhram Mar 26, 2019

shane-tomlinson left a comment

philbooth commented Mar 28, 2019

philbooth commented Mar 28, 2019

philbooth commented Mar 28, 2019

philbooth commented Mar 28, 2019 •

edited

Loading

philbooth commented Mar 28, 2019

philbooth commented Mar 28, 2019

davismtl commented Mar 28, 2019 •

edited

Loading

philbooth commented Mar 28, 2019 •

edited

Loading

davismtl commented Mar 28, 2019

philbooth commented Mar 28, 2019

vbudhram left a comment

vbudhram Mar 28, 2019

vbudhram Mar 28, 2019

philbooth Mar 28, 2019

philbooth commented Mar 28, 2019


		<body style="-ms-text-size-adjust: 100%; -webkit-text-size-adjust: 100%; margin: 0; padding: 0;">

		<script type="application/ld+json">

feat(email): reinstate account verification reminder emails #2990

feat(email): reinstate account verification reminder emails #2990

Conversation

philbooth commented Mar 26, 2019 • edited Loading

shane-tomlinson commented Mar 26, 2019

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

Choose a reason for hiding this comment

vbudhram left a comment

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

shane-tomlinson left a comment

Choose a reason for hiding this comment

philbooth commented Mar 28, 2019

philbooth commented Mar 28, 2019

philbooth commented Mar 28, 2019

philbooth commented Mar 28, 2019 • edited Loading

philbooth commented Mar 28, 2019

philbooth commented Mar 28, 2019

davismtl commented Mar 28, 2019 • edited Loading

philbooth commented Mar 28, 2019 • edited Loading

davismtl commented Mar 28, 2019

philbooth commented Mar 28, 2019

vbudhram left a comment

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

philbooth commented Mar 28, 2019

philbooth commented Mar 26, 2019 •

edited

Loading

philbooth commented Mar 28, 2019 •

edited

Loading

davismtl commented Mar 28, 2019 •

edited

Loading

philbooth commented Mar 28, 2019 •

edited

Loading