Add support for Phone mention without SMS domain @expensify.sms

[FitseTLT]

Fixed Issues

$ GH_LINK

Tests

1. What unit/integration tests cover your change? What autoQA tests cover your change?
2. What tests did you perform that validates your changed worked?

---

1. First on Expensify App change expensify-common dependency to point to latest commit of the current PR
   https://github.com/Expensify/App/blob/f438cb7901c4405e7ec2bbb22aaa904bb5b8fb66/package.json#L103-L104

    "expensify-common": "git+ssh://[email protected]/Expensify/expensify-common.git#562c315123a44c4386b85eb02c5b90b20db901fa",

2. run npm i
3. Change suggestion code in order to test so change
   https://github.com/Expensify/App/blob/f438cb7901c4405e7ec2bbb22aaa904bb5b8fb66/src/pages/home/report/ReportActionCompose/SuggestionMention.js#L166

                    login: Str.removeSMSDomain(detail.login),

2024-02-13.23-17-20.mp4

QA

1. What does QA need to do to validate your changes?
2. What areas to they need to test for regressions?

---

same

[FitseTLT]

@srikarparsi Don't worry about it. @c3024 and @jasperhuangg will handle the review

[c3024]

This appears to be working fine but I found a couple of issues that do not appear to be related this.

Backend sends errors when we send mentions with numbers which are not previously interacted with. This is working fine on main. Could you recheck all places where ExpensiMark is used to ensure nothing is broken? @FitseTLT

mentions1.mp4

1. For users (email ids and phone numbers) with whom the sender has not interacted, they can send email ids as mentions but backend returns an error if they send a phone number as a mention.

mentions2.mp4

2. For some numbers if I try to start a chat directly it says they are VoIP or Landline numbers. But if I send these numbers as mentions and click on them then a chat can be started.

Should they be fixed before proceeding with this? @jasperhuangg

[c3024]

Could you add unit tests for these changes in _tests_? @FitseTLT

[FitseTLT]

Could you add unit tests for these changes in _tests_? @FitseTLT

It was more proper to update the existing test, so updated!

[c3024]

Please refer to this as well. @FitseTLT

[FitseTLT]

@c3024 Yes You are right I did reproduce the error too but obviously it means there is a BE job also required in parallel to these changes of removing the sms domain in phone mentions.
Now if you mention a phone number contact that you haven't chatted before BE returns error

{
  "2629349569912594160": {
    "errors": {
      "1707933996120892": "Auth CreateReportAction returned an error"
    }
  }
}

cc: @jasperhuangg

[jasperhuangg]

Cool, I'll take a look at fixing that

[jasperhuangg]

@FitseTLT I investigated a back-end solution, and found that the proper solution should actually lie in the front-end.

Here, when we see that the mentioned user is a phone number (that doesn't have @expensify.sms), we should append @expensify.sms to the comment. This is because the back-end assumes that the text inside of the <mention-user> should be a valid user login. For SMS accounts, logins have to end with @expensify.sms.

Then, when we render the comment, we should omit the @expensify.sms from the comment and the user's account page.

Let me know if you have any additional questions, and sorry I didn't catch this sooner!

[jasperhuangg]

@FitseTLT I'm going to convert this to a draft while you work on it, let me know when it's ready for another review!

[FitseTLT]

@c3024 @jasperhuangg Ready for review

[c3024]

Did you check if the changes you made are working correctly for the issues discussed in the previous comments till date? @FitseTLT

[FitseTLT]

Did you check if the changes you made are working correctly for the issues discussed in the previous comments till date? @FitseTLT

Yep

[c3024]

Could you post a video of a user starting a chat with a new number?

[FitseTLT]

2024-02-22.18-55-24.mp4

[c3024]

From @jasperhuangg 's comment here

Then, when we render the comment, we should omit the @expensify.sms from the comment and the user's account page.

Let me know if you have any additional questions, and sorry I didn't catch this sooner!

@FitseTLT Can you see if you can omit the @expensify.sms from the report action, LHN subtitle and user's account page?

For report action this suggestion by @aim-salam in an earlier proposal for this issue appears to be useful.

props.tnode.init.textNode.data = props.tnode.init.textNode.data.split('@expensify')[0]

(formatPhoneNumber or removeSMSDomain is preferred instead of splitting like this)

for MentionUserRenderer.tsx here

[FitseTLT]

From @jasperhuangg 's comment here

Then, when we render the comment, we should omit the @expensify.sms from the comment and the user's account page.
Let me know if you have any additional questions, and sorry I didn't catch this sooner!

@FitseTLT Can you see if you can omit the @expensify.sms from the report action, LHN subtitle and user's account page?

For report action this suggestion by @aim-salam in an earlier proposal for this issue appears to be useful.

props.tnode.init.textNode.data = props.tnode.init.textNode.data.split('@expensify')[0]

(formatPhoneNumber or removeSMSDomain is preferred instead of splitting like this)

for MentionUserRenderer.tsx here

Yeah definitely! We we will deal with the removal of the sms domain on the main issue only focusing on the phone number mention parsing here 👍

[c3024]

LGTM

expensimark.mp4