Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD PR #17578] Feature Request: Search using Formatted phone numbers like (123) 456-7890 #17255

Closed
rushatgabhane opened this issue Apr 11, 2023 · 21 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Reviewing Has a PR in review

Comments

@rushatgabhane
Copy link
Member

rushatgabhane commented Apr 11, 2023

Coming from #16099 (comment)

Problem

When you copy phone numbers from your phone book they have a format (eg: +1 (123) 456-7890, +43 123 45 67 89). Pasting them in our search gives no results :/

image

Solution

Accept formatted numbers in our search component.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016055d98aeee1fd52
  • Upwork Job ID: 1645810898736164864
  • Last Price Increase: 2023-04-11
@rushatgabhane rushatgabhane added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Apr 11, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@rushatgabhane
Copy link
Member Author

cc: @puneetlath in case you wanna assign this to yourself

@puneetlath
Copy link
Contributor

Oh thanks @rushatgabhane. I'll take it. Do you want to C+ it?

@rushatgabhane
Copy link
Member Author

rushatgabhane commented Apr 11, 2023

sure, much appreciated!

@rushatgabhane
Copy link
Member Author

@puneetlath do you think problem 2 is a real problem? Should I raise it on slack for feedback?

@puneetlath
Copy link
Contributor

Yeah I'm not convinced about problem 2. But we should definitely solve problem 1.

@rushatgabhane
Copy link
Member Author

After some disucssion on slack, I've removed problem 2 from OP.

@koko57
Copy link
Contributor

koko57 commented Apr 11, 2023

Hi I'm Agata from Callstack - expert contributor group - I'll take this one :)

@puneetlath puneetlath added the Internal Requires API changes or must be handled by Expensify staff label Apr 11, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

Current assignee @rushatgabhane is eligible for the Internal assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added the Overdue label Apr 14, 2023
@puneetlath puneetlath added the Reviewing Has a PR in review label Apr 14, 2023
@melvin-bot melvin-bot bot removed the Overdue label Apr 14, 2023
@puneetlath
Copy link
Contributor

Review is ongoing. We're getting close to merging.

@rushatgabhane
Copy link
Member Author

@puneetlath @koko57 could you please tag me in the PR? I wasn't requested a review. Thanks!

@rushatgabhane
Copy link
Member Author

small discussion on what to do with invalid numbers https://expensify.slack.com/archives/C01GTK53T8Q/p1681599809454409

@s77rt
Copy link
Contributor

s77rt commented Apr 16, 2023

Not a Proposal

What is the root cause of that problem?

E/App already support such formatting, something like +1 (534) 543 2154 works but +43 123 45 67 89 does not this is due to our regex failure, the regex seems meant for US numbers only

The regex fails here

const isPhoneNumber = CONST.REGEX.PHONE_WITH_SPECIAL_CHARS.test(searchInputValue);
const searchValue = isPhoneNumber ? searchInputValue.replace(CONST.REGEX.NON_NUMERIC_WITH_PLUS, '') : searchInputValue;

We usually strip the input from anything that is not a digit or a plus symbol to match the e164 format but since the regex fail we take the input as is

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

  1. Since we are going to use awesome-phonenumber then we can remove that regex and use the lib instead:
    const parsedPhoneNumber = parsePhoneNumber(LoginUtils.appendCountryCode(searchInputValue));
    const searchValue = parsedPhoneNumber.valid ? parsedPhoneNumber.number.e164 : searchInputValue;
  1. Update appendCountryCode to be validation free:
    return phone.startsWith('+') ? phone : `+${countryCodeByIP}${phone}`;

What alternative solutions did you explore? (Optional)

None

@s77rt
Copy link
Contributor

s77rt commented Apr 19, 2023

I think we are going to do this as a part of #17578.

@koko57
Copy link
Contributor

koko57 commented Apr 19, 2023

@rushatgabhane @puneetlath so it'll be on hold now?

@puneetlath
Copy link
Contributor

Ah interesting, yes. It looks like that PR will naturally handle this. I'll put this on hold and we can double-check after that is merged whether this is still needed.

@puneetlath puneetlath changed the title Feature Request: Search using Formatted phone numbers like (123) 456-7890 [HOLD PR #17578] Feature Request: Search using Formatted phone numbers like (123) 456-7890 Apr 19, 2023
@puneetlath puneetlath added Weekly KSv2 and removed Daily KSv2 labels Apr 19, 2023
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels May 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 12, 2023

This issue has not been updated in over 15 days. @puneetlath, @koko57, @rushatgabhane eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@s77rt
Copy link
Contributor

s77rt commented May 12, 2023

I think we can close this. It's handled already.

@puneetlath
Copy link
Contributor

Ah yep. This is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

6 participants