[AddressPage] Zip Code regex based validation #15880

Prince-Mendiratta · 2023-03-11T18:04:00Z

Details

With this PR, we are making the following changes:

All fields in the Home Address page will no longer accept just empty spaces, except Address Line 2.
The zip code field will now ensure that the value entered is correct according to the zip code format of the chosen country.
The acceptable format will be shown as a hint normally and in the error text as well.

Fixed Issues

Tests

For the testing, you can also use ChatGPT to generate random address from a specific country (very helpful tip).
ChatGPT prompt: Generate 5 random addresses from {country}

All fields except zip code input:

All tests are based on Step 0 as the pre requisite.
0. Go to Settings > Profile > Personal Details > Home Address.

Test 1

Enter any address in the address line 1.
Choose any address from the suggestions.
Ensure that the values are entered correctly.
Click on save.
Open the Home Address again. Ensure values are as entered.

Test 2

Enter just spaces in any field.
Ensure that an error is shown as soon as you unfocus on the field.
Ensure you are not allowed to save the values as long you don't fix the error.
Repeat the steps for all fields accept Address Line 2 and Zip / Postcode, where we allow empty values.

Extensive tests specifically for zip code field:
Test 1

Enter any address from any country in Address Line 1.
Click on any suggestion.
Ensure that the fields are filled correctly.
If the zip code was in the chosen address, ensure that it was filled and was correct according to the country zip code format so no error is shown.

Test 2

Choose any random country from the Country dropdown.
Ensure that the Format: is updated accordingly.
Ensure that the zip code format is correct for the chosen country.
Repeat this test for as many countries as you can.
Note that some countries do not have a postal code system so empty values for zip code are allowed in this case.

Broadly, the countries can be classified into two parts:

Without a regex code.
With a regex code.

A few sample for the same are:

United Arab Emirates, Benin, Fiji, Grenada, North Korea.
(Notably) Bahrain, Azerbaijan, Argentina, Barbados, Bermuda, Brazil, Canada, United Kingdom, Isle of Man, Iran, Jersey, Marshall Islands, Niue, Saudi Arabia, South Sudan.

You can also choose and test any country at random as well.

Repeat all the above tests in the Spanish language as well.

Verify that no errors appear in the JS console

Offline tests

N/A. Changes do not depend/affect on the connectivity status of the component.

QA Steps

Make sure that the "Use Staging Server" toggle is turned ON in Settings > Preferences as this PR is dependent on a backend change, which might or might not be deployed to prod by the time this reaches QA.
Same as above.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

web.mp4

Mobile Web - Chrome

android-mWeb.mp4

Mobile Web - Safari

iOS-mWeb.mp4

Desktop

desktop.mp4

iOS

ios.mp4

Android

android.mp4

MelvinBot · 2023-03-11T18:04:08Z

@Beamanator @eVoloshchak One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

src/CONST.js

src/languages/en.js

src/languages/es.js

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

Prince-Mendiratta · 2023-03-11T18:16:36Z

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

@@ -97,7 +104,6 @@ class AddressPage extends Component {
        const requiredFields = [
            'addressLine1',
            'city',
-            'zipPostCode',


We want to possibly allow empty values for zip code where there is no postal code system. Waiting for clarification on the expected behaviour here before moving ahead with this case.

Removing this here is fine but I think it's still better to show fieldRequired error than incorrectZipFormat when country supports postal code and this field is empty. 2 options to fix:

insert zipPostCode conditionally in requireFields

validate emptiness first before this line:

if (!countrySpecificZipRegex.test(values.zipPostCode.trim())) {

I was thinking we could also go down another route, instead of hiding the field, how about we disable it instead and show that "This country does not require a postal code" in place of the format hint?

You can ask that on slack thread. IMO it still doesn't make sense to show that field to users who are never familiar with postal code.

Agree with @0xmiroslav is we know the country does not have a Zip code, we should ideally not show it. But if that is too complicated of a solution, we could go with the fieldRequired for countries supporting which have zipCode

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

Prince-Mendiratta · 2023-03-11T18:20:17Z

I'm waiting for clarification on the expected behaviour in cases where the postal code system does not exist for a country, asked here in slack. Will add the videos once this is resolved.

cc @Beamanator @0xmiroslav

Prince-Mendiratta · 2023-03-11T18:28:32Z

~~Not sure how to proceed with tackling the failing test, it's because of me not adding the zipSample and zipRegex in the es.js file to keep the code DRY.~~

Resolved, created a separate object for storing regexes.

src/CONST.js

eVoloshchak · 2023-03-14T10:41:51Z

Bug:
when saving an address with no zip code (for a country that has no zipRegex), the new address replaced with an old adress in a couple of seconds

Screen.Recording.2023-03-14.at.12.29.51.mov

(notice how Angola is correctly displayed for a split second, and then it changes back to Ukraine, which is the previous address)
Native platforms behave the same, looks like a back-end issue

Prince-Mendiratta · 2023-03-14T10:44:57Z

@eVoloshchak Thanks for testing it out! Yes, I'm aware of this bug right now. This happens because when we save an empty zip code, the backend changes have still not been deployed to allow empty zip code values. Thus, the new address is not saved and it reverts back to the previous address.

The behaviour for empty zip code values is still being discussed in slack

src/CONST.js

0xmiros · 2023-03-14T11:57:25Z

Just to clarify, not sure why @eVoloshchak was assigned here (something wrong with pullbear). I am C+ on the linked issue and already reviewed proposals.
@mountiny I experienced this several times and I think this issue happens only to me (i.e. I am assigned on issue but assigned another C+ on PR) and related to my concern I raised on slack yesterday.

eVoloshchak · 2023-03-14T12:10:29Z

Just to clarify, not sure @eVoloshchak was assigned here (something wrong with pullbear). I am C+ on the linked issue and already reviewed proposals.

Oh, I was thinking this is an internal PR since I wasn't assigned to the issue. Must be something with Melvin
All you then!

mountiny · 2023-03-16T20:36:13Z

@0xmiroslav this should be fixed now, right?

0xmiros · 2023-03-16T20:58:48Z

@0xmiroslav this should be fixed now, right?

@mountiny Right. All similar issues on which I tagged you (sorry annoying you so many times) should be fixed now. Thanks

mountiny

@0xmiroslav @Prince-Mendiratta This is looking good to me, left some comments, I thin @Beamanator should be working with limited availability this week.

What is left to resolve on this PR?

src/CONST.js

mountiny · 2023-03-21T15:52:04Z

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

@@ -97,7 +104,6 @@ class AddressPage extends Component {
        const requiredFields = [
            'addressLine1',
            'city',
-            'zipPostCode',


Agree with @0xmiroslav is we know the country does not have a Zip code, we should ideally not show it. But if that is too complicated of a solution, we could go with the fieldRequired for countries supporting which have zipCode

Beamanator · 2023-03-27T11:33:24Z

@Prince-Mendiratta can you please resolve the Github comments that have been resolved? I see a lot still open and now that I have the time to jump back into reviewing I'm a bit confused if I should check all open comment threads or not :D

Beamanator

Sorry to jump in after a long delay and request changes, but I think we can make some of this a bit cleaner :D

src/CONST.js

src/languages/en.js

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

src/languages/es.js

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

src/CONST.js

Prince-Mendiratta · 2023-03-27T11:52:24Z

@Beamanator Hi, welcome back! Most of the comments were just me communicating the logic behind some logic, have resolved those to keep the ones still relevant open!

Beamanator

Overall looking great! added 1 tiny comment, but requesting changes b/c linting is failing :D

also @Prince-Mendiratta why are you force-pushing some changes? You shouldn't ever need to do that 🤔 But maybe I'm missing something

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

Beamanator

Nice, looking good to me! @0xmiroslav can you please do the platform tests when you have time? 👍

Beamanator · 2023-03-27T14:39:36Z

Aah we're also waiting on the back-end change to allow zip code to be empty, right? I'll get right on that

Prince-Mendiratta · 2023-03-27T14:40:07Z

also @Prince-Mendiratta why are you force-pushing some changes? You shouldn't ever need to do that 🤔 But maybe I'm missing something

@Beamanator As far as I know, you're never supposed to force push on the main/master branch, the history on feature branches aren't that important so shouldn't be an issue. As for why I force pushed, I pulled the latest changes on the main branch to resolve conflicts and instead of merging, I rebase the feature branch to avoid that "merged main" commit. This removes the GPG signature so I have to rebase my commits again to GPG sign them, thus requiring to force push the changes. I usually stop doing rebases and start with merges once the number/timeline of commits become relevant and important.

Hope this makes sense. If you feel that I'm doing something incorrectly, please do point out!

Prince-Mendiratta · 2023-03-27T14:40:59Z

Aah we're also waiting on the back-end change to allow zip code to be empty, right? I'll get right on that

Yup, also can we confirm the final behaviour for countries with no postal system on backend and frontend? cc @0xmiroslav

0xmiros · 2023-03-27T14:41:33Z

Aah we're also waiting on the back-end change to allow zip code to be empty, right? I'll get right on that

right, I am about to perform full review after backend deploy so was waiting for you 🙂

Beamanator · 2023-03-31T10:33:43Z

Back-end PR is on staging! 💪

0xmiros · 2023-03-31T10:35:02Z

Back-end PR is on staging! 💪

Great! will wrap this up today

0xmiros · 2023-03-31T13:03:58Z

@Prince-Mendiratta please pull from main

0xmiros · 2023-03-31T13:05:00Z

and don't use force-push from now on

Prince-Mendiratta · 2023-03-31T13:16:12Z

@0xmiroslav updated! I will upload the videos as well soon.

Prince-Mendiratta · 2023-03-31T15:04:42Z

All videos updated in the PR description.

@0xmiroslav kindly enable the "Use staging server" toggle when testing as the backend PR is on staging for now.

Also, @Beamanator I think we need not wait for the backend PR to hit prod once this is merged since the only case that is significant for the backend changes to be required would be empty zip code, which is not that important and can be deployed to prod as the lifecycle continues.

0xmiros · 2023-03-31T15:22:26Z

Repeat this test for as many countries as you can.

@Prince-Mendiratta can we be more specific in test step? (tester doesn't know which country doesn't support zipcode)

Prince-Mendiratta · 2023-03-31T15:30:08Z

@0xmiroslav updated with a few notable ones.

0xmiros · 2023-03-31T15:41:44Z

I think "This field is required." message is more accurate when empty field.
Or is it better to show like this for users to see hint?

Signed-off-by: Prince Mendiratta <prince.mendi@gmail.com>

Prince-Mendiratta · 2023-03-31T17:16:04Z

@0xmiroslav updated as per discussions in slack!

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

Signed-off-by: Prince Mendiratta <prince.mendi@gmail.com>

0xmiros · 2023-04-02T14:57:50Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

mchrome.mov

Mobile Web - Safari

msafari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

0xmiros

Latest changes look good, tests well in all possible cases.

@Prince-Mendiratta let's add setting "Use Staging Server" preference in Tests step in case backend won't be deployed to production before QA.

cc: @Beamanator

mountiny

Big one, great job @Prince-Mendiratta

All yours @Beamanator

Beamanator · 2023-04-03T12:45:26Z

Reviewing in a few minute! 👍

Beamanator

Yep looking good to go let's merge this! 🥳

OSBotify · 2023-04-03T12:55:37Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-04-03T19:27:48Z

🚀 Deployed to staging by https://github.com/Beamanator in version: 1.2.94-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2023-04-05T02:28:35Z

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.94-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

thesahindia · 2023-05-09T14:35:31Z

This PR caused a regression. Some zip code samples were incorrect.

Prince-Mendiratta requested a review from a team as a code owner March 11, 2023 18:04

melvin-bot bot requested review from Beamanator and eVoloshchak and removed request for a team March 11, 2023 18:04

Prince-Mendiratta mentioned this pull request Mar 11, 2023

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

Closed

6 tasks

Prince-Mendiratta commented Mar 11, 2023

View reviewed changes

Prince-Mendiratta commented Mar 13, 2023

View reviewed changes

src/CONST.js Show resolved Hide resolved

eVoloshchak reviewed Mar 14, 2023

View reviewed changes

src/CONST.js Show resolved Hide resolved

mountiny assigned Prince-Mendiratta Mar 16, 2023

mountiny reviewed Mar 21, 2023

View reviewed changes

Beamanator requested changes Mar 27, 2023

View reviewed changes

src/pages/settings/Profile/PersonalDetails/AddressPage.js Outdated Show resolved Hide resolved

Beamanator reviewed Mar 27, 2023

View reviewed changes

src/pages/settings/Profile/PersonalDetails/AddressPage.js Outdated Show resolved Hide resolved

Beamanator self-requested a review March 27, 2023 14:38

Beamanator previously approved these changes Mar 27, 2023

View reviewed changes

Merge branch 'main' into zip-code-validation

f07066f

Prince-Mendiratta added 2 commits March 31, 2023 22:17

reorder input fields

3a39ad6

Signed-off-by: Prince Mendiratta <prince.mendi@gmail.com>

change error message on empty string

d35b5fc

Signed-off-by: Prince Mendiratta <prince.mendi@gmail.com>

0xmiros reviewed Mar 31, 2023

View reviewed changes

src/pages/settings/Profile/PersonalDetails/AddressPage.js Outdated Show resolved Hide resolved

0xmiros reviewed Mar 31, 2023

View reviewed changes

src/pages/settings/Profile/PersonalDetails/AddressPage.js Outdated Show resolved Hide resolved

auto capitalize characters in zip

3e7ca06

Signed-off-by: Prince Mendiratta <prince.mendi@gmail.com>

0xmiros approved these changes Apr 2, 2023

View reviewed changes

mountiny approved these changes Apr 3, 2023

View reviewed changes

Beamanator approved these changes Apr 3, 2023

View reviewed changes

Beamanator merged commit 2123e1e into Expensify:main Apr 3, 2023

OSBotify mentioned this pull request Apr 3, 2023

Deploy Checklist: New Expensify 2023-04-03 #16880

Closed

21 tasks

kbecciv mentioned this pull request Apr 4, 2023

Profile - System allows you to save a dot instead of any information for home address fields #16928

Closed

6 tasks

thesahindia mentioned this pull request May 9, 2023

[HOLD for payment 2023-05-05] [$1000] Wrong sample zip code as per the regex for country KIRIBATI #17759

Closed

6 tasks

conorpendergrast mentioned this pull request Jun 19, 2023

Details - Zip code does not shown a error message #20899

Closed

6 tasks

[AddressPage] Zip Code regex based validation #15880

[AddressPage] Zip Code regex based validation #15880

Conversation

Prince-Mendiratta commented Mar 11, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Mar 11, 2023

Prince-Mendiratta Mar 11, 2023

Choose a reason for hiding this comment

0xmiros Mar 12, 2023

Choose a reason for hiding this comment

Prince-Mendiratta Mar 12, 2023

Choose a reason for hiding this comment

0xmiros Mar 12, 2023

Choose a reason for hiding this comment

mountiny Mar 21, 2023

Choose a reason for hiding this comment

Prince-Mendiratta commented Mar 11, 2023

Prince-Mendiratta commented Mar 11, 2023 • edited Loading

eVoloshchak commented Mar 14, 2023

Prince-Mendiratta commented Mar 14, 2023

0xmiros commented Mar 14, 2023 • edited Loading

eVoloshchak commented Mar 14, 2023

mountiny commented Mar 16, 2023

0xmiros commented Mar 16, 2023 • edited Loading

mountiny left a comment

Choose a reason for hiding this comment

mountiny Mar 21, 2023

Choose a reason for hiding this comment

Beamanator commented Mar 27, 2023

Beamanator left a comment

Choose a reason for hiding this comment

Prince-Mendiratta commented Mar 27, 2023

Beamanator left a comment

Choose a reason for hiding this comment

Beamanator left a comment

Choose a reason for hiding this comment

Beamanator commented Mar 27, 2023

Prince-Mendiratta commented Mar 27, 2023

Prince-Mendiratta commented Mar 27, 2023

0xmiros commented Mar 27, 2023

Beamanator commented Mar 31, 2023

0xmiros commented Mar 31, 2023

0xmiros commented Mar 31, 2023

0xmiros commented Mar 31, 2023

Prince-Mendiratta commented Mar 31, 2023

Prince-Mendiratta commented Mar 31, 2023

0xmiros commented Mar 31, 2023

Prince-Mendiratta commented Mar 31, 2023

0xmiros commented Mar 31, 2023 • edited Loading

Prince-Mendiratta commented Mar 31, 2023

0xmiros commented Apr 2, 2023

Reviewer Checklist

Screenshots/Videos

0xmiros left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

Beamanator commented Apr 3, 2023

Beamanator left a comment

Choose a reason for hiding this comment

OSBotify commented Apr 3, 2023

OSBotify commented Apr 3, 2023

OSBotify commented Apr 5, 2023

thesahindia commented May 9, 2023

Prince-Mendiratta commented Mar 11, 2023 •

edited

Loading

Prince-Mendiratta commented Mar 11, 2023 •

edited

Loading

0xmiros commented Mar 14, 2023 •

edited

Loading

0xmiros commented Mar 16, 2023 •

edited

Loading

0xmiros commented Mar 31, 2023 •

edited

Loading