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

Restore transaction when not saving distance in edit page #32334

Merged
merged 13 commits into from
Dec 8, 2023
45 changes: 40 additions & 5 deletions src/components/DistanceRequest/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import useThemeStyles from '@styles/useThemeStyles';
import variables from '@styles/variables';
import * as MapboxToken from '@userActions/MapboxToken';
import * as Transaction from '@userActions/Transaction';
import * as TransactionEdit from '@userActions/TransactionEdit';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import DistanceRequestFooter from './DistanceRequestFooter';
Expand All @@ -38,6 +39,9 @@ const propTypes = {
/** Are we editing an existing distance request, or creating a new one? */
isEditingRequest: PropTypes.bool,

/** Are we editing a new distance request */
isEditingNewRequest: PropTypes.bool,

/** Called on submit of this page */
onSubmit: PropTypes.func.isRequired,

Expand All @@ -61,17 +65,17 @@ const defaultProps = {
transactionID: '',
report: {},
isEditingRequest: false,
isEditingNewRequest: false,
transaction: {},
};

function DistanceRequest({transactionID, report, transaction, route, isEditingRequest, onSubmit}) {
function DistanceRequest({transactionID, report, transaction, route, isEditingRequest, isEditingNewRequest, onSubmit}) {
const styles = useThemeStyles();
const {isOffline} = useNetwork();
const {translate} = useLocalize();

const [optimisticWaypoints, setOptimisticWaypoints] = useState(null);
const [hasError, setHasError] = useState(false);
const isEditing = Navigation.getActiveRoute().includes('address');
const reportID = lodashGet(report, 'reportID', '');
const waypoints = useMemo(() => optimisticWaypoints || lodashGet(transaction, 'comment.waypoints', {waypoint0: {}, waypoint1: {}}), [optimisticWaypoints, transaction]);
const waypointsList = _.keys(waypoints);
Expand All @@ -90,12 +94,38 @@ function DistanceRequest({transactionID, report, transaction, route, isEditingRe
const haveValidatedWaypointsChanged = !_.isEqual(previousValidatedWaypoints, validatedWaypoints);
const isRouteAbsentWithoutErrors = !hasRoute && !hasRouteError;
const shouldFetchRoute = (isRouteAbsentWithoutErrors || haveValidatedWaypointsChanged) && !isLoadingRoute && _.size(validatedWaypoints) > 1;
const transactionWasSaved = useRef(false);

useEffect(() => {
MapboxToken.init();
return MapboxToken.stop;
}, []);

useEffect(() => {
if (!isEditingNewRequest && !isEditingRequest) {
return () => {};
}
// This effect runs when the component is mounted and unmounted. It's purpose is to be able to properly
// discard changes if the user cancels out of making any changes. This is accomplished by backing up the
// original transaction, letting the user modify the current transaction, and then if the user ever
// cancels out of the modal without saving changes, the original transaction is restored from the backup.

// On mount, create the backup transaction.
if (isEditingNewRequest || isEditingRequest) {
TransactionEdit.createBackupTransaction(transaction);
}

return () => {
// If the user cancels out of the modal without without saving changes, then the original transaction
// needs to be restored from the backup so that all changes are removed.
if (transactionWasSaved.current || (!isEditingNewRequest && !isEditingRequest)) {
return;
}
TransactionEdit.restoreOriginalTransactionFromBackup(transaction.transactionID);
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

useEffect(() => {
const transactionWaypoints = lodashGet(transaction, 'comment.waypoints', {});
if (!lodashGet(transaction, 'transactionID') || !_.isEmpty(transactionWaypoints)) {
Expand Down Expand Up @@ -134,7 +164,7 @@ function DistanceRequest({transactionID, report, transaction, route, isEditingRe
}, [waypoints, previousWaypoints]);

const navigateBack = () => {
Navigation.goBack(isEditing ? ROUTES.MONEY_REQUEST_CONFIRMATION.getRoute(iouType, reportID) : ROUTES.HOME);
Navigation.goBack(isEditingNewRequest ? ROUTES.MONEY_REQUEST_CONFIRMATION.getRoute(iouType, reportID) : ROUTES.HOME);
};

/**
Expand Down Expand Up @@ -182,8 +212,13 @@ function DistanceRequest({transactionID, report, transaction, route, isEditingRe
setHasError(true);
return;
}

if (isEditingNewRequest || isEditingRequest) {
transactionWasSaved.current = true;
}

onSubmit(waypoints);
}, [onSubmit, setHasError, hasRouteError, isLoadingRoute, isLoading, validatedWaypoints, waypoints]);
}, [onSubmit, setHasError, hasRouteError, isLoadingRoute, isLoading, validatedWaypoints, waypoints, isEditingNewRequest, isEditingRequest]);

const content = (
<>
Expand Down Expand Up @@ -238,7 +273,7 @@ function DistanceRequest({transactionID, report, transaction, route, isEditingRe
</>
);

if (!isEditing) {
if (!isEditingNewRequest) {
Comment on lines -241 to +274
Copy link
Contributor

@neil-marcellini neil-marcellini Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change, would you please explain it and include a comment in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neil-marcellini We add isEditingNewRequest prop in NewDistanceRequestPage to centralize the onSubmit logic on this page so we replace all isEditing with isEditingNewRequest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok yes I understand isEditingNewRequest is true when we click the distance field from the confirmation screen while creating a new request. This condition is deciding whether to return only content, or to wrap it with a screen wrapper and header. We don't need the header for the new distance request page, nor for the editing request page, but we are missing a header when editing a new distance request with the current change 😕
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dukenv0307 please fix this bug

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neil-marcellini Not see any header is missing from my testing.

web21.mov

return content;
}

Expand Down
23 changes: 0 additions & 23 deletions src/pages/EditRequestDistancePage.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import useNetwork from '@hooks/useNetwork';
import usePrevious from '@hooks/usePrevious';
import Navigation from '@libs/Navigation/Navigation';
import * as IOU from '@userActions/IOU';
import * as TransactionEdit from '@userActions/TransactionEdit';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import reportPropTypes from './reportPropTypes';
Expand Down Expand Up @@ -52,7 +51,6 @@ const defaultProps = {
function EditRequestDistancePage({report, route, transaction, transactionBackup}) {
const {isOffline} = useNetwork();
const {translate} = useLocalize();
const transactionWasSaved = useRef(false);
const hasWaypointError = useRef(false);
const prevIsLoading = usePrevious(transaction.isLoading);

Expand All @@ -66,26 +64,6 @@ function EditRequestDistancePage({report, route, transaction, transactionBackup}
}
}, [transaction, prevIsLoading, report]);

useEffect(() => {
// This effect runs when the component is mounted and unmounted. It's purpose is to be able to properly
// discard changes if the user cancels out of making any changes. This is accomplished by backing up the
// original transaction, letting the user modify the current transaction, and then if the user ever
// cancels out of the modal without saving changes, the original transaction is restored from the backup.

// On mount, create the backup transaction.
TransactionEdit.createBackupTransaction(transaction);

return () => {
// If the user cancels out of the modal without without saving changes, then the original transaction
// needs to be restored from the backup so that all changes are removed.
if (transactionWasSaved.current) {
return;
}
TransactionEdit.restoreOriginalTransactionFromBackup(transaction.transactionID);
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

/**
* Save the changes to the original transaction object
* @param {Object} waypoints
Expand All @@ -101,7 +79,6 @@ function EditRequestDistancePage({report, route, transaction, transactionBackup}
return;
}

transactionWasSaved.current = true;
IOU.editMoneyRequest(transaction, report.reportID, {waypoints});

// If the client is offline, then the modal can be closed as well (because there are no errors or other feedback to show them
Expand Down
17 changes: 15 additions & 2 deletions src/pages/iou/NewDistanceRequestPage.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import React, {useEffect} from 'react';
import React, {useCallback, useEffect} from 'react';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import DistanceRequest from '@components/DistanceRequest';
import Navigation from '@libs/Navigation/Navigation';
import reportPropTypes from '@pages/reportPropTypes';
import * as IOU from '@userActions/IOU';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import {iouPropTypes} from './propTypes';

const propTypes = {
Expand Down Expand Up @@ -44,6 +46,8 @@ const defaultProps = {
// You can't use Onyx props in the withOnyx mapping, so we need to set up and access the transactionID here, and then pass it down so that DistanceRequest can subscribe to the transaction.
function NewDistanceRequestPage({iou, report, route}) {
const iouType = lodashGet(route, 'params.iouType', 'request');
const reportID = lodashGet(route, 'params.reportID', '');
const isEditing = Navigation.getActiveRoute().includes('address');

useEffect(() => {
if (iou.transactionID) {
Expand All @@ -52,12 +56,21 @@ function NewDistanceRequestPage({iou, report, route}) {
IOU.setUpDistanceTransaction();
}, [iou.transactionID]);

const onSubmit = useCallback(() => {
if (isEditing) {
Navigation.goBack(ROUTES.MONEY_REQUEST_CONFIRMATION.getRoute(iouType, reportID));
return;
}
IOU.navigateToNextPage(iou, iouType, report);
}, [iou, iouType, isEditing, report, reportID]);

return (
<DistanceRequest
report={report}
route={route}
isEditingNewRequest={isEditing}
transactionID={iou.transactionID}
onSubmit={() => IOU.navigateToNextPage(iou, iouType, report)}
onSubmit={onSubmit}
/>
);
}
Expand Down