[HOLD for payment 2024-12-25] [$250] Room-On inviting a phone contact via suggestions in room chat, expensify.sms shown in details

[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.66-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: 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. Launch app
2. Open a room chat
3. Invite a phone number via suggestion eg: +919789945670
4. Send the message
5. From whisper, tap invite
6. Tap header -- member
7. Open the phone number member
8. Note expensify.sms is shown with phone number
9. Navigate to room conversation
10. Tap on highlighted invited member
11. Tap header -- member - phone number contact
12. Now note expensify.sms is not shown

Expected Result:

Expensify.sms must not be shown with phone number in details page when inviting a phone contact via suggestions in room chat

Actual Result:

Expensify.sms is shown with phone number in details page when inviting a phone contact via suggestions in room chat

Workaround:

Unknown

Platforms:

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

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6673857_1732338819810.Screenrecorder-2024-11-23-10-34-39-415_compress_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021861102829467266920
• Upwork Job ID: 1861102829467266920
• Last Price Increase: 2024-11-25
• Automatic offers:
• Kalydosos | Contributor | 105507469

Issue Owner

Current Issue Owner: @

[melvin-bot]

Triggered auto assignment to @garrettmknight (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.

[abzokhattab]

Edited by proposal-police: This proposal was edited at 2024-11-23 13:57:15 UTC.

Proposal

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

When inviting a phone contact via suggestions in the room chat, expensify.sms is incorrectly displayed.

What is the root cause of that problem?

We do not remove the SMS domain (expensify.sms) from the display name on the room participant page:

App/src/pages/RoomMemberDetailsPage.tsx

Line 50 in 93c573b

const displayName = details.displayName ?? '';

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

We should use the getDisplayNameOrDefault function, which appropriately handles cases where the current contact is an SMS contact, among other conditions. This approach is already implemented in several places, such as:

• ReportActionItem.tsx#L606

• WorkspacesListRow.tsx#L219

The proposed implementation would look like this:

const displayName = getDisplayNameOrDefault(details);

What alternative solutions did you explore? (Optional)

An alternative proposal is to use Str.removeSMSDomain:

const displayName = getDisplayNameOrDefault(details);

[abzokhattab]

Proposal updated

Same approach but rephrased the proposal.

[Kalydosos]

Proposal

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

On inviting a phone contact via suggestions in room chat, expensify.sms shown in details

What is the root cause of that problem?

the display name was not retrieved with the usual considerations including removing any sms domain and formatting the phone number

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

we should change the following code from

App/src/pages/RoomMemberDetailsPage.tsx

Line 50 in 93c573b

const displayName = details.displayName ?? '';

into

const displayName = formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(details));

RESULT

it works fine

What alternative solutions did you explore? (Optional)

None

[melvin-bot]

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

[melvin-bot]

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

[klajdipaja]

Edited by proposal-police: This proposal was edited at 2024-11-26 11:50:34 UTC.

Proposal

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

When inviting a memeber to a room the displaed name shows the full phone number including exepnsify.sms.

What is the root cause of that problem?

When we invite a user by phone number both displayName and login information of the user are set to the full phone number.
Part of the problem is the backend here.
The response of InviteToRoom endpoint returns the onxy merge changes for personalDetails which contain the details of all accounts and for a non existing user on Expensify it returns both displayName and login as the full name.

If we click on the invited user on the message we then get the correct information from the BE andpersonalDetails key is updated with the formatted displayName which does not include the sms domain.

In the frontend we rely only on the displayName that the backend returns here:

App/src/pages/RoomMemberDetailsPage.tsx

Line 50 in 93c573b

const displayName = details.displayName ?? '';

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

Option 1.

We can add a front end fix to format the displayName if it is an sms login by changing the line where we read the displayName into this:

  const displayName = Str.isSMSLogin(details.displayName) ? LocalePhoneNumber.formatPhoneNumber(details.login) : PersonalDetailsUtils.getDisplayNameOrDefault(details);

I added PersonalDetailsUtils.getDisplayNameOrDefault(details) for some consistency with the other usages of the displayName like the one in MentionUserRenderer but we can also keep it like this:

  const displayName = Str.isSMSLogin(details.displayName) ? LocalePhoneNumber.formatPhoneNumber(details.login) : details.displayName;

Option 2.
In RoomMemberDetailsPage do:

const displayName = PersonalDetailsUtils.getDisplayNameOrDefault(details);

And change the getDisplayNameOrDefault function so that it returns the formattedNumber as display name when the login and displayName are both the phone number.
In this block when we have the data that we have for the current bug we just remove the SMS domain from the display name:

App/src/libs/PersonalDetailsUtils.ts

Line 63 in 084a711

if (displayName === passedPersonalDetails?.login && Str.isSMSLogin(passedPersonalDetails?.login)) {

We could modify this to:

        displayName = LocalePhoneNumber.formatPhoneNumber(displayName);
    }

which would provide a consistent handling of the display name across the app instead of having to do another transformation on top of the displayName when we have a phone number. We need this now for example in the ProfilePage; for the new user if you click on Profile button there's a few moments when you see the display name returned by getDisplayNameOrDefault and then when the backend response comes in you see the properly formatted phone number

What alternative solutions did you explore? (Optional)

[klajdipaja]

Updated proposal #53003 (comment) due to premature click on the submit button and also added another option while I was at it :D

[Kalydosos]

@klajdipaja the test is already done in

App/src/libs/LocalePhoneNumber.ts

Lines 25 to 28 in c7c7690

	// do not parse the string, if it doesn't contain the SMS domain and it's not a phone number
	if (number.indexOf(CONST.SMS.DOMAIN) === -1 && !CONST.REGEX.DIGITS_AND_PLUS.test(number)) {
	return number;
	}

formatPhoneNumber will not format the text if it is not a phone number

[klajdipaja]

@Kalydosos yes I am aware of that. IMO making that explicit check adds valuable information for the developer reading that line. We are formatting the displayName as a phone number because we expect it that sometimes it can be a phone number.

I also don't like that the formatPhoneNumber just returns the text if it's not a number, it doesn't sound like something a formatter should do.

[klajdipaja]

Proposal updated #53003 (comment). Added Option 2 to handle the issue on a broader scope

[Kalydosos]

@jayeshmangwani can you give a look at our proposals ?

[jayeshmangwani]

Added Option 2 to handle the issue on a broader scope

@klajdipaja Then your proposal will be the same as this one #53003 (comment), right?

[jayeshmangwani]

@Kalydosos What difference will it make if we use formatPhoneNumber for PersonalDetailsUtils.getDisplayNameOrDefault(details)?

We can simply use PersonalDetailsUtils.getDisplayNameOrDefault(details) alone, and it will still work correctly. So, what is the purpose of using formatPhoneNumber here?

[klajdipaja]

Added Option 2 to handle the issue on a broader scope

@klajdipaja Then your proposal will be the same as this one #53003 (comment), right?

@jayeshmangwani No it's very different.
That proposal is suggesting using PersonalDetailsUtils.getDisplayNameOrDefault(details) to get the display name in the desired format but that would not return the desired display name.

Assume I have this phone number as my login method: +355678899777. The userDetails.login for this would be: [email protected].
If you have const displayName=PersonalDetailsUtils.getDisplayNameOrDefault(details) the display name would be +355678899777 and that's not what we want, we want to display 067 889 9777.

Both my first option and the proposal from @Kalydosos use the formatPhoneNumber after we get the display name to format the number in the desired display format.

My Option 2. takes that a step further because as you just did in your thinking we assume that PersonalDetailsUtils.getDisplayNameOrDefault(details) will return a display name that can be used just like that to be displayed; apparantely it does not do what we think it does. We should change the internals of that fucntion so that if the displayName is a phone number we format it in there instead of moving that responsibility to whoever calls the function.

[jayeshmangwani]

Thanks for the info, @klajdipaja. After inviting the user to the workspace, we are displaying the formatted phone number for the user on the Members page. IMO, we should also show it the same way on the Details page.

App/src/pages/RoomMembersPage.tsx

Line 237 in f810ac6

text: formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(details)),

If that’s the case, then we should implement formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(details)) as @Kalydosos suggested in the proposal.

[Kalydosos]

@Kalydosos What difference will it make if we use formatPhoneNumber for PersonalDetailsUtils.getDisplayNameOrDefault(details)?

We can simply use PersonalDetailsUtils.getDisplayNameOrDefault(details) alone, and it will still work correctly. So, what is the purpose of using formatPhoneNumber here?

@jayeshmangwani using formatPhoneNumber is absolutely necessary because that's the only way to display the phone number in a local and user-friendly format and to maintain consistency with all the other screens as you can see in the images and videos below

it is used in the room page to reformat the invite phone number

reformating1.mp4

it is used in the room members list screen and when not used in the member detail page the display is inconsistent

withnoformating.mp4

just like in the issue demo video

so its usage is absolutely necessary. It is used in the members list screen here also (as you have pointed out indeed)

App/src/pages/RoomMembersPage.tsx

Line 237 in f810ac6

text: formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(details)),

[klajdipaja]

@jayeshmangwani I completely agree that we need to format the phone number and that @Kalydosos proposal is good for that.

What I would like to point out is that the RCA for this issue manifests in bugs in many places and we need to look at this specific bug in a more holistic approach.
We either need to replace formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault(details)) in all places where we don't take into account that getDisplayNameOrDefault results in a un-displayable phone number or we fix the internals of the getDisplayNameOrDefault function so that it does what the name of the functions says (Option 2 of my proposal)
Attached you can see two cases where we have almost the same issue as here by showing the improperly formatted phone number which then changes if you visit the profile of the new user.

Recoding 1. Invited a new user to the transaction thread. Notice how the phone number when you have contains the country code and once you visit the profile it does not anymore:

Screen.Recording.2024-11-27.at.10.59.50.AM.mov

Recording 2. Submit expense to a new user by phone number. Once the report is created notice all the displayed names include the country code as well and when you visit the profile everything changes.

Screen.Recording.2024-11-27.at.11.01.27.AM.mov

[abzokhattab]

adding the formatPhoneNumber is more of an implmentation detail as the main issue was that the expensify.sms is shown in the title not the format... thus i think this is more of an implementation details ...

if we decide to include the formatPhoneNumber this should be done in other places other well:

[jayeshmangwani]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• 2b. Reported on staging (eg. found during regression or PR testing)
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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: Make the members view of rooms and expense chats consistent with groups #47201 (comment)

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal

1. Open the app and go to a room chat.
2. Invite a phone number via suggestion, e.g., +919789945670.
3. Send the message.
4. From Whisper, tap Invite.
5. Tap the header — Member.
6. Open the phone number member.
7. Verify that expensify.sms is not shown with the phone number.

Do we agree 👍 or 👎

[Kalydosos]

no update on the payment. Is it because the ticket is closed ?

[jayeshmangwani]

No, but it seems that due to the holiday season, Garrett is OOO until December 30.

[Kalydosos]

Ok i see, thx for the info @jayeshmangwani 👍

[Kalydosos]

@garrettmknight please let me know if there is anything i must do for the payment process, thx

[luacmartins]

I'm not sure why the merge commit closed this issue, but we're still missing payment here.

[melvin-bot]

Current assignee @jayeshmangwani is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

📣 @Kalydosos 🎉 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 📖

[garrettmknight]

Payment Summary:

• Contributor: @Kalydosos $250 via Upwork - offer is out
• Reviewer: @jayeshmangwani $250 via NewDot

[Kalydosos]

@garrettmknight offer accepted, thanks a lot 👍

[Kalydosos]

@garrettmknight do you need anything from me to finalize the process on upwork ? please let me know, thx

[garrettmknight]

Nope! Just needed you to accept the offer. All paid up - @jayeshmangwani please request in ND when you're ready.

[JmillsExpensify]

Confirming payment summary:

• Contributor: @Kalydosos $250 via Upwork
• Reviewer: @jayeshmangwani $250 via New Expensify

[garrettmknight]

$250 approved for @jayeshmangwani