lanitochka17 commented 11 months 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: 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:

Open New Expensify app
Navigate to 1:1 DM with any of the Attendees you split bill with
Delete the IOU report
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?

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Android: Native

https://github.com/Expensify/App/assets/78819774/453fd44b-1d2d-47f4-94f8-ad8521716797

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[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] 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 commented 11 months ago

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.

https://github.com/Expensify/App/blob/fe282b45cb13e01519282ccc023e5bfbd7714158/src/libs/actions/IOU.js#L2133-L2137

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: https://github.com/Expensify/App/blob/fe282b45cb13e01519282ccc023e5bfbd7714158/src/libs/actions/IOU.js#L2134-L2135

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 https://github.com/Expensify/App/issues/29183#issuecomment-1755815680 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[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

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

twisterdotcom commented 11 months ago

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

tienifr commented 11 months ago

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 commented 11 months ago

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[bot] commented 11 months ago

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

Make sure you've read and understood the contributing guidelines.
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.
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.
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 commented 11 months ago

@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 commented 11 months ago

Reviewing...

tienifr commented 11 months ago

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 commented 11 months ago

@parasharrajat intrigued for your take here please.

parasharrajat commented 11 months ago

Sure. Give me sometime to get back to it.

melvin-bot[bot] commented 11 months ago

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

twisterdotcom commented 11 months ago

Bump here @parasharrajat

melvin-bot[bot] commented 11 months ago

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

parasharrajat commented 11 months ago

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[bot] commented 11 months ago

@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 commented 11 months ago

Still waiting on proposals then I suppose.

tienifr commented 11 months ago

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[bot] commented 11 months ago

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

melvin-bot[bot] commented 11 months ago

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

twisterdotcom commented 11 months ago

Bump here @parasharrajat.

melvin-bot[bot] commented 11 months ago

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

parasharrajat commented 11 months ago

I will review it in 2 hours.

melvin-bot[bot] commented 11 months ago

@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 commented 11 months ago

Can we assign @tienifr?

parasharrajat commented 11 months ago

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.

:ribbon: :eyes: :ribbon: C+ reviewed

melvin-bot[bot] commented 11 months ago

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

iwiznia commented 11 months ago

hmmmmm this seems like a duplicate of https://github.com/Expensify/App/issues/26569 which will be fixed by https://github.com/Expensify/App/pull/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 commented 11 months ago

Seems like a dupe

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

parasharrajat commented 10 months ago

Bump @sobitneupane @adamgrzybowski @JmillsExpensify @mountiny https://github.com/Expensify/App/issues/29497#issuecomment-1792400247

sobitneupane commented 10 months ago

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

s77rt commented 10 months ago

@parasharrajat This is a dupe and https://github.com/Expensify/App/pull/26498 will fix it

Expensify / App

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

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Bug0 Triage Checklist (Main S/O)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

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