[Paid] [$8000] zip /postcode validation error message not displayed for entering , on the Home address screen

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. go to Settings > profile> personal details >Home address
2. type the zip /postcode as ,

Expected Result:

it should show validation error message

Actual Result:

does not show validation message

Workaround:

unknown

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: 1.2.67-7
Reproducible in staging?: y
Reproducible in production?: new feature
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:

Screen.Recording.2023-02-08.at.12.16.24.PM.mov

Recording.1468.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~015b692899f4b1568b
• Upwork Job ID: 1623772333283323904
• Last Price Increase: 2023-03-02

[MelvinBot]

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

[MelvinBot]

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

[Beamanator]

A bit more context: on the "Personal Information" step of setting up a bank account show an error when entering , in the Zip code field. Noted in slack here: https://expensify.slack.com/archives/C049HHMV9SM/p1675844442499409?thread_ts=1675838827.973319&cid=C049HHMV9SM

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[esh-g]

Proposal

Restating the problem

Some unwanted characters are allowed in zip code

Root Cause

Regex is not being validated correctly for zip code

Solution

Here are some conditions for the post code:

• Every postal code system uses only A-Z and/or 0-9 and sometimes space/dash
• Not every country uses postal codes (ex. Ireland outside of Dublin), but we'll ignore that here.
• The shortest postal code format is Sierra Leone with NN
• The longest is American Samoa with NNNNN-NNNNNN
• You should allow one space or dash.
• Should not begin or end with space or dash

^[a-z0-9][a-z0-9\- ]{0,10}[a-z0-9]$
This regex should cover all those conditions

source: https://stackoverflow.com/a/19844362/15373641

So I'll just this validation to the Personal information Page as well as the Home Address Screen. Not posting a code diff due to changed proposal guidelines

Note:
As mentioned by @Beamanator in https://expensify.slack.com/archives/C049HHMV9SM/p1675845424815519?thread_ts=1675838827.973319&cid=C049HHMV9SM We also have to allow letters like in Canadian zip codes, therefore allowing the alphabets

[Prince-Mendiratta]

Proposal

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

The validation on the AddressPage component is insufficient, leading to many problems. We need to add proper validation for all the fields in the component.

What is the root cause of that problem?

The root cause behind this issue is that none of the values on the home address are validated properly, the only thing checked is if the fields are empty or not in AddressPage.

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Lines 115 to 122 in b113107

	_.each(requiredFields, (fieldKey) => {
	if (!_.isEmpty(values[fieldKey])) {
	return;
	}
	errors[fieldKey] = this.props.translate('common.error.fieldRequired');
	});
	return errors;

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

The validation in AddressPage is outdated and does not validate all fields properly. The best case would be to check the validations performed in CompanyPage and then use the ValidationUtils to ensure that not just the zip code, but all fields are correctly validated.
EDIT:
Adding the required checks for each field:

For each required field, we can run it through ValidationUtils.isRequiredFulfilled() first.

Zip code: Zip codes across the world can vary differently, they need not be of 5 digits only. I suggest we just do a basic sanity check and not do anything else.

• Solution 1
  Create a map of regexes with respective country specific zip code matching and then validate according to user input.

• Solution 2 (Better IMO)
  It doesn't really matter whatever the zip code the user enters. We can just perform a basic sanity check and then let the user enter whatever they want. The regex for the same is - ^[\w][\w\- ]{0,10}[\w]$.

[abdulrahuman5196]

Proposal

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

Validation errors are not shown in the address page with zip code

What is the root cause of that problem?

We don't validate zip codes. In fact In the particular page i don't see any validation except empty check validation.
https://github.com/Expensify/App/blob/main/src/pages/settings/Profile/PersonalDetails/AddressPage.js#L116

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

We can add proper validation for all fields in the particular page including the concerned zip code.
We already have validations for these in other flows like company step. We can reuse the same functionality here as well.

For ex: We already have validation util for zipcode here - https://github.com/Expensify/App/blob/main/src/libs/ValidationUtils.js#L167

[hackykitty]

Proposal

Re-state the problem

zip/postcode validation error message not displayed for entering , on the Home address screen

What is the root cause of that problem?

The root cause of this problem is the non-existent validation function for zip/postcode field.

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

Solution is very simple!!! Before clicking save button, just validate the zip/postcode field by using this function and show the validation error message if the user entered wrong.

/**
 * @param {String} postcode 
 * @returns {Boolean}
 */
function isValidPostalCode(postcode) {
    return /^[0-9]{5}(?:-[0-9]{4})?$/.test(postcode);
}

What alternative solutions did you explore? (Optional)

Optionally, we can use postal code API for postal code validation from https://developer.bring.com/api/postal-code/

[Prince-Mendiratta]

My proposal will resolve #15003 as well!

[Changvang]

@twisterdotcom
Hmm, Yeah, the vadationUtils.isRequiredFulfilled seem good with #15003 , but with this issue I don't think it enough. Like it can't check zip code properly, like " , " still pass check validate. You can check it.

And with add some space to pass the validation of required field, I can see other form get this issue like AddDebitCardPage.js, you see the billing address is required field but still not show required when I type more space. The validationUtils has using in this form, but not for address field, maybe other form have this scenario too.

[mananjadhav]

@anmurali @Beamanator Can you please assign another C+ here? I have backlog of issues to cover from my OOO.

[0xmiros]

@esh-g your regex doesn't cover capital letters

[0xmiros]

@Prince-Mendiratta your proposal doesn't explain what validation to use exactly for Zip / Postcode. Please note that it's a bit different rule from Zip code in Personal information page in bank account step. It should allow letters as well as numbers. Here's context

[0xmiros]

@abdulrahuman5196 same feedback as above

[0xmiros]

@hackykitty same feedback as above on your main solution. Regarding optional solution, this is pretty simple and we may not need 3rd party api to get postal code.

[MelvinBot]

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

[MelvinBot]

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

• [AddressPage] Zip Code regex based validation #15880

If no regressions arise, payment will be issued on 2023-04-12. 🎊

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

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

[MelvinBot]

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:

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

[Prince-Mendiratta]

Hey folks! Sharing the timeline for this issue for calculation of bonus:

🐛 Issue created at: Wed, 08 Feb 2023 22:09:49 GMT
🧯 Help Wanted at: Mon, 20 Feb 2023 08:57:41 GMT
🕵🏻 Contributor assigned at: Tue, 07 Mar 2023 11:04:29 GMT
✋🏻 Issue put on HOLD in favour of another PR and backend changes: Tue, 07 Mar 2023 11:05:19 GMT
🛸 Backend changes pushed to staging: Fri, 31 Mar 2023 10:33:43 GMT
🎯 PR merged at: Mon, 03 Apr 2023 12:50:52 GMT
⌛ Business Days Elapsed between backend changes published and PR merge: 1

[MelvinBot]

@Beamanator, @anmurali, @Prince-Mendiratta, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Beamanator]

Bump @anmurali are you able to take care of this payment? I can assign another Bug-Zero team member if needed

[Beamanator]

Another quick bump @anmurali - I'm OOO till Thurs, I will ask for a new BZ if we haven't moved forward by then :D 👍

[anmurali]

Sent offers

[Prince-Mendiratta]

@anmurali Thank you, accepted!

[gadhiyamanan]

@anmurali accepted
Thanks!

[anmurali]

@0xmiroslav can you please complete the checklist

[0xmiros]

There are 2 issues here:

• validation with trimmed value
  No PR caused regression but we missed trim when implement required validation in [N7 Account Settings] Create personal details pages #14290 so first 3 items can be checked off.
  For regression test step, I suggest to add new checklist item like this:

If the PR adds new input field, I verified that validation is properly checked, i.e. with trimmed value.

• zipcode validation
  I'd say this is a feature rather than bug so no regression step required

[gadhiyamanan]

@anmurali please close the upwork contract
Thanks!

[MelvinBot]

@Beamanator, @anmurali, @Prince-Mendiratta, @0xmiroslav Huh... This is 4 days overdue. Who can take care of this?

[anmurali]

We're all done here!