search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.49k stars 2.85k forks source link

[HOLD for payment 2024-10-29] [$250] Switch to OD - Exit survey reason is not cleared after dismissing RHP and page refresh #50215

Open IuliiaHerets opened 2 weeks ago

IuliiaHerets commented 2 weeks ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: v9.0.44-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): applausetester+shsb22cp111@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Login to any account
  2. Navigate to settings > Switch to Expensify Classic
  3. Select any of the options > Next > Fill in the next form with any text > Next
  4. Before clicking on "Switch to Expensify Classic" dismiss the RHP by clicking back or anywhere on the page
  5. Refresh your browser > Navigate to settings > Switch to Expensify Classic
  6. Here notice that the previously selected answer is displayed as selected
  7. Click Next, here notice that the next page field is empty
  8. Navigate back to the first flow > Change to any other option > Next
  9. Here notice that the previously field out text (before refreshing) is displayed

Expected Result:

The previously selected choice and the filled text is cleared once the field is dismissed and the page is refreshed

Actual Result:

Step 6. The previously selected answer is displayed as selected with a checkmark Step 9. The previously filled-out answer is displayed

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/daed7433-8575-4b5c-987e-672d6df3115c

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021843834311761874490
  • Upwork Job ID: 1843834311761874490
  • Last Price Increase: 2024-10-09
  • Automatic offers:
    • suneox | Reviewer | 104410311
    • truph01 | Contributor | 104410313
Issue OwnerCurrent Issue Owner: @anmurali
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

IuliiaHerets commented 2 weeks ago

@anmurali FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

abzokhattab commented 2 weeks ago

Proposal

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

The exit survey reason is not cleared after dismissing the RHP and refreshing the page.

What is the root cause of that problem?

We don't reset the draft reason when dismissing the modal here .

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

  1. We should add a clearDraftReason utility function to the ExitSurvey functions. This function will set the Onyx value of EXIT_SURVEY_REASON_FORM_DRAFT to null when the component is mounted.
function clearDraftReason() {
    Onyx.set(ONYXKEYS.FORMS.EXIT_SURVEY_REASON_FORM_DRAFT, null);
}
  1. Then, call this function in the Exit Survey Reason page:
useEffect(() => {
    ExitSurvey.clearDraftReason();
}, []);

  1. Remove || !draftReason from this useEffect .

  2. Remove shouldSaveDraft from the InputWrapper .

POC

https://github.com/user-attachments/assets/cd61a58a-d6f1-477c-9e12-9c350406af60

What alternative solutions did you explore? (Optional)

truph01 commented 2 weeks ago

Edited by proposal-police: This proposal was edited at 2024-10-11 03:01:16 UTC.

Proposal

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

What is the root cause of that problem?

https://github.com/Expensify/App/blob/4ffbd422296227ae003bfe0b5e584188bae5d68d/src/pages/settings/ExitSurvey/ExitSurveyReasonPage.tsx#L89

and

https://github.com/Expensify/App/blob/4ffbd422296227ae003bfe0b5e584188bae5d68d/src/pages/settings/ExitSurvey/ExitSurveyResponsePage.tsx#L141

we always use shouldSaveDraft, which will always store the current input value to the draft. So

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

https://github.com/Expensify/App/blob/4ffbd422296227ae003bfe0b5e584188bae5d68d/src/pages/settings/ExitSurvey/ExitSurveyResponsePage.tsx#L110 to:

    const validate = (values) => {
        const errors = ValidationUtils.getFieldRequiredErrors(values, [INPUT_IDS.RESPONSE]);
        return errors;
    };

since previously, it used the data from draft.

https://github.com/Expensify/App/blob/4ffbd422296227ae003bfe0b5e584188bae5d68d/src/pages/settings/ExitSurvey/ExitSurveyResponsePage.tsx#L64-L67 to:

    const submitForm = useCallback((values) => {
        ExitSurvey.saveResponse(values.response);   Navigation.navigate(ROUTES.SETTINGS_EXIT_SURVEY_CONFIRM.getRoute(ROUTES.SETTINGS_EXIT_SURVEY_RESPONSE.route));
    }, []);

What alternative solutions did you explore? (Optional)

function resetExitForm() {
    Onyx.set(ONYXKEYS.FORMS.EXIT_SURVEY_REASON_FORM, null);
    Onyx.set(ONYXKEYS.FORMS.EXIT_SURVEY_REASON_FORM_DRAFT, null);
    Onyx.set(ONYXKEYS.FORMS.EXIT_SURVEY_RESPONSE_FORM, null);
    Onyx.set(ONYXKEYS.FORMS.EXIT_SURVEY_RESPONSE_FORM_DRAFT, null);
}
                              action() {
                                  resetExitForm();
                                  Navigation.navigate(ROUTES.SETTINGS_EXIT_SURVEY_REASON);
                              },

or 

                              action() {
                                  resetExitForm();
                                  InteractionManager.runAfterInteractions(() => {
                                      Navigation.navigate(ROUTES.SETTINGS_EXIT_SURVEY_REASON);
                                  });
                              },

For the bug "The draft value saves for a response is not shown when we refresh the page and go to the response page"

As we can see, the data for inputValues comes from draftValues, which is populated by useOnyx. When draftValues is still loading, it will be undefined, causing the state const [inputValues, setInputValues] = useState<Form>(() => ({...draftValues})) to initialize with an empty object {}.

https://github.com/Expensify/App/blob/abdfb7233e040a61d1e4433e202a8b644c7455f4/src/components/Form/FormProvider.tsx#L92

    const [draftValues, draftValuesResult] = useOnyx<OnyxFormDraftKey, Form>(`${formID}Draft`);
    const didInitValuesRef = useRef(false);
    const isLoadingDraftValue = isLoadingOnyxValue(draftValuesResult);
    useEffect(() => {
        if (isLoadingDraftValue || didInitValuesRef.current) {
            return;
        }
        didInitValuesRef.current = true;
        setInputValues({...draftValues});
    }, [draftValues, isLoadingDraftValue]);

With it, we only set the inputValues if the draftValues is loaded successfully.

melvin-bot[bot] commented 2 weeks ago

@anmurali Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

anmurali commented 2 weeks ago

It feels like a nice to have, but sure, let's fix it.

suneox commented 1 week ago

Thanks for all the proposals. In this flow, we have implemented a draft functionality, and we need to retain this feature. We’re still looking for a proposal that maintains the draft functionality—we just need to ensure that we can start a new flow clearly.

truph01 commented 1 week ago

Proposal updated

suneox commented 1 week ago

The alternative solution from @truph01 proposal LGTM. It retains the draft functionality and only resets the flow when we start a new process as same the IOU.startMoneyRequest flow

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 week ago

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

abzokhattab commented 1 week ago

But i think we use the clearDraft on mounting in multiple locations: isnt it better to do it on the component level ? no?

we have this pattern in multiple locations, so that is why i preferred clearing using useEffect as i suggested to keep consistency:

cc @arosiclair @suneox

suneox commented 1 week ago

@abzokhattab Most of your reference code clears specific data upon mount but it doesn’t address clearing draft data or only clears draft data with specific logic. Therefore, it isn't a common pattern for clear draft data when starting a new flow.

abzokhattab commented 1 week ago

@abzokhattab Most of your reference code clears specific data upon mount but it doesn’t address clearing draft data or only clears draft data with specific logic. Therefore, it isn't a common pattern for clear draft data when starting a new flow.

Thanks for the prompt review i've cleaned the referenced examples ... but i think this approach makes the code more modular and keeps the clearing logic within the relevant component, reducing side effects and aligning with patterns already used in similar components like CreateReportFieldsPage and SubscriptionSize, ... . It keeps the codebase cleaner since the clearing logic is added on the component itself not by another component .

but lets see what @arosiclair thinks

Thanks again for your feedback!!

twilight2294 commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-10-11 09:39:50 UTC.

Proposal

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

Actual Bug: The draft value saves for a response is not shown when we refresh the page and go to the response page

What is the root cause of that problem?

We are only meant to clear the draft values when the user actual goes to OD on the Confirm page:

https://github.com/user-attachments/assets/c4440c77-416e-4c31-8787-6d703c935160

So here the actual bug is that when we fill out the reason and response then go to confirm page and click outside RHP without pressing confirm, then refresh and go back to flow, the draft value for response is not shown:

https://github.com/user-attachments/assets/56577f26-b0b5-4b18-8c87-f18df89e6ffa

the values should only be cleared when we click confirm and go to OD.

Here is the step where we set the values to null: https://github.com/Expensify/App/blob/285bde8073f84dad0ca6d63ef6ec251b10e52882/src/pages/settings/ExitSurvey/ExitSurveyConfirmPage.tsx#L84-L87

https://github.com/Expensify/App/blob/abdfb7233e040a61d1e4433e202a8b644c7455f4/src/libs/actions/ExitSurvey.ts#L32-L54

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

So to solve the actual bug here, we should display the draft value of the response we can do that by setting passing value prop to InputWrapper here:


--- a/src/pages/settings/ExitSurvey/ExitSurveyResponsePage.tsx
+++ b/src/pages/settings/ExitSurvey/ExitSurveyResponsePage.tsx

@@ -114,6 +115,7 @@ function ExitSurveyResponsePage({route, navigation}: ExitSurveyResponsePageProps
                         <InputWrapper
                             InputComponent={TextInput}
                             inputID={INPUT_IDS.RESPONSE}
+                            value={draftResponse}
                             label={translate(`exitSurvey.responsePlaceholder`)}
                             accessibilityLabel={translate(`exitSurvey.responsePlaceholder`)}
                             role={CONST.ROLE.PRESENTATION}

This way we actually address the bug.

NOTE: This is the default behaviour we follow throughout the application with all forms

What alternative solutions did you explore? (Optional)

twilight2294 commented 1 week ago

@suneox @arosiclair can you please review my proposal, I think the acutal bug here is that the draft value is not displayed for response, I have provided the solution along with video as well please check.

Even in the video attached in the bug description, you can see that the tester doesn't get the draft value for response they have to go back select another option and come back that is when they see the draft value

Also, the convention to pass value prop is followed throughout our app in major components like AddressForm: https://github.com/Expensify/App/blob/abdfb7233e040a61d1e4433e202a8b644c7455f4/src/components/AddressForm.tsx#L182

TextSelector:

https://github.com/Expensify/App/blob/abdfb7233e040a61d1e4433e202a8b644c7455f4/src/components/TextPicker/TextSelectorModal.tsx#L106

suneox commented 1 week ago

@suneox @arosiclair can you please review my proposal, I think the acutal bug here is that the draft value is not displayed for response, I have provided the solution along with video as well please check.

@twilight294 Sure I'll start review your proposal within today

twilight2294 commented 1 week ago

Thank you 😄

truph01 commented 1 week ago

Proppsal updated

suneox commented 1 week ago

Thanks for the update. Let’s ensure we clarify and follow the expected behavior: * The previously selected choice * and * the filled text is cleared * once the field is dismissed (at step 4) and the page is refreshed (at step 5)

The latest proposals from @abzokhattab and @twilight294 still doesn't match the expected result.

cc: @arosiclair

twilight2294 commented 1 week ago

@suneox that is what i am trying to put forward here, the actual bug is different then mentioned in the OP :)), If you check the card shipping missingDetails RHP flow, you can observe the same behaviour

suneox commented 1 week ago

@suneox that is what i am trying to put forward here, the actual bug is different then mentioned in the OP :)), If you check the card shipping missingDetails RHP flow, you can observe the same behaviour

@twilight294 I’m not sure about this. Let’s wait for the internal team to make a decision. If the expected behavior doesn't reset values when starting a new flow, maybe we can go with your solution.

truph01 commented 1 week ago

If the expected behavior doesn't reset values when starting a new flow

@suneox Do you have any feedback about my updated solution?

suneox commented 1 week ago

If the expected behavior doesn't reset values when starting a new flow

@suneox Do you have any feedback about my updated solution?

@truph01 Your solution about reload page doesn't show draftValue can override the value or defaultValue has passed into the InputWrapper in other places after draftValue is loaded. Since this issue only occurs on this specific page, and we don't have context on another page so we should avoid making changes that could impact almost all forms. We need to wait for internal team feedback on the expected result

truph01 commented 1 week ago

Your solution can override the value or defaultValue has passed into the InputWrapper in other places after draftValue is loaded

But we only want to set the value for defaultValue after the draftValue has loaded, right? Otherwise, while the draftValue is still loading, its value will be undefined, leading to the bug we mentioned in this issue.

Since this issue only occurs on this specific page, and we don't have context on another page

A similar bug can be reproduced in /settings/subscription/request-early-cancellation-survey, when we select any option and then refresh page, that option is not loaded as the default value:

https://github.com/user-attachments/assets/37b8f47f-fa3e-4df4-94fc-649bf06a2718

We need to wait for internal team feedback on the https://github.com/Expensify/App/issues/50215#issuecomment-2407722373

Sure, I just need to share my solution about that bug since you commented "If the expected behavior doesn't reset values when starting a new flow, maybe we can go with your solution."

arosiclair commented 1 week ago

The alternative solution from @truph01 proposal LGTM. It retains the draft functionality and only resets the flow when we start a new process as same the IOU.startMoneyRequest flow

🎀 👀 🎀 C+ reviewed

I agree with this. Let's reset the form if the user cancels. The draft can persist if they refresh without canceling.

melvin-bot[bot] commented 1 week ago

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

Offer link Upwork job

melvin-bot[bot] commented 1 week ago

📣 @truph01 🎉 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 📖

truph01 commented 1 week ago

PR is ready

melvin-bot[bot] commented 2 days ago

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

melvin-bot[bot] commented 2 days ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.51-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-10-29. :confetti_ball:

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

melvin-bot[bot] commented 2 days ago

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: