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

Fix receipt disappears when dismissing distance editor after receipt is generated #51519

Merged
6 changes: 6 additions & 0 deletions src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1182,7 +1182,13 @@ const CONST = {
PENDING: 'Pending',
POSTED: 'Posted',
},
STATE: {
CURRENT: 'current',
DRAFT: 'draft',
BACKUP: 'backup',
},
},

MCC_GROUPS: {
AIRLINES: 'Airlines',
COMMUTER: 'Commuter',
Expand Down
14 changes: 10 additions & 4 deletions src/hooks/useFetchRoute.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,22 @@
import isEqual from 'lodash/isEqual';
import {useEffect} from 'react';
import type {OnyxEntry} from 'react-native-onyx';
import * as IOUUtils from '@libs/IOUUtils';
import * as TransactionUtils from '@libs/TransactionUtils';
import * as TransactionAction from '@userActions/Transaction';
import type {IOUAction} from '@src/CONST';
import CONST from '@src/CONST';
import type {Transaction} from '@src/types/onyx';
import type {WaypointCollection} from '@src/types/onyx/Transaction';
import type TransactionState from '@src/types/utils/TransactionStateType';
import useNetwork from './useNetwork';
import usePrevious from './usePrevious';

export default function useFetchRoute(transaction: OnyxEntry<Transaction>, waypoints: WaypointCollection | undefined, action: IOUAction) {
export default function useFetchRoute(
transaction: OnyxEntry<Transaction>,
waypoints: WaypointCollection | undefined,
action: IOUAction,
transactionState: TransactionState = CONST.TRANSACTION.STATE.CURRENT,
) {
const {isOffline} = useNetwork();
const hasRouteError = !!transaction?.errorFields?.route;
const hasRoute = TransactionUtils.hasRoute(transaction);
Expand All @@ -27,8 +33,8 @@ export default function useFetchRoute(transaction: OnyxEntry<Transaction>, waypo
return;
}

TransactionAction.getRoute(transaction.transactionID, validatedWaypoints, IOUUtils.shouldUseTransactionDraft(action));
}, [shouldFetchRoute, transaction?.transactionID, validatedWaypoints, isOffline, action]);
TransactionAction.getRoute(transaction.transactionID, validatedWaypoints, transactionState);
}, [shouldFetchRoute, transaction?.transactionID, validatedWaypoints, isOffline, action, transactionState]);

return {shouldFetchRoute, validatedWaypoints};
}
2 changes: 2 additions & 0 deletions src/libs/API/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -907,6 +907,7 @@ const READ_COMMANDS = {
SEND_PERFORMANCE_TIMING: 'SendPerformanceTiming',
GET_ROUTE: 'GetRoute',
GET_ROUTE_FOR_DRAFT: 'GetRouteForDraft',
GET_ROUTE_FOR_BACKUP: 'GetRouteForBackup',
GET_STATEMENT_PDF: 'GetStatementPDF',
OPEN_ONFIDO_FLOW: 'OpenOnfidoFlow',
OPEN_INITIAL_SETTINGS_PAGE: 'OpenInitialSettingsPage',
Expand Down Expand Up @@ -967,6 +968,7 @@ type ReadCommandParameters = {
[READ_COMMANDS.SEND_PERFORMANCE_TIMING]: Parameters.SendPerformanceTimingParams;
[READ_COMMANDS.GET_ROUTE]: Parameters.GetRouteParams;
[READ_COMMANDS.GET_ROUTE_FOR_DRAFT]: Parameters.GetRouteParams;
[READ_COMMANDS.GET_ROUTE_FOR_BACKUP]: Parameters.GetRouteParams;
[READ_COMMANDS.GET_STATEMENT_PDF]: Parameters.GetStatementPDFParams;
[READ_COMMANDS.OPEN_ONFIDO_FLOW]: null;
[READ_COMMANDS.OPEN_INITIAL_SETTINGS_PAGE]: null;
Expand Down
52 changes: 45 additions & 7 deletions src/libs/actions/Transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import ONYXKEYS from '@src/ONYXKEYS';
import type {PersonalDetails, RecentWaypoint, ReportAction, ReportActions, ReviewDuplicates, Transaction, TransactionViolation, TransactionViolations} from '@src/types/onyx';
import type {OnyxData} from '@src/types/onyx/Request';
import type {WaypointCollection} from '@src/types/onyx/Transaction';
import type TransactionState from '@src/types/utils/TransactionStateType';

let recentWaypoints: RecentWaypoint[] = [];
Onyx.connect({
Expand Down Expand Up @@ -203,13 +204,27 @@ function removeWaypoint(transaction: OnyxEntry<Transaction>, currentIndex: strin
return Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION}${transaction?.transactionID}`, newTransaction);
}

function getOnyxDataForRouteRequest(transactionID: string, isDraft = false): OnyxData {
function getOnyxDataForRouteRequest(transactionID: string, transactionState: TransactionState = CONST.TRANSACTION.STATE.CURRENT): OnyxData {
let keyPrefix;
switch (transactionState) {
case CONST.TRANSACTION.STATE.DRAFT:
keyPrefix = ONYXKEYS.COLLECTION.TRANSACTION_DRAFT;
break;
case CONST.TRANSACTION.STATE.BACKUP:
keyPrefix = ONYXKEYS.COLLECTION.TRANSACTION_BACKUP;
break;
case CONST.TRANSACTION.STATE.CURRENT:
default:
keyPrefix = ONYXKEYS.COLLECTION.TRANSACTION;
break;
}

return {
optimisticData: [
{
// Clears any potentially stale error messages from fetching the route
onyxMethod: Onyx.METHOD.MERGE,
key: `${isDraft ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
key: `${keyPrefix}${transactionID}`,
value: {
comment: {
isLoading: true,
Expand All @@ -224,18 +239,26 @@ function getOnyxDataForRouteRequest(transactionID: string, isDraft = false): Ony
successData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${isDraft ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
key: `${keyPrefix}${transactionID}`,
value: {
comment: {
isLoading: false,
},
// When the user opens the distance request editor and changes the connection from offline to online,
// the transaction's pendingFields and pendingAction will be removed, but not transactionBackup.
// We clear the pendingFields and pendingAction for the backup here to ensure consistency with the transaction.
// Without this, the map will not be clickable if the user dismisses the distance request editor without saving.
...(transactionState === CONST.TRANSACTION.STATE.BACKUP && {
pendingFields: {waypoints: null},
pendingAction: null,
}),
},
},
],
failureData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${isDraft ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`,
key: `${keyPrefix}${transactionID}`,
value: {
comment: {
isLoading: false,
Expand Down Expand Up @@ -264,15 +287,30 @@ function sanitizeRecentWaypoints(waypoints: WaypointCollection): WaypointCollect
* Gets the route for a set of waypoints
* Used so we can generate a map view of the provided waypoints
*/
function getRoute(transactionID: string, waypoints: WaypointCollection, isDraft: boolean) {

function getRoute(transactionID: string, waypoints: WaypointCollection, routeType: TransactionState = CONST.TRANSACTION.STATE.CURRENT) {
const parameters: GetRouteParams = {
transactionID,
waypoints: JSON.stringify(sanitizeRecentWaypoints(waypoints)),
};

API.read(isDraft ? READ_COMMANDS.GET_ROUTE_FOR_DRAFT : READ_COMMANDS.GET_ROUTE, parameters, getOnyxDataForRouteRequest(transactionID, isDraft));
}
let command;
switch (routeType) {
case CONST.TRANSACTION.STATE.DRAFT:
command = READ_COMMANDS.GET_ROUTE_FOR_DRAFT;
break;
case CONST.TRANSACTION.STATE.CURRENT:
command = READ_COMMANDS.GET_ROUTE;
break;
case CONST.TRANSACTION.STATE.BACKUP:
command = READ_COMMANDS.GET_ROUTE_FOR_BACKUP;
break;
default:
throw new Error('Invalid route type');
}

API.read(command, parameters, getOnyxDataForRouteRequest(transactionID, routeType));
}
/**
* Updates all waypoints stored in the transaction specified by the provided transactionID.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ function IOURequestStepConfirmation({
const isPolicyExpenseChat = useMemo(() => participants?.some((participant) => participant.isPolicyExpenseChat), [participants]);
const formHasBeenSubmitted = useRef(false);

useFetchRoute(transaction, transaction?.comment?.waypoints, action);
useFetchRoute(transaction, transaction?.comment?.waypoints, action, IOUUtils.shouldUseTransactionDraft(action) ? CONST.TRANSACTION.STATE.DRAFT : CONST.TRANSACTION.STATE.CURRENT);

useEffect(() => {
const policyExpenseChat = participants?.find((participant) => participant.isPolicyExpenseChat);
Expand Down
20 changes: 19 additions & 1 deletion src/pages/iou/request/step/IOURequestStepDistance.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import useNetwork from '@hooks/useNetwork';
import usePolicy from '@hooks/usePolicy';
import usePrevious from '@hooks/usePrevious';
import useThemeStyles from '@hooks/useThemeStyles';
import * as Report from '@libs/actions/Report';
import DistanceRequestUtils from '@libs/DistanceRequestUtils';
import type {MileageRate} from '@libs/DistanceRequestUtils';
import * as ErrorUtils from '@libs/ErrorUtils';
Expand Down Expand Up @@ -78,7 +79,18 @@ function IOURequestStepDistance({
},
[optimisticWaypoints, transaction],
);
Copy link
Contributor

Choose a reason for hiding this comment

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

@wildan-m ⚠️ I noticed an issue which happens only when the RHP is dismissed for the first time -> the map component in the report resets / reloads. This doesn't happen if we open Distance RHP again and dismiss for the 2nd time.

issue.mov

I'm pretty sure that this will be reported as regression if we ship it this way, because since we fixed the issue in the sense that the map does show up now when dismissing the RHP, we now have that flicker map reset / reload on first time RHP dismissal.

Please look into this and see if you can figure out a way to not have the report map reset / reload when the RHP is dismissed for the first time and instead keep it the way it looks when it loads for the first time while RHP is still open if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ikevin127 After further investigation, this issue related to my previous mentioned issue #48630 (comment) where transactionBackup is not exactly the same with unmodified transaction data.

For instance, in backup, the pendingFields is not removed, this will cause incorrect result here:

if (hasReceipt) {
receiptURIs = ReceiptUtils.getThumbnailAndImageURIs(updatedTransaction ?? transaction);
}

I think the only way to resolve that flicker issue and other fields flicker is to make the backup data exactly the same with unmodified transaction data when they are online, and it will require BE change.

@Gonals is it possible that when we call GetRouteForBackup we also get the same unmodified transaction data when they are online?

The missing fields can be seen in previous comment #48630 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@wildan-m Thanks for the investigation and explanation, I just wanted to point it out and ask @Gonals whether we should fix it now or in another issue.

Copy link
Contributor Author

@wildan-m wildan-m Oct 27, 2024

Choose a reason for hiding this comment

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

I found another issue where the generated map is not clickable due to pendingFields and pendingAction not being removed. Making transactionBackup identical to unmodified transaction when online is crucial to prevent flickering and an unclickable map, but I'm unsure if this is possible from the backend perspective.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this has a different root cause, right? Since it requires BE changes, I think it is a different 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.

I think this has a different root cause, right? Since it requires BE changes, I think it is a different bug

@Gonals I agree that flickering is not directly related to the original issue and fixing it is not straightforward.

const {shouldFetchRoute, validatedWaypoints} = useFetchRoute(transaction, waypoints, action);

const backupWaypoints = transactionBackup?.pendingFields?.waypoints ? transactionBackup?.comment?.waypoints : undefined;
// When online, fetch the backup route to ensure the map is populated even if the user does not save the transaction.
// Fetch the backup route first to ensure the backup transaction map is updated before the main transaction map.
// This prevents a scenario where the main map loads, the user dismisses the map editor, and the backup map has not yet loaded due to delay.
useFetchRoute(transactionBackup, backupWaypoints, action, CONST.TRANSACTION.STATE.BACKUP);
const {shouldFetchRoute, validatedWaypoints} = useFetchRoute(
transaction,
waypoints,
action,
IOUUtils.shouldUseTransactionDraft(action) ? CONST.TRANSACTION.STATE.DRAFT : CONST.TRANSACTION.STATE.CURRENT,
);
const waypointsList = Object.keys(waypoints);
const previousWaypoints = usePrevious(waypoints);
const numberOfWaypoints = Object.keys(waypoints).length;
Expand Down Expand Up @@ -213,6 +225,12 @@ function IOURequestStepDistance({
return;
}
TransactionEdit.restoreOriginalTransactionFromBackup(transaction?.transactionID ?? '-1', IOUUtils.shouldUseTransactionDraft(action));

// If the user opens IOURequestStepDistance in offline mode and then goes online, re-open the report to fill in missing fields from the transaction backup
if (!transaction?.reportID) {
return;
}
Report.openReport(transaction?.reportID);
};
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, []);
Expand Down
6 changes: 6 additions & 0 deletions src/types/utils/TransactionStateType.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import type {ValueOf} from 'type-fest';
import type CONST from '@src/CONST';

type TransactionStateType = ValueOf<typeof CONST.TRANSACTION.STATE>;

export default TransactionStateType;
Loading