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

Personal details - "common.zipCodeExampleFormat" appears under Zip / Postcode field #38660

Closed
6 tasks done
izarutskaya opened this issue Mar 20, 2024 · 27 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. DeployBlockerCash This issue or pull request should block deployment Engineering Reviewing Has a PR in review Weekly KSv2

Comments

@izarutskaya
Copy link

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.55-0
Reproducible in staging?: Y
Reproducible in production?: N
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

Precondition:

  • Address is not filled in Private section.
  1. Go to staging.new.expensify.com.
  2. Go to Profile
  3. Click Address.

Expected Result:

There will be no strings under Zip / Postcode field since the address is not filled.

Actual Result:

"common.zipCodeExampleFormat" appears under Zip / Postcode field.

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

Bug6420278_1710929184259!zipcode

View all open jobs on GitHub

@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 20, 2024
Copy link

melvin-bot bot commented Mar 20, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Mar 20, 2024
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Mar 20, 2024

Triggered auto assignment to @neil-marcellini (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@izarutskaya
Copy link
Author

@sakluger 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.

We think this issue might be related to the #vip-vsb.

@izarutskaya
Copy link
Author

Production

image (2)

@Krishna2323
Copy link
Contributor

Krishna2323 commented Mar 20, 2024

Proposal

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

Personal details - "common.zipCodeExampleFormat" appears under Zip / Postcode field.

What is the root cause of that problem?

No fallback is provided when country is not there. So zipSampleFormat becomes empty

const zipSampleFormat = (country && (CONST.COUNTRY_ZIP_REGEX_DATA[country] as CountryZipRegex)?.samples) ?? '';
const zipFormat: MaybePhraseKey = ['common.zipCodeExampleFormat', {zipSampleFormat}];

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

Add CONST.COUNTRY_ZIP_REGEX_DATA.US.samples as fallback for zipSampleFormat.

const zipSampleFormat = country ? (CONST.COUNTRY_ZIP_REGEX_DATA[country] as CountryZipRegex)?.samples : CONST.COUNTRY_ZIP_REGEX_DATA.US.samples;

Result

@Krishna2323
Copy link
Contributor

Seems like offending is deployed to production 2 weeks ago: #34149
Maybe we switched to use the tsx file recently.

@hungvu193
Copy link
Contributor

Regression from this one: #37200
cc @hurali97

@Krishna2323
Copy link
Contributor

@hungvu193 are you sure that Regression is from #37200?

@hungvu193
Copy link
Contributor

@hungvu193 are you sure that Regression is from #37200?

I believe so 😄

@hurali97
Copy link
Contributor

hurali97 commented Mar 20, 2024

@hungvu193 This issue is not regressed by #37200. I commented out the cache mechanism which was introduced in the PR and the issue still persists.

After applying @Krishna2323 's suggestion here, the issue seems to be resolved.

VEED.CREATE.Edit.mp4

@hungvu193
Copy link
Contributor

hungvu193 commented Mar 20, 2024

@hungvu193 This issue is not regressed by #37200. I commented out the cache mechanism which was introduced in the PR and the issue still persists.

After applying @Krishna2323 's suggestion here, the issue seems to be resolved.

VEED.CREATE.Edit.mp4

I apologize if I was wrong, but can you try to revert your PR and test it? Or you can checkout this commit f042015061 (this is the commit before your PR got merged), the issue will disappear.
Here's the console btw:
Screenshot 2024-03-20 at 18 17 09

@hurali97
Copy link
Contributor

@hungvu193 I checked out to the commit and below is what I get now:

Screenshot 2024-03-20 at 4 19 30 PM

Is it correct as we don't have the placeholder US number?

@hungvu193
Copy link
Contributor

hungvu193 commented Mar 20, 2024

I think @izarutskaya mentioned it here as expected result but I'm not sure.
Screenshot 2024-03-20 at 18 21 23

@hurali97
Copy link
Contributor

@hungvu193 Okay I am working on to find the cause in my PR 👍

@hungvu193
Copy link
Contributor

@hungvu193 Okay I am working on to find the cause in my PR 👍

AFAIK, common.zipCodeExampleFormat doesn't seem to exist in cacheForLocale. I hope this will help.

@hurali97
Copy link
Contributor

@hungvu193 I have tried commenting out the cache mechanism but things are still the same. It's probably due to the fact that getTranslatedPhrase is a recursive function, so there's something fishy going on.

@hungvu193
Copy link
Contributor

@hungvu193 I have tried commenting out the cache mechanism but things are still the same. It's probably due to the fact that getTranslatedPhrase is a recursive function, so there's something fishy going on.

I don't have context about how cache work, but any reason other phrases exist in cacheForLocale except common. zipCodeExampleFormat?

@hurali97
Copy link
Contributor

hurali97 commented Mar 20, 2024

Hey @hungvu193,

So I have found the issue. Basically, common. zipCodeExampleFormat doesn't work because we have a condition which checks whether translatedPhrase is a truthy value and common.zipCodeExampleFormat returns an empty string, which is a falsy value and the code returns the key as is. And the other phrases works because they return something other than an empty string.

Below is how I think we should change the condition:

Screenshot 2024-03-20 at 4 45 18 PM

With this change we have the same output as of production.

@hungvu193
Copy link
Contributor

@hungvu193 I have tried commenting out the cache mechanism but things are still the same. It's probably due to the fact that getTranslatedPhrase is a recursive function, so there's something fishy going on.

I don't have context about how cache work, but any reason other phrases exist in cacheForLocale except common. zipCodeExampleFormat?

@hurali97 Cool. This is only the concern I have now 🤔

@hurali97
Copy link
Contributor

@hungvu193 I have tried commenting out the cache mechanism but things are still the same. It's probably due to the fact that getTranslatedPhrase is a recursive function, so there's something fishy going on.

I don't have context about how cache work, but any reason other phrases exist in cacheForLocale except common. zipCodeExampleFormat?

@hurali97 Cool. This is only the concern I have now 🤔

Oh yes. The thing is in cache we don't store any phrases which are bound to a variable as it kills the purpose of cache.

@hungvu193
Copy link
Contributor

Oh yes. The thing is in cache we don't store any phrases which are bound to a variable as it kills the purpose of cache.

Great!. Seems @neil-marcellini is OOO. cc @roryabraham

@dragnoir
Copy link
Contributor

maybe better check for null and undefined, allowing other values to pass through

if (translatedPhrase !== null && translatedPhrase !== undefined) {
    return translatedPhrase;
}

@hurali97
Copy link
Contributor

@dragnoir Oh yeah, agreed 👍

@hungvu193 How do you want this issue to be addressed? Should I create a PR fixing this issue or you've someone else to do it?

@hungvu193
Copy link
Contributor

@dragnoir Oh yeah, agreed 👍

@hungvu193 How do you want this issue to be addressed? Should I create a PR fixing this issue or you've someone else to do it?

I think you should create a PR to fix it if you're available

@roryabraham
Copy link
Contributor

I requested some changes on the PR, but since @hurali97 appears to be out for the day @hayata-suenaga has volunteered to do it. Thanks @hayata-suenaga!

@roryabraham roryabraham added Hourly KSv2 and removed Reviewing Has a PR in review Weekly KSv2 labels Mar 21, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Hourly KSv2 labels Mar 21, 2024
@roryabraham
Copy link
Contributor

Closing this as it is fixed, and no additional payments are due since this was a regression

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. DeployBlockerCash This issue or pull request should block deployment Engineering Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants