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-10-10] [$500] Web - Country and State fields are empty if accessed via deep link #27814

Closed
3 of 6 tasks
kbecciv opened this issue Sep 19, 2023 · 44 comments
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

@kbecciv
Copy link

kbecciv commented Sep 19, 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 App -> Navigate to Settings -> Profile -> Personal Details -> Home address
  2. Enter Address details > Save the updated address
  3. Copy address URL (https://staging.new.expensify.com/settings/profile/personal-details/address)
  4. Sign Out
  5. Paste URL and hit enter
  6. Login with your credentials
  7. 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?

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

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

@kbecciv 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
Copy link

melvin-bot bot commented Sep 19, 2023

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

@melvin-bot 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
Copy link

melvin-bot bot commented Sep 19, 2023

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

@melvin-bot 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
Copy link

melvin-bot bot commented Sep 19, 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

@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 19, 2023

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

@saranshbalyan-1234
Copy link
Contributor

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?

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.
const [state, setState] = useState(address.state);

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

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
Copy link

lcsvcn commented Sep 19, 2023

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:

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

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:

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
Copy link
Contributor

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?

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

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

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

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
Copy link
Contributor

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

@eVoloshchak
Copy link
Contributor

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
Copy link

melvin-bot bot commented Sep 21, 2023

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

@saranshbalyan-1234
Copy link
Contributor

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

@melvin-bot melvin-bot bot added the Overdue label Sep 24, 2023
@eVoloshchak
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Sep 24, 2023
@saranshbalyan-1234
Copy link
Contributor

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

@eVoloshchak
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

@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 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 melvin-bot bot removed the Reviewing Has a PR in review label Oct 3, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2023

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.

  • 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:

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
Copy link

melvin-bot bot commented Oct 3, 2023

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
Copy link
Contributor

saranshbalyan-1234 commented Oct 3, 2023

@ufumerfarooq
Copy link

@saranshbalyan-1234 yes, you are right.

@saranshbalyan-1234
Copy link
Contributor

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

@ufumerfarooq
Copy link

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

@saranshbalyan-1234
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@saranshbalyan-1234
Copy link
Contributor

Thanks for explaining :)

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Oct 10, 2023

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
Copy link

ufumerfarooq commented Oct 10, 2023

@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
Copy link
Contributor

applied @Christinadobrzyn please check

@Christinadobrzyn
Copy link
Contributor

@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
Copy link
Contributor

Christinadobrzyn commented Oct 10, 2023

@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
Copy link
Contributor

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

@saranshbalyan-1234
Copy link
Contributor

@Christinadobrzyn thank you

@eVoloshchak
Copy link
Contributor

  • 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
Copy link
Contributor

Regression Test Proposal

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

Do we agree 👍 or 👎

@JmillsExpensify
Copy link

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

@Christinadobrzyn
Copy link
Contributor

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

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

Closing!

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

10 participants