Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.79k forks source link

[HOLD for payment][$250] Categorizing - Distance does not show route and button loads infinitely on confirmation page #47739

Closed lanitochka17 closed 2 weeks ago

lanitochka17 commented 1 month ago

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.22-7 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

Expected Result:

In Step 8, the map will show the correct route and the button will not load infinitely In Step 11, Distance and Rate fields will show the correct data

Actual Result:

In Step 8, the map does not show the correct route (no green route line) and the button loads infinitely In Step 11, Distance and Rate fields show "Pending"

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/b635b7d0-52c2-4dcd-a509-d757f8d27b57

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015f5f740a4477255d
  • Upwork Job ID: 1826736671618731941
  • Last Price Increase: 2024-08-22
Issue OwnerCurrent Issue Owner: @hungvu193
melvin-bot[bot] commented 1 month ago

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

lanitochka17 commented 1 month ago

@johncschuster FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

daledah commented 1 month ago

Proposal

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

In Step 8, the map does not show the correct route (no green route line) and the button loads infinitely In Step 11, Distance and Rate fields show "Pending"

What is the root cause of that problem?

I cannot reproduce the issue because you cannot edit Distance in a track expense:

https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/components/MoneyRequestConfirmationListFooter.tsx#L332

The only problem I faced is that the route does not show in confirmation screen:

Screenshot 2024-08-21 at 08 55 20

That's because we cleared transaction's routes on success here. Later it would be retrieved by GetRoute API, for example in distance editor here.

So apparently we do not call GetRoute in confirmation screen so we couldn't see the route.

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

Call GetRoute in confirmation screen if:

const isEmptyCoordinates = !transaction?.routes?.route0?.geometry?.coordinates?.length;
const isLoadingRoute = transaction?.comment?.isLoading ?? false;
const validatedWaypoints = TransactionUtils.getValidWaypoints(transaction?.comment?.waypoints);
const shouldFetchRoute = isEmptyCoordinates && !isLoadingRoute && Object.keys(validatedWaypoints).length > 1;

useEffect(() => {
    if (isOffline || !shouldFetchRoute) {
        return;
    }
    TransactionAction.getRoute(transactionID, validatedWaypoints, action === CONST.IOU.ACTION.CREATE);
}, [shouldFetchRoute, transactionID, validatedWaypoints, isOffline, action]);
bernhardoj commented 1 month ago

Proposal

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

Distance map doesn't show the green route when categorizing a track expense and when opening the distance edit page, the button loads infinitely.

What is the root cause of that problem?

When the track expense is completed, the routes object whcih contains the green line route is cleared. https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/libs/actions/IOU.ts#L1509-L1515

So, when we try to categorize it and it opens the confirmation page, we don't see the green route line. The route will be re-fetched if we open the distance page. https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/pages/iou/request/step/IOURequestStepDistance.tsx#L162-L167

But in our case, it loads infinitely. It's because, when we call the GetRoute request, it saves the route to transactions_xxx, meanwhile, we use transactionsDraft_xxx when categorizing the expense. https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/pages/iou/request/step/withFullTransactionOrNotFound.tsx#L76-L78

So, shouldFetchRoute stays true because the route is still empty, https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/pages/iou/request/step/IOURequestStepDistance.tsx#L98 https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/pages/iou/request/step/IOURequestStepDistance.tsx#L103

and the loading shows indefinitely. https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/pages/iou/request/step/IOURequestStepDistance.tsx#L513

Also, there is another issue with STRICT MODE. The code below that saves and restores backup is triggered twice because of the strict mode. The problem is, the restore logic is called late.

create backup
create backup
restore backup to draft

https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/pages/iou/request/step/IOURequestStepDistance.tsx#L187-L205

It's because the restore needs to connect to the onyx first which delays the logic, https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/libs/actions/TransactionEdit.ts#L29-L39

and the restore logic will remove the restore the backup to the draft and remove the backup. Once we close the page, the restore logic is called again, but this time, the backup is already empty, so we restore an empty data (null) to the transaction draft which clears it.

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

To fix the infinite loading, we need to call GetRouteForDraft when categorizing.

TransactionAction.getRoute(transaction.transactionID, validatedWaypoints, action === CONST.IOU.ACTION.CREATE || IOUUtils.isMovingTransactionFromTrackExpense(action));

The last param tells whether to use draft or not. There are many places where we need to update from action === CONST.IOU.ACTION.CREATE to action === CONST.IOU.ACTION.CREATE || IOUUtils.isMovingTransactionFromTrackExpense(action)) like in waypoint page so we can update the waypoint while categorizing too. https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/pages/iou/request/step/IOURequestStepWaypoint.tsx#L158

To fix the missing route issue, we need to fetch the route in confirm page too following the similar logic we have in distance page. To keep it clean, we can make it a hook. https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/pages/iou/request/step/IOURequestStepDistance.tsx#L162-L167

useFetchRoute.ts

export default function (transaction, waypoints, action) {
    const {isOffline} = useNetwork();
    const hasRouteError = !!transaction?.errorFields?.route;
    const hasRoute = TransactionUtils.hasRoute(transaction);
    const isRouteAbsentWithoutErrors = !hasRoute && !hasRouteError;
    const isLoadingRoute = transaction?.comment?.isLoading ?? false;
    const validatedWaypoints = TransactionUtils.getValidWaypoints(waypoints);
    const previousValidatedWaypoints = usePrevious(validatedWaypoints);
    const haveValidatedWaypointsChanged = !isEqual(previousValidatedWaypoints, validatedWaypoints);
    const shouldFetchRoute = (isRouteAbsentWithoutErrors || haveValidatedWaypointsChanged) && !isLoadingRoute && Object.keys(validatedWaypoints).length > 1;

    useEffect(() => {
        if (isOffline || !shouldFetchRoute) {
            return;
        }
        TransactionAction.getRoute(transaction.transactionID, validatedWaypoints, action === CONST.IOU.ACTION.CREATE || IOUUtils.isMovingTransactionFromTrackExpense(action));
    }, [shouldFetchRoute, transaction.transactionID, validatedWaypoints, isOffline, action]);

    return {shouldFetchRoute, validatedWaypoints};
}

Then, use it in distance and confirm page. For example, in confirm page,

useFetchRoute(transaction, transaction.comment.waypoints, action);

To solve the strict mode issue, we can cancel the restore if there is a create backup request.

let connectionID;

function createBackupTransaction(transaction: OnyxEntry<Transaction>) {
    ...;
    Onyx.disconnect(connectionID);
    // Use set so that it will always fully overwrite any backup transaction that could have existed before
    Onyx.set(`${ONYXKEYS.COLLECTION.TRANSACTION_BACKUP}${transaction.transactionID}`, newTransaction);
}

function restoreOriginalTransactionFromBackup(transactionID: string, isDraft: boolean) {
    connectionID = Onyx.connect({...});
}

This will behaves as if we are updating the backup.

create backup
create backup
(the 2nd backup replaces the 1st one, but there will be no restore)

https://github.com/Expensify/App/blob/685bb4c7bbaa2ef332896de5cace193edb9c6a51/src/pages/iou/request/step/IOURequestStepDistance.tsx#L202 Notice that we need to update the action === CONST.IOU.ACTION.CREATE condition too as explained above.

johncschuster commented 1 month ago

Pinging my pal, @neil-marcellini for a little insight into the best project for this. I feel like it could be #wave-collect, but I'm not certain. Can you point me in the right direction?

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

hungvu193 commented 1 month ago

Thanks for the proposals, everyone! @bernhardoj 's proposal looks good to me as it has correct RCA and fixes both issues in the OP.

๐ŸŽ€ ๐Ÿ‘€ ๐ŸŽ€ C+ review!

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @carlosmiceli, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

bernhardoj commented 1 month ago

PR is ready

cc: @hungvu193

hungvu193 commented 3 weeks ago

This issue is now ready for payment @johncschuster

johncschuster commented 2 weeks ago

Payment Summary:

Contributor: @bernhardoj owed $250 via NewDot Contributor+: @hungvu193 owed $250 via NewDot

@bernhardoj / @hungvu193 do you feel this requires regression test steps? If so, can you please provide them? Thank you!

hungvu193 commented 2 weeks ago

Regression test:

  1. Open self DM.
  2. Create a new distance track expense.
  3. Select Categorize it.
  4. Select any workspace.
  5. In confirm page, verify the map shows the green route line.
  6. Press the Distance field.
  7. Verify the map shows the green route line.
  8. Verify the button doesn't infinitely show loading indicator.
  9. Close the categorize page.
  10. Open the track expense report.
  11. Verify the distance and rate don't show Pending.

Do we ๐Ÿ‘ or ๐Ÿ‘Ž ?

garrettmknight commented 2 weeks ago

$250 approved for @hungvu193

johncschuster commented 2 weeks ago

All BZ Steps are complete. We're just waiting on paying out @bernhardoj. Closing!

bernhardoj commented 2 weeks ago

Requested in ND.

JmillsExpensify commented 2 weeks ago

$250 approved for @bernhardoj