[HOLD for payment 2023-10-10] [$500] Web - Country and State fields are empty if accessed via deep link #27814

kbecciv · 2023-09-19T21:07:30Z

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:

Open App -> Navigate to Settings -> Profile -> Personal Details -> Home address
Enter Address details > Save the updated address
Copy address URL (https://staging.new.expensify.com/settings/profile/personal-details/address)
Sign Out
Paste URL and hit enter
Login with your credentials
Home address page opens but "Country" and "State" values are missing

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

melvin-bot · 2023-09-19T21:07:35Z

Job added to Upwork: https://www.upwork.com/jobs/~010def5669f0fd1df5

melvin-bot · 2023-09-19T21:07:36Z

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

melvin-bot · 2023-09-19T21:07:38Z

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

melvin-bot · 2023-09-19T21:07:40Z

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

melvin-bot · 2023-09-19T21:07:46Z

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

saranshbalyan-1234 · 2023-09-19T21:21:55Z

Proposal

Please 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?

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Line 70 in f6efb26

const address = lodashGet(privatePersonalDetails, 'address') || {};

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.

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Line 73 in f6efb26

const [state, setState] = useState(address.state);

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Line 65 in f6efb26

 const [currentCountry, setCurrentCountry] = useState(PersonalDetails.getCountryISO(lodashGet(privatePersonalDetails, 'address.country'))); 

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

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Line 65 in f6efb26

 const [currentCountry, setCurrentCountry] = useState(PersonalDetails.getCountryISO(lodashGet(privatePersonalDetails, 'address.country'))); 

to this

    const [currentCountry, setCurrentCountry] = useState(PersonalDetails.getCountryISO(address.country));

What alternative solutions did you explore? (Optional)

N/A

lcsvcn · 2023-09-19T22:45:14Z

Proposal

Please 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?

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:

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Line 65 in f6efb26

 const [currentCountry, setCurrentCountry] = useState(PersonalDetails.getCountryISO(lodashGet(privatePersonalDetails, 'address.country'))); 

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Line 73 in f6efb26

const [state, setState] = useState(address.state);

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 src/pages/settings/Profile/PersonalDetails/AddressPage.js:

    const address = lodashGet(privatePersonalDetails, 'address') || {};
    const isLoadingPersonalDetails = lodashGet(privatePersonalDetails, 'isLoading', true);

    const [state, setState] = useState('');
    const [country, setCountry] = useState('');
    const [firstStreet, setFirstStreet] = useState('');
    const [secondStreet, setSecondStreet] = useState('');
    
    useEffect(() => {
       if(isLoadingPersonalDetails === true) { 
            return;
        }
        
        const street =  (address.street || '').split('\n');
        
        setState(address.state);
        setCountry(address.country);
        setFirstStreet(street[0]);
        setFirstStreet(street[1]);
    }, [address, isLoadingPersonalDetails]);

We can keep the street1, street2 as it is in the line:

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Line 72 in f6efb26

const [street1, street2] = (address.street || '').split('\n');

Because they currently aren't using setState. I would suggest changing to the setState, even tho we are not changing theirs states, and use in the useEffect to avoid future bugs similar to this, because street1 and street2 are using outdated values. But that part is optional, since it work fine without changing them, so that part is completely optional and up to the review to see if they agree with the modification or not. If we would not change we just change the state and country part of the implementation that I suggested.

This is a video working locally with the changes that I mention above.

Video

video.bug.mov

Here 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

DylanDylann · 2023-09-20T08:42:29Z

Proposal

Please 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?

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Line 73 in 70e9fa0

const [state, setState] = useState(address.state);

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Line 65 in 70e9fa0

 const [currentCountry, setCurrentCountry] = useState(PersonalDetails.getCountryISO(lodashGet(privatePersonalDetails, 'address.country'))); 

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

 const handleAddressChange = (value, key) => { 

 if (key !== 'country' && key !== 'state') { 

 return; 

 } 

 if (key === 'country') { 

 setCurrentCountry(value); 

 setState(''); 

 return; 

 } 

 setState(value); 

 };

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

const [city, setCity] = useState(address.city);

When country change we need to re-set city to empty like this

const handleAddressChange = (value, key) => {
        if (key !== 'country' && key !== 'state' && key !== 'city') {          // UPDATE HERE
            return;
        }
        if (key === 'country') {
            setCurrentCountry(value);
            setState('');
            setCity('');         // UPDATE HERE
            return;
        }
        setState(value);
        setCity(value);

    };

and then in here

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Line 211 in 70e9fa0

defaultValue={address.city || ''}

we should use value instead of default value

value={city || ''}

After that, we need to update country, state, city when address change

useEffect(() => {
        setState(address.state);
        setCity(address.city);
        setCurrentCountry(PersonalDetails.getCountryISO(address.country));
    }, [address])

Result

Screen-Recording-2023-09-20-at-15.39.23.mp4

Christinadobrzyn · 2023-09-20T18:53:23Z

This can be reproduced- @eVoloshchak let me know what you think of any of the provided proposals

eVoloshchak · 2023-09-21T14:51:20Z

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

When country change we need to re-set city to empty like this

@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!

melvin-bot · 2023-09-21T14:51:27Z

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

saranshbalyan-1234 · 2023-09-24T18:05:46Z

@techievivek @eVoloshchak can you check, as this is not being updated.

eVoloshchak · 2023-09-24T19:34:15Z

@saranshbalyan-1234, it will be updated on Monday I think

saranshbalyan-1234 · 2023-09-24T19:36:21Z

@eVoloshchak am I still eligible for bonus as internal member is taking time to review?

eVoloshchak · 2023-09-24T19:38:56Z

Payment timelines are based on the day and timestamp the contributor is assigned to the Github issue by an Expensify employee

You haven't been assigned to this job yet, so the countdown hasn't started

Christinadobrzyn · 2023-09-26T17:34:37Z

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)

DylanDylann · 2023-09-28T08:13:25Z

@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

melvin-bot · 2023-10-03T15:39:28Z

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

melvin-bot · 2023-10-03T15:39:33Z

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:

rerender country and state if visited via deep link #28404

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.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

@eVoloshchak does not require payment (Eligible for Manual Requests)
@saranshbalyan-1234 requires payment
@ufumerfarooq requires payment

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 · 2023-10-03T15:39:35Z

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

saranshbalyan-1234 · 2023-10-03T15:45:38Z

@eVoloshchak @ufumerfarooq this PR caused this regression https://github.com/Expensify/App/pull/28362/files

ufumerfarooq · 2023-10-03T15:49:49Z

@saranshbalyan-1234 yes, you are right.

saranshbalyan-1234 · 2023-10-03T15:50:39Z

do i need to fix this in my PR or the one who caused this regression will fix?

ufumerfarooq · 2023-10-03T15:51:12Z

Don't know either, @eVoloshchak can guide you better

saranshbalyan-1234 · 2023-10-03T17:25:37Z

also, this PR i think is eligible for bonus, as PR is merged within 5 days, including weekends which means 3 business days.
Please check

eVoloshchak · 2023-10-03T19:33:13Z

do i need to fix this in my PR or the one who caused this regression will fix?

@saranshbalyan-1234, the PR that caused the regressing should fix this

saranshbalyan-1234 · 2023-10-03T19:35:25Z

also, this PR i think is eligible for bonus, as PR is merged within 5 days, including weekends which means 3 business days. Please check

@eVoloshchak can you please check this, and tell if i am wrong

eVoloshchak · 2023-10-03T19:46:29Z

Agree, according to my calculations this was merged in 46 hours (96 minus 48, since there were two weekend days)

saranshbalyan-1234 · 2023-10-03T20:39:04Z

Thanks for explaining :)

Christinadobrzyn · 2023-10-10T19:28:13Z

Payouts due after we get the checklist done:

Issue Reporter: $50 @ufumerfarooq (Paid in Upwork)
Contributor: $500 + $250 bonus @saranshbalyan-1234 (Paid in Upwork)
Contributor+: $500 + $250 bonus @eVoloshchak (paid outside Upwork

Eligible for 50% #urgency bonus? Y - based on #27814 (comment)

Upwork job is here.

@eVoloshchak can you complete the checklist for us?

ufumerfarooq · 2023-10-10T19:35:00Z

@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

saranshbalyan-1234 · 2023-10-10T20:09:57Z

applied @Christinadobrzyn please check

Christinadobrzyn · 2023-10-10T20:26:14Z

@ufumerfarooq Happy to clarify. We recently changed our payments:

Any GHs created after Aug 30th - the reporter gets $50
Any GHs created before Aug 30th - the reporter received $250 (old pricing and there are still some open GHs before Aug 30th)
If a contributor is also the reporter, the contributor will be paid as the reporter and also as the contributor (so $50 + $500, etc).

Christinadobrzyn · 2023-10-10T20:28:35Z

@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).

Christinadobrzyn · 2023-10-10T20:33:10Z

@eVoloshchak can you complete the #27814 (comment) for us? Thanks!

saranshbalyan-1234 · 2023-10-10T20:35:40Z

@Christinadobrzyn thank you

eVoloshchak · 2023-10-12T14:40:20Z

The PR that introduced the bug has been identified. Link to the PR: there isn't a PR that caused this bug, it was present in the initial implementation
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
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: I don't think an additional discussion is needed, this is a case that was missed during the initial implementation. We already have a reviewer checklist item precisely for these types of bugs: If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.

eVoloshchak · 2023-10-12T14:41:22Z

Regression Test Proposal

Go to Settings -> Profile -> Personal Details -> Home address
Fill out all the details
Log out
Open https://staging.new.expensify.com/settings/profile/personal-details/address
Log in and verify that state and country fields are populated

Do we agree 👍 or 👎

JmillsExpensify · 2023-10-12T18:42:52Z

$750 payment approved for @eVoloshchak based on the BZ summary.

Christinadobrzyn · 2023-10-12T20:43:00Z

regression test here - https://github.com/Expensify/Expensify/issues/325802

Payment is complete - based on - #27814 (comment)

Closing!

kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 19, 2023

melvin-bot bot changed the title ~~Web - Country and State fields are empty if accessed via deep link~~ [$500] Web - Country and State fields are empty if accessed via deep link Sep 19, 2023

melvin-bot bot assigned peterdbarkerUK Sep 19, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 19, 2023

melvin-bot bot assigned Christinadobrzyn Sep 19, 2023

melvin-bot bot assigned eVoloshchak Sep 19, 2023

melvin-bot bot assigned techievivek Sep 21, 2023

abzokhattab mentioned this issue Sep 22, 2023

[HOLD for payment 2023-10-16] [HOLD for payment 2023-10-10] [$500] Web - City field is not clear when select another country #28007

Closed

6 tasks

melvin-bot bot added the Overdue label Sep 24, 2023

melvin-bot bot removed the Overdue label Sep 24, 2023

trjExpensify mentioned this issue Sep 25, 2023

[HOLD for payment 2023-10-09] [$500] Android - State gets cleared when selecting the same country #28034

Closed

6 tasks

melvin-bot bot changed the title ~~[$500] Web - Country and State fields are empty if accessed via deep link~~ [HOLD for payment 2023-10-10] [$500] Web - Country and State fields are empty if accessed via deep link Oct 3, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Oct 3, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 10, 2023

lcsvcn mentioned this issue Oct 10, 2023

Contact methods- Changes will be reverted if added after deep linking or if quick opening the settings on slow network #29179

Closed

6 tasks

Christinadobrzyn closed this as completed Oct 12, 2023

DylanDylann mentioned this issue Oct 25, 2023

[$500] Profile - Address - Zip-code is saved when changing country but the city is cleared #30304

Closed

6 tasks

[HOLD for payment 2023-10-10] [$500] Web - Country and State fields are empty if accessed via deep link #27814

[HOLD for payment 2023-10-10] [$500] Web - Country and State fields are empty if accessed via deep link #27814

Comments

kbecciv commented Sep 19, 2023 • edited by Christinadobrzyn Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Sep 19, 2023

melvin-bot bot commented Sep 19, 2023

melvin-bot bot commented Sep 19, 2023 • edited by Christinadobrzyn Loading

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Sep 19, 2023

melvin-bot bot commented Sep 19, 2023

saranshbalyan-1234 commented Sep 19, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

lcsvcn commented Sep 19, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

DylanDylann commented Sep 20, 2023

Proposal

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

What is the root cause of that problem?

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

Result

Christinadobrzyn commented Sep 20, 2023

eVoloshchak commented Sep 21, 2023

melvin-bot bot commented Sep 21, 2023

saranshbalyan-1234 commented Sep 24, 2023

eVoloshchak commented Sep 24, 2023

saranshbalyan-1234 commented Sep 24, 2023

eVoloshchak commented Sep 24, 2023

Christinadobrzyn commented Sep 26, 2023

DylanDylann commented Sep 28, 2023

melvin-bot bot commented Oct 3, 2023

melvin-bot bot commented Oct 3, 2023 • edited by Christinadobrzyn Loading

melvin-bot bot commented Oct 3, 2023 • edited by Christinadobrzyn Loading

saranshbalyan-1234 commented Oct 3, 2023 • edited Loading

ufumerfarooq commented Oct 3, 2023

saranshbalyan-1234 commented Oct 3, 2023

ufumerfarooq commented Oct 3, 2023

saranshbalyan-1234 commented Oct 3, 2023

eVoloshchak commented Oct 3, 2023

saranshbalyan-1234 commented Oct 3, 2023

eVoloshchak commented Oct 3, 2023

saranshbalyan-1234 commented Oct 3, 2023

Christinadobrzyn commented Oct 10, 2023 • edited Loading

ufumerfarooq commented Oct 10, 2023 • edited Loading

saranshbalyan-1234 commented Oct 10, 2023

Christinadobrzyn commented Oct 10, 2023

Christinadobrzyn commented Oct 10, 2023 • edited Loading

Christinadobrzyn commented Oct 10, 2023

saranshbalyan-1234 commented Oct 10, 2023

eVoloshchak commented Oct 12, 2023

eVoloshchak commented Oct 12, 2023

JmillsExpensify commented Oct 12, 2023

Christinadobrzyn commented Oct 12, 2023

kbecciv commented Sep 19, 2023 •

edited by Christinadobrzyn

Loading

melvin-bot bot commented Sep 19, 2023 •

edited by Christinadobrzyn

Loading

lcsvcn commented Sep 19, 2023 •

edited

Loading

melvin-bot bot commented Oct 3, 2023 •

edited by Christinadobrzyn

Loading

melvin-bot bot commented Oct 3, 2023 •

edited by Christinadobrzyn

Loading

saranshbalyan-1234 commented Oct 3, 2023 •

edited

Loading

Christinadobrzyn commented Oct 10, 2023 •

edited

Loading

ufumerfarooq commented Oct 10, 2023 •

edited

Loading

Christinadobrzyn commented Oct 10, 2023 •

edited

Loading