Payment due 2024-10-04 [$250] When navigate to Workspace Editor then back to Inbox, it is taking me to my self DM rather than last chat

m-natarajan commented 2 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: 9.0.31-6 Reproducible in staging?: Y Reproducible in production?: Y 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: @danielrvidal Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1725904646621169

Action Performed:

Go to Concierge DM
Click a task, navigated to the task report
Click a link in the task, navigated to the categories page in the workspace settings
Click the back caret in the workspace settings, navigated to the Workspaces page in account settings
Click the Inbox bottom tab

Expected Result:

Should navigate me back to the concierge chat

Actual Result:

Navigating back to self DM

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

https://github.com/user-attachments/assets/14c0213b-0526-4824-871e-4867a27c1d80

https://github.com/user-attachments/assets/351f9a84-fae8-4327-b326-f3a222937b20

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021834563126903568456
Upwork Job ID: 1834563126903568456
Last Price Increase: 2024-09-13
Automatic offers:

c3024 | Contributor | 103991019

Issue Owner

Current Issue Owner: @zanyrenney

melvin-bot[bot] commented 2 months ago

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

Tony-MK commented 2 months ago

Proposal

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

When navigate to Workspace Editor then back to Inbox, it is taking me to my self DM rather than last chat

What is the root cause of that problem?

The ROUTES.WORKSPACE_CATEGORIES does not have and use a backTo param.

https://github.com/Expensify/App/blob/172504115675f5c6aad87fe4e96e8166b2183b14/src/libs/actions/Report.ts#L3374

Therefore, when the HeaderWithBackButton is clicked, the modal is dismissed instead of navigating to the backTo URL.

https://github.com/Expensify/App/blob/1b479b0602158fea8eb427fb215a14a18926c58f/src/pages/workspace/WorkspaceInitialPage.tsx#L415-L417

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

We should create and use a backTo param set to the chat reportID where the workspaceCategoriesLink was clicked.

https://github.com/Expensify/App/blob/172504115675f5c6aad87fe4e96e8166b2183b14/src/ROUTES.ts#L758

c3024 commented 1 month ago

Proposal

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

Navigating to the Inbox from the Workspace Initial Settings page takes the user to Self DM instead of the last chat.

What is the root cause of that problem?

When we click on Inbox, we navigate to ROUTES.HOME
https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx#L107-L108
which corresponds to BottomTabNavigator
https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/libs/Navigation/linkingConfig/config.ts#L65-L69
and get the matchingCentralPaneRoute and dispatch it here:
https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/libs/Navigation/linkTo/index.ts#L166-L175

It gets the routeID from getTopMostReportIDFromRHP, https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/libs/Navigation/linkingConfig/getMatchingCentralPaneRouteForState.ts#L64-L66

but it returns routeID as '' because there is no RHP in the state.
https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/libs/Navigation/linkingConfig/getMatchingCentralPaneRouteForState.ts#L31

When there is no reportID in the params, we find the lastAccessedReportID here:
https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/pages/home/ReportScreen.tsx#L140-L149
For first-time Expensify users, the lastAccessedReportID returns the first non-Concierge report's reportID:
https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/libs/ReportUtils.ts#L1338-L1345

Since this does not check for the last read time of the report and simply returns whichever non-empty Concierge report it finds first, it can navigate the user to any page depending on the order in the array.

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

We correctly show getMostRecentlyVisitedReport for old (non-first-time Expensify users) here:
https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/libs/ReportUtils.ts#L1356-L1358
We can use the same logic for first-time Expensify users as well:
https://github.com/Expensify/App/blob/00bc167940607f1418109f00e2b4e2d7d4ae858b/src/libs/ReportUtils.ts#L1345

What alternative solutions did you explore? (Optional)

As I see it, we do not exclude system chats for old users with more than 2 reports. Since system chat is the DM with Expensify, and a user cannot chat in it because the composer is disabled, I think we can exclude system chats in all cases and navigate the user to the last accessed report among the non-system chat reports. This can simplify the code.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

zanyrenney commented 1 month ago

This weird routing seems like something is broken in the task routing / worksapce. I think this is a quality issue

jayeshmangwani commented 1 month ago

@Tony-MK If we add backTo to the workspaceCategoriesLink, pressing the workspace categories page header will redirect to the report screen instead of the settings/workspaces (which is the existing flow). So, I don’t think we need to change the backTo flow. Instead, we can make changes in linkTo/index.ts to fix the logic for first-time users.

jayeshmangwani commented 1 month ago

@c3024 I think we can remove the isFirstTimeNewExpensifyUser check block altogether. As you're suggesting, we should return lastRead instead of reportsValues.find((report) => !isConciergeChatReport(report));.

The only remaining thing to check is whether reportsValues.length === 1, but from my testing with a few users, it seems we always have reportsValues.length >= 2. So, do we even need that check, or can we remove it?

c3024 commented 1 month ago

We should have at least Concierge and Self DM reports, so I think we can remove that condition of checking for one report (after excluding the Expensify (system) chat) as well. I went down the rabbit hole of git blame and found that this piece of code is a remnant from the pre-Self DM era.

https://github.com/Expensify/App/commit/57de0362597602cc45fbcf1055db1529d3c71796#diff-65c096044d5f69b35bcdec14c99c4fda4580759df9b1a7c36650d58eea276f1d

I think we can remove it. There could be backend failures sometimes, and only one report might be returned, but such failure cases don't need to be considered here in my opinion.

jayeshmangwani commented 1 month ago

We can proceed with @c3024 's Proposal and return the last read from findLastAccessedReport, just like we do for any other existing user.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

📣 @c3024 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

bondydaa commented 1 month ago

looks like this was deployed to prod but maybe automation failed? https://github.com/Expensify/App/pull/49324#issuecomment-2379772410

c3024 commented 1 month ago

Yes, this should be on HOLD till 4-Oct for payment.

zanyrenney commented 1 month ago

I am OOO until 7th october, reassigning a bug team member to help with tomorrow's payment!

melvin-bot[bot] commented 1 month ago

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

zanyrenney commented 1 month ago

whoops didn't mean to unassign Joe, just as per the process, assigning myself so i will take this back if not resolved.