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

[HOLD for payment 2023-07-14] [$1000] Web - Country field does not autofill for certain addresses. #21520

Closed
1 of 6 tasks
kavimuru opened this issue Jun 25, 2023 · 45 comments
Closed
1 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Jun 25, 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.Open your profile.
2.Click on "Settings".
3.Click on "Personal Details".
4.Click on "Home Address".
5.Notice that when selecting an address from the options menu, the country field is not automatically filled.

Expected Result:

The Country field should be auto filled, similar to when the user selects other addresses.

Actual Result:

The Country field is not auto filled for certain addresses.

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?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.32-5
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: Any additional supporting documentation

Bug.mp4
Recording.2263.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~014d5d43c97434907d
  • Upwork Job ID: 1673420349109485568
  • Last Price Increase: 2023-06-26
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 25, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 25, 2023

Bug0 Triage Checklist (Main S/O)

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

@joekaufmanexpensify
Copy link
Contributor

I can reproduce this. But it seems like it only happens when you type in the specific address bangladesh border road into the search box and then select it. I can't get this to not work for any other address (and I tried a bunch).

Part of me wonders if this has something to do with the data we're getting from the API for this address, which prevents auto-filling? Regardless, it is reproducible, so we should fix (as long as this isn't an issue with the address API that we have no control over).

@joekaufmanexpensify joekaufmanexpensify added the External Added to denote the issue can be worked on by a contributor label Jun 26, 2023
@melvin-bot melvin-bot bot changed the title Web - Country field does not autofill for certain addresses. [$1000] Web - Country field does not autofill for certain addresses. Jun 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 26, 2023

Job added to Upwork: https://www.upwork.com/jobs/~014d5d43c97434907d

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

melvin-bot bot commented Jun 26, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jun 26, 2023

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

@nishancx
Copy link
Contributor

nishancx commented Jun 27, 2023

@joekaufmanexpensify This seems like address API response data issue. Adresses are fetched from /place/autocomplete/json, the response from API contains an array address_components which contains address auto fll data.
First recommendation for bangladesh only contains addressLine1 thats why counry isn't being autofilled. Maybe some entries in address data json need to be updated.

@joekaufmanexpensify
Copy link
Contributor

@nishancx Thanks for confirming! Is that something we can update on our end. Or just an issue related to the API we use (over which we have no control).

@crystal6117
Copy link

Hello, @joekaufmanexpensify. I think this is an issue in google place data. So we have to find a solution not to show such addresses in google place autocomplete component.

@joekaufmanexpensify
Copy link
Contributor

Hm, got it. If this is an issue with the google API data, then I'm not sure we should do anything. I think we'd rather have an address not auto-complete, than remove an address that exists from the search results. Curious if you agree @mollfpr ?

@mollfpr
Copy link
Contributor

mollfpr commented Jun 28, 2023

@joekaufmanexpensify The response from Google API only returns the type of route we use for address line 1, but we still can get the country-long name from the autocomplete data.

// Make sure that the order of keys remains such that the country is always set above the state.
// Refer to https://github.com/Expensify/App/issues/15633 for more information.
const {state: stateAutoCompleteFallback = '', city: cityAutocompleteFallback = ''} = GooglePlacesUtils.getPlaceAutocompleteTerms(autocompleteData.terms);

Screenshot 2023-06-28 at 08 51 42

We can get the country name from the autocomplete fallback and get the country's short-name by searching it on CONST.ALL_COUNTRIES.

@joekaufmanexpensify
Copy link
Contributor

@mollfpr Got it, thanks! In that case, it sounds like this is something we can fix. Is that right? If so, then let's keep this open for proposals.

@nishancx
Copy link
Contributor

Proposal

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

Web - Country field does not auto-fill for certain addresses (eg. Bangladesh Border Road)

What is the root cause of that problem?

This seems like address API response data issue. Addresses are fetched from /place/autocomplete/json, the response contains an array address_components which contains address auto-fill data.
First recommendation for Bangladesh only contains addressLine1 that's why country isn't being auto-filled.

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

We are fetching country with getAddressComponents().
And we are already fetching state and city with getPlaceAutocompleteTerms() and it also returns country which we can use as a fallback.
country fetched with getPlaceAutocompleteTerms() contains full name of country, which needs to be converted to country code in order to perform checks later down in the code. This conversion can be done by using lodash to findKey from CONST.ALL_COUNTRIES.
Now that we have fallback for country code fetched from getPlaceAutocompleteTerms(), it can be used as a fallback for country fetched from getAddressComponents() to solve this issue.

@mollfpr
Copy link
Contributor

mollfpr commented Jun 29, 2023

Thanks for the proposal @nishancx

It is similar to what I have in mind, and the proposal looks good!

🎀 👀 🎀 C+ reviewed!

@melvin-bot
Copy link

melvin-bot bot commented Jun 29, 2023

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

@francoisl
Copy link
Contributor

Looks good to me!

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

melvin-bot bot commented Jun 29, 2023

📣 @mollfpr You have been assigned to this job!
Please apply to this job in Upwork here and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot
Copy link

melvin-bot bot commented Jun 29, 2023

📣 @nishancx 🎉 An offer has been automatically sent to your Upwork account 🎉

Contributor - [$1000] Web - Country field does not autofill for certain addresses. Please accept the offer 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 📖

@nishancx
Copy link
Contributor

Offer accepted.
Ill try to make a PR within 24 hours.

@nishancx
Copy link
Contributor

nishancx commented Jun 29, 2023

@francoisl @mollfpr @joekaufmanexpensify I have finished everything except testing with high traffic account, I have requested for my account (nishancx@gmail.com) to be converted to high traffic account in #expensify-open-source and I'm waiting for the request to go through.

bondydaa added a commit that referenced this issue Jul 3, 2023
Fix: Web - Country field does not autofill for certain addresses. #21520
@usmantariq96
Copy link

@kavimuru please update the "Issue reported by:" with my GH Handle
Thanks

@melvin-bot
Copy link

melvin-bot bot commented Jul 6, 2023

@francoisl, @mollfpr, @nishancx, @joekaufmanexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@joekaufmanexpensify
Copy link
Contributor

Not overdue. PR is in review.

@nishancx
Copy link
Contributor

nishancx commented Jul 7, 2023

Not overdue. PR is in review.

The PR is already deployed to production.

@francoisl
Copy link
Contributor

Ah, the automation didn't work because the pull request description doesn't follow the proper format.

image

I'm going to remove the Reviewing label like it was supposed to do after the production deploy. For the next PRs, make sure to use the format $ https://github.com/Expensify/App/issues/XXXX 🙂

@francoisl francoisl added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Jul 7, 2023
@francoisl francoisl changed the title [$1000] Web - Country field does not autofill for certain addresses. [HOLD for payment 2023-07-14] [$1000] Web - Country field does not autofill for certain addresses. Jul 7, 2023
@nishancx
Copy link
Contributor

nishancx commented Jul 8, 2023

Sure @francoisl, thank you.

@nishancx
Copy link
Contributor

nishancx commented Jul 8, 2023

When should I submit work in upwork right now or after 7 days regression period?

Also, if no regressions happen, this should be eligible for bonus right? Since PR got merged within 3 days, 23 hours with weekend in between and without counting weekend it got merded within 1 day, 23 hours of accepting proposal.

@melvin-bot melvin-bot bot added the Overdue label Jul 10, 2023
@joekaufmanexpensify
Copy link
Contributor

Yep, we'll send an offer in upwork soon. We'll issue payment on 2023-07-14. And then this does qualify for a speed bonus. That will be added when we issue payment!

@melvin-bot melvin-bot bot removed the Overdue label Jul 10, 2023
@joekaufmanexpensify
Copy link
Contributor

joekaufmanexpensify commented Jul 11, 2023

Manually creating BZ checklist, since the automation did not work here:

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:

  • [@mollfpr ] The PR that introduced the bug has been identified. Link to the PR: No offending PR is causing the issue. We were unaware there was a result from Google API that doesn't have a country value. So this is an improvement we made to handle that.
  • [@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, see above.
  • [@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: This is a one-time bug, so the regression test should be enough.
  • [@mollfpr ] Determine if we should create a regression test for this bug.
  • [@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.
  • [@joekaufmanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/300016

@joekaufmanexpensify
Copy link
Contributor

@mollfpr could you please complete your portion of the BZ checklist when you have a sec? Thanks!

@joekaufmanexpensify
Copy link
Contributor

@mollfpr offer sent for $1,000 (We'll issue $500 as bonus during payment)!

@joekaufmanexpensify
Copy link
Contributor

@usmantariq96 offer sent for $250!

@usmantariq96
Copy link

@joekaufmanexpensify offer accepted.
Thanks

@joekaufmanexpensify
Copy link
Contributor

Bumped BZ checklist in Slack. On track to issue payment tomorrow!

@mollfpr
Copy link
Contributor

mollfpr commented Jul 14, 2023

 [@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 is causing the issue. We were unaware there was a result from Google API that doesn't have a country value. So this is an improvement we made to handle that.

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

This is a one-time bug, so the regression test should be enough.

 [@mollfpr ] Determine if we should create a regression test for this bug.
 [@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.

  1. Sign in to the App
  2. Go to the Home Address page from Settings > Profile > Personal Details > Home Address
  3. Enter Bangladesh in the Address line 1 field
  4. Select an option (In this case, Bangladesh Border Road , which was causing the issue)
  5. Verify that the Country field is filled with Bangladesh
  6. 👍 or 👎

@francoisl
Copy link
Contributor

👍 for the regression steps (and the rest too)

@joekaufmanexpensify
Copy link
Contributor

Great, thanks! I updated the BZ checklist with that information. I also created a regression test issue and linked it above.

@joekaufmanexpensify
Copy link
Contributor

BZ checklist is complete. All set to issue payment here!

@joekaufmanexpensify
Copy link
Contributor

@nishancx $1,500 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

@mollfpr $1,500 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

@usmantariq96 $250 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

Upwork job closed.

@joekaufmanexpensify
Copy link
Contributor

Bug is fixed, BZ checklist complete, and all payment issued. Closing as this is all set. Thank you everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

7 participants