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

[$500] IOU - App redirects to same page after deleting Manual IOU and returning to previous page #28839

Closed
6 tasks done
lanitochka17 opened this issue Oct 4, 2023 · 24 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Oct 4, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Issue found when executing PR #28247

Action Performed:

  1. Launch New Expensify app
  2. Go to any chat > + > Request money
  3. Create a Manual IOU
  4. Back in 1:1 DM, create a Scan IOU
  5. Tap on the IOU preview to go to IOU report
  6. Tap on the Manual IOU and delete it
  7. In the report page, return to the previous page

Expected Result:

User is landed in the previous page which is 1:1 DM

Actual Result:

User is landed to the same IOU report page

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.77-5

Reproducible in staging?: Yes

Reproducible in production?: No

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6224781_1696434117548.Screen_Recording_20231004_210422_New_Expensify.mp4

Production

Screen_Recording_20231004_211424_New.Expensify.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e3f1510a12df23f3
  • Upwork Job ID: 1709986886517207040
  • Last Price Increase: 2023-10-05
@lanitochka17 lanitochka17 added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

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

@OSBotify
Copy link
Contributor

OSBotify commented Oct 4, 2023

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

Triggered auto assignment to @robertjchen (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@mountiny mountiny added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Oct 4, 2023
@mountiny
Copy link
Contributor

mountiny commented Oct 4, 2023

@tienifr I think this is a regression from your PR and should be handled as so, this should have been tested in your PR cc @mananjadhav

Can you please comment on this issue so we can assign you and you can fix this as a follow up in the regression period.

I dont think this is a deploy blocker because this was (differently) broken before, but I think that PR should not just leave the App in this state

@tienifr
Copy link
Contributor

tienifr commented Oct 4, 2023

Thanks for pointing out. Let me investigate.

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Oct 5, 2023

@tienifr I think this is a regression from your PR

@mountiny , @tienifr which PR?

@tienifr
Copy link
Contributor

tienifr commented Oct 5, 2023

@mountiny @mallenexpensify I don't think this comes from my PR. After revert these changes, I still can see this bug

Screen.Recording.2023-10-05.at.11.59.13.mov

and another bug

Screen.Recording.2023-10-05.at.11.59.44.mov

@mountiny
Copy link
Contributor

mountiny commented Oct 5, 2023

Thanks

@mountiny mountiny added the External Added to denote the issue can be worked on by a contributor label Oct 5, 2023
@melvin-bot melvin-bot bot changed the title IOU - App redirects to same page after deleting Manual IOU and returning to previous page [$500] IOU - App redirects to same page after deleting Manual IOU and returning to previous page Oct 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 5, 2023

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

@paultsimura
Copy link
Contributor

paultsimura commented Oct 5, 2023

Proposal

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

When deleting an IOU request, "goBack" navigates to the same page.

What is the root cause of that problem?

The root cause lies in the following code:

App/src/libs/actions/IOU.js

Lines 1651 to 1663 in 1663440

// STEP 7: Navigate the user depending on which page they are on and which resources were deleted
if (isSingleTransactionView && shouldDeleteTransactionThread && !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.reportID));
return;
}
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));
}

On Navigation.goBack(ROUTES.HOME);, we go back to the chatReport, and then navigate to the same page again. So now the navigation contains the chatReport as the previous route.

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

We are already handling cases of 2 different nested levels:

  1. Deleting the money request from IOU Report screen;
  2. Deleting the money request from the single Transaction screen;

However, we need to make these changes:

    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));
+       if (isSingleTransactionView) {
+           // Pop the deleted transaction screen before navigating.
+           Navigation.goBack(ROUTES.HOME);
+       }
+       const chatReportRoute = ROUTES.REPORT_WITH_ID.getRoute(iouReport.chatReportID);
+       if (!Navigation.isActiveRoute(chatReportRoute)) {
+           Navigation.navigate(chatReportRoute);
+       }
    }

This code does 2 things:

  1. Fixes another related existing bug, when navigating back after removing a money request from a single transaction screen returns Not Found:

    Bug demo
    iou-transaction-crash-1.mov
  2. Verifies that we do not navigate to the same report page if we are already on it.

Result (both scenarios):

iou-delete-navigation-chrome.mov

What alternative solutions did you explore? (Optional)

It may make sense to:

  • Integrate the "Do not navigate to the active route" behavior inside the Navigation.navigate;
  • As well as something like Navigation.goBackTimes(times: number) that will use navigationRef.current.dispatch({...StackActions.pop(times) in order to pop N latest routes from the navigation

WDYT?

@DylanDylann
Copy link
Contributor

@mountiny please help check if it dupes #27436 (comment)

@Ollyws
Copy link
Contributor

Ollyws commented Oct 6, 2023

Does seem similar, this should probably be also on hold for #26498 if not closed.

@mountiny
Copy link
Contributor

mountiny commented Oct 6, 2023

Put this on hold

@mountiny mountiny changed the title [$500] IOU - App redirects to same page after deleting Manual IOU and returning to previous page [HOLD #26498] [$500] IOU - App redirects to same page after deleting Manual IOU and returning to previous page Oct 6, 2023
@melvin-bot melvin-bot bot added the Overdue label Oct 9, 2023
@robertjchen robertjchen added the Reviewing Has a PR in review label Oct 9, 2023
@melvin-bot melvin-bot bot removed the Overdue label Oct 9, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 16, 2023

@robertjchen, @Ollyws, @mallenexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2023

@robertjchen, @Ollyws, @mallenexpensify Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot
Copy link

melvin-bot bot commented Oct 20, 2023

@robertjchen, @Ollyws, @mallenexpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@mallenexpensify mallenexpensify added Weekly KSv2 and removed Daily KSv2 labels Oct 20, 2023
@mallenexpensify
Copy link
Contributor

Bumped to weekly, as it's on hold

@mallenexpensify mallenexpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 20, 2023
@robertjchen
Copy link
Contributor

Still on hold for now

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Nov 29, 2023
Copy link

melvin-bot bot commented Nov 29, 2023

This issue has not been updated in over 15 days. @robertjchen, @Ollyws, @mallenexpensify eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@robertjchen
Copy link
Contributor

Still on hold for now

@mallenexpensify mallenexpensify changed the title [HOLD #26498] [$500] IOU - App redirects to same page after deleting Manual IOU and returning to previous page [$500] IOU - App redirects to same page after deleting Manual IOU and returning to previous page Jan 24, 2024
@mallenexpensify mallenexpensify added Weekly KSv2 and removed Reviewing Has a PR in review Monthly KSv2 labels Jan 24, 2024
@mallenexpensify
Copy link
Contributor

@Ollyws I'm a lil lost here, can you help by giving an update? I checked the PR we were held on and that seems to have hit production a while ago #26498

I removed the Reviewing label here and bumped to weekly so it won't get lost

@melvin-bot melvin-bot bot removed the Overdue label Jan 24, 2024
@Ollyws
Copy link
Contributor

Ollyws commented Jan 24, 2024

I can no longer reproduce, we're good to close this one.

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. Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants