[HOLD for payment 2024-09-18][$250] Profile - Phone number login is displayed as @expensify.sms

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.29-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

1. Login with phone number
2. Go to Account settings
3. Take a look at the top of the page

Expected Result:

Phone number should be displayed without suffix @expensify.sms

Actual Result:

Phone number is displayed with suffix @expensify.sms

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021833010029599658221
• Upwork Job ID: 1833010029599658221
• Last Price Increase: 2024-09-09
• Automatic offers:
• jjcoffee | Reviewer | 103887462
• dominictb | Contributor | 103887465

Issue Owner

Current Issue Owner: @kadiealexander

[melvin-bot]

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[FitseTLT]

Edited by proposal-police: This proposal was edited at 2024-09-05 14:03:16 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Phone number login is displayed as @expensify.sms

What is the root cause of that problem?

We didn't remove sms domain here

App/src/components/AccountSwitcher.tsx

Line 163 in 6760f96

{currentUserPersonalDetails?.login}

What changes do you think we should make in order to solve the problem?

We need to remove sms domain

                            {Str.removeSMSDomain(currentUserPersonalDetails?.login ?? '')}

We might also need to do the same here for the display name too

App/src/components/AccountSwitcher.tsx

Line 146 in 6760f96

{currentUserPersonalDetails?.displayName}

It might be more centralized if we remove sms domains in useCurrentUserPersonalDetails

    const login = accountPersonalDetails?.login;
    const currentUserPersonalDetails: PersonalDetails = useMemo(
        () => ({...accountPersonalDetails, ...(login && {login: Str.removeSMSDomain(login ?? '')}), accountID}),

What alternative solutions did you explore? (Optional)

If we want to format phone number in addition to removing sms domain we can use formatPhoneNumber from useLocalize here

App/src/components/AccountSwitcher.tsx

Line 34 in 6760f96

const {translate} = useLocalize();

    const {translate, formatPhoneNumber} = useLocalize();

                            {formatPhoneNumber(currentUserPersonalDetails?.login ?? '')}

[Nodebrute]

Edited by proposal-police: This proposal was edited at 2024-09-05 14:09:22 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Phone number login is displayed as @expensify.sms

What is the root cause of that problem?

We are not removing the expensify here domain also we are not formatting the phone number as we do in Profile page

What changes do you think we should make in order to solve the problem?

We can use LocalePhoneNumber.formatPhoneNumber here. we can do something like this

 { LocalePhoneNumber.formatPhoneNumber(currentUserPersonalDetails?.login ?? '')}

Note

We use the same LocalePhoneNumber.formatPhoneNumber in useLocalize() so it's the same approach

What alternative solutions did you explore? (Optional)

Or we can format phone number directly in useCurrentUserPersonalDetails.ts

[FitseTLT]

Updated to add alternative approach

[Nodebrute]

Proposal updated added a note

[dominictb]

Proposal

Please re-state the problem that we are trying to solve in this issue.

• Phone number is displayed with suffix @expensify.sms

What is the root cause of that problem?

• When implementing AccountSwitcher component, we don't remove @expensify.sms in:
  

  App/src/components/AccountSwitcher.tsx

  Line 51 in 6760f96

  description: personalDetails?.login,

  
  and
  

  App/src/components/AccountSwitcher.tsx

  Line 163 in 6760f96

  {currentUserPersonalDetails?.login}

What changes do you think we should make in order to solve the problem?

• We need to remove @expensify.sms domain by using Str.removeSMSDomain in:
  

  App/src/components/AccountSwitcher.tsx

  Line 51 in 6760f96

  description: personalDetails?.login,

  
  and
  

  App/src/components/AccountSwitcher.tsx

  Line 163 in 6760f96

  {currentUserPersonalDetails?.login}

• Note: We cannot remove @expensify.sms in useCurrentUserPersonalDetails directly because useCurrentUserPersonalDetails are used in other positions. For example, in:
  

  App/src/components/AccountSwitcher.tsx

  Line 78 in 6760f96

  if (delegateEmail === currentUserPersonalDetails.login) {

  
  and
  

  App/src/components/AccountSwitcher.tsx

  Line 101 in 6760f96

  .filter(({email}) => email !== currentUserPersonalDetails.login)

  
  the delegateEmail can be contained @expensify.sms, so if we remove the @expensify.sms from currentUserPersonalDetails.login, condition delegateEmail === currentUserPersonalDetails.login can be failed.

What alternative solutions did you explore? (Optional)

Result

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~021833010029599658221

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee (External)

[jjcoffee]

Thanks for all the proposals! I really appreciate the attention to detail in @dominictb's proposal in identifying that applying the formatting in useCurrentUserPersonalDetails would be a bad idea, as well as the two locations we need to apply the formatting to. So I'm happy to select their proposal.

We probably need to settle on whether or not the phone number should be formatted or simply stripped of the @expensify.sms, but that's a minor detail that can be sorted out on the PR. @kadiealexander Do you have any guidance here?

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @luacmartins, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[Nodebrute]

@jjcoffee Thanks for the review. The only difference between my proposal and @dominictb's is that he included an extra place where the same fix is needed. This is a minor detail that isn't within the scope of the issue. Typically, C+ doesn’t select proposals that are very similar to previous ones with just one additional place added.

[jjcoffee]

@Nodebrute No problem! As I mentioned in my review there was also a second component to the decision, namely that @dominictb was thorough enough to notice applying the formatting in useCurrentUserPersonalDetails would lead to potential regressions. I think it makes sense to choose a thorough proposal with attention to detail over ones that lack those traits; it's worth taking care over the small details even with relatively simple issues.

[Nodebrute]

@jjcoffee Thank you for getting back to me so quickly. I would like to clarify that using useCurrentUserPersonalDetails was not included in my main proposal. Therefore, I don’t believe my proposal is incorrect.

[luacmartins]

I agree with @jjcoffee's decision. I'm not sure if formatting the number is necessary, since the login is actually not formatted

[melvin-bot]

📣 @jjcoffee 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @dominictb 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[jjcoffee]

• The PR that introduced the bug has been identified. Link to the PR: Delegate access: Add account switcher #47338
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/47338/files#r1778216717
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A - just needed to test phone numbers
• Determine if we should create a regression test for this bug. Yes
• If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

1. In OD, assign an account with phone number login as Copilot following this doc
2. In ND, login with that phone number
3. Go to Settings
4. Verify that the phone number under your user avatar (top left) does not contain @expensify.sms domain
5. Press the account avatar to open the account switcher
6. Verify that the phone number in the account switcher does not contain @expensify.sms domain

Do we agree 👍 or 👎

[jjcoffee]

Oops looks like the automation failed on this one! The PR was deployed Sept 11 (see checklist), so should be (over)due for payment. cc @kadiealexander

[dominictb]

@kadiealexander Good to pay.

[melvin-bot]

@luacmartins, @jjcoffee, @kadiealexander, @dominictb Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[kadiealexander]

Payouts due:

• Contributor: $250 @dominictb
• Contributor+: $250 @jjcoffee

Upwork job is here.

[kadiealexander]

https://github.com/Expensify/Expensify/issues/432325