[$250] Chat - Phone number mention in edit comment shows @expensify.sms

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.65-5
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4506240
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause_internal team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to chat.
3. Send a phone number mention.
4. Right click on the message > Edit comment.

Expected Result:

Phone number mention in edit comment will not show @expensify.sms.

Actual Result:

Phone number mention in edit comment shows @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

Bug6461260_1714031914746.bandicam_2024-04-25_15-56-40-903.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f67e599996d40e27
• Upwork Job ID: 1785006145906913280
• Last Price Increase: 2024-05-21
• Automatic offers:
• ahmedGaber93 | Reviewer | 102448681

Issue Owner

Current Issue Owner: @muttmuure

[melvin-bot]

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

[izarutskaya]

We think this issue might be related to the #vip-vsb.

[melvin-bot]

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

[melvin-bot]

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

[kaushiktd]

Proposal

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

Chat - Phone number mention in edit comment shows @expensify.sms

What is the root cause of that problem?

When a phone number is submitted or tagged, the ReportUtils.buildOptimisticAddCommentReportAction function is called here, Within this function, the getParsedComment function is invoked. Afterwards, the addDomainToShortMention function is called. This function checks whether the provided number is valid or not and then appends the @expensify.sms domain to it. Finally, the updated information is formatted in Parse Format to Onyx.

function named getParsedComment that parses inserted text using the Expensify-Common repository.This function should replace a given string with @expensify.sms as shown in the test case here.

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

You need to verify if the parsed message adheres to a valid phone number format. If it meets the criteria for a phone number, you should eliminate the @expensify.sms domain during editing. Please update the provided code accordingly here.

let message = parser.htmlToMarkdown(getActionHtml(reportAction));
const lastContentAfterAt = message.substring(message.lastIndexOf("@"));
if(lastContentAfterAt == CONST.SMS.DOMAIN) {
    const beforeLastAt = message.substring(0, message.lastIndexOf("@"));
    message = beforeLastAt;
}  

Report.saveReportActionDraft(reportID, reportAction, message);

Or, you need to update with expensify-common. If it replaces without @expensify.sms, it will update to optimistic data and will not be reproduced after the API request.

Video

https://app.screencastify.com/v3/watch/J0jFdmbffkxugYfgIxF2

[muttmuure]

@ahmedGaber93 proposal above to take a look at

[ahmedGaber93]

Reviewing today

[ahmedGaber93]

Still waiting for better proposals

[kaushiktd]

@ahmedGaber93 can you please help me learn the issue you are expecting in my proposal?

[ahmedGaber93]

@kaushiktd Thanks for the proposal.

Your root cause is incomplete, you talk about optimistic data, but this issue also happened with the real data, if you try to prevent adding @expensify.sms to the optimistic data, you will still able to reproduce the issue after the API request done and back with the real data.

Feel free to update your proposal if you have any updates.

[kaushiktd]

Thanks for your insights @ahmedGaber93.

Proposal Updated.

[melvin-bot]

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

[ahmedGaber93]

Or, you need to update with expensify-common. If it replaces without @expensify.sms, it will update to optimistic data and will not be reproduced after the API request.

@kaushiktd Can you explain with code how you will do this?

[kaushiktd]

@ahmedGaber93 This repo is externally included in the expensify-app, so any changes made to it will not be reflected on our app side. However, I'm pretty sure that when resolved from expensify-common, it will return a response like below. Therefore, please open a new issue on expensify-common.

<mention-user>@+19728974297</mention-user>

[ra-md]

Proposal

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

Phone number mention in edit comment shows SMS domain.

What is the root cause of that problem?

In here, we don't remove the sms domain.

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

We can use Str.removeSMSDomain to remove the SMS domain, and we also need to detect if the mention is a valid mention using Str.isValidMention.

function removeDomain(comment: string) {
   return comment.replace(/(?<=\d)@expensify\.sms/gi, "");
}

Use the function to remove sms domain here

Report.saveReportActionDraft(reportID, reportAction, removeDomain(parser.htmlToMarkdown(getActionHtml(reportAction))));

What alternative solutions did you explore? (Optional)

Adjust the regex in Str.removeSMSDomain to: /(?<=\d)@expensify\.sms/gi, so we can use Str.removeSMSDomain directly without creating another function.

Report.saveReportActionDraft(reportID, reportAction, Str.removeSMSDomain(parser.htmlToMarkdown(getActionHtml(reportAction))));

[ahmedGaber93]

please open a new issue on expensify-common.

@kaushiktd if this issue fix require changes in expensify-common, you can disscus it here in this issue as a part of your proposal, then if your proposal accepted you can open PR in expensify-common

[ahmedGaber93]

@ra-md Thanks for the proposal.
we use htmlToMarkdown in many places, and removing SMSDomain in every where we use it seems weird.
also your fix will failed if the mention is part of text like text before @mention text after, because the full text will return false in isValidMention

[kaushiktd]

@ahmedGaber93 I haven't tested with the exact code from the expensify-common repository because it's added as a dependency, and I can't update any code in expensify-common. However, I've identified that the root cause is generating from the parser called from the expensify-common repository. So either you can remove @expensify.sms when edit the comment, mention here or can modify to expensify-common repo.

[ahmedGaber93]

I can't update any code in expensify-common. However, I've identified that the root cause is generating from the parser called from the expensify-common repository.

@kaushiktd you can edit the package under node_modules folder, or download it locally and test it.

[kaushiktd]

@ahmedGaber93 I've checked with expensify-common. Now it will return same as expected like below:

<mention-user>@+917016113634</mention-user>

But API endpoint of AddComment returns response like Auth CreateReportAction returned an error.
So just I want to know that @expensify.sms domain is also validating on BE or not when submit comment.

[ahmedGaber93]

I've checked with expensify-common. Now it will return same as expected like below:

@kaushiktd how can I evaluate your fix without sharing your code changes? Please share your code changes 🙏

[muttmuure]

waiting on linked PR

[SzymczakJ]

PR was merged to expensify-common, tomorrow I'll try to bump it in App

[ahmedGaber93]

@SzymczakJ We already have PR that will bump it in App ☝️

[SzymczakJ]

nice!

[ahmedGaber93]

PR #44262 still in review.

[mvtglobally]

Issue is reproducible during KI retests.

1719763299836.bandicam_2024-06-30_19-01-12-658.mp4

[ahmedGaber93]

PR #44262 is still in review

[ahmedGaber93]

PR #44262 is merged into main but not deployed to staging yet. Tested main locally and works well.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[ahmedGaber93]

The above issue ☝️ is related to another issue that fix with in the same PR #44262 and not related to our PR

[ahmedGaber93]

@muttmuure This issue was deployed to production 8 days ago. We can pay out and close it now.

Payment summary:

• @ahmedGaber93 requires payment (C+)
• @SzymczakJ does not require payment (Contractor)
• @truph01 ask for split contributor bounty here

[ahmedGaber93]

@muttmuure bump for payment.

[muttmuure]

Handling!

[muttmuure]

@ahmedGaber93 $250
@truph01 $125

[muttmuure]

@truph01 I've invited you to the job here: https://www.upwork.com/jobs/~0132841b006b8ee44f

[truph01]

@muttmuure I applied thanks

[melvin-bot]

@flodnv, @ahmedGaber93, @muttmuure, @SzymczakJ Whoops! This issue is 2 days overdue. Let's get this updated quick!

[muttmuure]

Invited

[truph01]

@muttmuure Thanks I accepted it

[muttmuure]

Paid