[HOLD for payment 2023-06-01] [$1000] The Home Address State/Province field is autofilled with the country for UK addresses

kavimuru commented 1 year ago

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:

Go to Settings -> Profile -> Personal details -> Home Address
In Address line 1 type "5 New Street" and select the result with "Edinburgh"
Note that the state is "Scotland" which is a country and not a state

Expected Result:

State/Province field should only be populated with a state or county

Actual Result:

State/Province field is populated with a country

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

Version Number: 1.3.12.0 Reproducible in staging?: y Reproducible in production?: 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: Any additional supporting documentation

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0101458ae8ee232f2f
Upwork Job ID: 1656130500883591168
Last Price Increase: 2023-05-10

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] 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
[x] 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.
[x] 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.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

kavimuru commented 1 year ago

Proposal

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

We want to ensure that State/Province field is filled with a state or province, rather than a country.

What is the root cause of that problem?

The administrative_area_level_1 param that we're currently using from addressComponents to fill out the State/Province field is actually the country (England, Wales, Scotland, etc.) for UK addresses. According to Google's docs, administrative_area_level_1 is only really "guaranteed" to be a state for US addresses.

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

For UK addresses, we should use administrative_area_level_2 instead. So we need to pull it in here, maybe calling it stateFallback: https://github.com/Expensify/App/blob/2bdeca5460f22610bfd18e5ef44ff3c56b2c2e6a/src/components/AddressSearch/index.js#L109-L130 Making sure in the getAddressComponents call that we pull in the long_name as we are not using the state picker for non-US addresses. So we'll get something like: const { street_number: streetNumber, route: streetName, subpremise, locality, sublocality, postal_town: postalTown, postal_code: zipCode, administrative_area_level_1: state, administrative_area_level_2: stateFallback, country, } = GooglePlacesUtils.getAddressComponents(addressComponents, { street_number: 'long_name', route: 'long_name', subpremise: 'long_name', locality: 'long_name', sublocality: 'long_name', postal_town: 'long_name', postal_code: 'long_name', administrative_area_level_1: 'short_name', administrative_area_level_2: 'long_name', country: 'short_name', }); Then we use the field to override the state for UK addresses only, which can go after this: https://github.com/Expensify/App/blob/2bdeca5460f22610bfd18e5ef44ff3c56b2c2e6a/src/components/AddressSearch/index.js#L163-L167 // UK addresses return the country (e.g. England) in the state field we use for other countries if (country === 'GB') { values.state = stateFallback; }

Test addresses

Non-US addresses where administrative_area_level_1 IS the state/province:

Kurt-Schwitters-Platz 1
19 Chem. de Courcelle
Salagatan 29A UK addresses where administrative_area_level_1 is country:
5 New Street, Edinburgh
5 New Street Square, London
Main St, Huby, York

kadiealexander commented 1 year ago

Able to reproduce:

kadiealexander commented 1 year ago

@kavimuru could you please tag the person who submitted the proposal in the post?

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~0101458ae8ee232f2f

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

jjcoffee commented 1 year ago

@kadiealexander That one's mine (see here). I'm going to repost as it looks like some of the formatting went wonky!

jjcoffee commented 1 year ago

Proposal

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

We want to ensure that State/Province field is filled with a state or province, rather than a country.

What is the root cause of that problem?

The administrative_area_level_1 param that we're currently using from addressComponents to fill out the State/Province field is actually the country (England, Wales, Scotland, etc.) for UK addresses. According to Google's docs, administrative_area_level_1 is only really "guaranteed" to be a state for US addresses.

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

For UK addresses, we should use administrative_area_level_2 instead. So we need to pull it in here, maybe calling it stateFallback:

https://github.com/Expensify/App/blob/2bdeca5460f22610bfd18e5ef44ff3c56b2c2e6a/src/components/AddressSearch/index.js#L109-L130

Making sure in the getAddressComponents call that we pull in the long_name as we are not using the state picker for non-US addresses. So we'll get something like:

const {
    street_number: streetNumber,
    route: streetName,
    subpremise,
    locality,
    sublocality,
    postal_town: postalTown,
    postal_code: zipCode,
    administrative_area_level_1: state,
    administrative_area_level_2: stateFallback,
    country,
} = GooglePlacesUtils.getAddressComponents(addressComponents, {
    street_number: 'long_name',
    route: 'long_name',
    subpremise: 'long_name',
    locality: 'long_name',
    sublocality: 'long_name',
    postal_town: 'long_name',
    postal_code: 'long_name',
    administrative_area_level_1: 'short_name',
    administrative_area_level_2: 'long_name',
    country: 'short_name',
});

Then we use the field to override the state for UK addresses only, which can go after this:

https://github.com/Expensify/App/blob/2bdeca5460f22610bfd18e5ef44ff3c56b2c2e6a/src/components/AddressSearch/index.js#L163-L167

// UK addresses return the country (e.g. England) in the state field we use for other countries
if (country === 'UK') {
    values.state = stateFallback;
}

Test addresses

Non-US addresses where administrative_area_level_1 IS the state/province:

Kurt-Schwitters-Platz 1
19 Chem. de Courcelle
Salagatan 29A

UK addresses where administrative_area_level_1 is country:

5 New Street, Edinburgh
5 New Street Square, London
Main St, Huby, York

MonilBhavsar commented 1 year ago

We discussed in the bug report slack thread, and the expected behavior is - For UK addresses, we won't display country(England/Wales/Scotland/Northern Ireland) in state/province field, instead get the real state/province from google API cc @mollfpr

mollfpr commented 1 year ago

Sorry for the delay! I'll start reviewing the proposals!

mollfpr commented 1 year ago

@MonilBhavsar checked the @jjcoffee proposal, and it works well. We can go with their proposal!.

🎀 👀 🎀 C+ reviewed!

MonilBhavsar commented 1 year ago

Looks good to me too 👍

melvin-bot[bot] commented 1 year ago

📣 @jjcoffee You have been assigned to this job by @MonilBhavsar! 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 commented 1 year ago

@MonilBhavsar @mollfpr PR is ready!

melvin-bot[bot] commented 1 year ago

@mollfpr, @jjcoffee, @MonilBhavsar, @kadiealexander Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

jjcoffee commented 1 year ago

Chill Melv - we're just waiting for production deploy

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

https://github.com/Expensify/App/pull/19046

If no regressions arise, payment will be issued on 2023-06-01. :confetti_ball:

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

melvin-bot[bot] commented 1 year ago

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:

[x] [@mollfpr] The PR that introduced the bug has been identified. Link to the PR: n/a as discussed here.
[x] [@mollfpr] 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 as discussed here.
[x] [@mollfpr] 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 as discussed here.
[x] [@mollfpr] Determine if we should create a regression test for this bug.
[x] [@mollfpr] 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.
[x] [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/287980

mollfpr commented 1 year ago

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR: [@mollfpr] 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:

No offending PR. This is an edge case and improvement for UK addresses.

[@mollfpr] 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:

Regression should be enough.

[@mollfpr] Determine if we should create a regression test for this bug.

Indeed.

[@mollfpr] 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.

Propose regression step

Go to Settings -> Profile -> Personal details -> Home Address
In Address line 1 type "5 New Street" and select the result with "Edinburgh"
Ensure that the state/province field is "Edinburgh"
👍 or 👎

kadiealexander commented 1 year ago

Contracts sent to @mollfpr and @jjcoffee :)

Assigned: May 17th 3am GMT+12 Merged: May 19th 5:35pm GMT+12

Qualifies for the speed bonus :)

jjcoffee commented 1 year ago

@kadiealexander Accepted, thanks! Also just to note that the reporting bonus is also due for me.

kadiealexander commented 1 year ago

Paid $1750 to @jjcoffee (Job + speed bonus + reporting bonus) Paid $1500 to @mollfpr (job + speed bonus)

Thanks for the awesome work team!

Expensify / App