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

[$1000] The Home Address City field is not filled in by Places autocomplete for addresses in certain non-US countries #16392

Closed
6 tasks done
kavimuru opened this issue Mar 22, 2023 · 33 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Mar 22, 2023

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 -> Personal details -> Home Address
  2. In Address line 1 type "5 New Street Square" and select the first result
  3. Note that the city (London) is not filled in

Expected Result:

City field should be populated.

Actual Result:

City field empty

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.88-0
Reproducible in staging?: y
Reproducible in production?: y
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:

home-address-issue-2023-03-17_17.33.45.mp4
Recording.20.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01688c30375186fd45
  • Upwork Job ID: 1638833905407811584
  • Last Price Increase: 2023-03-23
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 22, 2023
@MelvinBot
Copy link

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

@MelvinBot

This comment was marked as duplicate.

@dylanexpensify
Copy link
Contributor

reviewing now!

@dylanexpensify dylanexpensify added the External Added to denote the issue can be worked on by a contributor label Mar 23, 2023
@melvin-bot melvin-bot bot changed the title The Home Address City field is not filled in by Places autocomplete for addresses in certain non-US countries [$1000] The Home Address City field is not filled in by Places autocomplete for addresses in certain non-US countries Mar 23, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 23, 2023
@MelvinBot
Copy link

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

@jjcoffee
Copy link
Contributor

jjcoffee commented Mar 23, 2023

Proposal

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

When the city (in this case, London) is in the autocomplete result, it should fill the City field when we select that result.

What is the root cause of that problem?

We are parsing the addressComponents from the Place Details API in a way that is not compatible with all countries. In the UK, for example, addresses within cities only have the postal_town field (which we currently don't use at all), whereas locality is used for towns/villages.

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

Pull in the postal_town field in the AddressSearch component here:

const {
street_number: streetNumber,
route: streetName,
locality: city,
sublocality: cityFallback, // Some locations only return sublocality instead of locality
postal_code: zipCode,
administrative_area_level_1: state,
country,
} = GooglePlacesUtils.getAddressComponents(addressComponents, {
street_number: 'long_name',
route: 'long_name',
locality: 'long_name',
sublocality: 'long_name',
postal_code: 'long_name',
administrative_area_level_1: 'short_name',
country: 'short_name',
});

Then add it in as an extra fallback here, so that we pull it in if no locality is available:

const values = {
street: props.value ? props.value.trim() : '',
city: city || cityFallback,
zipCode,
state,
country: '',
};

From what I can see and from reading the docs, when postal_town is available, the sublocality (or cityFallback as we call it) is more used as a city subdivision (e.g. a suburb), which wouldn't be appropriate for filling the city field. So it's safe to use the fallback in the order city || postalTown || cityFallback (i.e. locality || postal_town || sublocality).

We probably also want to rename the cityFallback variable so it's clearer what each fallback does and what its source is.

Finally, some test addresses:

  • 5 New Street Square, London (postal_town)
  • 19 Coppergate, York (postal_town)
  • Main St, Huby, York (postal_town and locality where locality is the town name, and so should be used for the "City" field)
  • Salagatan 29A, 753 30 Uppsala, Sweden (postal_town with sublocality, where postal_town takes precedence over sublocality which is a city subdivision)
  • Rutenweg 8, 39291 Möckern, Germany (locality only)

@parasharrajat
Copy link
Member

@jjcoffee's proposal looks good to me. Even though I don't have much idea about it but it seems the fallback order is correct.

cc: @stitesExpensify

🎀 👀 🎀 C+ reviewed

@Snehal-Techforce
Copy link

Proposal

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

The Home Address City field is not filled in by Places autocomplete for addresses in certain non-US countries.

What is the root cause of that problem:

Here In the addressComponent from the Place Detail API is not showing “locality” parameter for some of the non-US countries, which fill up the city field.

locality: city,

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

To get city value, a new parameter “postal_town” needs to be added in the json parameter of the getAddressComponents. Apart from that another argument with OR ( || ) condition “postal_town” needs to be added to the city field in the values object. Hence the city field will be checked initially and if not available in response then it will consider cityFallBack else postal_town value will be taken as a city.

sol

What alternative solutions did you explore:

None

Expected result:

Screenshot 2023-03-24 at 3 59 46 PM

@parasharrajat
Copy link
Member

@Snehal-Techforce We strictly prohibit duplicate proposals. Please read the contribution guidelines before making a proposal.

@Snehal-Techforce
Copy link

@parasharrajat Thanks for your reply. We also do not prefer to submit duplicate proposal as per the guidelines. Also, will take care and not to submit close to similar proposal.

@melvin-bot melvin-bot bot added the Overdue label Mar 27, 2023
@dylanexpensify
Copy link
Contributor

Not overdue, seems @jjcoffee should get the job once @stitesExpensify agrees!

@melvin-bot melvin-bot bot removed the Overdue label Mar 27, 2023
@stitesExpensify
Copy link
Contributor

@jjcoffee proposal LGTM!

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 27, 2023
@MelvinBot
Copy link

📣 @jjcoffee You have been assigned to this job by @stitesExpensify!
Please apply to this job in Upwork 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 📖

@jjcoffee
Copy link
Contributor

jjcoffee commented Mar 27, 2023

Hey @parasharrajat, @stitesExpensify, @dylanexpensify - PR is ready!

@jjcoffee
Copy link
Contributor

jjcoffee commented Apr 3, 2023

Looks like the automation failed here as the PR is in production, is there a way to kick Melvin into gear? Or does someone manually update the labels and paste in the checklist?

@jjcoffee
Copy link
Contributor

jjcoffee commented Apr 5, 2023

@dylanexpensify Sorry to be a pain, but do you think you can paste in the BugZero Checklist (e.g. here)? I'd do it myself, but I'm not sure who is meant to be tagged in each task!

Also, I think this should be held for payment for 7/4 as the PR was deployed to production on 31/3, I'm not sure if you/someone needs to add/update the labels?

@parasharrajat
Copy link
Member

@dylanexpensify Can you please update the issue title and add a checklist template here?

@jjcoffee
Copy link
Contributor

@dylanexpensify Gentle bump on this as it's overdue for payment (the PR was deployed to production on 31/3).

@dylanexpensify
Copy link
Contributor

Hi team! So sorry about the wait! Will do today!!!

@dylanexpensify
Copy link
Contributor

Please apply here @jjcoffee @parasharrajat!!

@jjcoffee
Copy link
Contributor

Thanks - applied! Just an FYI, this one should also be due the 50% timeliness bonus (assigned 27/3, PR merged 28/3).

@dylanexpensify
Copy link
Contributor

Nice one! @jjcoffee @parasharrajat sent offers (will include bonus in payout)!

@jjcoffee
Copy link
Contributor

@dylanexpensify When I go in to accept the offer I'm getting the message "This offer is not available anymore", and in an email notification from Upwork: "Your proposal was declined for the following reason:Job closed". Maybe you closed the job early?

Also, forgot that I actually also reported this bug originally, so there's the reporting bonus to add too!

@dylanexpensify
Copy link
Contributor

ah silly upwork! Hold and let me get you a new offer (and yes, will pay out reporting too! :D)

@dylanexpensify
Copy link
Contributor

@jjcoffee just sent you an offer!

@jjcoffee
Copy link
Contributor

Thanks, accepted!

@dylanexpensify
Copy link
Contributor

Alright fam, payments sent out to both of you! Thanks for your patience and apologies for my delay here!

@dylanexpensify
Copy link
Contributor

Regression Test here!

@dylanexpensify
Copy link
Contributor

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:

  • @parasharrajat @jjcoffee The PR that introduced the bug has been identified. Link to the PR: New Pronouns page #12301
  • @parasharrajat @jjcoffee 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: N/A - @Beamanator the author is aware and part of the discussion when this regression was caught.
  • @parasharrajat @jjcoffee A discussion in #contributor-plus 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:
  • Regression Test Created
  • Payments sent, contracts ended, post closed

@dylanexpensify
Copy link
Contributor

@parasharrajat @jjcoffee can you complete the above 3 tasks then we can close out!

@parasharrajat
Copy link
Member

parasharrajat commented Apr 12, 2023

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:

@parasharrajat @jjcoffee The PR that introduced the bug has been identified. Link to the PR:
Issue kind of originated since the first implementation #5643

@parasharrajat @jjcoffee 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: N/A - I think it was an edge case which is hard to card as there can be a lot of addresses that can't be tested so not the PR author's mistake.

@parasharrajat @jjcoffee A discussion in #contributor-plus 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: I don't think we could have done anything better to catch these errors. This is hard to catch edge cases and depends on geographical areas of the world.

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. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants