[HOLD for payment 2024-04-15] Address - Saved address without zip code is not reflected on secondary device

[kbecciv]

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.57-2
Reproducible in staging?: y
Reproducible in production?: y
Issue reported by:

Action Performed:

1. Sign in with the same account on a main and secondary device
2. On the main device, Navigate to Profile > Address
3. Add address without including zip code
4. On secondary device, observe the Address field

Expected Result:

Added address should be reflected on secondary device

Actual Result:

Added address is not reflected on secondary device

Workaround:

n/a

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

Bug6429118_1711566314478.Screen_Recording_2024-03-27_at_9.55.11_at_night.mp4

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @paultsimura

[melvin-bot]

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

[kbecciv]

@laurenreidexpensify I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[habasefa]

This looks BE to me. And it still works with some places like Cairo Desert.

[bernhardoj]

Proposal

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

Saving an address without zip code isn't reflected on the secondary device.

What is the root cause of that problem?

When we save an address with the US (or some other countries) as the country without a zip code, the request actually fails, that's why it's not reflected on the 2nd device.

For the US country, a zip code is required. This issue happens after this PR. We have a list of zip code regex for each country here:
https://github.com/Expensify/App/blob/main/src/CONST.ts#L2161

If it's a US country, then it will use the regex for the US address. However, we only apply the regex if the zip code is not empty.

App/src/components/AddressForm.tsx

Lines 121 to 129 in 469b112

	if (countrySpecificZipRegex && values.zipPostCode) {
	if (!countrySpecificZipRegex.test(values.zipPostCode.trim().toUpperCase())) {
	if (ValidationUtils.isRequiredFulfilled(values.zipPostCode.trim())) {
	errors.zipPostCode = ['privatePersonalDetails.error.incorrectZipFormat', countryZipFormat];
	} else {
	errors.zipPostCode = 'common.error.fieldRequired';
	}
	}
	} else if (!CONST.GENERIC_ZIP_CODE_REGEX.test(values?.zipPostCode?.trim()?.toUpperCase() ?? '')) {

Before the PR, we always test the regex even when the zip code is empty.

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

Always test the regex if exists by removing && values.zipPostCode. We can apply a null chaining to the zip code value if needed.

Also, I think we can improve the failure handling of the address by reverting it to the prev value if fails so it's clear that the saving has failed. We can also show the error message from BE, but I'm not sure where to put the error as the error is attached to private_personalDetails onyx data. And last, I don't know how we will be able to test the failure if we already fix the issue.

[allgandalf]

Proposal

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

What is the root cause of that problem?

As metioned by @bernhardoj , the BE rejects the request if we don't have zipcode for specific countries.

Also,

We don't include zip code in required fields:

App/src/components/AddressForm.tsx

Line 95 in 469b112

const requiredFields = ['addressLine1', 'city', 'country', 'state'] as const;

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

We should make zip code as a mandatory field over here.

const requiredFields = ['addressLine1', 'city', 'country', 'state', 'zipPostCode' ] as const;

This will also make sure that we always test the regex against values

Note

If we want to make zip code mandatory for only certain counties then we can have a check for the country code, and if in const.ts file we don't have a empty json for that then we will mandate zip code other wise we won't

• Example:

App/src/CONST.ts

Lines 2162 to 2173 in 50d35d8

	AC: {
	regex: /^ASCN 1ZZ$/,
	samples: 'ASCN 1ZZ',
	},
	AD: {
	regex: /^AD[1-7]0\d$/,
	samples: 'AD206, AD403, AD106, AD406',
	},
	// We have kept the empty object for the countries which do not have any zip code validation
	// to ensure consistency so that the amount of countries displayed and in this object are same
	AE: {},

Over here we will madate zip code for AC and AD but not for AE as it is empty json:
The implementation would be taken from :

App/src/components/AddressForm.tsx

Line 113 in 469b112

const countryRegexDetails = (values.country ? CONST.COUNTRY_ZIP_REGEX_DATA?.[values.country] : {}) as CountryZipRegex;

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

[melvin-bot]

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

[laurenreidexpensify]

@paultsimura can you take a look and confirm is this is BE?

[paultsimura]

At first glance, the points provided by @bernhardoj look reasonable, and I would alter the expected behavior described in this issue. But I need some time to investigate deeper – will post an update shortly.

[paultsimura]

The proposal by @bernhardoj looks good to me – IMO it makes more sense than the expected behavior on the issue.

The request validation fails on the BE – and it's expected – but the FE validation needs to be fixed as described in the proposal to match the BE logic.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[MonilBhavsar]

I agree with the proposal, but should we also add zipCode to the list of requiredFields if the request actually fails if zipCode is not passed?

[paultsimura]

should we also add zipCode to the list of requiredFields if the request actually fails if zipCode is not passed?

We can't do this because these are some countries where addresses do not require zipCode, and making zipCode required regardless of the country will break the logic for them. For such countries, the BE request doesn't fail.
@MonilBhavsar

[laurenreidexpensify]

@trjExpensify this feels like a settings sync issue - would you consider it a #wave-collect bug?

[MonilBhavsar]

We can't do this because these are some countries where addresses do not require zipCode, and making zipCode required regardless of the country will break the logic for them. For such countries, the BE request doesn't fail.

Thanks for clarifying! In that case proposal looks good to me.

[bernhardoj]

PR is ready

cc: @paultsimura

[trjExpensify]

@trjExpensify this feels like a settings sync issue - would you consider it a #wave-collect bug?

Yeah, I would, because the address in private details is used for Ecard shipping etc.

[melvin-bot]

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

[melvin-bot]

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

• Fix user can save address without zip code even when the country requires zip code #39486

If no regressions arise, payment will be issued on 2024-04-15. 🎊

For reference, here are some details about the assignees on this issue:

• @paultsimura requires payment (Needs manual offer from BZ)
• @bernhardoj requires payment (Needs manual offer from BZ)

[melvin-bot]

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:

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

[paultsimura]

• The PR that introduced the bug has been identified. Link to the PR: [TS migration] Migrate 'AddressForm.js' component to TypeScript #34149
• 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: https://github.com/Expensify/App/pull/34149/files#r1563939061
• 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: N/A
• Determine if we should create a regression test for this bug: Probably no*
• 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.

Note: I believe there should have been a regression test for zip codes to be mandatory for specific countries. If not - please let me know, I'll suggest one

[paultsimura]

@laurenreidexpensify just a heads up: this issue was created before the announced price drop.

[laurenreidexpensify]

Payment Summary:

• C+ @paultsimura $500 - please apply in Upwork - https://www.upwork.com/jobs/~0154b61e7a159d9086C+
• Contributor @bernhardoj $500 - please apply in Upwork - https://www.upwork.com/jobs/~0154b61e7a159d9086

[bernhardoj]

Applied!

[paultsimura]

Same for me, thanks

[melvin-bot]

Payment Summary

Upwork Job

• ROLE: @paultsimura paid $(AMOUNT) via Upwork (LINK)
• ROLE: @bernhardoj paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@laurenreidexpensify)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[laurenreidexpensify]

Payment Summary:

• C+ @paultsimura $500 - paid in upwork - upwork.com/jobs/~0154b61e7a159d9086
• Contributor @bernhardoj $500 - please accept offer in upwork - upwork.com/jobs/~0154b61e7a159d9086

[bernhardoj]

@laurenreidexpensify accepted

[laurenreidexpensify]

Payment Summary:

• C+ @paultsimura $500 - paid in upwork - upwork.com/jobs/~0154b61e7a159d9086
• Contributor @bernhardoj $500 - paid in upwork - upwork.com/jobs/~0154b61e7a159d9086

[bernhardoj]

@laurenreidexpensify I don't see it's being paid yet

Is it delayed?