Android - Thread-Mention phone number on reply thread shows expensify.sms in header

[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: 1.4.58 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. Launch app
2. Tap on phone contact report
3. Enter @ followed by phone number in compose
4. Send the message
5. Long press and select reply in thread

Expected Result:

@ followed by phone number should be displayed in header on selecting reply in thread

Actual Result:

@ followed by phone number should be displayed in header on selecting reply in thread but header displays @ followed by phone number.expensify.sms

Workaround:

Unknown

Platforms:

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

• [x] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/2fa22ee8-e53a-4b28-a4f1-826307d70485

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[ahmedGaber93]

Proposal

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

Android - Thread-Mention phone number on reply thread shows expensify.sms in header

What is the root cause of that problem?

in getDisplayNameOrDefault when we get the user details display Name, we removeSMSDomain only in the below case. https://github.com/Expensify/App/blob/edacb67735b14cfb484352a7267c7804ab5e9fe8/src/libs/PersonalDetailsUtils.ts#L37-L41

in this issue case the personalDetails for this user not have login and displayName set by backend [email protected]

{
  "accountID": 16574166,
  "avatar": "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_7.png",
  "displayName": "[email protected]",
  "firstName": "",
  "lastName": ""
}

So the removeSMSDomain not applied for this case.

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

we need to update the condition here to be

// if (displayName === passedPersonalDetails?.login && Str.isSMSLogin(passedPersonalDetails?.login)) {
if (
    (displayName === passedPersonalDetails?.login && Str.isSMSLogin(passedPersonalDetails?.login)) ||
    (!passedPersonalDetails?.login && Str.isSMSLogin(displayName)) // <<<< this issue case
)

This will fix the issue in all places, Header, LHN, welcome message and profile details (if you click on the header)

What alternative solutions did you explore? (Optional)

we can also remove the condition to apply Str.removeSMSDomain in all cases like what we do in MentionUserRenderer here

// if (displayName === passedPersonalDetails?.login && Str.isSMSLogin(passedPersonalDetails?.login)) { 
     displayName = Str.removeSMSDomain(displayName); 
// } 

[nkdengineer]

Proposal

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

• Android - Thread-Mention phone number on reply thread shows expensify.sms in header

What is the root cause of that problem?

• We get the report name from here. In case of this bug, parentReportAction.message[0].text is something like [email protected] if parent message is @+123456789

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

• We should update the above to:

        const parentReportActionMessage = (
            ReportActionsUtils.isApprovedOrSubmittedReportAction(parentReportAction)
                ? ReportActionsUtils.getReportActionMessageText(parentReportAction)
                : Str.removeSMSDomain(parentReportAction?.message?.[0]?.text ?? '')

What alternative solutions did you explore? (Optional)

• The main solution does not consider the case, user types "[email protected]" directly. In this case, we should not remove "@expensify.sms".

  The solution is:

• Assume that user send a message @+123456789 What are you doing?
• The reportAction.message will be:

{
    "html": "<mention-user>@[email protected]</mention-user> What are you doing?",
    "text": "@[email protected] What are you doing?",
    "type": "COMMENT"
}

• We will check all the <mention-user> tags in html and remove all the sms domains. After this step, html will be:

<mention-user>@+123456789</mention-user> What are you doing?

• Finally, we convert the above html to text, and use this text as report name instead of using parentReportAction?.message?.[0]?.text ?? '' in here, it will be:

@+123456789 What are you doing?

• Here is the implementation of the above idea: We just need to update this to:

const parser = new ExpensiMark();
        const parentReportActionMessage = (
            ReportActionsUtils.isApprovedOrSubmittedReportAction(parentReportAction)
                ? ReportActionsUtils.getReportActionMessageText(parentReportAction)
                : parser.htmlToText(parentReportAction?.message?.[0]?.html ?? '').replace(/(<mention-user>)(.*?)(<\/mention-user>)/gi, function(match, openTag, content, closeTag) {
                    content = Str.removeSMSDomain(content);  
                    return openTag + content + closeTag;
                })
        ).replace(/(\r\n|\n|\r)/gm, ' ');

In the above:

• <mention-user>)(.*?)(<\/mention-user> is the regex matching all the text inside mention-user tag.
• We can consider applying this change to other positions, such as LHN, ...

[hayes102]

I am not sure if this is actually a bug. seems like a minor issue. cc @mountiny

[melvin-bot[bot]]

📣 @hayes102! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[anmurali]

Ah I think it is worth fixing cause that format +@[email protected] is internal and means nothing to the end user and could confuse them

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[mountiny]

Agreed with Anu

[rayane-d]

Reviewing proposals...

[rayane-d]

@ahmedGaber93 The root cause in your proposal is incorrect; we don't use PersonalDetailsUtils.getDisplayNameOrDefault when determining a report's name. I also tried your suggested changes, but they didn't fix the bug.

[rayane-d]

@nkdengineer, you've identified the correct root cause in your proposal and suggested changing LGTM. It uses the same approach as we are doing with the last message text in LHN subtitles here. I prefer your main solution over the alternative, as currently, when the user types @[email protected] directly in a message, we remove the @expensify.com part, as shown in the video below. Additionally, the intention in this issue is to hide the format @expensify.sms in UI everywhere as it is internal.

https://github.com/Expensify/App/assets/77965000/fb3e45dc-4e26-4de5-8091-303ec50e2100

@nkdengineer's proposal LGTM! 🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

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

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot[bot]]

📣 @rayane-djouah 🎉 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[bot]]

📣 @nkdengineer You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

[rayane-d]

@nkdengineer please let us know your ETA

[nkdengineer]

@rayane-djouah PR https://github.com/Expensify/App/pull/39994 is ready to review

[rayane-d]

@nkdengineer we agreed on the main solution in your proposal https://github.com/Expensify/App/issues/39305#issuecomment-2042786695, didn't we? I see that the PR is implementing the alternative one.

[nkdengineer]

As I mentioned in the PR, we need to consider the case: User sends message "test message [email protected]" directly, If we apply the main solution, it will be "test message +123456789". But I think the expected is "test message [email protected]"

[rayane-d]

@anmurali, @thienlnam, we need clarification about the expected result when the user sends a message that contains "@expensify.sms" (not necessarily a mention); as seen in the screenshot below, currently, we remove all "@expensify.sms" occurrences on the message when displaying the last message of the report in the LHN subtitle.

In this issue, should we:

1. Follow the same approach used in LHN subtitles and remove any "@expensify.sms" occurrences when displaying the message in the thread header.
2. Remove it only from mentions. In this case, do we need to change the logic of LHN subtitles to follow the same approach for consistency?

[rayane-d]

Asked in Slack

[rayane-d]

@nkdengineer, we got an answer in Slack; we should implement your main solution.

[rayane-d]

Deployed to production https://github.com/Expensify/App/pull/39994#issuecomment-2069779658

[rayane-d]

@anmurali, please remove the Reviewing label and bump to Daily as payment is due

[rayane-d]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [x] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/37559
• [x] 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/37559#issuecomment-2093478782
• [x] 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.
• [x] Determine if we should create a regression test for this bug. Yes.
• [x] 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

We should update this tests to include:

1. Go to any room and type @
2. Select a user with phone number login
3. Send the message
4. Long press and select reply in thread
5. Verify that @ followed by phone number (eg: @+123456789) should be displayed in thread header without an @expensify.sms suffix.

• Do we agree 👍 or 👎

[rayane-d]

@anmurali friendly bump for payment

[anmurali]

Done!

[nkdengineer]

Sorry @anmurali It seems I wasn't paid here, could you please reopen the issue to help with this?

TIA