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

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 #48840

Closed
1 of 6 tasks
m-natarajan opened this issue Sep 9, 2024 · 25 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Sep 9, 2024

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:

  1. Go to Concierge DM
  2. Click a task, navigated to the task report
  3. Click a link in the task, navigated to the categories page in the workspace settings
  4. Click the back caret in the workspace settings, navigated to the Workspaces page in account settings
  5. 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
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

2024-09-09_09-58-29.mp4
Recording.531.mp4

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 OwnerCurrent Issue Owner: @zanyrenney
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 9, 2024
Copy link

melvin-bot bot commented Sep 9, 2024

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
Copy link
Contributor

Tony-MK commented Sep 10, 2024

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.

workspaceCategoriesLink: `${environmentURL}/${ROUTES.WORKSPACE_CATEGORIES.getRoute(onboardingPolicyID ?? '-1')}`,

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

if (route.params?.backTo) {
Navigation.resetToHome();
Navigation.isNavigationReady().then(() => Navigation.navigate(route.params?.backTo as Route));

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.

getRoute: (policyID: string, categoryName: string) => `settings/workspaces/${policyID}/categories/${encodeURIComponent(categoryName)}` as const,

@melvin-bot melvin-bot bot added the Overdue label Sep 12, 2024
@c3024
Copy link
Contributor

c3024 commented Sep 13, 2024

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

const route = activeWorkspaceID ? (`/w/${activeWorkspaceID}/${ROUTES.HOME}` as Route) : ROUTES.HOME;
Navigation.navigate(route);

which corresponds to BottomTabNavigator
[NAVIGATORS.BOTTOM_TAB_NAVIGATOR]: {
path: ROUTES.ROOT,
initialRouteName: SCREENS.HOME,
screens: {
[SCREENS.HOME]: ROUTES.HOME,

and get the matchingCentralPaneRoute and dispatch it here:
const matchingCentralPaneRoute = getMatchingCentralPaneRouteForState(stateFromPath, rootState)!;
if (matchingCentralPaneRoute && 'name' in matchingCentralPaneRoute) {
root.dispatch({
type: CONST.NAVIGATION.ACTION_TYPE.PUSH,
payload: {
name: matchingCentralPaneRoute.name,
params: matchingCentralPaneRoute.params,
},
});
}

It gets the routeID from getTopMostReportIDFromRHP,

if (topmostBottomTabRoute.name === SCREENS.HOME) {
return {name: centralPaneName, params: {reportID: getTopMostReportIDFromRHP(state)}};
}

but it returns routeID as '' because there is no RHP in the state.

When there is no reportID in the params, we find the lastAccessedReportID here:

if (route.params.reportID) {
const reportActionID = route?.params?.reportActionID;
const isValidReportActionID = ValidationUtils.isNumeric(reportActionID);
if (reportActionID && !isValidReportActionID) {
navigation.setParams({reportActionID: ''});
}
return;
}
const lastAccessedReportID = ReportUtils.findLastAccessedReport(!canUseDefaultRooms, !!route.params.openOnAdminRoom, activeWorkspaceID)?.reportID;

For first-time Expensify users, the lastAccessedReportID returns the first non-Concierge report's reportID:

App/src/libs/ReportUtils.ts

Lines 1338 to 1345 in 00bc167

if (isFirstTimeNewExpensifyUser) {
// Filter out the systemChat report from the reports list, as we don't want to drop the user into that report over Concierge when they first log in
reportsValues = reportsValues.filter((report) => !isSystemChat(report)) ?? [];
if (reportsValues.length === 1) {
return reportsValues[0];
}
return adminReport ?? reportsValues.find((report) => !isConciergeChatReport(report));

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:

App/src/libs/ReportUtils.ts

Lines 1356 to 1358 in 00bc167

const lastRead = getMostRecentlyVisitedReport(reportsValues, allReportMetadata);
return adminReport ?? lastRead;

We can use the same logic for first-time Expensify users as well:
return adminReport ?? reportsValues.find((report) => !isConciergeChatReport(report));

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.

@zanyrenney zanyrenney added the External Added to denote the issue can be worked on by a contributor label Sep 13, 2024
@melvin-bot melvin-bot bot changed the title When navigate to Workspace Editor then back to Inbox, it is taking me to my self DM rather than last chat [$250] When navigate to Workspace Editor then back to Inbox, it is taking me to my self DM rather than last chat Sep 13, 2024
Copy link

melvin-bot bot commented Sep 13, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 13, 2024
Copy link

melvin-bot bot commented Sep 13, 2024

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

@zanyrenney
Copy link
Contributor

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

@jayeshmangwani
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

c3024 commented Sep 16, 2024

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.

57de036#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
Copy link
Contributor

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

Copy link

melvin-bot bot commented Sep 16, 2024

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 16, 2024
Copy link

melvin-bot bot commented Sep 16, 2024

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

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 17, 2024
@muttmuure muttmuure moved this to CRITICAL in [#whatsnext] #quality Sep 30, 2024
@bondydaa
Copy link
Contributor

looks like this was deployed to prod but maybe automation failed? #49324 (comment)

@c3024
Copy link
Contributor

c3024 commented Oct 1, 2024

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

@zanyrenney zanyrenney changed the title [$250] When navigate to Workspace Editor then back to Inbox, it is taking me to my self DM rather than last chat 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 Oct 1, 2024
@zanyrenney
Copy link
Contributor

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

@zanyrenney zanyrenney removed their assignment Oct 2, 2024
@zanyrenney zanyrenney removed the Bug Something is broken. Auto assigns a BugZero manager. label Oct 2, 2024
@zanyrenney zanyrenney assigned zanyrenney and unassigned zanyrenney Oct 2, 2024
@zanyrenney zanyrenney added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 2, 2024
Copy link

melvin-bot bot commented Oct 2, 2024

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
Copy link
Contributor

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

@joekaufmanexpensify
Copy link
Contributor

Sounds good! Happy to help here.

@joekaufmanexpensify
Copy link
Contributor

Payment here is:

@joekaufmanexpensify
Copy link
Contributor

All set for Friday!

@joekaufmanexpensify joekaufmanexpensify added Weekly KSv2 and removed Daily KSv2 labels Oct 2, 2024
@joekaufmanexpensify
Copy link
Contributor

All set to pay!

@joekaufmanexpensify
Copy link
Contributor

@c3024 $250 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

Upwork job closed.

@joekaufmanexpensify
Copy link
Contributor

Closing as all that's left is for @jayeshmangwani to request $250 via NewDot whenever you're ready!

@garrettmknight
Copy link
Contributor

$250 approved for @jayeshmangwani

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. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Development

No branches or pull requests

8 participants