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

[$500] Invalid phone number is accepted when starting a new chat #32242

Closed
1 of 6 tasks
m-natarajan opened this issue Nov 30, 2023 · 17 comments
Closed
1 of 6 tasks

[$500] Invalid phone number is accepted when starting a new chat #32242

m-natarajan opened this issue Nov 30, 2023 · 17 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@m-natarajan
Copy link

m-natarajan commented Nov 30, 2023

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.5-1
Reproducible in staging?: y
Reproducible in production?: y
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
Expensify/Expensify Issue URL:
Issue reported by: @MariaHCD
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1701265640069269

Action Performed:

  1. Start new chat
  2. Enter +112345678900
  3. Observe this invalid phone number is parsed into a valid number

Expected Result:

Invalid number should be detected

Actual Result:

The number is parsed into a valid phone number

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

Add any screenshot/video evidence

New Expensify - Google Chrome (2)

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b8deeaa5835d9589
  • Upwork Job ID: 1730299555744174080
  • Last Price Increase: 2023-11-30
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 30, 2023
Copy link

melvin-bot bot commented Nov 30, 2023

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

Copy link

melvin-bot bot commented Nov 30, 2023

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

@devstar1014
Copy link

devstar1014 commented Nov 30, 2023

Proposal

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

Invalid phone number is accepted when starting a new chat

What is the root cause of that problem?

Now, our project is using parsePhoneNumber function of awesome-phonenumber package which has some bug.
It sometimes accept invalid phone number as valid so we should add custom checking code after validation.
When parse number, this function convert input string to e164 type in module.

const parsedPhoneNumber = parsePhoneNumber(LoginUtils.appendCountryCode(Str.removeSMSDomain(searchInputValue)));

Because of it, some invalid input string is changed to valid string.
This occurs about only invalid string.
You can know about it If you check following photo.
Screenshot_2

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

If invalid string is entered, its input and e164 string is different, while valid string is entered, its input and e164 string is same.
so we can just add comparison code between input and e164 strings.

In OptionsListUtils.js 1132 Line
From

 const parsedPhoneNumber = parsePhoneNumber(LoginUtils.appendCountryCode(Str.removeSMSDomain(searchInputValue)));
    const searchValue = parsedPhoneNumber.possible ? parsedPhoneNumber.number.e164 : searchInputValue.toLowerCase();

To

 const parsedPhoneNumber = parsePhoneNumber(LoginUtils.appendCountryCode(Str.removeSMSDomain(searchInputValue)));
 const searchValue = parsedPhoneNumber.possible ? parsedPhoneNumber.number.e164 : searchInputValue.toLowerCase();
if(parsedPhoneNumber.possible){
     parsedPhoneNumber.possible = parsedPhoneNumber.number.input == parsedPhoneNumber.number.e164
}

This is result.
Untitled

I checked these results in native app and chrome. As a result it works well now.

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@NicMendonca NicMendonca added the External Added to denote the issue can be worked on by a contributor label Nov 30, 2023
@melvin-bot melvin-bot bot changed the title Invalid phone number is accepted when starting a new chat [$500] Invalid phone number is accepted when starting a new chat Nov 30, 2023
Copy link

melvin-bot bot commented Nov 30, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 30, 2023
Copy link

melvin-bot bot commented Nov 30, 2023

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

@kaushiktd
Copy link
Contributor

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

Invalid phone number is accepted when starting a new chat

What is the root cause of that problem?

the country code 1 is removed from the number +112345678900 resulting in +12345678900, might occur due to how the country code addition and formatting are handled in the context of the awesome-phonenumber library.

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

https://github.com/Expensify/App/blob/main/src/libs/OptionsListUtils.js#L1623
in

getHeaderMessage()
const phraseNumber = parsePhoneNumber(LoginUtils.appendCountryCode(searchValue));

if(phraseNumber.number.input !== phraseNumber.number.e164){
        return Localize.translate(preferredLocale, 'messages.errorMessageInvalidPhone');
    }

this will set the error if the input and e164 value is not same which solves the problem of not showing error message

also to hide the popup we need to wrap in condition,
https://github.com/Expensify/App/blob/main/src/libs/OptionsListUtils.js#L1330

  if(parsedPhoneNumber.number.input == parsedPhoneNumber.number.e164){  
       
        // Generates an optimistic account ID for new users not yet saved in Onyx
        const optimisticAccountID = UserUtils.generateAccountID(searchValue);
        const personalDetailsExtended = {
            ...personalDetails,
            [optimisticAccountID]: {
                accountID: optimisticAccountID,
                login: searchValue,
                avatar: UserUtils.getDefaultAvatar(optimisticAccountID),
            },
        };
     //Rest of code .....
    }

Video:

https://drive.google.com/file/d/1FcEDYvB6-CxJ7WZsStJkBJ9FKr0zIFTz/view?usp=drive_link

@melvin-bot melvin-bot bot added the Overdue label Dec 4, 2023
@situchan

This comment was marked as off-topic.

@situchan

This comment was marked as off-topic.

@akinwale
Copy link
Contributor

akinwale commented Dec 5, 2023

Taking over as C+ here.

The existing proposals do not adequately address the issue. Looking for more proposals.

@erichasinternet
Copy link

Proposal

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

An invalid phone number is accepted when starting a new chat.

What is the root cause of that problem?

The core issue at hand stems from the functionality of the parsePhoneNumber method. When the input +112345678900 is processed by this method, it produces the following result:

{
    "number": {
        "input": "+112345678900",
        "international": "+1 234-567-8900",
        "national": "(234) 567-8900",
        "e164": "+12345678900",
        "rfc3966": "tel:+1-234-567-8900",
        "significant": "2345678900"
    },
    "regionCode": "US",
    "valid": true,
    "possible": true,
    "canBeInternationallyDialled": true,
    "possibility": "is-possible",
    "type": "fixed-line-or-mobile",
    "typeIsMobile": true,
    "typeIsFixedLine": true,
    "countryCode": 1
}

While the number.input value appears to be invalid due to the presence of an additional '1', the e164 value eliminates this extraneous digit, resulting in the representation of a valid phone number.

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

We can improve the existing code by adding an extra check, comparing the e164 and input values for equality.

    const parsedPhoneNumber = parsePhoneNumber(LoginUtils.appendCountryCode(searchValue));
    const isValidPhone = parsedPhoneNumber.possible && isValidPhone.number.e164 === isValidPhone.number.input;

We'll also need to add the e164 and input comparison here to prevent both an account and an error message from being returned simultaneously.

Bug-fix demo:

Screenshot of expected result:
Screenshot 2023-12-05 at 1 03 55 PM

Video showing expected behavior:

Screen.Recording.2023-12-05.at.1.11.20.PM.mov

Copy link

melvin-bot bot commented Dec 5, 2023

📣 @erichasinternet! 📣
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.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@erichasinternet
Copy link

Contributor details
Your Expensify account email: elawson545@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01bb13350fc87c5468

Copy link

melvin-bot bot commented Dec 5, 2023

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@FitseTLT
Copy link
Contributor

FitseTLT commented Dec 5, 2023

This will be fixed by #31726

Copy link

melvin-bot bot commented Dec 5, 2023

@ntdiary, @NicMendonca Huh... This is 4 days overdue. Who can take care of this?

@MariaHCD
Copy link
Contributor

MariaHCD commented Dec 6, 2023

Seems like this one might be a dupe of #28492

Thoughts, @akinwale?

@akinwale
Copy link
Contributor

akinwale commented Dec 6, 2023

Seems like this one might be a dupe of #28492

Thoughts, @akinwale?

Yeah, looks like it. We can close this one.

@MariaHCD MariaHCD closed this as completed Dec 6, 2023
@melvin-bot melvin-bot bot removed the Overdue label Dec 6, 2023
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. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

10 participants