search

mozilla / fxa-auth-server

DEPRECATED - Migrated to https://github.com/mozilla/fxa
Mozilla Public License 2.0
399 stars 121 forks source link

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

Closed philbooth closed 5 years ago

philbooth commented 5 years ago

Fixes #2939.

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 commented 5 years ago

\o/

philbooth commented 5 years ago

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:

  1. Sign up for an account. Don't verify.

  2. 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'

  3. 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:

Rendered view of the first reminder email

And this is the second one:

Rendered view of the second reminder email

@mozilla/fxa-devs r?

philbooth commented 5 years ago

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

Screenshot showing the unread verification reminder email in a GMail inbox
philbooth commented 5 years ago

...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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

davismtl commented 5 years ago

@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 commented 5 years ago

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

https://github.com/mozilla/fxa-auth-server/pull/2990#discussion_r269916968

davismtl commented 5 years ago

@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 commented 5 years ago

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.

philbooth commented 5 years ago

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

Rendered detail of the first verification reminder email Rendered detail of the second verification reminder email