feat(recovery-phone): Display phone number in desired format (masking + Twilio's nationalFormat) by LZoog · Pull Request #18340 · mozilla/fxa

LZoog · 2025-02-08T01:18:08Z

Because:

We want to show the national format provided by Twilio to better display phone numbers for users, but want to hide the formatting when a user's session is not verified

This commit:

Adds a Twilio lookup call at recovery phone number add for nationalFormat, reads nationalFormat from DB when querying recovery phone data
Updates our server response to return the last 4 digits of a phone number instead of the mask, displays accordingly on the client including new copy
Updates account resolver recovery phone auth-server call to ensure only the last 4 digits are returned if a user's session is not verified
postAddRecoveryPhone email copy tweak

closes FXA-11044

dschom · 2025-02-11T22:49:53Z

+    return undefined;
+  }
+  const { uid: userId, phoneNumber, lookupData } = row;
+  const nationalFormat = isLookupDataObject(lookupData)


This seems a lot simpler to me:

const nationalFormat = lookupData?.['nationalFormat']

We are at the DB layer here, so there might be some casting going on here. We know what we store is either null or a JsonObject, and if it's object it should contain nationalFormat, but might not. Either way the type guard doesn't do much for me, and I like the cleanliness of optimistic resolution. Just my two cents.

Yeah this was meant to be a thing to clean up for sure. I had tried that, and I get this:

Element implicitly has an 'any' type because expression of type '"nationalFormat"' can't be used to index type 'string | number | boolean | JsonArray | JsonObject'. Property 'nationalFormat' does not exist on type 'string | number | boolean | JsonArray | JsonObject'.

I wasn't sure if there was a better way to pull JSON out? But I can just go with
const nationalFormat = (lookupData as { nationalFormat?: string })?.nationalFormat which works. 👍

dschom · 2025-02-11T22:51:13Z

        new RecoveryPhoneNotEnabled()
      );
-      expect(() => service.maskPhoneNumber('+15550005555')).toThrow(
+      expect(() => service.stripPhoneNumber('+15550005555')).toThrow(


In someone ways I'd prefer both things to exist. Seems like UX flip flops a lot, and additive changes are easier to track.

I'm not sure if I like the back-end handling adding the bullet points. Since that feels more presentation layer to me, I think it's nicer for the front-end to handle it - so if we want to change this in the future, we can in those new recovery-phone-utils functions.

dschom · 2025-02-11T22:52:17Z

+            phoneNumber
+          );
+        } catch (e) {
+          // This should not fail since the number was already validated with Twilio but


I'd remove this... If it shouldn't fail, let's not worry about the try catch. If we are worried about the twilio call failing, then let's manually capture a sentry error so we at least know this feature isn't functioning properly.

There is just the possibility of a network error here for whatever reason. I can remove it if we want though.

dschom · 2025-02-11T22:59:07Z

+    // Clamp lastN between 0 and digits.length
+    if (lastN > digits.length) {
+      lastN = digits.length;
+    } else if (lastN <= 0) {


Semantically this doesn't make sense to me. Would would a negaive lastN digits even mean? I'd error or set to 0 as I did above.

I agree - I actually wasn't sure why we allowed a negative number in the other function, but right now this basically treats negative numbers the same as 0. I've changed this locally to just return an empty string for <= 0 as I think that's more clear.

dschom · 2025-02-11T23:05:24Z

    phoneNumber: string,
    headers?: Headers
-  ) {
+  ): Promise<{ nationalFormat?: string }> {


This also has a success flag. Might nice to include it too.

dschom · 2025-02-11T23:08:56Z

+    // OTP confirm page before then. "Basic lookups" from Twilio are free, so don't
+    // bother persisting in redis.
+    // https://www.twilio.com/en-us/user-authentication-identity/pricing/lookup
+    const { nationalFormat } = await this.smsManager.phoneNumberLookup(


I think this is true, but we might still hit twilio API rate limits. I am pretty sure customs will keep us safe, and we wouldn't encounter this type of volume, but just wanted to mention it.

Ah yeah. I was so torn on sticking this into redis or not, we only store OTP codes there now from what I can tell. Maybe this is worth noting somewhere in some docs.

I decided to leave this and just updated the comment noting it would be a network problem. I'm OK if we remove this later though.

dschom · 2025-02-11T23:12:40Z

              uid,
            }
          );
+          return { nationalFormat };


Should we maybe return both the phoneNumber and the nationalFormat, might be a better API design. Also it's unlikely but possible that the phoneNumber exists, but the nationalFormat does not...

dschom

Looks good to me. r+wc

LZoog · 2025-02-12T17:14:39Z

I'm going to go ahead and open this out of draft but still want to add a few more tests.

dschom · 2025-02-13T20:33:43Z

              uid,
            }
          );
+          return { phoneNumber, nationalFormat };


I think this is the problem... Missing { status: RecoveryPhoneStatus.SUCCESS }

… + Twilio's nationalFormat) Because: * We want to show the national format provided by Twilio to better display phone numbers for users This commit: * Adds a Twilio lookup call at recovery phone number add for nationalFormat, reads nationalFormat from DB when querying recovery phone data * Updates our server response to return the last 4 digits of a phone number instead of the mask, displays accordingly on the client including new copy * Updates account resolver recovery phone auth-server call to ensure only the last 4 digits are returned if a user's session is not verified closes FXA-11044

LZoog commented Feb 8, 2025

View reviewed changes

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

LZoog force-pushed the FXA-11044 branch 4 times, most recently from 92dd1a7 to a8c9e68 Compare February 11, 2025 17:37

LZoog commented Feb 11, 2025

View reviewed changes

Comment thread packages/fxa-settings/src/components/Settings/PageRecoveryPhoneSetup/index.tsx Outdated

LZoog commented Feb 11, 2025

View reviewed changes

Comment thread packages/fxa-settings/src/pages/Signin/SigninRecoveryChoice/container.tsx Outdated

LZoog force-pushed the FXA-11044 branch from a8c9e68 to 3d2fa6e Compare February 11, 2025 19:00

LZoog commented Feb 11, 2025

View reviewed changes

Comment thread packages/fxa-settings/src/pages/Signin/SigninRecoveryChoice/container.tsx Outdated

dschom reviewed Feb 11, 2025

View reviewed changes

Comment thread packages/fxa-graphql-api/src/gql/model/recoveryPhone.ts

dschom reviewed Feb 11, 2025

View reviewed changes

Comment thread packages/fxa-settings/src/components/Settings/SubRow/index.test.tsx Outdated

dschom reviewed Feb 11, 2025

View reviewed changes

Comment thread packages/fxa-settings/src/lib/recovery-phone-utils.test.tsx Outdated

dschom reviewed Feb 11, 2025

View reviewed changes

Comment thread packages/fxa-settings/src/pages/Signin/SigninRecoveryPhone/container.tsx Outdated

dschom approved these changes Feb 11, 2025

View reviewed changes

LZoog force-pushed the FXA-11044 branch from 3d2fa6e to b566065 Compare February 12, 2025 17:05

LZoog marked this pull request as ready for review February 12, 2025 17:15

LZoog requested review from a team as code owners February 12, 2025 17:15

LZoog force-pushed the FXA-11044 branch 2 times, most recently from cac5e46 to b85d415 Compare February 13, 2025 17:47

bcolsson approved these changes Feb 13, 2025

View reviewed changes

LZoog force-pushed the FXA-11044 branch from b85d415 to 8e924e1 Compare February 13, 2025 18:55

LZoog force-pushed the FXA-11044 branch from 8e924e1 to 0b92ab0 Compare February 13, 2025 19:19

dschom reviewed Feb 13, 2025

View reviewed changes

LZoog force-pushed the FXA-11044 branch 2 times, most recently from 28346ca to b28bd9c Compare February 13, 2025 21:11

LZoog force-pushed the FXA-11044 branch from b28bd9c to 56beb12 Compare February 13, 2025 22:00

LZoog mentioned this pull request Feb 13, 2025

chore(tests): Fix mock import issue, more investigation needed #18374

Merged

LZoog merged commit 27e14c9 into main Feb 13, 2025

LZoog deleted the FXA-11044 branch February 13, 2025 22:34

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

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!

Uh oh!

Uh oh!

Uh oh!

dschom left a comment

Choose a reason for hiding this comment

Uh oh!

LZoog commented Feb 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LZoog commented Feb 8, 2025 •

edited

Loading

dschom Feb 11, 2025 •

edited

Loading

LZoog Feb 12, 2025 •

edited

Loading

dschom Feb 11, 2025 •

edited

Loading