[HOLD for payment 2024-11-11] [$250] Distance - Receipt disappears when dismissing distance editor after receipt is generated

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.29-0
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 protected]
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause-Internal team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to workspace chat.
3. Go offline.
4. Submit a distance expense.
5. Go to transaction thread.
6. Click Distance.
7. Go online.
8. After the receipt in the background is generated, dismiss the distance editor by clicking RHP back button.

Expected Result:

The receipt in the transaction thread will remain generated.

Actual Result:

The receipt in the transaction thread turns to placeholder thumbnail.
Sometimes, the map loads, but the map is not clickable (it is still in the loading state and not generating the actual receipt).

This issue also happens when opening distance editor right after the distance expense is submitted before the receipt is generated.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6592667_1725441403897.20240904_171033.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021832157502208534261
• Upwork Job ID: 1832157502208534261
• Last Price Increase: 2024-10-18
• Automatic offers:
• ikevin127 | Reviewer | 104586775
• wildan-m | Contributor | 104586777

Issue Owner

Current Issue Owner: @isabelastisser

[melvin-bot]

Triggered auto assignment to @isabelastisser (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.

[nkdengineer]

Edited by proposal-police: This proposal was edited at 2024-09-09 05:24:30 UTC.

Proposal

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

The receipt in the transaction thread turns to placeholder thumbnail.
Sometimes, the map loads, but the map is not clickable (it is still in the loading state and not generating the actual receipt).

What is the root cause of that problem?

When we open the distance page, the transaction backup data has no route data. After BE returns the data, MoneyRequestView is loaded transaction data from BE. If we close the distance page without saving any thing, the transaction data is reverted to the transaction backup data which is outdated data.

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

Lines 186 to 192 in d3c995a

	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) {
	TransactionEdit.removeBackupTransaction(transaction?.transactionID ?? '-1');
	return;
	}

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

We should check if at the time we open the distance page, we don't have the route data and at the time we close the page, we have the route data, and we will not revert the transaction to transaction backup data

const hasRouteRef = useRef(TransactionUtils.hasRoute(transaction));
hasRouteRef.current = TransactionUtils.hasRoute(transaction);
useEffect(() => {
    if (isCreatingNewRequest) {
        return () => {};
    }

    // On mount, create the backup transaction.
    TransactionEdit.createBackupTransaction(transaction);
    const hasRoute = TransactionUtils.hasRoute(transaction);
    const pendingRoute = transaction?.pendingAction ?? transaction?.pendingFields?.waypoints;

    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 || (!hasRoute && !!pendingRoute && hasRouteRef.current)) {
            TransactionEdit.removeBackupTransaction(transaction?.transactionID ?? '-1');
            return;
        }
        TransactionEdit.restoreOriginalTransactionFromBackup(transaction?.transactionID ?? '-1', IOUUtils.shouldUseTransactionDraft(action));
    };
    // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [transaction?.pendingAction]);

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

Lines 186 to 192 in d3c995a

	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) {
	TransactionEdit.removeBackupTransaction(transaction?.transactionID ?? '-1');
	return;
	}

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[melvin-bot]

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

[ikevin127]

@nkdengineer Thanks for the proposal. I was able to reproduce the issue but I'm struggling with applying the solution, specifically the changes suggested at step 3 and without that the issue is still reproducible therefore I cannot verify the proposal.

Are you able to provide a testing branch with the working solution ? That would help verifying the solution and if the RCA matches the working solution then I would be able to finalize the proposal review. Thanks!

[nkdengineer]

@ikevin127 Ahh, sorry I proposed another bug. Updated my proposal for the original bug.

[ikevin127]

@nkdengineer Thanks for the update, I tested the solution and while it seems to fix the issue at first - the solution is not reliable because is breaking functionality that was working as expected before the change:

• check out the attached video where I show the discrepancy between local dev (proposed solution) vs current staging

Video

Screen.Recording.2024-09-10.at.18.mp4

Additionally, while going back online with the RHP open I had instances where the map receipt in the background report would show 404 not found:

I think a solution that would be acceptable for this issue should not break functionality that currently works well.

In addition, if proposing changes to currently existing code logic, some steps should be included for the reviewers to verify that current functionality is not affected by the proposed changes.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[isabelastisser]

Waiting for new proposals!

[ikevin127]

💰 Looking for proposals! Please keep in mind the previous proposal's pitfalls detailed in #48630 (comment) review.

[melvin-bot]

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

[isabelastisser]

Still waiting for proposals.

[ikevin127]

💰 Still looking for proposals! Please keep in mind the previous proposal's pitfalls detailed in #48630 (comment) review.

[melvin-bot]

@isabelastisser @ikevin127 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[wildan-m]

Proposal

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

Receipt disappears when dismissing distance editor after it is generated.

What is the root cause of that problem?

TransactionBackup routes are not being updated when online, causing the receipt route to disappear when the modal is dismissed.

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

This solution will require backend modifications.

Calculate transaction backup routes when there are pending waypoints, valid routes, and the system is online.

Create new API endpoint similar to GET_ROUTE_FOR_DRAFT but for backup.

    GET_ROUTE_FOR_BACKUP: 'GetRouteForBackup',

Add new type to define transactionStates

    TRANSACTION_STATE: {
        CURRENT: 'current',
        DRAFT: 'draft',
        BACKUP: 'backup',
    }

modify useFetchRoute to accept new parameter transactionState

export default function useFetchRoute(transaction: OnyxEntry<Transaction>, waypoints: WaypointCollection | undefined, action: IOUAction, transactionState: TransactionState = CONST.TRANSACTION_STATE.CURRENT) {
...............
    useEffect(() => {
        if (isOffline || !shouldFetchRoute || !transaction?.transactionID) {
            return;
        }

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

Modify getOnyxDataForRouteRequest and getRoute to accept new transactionState param

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;
    }


....


function getRoute(transactionID: string, waypoints: WaypointCollection, routeType: TransactionState = CONST.TRANSACTION_STATE.CURRENT) {
    if (routeType === CONST.TRANSACTION_STATE.BACKUP)
    {
        mockGetBackupRoute(transactionID);
    }

    const parameters: GetRouteParams = {
        transactionID,
        waypoints: JSON.stringify(waypoints),
    };

    let command;
    switch (routeType) {
        case 'draft':
            command = READ_COMMANDS.GET_ROUTE_FOR_DRAFT;
            break;
        case 'current':
            command = READ_COMMANDS.GET_ROUTE;
            break;
        case 'backup':
            command = READ_COMMANDS.GET_ROUTE_FOR_BACKUP;
            break;
        default:
            throw new Error('Invalid route type');
    }
.......

Adjust useFetchRoute usage:
src/pages/iou/request/step/IOURequestStepConfirmation.tsx

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

call useFetchRoute for backup in addition to current fetch, we can't use current transaction waypoints since that waypoints can be modified while offline.
src/pages/iou/request/step/IOURequestStepDistance.tsx

    const backupWaypoints = !!transactionBackup?.pendingFields?.waypoints ? transactionBackup?.comment?.waypoints : undefined;

    const { shouldFetchRoute, validatedWaypoints } = useFetchRoute(transaction, waypoints, action, IOUUtils.shouldUseTransactionDraft(action) ? CONST.TRANSACTION_STATE.DRAFT : CONST.TRANSACTION_STATE.CURRENT);
    useFetchRoute(transactionBackup, backupWaypoints, action, CONST.TRANSACTION_STATE.BACKUP);

Branch for this solution

What alternative solutions did you explore? (Optional)

N/A

[wildan-m]

Re-testing && creating the PR....

[wildan-m]

@Gonals what is the API name? is it GetRouteForBackup ?

[wildan-m]

Ah, seems dev server doesn't have that API, the API exists after switch to staging

@ikevin127 @Gonals Please confirm that the scope of this issue is only to restore the map

If the user go back without save the transaction, some fields would still missing. (e.g. merchant, customUnitID)

The missing fields not come from GetRoute or GetRouteForBackup but it's come from openReport API

openReport API only update the transaction not its backup.

What can we do?

1. There should be a way to also update the transaction backup missing fields, maybe calling GetRoute/GetRouteForBackup should also update the missing fields? (Require other BE changes)
2. Continue creating the PR. scope this issue to only show map and resolve other missing fields in separate issue.

Which option we should choose?

Backup (Left) vs Current Transaction (Right)

[ikevin127]

2. Continue creating the PR. scope this issue to only show map and resolve other missing fields in separate issue.

This would be my choice since pushing more BE changes would only extend the duration of fixing this specific issue. Once @Gonals comes back with the details of the API pull request changes and you have the required data to move forward with the PR and implement the fix, we should be all good.

[wildan-m]

@ikevin127 The PR is Ready #51519

@Gonals The previous found issue #48630 (comment) can be resolved by calling openReport(transaction?.reportID) after the backup restored. This way we wouldn't need to change BE.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[ikevin127]

We discussed the upcoming regression in the PR (#51519 (comment)) and we decided to ship it like this because the fix for it was out of scope for the issue.

Therefore even though this is technically a regression of our PR, we decided not to count it as regression when it comes to compensation.

[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.56-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

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

If no regressions arise, payment will be issued on 2024-11-11. 🎊

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

• @wildan-m requires payment automatic offer (Contributor)
• @ikevin127 requires payment automatic offer (Reviewer)

[melvin-bot]

@ikevin127 @isabelastisser The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[ikevin127]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production
• 2b. Reported on staging (deploy blocker)
• 2c. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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: https://github.com/Expensify/App/pull/47977/files#r1828576545.

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: N/A.

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

Regression Test Proposal

Test:

1. Open app.
2. Go to a workspace chat.
3. Go offline.
4. Submit a distance expense.
5. Navigate to the transaction thread.
6. Click on the Distance (Pending...) field.
7. Go online.
8. After the receipt in the background is generated, dismiss the RHP distance editor by clicking the RHP back button or click outside of the RHP modal.
9. Verify that the report's distance expense receipt map will load and show-up after the RHP is dismissed / closed.

Do we agree 👍 or 👎.

[wildan-m]

@Gonals for consistency and to avoid potential dev only bug, I think it's better if GET_ROUTE_FOR_BACKUP endpoint also deployed to BE dev server

[isabelastisser]

@ikevin127 @wildan-m, the payments are processed in Upwork and the regression test issue is created. Is there an ongoing discussion here or can I close this issue now? Thanks!

[wildan-m]

I think we can close it, the follow-up issue will be handled separately. #51785

[isabelastisser]

All set!