Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sms): Create post add, post change, post remove recovery phone email templates #18161

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LZoog
Copy link
Contributor

@LZoog LZoog commented Dec 20, 2024

Because:

  • We want to send users an email when they add/change/remove a backup authentication phone number

This commit:

  • Adds 3x email templates including storybook and l10n
  • Adds basic mailer methods
  • Adjusts spacing after "You requested it from:" and "You enabled it from:"

closes FXA-10240

@LZoog LZoog force-pushed the FXA-10240 branch 3 times, most recently from 32ae4eb to 9cfcb94 Compare December 23, 2024 17:41
@LZoog LZoog marked this pull request as ready for review December 23, 2024 17:42
@LZoog LZoog requested review from a team as code owners December 23, 2024 17:42
…mail templates

Because:
* We want to send users an email when they add/change/remove a backup authentication phone number

This commit:
* Adds 3x email templates including storybook and l10n
* Adds basic mailer methods
* Adjusts spacing after "You requested it from:" and "You enabled it from:"

closes FXA-10240
Copy link
Contributor

@vpomerleau vpomerleau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on these and creating the mailer methods! The implementation matches the design, but in reviewing these I noticed that we might want to modify a few things (e.g., change password instead of reset password, which I think you might have noticed would make more sense based on the links you've included in storybooks)

Going to go ahead and approve, and will leave to your discretion whether or not the suggestions should be applied.

Mailer.prototype.postAddRecoveryPhoneEmail = function (message) {
const templateName = 'postAddRecoveryPhone';
const links = this._generateLinks(
this.initiatePasswordResetUrl,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if these emails should include a link to change the password instead of reset the password 🤔 I just checked some of our other emails for settings changes and they include "change your password" prompts.

Should we also suggest that they review their two-step authentication settings?

Similar to the recovery key emails that include a prompt to delete the key if it wasn't configured by the user: https://storage.googleapis.com/mozilla-storybooks-fxa/commits/latest/fxa-auth-server/index.html?path=/story/fxa-emails-templates-postaddaccountrecovery--post-add-account-recovery

device: this._formatUserAgentInfo(message),
privacyUrl: links.privacyUrl,
link: links.link,
// TODO, get actual last 4 when functionality is implemented
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add ticket number in TODO? This should be updated in FXA-10370

postAddRecoveryPhone-title = You created a new backup recovery method
# Variables:
# $lastFourPhoneNumber (String) - The last four digits of the user's phone number
postAddRecoveryPhone-description = You added ••••••{ $lastFourPhoneNumber } as your backup recovery phone
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add spaces or dashes to the phone number? e.g., •••-•••-1234

Copy link
Contributor Author

@LZoog LZoog Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since not every country uses this format, I'm not sure we want to do that? It wouldn't match Figma anyway. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice-to-have for readability, not a requirement, but we might need to review anyway once we support more countries because the number of digits won't be the same

postAddRecoveryPhone-how-protect = How this protects your account
# Variables:
# $twoFactorSupportLink (String) - a link to a support article about two factor authentication
postAddRecoveryPhone-how-protect-plaintext = How this protects your account: { $twoFactorSupportLink }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For plaintext, link probably shouldn't be included in localized string, and the link could just be mentioned in a comment

Copy link
Contributor Author

@LZoog LZoog Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have a few other things similar to this, like subscriptionFirstInvoice-content-invoice-number-plaintext. For this I thought it'd be nice for localizers to have the context that this was going to be a link and what that link was, but yes I can pull this out (they can have context with a comment).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example you mention includes a variable in string (not a link), so that seems a bit different :)

Copy link
Contributor Author

@LZoog LZoog Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a variable though, it'll be coming from templateValues. I have a TODO in that referenced ticket to update the hard coded string (it needs UTM params etc.)

Other places we send links in plain text like this with a variable:

  • partials/viewInvoice/index.txt (for invoiceLink)
  • partials/support/index.txt (for supportUrl)
  • templates/fradulentAccountDeletion/index.txt (for mozillaSupportUrl

</mj-text>

<mj-text css-class="text-body-no-margin">
<span data-l10n-id="postAddRecoveryPhone-description" data-l10n-args="<%= JSON.stringify({ lastFourPhoneNumber }) %>">You added ••••••<%- lastFourPhoneNumber %> as your backup recovery phone</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update phone number format here too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how the phone numbers are going to render in RTL languages - we might need to add a span with dir="ltr" to force the number to stay in ltr as is expected for both ltr and rtl languages

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not sure we want to update the format per Figma BUT you make a good point about adding <span dir="ltr"> here that I will do.

@@ -0,0 +1,4 @@
postRemoveRecoveryPhone-subject = Backup recovery phone removed
postRemoveRecoveryPhone-title = Backup recovery phone removed
postRemoveRecoveryPhone-description = Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup recovery codes available for use.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we missed a terminology update in Figma. This should be "backup authentication codes" - I've updated Figma to have the correct term.

Suggested change
postRemoveRecoveryPhone-description = Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup recovery codes available for use.
postRemoveRecoveryPhone-description = Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup authentication codes available for use.

<mj-section>
<mj-column>
<mj-text css-class="text-body">
<span data-l10n-id="postRemoveRecoveryPhone-description">Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup recovery codes available for use.</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<span data-l10n-id="postRemoveRecoveryPhone-description">Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup recovery codes available for use.</span>
<span data-l10n-id="postRemoveRecoveryPhone-description">Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup authentication codes available for use.</span>

@@ -0,0 +1,10 @@
postRemoveRecoveryPhone-title = "Backup recovery phone removed"

postRemoveRecoveryPhone-description = "Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup recovery codes available for use."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
postRemoveRecoveryPhone-description = "Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup recovery codes available for use."
postRemoveRecoveryPhone-description = "Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup authentication codes available for use."

])],
['html', [
{ test: 'include', expected: 'Backup recovery phone removed' },
{ test: 'include', expected: 'Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup recovery codes available for use.' },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{ test: 'include', expected: 'Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup recovery codes available for use.' },
{ test: 'include', expected: 'Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup authentication codes available for use.' },

]],
['text', [
{ test: 'include', expected: 'Backup recovery phone removed' },
{ test: 'include', expected: 'Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup recovery codes available for use.' },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{ test: 'include', expected: 'Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup recovery codes available for use.' },
{ test: 'include', expected: 'Your backup recovery phone has been removed from your two-step authentication settings. You still have your backup authentication codes available for use.' },

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catches here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants