[$1000] Chat - Landing in recent chat if login via link with user you don't have chat with

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

Precondition - User A has chat with User B. User C doesn't have chat with user B. Login as user A.

1. Open http://staging.new.expensify.com/
2. Open chat with user B
3. Copy link to the chat
4. Log out of the account
5. "Paste & go" copied link
6. Login with account of user C.

Expected Result:

User lands in "You don't have access to this chat"

Actual Result:

User lands in the most recent chat

Workaround:

Unknown|

Platforms:

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

• [ ] Android / native
• [ ] Android / Chrome
• [x] iOS / native
• [x] iOS / Safari
• [x] MacOS / Chrome / Safari
• [ ] MacOS / Desktop

Version Number: 1.2.97.2

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

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/230742906-6153e1f5-c253-41df-848b-95997658c5bb.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/~0194a9f940ab105839
• Upwork Job ID: 1646643876962811904
• Last Price Increase: 2023-04-13

[MelvinBot]

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

[MelvinBot]

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
• [x] 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.
• [x] 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.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[sakluger]

Verified that this is not deeplinking to the correct page. The issue only occurs when I go to that link while not signed in. If i'm already signed into the 3rd account, then it brings me to the "You don't have access to that chat" page correctly.

[MelvinBot]

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

[MelvinBot]

Current assignee @sakluger is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

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

[MelvinBot]

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

[dukenv0307]

Proposal

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

User always lands in the most recent chat when they access the chat by link/deep link into the chat before they log in.

This not only happens with a "user you don't have chat with" but it happens for all chats, no matter which chat you try to get in using the link, you'll always land on the most recent chat.

What is the root cause of that problem?

It's here https://github.com/Expensify/App/blob/4607dcb69b6d5bb7607f652f8a78327c7e002183/src/libs/Navigation/AppNavigator/MainDrawerNavigator.js#L103, we're always setting the chat to the most recent chat after the user logs in.

Here're what happens:

1. User tries to navigate to the chat /r/123 link
2. User is not logged in yet, so they have to login
3. After logged in, now the initialParams.reportID is always empty so according to this logic https://github.com/Expensify/App/blob/4607dcb69b6d5bb7607f652f8a78327c7e002183/src/libs/Navigation/AppNavigator/MainDrawerNavigator.js#L100, the report screen will be reset to the most recent chat's reportID, ignoring the chat that the user is trying to get to in 1.

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

We need to check if the user is trying to get to a chat before logging in. If yes then do not override the chat with the most recent chat.

That means replacing this logic https://github.com/Expensify/App/blob/4607dcb69b6d5bb7607f652f8a78327c7e002183/src/libs/Navigation/AppNavigator/MainDrawerNavigator.js#L103 with something like:

const reportScreenReportID = lodashGet(state, 'routes[0].state.routes[0].params.reportID', '');

if (!reportScreenReportID) {
    Navigation.setParams(initialNextParams, reportScreenKey);
}

What alternative solutions did you explore? (Optional)

The above fix can be improved a bit by avoiding calculation of the initialParams in the first place if we see that the reportID is already present in the ReportScreen navigation param.

[MelvinBot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[MelvinBot]

@bondydaa, @sakluger, @sobitneupane Whoops! This issue is 2 days overdue. Let's get this updated quick!

[bondydaa]

Thanks for the write up @dukenv0307, the only thing I don't think we should be doing is diving into the state object like lodashGet(state, 'routes[0].state.routes[0].params.reportID', '');.

I'm wondering if instead we should do something like save the reportID from the url of /r/123 into a global state (if we have that?) or maybe into a client only onyx key (not sure this is a thing either) and then as you've said when we get to https://github.com/Expensify/App/blob/4607dcb69b6d5bb7607f652f8a78327c7e002183/src/libs/Navigation/AppNavigator/MainDrawerNavigator.js#L100 look for that value and if it exists trigger that we load it?

[sobitneupane]

@dukenv0307 We are expecting "You don't have access to this chat" screen if the reportID cannot be accessed by the user. Will your proposal yield the expected output? While testing, I could not get the expected output, the report keeps on loading instead.

[dukenv0307]

While testing, I could not get the expected output, the report keeps on loading instead.

@sobitneupane I just tested again and my proposal works well.

https://user-images.githubusercontent.com/129500732/232703538-cafdac9a-36d3-451f-90bd-11a3a06e07db.mp4

Could you share the video that you tested?

[dukenv0307]

@bondydaa We also can come up with your suggestion If do that, we can set new field (reportScreenReportID) on ONYX in here https://github.com/Expensify/App/blob/e702c3a45934767da5a5da5f70f642e0da89bcb3/src/Expensify.js#L154 And then we also need to replace this logic https://github.com/Expensify/App/blob/4607dcb69b6d5bb7607f652f8a78327c7e002183/src/libs/Navigation/AppNavigator/MainDrawerNavigator.js#L103 with something like:

            const reportScreenReportID = this.props.reportScreenReportID

            if (!reportScreenReportID) {
                Navigation.setParams(initialNextParams, reportScreenKey);
            }

It also works for me

cc @sobitneupane

[sobitneupane]

We are expecting "You don't have access to this chat" screen if the reportID cannot be accessed by the user. Will your proposal yield the expected output? While testing, I could not get the expected output, the report keeps on loading instead.

My bad. I could not reproduce it any longer.

The proposal from @dukenv0307 looks good to me. 🎀👀🎀 C+ reviewed

cc: @bondydaa

[MelvinBot]

📣 @dukenv0307 You have been assigned to this job by @bondydaa! Please apply to this job in Upwork 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]

assigning @dukenv0307 but I just want to clarify which proposal @sobitneupane agreed with?

https://github.com/Expensify/App/issues/17188#issuecomment-1509764446

or

https://github.com/Expensify/App/issues/17188#issuecomment-1512830004

[sobitneupane]

@bondydaa I prefer the prior proposal. The only reason is urls are subject to change, and change in url might break the solution if we use url to retrieve reportID.

[dukenv0307]

In my opinion, we should come up with my first solution because it is simple and we also use the same thing in https://github.com/Expensify/App/blob/4607dcb69b6d5bb7607f652f8a78327c7e002183/src/libs/Navigation/AppNavigator/MainDrawerNavigator.js#L102 The @bondydaa solution also be good but I see that it seems a bit complicated. @bondydaa What do your thought?

[bondydaa]

oh wow I didn't realize we were already accessing something out of state like that 🤔.

I chatted with @tgolen real quick about that and that instance in MainDrawerNavigator.js is more of a "do as I say, not as I do" type thing. It's a hack that was required in that particular case and should get cleaned up once the navigation library changes but isn't a pattern we should continue to repeat.

I wouldn't worry too much about urls changing, if they do then we'll update the code here as well so it still works (more $$ for contributors to fix another bug 😁). You could make the same argument that react could change the state object at anytime and it would break as well. We can't 100% future proof anything we build so I think both arguments don't hold much weight.

So I think we should go with this https://github.com/Expensify/App/issues/17188#issuecomment-1512830004

[sakluger]

Looks like the PR is still in review, making steady progress. (Adding this comment to make it easier to go through my K2 assigned issues).

[sakluger]

@dukenv0307 what's the status of the PR? Looks like we may be waiting on another issue before we can test it fully?

[dukenv0307]

@sakluger sorry for my delay, we cannot test the PR fully in native because have some bugs when open with deep link in native.

[sakluger]

Looks like the PR is making steady progress and is getting close.

[bondydaa]

I'm dropping this to weekly (if botify will allow me), it's definitely not as important as Manual Request stuff which needs attention for EC3.

[sakluger]

I'm going to move this back to daily since EC3 is happening, so this should be prioritized again.

[sakluger]

Hey @dukenv0307 - just checking in, what's the latest on this one? I don't see any new commits or comments on the PR since May 10, is the PR held on anything?

[dukenv0307]

@sakluger We are discussing in slack about a problem in the PR

[sakluger]

Thanks, here's the Slack thread for posterity: https://expensify.slack.com/archives/C01GTK53T8Q/p1682549395857859

[sakluger]

Looks like we came to a decision on the Slack thread. @bondydaa @dukenv0307 what's the next step here? Does the PR need to be updated now?

[dukenv0307]

@sakluger We make a decision that not drop into the last report If reportID is not present in route. Instead, we will show a welcome screen the same as whatapps like this. That is what we discuss in Slack, wait for confirmation of @bondydaa

[sakluger]

Sounds good, thanks! I'm OOO for the next week so I'm going to reapply the BZ label, @bondydaa let us know how we should proceed here.

[melvin-bot[bot]]

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

[miljakljajic]

Apologies I am just seeing this assignment that must have come in over the weekend - I'm participating in this Guides Experiment and so have temporarily left the BZ team. Reapplying the label.

[melvin-bot[bot]]

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

[stephanieelliott]

Hey @dukenv0307 can you give a status update on this PR please?

[dukenv0307]

@bondydaa can you confirm here the next step we need to do?

[stephanieelliott]

Tossing this one back to you @sakluger since I am about to go OOO tomorrow.

[sakluger]

Bumped Bondy in the Slack thread. He's OOO today so I don't expect a response for a day or two.

[sakluger]

@bondydaa checking in again, mind updating with the next steps here?

[bondydaa]

left a comment on the PR.

To summarize, I think we should still implement the "default view" for when you load new.expensify.com without any additional route and we stop automatically re-loading the most recent chat by default to display.

if a user loads new.expensify.com/settings then it should open that route/modal but with the default view displayed, not a recent chat or the chat they might have previously been viewing.

if a user loads new.expensify.com/r/123, then opens the settings modal/route then the settings modal would display above the report that is loaded but if they reloaded the page then it would just open new.expensify.com/settings with the default view again.

I don't think users care too much about maintaining the last chat they had open between reloads, if they do then they can write in and we can prioritize it then so let's keep it simple for now: if a report route is loaded then load that chat, otherwise display the default view.

[bondydaa]

okay so I'd started another thread here https://expensify.slack.com/archives/C01GTK53T8Q/p1687897551415069 and seems like we've changed course and don't really agree on adding the default screen.

I retested this bug and it doesn't exist anymore so I think the navigation refactor took care of it for us.

but I'm going to close this out for now.

if someone wants to propose the default view as a new feature and to stop auto-loading the most recent chat you can pick up the torch in that thread i linked.

[dukenv0307]

@bondydaa @sakluger Should we have a payment in this issue? Because this issue was assigned and we spent sometimes to work on the PR before we changed the navigation flow to ReportScreenWrapper that fixed this issue.

[sakluger]

I agree that we should have a partial payment for this issue, since work was done before we decided changing direction. I'm going to pay out 50%, I think that's a fair amount given the amount of work done so far.

[sakluger]

I sent offers to @dukenv0307 and @sobitneupane in Upwork - please let me know once you've accepted, thanks!

[sobitneupane]

@sakluger Accepted the offer. Thanks.

[dukenv0307]

@sakluger I've accepted, thank you!

[sakluger]

All paid, thanks!