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

[$250] Improve Phone Number Entry Validation and Error Messaging for Users #55337

Open
1 of 8 tasks
m-natarajan opened this issue Jan 16, 2025 · 26 comments
Open
1 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Jan 16, 2025

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


Version Number: 9.0.86-1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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: @quinthar
Slack conversation (hyperlinked to channel name): convert

Action Performed:

  1. Go to the phone number input field.
  2. Attempt to enter a phone number without a + or using typical formatting like parentheses or dashes.
  3. Note that the system rejects the input and displays the error message.

Expected Result:

The system should accept phone numbers with or without the + prefix and ignore non-numeric characters during validation.
The error message should be clear and provide an example of a correctly formatted phone number.
Update the error message to:
"Please enter a complete phone number (e.g., +1-(201)-867-5309)."
This provides a clearer example of the expected format.

Actual Result:

The system requires a strict format starting with + and rejects numbers with non-numeric characters.
The error message shown is vague and unhelpful: "Please enter a valid phone number."

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: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Image

Image

Image

Image

Image

Image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021880325640829343919
  • Upwork Job ID: 1880325640829343919
  • Last Price Increase: 2025-01-17
  • Automatic offers:
    • suneox | Reviewer | 105808946
    • huult | Contributor | 105808948
Issue OwnerCurrent Issue Owner: @suneox
@m-natarajan m-natarajan added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Improvement Item broken or needs improvement. labels Jan 16, 2025
Copy link

melvin-bot bot commented Jan 16, 2025

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

@huult
Copy link
Contributor

huult commented Jan 16, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-01-16 04:29:02 UTC.

Proposal

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

Improve Phone Number Entry Validation and Error Messaging for Users

What is the root cause of that problem?

Improvement

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

we should update validate function like this:

  1. The system should accept phone numbers with or without the + prefix and ignore non-numeric characters during validation.

const validate = useCallback(
    (
        values: FormOnyxValues<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM>
    ): FormInputErrors<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM> => {
        const errors: FormInputErrors<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM> = {};

        // Validate that the phone number field is not empty
        if (!ValidationUtils.isRequiredFulfilled(values[INPUT_IDS.PHONE_NUMBER])) {
            errors[INPUT_IDS.PHONE_NUMBER] = translate('common.error.fieldRequired');
            return errors; // Early return if field is empty
        }

        try {
            // Sanitize input: Remove all non-numeric characters except the leading '+'
            const sanitizedPhoneNumber = values[INPUT_IDS.PHONE_NUMBER].replace(/[^+\d]/g, '');

            // Append country code if missing
            const phoneNumberWithCountryCode = LoginUtils.appendCountryCode(sanitizedPhoneNumber);

            // Parse and validate the phone number
            const parsedPhoneNumber = PhoneNumberUtils.parsePhoneNumber(phoneNumberWithCountryCode);

            // Check if the phone number is possible and valid in E.164 format
            if (!parsedPhoneNumber.possible || !Str.isValidE164Phone(phoneNumberWithCountryCode)) {
                errors[INPUT_IDS.PHONE_NUMBER] = translate('bankAccount.error.phoneNumber');
            }
        } catch (error) {
            // Handle unexpected errors during parsing or validation
            console.error('Error validating phone number:', error);
            errors[INPUT_IDS.PHONE_NUMBER] = translate('bankAccount.error.phoneNumber');
        }

        // Clear the error if the user tries to validate the form and there are errors
        if (validateLoginError && Object.keys(errors).length > 0) {
            PersonalDetails.clearPhoneNumberError();
        }

        return errors;
    },
    [translate, validateLoginError],
);
  1. The error message should be clear and provide an example of a correctly formatted phone number.

phoneNumber: 'Please enter a valid phone number.',

to

Please enter a complete phone number (e.g., +1-(201)-867-5309).

or like

phoneNumber: `Please enter a valid phone number, with the country code (e.g. ${CONST.EXAMPLE_PHONE_NUMBER})`,
to consistent

  1. Format the phone number with the new update

PersonalDetails.updatePhoneNumber(values?.phoneNumber ?? '', currenPhoneNumber);

Update to

    const phoneNumberWithCountryCode = LoginUtils.appendCountryCode(values?.phoneNumber ?? '');
    const parsedPhoneNumber = PhoneNumberUtils.parsePhoneNumber(phoneNumberWithCountryCode);
    PersonalDetails.updatePhoneNumber(parsedPhoneNumber.number?.e164 ?? '', currenPhoneNumber);

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Create validation UI tests for each case update

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.

@shubham1206agra
Copy link
Contributor

shubham1206agra commented Jan 16, 2025

Proposal

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

Improve Phone Number Entry Validation and Error Messaging for Users

What is the root cause of that problem?

In validation function here

const validate = useCallback(
(values: FormOnyxValues<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM>): FormInputErrors<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM> => {
const errors: FormInputErrors<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM> = {};
if (!ValidationUtils.isRequiredFulfilled(values[INPUT_IDS.PHONE_NUMBER])) {
errors[INPUT_IDS.PHONE_NUMBER] = translate('common.error.fieldRequired');
}
const phoneNumberWithCountryCode = LoginUtils.appendCountryCode(values[INPUT_IDS.PHONE_NUMBER]);
const parsedPhoneNumber = PhoneNumberUtils.parsePhoneNumber(values[INPUT_IDS.PHONE_NUMBER]);
if (!parsedPhoneNumber.possible || !Str.isValidE164Phone(phoneNumberWithCountryCode.slice(0))) {
errors[INPUT_IDS.PHONE_NUMBER] = translate('bankAccount.error.phoneNumber');
}
// Clear the error when the user tries to validate the form and there are errors
if (validateLoginError && !!errors) {
PersonalDetails.clearPhoneNumberError();
}
return errors;
},
[translate, validateLoginError],
);

We have 2 problems -

  1. We always try to append country code here, but we used unappended phone number to check for possible number here
  2. We strictly try to check for valid E164 format only, which neglects other formats.
  3. We use bankAccount.error.phoneNumber as error.

When we show the error message the size of the view that contains the avatar increases which causes avatar to get centre with respect to the DotIndicator (error message).

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

Fixes

  1. Use appended phone number to validate phone number
  2. Use isValidPhoneFormat instead of isValidE164Phone here
  3. Maybe use placeholder text in the input to indicate a valid phone number, or, update the validation error to the following:
    Please enter a valid phone number (e.g. ${CONST.EXAMPLE_PHONE_NUMBER})

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

NA since this is validation change

What alternative solutions did you explore? (Optional)

NA

@JastiDev
Copy link

Proposal

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

Update the phone number validation when the user enters the phone number incorrectly.

What is the root cause of that problem?

Here is the code for phone number validation

    const validate = useCallback(
        (values: FormOnyxValues<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM>): FormInputErrors<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM> => {
            const errors: FormInputErrors<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM> = {};
            if (!ValidationUtils.isRequiredFulfilled(values[INPUT_IDS.PHONE_NUMBER])) {
                errors[INPUT_IDS.PHONE_NUMBER] = translate('common.error.fieldRequired');
            }
            const phoneNumberWithCountryCode = LoginUtils.appendCountryCode(values[INPUT_IDS.PHONE_NUMBER]);
            const parsedPhoneNumber = PhoneNumberUtils.parsePhoneNumber(values[INPUT_IDS.PHONE_NUMBER]);
            if (!parsedPhoneNumber.possible || !Str.isValidE164Phone(phoneNumberWithCountryCode.slice(0))) {
                errors[INPUT_IDS.PHONE_NUMBER] = translate('bankAccount.error.phoneNumber');
            }


            // Clear the error when the user tries to validate the form and there are errors
            if (validateLoginError && !!errors) {
                PersonalDetails.clearPhoneNumberError();
            }
            return errors;
        },
        [translate, validateLoginError],
    );

These code didn't include correct logic according to the requirement.

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

We have to update this code according to the requirement by adding new validation logic.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

@lionuncle
Copy link

lionuncle commented Jan 16, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-01-16 11:01:48 UTC.

Proposal

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

Improve phone number input validation and provide clear error messages for users.

What is the root cause of that problem?

The validation function in the following file is causing issues:

'src/pages/settings/Profile/PersonalDetails/PhoneNumberPage.tsx' on line 52

src/languages/en.ts on line 1989

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

Fixes:
Use the appended phone number for validation.

Update the error message to provide a clearer example:

New Message: "Please enter a complete phone number (e.g., +1-(201)-867-5309)."

Update in

src/pages/settings/Profile/PersonalDetails/PhoneNumberPage.tsx (line 52).

const validate = useCallback(
        (values: FormOnyxValues<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM>): FormInputErrors<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM> => {
            const errors: FormInputErrors<typeof ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM> = {};
            const phoneNumber = values[INPUT_IDS.PHONE_NUMBER];

            if (!ValidationUtils.isRequiredFulfilled(phoneNumber)) {
                errors[INPUT_IDS.PHONE_NUMBER] = translate('common.error.fieldRequired');
                return errors;
            }

            try {
                let sanitizedNumber = phoneNumber.trim();

                if (sanitizedNumber.startsWith('00')) {
                    sanitizedNumber = '+' + sanitizedNumber.slice(2);
                }

                const digitsOnly = sanitizedNumber.replace(/\D/g, '');

                if (digitsOnly.length < 10 || digitsOnly.length > 11) {
                    errors[INPUT_IDS.PHONE_NUMBER] = translate('bankAccount.error.phoneNumber');
                    return errors;
                }

                if (/(.)\1{7,}/.test(digitsOnly)) {
                    errors[INPUT_IDS.PHONE_NUMBER] = translate('bankAccount.error.phoneNumber');
                    return errors;
                }

                let fullNumber = digitsOnly;

                if (digitsOnly.length === 10) {
                    fullNumber = `+1${digitsOnly}`;
                } else if (digitsOnly.length === 11 && digitsOnly.startsWith('1')) {
                    fullNumber = `+${digitsOnly}`;
                } else {
                    errors[INPUT_IDS.PHONE_NUMBER] = translate('bankAccount.error.phoneNumber');
                    return errors;
                }

                try {
                    const parsedPhoneNumber = PhoneNumberUtils.parsePhoneNumber(fullNumber);
                    if (!parsedPhoneNumber.possible || !Str.isValidE164Phone(fullNumber)) {
                        errors[INPUT_IDS.PHONE_NUMBER] = translate('bankAccount.error.phoneNumber');
                        return errors;
                    }
                } catch (error) {
                    console.error('Error parsing phone number:', error);
                    errors[INPUT_IDS.PHONE_NUMBER] = translate('bankAccount.error.phoneNumber');
                    return errors;
                }
            } catch (error) {
                console.error('Error validating phone number:', error);
                errors[INPUT_IDS.PHONE_NUMBER] = translate('bankAccount.error.phoneNumber');
            }

            if (validateLoginError && Object.keys(errors).length > 0) {
                PersonalDetails.clearPhoneNumberError();
            }

            return errors;
        },
        [translate, validateLoginError],
    );

Modify in

src/languages/en.ts (line 1989):

phoneNumber: 'Please enter a complete phone number (e.g., +1-(201)-867-5309).',

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

  1. Accept phone numbers with typical formatting (e.g., (201)-867-5309 or +1 (201) 867-5309).
  2. Reject phone numbers with insufficient digits (e.g., 123) or excessive special characters (e.g., abc123).
  3. Ensure the layout remains stable when the error message is displayed.

What alternative solutions did you explore? (Optional)

Adding a country selector with flags to allow users to choose their country code directly. This would eliminate the need for users to manually type the + prefix and ensure proper formatting. While this approach enhances usability

@trjExpensify
Copy link
Contributor

Does anyone have in the RCA why this existing invalidPhoneNumber error message didn't show? https://github.com/Expensify/App/blame/73a82fe80190df20709afbbf5ea0f687ba8ddb31/src/languages/en.ts#L1851

@shubham1206agra
Copy link
Contributor

@trjExpensify Because we use bankAccount.error.phoneNumber in the validation error. I have said this in my RCA. See point 3.

@samranahm
Copy link

Proposal

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

Improve Phone Number Entry Validation and Error Messaging for Users

What is the root cause of that problem?

1. The issue is that our validation function processes values[INPUT_IDS.PHONE_NUMBER] (the user's input) to parse the phone number. If the user doesn't include the '+' prefix, the system doesn't accept the format.

2. We are getting wrong error message because we are fetching bankAccount.error.phoneNumber

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

We should pass phoneNumberWithCountryCode to PhoneNumberUtils.parsePhoneNumber instead of values[INPUT_IDS.PHONE_NUMBER]

 const parsedPhoneNumber = PhoneNumberUtils.parsePhoneNumber(phoneNumberWithCountryCode);

to get the correct error message we should update

errors[INPUT_IDS.PHONE_NUMBER] = translate('bankAccount.error.phoneNumber');

to

translate('common.error.phoneNumber');

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

If we want to allow the user to follow format other then E164 we can use

   const cleanedPhoneNumber = values[INPUT_IDS.PHONE_NUMBER].replace(/[\s\-\(\)]/g, '');

   const phoneNumberWithCountryCode = LoginUtils.appendCountryCode(cleanedPhoneNumber);
   const parsedPhoneNumber = PhoneNumberUtils.parsePhoneNumber(phoneNumberWithCountryCode);
   
   if (!parsedPhoneNumber.possible || !Str.isValidE164Phone(phoneNumberWithCountryCode.slice(0))) {                  
      errors[INPUT_IDS.PHONE_NUMBER] = translate('common.error.phoneNumber');
   }

This approach allows the user to enter the phone number in any way as their preference (e.g., with spaces, dashes, or parentheses), but the parsedPhoneNumber will remain consistent.

@trjExpensify
Copy link
Contributor

Because we use bankAccount.error.phoneNumber in the validation error.

Why? Given that we have privatePersonalDetails.error.invalidPhoneNumber here.

@shubham1206agra
Copy link
Contributor

Because we use bankAccount.error.phoneNumber in the validation error.

Why? Given that we have privatePersonalDetails.error.invalidPhoneNumber here.

@trjExpensify Might have been sync mistake while creating that page.

@bfitzexpensify bfitzexpensify added the External Added to denote the issue can be worked on by a contributor label Jan 17, 2025
@melvin-bot melvin-bot bot changed the title Improve Phone Number Entry Validation and Error Messaging for Users [$250] Improve Phone Number Entry Validation and Error Messaging for Users Jan 17, 2025
Copy link

melvin-bot bot commented Jan 17, 2025

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 17, 2025
Copy link

melvin-bot bot commented Jan 17, 2025

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

Copy link

melvin-bot bot commented Jan 21, 2025

@suneox, @bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Jan 21, 2025
@suneox
Copy link
Contributor

suneox commented Jan 21, 2025

Thank you for all the proposals. I'll check them within today.

@melvin-bot melvin-bot bot removed the Overdue label Jan 21, 2025
@suneox
Copy link
Contributor

suneox commented Jan 21, 2025

Base on expected result
The system should accept phone numbers with or without the + prefix and ignore non-numeric characters during validation.

@huult proposal satisfies the requirement. It ignores non-numeric characters before formatting/validation, and currently the BE only allow phone numbers in E.164 format can be saved other formats cannot be saved.

Image

Therefore, we need to format the phone number to E.164 before submit to BE, and it also included in his proposal.

@shubham1206agra Your proposal does not support saving in another formats like +1 201-867-5309.

+1 201-867-5309
Screen.Recording.2025-01-21.at.23.25.57.mp4

However, I agree that we should fix the same issue in substeps/PhoneNumber.tsx

Overall, we can proceed with @huult proposal and apply the same fix in substeps/PhoneNumber.tsx. The messy code cleanup from the selected proposal can be performed in a PR.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Jan 21, 2025

Triggered auto assignment to @NikkiWines, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@shubham1206agra
Copy link
Contributor

Sorry @suneox, but this is a BE problem then since we should allow saving in different formats as it is. We allow these formats in BA flow, so why not here?

@suneox
Copy link
Contributor

suneox commented Jan 21, 2025

Sorry @suneox, but this is a BE problem then since we should allow saving in different formats as it is. We allow these formats in BA flow, so why not here?

I'm not sure if the BE can support various phone number formats with different country codes and patterns to process with a phone number mask, such as +1-(201)-867-5309 as outlined in the expected result or +84 098.730.033. I think the scope of this issue is only focus on the client side with masked input and ensure phone numbers are saved in E.164 format.

cc: @bfitzexpensify

@shubham1206agra
Copy link
Contributor

@suneox If it was explicitly written in expected result that we need to save in E.164 format, I would have adjusted the logic. Since this is not in scope of the actual issue. We should not make decision on something that is outside scope of the issue or can be easily modified during the PR phase.

@suneox
Copy link
Contributor

suneox commented Jan 21, 2025

We can wait for feedback from the internal team

@NikkiWines
Copy link
Contributor

I think the scope of this issue is only focus on the client side with masked input and ensure phone numbers are saved in E.164 format.

Correct. Phone numbers are currently saved in this format, so I don't consider it out of the scope of the issue to take that into account when making proposals for this issue.

I think @huult's proposal looks good too. And we should use this format for the error message for the sake of consistency

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 22, 2025
Copy link

melvin-bot bot commented Jan 22, 2025

📣 @suneox 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Jan 22, 2025

📣 @huult 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@huult
Copy link
Contributor

huult commented Jan 23, 2025

Thank you all, I will create pull request within 24 hours.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 26, 2025
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Feb 4, 2025
@michaelkwardrop
Copy link
Contributor

Excited to see this live 👀

@suneox
Copy link
Contributor

suneox commented Feb 7, 2025

We are still working on an edge case in the PR.

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. External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2
Projects
Status: No status
Development

No branches or pull requests