[HOLD for payment 2024-08-29] [$250] iOS - Thread - Back button in the main chat reopens main chat after leaving thread

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.11-2 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4760347 Issue reported by: Applause - Internal Team

Action Performed:

Launch New Expensify app
Go to DM
Send a message
Long press on the message > Reply in thread
Send a reply in thread
Tap on the report header
Tap Leave
Tap back button on the top left in the main chat

Expected Result:

App will return to LHN

Actual Result:

App reopens the main chat

Workaround:

Unknwon

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/d50d81eb-31b3-419e-a49c-7b7540fac878

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~014c369a294716b5b5
Upwork Job ID: 1816166555115213621
Last Price Increase: 2024-07-31

Issue Owner

Current Issue Owner: @garrettmknight / @jliexpensify

melvin-bot[bot] commented 1 month ago

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

We think that this bug might be related to #wvip-vsp

lanitochka17 commented 1 month ago

@garrettmknight 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

FitseTLT commented 1 month ago

Proposal

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

Thread - Back button in the main chat reopens main chat after leaving thread

What is the root cause of that problem?

We are navigating to last accessed report after leaving room here https://github.com/Expensify/App/blob/a79189f2294a07a737bebbf5423f7d19ebf9d467/src/libs/actions/Report.ts#L2790-L2791 Which is the main chat from which the thread was created. Therefore, when we navigate back we navigate to main chat once again as it existed in the navigation route state even before leaving thread.

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

This behaviour is expected for leaving room but for leaving chat thread we should only goBack, so change https://github.com/Expensify/App/blob/a79189f2294a07a737bebbf5423f7d19ebf9d467/src/libs/actions/Report.ts#L2790-L2791

if (isChatThread) {
        Navigation.goBack();
    } else {
        navigateToMostRecentReport(report);
    }

What alternative solutions did you explore? (Optional)

dominictb commented 1 month ago

Proposal

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

App reopens the main chat

What is the root cause of that problem?

After leaving the thread, we call navigateToMostRecentReport

https://github.com/Expensify/App/blob/bf69d5d8bf08ccc741baf8f91fc765b593d2f6b0/src/libs/actions/Report.ts#L2790

In the detail implementation of navigateToMostRecentReport, we trigger goBack with fallbackRoute.

https://github.com/Expensify/App/blob/bf69d5d8bf08ccc741baf8f91fc765b593d2f6b0/src/libs/actions/Report.ts#L2663

In this case, fallbackRoute is the last route, so it's duplicated.

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

We should call goBack to remove the current route, then go to the correspond lastAccessedReportRoute like what we already did in here

if (lastAccessedReportID) {
        const lastAccessedReportRoute = ROUTES.REPORT_WITH_ID.getRoute(lastAccessedReportID ?? '-1');
        Navigation.goBack();
        Navigation.navigate(lastAccessedReportRoute)
    } else {

What alternative solutions did you explore? (Optional)

bernhardoj commented 1 month ago

Proposal

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

Pressing the back button after leaving a thread shows the main chat again.

What is the root cause of that problem?

When we leave a thread, we already call dismiss modal to close the RHP first, https://github.com/Expensify/App/blob/98eb1bbf99bfc0e0c0f7f5a4bc1c84c510924504/src/pages/ReportDetailsPage.tsx#L219-L227

the leaveChat then calls navigateToMostRecentReport to perform goBack with a fallback route of the main chat report. https://github.com/Expensify/App/blob/98eb1bbf99bfc0e0c0f7f5a4bc1c84c510924504/src/libs/actions/Report.ts#L2660-L2663

In an ideal case, the RHP is already closed and the goBack with the fallback route will pop every report screen until it's arrived at the fallback route. https://github.com/Expensify/App/blob/98eb1bbf99bfc0e0c0f7f5a4bc1c84c510924504/src/libs/Navigation/Navigation.ts#L230-L239

However, in this case, the RHP is still in the nav state stack, so the goBack will replace the RHP with the main chat report. https://github.com/Expensify/App/blob/98eb1bbf99bfc0e0c0f7f5a4bc1c84c510924504/src/libs/Navigation/Navigation.ts#L222-L225

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

We need to delay the navigation like we did in https://github.com/Expensify/App/pull/44537.

Navigation.dismissModal();
Navigation.isNavigationReady().then(() => // leaveRoom / leaveGroupChat);

or use setTimeout/InteractionManager/useWaitForNavigate hook

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~014c369a294716b5b5

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

garrettmknight commented 1 month ago

@jliexpensify I'm headed out until August 5th. I'll pick this back up when I'm back. Thanks for your help in the meantime!

jliexpensify commented 1 month ago

No worries! Bumping @eVoloshchak for reviews please.

melvin-bot[bot] commented 1 month ago

@garrettmknight, @eVoloshchak, @jliexpensify Huh... This is 4 days overdue. Who can take care of this?

jliexpensify commented 1 month ago

Bumping @eVoloshchak for reviews! Also bumping on Slack.

eVoloshchak commented 1 month ago

I think we should proceed with @bernhardoj's proposal, this is the easiest and most reliable approach

🎀👀🎀 C+ reviewed!

melvin-bot[bot] commented 1 month ago

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

dominictb commented 1 month ago

@eVoloshchak Did you test the selected solution? It didn't work on my side. After leaving room, user is redirected to LHN (it's incorrect)

https://github.com/user-attachments/assets/0d15f716-bc10-49a8-ba12-6d32f238f6bf

melvin-bot[bot] commented 1 month ago

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

jliexpensify commented 1 month ago

Waiting on @eVoloshchak to address the most recent comment

melvin-bot[bot] commented 1 month ago

@garrettmknight, @eVoloshchak, @jliexpensify, @jasperhuangg Eep! 4 days overdue now. Issues have feelings too...

eVoloshchak commented 1 month ago

After leaving room, user is redirected to LHN (it's incorrect)

@dominictb, this seems to be the expected result

jasperhuangg commented 1 month ago

Yep I just double-checked and it seems like @bernhardoj's solution has the correct behavior, assigning them.

dominictb commented 1 month ago

@eVoloshchak @jasperhuangg I don't know why you said it's expected behavior. App will return to LHN if

Tap Leave
Tap back button on the top left in the main chat

But if you check my video above with the selected solution, after leaving room, app returns to LHN without tapping back button

bernhardoj commented 1 month ago

PR is ready

cc: @eVoloshchak

melvin-bot[bot] commented 2 weeks ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 2 weeks ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.23-0 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/46861

If no regressions arise, payment will be issued on 2024-08-29. :confetti_ball:

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

@eVoloshchak requires payment through NewDot Manual Requests
@bernhardoj requires payment through NewDot Manual Requests

melvin-bot[bot] commented 2 weeks ago

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:

[ ] [@eVoloshchak] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@eVoloshchak] 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:
[ ] [@eVoloshchak] 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:
[ ] [@eVoloshchak] Determine if we should create a regression test for this bug.
[ ] [@eVoloshchak] 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.
[ ] [@garrettmknight / @jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

jliexpensify commented 2 weeks ago

Garrett is back! Unassigning myself

melvin-bot[bot] commented 1 week ago

Payment Summary

Upwork Job

Reviewer: @eVoloshchak owed $250 via NewDot
Reviewer: @bernhardoj owed $250 via NewDot

BugZero Checklist (@garrettmknight)

[x] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
[x] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1816166555115213621/hired)
[ ] I have paid out the Upwork contracts or cancelled the ones that are incorrect
[x] I have verified the payment summary above is correct

garrettmknight commented 1 week ago

Payment summary looks right to me:

@eVoloshchak can you finish the checklist before you request payment?
@bernhardoj please request payment when you're ready.

bernhardoj commented 1 week ago

Requested in ND.

eVoloshchak commented 1 week ago

The PR that introduced the bug has been identified. Link to the PR: I'm unable to pinpoint the PR that has caused this, looks like this is the original implementation
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: N/A
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: this is a pretty straightforward bug, I don't think an additional discussion is needed

Regression Test Proposal

Open any chat
Send a message
Reply in the thread of the message
Press the chat header and press Leave
Verify you are navigated back to the main chat
Press the go back button
Verify you are navigated back to LHN

Do we agree 👍 or 👎

garrettmknight commented 6 days ago

Dropping to weekly while @eVoloshchak request payment.

JmillsExpensify commented 6 days ago

$250 approved for @eVoloshchak

JmillsExpensify commented 6 days ago

$250 approved for @eVoloshchak

garrettmknight commented 6 days ago

@JmillsExpensify did you get the request from @bernhardoj too?

Expensify / App