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

[$250] Distance - Receipt disappears when dismissing distance editor after receipt is generated #48630

Open
6 tasks done
izarutskaya opened this issue Sep 5, 2024 · 60 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@izarutskaya
Copy link

izarutskaya commented Sep 5, 2024

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: 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: applausetester+kh010901@applause.expensifail.com
Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com
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 OwnerCurrent Issue Owner: @ikevin127
@izarutskaya izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 5, 2024
Copy link

melvin-bot bot commented Sep 5, 2024

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
Copy link
Contributor

nkdengineer commented Sep 5, 2024

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.

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]);

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)

@isabelastisser isabelastisser added External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Sep 6, 2024
Copy link

melvin-bot bot commented Sep 6, 2024

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

@melvin-bot melvin-bot bot changed the title Distance - Receipt disappears when dismissing distance editor after receipt is generated [$250] Distance - Receipt disappears when dismissing distance editor after receipt is generated Sep 6, 2024
Copy link

melvin-bot bot commented Sep 6, 2024

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

@ikevin127

This comment was marked as outdated.

@ikevin127
Copy link
Contributor

@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
Copy link
Contributor

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

@ikevin127
Copy link
Contributor

@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:

Screenshot 2024-09-10 at 18 03 30

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.

Copy link

melvin-bot bot commented Sep 13, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Sep 13, 2024
@isabelastisser
Copy link
Contributor

Waiting for new proposals!

@ikevin127
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Sep 14, 2024
Copy link

melvin-bot bot commented Sep 17, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Sep 17, 2024
@isabelastisser
Copy link
Contributor

Still waiting for proposals.

@ikevin127
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Sep 18, 2024
Copy link

melvin-bot bot commented Sep 19, 2024

@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!

Copy link

melvin-bot bot commented Sep 20, 2024

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

@wildan-m
Copy link
Contributor

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

@melvin-bot melvin-bot bot added the Overdue label Sep 21, 2024
@ikevin127
Copy link
Contributor

♻️ Reviewing this and asking the team what's the procedure when proposals request BE changes - will get back with feedback next week.

@Gonals
Copy link
Contributor

Gonals commented Oct 21, 2024

Getting to it today!

@melvin-bot melvin-bot bot added the Overdue label Oct 21, 2024
@Gonals
Copy link
Contributor

Gonals commented Oct 21, 2024

@wildan-m, how will GET_ROUTE_FOR_BACKUP differ from GET_ROUTE_FOR_DRAFT? Do you expect to pass the same parameters and receive the same response? If so, why not calling GET_ROUTE_FOR_DRAFT in this flow?

@wildan-m
Copy link
Contributor

@Gonals the only difference is target onyx key from the server. it should target backup onyx key

Copy link

melvin-bot bot commented Oct 21, 2024

@Gonals, @isabelastisser, @ikevin127 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@ikevin127
Copy link
Contributor

Not overdue, we're working on the BE side after which the FE pull request will follow.

@melvin-bot melvin-bot bot removed the Overdue label Oct 21, 2024
@wildan-m
Copy link
Contributor

If so, why not calling GET_ROUTE_FOR_DRAFT in this flow?

@Gonals calling that API will update the key for draft not for back up

@Gonals
Copy link
Contributor

Gonals commented Oct 24, 2024

Gotcha. Backend PR incoming

@melvin-bot melvin-bot bot added the Overdue label Oct 24, 2024
@Gonals
Copy link
Contributor

Gonals commented Oct 24, 2024

Backend PR in review

@ikevin127
Copy link
Contributor

Not overdue, BE pull request is in review after which the FE pull request will follow.

@melvin-bot melvin-bot bot removed the Overdue label Oct 24, 2024
@Gonals
Copy link
Contributor

Gonals commented Oct 25, 2024

BE deployed to staging. FE can now test with it

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 25, 2024
Copy link

melvin-bot bot commented Oct 25, 2024

📣 @ikevin127 🎉 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

Copy link

melvin-bot bot commented Oct 25, 2024

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

@wildan-m
Copy link
Contributor

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

@wildan-m
Copy link
Contributor

wildan-m commented Oct 25, 2024

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

@wildan-m
Copy link
Contributor

wildan-m commented Oct 25, 2024

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)

image

@ikevin127
Copy link
Contributor

  1. 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
Copy link
Contributor

@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.

Copy link

melvin-bot bot commented Oct 31, 2024

⚠️ 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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Development

No branches or pull requests

6 participants