[$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
• [x] Android: HybridApp
• [ ] Android: mWeb Chrome
• [ ] iOS: Standalone
• [ ] iOS: HybridApp
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/e2360e07-08ee-49cd-813d-eaa7078cf2d3

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

Issue Owner

Current Issue Owner: @Kalydosos

[melvin-bot[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: https://github.com/Expensify/App/blob/93c573b1baeadd54460fcb4492b4aa2e280f844d/src/pages/RoomMemberDetailsPage.tsx#L50

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

https://github.com/Expensify/App/blob/93c573b1baeadd54460fcb4492b4aa2e280f844d/src/pages/RoomMemberDetailsPage.tsx#L50

into

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

RESULT

it works fine

What alternative solutions did you explore? (Optional)

None

[melvin-bot[bot]]

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

[melvin-bot[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: https://github.com/Expensify/App/blob/93c573b1baeadd54460fcb4492b4aa2e280f844d/src/pages/RoomMemberDetailsPage.tsx#L50

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: https://github.com/Expensify/App/blob/084a711842d6be379b18491ebe2cc6b48280006e/src/libs/PersonalDetailsUtils.ts#L63 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 https://github.com/Expensify/App/issues/53003#issuecomment-2500478874 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 https://github.com/Expensify/App/blob/c7c76907f1f948a9be47df34452c292bdb323b71/src/libs/LocalePhoneNumber.ts#L25-L28

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 https://github.com/Expensify/App/issues/53003#issuecomment-2500478874. 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 https://github.com/Expensify/App/issues/53003#issuecomment-2495487069, 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. https://github.com/Expensify/App/blob/f810ac6d4eb6ad0f70cae37738d1e23d5b0a1585/src/pages/RoomMembersPage.tsx#L237

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

https://github.com/user-attachments/assets/38ce9763-a78b-42a4-9c48-d17e56b9be0e

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

https://github.com/user-attachments/assets/00dfe7b0-ea07-45af-ba96-99687dbea6fd

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) https://github.com/Expensify/App/blob/f810ac6d4eb6ad0f70cae37738d1e23d5b0a1585/src/pages/RoomMembersPage.tsx#L237

[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:

https://github.com/user-attachments/assets/c0a06582-cd08-458c-b611-fb06c9becf72

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.

https://github.com/user-attachments/assets/9267730c-6d56-47df-8cce-f84d353ed125

[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:

[Kalydosos]

@jayeshmangwani in the very specific case, dealing with phone number and for the very screens involved we need to format the phone number and maintain consistency with the same contextual screens. Maybe there are some other screens elsewhere needing that formatting but we should not adress all these issues here in this ticket. It may seems simple to adress them but the reality is that each screen has its context and purpose and reformatting can be or not relevant depending on that context but in this very case, reformatting is more than necessary to maintain consistency imho

[jayeshmangwani]

Thank you all for your respective proposals: @abzokhattab , @Kalydosos and @klajdipaja

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 ...

@abzokhattab your point is definitely valid that this can be handled in the PR, and we would catch inconsistencies there. However, what I personally think is that we should go with @Kalydosos proposal in this specific case, as the change is very small regarding how we should display the username (fallback name). Additionally, they are suggesting applying the same logic to the Member Details page as we do on the Members page. The rest, I can leave to the internal engineer if you feel otherwise.

[jayeshmangwani]

I agree with @Kalydosos's Proposal. We can use formatPhoneNumber(PersonalDetailsUtils.getDisplayNameOrDefault()) for the displayName value.

Also, there are a few pages where we are using PersonalDetailsUtils.getDisplayNameOrDefault() only as the displayName. We would need to fix those inconsistencies as part of this issue.

🎀 👀 🎀 C+ reviewed

[melvin-bot[bot]]

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

[melvin-bot[bot]]

❌ There was an error making the offer to @Kalydosos for the Contributor role. The BZ member will need to manually hire the contributor.

[Kalydosos]

@jayeshmangwani i think it will be difficult to resolve the other non use of formatPhoneNumber here in this ticket as we need to measure any unexpected impact that could have on the context and the purpose of these pages/screens. And already juding by the glimpse on the refactoring in this image https://github.com/Expensify/App/issues/53003#issuecomment-2503519015 i can tell it can be simple or not as we should do an impact check for each screen. I advise us to just stick to what is known as broken (the issue in this ticket) as we can have unexpected impact while conforming other screens imho. My PR is ready for this issue if we go in my advised direction otherwise the refactoring of the whole screens can take some considerable time and this fix could take also some time to be delivered in production. Another way is to have this delivered quickly and create a refactoring ticket that could take the necessary time to be ready imho

[jayeshmangwani]

@Kalydosos I don’t think it will take a lot of time. We can handle those in issue only, as I believe it’s just a UI confirmation, and we can address it on a case-by-case basis. @luacmartins , what do you think?

[abzokhattab]

but wait which format do we want ... i guess we might need to ask the design team

claiming that this is a user-friendly format is not correct

the new format shown here hides the country code in most cases, so why do you think that is correct? as a user i would be confused 🤔

[luacmartins]

@Kalydosos I don’t think it will take a lot of time. We can handle those in issue only, as I believe it’s just a UI confirmation, and we can address it on a case-by-case basis. @luacmartins , what do you think?

I agree

[Kalydosos]

@jayeshmangwani @luacmartins ok for me then as we all agreed :+1:

[jayeshmangwani]

the new format shown https://github.com/Expensify/App/issues/53003#issuecomment-2495946266 hides the country code in most cases, so why do you think that is correct? as a user i would be confused 🤔

This will hide the country code if it's in the same region only