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

[HOLD for payment 2024-02-07] [HOLD 30868][$1000] Drop user back to the last chat they were on when they leave a room. #30778

Closed
techievivek opened this issue Nov 2, 2023 · 130 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement.

Comments

@techievivek
Copy link
Contributor

techievivek commented Nov 2, 2023

Problem:

Currently, if you leave a Room or Group you are dropped into your Concierge DM. The best reasoning for this is that you might have a support question or request. This is an assumption that, when weighed up against other options, feels less intuitive or useful.

Solution:

Drop users into whatever chat they were previously viewing before the viewed the room or group they left. Dropping a user into a chat they were just viewing is more likely to be useful than dropping a user into a Concierge DM.

E.g.:

  1. Views #social
  2. Views DM with Vit
  3. Leaves DM with Vit
  4. Drops into #social

On mobile, you should be dropped into the "chats screen".
Let me know if you have any questions or need clarification with anything, thanks.

This was reported here: https://expensify.slack.com/archives/C049HHMV9SM/p1697506231709359

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c9dae839c28297cd
  • Upwork Job ID: 1720056216593899520
  • Last Price Increase: 2023-11-06
  • Automatic offers:
    • getusha | Reviewer | 27580831
    • eh2077 | Contributor | 27580835
@techievivek techievivek added External Added to denote the issue can be worked on by a contributor Daily KSv2 Improvement Item broken or needs improvement. Help Wanted Apply this label when an issue is open to proposals by contributors labels Nov 2, 2023
@techievivek techievivek self-assigned this Nov 2, 2023
@melvin-bot melvin-bot bot changed the title Drop user back to the last chat they were on when they leave a room. [$500] Drop user back to the last chat they were on when they leave a room. Nov 2, 2023
Copy link

melvin-bot bot commented Nov 2, 2023

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

Copy link

melvin-bot bot commented Nov 2, 2023

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

@techievivek
Copy link
Contributor Author

This improvement was discussed here: https://expensify.slack.com/archives/C049HHMV9SM/p1697506231709359

@FitseTLT
Copy link
Contributor

FitseTLT commented Nov 2, 2023

Proposal

I would like to work on it. According to the comment on the issue we need to change this lines of code to navigate the user to the last report using ReportUtils.findLastAccessedReport

App/src/libs/actions/Report.js

Lines 2074 to 2078 in 3db5ff4

if (isWorkspaceMemberLeavingWorkspaceRoom) {
const participantAccountIDs = PersonalDetailsUtils.getAccountIDsByLogins([CONST.EMAIL.CONCIERGE]);
const chat = ReportUtils.getChatByParticipants(participantAccountIDs);
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(chat.reportID));
}

@dukenv0307
Copy link
Contributor

dukenv0307 commented Nov 2, 2023

Proposal

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

Drop user back to the last chat they were on when they leave a room

What is the root cause of that problem?

Actually when we leave room, we navigate to parent report if it's a thread, otherwise we will navigate to concierge chat

Navigation.dismissModal();
if (Navigation.getTopmostReportId() === prevOnyxReportID) {
Navigation.setShouldPopAllStateOnUP();
Navigation.goBack(ROUTES.HOME, false, true);
}
if (prevReport.parentReportID) {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(prevReport.parentReportID));
return;
}
Report.navigateToConciergeChat();

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

We can store a value in Onyx like LAST_VIEWED_REPORT_ID. Whenever reportID from route is changed we will update this key if this reportID is valid.

And then when we leave the room we will goBack to the last report that is stored from this key. If the room is the first room we viewed we will calculate the last access reportID base on findLastAccessedReport with excluding this room.

Navigation.dismissModal();
if (Navigation.getTopmostReportId() === prevOnyxReportID) {
Navigation.setShouldPopAllStateOnUP();
Navigation.goBack(ROUTES.HOME, false, true);
}
if (prevReport.parentReportID) {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(prevReport.parentReportID));
return;
}
Report.navigateToConciergeChat();

I think we should also remove this logic here because we already have navigate logic when leaving room in ReportScreen

App/src/libs/actions/Report.js

Lines 2074 to 2077 in 3db5ff4

if (isWorkspaceMemberLeavingWorkspaceRoom) {
const participantAccountIDs = PersonalDetailsUtils.getAccountIDsByLogins([CONST.EMAIL.CONCIERGE]);
const chat = ReportUtils.getChatByParticipants(participantAccountIDs);
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(chat.reportID));

What alternative solutions did you explore? (Optional)

@rayane-djouah
Copy link
Contributor

rayane-djouah commented Nov 2, 2023

Proposal

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

We want to drop the user back to the last chat they were on when they leave a room, instead of droping them to Concierge chat.

What is the root cause of that problem?

We are navigating to concierge chat after leaving the room here:

App/src/libs/actions/Report.js

Lines 2074 to 2078 in c79529a

if (isWorkspaceMemberLeavingWorkspaceRoom) {
const participantAccountIDs = PersonalDetailsUtils.getAccountIDsByLogins([CONST.EMAIL.CONCIERGE]);
const chat = ReportUtils.getChatByParticipants(participantAccountIDs);
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(chat.reportID));
}

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

we should implement a new function in ReportUtils called findPreviousAccessedReport. This function will determine the previous chat that the user was on

function findPreviousAccessedReport(reports) {
    let sortedReports = sortReportsByLastRead(reports);

    // Get the last report in the array, which is the current report.
    const currentReport = sortedReports[sortedReports.length - 1];

    // Find the index of the current report in the array.
    const currentIndex = sortedReports.indexOf(currentReport);

    if (currentIndex > 0) {
        // If the current report is not the first report in the array,
        // return the report before it as the previously accessed report.
        return sortedReports[currentIndex - 1];
    } else {
        // If the current report is the first report in the array,
        // there is no previously accessed report.
        return null;
    }
}

and the navigation logic should be updated as follows:

    if (isWorkspaceMemberLeavingWorkspaceRoom) {
        const previousReport = ReportUtils.findPreviousAccessedReport(allReports);
        if(previousReport){
             Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(previousReport.reportID));
        } else {
            const participantAccountIDs = PersonalDetailsUtils.getAccountIDsByLogins([CONST.EMAIL.CONCIERGE]);
            const chat = ReportUtils.getChatByParticipants(participantAccountIDs);  
            Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(chat.reportID));      
        }
    }

What alternative solutions did you explore? (Optional)

We can go back twice to get into the previous chat.

    if (isWorkspaceMemberLeavingWorkspaceRoom) {
        Navigation.goBack();
        Navigation.goBack();
    }

@orshikh-dev
Copy link

Contributor details
Your Expensify account email: orshikh.dev@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01477bce71bc1bc694

Copy link

melvin-bot bot commented Nov 2, 2023

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

@techievivek
Copy link
Contributor Author

@getusha Can you please review the above proposal, this is part of wave8 milestone and we want to get this shipped ASAP, thanks.

@getusha
Copy link
Contributor

getusha commented Nov 2, 2023

We can store a value in Onyx like LAST_VIEWED_REPORT_ID. Whenever reportID from route is changed we will update this key if this reportID is valid.

@dukenv0307 is there any reason, to store the value in Onyx? can't we do it without introducing a new key?

@dukenv0307
Copy link
Contributor

@getushai think we can create a ref like prevReportID in ReportScreen to store the previous reportID. That will accept this will be reset when we refresh the page.

If it's not the expected we can use a new Onyx key.

@getusha
Copy link
Contributor

getusha commented Nov 2, 2023

@dukenv0307 could you point me where we will update the value of the ref from? i feel like there is a better way to get the navigation history and use it for this case instead of storing a new value.

@dukenv0307
Copy link
Contributor

We will use usePrevious hook to store the previous reportID and then whenever the previous and the current reporrID is different we will update the ref to previous value.

@getusha
Copy link
Contributor

getusha commented Nov 2, 2023

@dukenv0307 If the previous report is another room and when we leave that room again where will the navigation land? in this case we will have the previous room path saved and it'll navigate to a room that doesn't exist, that's why i am not confidence in using a ref or any sort of state to store the path.

@techievivek Should we also address the scenario of a reload? In my opinion, I believe we should simply navigate to the Concierge in such cases.

@rayane-djouah
Copy link
Contributor

Proposal

Updated

@dukenv0307
Copy link
Contributor

@getusha In this case I think we should go to the last access to prevent this case or do nothing or when we leave room, we can reuse getTopmostReportId with excluding the room that we are leaving to get the last reportID.

cc @techievivek can you please confirm the expected again.

@getusha
Copy link
Contributor

getusha commented Nov 2, 2023

@Sourcecodedeveloper i was testing your proposal is there any reason you removed it for?

@techievivek
Copy link
Contributor Author

We can store a value in Onyx like LAST_VIEWED_REPORT_ID. Whenever reportID from route is changed we will update this key if this reportID is valid.

I don't think storing a single reportID will work, if we need to make it work we will need to store a stack of reportIDs in the order they were accessed. But that info is already available to us through navigation stack so maybe we can just reuse that?

@techievivek
Copy link
Contributor Author

@getusha

Should we also address the scenario of a reload? In my opinion, I believe we should simply navigate to the Concierge in such cases.

Do we reset the navigation stack when we reload the page, or does it persist? I would guess it persists so can't we figure out the last report they were on from it?

@techievivek
Copy link
Contributor Author

Can we not use findLastAccessedReport or getTopmostReportId to find the last report the user was on? If, in any case, we are unable to determine, then we can drop them to concierge chat.

@getusha
Copy link
Contributor

getusha commented Nov 3, 2023

But that info is already available to us through navigation stack so maybe we can just reuse that?

Yes i also that's something we should reuse. @dukenv0307 @rayane-djouah could you take a look at this approach and update your proposals if possible?

Do we reset the navigation stack when we reload the page, or does it persist? I would guess it persists so can't we figure out the last report they were on from it?

It think the state will reset incase of a reload, can confirm that logging navigationRef.getState().

@dukenv0307
Copy link
Contributor

@getusha I'm pretty sure the the stack is reset after reloading.

We have a problem that is if we visit a report many time. Navigation stack will have many screen of this report. And then when we leave this room, in navigation stack still have this screen.

@techievivek techievivek changed the title [$500] Drop user back to the last chat they were on when they leave a room. [$1000] Drop user back to the last chat they were on when they leave a room. Nov 6, 2023
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 31, 2024
@melvin-bot melvin-bot bot changed the title [HOLD 30868][$1000] Drop user back to the last chat they were on when they leave a room. [HOLD for payment 2024-02-07] [HOLD 30868][$1000] Drop user back to the last chat they were on when they leave a room. Jan 31, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jan 31, 2024
Copy link

melvin-bot bot commented Jan 31, 2024

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

Copy link

melvin-bot bot commented Jan 31, 2024

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

If no regressions arise, payment will be issued on 2024-02-07. 🎊

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

This comment was marked as off-topic.

@getusha
Copy link
Contributor

getusha commented Feb 1, 2024

^ false alarm

@techievivek
Copy link
Contributor Author

Adding bug label so a BZ member can help with the payment here, thanks for working on this.

@techievivek techievivek added the Bug Something is broken. Auto assigns a BugZero manager. label Feb 2, 2024
Copy link

melvin-bot bot commented Feb 2, 2024

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 2, 2024
@techievivek
Copy link
Contributor Author

@garrettmknight The bug has already been fixed and deployed, I added the bug label so a BZ member can help with the payment #30778 (comment), thanks.

@melvin-bot melvin-bot bot added the Overdue label Feb 5, 2024
@techievivek
Copy link
Contributor Author

Not overdue, the PR has already been merged, and we are currently in the regression testing phase.

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Overdue Daily KSv2 labels Feb 5, 2024
@garrettmknight
Copy link
Contributor

garrettmknight commented Feb 8, 2024

All paid out - adding the bugzero checklist for @getusha

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@getusha ] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@garrettmknight] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@melvin-bot melvin-bot bot removed the Overdue label Feb 8, 2024
@getusha
Copy link
Contributor

getusha commented Feb 8, 2024

Regression Test Proposal

Case 1

  • Open a chat with User X
  • Create a new room named room-1
  • Leave room-1
  • Verify that you are navigated back to the chat with User X.

Case 2

  • Open a chat with User X
  • Create a new room named room-2 and room-3
  • Open room-2
  • Open room-3
  • Leave room-3
  • Verify that you are navigated back to room-2
  • Leave room-2
  • Verify that you are navigated back to the chat with User X.

Do we agree 👍 or 👎

@getusha
Copy link
Contributor

getusha commented Feb 8, 2024

cc @techievivek @eh2077 ^

@garrettmknight
Copy link
Contributor

All set!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement.
Projects
None yet
Development

No branches or pull requests