-
Notifications
You must be signed in to change notification settings - Fork 3k
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-10-10] [$500] Web - Country and State fields are empty if accessed via deep link #27814
Comments
Job added to Upwork: https://www.upwork.com/jobs/~010def5669f0fd1df5 |
Triggered auto assignment to @peterdbarkerUK ( |
Bug0 Triage Checklist (Main S/O)
|
Triggered auto assignment to @Christinadobrzyn ( |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak ( |
ProposalPlease re-state the problem that we are trying to solve in this issue."Country" and "State/Province" values are missing while using deeplink What is the root cause of that problem?
The address object is initially empty, which is causing the issue as we are not setting the state and country states once the address object is changed.
What changes do you think we should make in order to solve the problem? useEffect(() => {
setState(address.state)
setCurrentCountry(address.country)
}, [address]);
Also we can move country state from
to this const [currentCountry, setCurrentCountry] = useState(PersonalDetails.getCountryISO(address.country)); What alternative solutions did you explore? (Optional)N/A |
ProposalPlease re-state the problem that we are trying to solve in this issueCountry and State fields are empty if accessed via deep link What is the root cause of that problem?The issue arises from the fact that the address.state and address.country values are being fetched after the initial state initialization. This delay in fetching the values is due to the presence of an isLoading variable. Consequently, when the initial state is set, it is based on an empty JSON object because the actual values have not yet been retrieved. This results in incorrect initialization with empty values, which should ideally reflect the user's address information. What changes do you think we should make in order to solve the problem?I recommend initializing all fields (such as state, country, firstStreet, and secondStreet) with empty values initially. Then, use the useEffect hook to update these fields only when the isLoadingPersonalDetails variable indicates that the data retrieval process has completed. Please note that both of these lines in the code:
Are unnecessary, as they set values when the address data is still loading. This approach simplifies the code and ensures that we always work with the most up-to-date values. At
We can keep the
Because they currently aren't using This is a video working locally with the changes that I mention above. Videovideo.bug.movHere are my suggested changes for review If my proposal is accepted, I can open a PR in a few hours to fix this. Thank you! What alternative solutions did you explore? (Optional)N/A |
ProposalPlease re-state the problem that we are trying to solve in this issue.Country and State fields are empty if accessed via deep link What is the root cause of that problem?Firstly, a small question: Why only country and State fields are empty?
Let's see here, we only use useState for country and state field. After the default value of useState is updated, the state value is not updated. It causes this issue. This is BUG 1 We only use useState for country and state fields because we need to re-set 2 fields when changing country App/src/pages/settings/Profile/PersonalDetails/AddressPage.js Lines 119 to 129 in 70e9fa0
I think the city field also should be clear after changing country like state field. This is BUG 2 What changes do you think we should make in order to solve the problem?Firstly, we need to use useState for city field
When country change we need to re-set city to empty like this
and then in here
we should use value instead of default value
After that, we need to update country, state, city when address change
ResultScreen-Recording-2023-09-20-at-15.39.23.mp4 |
This can be reproduced- @eVoloshchak let me know what you think of any of the provided proposals |
I think we should proceed with @saranshbalyan-1234's proposal There are some additional suggestions from @lcsvcn and @DylanDylann that extend @saranshbalyan-1234's proposal, but I think the proposals aren't significantly different, @saranshbalyan-1234's proposal was the first to correctly identify the root cause and propose an appropriate fix
@DylanDylann, I see where you're coming from, but I believe this is by design. We don't clear anything besides state when changing a country, it's not necessarily linear, you can populate all of the fields in any order 🎀👀🎀 C+ reviewed! |
Triggered auto assignment to @techievivek, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@techievivek @eVoloshchak can you check, as this is not being updated. |
@saranshbalyan-1234, it will be updated on Monday I think |
@eVoloshchak am I still eligible for bonus as internal member is taking time to review? |
You haven't been assigned to this job yet, so the countdown hasn't started |
Hey @techievivek can you check this proposal review to see if you're okay with us moving forward with assigning the job to @saranshbalyan-1234 #27814 (comment) |
@eVoloshchak In this PR, we apply logic to reset the city field when changing country. So please help to fix this bug with the city field in the PR phrase |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.76-6 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-10-10. 🎊 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.
For reference, here are some details about the assignees on this issue:
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:
|
@eVoloshchak @ufumerfarooq this PR caused this regression https://github.com/Expensify/App/pull/28362/files |
@saranshbalyan-1234 yes, you are right. |
do i need to fix this in my PR or the one who caused this regression will fix? |
Don't know either, @eVoloshchak can guide you better |
also, this PR i think is eligible for bonus, as PR is merged within 5 days, including weekends which means 3 business days. |
@saranshbalyan-1234, the PR that caused the regressing should fix this |
@eVoloshchak can you please check this, and tell if i am wrong |
Agree, according to my calculations this was merged in 46 hours (96 minus 48, since there were two weekend days) |
Thanks for explaining :) |
Payouts due after we get the checklist done: Issue Reporter: $50 @ufumerfarooq (Paid in Upwork) Eligible for 50% #urgency bonus? Y - based on #27814 (comment) Upwork job is here. @eVoloshchak can you complete the checklist for us? |
@Christinadobrzyn can you please guide me on how the payouts work for reporters? From past issues, it seems like some reporters get 50 and some get 250 and 500 |
applied @Christinadobrzyn please check |
@ufumerfarooq Happy to clarify. We recently changed our payments:
|
@saranshbalyan-1234 I think you'll see an offer now, I'll hire you for the $500 and include the $250 bonus (so the payment will be $750 but the breakdown on the offer will be different). |
@eVoloshchak can you complete the #27814 (comment) for us? Thanks! |
@Christinadobrzyn thank you |
|
Regression Test Proposal
Do we agree 👍 or 👎 |
$750 payment approved for @eVoloshchak based on the BZ summary. |
regression test here - https://github.com/Expensify/Expensify/issues/325802 Payment is complete - based on - #27814 (comment) Closing! |
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:
"Country" and "State/Province" values should be shown if saved by user
Actual Result:
"Country" and "State/Province" values are missing given that user's saved the values
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.70.7
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
country_state_value_missing.mov
Recording.4612.mp4
Expensify/Expensify Issue URL:
Issue reported by: @ufumerfarooq
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695140296239169
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: