[HOLD for payment 2024-07-26] [$250] BUG: Failed distance request isn't cleaned up properly

[neil-marcellini]

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: main
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers): applausetester+kh300501@applause.expensifail.com
Logs: 88c4715bea025f6f-SIN
Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/400806#top
Issue reported by: @neil-marcellini
Slack conversation:

Action Performed:

1. Log into NewDot
2. Create a workspace if needed
3. Go offline in the Network tab of dev tools, or using the force offline switch
4. Create a distance request on that workspace
5. Log into the same account on OldDot (expensify.com.dev)
6. Go to your group workspaces
7. Delete the workspace
8. Go back to NewDot, and go online
9. Verify the request fails to create
10. Click on the request
11. Verify there is an error message that says "Unexpected error submitting this expense. Please try again later. The workspace is no longer accessible. Please try again on a different workspace."
12. Click the X to dismiss the error

Expected Result:

The failed request should disappear completely after the error is dismissed. The transaction report should be deleted and the request should be removed from the workspace chat. Everything needs to be cleaned up. The user can get navigated back the the archived workspace chat, or something like that.

We should also probably improve the error message, and have it translated vs only in English from the backend. Requesting copy now.

Actual Result:

The transaction report remains with a RBR indicator, and the request is also still visible in the archived workspace chat

Workaround:

Sign out and back in to clear all optimistic/failure data

Platforms:

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

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

Screenshots/Videos

335710534-aaa87b61-51d5-420e-bad6-1cb41c493858.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b6ac9eda43aba0be
• Upwork Job ID: 1796631286961946624
• Last Price Increase: 2024-06-07
• Automatic offers:
• dominictb | Contributor | 102768394

Issue Owner

Current Issue Owner: @sobitneupane

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @CherylWalsh (Waiting for copy), see https://stackoverflow.com/c/expensify/questions/7025/ for more details.

[neil-marcellini]

Hi @CherylWalsh, could we please have your expertise in coming up with a helpful error message for the user to see in this case. It's likely to happen if they create a distance request while offline, and then before they go back online an admin deletes the policy, or removes them from it.

[goldenbear101]

could I take the job please

[dominictb]

Proposal

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

The transaction report remains with a RBR indicator, and the request is also still visible in the archived workspace chat

What is the root cause of that problem?

When clearing the transaction error in

App/src/components/ReportActionItem/MoneyRequestView.tsx

Line 349 in 0cb2dc0

onClose={() => {

, the app's simply clearing the transaction error, the transaction itself, the parent report action, transaction thread, iou report & report preview (in case this is the only transaction in the IOU) still remains.

Also the app's showing the generic back-end error as is, it should be updated to be more helpful as confirmed above.

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

When clearing the transaction error, we can check that the transaction is having error while pending creation (ie. by checking that the transaction has errors and the transaction thread report has errorFields.createChat like this), if that's true, we should cleanly delete the transaction/money request, like we did in

App/src/libs/actions/IOU.ts

Line 5198 in 0cb2dc0

function deleteMoneyRequest(transactionID: string, reportAction: OnyxTypes.ReportAction, isSingleTransactionView = false) {

Here're the summary list of items:

• Delete the transaction itself
• Delete the transaction thread report
• Delete the IOU report and the IOU report preview report action in the chat report, if the transaction is the only one in the IOU report
• Delete the transaction report action in the IOU report, if there're other transactions in the IOU report
• Navigate to the correct report after the deletion (either IOU report or chat report depending on if IOU report was deleted) in step 3

And a few other caveats/conditions and implementation details for the different deletion, we can look into the deleteMoneyRequest method above and make sure the transaction is cleanly deleted similar to in there. We can either create a new method deleteErroredTransaction, or in deleteMoneyRequest, if the transaction is pendingAction add, handle the deletion but do not call the API endpoint to delete, then we can use deleteMoneyRequest when the user closes the error.

For the back-end error message, we have the suggestion here to map the back-end specific error to front-end error translation key. This can be easily accomplished by transforming the error here, after getting the list of errors, we check if it contains an error with "Unexpected error submitting this expense. Please try again later. The workspace is no longer accessible. Please try again on a different workspace." message, if yes, swap it with an error translation key that we defined like iou.error.workspaceNotAccessible (and optionally filter out the genericCreateFailureMessage as it's not really needed any more). Then it will show just fine in the front-end.

What alternative solutions did you explore? (Optional)

Aside from using deleteMoneyRequest or equivalent, using ReportActions.clearAllRelatedReportActionErrors for the linked report action in IOU report of the transaction (like we did here) could also work.

[dominictb]

Proposal updated to add some more details

[mallenexpensify]

@CherylWalsh 👀 on the above plz.

Hi @CherylWalsh, could we please have your expertise in coming up with a helpful error message for the user to see in this case. It's likely to happen if they create a distance request while offline, and then before they go back online an admin deletes the policy, or removes them from it.

@sobitneupane 👀 on @dominictb 's proposal above.

@goldenbear101 , plz review CONTRIBUTING.md if you'd like to contribute to the program. Thx

[goldenbear101]

Proposal

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

The issue at hand is that the transaction report remains with a Retryable Blocked Request (RBR) indicator, and the request is still visible in the archived workspace chat after an error is dismissed.

Root Cause
When clearing the transaction error in App/src/components/ReportActionItem/MoneyRequestView.tsx (line 349 in commit 0cb2dc0), the application only clears the transaction error. However, the transaction itself, the parent report action, transaction thread, IOU report, and report preview (if it is the only transaction in the IOU) still remain. Additionally, the application displays a generic backend error message, which should be updated to be more user-friendly and helpful.

Proposed Changes
To solve the problem, the following changes are recommended:

1. Enhanced Error Handling and Cleanup
   When clearing the transaction error, check if the transaction has an error while pending creation (e.g., by verifying that the transaction has errors and the transaction thread report has errorFields.createChat).

onClose={() => { 
    // Check if the transaction has errors and the transaction thread report has errorFields.createChat
    if (transaction.hasErrors && transactionThreadReport.errorFields.createChat) {
        // Call the deleteMoneyRequest method to cleanly delete the transaction/money request
        deleteErroredTransaction(transactionID, reportAction);
    } else {
        // Clear only the transaction error
        clearTransactionError(transactionID);
    }
}}

2. Specific Steps for Cleanup
   Implement the deleteErroredTransaction method:

function deleteErroredTransaction(transactionID, reportAction) {
    // Delete the transaction itself
    deleteTransaction(transactionID);

    // Delete the transaction thread report
    deleteTransactionThreadReport(transactionID);

    // Delete the IOU report and the IOU report preview report action in the chat report, if the transaction is the only one in the IOU report
    if (isOnlyTransactionInIOUReport(transactionID)) {
        deleteIOUReport(transactionID);
        deleteIOUReportPreview(transactionID);
    } else {
        // Delete the transaction report action in the IOU report, if there are other transactions in the IOU report
        deleteTransactionReportAction(transactionID);
    }

    // Navigate to the correct report after the deletion (either IOU report or chat report depending on if IOU report was deleted)
    navigateToCorrectReport(transactionID);
}

3. Method Implementation
   Create a new method deleteErroredTransaction or modify deleteMoneyRequest to handle deletion for transactions with pendingAction: add without calling the API endpoint to delete.

function deleteMoneyRequest(transactionID, reportAction, isSingleTransactionView = false) {
    if (transaction.pendingAction === 'add') {
        // Handle the deletion but do not call the API endpoint to delete
        deleteErroredTransaction(transactionID, reportAction);
    } else {
        // Existing delete logic
    }
}

4. Improved Error Messaging
   Update the backend to provide clearer and translated error messages, or provide a translation key for error messages to enable translation in the frontend.

Backend:

app.post('/create-distance-request', (req, res) => {
    const { workspaceId } = req.body;

    // Check if the workspace exists
    const workspace = getWorkspaceById(workspaceId);
    if (!workspace) {
        return res.status(400).json({
            error: 'WorkspaceDeleted',
            message: 'The workspace is no longer accessible.',
            translationKey: 'workspace_deleted',
        });
    }

    // Handle the request creation logic
    // ...
});

FRONTEND:

// Display the translated error message
const errorMessage = translateErrorMessage(error.translationKey || 'genericCreateFailureMessage');
showError(errorMessage);

Alternative Solutions
As an alternative to using deleteMoneyRequest or an equivalent method, ReportActions.clearAllRelatedReportActionErrors for the linked report action in the IOU report of the transaction (as done previously) could also be a viable solution.

By implementing these changes, the failed transaction requests will be properly cleaned up, improving the user experience and ensuring the application state is correctly maintained.

[mallenexpensify]

@sobitneupane , 👀 on the proposals above plz
@CherylWalsh , 👀 on the copy above too, thx

[goldenbear101]

I hope it's okay that I used AI to confirm my synopsis. I'm asking because I am new, If anything is needed to be cleared I would be more than happy to elaborate.

[sobitneupane]

Thanks for the proposal @dominictb

And a few other caveats/conditions and implementation details for the different deletion, we can look into the deleteMoneyRequest method above and make sure the transaction is cleanly deleted similar to in there. We can either create a new method deleteErroredTransaction, or in deleteMoneyRequest, if the transaction is pendingAction add, handle the deletion but do not call the API endpoint to delete, then we can use deleteMoneyRequest when the user closes the error.

I believe we can add a new parameter to determine whether to make api request or not.

For the back-end error message, the back-end can provide a clearer translated message, or provide a translation key for the message which will enable translation in the front-end.

How are we handling other server returned error/messages in the app? Do we return translation key?

Could you please provide more details about alternative solutions?

[melvin-bot]

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

[neil-marcellini]

I don't think we should use the deleteMoneyRequest action for this, since that's not really what we're doing. We can re-use the code which sets up the optimistic data for it, and extract it into a separate function called cleanUpMoneyRequest or something similar.

For the errors, the backend is currently returning a descriptive error message. Maybe we could check for the exact error message, and then swap it out for a translation key on the frontend somehow? Would that work? That would be easiest and fastest. The error thrown from the backend is deep in a tree of functions so it would be kind of tricky to return a translation key. I could make it work, but would prefer not to.

[sobitneupane]

Waiting for update.
#42950 (comment)
#42950 (comment)

[dominictb]

PR will be ready in a few hours (at most)!

[neil-marcellini]

We had a delay because I suggested using a new pattern in an attempt to improve the error message, but that had to get reviewed by the team and it was not approved. Now I'm requesting that @dominictb removes the error message swapping from the PR and we merge the rest of it.

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

• fix: delete money request if failed to create chat #44142

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

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

• @sobitneupane requires payment through NewDot Manual Requests
• @dominictb requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:
• [@sobitneupane] 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:
• [@sobitneupane] A discussion in #expensify-bugs 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:
• [@sobitneupane] Determine if we should create a regression test for this bug.
• [@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mallenexpensify]

Contributor: @dominictb paid $250 via Upwork
Contributor+: @sobitneupane due $250 via NewDot

@sobitneupane plz complete the BZ checklist above.

[sobitneupane]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#34087

• [@sobitneupane] 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:

Comment for similar issue : #34087 (comment)

• [@sobitneupane] A discussion in #expensify-bugs 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:

Not required.

• [@sobitneupane] Determine if we should create a regression test for this bug.

Yes.

• [@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

#42950 (comment)

[sobitneupane]

Regression Test Proposal

1. Log into NewDot
2. Create a workspace if needed
3. Go offline
4. Create a distance request on that workspace
5. Log into the same account, but in another device and delete the workspace on which distance request is created.
6. Go back to previous device, and go online
7. Verify the request fails to create
8. Click on the request
9. Verify there is an error message.
10. Click the X to dismiss the error.
11. Verify that the transaction, transaction thread(if it is only transaction) and the request is removed from the app.

Do we agree 👍 or 👎

[JmillsExpensify]

$250 approved for @sobitneupane

[mallenexpensify]

Thanks @sobitneupane. Test case created

• https://github.com/Expensify/Expensify/issues/416543