-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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-04-05] [$1000] State is not updated when address outside USA is entered when the state picker is displayed #15633
Comments
Triggered auto assignment to @arielgreen ( |
Bug0 Triage Checklist (Main S/O)
|
@arielgreen Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@arielgreen Whoops! This issue is 2 days overdue. Let's get this updated quick! |
This was already reported by Ashim Sharma here; asked for his GH handle so we can give proper credit here. |
Job added to Upwork: https://www.upwork.com/jobs/~01208ef7601c16165a |
Current assignee @arielgreen is eligible for the External assigner, not assigning anyone new. |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane ( |
Triggered auto assignment to @techievivek ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.In this issue, when the StatePicker dropdown is visible, if we were to input an address based in the USA, the state input changes to a textInput but without the state displayed. What is the root cause of that problem?The root cause behind the issue lies in the way the state input changes when a new address is entered. App/src/pages/settings/Profile/PersonalDetails/AddressPage.js Lines 179 to 191 in b703eac
When we enter a USA based address, the state picker has a value assigned to it which is used to select the state in the dropdown. When we manually change the country to any other outside the USA, it correctly displays the previous value. However, when a new address is selected from outside the USA, the state picker defaults to an empty value. This happens because of the value being overwritten by the Form component. In this component, each part of the address has it's value changed one by one: App/src/components/AddressSearch.js Lines 145 to 148 in b703eac
The App/src/components/AddressSearch.js Lines 123 to 129 in b703eac
EDIT
What changes do you think we should make in order to solve the problem?The fix can be as simple as changing the country's value first and then the state's value. This can be achieved by reordering the What alternative solutions did you explore? (Optional)EDIT if (!_.findKey(this.props.items, item => item.value === value)) {
return;
} This will allow the value to not default to an empty one when any value outside the list of items is encountered. However, I do not recommend this option since this issue is the ONLY occurrence of this issue with the Picker component. There are many other components that either directly or indirectly make use of the |
ProposalPlease re-state the problem that we are trying to solve in this issue.Re-selecting an address on address search page causes the state to not change. What is the root cause of that problem?The root cause of the issue is an incorrect render of the What changes do you think we should make in order to solve the problem?I think we should solve the issue in the Picker component. If the value supplied does not exist in the list of items that are supplied, then we should log a warning in the console and NOT update the component. The following can be added to the Picker component:
We'll also need the component to extend Alternatively, we can also prevent the extra re-render by creating a fix upstream in the What alternative solutions did you explore? (Optional)Changing the order of fields in the |
ProposalPlease re-state the problem that we are trying to solve in this issue.State is not updated when address outside USA is entered when the state picker is displayed #15633 What is the root cause of that problem?For some countries, the state picker is not displayed. For these countries, the state field is a text field. When a user enters a state outside the US, the state field is not updated as the data returned from the google API is not consistent. What changes do you think we should make in order to solve the problem?We would have to make changes to the google API to return the state code instead of the state name. This would allow us to update the state field when a user enters a state outside the US. What alternative solutions did you explore? (Optional)Doing the mapping of each country response form the google to know where our required data is located. |
📣 @logiclingusitic! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Format:
|
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
@ashimsharma10 FYI |
|
Contributor details |
✅ Contributor details stored successfully. Thank you for contributing to Expensify! |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.91-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2023-04-05. 🎊 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.
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
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:
|
Posting the timeline for this issue here for an overview, calculated using this tool: Issue timeline analysis: 💰 Timeline Bonus/Penalty: 50% Bonus! 🎉 |
Nice @Prince-Mendiratta 👍 👍 👍 |
Would be awesome if we can integrate this into Melvin. |
Thanks @allroundexperts! @mountiny will try to work on this when he has some extra bandwidth 💯 |
@Prince-Mendiratta this is very very cool! |
Issuing the following payments: |
@rushatgabhane what are your thoughts on whether or not a regression test would be valuable here? |
Sure, let's add a regression test for it.
|
Sounds good! Creating an issue. @rushatgabhane @techievivek can you please complete the checklist so we can close this issue out? |
Done. We don't need to start a discussion because this is not related to a regression, so we are all good here. Thanks, @arielgreen. |
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:
Expected Result:
State should be set automatically
Actual Result:
State field is left empty
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.78-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:
2023-02-24.23-00-29.mp4
Recording.1618.mp4
Expensify/Expensify Issue URL:
Issue reported by: @ashimsharma10 @Prince-Mendiratta
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1677259948200949
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: