task(recovery-phone, auth): Allow for fallback sms messages by dschom · Pull Request #18433 · mozilla/fxa

dschom · 2025-02-22T02:11:00Z

Because

We want the messages we send out to be under one gsm segment in length
Length isn't straightforwards. It varies based on encoding.
Language translation may impact length.

This pull request

Adds fallback messages for 'formatted messages'
Adds ability to determine SMS segment length
Removes some stray logs found in session.js
Changes config to specify max segment length
Makes getFormattedMessages a required parameter

Issue that this pull request solves

Closes: FXA-11072

Checklist

Put an x in the boxes that apply

My commit is GPG signed.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

vbudhram · 2025-02-22T03:09:12Z

  });

-  it('Should setup a phone number', async () => {
+  it.only('Should setup a phone number', async () => {


Stray .only

vbudhram · 2025-02-22T03:11:52Z

  };
+  const mockGetFormattedMessage = async (code: string) => {
+    return {
+      msg: `${code} is your Mozilla verification code. Expires in 5 minutes.`,


Maybe I'm over thinking but I wonder if we need to include the expiration. The expiration is on the webpage. For reference Twilio sends this message, Your Twilio verification code is: 859440 and does not include expiration time. In this case, the text lenght would be rough 40 characters so the risk for translations being over 160 would be much lower.

vbudhram · 2025-02-22T03:16:04Z

-        ? `${code} is your Mozilla verification code. Expires in 5 minutes.`
-        : `${code} is your Mozilla recovery code. Expires in 5 minutes.`;
+    const l10nTypeMap = {
+      setup: {


I do really like the flexibility of this 👍🏽

This is much nicer 👍

vbudhram

I had to resolve some merge conflicts on main, but made the recommended changes. Wouldn't hurt to have another set of eyes.

bcolsson

These strings should still probably use the brand term

bcolsson · 2025-02-24T21:51:58Z

+# https://twiliodeved.github.io/message-segment-calculator/
+# Messages should be limited to one segment
+# $code  - 6 digit code used to verify phone ownership when registering a recovery phone
+recovery-phone-setup-sms-short-body = Mozilla verification code: ${code}


This should use the brand term for Mozilla. Additionally - placeholders should be formatted as { $variable }

bcolsson · 2025-02-24T21:52:09Z

+# https://twiliodeved.github.io/message-segment-calculator/
+# Messages should be limited to one segment
+# $code  - 6 digit code used to verify phone ownership when registering a recovery phone
+recovery-phone-setup-sms-short-body = Mozilla code: ${code}


This should use the brand term for Mozilla. Additionally - placeholders should be formatted as { $variable }

Id should be recovery-phone-signin-sms-short-body.

vpomerleau

Spotted a ftl id mismatch, other comments are mainly questions/nits that are not blocking.

vpomerleau · 2025-02-24T22:06:04Z

-        env: 'RECOVERY_PHONE__SMS__MAX_MESSAGE_LENGTH',
+      maxMessageSegmentLength: {
+        default: 1,
+        doc: 'Max allows sms message segment length',


Nit

Suggested change

doc: 'Max allows sms message segment length',

doc: 'Max allowed sms message segment length',

vpomerleau · 2025-02-24T22:08:52Z

+# https://twiliodeved.github.io/message-segment-calculator/
+# Messages should be limited to one segment
+# $code  - 6 digit code used to verify phone ownership when registering a recovery phone
+recovery-phone-setup-sms-short-body = Mozilla code: ${code}


Id should be recovery-phone-signin-sms-short-body.

vpomerleau · 2025-02-24T22:09:51Z

-        ? `${code} is your Mozilla verification code. Expires in 5 minutes.`
-        : `${code} is your Mozilla recovery code. Expires in 5 minutes.`;
+    const l10nTypeMap = {
+      setup: {


This is much nicer 👍

vpomerleau · 2025-02-24T22:11:58Z

+      return {
+        msg: localizedMessage,
+        shortMsg: shortLocalizedMessage,
+        failsafeMsg: `Mozilla: ${code}`,


Using this failsafe all the time would save a lot of trouble 😅

vpomerleau · 2025-02-24T22:14:46Z

  it('handles message template when provided to set up phone number', async () => {
    mockOtpManager.generateCode.mockReturnValue(code);
-    const getFormattedMessage = jest.fn().mockResolvedValue('message');
+    // const getFormattedMessage = jest.fn().mockResolvedValue('message');


Delete this line?

vpomerleau · 2025-02-24T22:20:28Z

+          '${code} is your Mozilla recovery code. Expires in 5 minutes.',
+      },
+      'signin-short': {
+        id: 'recovery-phone-setup-sms-short-body',


Suggested change

id: 'recovery-phone-setup-sms-short-body',

id: 'recovery-phone-signin-sms-short-body',

vpomerleau · 2025-02-24T22:21:41Z

+# https://twiliodeved.github.io/message-segment-calculator/
+# Messages should be limited to one segment
+# $code  - 6 digit code used to verify phone ownership when registering a recovery phone
+recovery-phone-setup-sms-short-body = { -brand-mozilla } code: { $code }


Suggested change

recovery-phone-setup-sms-short-body = { -brand-mozilla } code: { $code }

recovery-phone-signin-sms-short-body = { -brand-mozilla } code: { $code }

vpomerleau · 2025-02-24T22:21:59Z

+# Shorter message sent by SMS with limited character length, please test translation with the messaging segment calculator
+# https://twiliodeved.github.io/message-segment-calculator/
+# Messages should be limited to one segment
+# $code  - 6 digit code used to verify phone ownership when registering a recovery phone


Suggested change

# $code - 6 digit code used to verify phone ownership when registering a recovery phone

# $code - 6 digit code used to sign in with a recovery phone as backup for two-step authentication

vpomerleau · 2025-02-24T22:26:49Z

+    // Pick the message with the best length and send it
+    const formattedMessages = await getFormattedMessages(code);
+    const body = this.getSafeSmsBody(formattedMessages);
    const msg = await this.smsManager.sendSMS({


Should this be wrapped in a try/catch block to prevent MessageBodyTooLong errors (though unlikely) from bubbling up to the user?

I'll punt on that for now, maybe ok to fail hard and fast here.

Because: - We want the messages we send out to be under one gsm segment in length - Length isn't straightforwards. It varies based on encoding. - Language translation may impact length. This Commit: - Adds fallbacks for 'formatted messages' - Adds ability to determine sms segment length - Removes some stray logs found in session.js - Changes config to specify max segment length

dschom requested a review from a team as a code owner February 22, 2025 02:11

dschom requested a review from vpomerleau February 22, 2025 02:12

vbudhram reviewed Feb 22, 2025

View reviewed changes

vpomerleau reviewed Feb 24, 2025

View reviewed changes

Comment thread packages/fxa-auth-server/lib/routes/recovery-phone.ts

vbudhram force-pushed the FXA-11072 branch 2 times, most recently from 1caa3ee to 82915e3 Compare February 24, 2025 21:03

vbudhram requested a review from a team as a code owner February 24, 2025 21:03

vbudhram requested a review from vpomerleau February 24, 2025 21:05

vbudhram approved these changes Feb 24, 2025

View reviewed changes

bcolsson requested changes Feb 24, 2025

View reviewed changes

vbudhram force-pushed the FXA-11072 branch from 82915e3 to 63b5669 Compare February 24, 2025 22:18

vpomerleau requested changes Feb 24, 2025

View reviewed changes

vbudhram force-pushed the FXA-11072 branch from 63b5669 to 44cf9af Compare February 24, 2025 22:52

vbudhram requested review from bcolsson and vpomerleau February 25, 2025 15:14

vpomerleau approved these changes Feb 25, 2025

View reviewed changes

bcolsson approved these changes Feb 25, 2025

View reviewed changes

vpomerleau merged commit fed0072 into main Feb 26, 2025

vpomerleau deleted the FXA-11072 branch February 26, 2025 00:23

	doc: 'Max allows sms message segment length',
	doc: 'Max allowed sms message segment length',

	id: 'recovery-phone-setup-sms-short-body',
	id: 'recovery-phone-signin-sms-short-body',

	recovery-phone-setup-sms-short-body = { -brand-mozilla } code: { $code }
	recovery-phone-signin-sms-short-body = { -brand-mozilla } code: { $code }

	# $code - 6 digit code used to verify phone ownership when registering a recovery phone
	# $code - 6 digit code used to sign in with a recovery phone as backup for two-step authentication

Conversation

dschom commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Because

This pull request

Issue that this pull request solves

Checklist

Screenshots (Optional)

Other information (Optional)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vbudhram left a comment

Choose a reason for hiding this comment

Uh oh!

bcolsson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpomerleau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dschom commented Feb 22, 2025 •

edited

Loading