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

[C+ Checklist Needs Completion] [$1000] The State filled is not cleared after selecting another country #23839

Closed
1 of 6 tasks
kavimuru opened this issue Jul 28, 2023 · 42 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Jul 28, 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. Go profile > Personal detail > Home Address Page
  2. Click on State and choose any state
  3. Now click on Country and select a country other than the United States.
    Notice that the state filled shows an abbreviation of the previously selected State

Expected Result:

The State filled is cleared

Actual Result:

The State filled is not cleared. Shows abbreviation of previously selected State

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.47-3
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

Test92.Stateabrrivation-1.mp4
Recording.1400.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01f0fbcbd5a20b5cfa
  • Upwork Job ID: 1686035992494268416
  • Last Price Increase: 2023-07-31
  • Automatic offers:
    • daveSeife | Reporter | 25899761
    • dukenv0307 | Contributor | 25996636
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 28, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 28, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 28, 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

@jeet-dhandha
Copy link
Contributor

jeet-dhandha commented Jul 29, 2023

Proposal

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

  • Currently we are selecting State first (when default Country is US) and then selecting other Country lets say India, then State is not getting empty.

What is the root cause of that problem?

  • We are not resetting the state input in form when we are changing the country input.
  • Also even if we update the state component from Picker to TextInput the form's value won't get updated.
  • Hence showing the previous selected state.

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

  • We can implement a custom formRef which acts like a ref as the below object and it changes values when inputValues or inputRefs changes in Form component:
{  resetField: (inputID) => {... reset the input field} }
  • We can use this ref in handleAddressChange function of AddressPage component to reset the State input field.
Form.js Code Reference
useEffect(() => {
    props.formRef({
        resetField: (inputID) => {
            if (!inputRefs.current[inputID] || !inputValues[inputID]) return;
            setInputValues((prevState) => ({...prevState, [inputID]: ''}));
        },
    });
}, [inputRefs, props.formRef, inputValues]);
When using `Form` in AddressPage.js
const handleAddressChange = (value, key) => {
        if (key !== 'country') {
            return;
        }
+       if (value !== currentCountry && formRef.current && formRef.current.resetField) {
+           formRef.current.resetField('state');
+       }
        setCurrentCountry(value);
    };
...rest code

<Form ...rest
+  formRef={(ref) => formRef.current = ref}
>

What alternative solutions did you explore? (Optional)

  • N/A

@dukenv0307
Copy link
Contributor

dukenv0307 commented Jul 29, 2023

Proposal

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

The State filled is not cleared after selecting another country

What is the root cause of that problem?

We handle the change of country in handleAddressChange here and we only change the country, not clear state

const handleAddressChange = (value, key) => {
if (key !== 'country') {
return;
}
setCurrentCountry(value);

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

  1. We can create a state to control state the same as country
const [state, setState] = useState(address.state);
  1. Add onValueChange={handleAddressChange} into StatePicker and TextInput



3. Change the default value here to state

defaultValue={address.state || ''}

  1. change defaultValue prop of TextInput to value

  2. In handleAddressChange we check if the key is country we will reset the state and if the key is state we set value to it state

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

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Jul 31, 2023
@greg-schroeder
Copy link
Contributor

Reproduced. Agreed that the state field should just be cleared

@melvin-bot melvin-bot bot removed the Overdue label Jul 31, 2023
@greg-schroeder greg-schroeder added the External Added to denote the issue can be worked on by a contributor label Jul 31, 2023
@melvin-bot melvin-bot bot changed the title The State filled is not cleared after selecting another country [$1000] The State filled is not cleared after selecting another country Jul 31, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 31, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01f0fbcbd5a20b5cfa

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

melvin-bot bot commented Jul 31, 2023

Current assignee @greg-schroeder is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Jul 31, 2023

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

@allroundexperts
Copy link
Contributor

Taking this over.

@Santhosh-Sellavel
Copy link
Collaborator

Left a comment here for volunteers to take over

@Santhosh-Sellavel Santhosh-Sellavel removed their assignment Jul 31, 2023
@allroundexperts
Copy link
Contributor

@greg-schroeder Can you please assign this to me so I can start reviewing the proposals?

@saalimzafar
Copy link

Proposal

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

The problem we are trying to solve in this issue is that the "state" field is not clearing after selecting another country in the AddressPage component.

What is the root cause of that problem?

The root cause of the problem lies in the handleAddressChange function within the AddressPage component. When the country is changed, the function updates the currentCountry state but does not reset the "state" field in the privatePersonalDetails.address object, leading to the "state" field retaining its previous value when a new country is selected.

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

To solve the problem, we need to modify the handleAddressChange function to reset the "state" field when a new country is selected. We'll also add the required state management hooks and define the setPrivatePersonalDetails function to manage the privatePersonalDetails state.

Here's the updated AddressPage component with the necessary modifications:

import React, {useState} from 'react';
import lodashGet from 'lodash/get';
import {CONST} from 'expensify-common/lib/CONST';
import * as PersonalDetails from '../../../../libs/actions/PersonalDetails';
import styles from '../../../../styles/styles';
// (Add other necessary imports)

function AddressPage() {
    const [privatePersonalDetails, setPrivatePersonalDetails] = useState({
        address: {
            street: '',
            city: '',
            state: '',
            zip: '',
            country: '',
        },
    });

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

    // Rest of the component code...

    const handleAddressChange = (value, key) => {
        if (key === 'country') {
            setCurrentCountry(value);
            // Reset the "state" field to an empty string when a new country is selected
            setPrivatePersonalDetails({
                ...privatePersonalDetails,
                address: {...privatePersonalDetails.address, state: ''}
            });
        }
    };

    // Rest of the component code...
}

By adding the useState hook for privatePersonalDetails and currentCountry, we can manage the state and correctly update the "state" field when a new country is selected. This ensures that the "state" field is cleared whenever a different country is chosen.

What alternative solutions did you explore? (Optional)

NA

@ares-dev05
Copy link

Proposal

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

The issue at hand is that the "state" field fails to clear after selecting a different country on the Home Address page within the Personal Details section of the profile.

What is the root cause of that problem?

In the AddressPage.js file, there is a code snippet that handles the UI/UX behavior based on the selected country. If the country is not the United States (US), the StatePicker component is replaced with a TextInput box. However, during this transition, the TextInput box displays the previous state value for the US. This happens because both the StatePicker and TextInput components use the same 'state' value.

To address this issue, we can create a separate 'state' hook variable specifically for the TextInput box when a non-US country is selected. We can then set this 'state' variable to the TextInput box. Consequently, when the user selects a country other than the US, we can reset the TextInput box for the non-US country. Subsequently, if the user selects the US again, the previous 'state' value will be restored.

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

To address this issue, we can implement a new 'state' hook variable by using the following code:

const [currentState, setCurrentState] = useState('');

To ensure the default value is set to an empty string, we need to initialize the 'currentState' variable accordingly.

In the handleAddressChange function, we should reset the currentState variable whenever a country is selected.

Finally, we need to establish a connection between the currentState variable and the state TextInput box for non-US countries.

import lodashGet from 'lodash/get';
import _ from 'underscore';
import React, {useState, useCallback} from 'react';
...

function AddressPage({privatePersonalDetails}) {
    usePrivatePersonalDetails();
    const {translate} = useLocalize();
    const [currentCountry, setCurrentCountry] = useState(PersonalDetails.getCountryISO(lodashGet(privatePersonalDetails, 'address.country')) || CONST.COUNTRY.US);
    const [currentState, setCurrentState] = useState('');

...

 const handleAddressChange = (value, key) => {
        if (key !== 'country') {
            return;
        }
        setCurrentCountry(value);
        setCurrentState('')
    };

...
return (
        <ScreenWrapper includeSafeAreaPaddingBottom={false}>
...
{isUSAForm ? (
                    <View style={styles.mhn5}>
                        <StatePicker
                            inputID="state"
                            defaultValue={address.state}
                        />
                    </View>
                ) : (
                    <TextInput
                        inputID="state"
                        label={translate('common.stateOrProvince')}
                        accessibilityLabel={translate('common.stateOrProvince')}
                        accessibilityRole={CONST.ACCESSIBILITY_ROLE.TEXT}
                        defaultValue={currentState || ''}
                        maxLength={CONST.FORM_CHARACTER_LIMIT}
                    />
                )}
...
  </ScreenWrapper>
    );
}

What alternative solutions did you explore? (Optional)

N/A

@melvin-bot
Copy link

melvin-bot bot commented Aug 2, 2023

📣 @ares-dev05! 📣
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.
Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@melvin-bot
Copy link

melvin-bot bot commented Aug 2, 2023

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

1 similar comment
@melvin-bot
Copy link

melvin-bot bot commented Aug 2, 2023

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

@ares-dev05
Copy link

Contributor details
Your Expensify account email: godknight122@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01afd03c01b66a1606

@melvin-bot
Copy link

melvin-bot bot commented Aug 2, 2023

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

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

melvin-bot bot commented Aug 7, 2023

📣 @allroundexperts Please request via NewDot manual requests for the Reviewer role ($1000)

@melvin-bot
Copy link

melvin-bot bot commented Aug 7, 2023

📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
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 📖

@roryabraham
Copy link
Contributor

roryabraham commented Aug 7, 2023

Thanks, LGTM 👍🏼

That said, I wonder if we should think about generalizing the address form a little bit. It looks like there's a lot of overlap between src/pages/settings/Profile/PersonalDetails/AddressPage and src/pages/ReimbursementAccount/AddressForm. Seems like they should be using a shared component.

That's probably out-of-scope for this issue though.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 8, 2023
@dukenv0307
Copy link
Contributor

dukenv0307 commented Aug 8, 2023

@allroundexperts The PR is ready for review #24148

@melvin-bot
Copy link

melvin-bot bot commented Aug 8, 2023

🎯 ⚡️ Woah @allroundexperts / @dukenv0307, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

  • when @dukenv0307 got assigned: 2023-08-07 20:37:45 Z
  • when the PR got merged: 2023-08-08 23:35:28 UTC

On to the next one 🚀

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Aug 10, 2023
@melvin-bot melvin-bot bot changed the title [$1000] The State filled is not cleared after selecting another country [HOLD for payment 2023-08-17] [$1000] The State filled is not cleared after selecting another country Aug 10, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Aug 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 10, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 10, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.52-5 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-08-17. 🎊

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 Aug 10, 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:

  • [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
  • [@allroundexperts] 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:
  • [@allroundexperts] 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:
  • [@allroundexperts] Determine if we should create a regression test for this bug.
  • [@allroundexperts] 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.
  • [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@greg-schroeder
Copy link
Contributor

Issue Participants:

Issue reported by: @daveSeife
Contributor: @dukenv0307
C+: @allroundexperts

Was this issue merged in time to be eligible for the speed bonus? Yes
Were their any regressions? ❌

Payment summary:

Reporter: $250
Contributor: $1,500 ($500 bonus)
C+: $1,500 ($500 bonus)

@greg-schroeder
Copy link
Contributor

Please request payment via NewDot @allroundexperts - all that's left here is taking care of the checklist!

@greg-schroeder greg-schroeder removed the Awaiting Payment Auto-added when associated PR is deployed to production label Aug 17, 2023
@greg-schroeder greg-schroeder changed the title [HOLD for payment 2023-08-17] [$1000] The State filled is not cleared after selecting another country [HOLD for checklist] [$1000] The State filled is not cleared after selecting another country Aug 17, 2023
@greg-schroeder greg-schroeder changed the title [HOLD for checklist] [$1000] The State filled is not cleared after selecting another country [C+ Checklist Needs Completion] [$1000] The State filled is not cleared after selecting another country Aug 17, 2023
@allroundexperts
Copy link
Contributor

Requested payment in ND. Onto the checklist!

@allroundexperts
Copy link
Contributor

Checklist

  1. PR which introduced this: 17548 Personal details push to page #21049
  2. Comment on the PR: https://github.com/Expensify/App/pull/21049/files#r1299446609
  3. I think a slack discussion is not needed here because this was a edge case which could have been caught only by doing rigorous QA.
  4. A regression test might be helpful here.
  5. The test steps given in the issue description look good enough to me.

@greg-schroeder
Copy link
Contributor

Filing regression test and closing!

@JmillsExpensify
Copy link

Reviewed the details for @allroundexperts. $1,500 approved for payment in NewDot based on BZ summary above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

10 participants