[HOLD for payment 2025-01-17] [$250] Add confirmation dialog on unsaved changes

[dubielzyk-expensify]

Problem

When users fill in details in the app (e.g. description on an expense) and tap back or the scrim/overlay by mistake, the user loses everything they filled it. This can be particularly annoying if you submit an expense and fill in multiple details only to misclick and have to start over. We saw this multiple times during our usability study.

Solution

To remedy this, let's add a confirmation dialog on these sort of interactions by throwing up a confirmation dialog for unsaved changes. Here's a video to demonstrate:

CleanShot.2024-12-06.at.15.13.09.mp4

Notes:

• If the value isn't changed, we should show the warning.
• Clicking Discard continues the action of the user (if back, go back. If scrim/overlay, closer the right-hand-pane) without saving the value.
• Any movement away from the screen should trigger the confirmation dialog (given the value has changed)

Questions:

• Can this be applied systematically or do we have to go through all flows? If not, we should probably start with Submit expense to try it out and continue from there.

cc @Expensify/design @JmillsExpensify @trjExpensify

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021864985847950506437
• Upwork Job ID: 1864985847950506437
• Last Price Increase: 2024-12-06
• Automatic offers:
• ahmedGaber93 | Reviewer | 105308658

Issue Owner

Current Issue Owner: @abekkala

[nkdengineer]

Edited by proposal-police: This proposal was edited at 2024-12-06 05:23:17 UTC.

Proposal

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

Add confirmation dialog on unsaved changes

What is the root cause of that problem?

New feature

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

1. Create a new component DiscardChangeModal based on ConfirmModal
2. Create a new state like isShowDiscardChangeModal
3. Will set isShowDiscardChangeModal as true when moving away from the screen without saving the value.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

NA

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[melvin-bot]

Triggered auto assignment to @abekkala (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

[melvin-bot]

⚠️ It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time ⚠️

[trjExpensify]

@dubielzyk-expensify make sure you add Bug or New feature labels to issues like this, to get it assigned out to a BZ member to manage and progress.

[melvin-bot]

Triggered auto assignment to Design team member for new feature review - @dubielzyk-expensify (NewFeature)

[melvin-bot]

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

[melvin-bot]

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

[Shahidullah-Muffakir]

Proposal

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

Add confirmation dialog on unsaved changes

What is the root cause of that problem?

New Feature

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

If we start from IOURequestStepDescription page,

App/src/pages/iou/request/step/IOURequestStepDescription.tsx

Line 57 in a9bf599

function IOURequestStepDescription({

1. we can use the already existing ConfirmModal
2. modify the function used for onBackButtonPress

App/src/pages/iou/request/step/IOURequestStepDescription.tsx

Line 139 in a9bf599

onBackButtonPress={navigateBack}

to use a modified function as:

    const handleNavigation = () => {
        if (newDescription !== currentDescription) {
            setIsConfirmationModalVisible(true);
        } else {
            Navigation.goBack(backTo);
        }
    };

5. add this:

  const [isConfirmationModalVisible, setIsConfirmationModalVisible] = useState(false);

       <ConfirmModal
                  title={'Discard Changes?'}
                  prompt='Are you sure you want to discard the changes you made?'
                  isVisible={isConfirmationModalVisible}
                  onConfirm={()=>{
                      setIsConfirmationModalVisible(false)
                      Navigation.goBack(backTo);

                  }}
                  onCancel={()=>{
                      setIsConfirmationModalVisible(false)
                  }}
                  confirmText={'Discard Changed'}
                  cancelText={'Cancel'}
                  danger
              />

we should create translation for the above used texts.
We can add the same ConfirmModal in all the pages needed.

Screen.20Recording.202024-12-06.20at.205.mp4

[bernhardoj]

Edited by proposal-police: This proposal was edited at 2024-12-10 07:13:19 UTC.

Proposal

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

We want to show a discard changes confirmation when closing the page if there is an unsaved changes.

What is the root cause of that problem?

New feature.

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

Since we want to apply this to many pages, we can create a new component called DiscardChangesConfirmation. This component will make use of beforeRemove event and prevent default if there is an unsaved changes. If the user press cancel, then we simply close the confirm modal, else, we continue with the navigation action that is previously prevented.

const DiscardChangesConfirmation = ({getHasUnsavedChanges}) => {
    const navigation = useNavigation();
    const [isVisible, setIsVisible] = useState(false);
    const preventedNavigationAction = useRef();

    const handlePreventRemove = useCallback((e) => {
        if (!getHasUnsavedChanges()) {
            return;
        }

        e.preventDefault();
        blockedNavigationAction.current = e.data.action;
        setIsVisible(true);
    }, [getHasUnsavedChanges]);

    useEffect(() => {
        navigation.addListener('beforeRemove', handlePreventRemove);
        return () => navigation.removeListener('beforeRemove', handlePreventRemove);
    }, [handlePreventRemove]);

    return (
        <ConfirmModal
            isVisible={isVisible}
            title='Discard changes?'
            prompt='Are you sure?'
            danger
            confirmText='Discard changes'
            cancelText='Cancel'
            onConfirm={() => {
                setIsVisible(false);
                navigation.dispatch(preventedNavigationAction.current)
            }}
            onCancel={() => setIsVisible(false)}
        />
    )
}

Then, to use this in the description page for example, we need to update the input first to be a controlled input.

App/src/pages/iou/request/step/IOURequestStepDescription.tsx

Lines 153 to 158 in 7d42188

	<InputWrapper
	InputComponent={TextInput}
	inputID={INPUT_IDS.MONEY_REQUEST_COMMENT}
	name={INPUT_IDS.MONEY_REQUEST_COMMENT}
	defaultValue={currentDescription}
	label={translate('moneyRequestConfirmationList.whatsItFor')}

const [description, setDescription] = useState(currentDescription);

<InputWrapper
    valueType="string"
    value={description}
    onValueChange={setDescription}

Then, add a isSavedRef to indicate that the user saves the value so we don't show discard confirm modal when the user saves the changes.

const isSavedRef = useRef(false);

And set it to true here.

App/src/pages/iou/request/step/IOURequestStepDescription.tsx

Lines 101 to 102 in 8981081

	const updateComment = (value: FormOnyxValues<typeof ONYXKEYS.FORMS.MONEY_REQUEST_DESCRIPTION_FORM>) => {
	const newComment = value.moneyRequestComment.trim();

And use the new component like this:

<DiscardChangesConfirmation
    getHasUnsavedChanges={() => {
        if (isSavedRef.current) {
            return false;
        }
        return description !== currentDescription
    }}
/>

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

[melvin-bot]

📣 @bjmmtin! 📣
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. Make sure you've read and understood the contributing guidelines.
2. 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.
3. 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.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[bjmmtin]

react-hook-form simplifies form management by providing easy-to-use hooks for registering inputs, handling submissions, and tracking form state.
useBlocker effectively prevents users from accidentally losing unsaved changes by prompting them with a modal alert before navigating away from the form.

import React, { useState } from 'react';
import { useForm } from 'react-hook-form';
import { useBlocker } from 'react-router-dom';

const MyForm = () => {
  const { register, handleSubmit, formState: { isDirty } } = useForm({
    defaultValues: { name: "", email: "" }
  });
  
  const [alertModal, setAlertModal] = useState({ show: false, title: "", content: "" });
  const [proceed, setProceed] = useState(false);
  const [navigateTo, setNavigateTo] = useState(null);

  const onSubmit = (data) => {
    console.log(data);
    // Reset the form or perform any other action after submission
  };

  const handleNavigation = ({ nextLocation }) => {
    if (isDirty) {
      setProceed(true);
      setNavigateTo(nextLocation.pathname);
      setAlertModal({
        show: true,
        title: "You have unsaved changes.",
        content: "All unsaved changes will be lost.",
      });
      return false; // Prevent navigation
    }
    return true; // Allow navigation
  };

  useBlocker(handleNavigation);

  const handleAlertConfirm = () => {
    setProceed(false);
    setAlertModal({ show: false, title: "", content: "" });
    // Navigate to the next location if proceed is true
    if (navigateTo) {
      // Use your navigation logic here, e.g., using useNavigate from react-router-dom
      // navigate(navigateTo);
    }
  };

  return (
    <div>
      {alertModal.show && (
        <div className="alert-modal">
          <h2>{alertModal.title}</h2>
          <p>{alertModal.content}</p>
          <button onClick={handleAlertConfirm}>Proceed</button>
          <button onClick={() => setAlertModal({ show: false })}>Cancel</button>
        </div>
      )}
      <form onSubmit={handleSubmit(onSubmit)}>
        <input {...register('name')} placeholder="Name" />
        <input {...register('email')} placeholder="Email" />
        <button type="submit">Submit</button>
      </form>
    </div>
  );
};

export default MyForm;

[ahmedGaber93]

Reviewing today

[mkzie2]

Proposal

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

Add confirmation dialog on unsaved changes

What is the root cause of that problem?

New feature

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

• Test branch: https://github.com/mkzie2/App/tree/mkzie2-issue/53679

1. Create a new DiscardChangesModal that uses ConfirmModal. To detect when the screen is removed, we can use the beforeRemove event. If we click on the confirm button, continue the navigate action. If we click on the cancel button, close this modal.

2. To apply this pattern to all pages, we can add this in FormWrapper then all pages that use a form always has this behavior.

• To do that firstly we need to detect whenever the value is changed. Define the initInputValuesRef to store the form's initial value. That will be the first value that is not an empty object of inputValues. Introduce a new prop isValueChanged in FormWrapper here and pass it here isValueChanged={!deepEqual(inputValues, initInputValuesRef.current)}.

const initInputValuesRef = useRef<Form>();

useEffect(() => {
    if (!!initInputValuesRef.current || isEmptyObject(inputValues)) {
        return;
    }

    initInputValuesRef.current = inputValues;
}, [inputValues]);

• Then we can add DiscardChangesModal here with hasUnsavedChanges={!isValueChanged}

<DiscardChangesModal hasUnsavedChanges={!isValueChanged} />

App/src/components/Form/FormWrapper.tsx

Line 125 in 37105a1

/>

Optional: If we don't want to apply for all forms of input, we can introduce a new prop in FormProvider shouldUseDiscardChangesModal and will only render DiscardChangesModal if shouldUseDiscardChangesModal is true. Then we will pass shouldUseDiscardChangesModal as true to FormProvider in all places that we want to use this feature.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[ahmedGaber93]

@bernhardoj I love your idea using beforeRemove, And I see you used it before here.

But I have some question:

1. How we will prevent the beforeRemove that will trigger after click save?
2. What is about the limitation with @react-navigation/native-stack (back gesture doesn't trigger beforeRemove event with native-stack)? This limitation has been fixed in version 7.x, but we use version 6.9

[bjmmtin]

now we are using the high version react-router-dom,
It's impossible to use the beforeRemove, it can't prevent the navigation and won't trigger after click save.
we can implement it only by using useBlocker.

[bernhardoj]

How we will prevent the beforeRemove that will trigger after click save?

Updated to handle this case

What is about the limitation with @react-navigation/native-stack (back gesture doesn't trigger beforeRemove event with native-stack)? This limitation has been fixed in version 7.x, but we use version 6.9

I think the only option we have is to disable the swipe gesture or update the navigation to 7.0 and use usePreventRemove hook.

[bernhardoj]

PR is ready

cc: @ahmedGaber93

[melvin-bot]

This issue has not been updated in over 15 days. @abekkala, @ahmedGaber93, @stitesExpensify, @bernhardoj, @dubielzyk-expensify eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[ahmedGaber93]

Deployed to staging

[melvin-bot]

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

[melvin-bot]

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

• Show discard confirmation modal when going back from a page with unsaved changes #54176

If no regressions arise, payment will be issued on 2025-01-17. 🎊

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

• @ahmedGaber93 requires payment automatic offer (Reviewer)
• @bernhardoj requires payment through NewDot Manual Requests

[melvin-bot]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ahmedGaber93] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@abekkala] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[ahmedGaber93]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ahmedGaber93] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.

Regression Test Proposal

1. Open submit expense flow page
2. Enter amount and proceed to the confirmation page
3. Open the Description form and keep the input empty
4. Go back
5. Verify the page closes
6. Open the page again
7. Type anything
8. Go back
9. Verify there is a discard confirmation modal
10. Press Cancel
11. Verify the modal closes, but the page doesn't close
12. Repeat step 8-9
13. Press Discard changes
14. Verify the modal and page closes
15. Repeat step 6-7
16. Save the changes
17. Verify the page closes and the change is saved
18. Repeat the same test (step 3-17) for Merchant field and Verify it has the same behavior in steps 5, 9, 11, 14, 17.

[abekkala]

PAYMENT SUMMARY FOR JAN 17

• Fix: @bernhardoj [$250, if no regressions] Payment via NewDot
• PR Review: @ahmedGaber93 [$250, if no regressions] Payment via NewDot

[abekkala]

@ahmedGaber93 irt to the regression step proposal. I feel like there should be a Step 19 which verifies the expected outcome Verify..., if you're repeating steps wouldn't there be another outcome that should be verified? is that correct?

[bernhardoj]

Requested in ND.

[ahmedGaber93]

Repeat step 3-17 but for Merchant field

@abekkala steps from 3-17 already contain Verify steps 5, 9, 11, 14, 17. But I agree it need to be more clear.

- Repeat step 3-17 but for Merchant field
+ Repeat the same test (step 3-17) for Merchant field and Verify it has the same behavior in steps 5, 9, 11, 14, 17.

Updated!

[abekkala]

this got a little buried so I'll paste again for better visibility:

PAYMENT SUMMARY FOR JAN 17

• Fix: @bernhardoj [$250] Payment via NewDot
• PR Review: @ahmedGaber93 [$250] Payment via NewDot

[JmillsExpensify]

$250 approved for @bernhardoj

[JmillsExpensify]

$250 approved for @ahmedGaber93