[HOLD for payment 2023-08-02] [$1000] In InviteMembers page when we search 998243aaaa we can invite +529982432222

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to Invite Members page in workspace or regular search (Chat switcher)
2. Enter 998243aaaa

Expected Result:

This should show No results found via search on the chat switcher, Invite new member search, or anywhere else

Actual Result:

In the list we can invite +529982432222

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number:
Reproducible in staging?: Needs reproduction
Reproducible in production?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:
Issue reported by: @ShogunFire
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1688428528032189

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01183b41848a55f578
• Upwork Job ID: 1678520668240605184
• Last Price Increase: 2023-07-17

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[GItGudRatio]

Proposal

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

We can see that when we try to search for a user using just text, a phone number shows up, which can be confusing.

What is the root cause of that problem?

To easily reproduce this problem, you just need to type one digit with 9 alphabets, which will cause a user to show up. For example, 1expensify. The reason why this happens is because of the way the library awesome-phonenumber works. When typing alphabets, it maps the corresponding alphabet to which number it would relate to on a keypad. This is the standard e164 format to use alphabets in place of numbers. Due to this, the text 1expensify is interpreted as +{countryCode}1397367439.

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

If we want to suppress this behaviour and ensure that No results found shows up instead, we need to add a conditional check here:

App/src/libs/OptionsListUtils.js

Line 778 in 60f4817

((Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue) && !Str.endsWith(searchValue, CONST.SMS.DOMAIN)) || parsedPhoneNumber.possible) &&

This check will be after the || operator, adding a check such that in addition to checking if the parsed phone number is possible, we also check if the input provided is of the format digits and plus by using the condition:
CONST.REGEX.DIGITS_AND_PLUS.test(parsedPhoneNumber.number.input).

What alternative solutions did you explore? (Optional)

N/A

[MitchExpensify]

This exists in regular search too from what I can see

[MitchExpensify]

[MitchExpensify]

Also reproduced on Invite new members:

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[ShogunFire]

Proposal

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

When we search a string beginning with numbers but also conatining letters sometimes they are recognized as phone numbers

What is the root cause of that problem?

It happens because of the way the library awesome-phonenumber works.

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

I propose that we make a new function that has the same goal as parsePhoneNumber and return the same thing but add a condition that the search value must be composed of numbers and + , we can then call this function everywhere where we are currently calling parsePhoneNumber

What alternative solutions did you explore? (Optional)

[ShogunFire]

PS: I know my proposal is quite similar to the existing one but this way we can resolve the problem everywhere. Also for my defense I reported this bug and had a proposal ready but I didn't know that I could send my proposal in the bug report.

[abdulrahuman5196]

Will review this in my morning

[getusha]

Not able to reproduce

[ShogunFire]

Hey sorry the steps have changed a bit

[getusha]

@ShogunFire can you share the steps?

[ShogunFire]

It depends of what country you are in and what is a valid number there, I think for the us the first number should be 1. Otherwise I think that you can put the international code and still reproduce. For example try with +52998243aaaa

[getusha]

@ShogunFire thank you i am able to reproduce it now.

[getusha]

Proposal

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

phone number with letters will show a phone number

What is the root cause of that problem?

letters in phone numbers are considered as possible from awesome-phonenumber library then we're converting it to numbers here if it is possible:

App/src/libs/OptionsListUtils.js

Line 637 in 60e73fa

const searchValue = parsedPhoneNumber.possible ? parsedPhoneNumber.number.e164 : searchInputValue.toLowerCase();

since it's recognized as possible it will convert it to numbers format. but previously it was decided not to be a valid phone number here we have fixed similar issue #21247

Maybe, but we don't translate +1-212-456ww7890 into +1-212-456997890. Either way, that's incorrect!
So there are two potential solutions: reject non-numeric phone numbers (ie: show an error when you enter +1-212-456ww7890) or translate the letters into numbers (ie: translate +1-212-456ww7890 into +1-212-456997890`.

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

We should not convert the original value to make the results consistent across all forms, then we should validate the phone number using Str.isPhoneValid by removing + from the value.

we should add this condition to validate the phone number further

(parsedPhoneNumber.possible && Str.isValidPhone(searchValue.slice(1))

here:

App/src/libs/OptionsListUtils.js

Line 789 in 60e73fa

((Str.isValidEmail(searchValue) && !Str.isDomainEmail(searchValue) && !Str.endsWith(searchValue, CONST.SMS.DOMAIN)) || parsedPhoneNumber.possible) &&

Result:

What alternative solutions did you explore? (Optional)

[MitchExpensify]

How does the above proposal look to you @abdulrahuman5196 ?

[GItGudRatio]

Thank you very much for your fair decision folks, I appreciate it a lot!

Really sorry for the delay, had a family issue 😓

@abdulrahuman5196 @amyevans PR is up, please help review!

[abdulrahuman5196]

Really sorry for the delay, had a family issue

No worries. Hope everything is fine. @GItGudRatio

[GItGudRatio]

Yes, it's better now. Thank you!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.45-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix text based mobile search on InviteMembers page #23366

If no regressions arise, payment will be issued on 2023-08-02. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @abdulrahuman5196 requires payment
• @GItGudRatio requires payment

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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:

• [@abdulrahuman5196] The PR that introduced the bug has been identified. Link to the PR:
• [@abdulrahuman5196] 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:
• [@abdulrahuman5196] 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:
• [@abdulrahuman5196] Determine if we should create a regression test for this bug.
• [@abdulrahuman5196] 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.
• [@MitchExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[MitchExpensify]

Reminder added to calendar for eventual payment. Let's square off the BZ checklist above in the meantime @abdulrahuman5196 ! 🚀

[abdulrahuman5196]

The PR that introduced the bug has been identified. Link to the PR:
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:
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:

This wasn't a regression. It should have been present for sometime but we only noticed the issue now.

Determine if we should create a regression test for this bug.
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.

No. We have already added UTs for this case. So regression test won't be required here.

@MitchExpensify

[MitchExpensify]

Awesome, thanks @abdulrahuman5196

[melvin-bot]

@amyevans, @MitchExpensify, @abdulrahuman5196, @GItGudRatio Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[amyevans]

I think @MitchExpensify is OOO, but bump for Monday 😄

[MitchExpensify]

Payment Summary:

Reporting: @ShogunFire $250 (Upwork) - Mind linking your Upwork profile?
C: @GItGudRatio $1500 (Upwork) - Mind linking your Upwork profile?
C+: @abdulrahuman5196 @1500 (Upwork) Paid

[ShogunFire]

https://www.upwork.com/freelancers/pierremichel5

[GItGudRatio]

@MitchExpensify - https://www.upwork.com/freelancers/~01ab4a74183ae4b88a

[MitchExpensify]

Great! Thank you both

@GItGudRatio paid and contract ended
@ShogunFire offer sent!

[MitchExpensify]

All paid! Thanks everyone