[$500] Split bill - When delete Split Bill report from attendee DM & go back - Error page showing

[lanitochka17]

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: 1.3.83-5

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

Logs: https://stackoverflow.com/c/expensify/questions/4856

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

Action Performed:

1. Open New Expensify app
2. Navigate to 1:1 DM with any of the Attendees you split bill with
3. Delete the IOU report
4. Tap Back button

Expected Result:

User should be landed on LHN or 1:1 DM

Actual Result:

"Hmm... It's not here" error page appears

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

Android: Native

Bug6234969_1697142018642.az_recorder_20231012_210513.mp4

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ac48abac656adba3
• Upwork Job ID: 1713880734494998528
• Last Price Increase: 2023-11-06

Issue Owner

Current Issue Owner: @parasharrajat

[melvin-bot]

Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[honnamkuan]

Proposal

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

When there is only 1 transaction is available in a IOU report, if the IOU transaction get deleted from the Transaction View page, it is deleted and user is routed to 1:1 DM page. Then, when user click on back button, it routes to a Not here error page.

What is the root cause of that problem?

When the last transaction in IOU report is deleted, the current logic will delete the transaction as well as IOU report, per IOU#deleteMoneyRequest

Currently there is a Navigation.goBack(ROUTES.HOME); at the end of function, which pop the last route, and then route user to 1:1 DM report.

App/src/libs/actions/IOU.js

Lines 2133 to 2137 in fe282b4

	if (shouldDeleteIOUReport) {
	// Pop the deleted report screen before navigating. This prevents navigating to the Concierge chat due to the missing report.
	Navigation.goBack(ROUTES.HOME);
	Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(iouReport.chatReportID));
	}

However, when user is in IOU Report > Transaction View page, that would only pop the route for Transaction View page.
When user click on Back, he is routed to the link for IOU Report, which would be deleted at that time, hence user is presented with Not Found page.

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

When user is deleting the last IOU transaction from Transaction View page, we need to pop both IOU Report and Transaction View routes from the Navigation stack, because both pages are deleted and no longer be accessible.

An example of how it can be done, by changing:

App/src/libs/actions/IOU.js

Lines 2134 to 2135 in fe282b4

	// Pop the deleted report screen before navigating. This prevents navigating to the Concierge chat due to the missing report.
	Navigation.goBack(ROUTES.HOME);

to

        if (isSingleTransactionView) {
            // Pop the deleted Transaction View screen
            Navigation.goBack(ROUTES.HOME);
        }
        // Pop the deleted Report Screen
        Navigation.goBack(ROUTES.HOME);

Note: when validating my proposed fix, I realised in order to validate the end-to-end fix, I will need to apply additional fixes similar to #29183 (comment) but in OptionRowLHNData to fix app crash when fullReport right after IOU report deletion and user get routed to LHN. That is done by replacing references of fullReport within withOnyx to fullReport ? fullReport.someID : '0'.

If the app crash bug has not been reported elsewhere or fixed before PR for this issue is raised, then I will include changes for OptionRowLHNData in the same PR raised for completeness of fix.

What alternative solutions did you explore? (Optional)

I have also considered using doing it like so

        if (isSingleTransactionView) {
            Navigation.pop(2);
        } else {
            Navigation.pop(1);
        }

where Navigation.pop is a new function that pop the specified number of recent routes from the navigation stack.
but strictly speaking, we can use existing function for the fix, so there is no real need to introduce new one, unless this is preferred by the proposal reviewer.

[melvin-bot]

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

[melvin-bot]

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

[twisterdotcom]

Just a note @lanitochka17 that if it's reproducable already, feel free to add External when you create it as well.

[tienifr]

Proposal

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

"Hmm... It's not here" error page appears after deleting money request and go back

What is the root cause of that problem?

If the money request is the only one in the IOU report, the IOU report will also be deleted after the money request is deleted.

In here, we only go back once, so the IOU report screen still remains in the navigation structure. So when we click go back, it will show not found page.

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

In here, if the page before the current transaction page is the IOU report page, we'll call another Navigation.goBack(ROUTES.HOME);.

if (isBackToIOUReport) { // get by checking the page behind the current page in the navigation state
     Navigation.goBack(ROUTES.HOME);
}
Navigation.goBack(ROUTES.HOME);

if the page before the current transaction page is the IOU report page

To check this, we can add a method to get the previous route report id (which uses the similar logic to getTopmostReportId), here're we'll pop the current route from the navigation state, then the getTopmostReportId of the resulting state will give us the report id of the "previous route".

const getPreviousRouteReportID = () => {
    const navigationState = navigationRef.getState();

    return getTopmostReportId({
        ...navigationState,
        routes: navigationState.routes.slice(0, -1)
    })
}

Then check the previous route report id against the IOU report id

const isBackToIOUReport = getPreviousRouteReportID() === iouReport.reportID;

We cannot rely on the isSingleTransactionView only because it will only work for the IOU Report -> Transaction Report navigation state, if the Transaction report is navigated to by other means, like Report A -> Transaction Report, we don't want to go back in this case because we'll lose the navigation history of Report A and it will pop Report A out unnecessarily, we should only pop the current transaction report in that case.

After the above change, the app crashes because in OptionRowLHNData we're not handling the case where the fullReport could be undefined. We can just fallback the fullReport.parentReportActionID (and some other ids accessed) to '0' like we did in other places to fix this crash.

What alternative solutions did you explore? (Optional)

Instead of the getPreviousRouteReportID implementation like above, we can modify the getTopmostReportId to be able to get the report id at nth location from the current route, but it's an implementation detail.

[imbngy]

Proposal

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

404like page loads after deleting the money request

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

This, but the if should not be made on the chat report ID which may still exist, but on the whole chatReport Object, like in the code below this one:

if (shouldDeleteIOUReport) {
    /*in this if im checking the reportID, which may not be null, so it should check the whole chatReport Object 
to see that if it has been deleted and if it is now null/undefined it should  go back to home screen / chatlist */
    if (iouReport.chatReportID == null || iouReport.chatReportID == undefined) {
        Navigation.goBack(ROUTES.HOME);
        Navigation.goBack(ROUTES.HOME); 
        //it should go back to the home screen / ChatList screen without loading the deleted report
    }
    else{
        Navigation.goBack(ROUTES.HOME);
        Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(iouReport.chatReportID));
    }
}

So the 2nd if should be something like:

    if(!chatReportExists(iouReport.chatReportID)){
          Navigation.goBack(ROUTES.HOME);
          Navigation.goBack(ROUTES.HOME); 
          //it should go back to the home screen / ChatList screen without loading the deleted report
    } ...

function chatReportExists( chatReportID ){ ... }

[melvin-bot]

📣 @imbngy! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[honnamkuan]

@tienifr your proposal is same as mine.

As mentioned in my proposal, the code example is an example, what is relevant is getting the root cause right and propose acceptable solution approach for the issue reported.

We should not fixate on code changes in proposal stage. When it is time to develop fix for the bug, the actual code changes in the PR can be reduced to simplify implementation, or increased to cover additional edge cases discovered during validation on all supported platforms.

[parasharrajat]

Reviewing...

[tienifr]

As mentioned in my proposal, the code example is an example, what is relevant is getting the root cause right and propose acceptable solution approach for the issue reported.

@honnamkuan let's keep discussion to minimal and wait for the C+ review. For your concern, my solution is clearly different from yours, it will rely on the navigation state rather than checking isSingleTransactionView in your solution (aka When user is deleting the last IOU transaction from Transaction View page). My proposal has nothing to do with the transaction view page.

We should not fixate on code changes in proposal stage.

I also didn't suggest any detailed code change here, I only gives my recommendation on the solution.

[twisterdotcom]

@parasharrajat intrigued for your take here please.

[parasharrajat]

Sure. Give me sometime to get back to it.

[melvin-bot]

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

[twisterdotcom]

Bump here @parasharrajat

[melvin-bot]

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

[parasharrajat]

I understood the root cause but I still need more info on how will reliably check if the last report was IOUreport or ExpenseReport.

[melvin-bot]

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

[twisterdotcom]

Still waiting on proposals then I suppose.

[tienifr]

I understood the root cause but I still need more info on how will reliably check if the last report was IOUreport or ExpenseReport.

@parasharrajat I've updated the proposal to clarify my approach for this. Hope it's clearer now.

To check this, we can add a method to get the previous route report id (which uses the similar logic to getTopmostReportId), here're we'll pop the current route from the navigation state, then the getTopmostReportId of the resulting state will give us the report id of the "previous route".

[melvin-bot]

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

[melvin-bot]

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

[twisterdotcom]

Bump here @parasharrajat.

[melvin-bot]

@twisterdotcom, @parasharrajat Eep! 4 days overdue now. Issues have feelings too...

[parasharrajat]

I will review it in 2 hours.

[melvin-bot]

@twisterdotcom @parasharrajat this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[twisterdotcom]

Can we assign @tienifr?

[parasharrajat]

So @tienifr's proposal solves the issue but I am unsure of if we should be using such patterns in the app.

Assigning an Engineer to get this reviewed.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[iwiznia]

hmmmmm this seems like a duplicate of #26569 which will be fixed by #26498
@sobitneupane @adamgrzybowski @JmillsExpensify @mountiny can you confirm that indeed this is the same issue (looks like it to me) and that the PR will fix this?

[s77rt]

Seems like a dupe

[melvin-bot]

@iwiznia, @twisterdotcom, @parasharrajat Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

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

[parasharrajat]

Bump @sobitneupane @adamgrzybowski @JmillsExpensify @mountiny #29497 (comment)

[sobitneupane]

@s77rt has taken over as the C+ of the issue. He has more information about the issue.

[s77rt]

@parasharrajat This is a dupe and #26498 will fix it