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

techievivek commented 1 year ago

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.:

Views #social
Views DM with Vit
Leaves DM with Vit
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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

techievivek commented 1 year ago

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

FitseTLT commented 1 year ago

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 https://github.com/Expensify/App/blob/3db5ff4a1924156c748291d25eb9d774d82f879e/src/libs/actions/Report.js#L2074-L2078

dukenv0307 commented 1 year ago

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

https://github.com/Expensify/App/blob/8ad83451972102ea0c638498424219b5614305c7/src/pages/home/ReportScreen.js#L313-L322

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. https://github.com/Expensify/App/blob/8ad83451972102ea0c638498424219b5614305c7/src/pages/home/ReportScreen.js#L313-L322

I think we should also remove this logic here because we already have navigate logic when leaving room in ReportScreen https://github.com/Expensify/App/blob/3db5ff4a1924156c748291d25eb9d774d82f879e/src/libs/actions/Report.js#L2074-L2077

What alternative solutions did you explore? (Optional)

rayane-djouah commented 1 year ago

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: https://github.com/Expensify/App/blob/c79529a66a05c6a924e24955229c6949ba198623/src/libs/actions/Report.js#L2074-L2078

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 commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

techievivek commented 1 year ago

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

getusha commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

Proposal

Updated

dukenv0307 commented 1 year ago

@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 commented 1 year ago

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

techievivek commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Upwork job price has been updated to $1000

techievivek commented 1 year ago

Doubled the pricing since this will require a little work debugging the navigation stack.

dukenv0307 commented 1 year ago

@techievivek Can you check my comment here https://github.com/Expensify/App/issues/30778#issuecomment-1792340348?

getusha commented 1 year ago

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.

In this case we can filter out the screen, i don't think it differs from filtering from stored paths, if take that approach.

techievivek commented 1 year ago

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

I see, but when I am testing this locally it correctly takes me back to the chats I accessed even after I have refreshed the page, how does that work? Is it because of findLastAccessedReport?

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.

I am not sure how this is a problem. Can you explain this with an example of how you think this could be a problem? For now, I think we know which chat the user is leaving so we can look at the content of the navigation stack from the top and find the first report which doesn't match the given reportID and that will be our destination, no? The important thing that we will need to consider here is that we will need to pop all the entries until the destination reportID.

getusha commented 1 year ago

I think we should handle the following cases.

Case 1

Open DM with User A
Open #room1
Leave #room1
Drops to DM with User A

Case 2

Open #room1
Open #room2
Leave #room2
Drops to #room1
Leave #room1
Drops to the chat before room1 if there is any, otherwise navigate to Concierge

Case 3

Open DM with User A
Open #room1
Reload the page
Leave #room1
Drops to DM with User A

getusha commented 1 year ago

Advertised on slack https://expensify.slack.com/archives/C01GTK53T8Q/p1699258134099569

dukenv0307 commented 1 year ago

I am not sure how this is a problem. Can you explain this with an example of how you think this could be a problem?

Here is an example.

We are in report A
Visit report B
Re-visit A

You can see navigation state in the video which we have two routes of report A. So it's hard to use navigation stack to detect the last viewed report.

https://github.com/Expensify/App/assets/129500732/9e6547bd-aae4-4137-8823-ff873212bf09

rayane-djouah commented 1 year ago

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: https://github.com/Expensify/App/blob/c79529a66a05c6a924e24955229c6949ba198623/src/libs/actions/Report.js#L2074-L2078

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

we should implement a new function in Navigation.js called navigateToPreviousReport. This function will determine the previous chat that the user was on

function navigateToPreviousReport() {
    const navigationState = navigationRef.current.getState();

    console.log(navigationState);

    let currentReportID = null;
    let lastCentralPaneNavigator = null;

    for (let i = navigationState.routes.length - 1; i >= 0; i--) {
        const route = navigationState.routes[i];

        if (route.name === "CentralPaneNavigator") {
            lastCentralPaneNavigator = route;
            currentReportID = route.params.params.reportID;
            console.log("Found CentralPaneNavigator with Report ID:", currentReportID);
            break;
        }
    }

    console.log("Current Report ID:", currentReportID);
    console.log("Last Central Pane Navigator:", lastCentralPaneNavigator);

    if (lastCentralPaneNavigator && currentReportID) {
        let previousReportID = null;

        outerLoop: for (let i = navigationState.routes.length - 1; i >= 0; i--) {
            const route = navigationState.routes[i];

            if (route === lastCentralPaneNavigator) {
                console.log("Skipping lastCentralPaneNavigator route");
                continue;
            }

            if (route.name === "CentralPaneNavigator") {
                console.log("route.params.params.reportID:");
                console.log(route.params.params.reportID);
                if (route.params.params.reportID !== currentReportID) {
                    previousReportID = route.params.params.reportID;
                    console.log("Found previous CentralPaneNavigator with Report ID:", previousReportID);
                    break outerLoop;
                }
            }
        }

        console.log("Previous Report ID:", previousReportID);

        if (previousReportID) {
            navigate(ROUTES.REPORT_WITH_ID.getRoute(previousReportID));
            console.log("Navigating to Previous Report ID:", previousReportID);
            return true;
        } else {
            console.log("No previous Report ID found. Navigation not performed.");
            return false;
        }
    } else {
        console.log("CentralPaneNavigator or currentReportID not found. Navigation not performed.");
            return false;
    }
}

and the navigation logic should be updated as follows:

    if (isWorkspaceMemberLeavingWorkspaceRoom) {
     Navigation.dismissModal();
     const didNavigate = Navigation.navigateToPreviousReport();
      if(!didNavigate){
      //if there is no previous report found 
      //On mobile we always get here because the navigation state is reset
           const participantAccountIDs = PersonalDetailsUtils.getAccountIDsByLogins([CONST.EMAIL.CONCIERGE]); 
           const chat = ReportUtils.getChatByParticipants(participantAccountIDs); 
           Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(chat.reportID)); 
           //On mobile this go to the chats screen
           Navigation.navigate(ROUTES.HOME); 

      }
    }

Result:

https://github.com/Expensify/App/assets/77965000/41193003-ce0d-4f17-9b0b-7b73091bee04

What alternative solutions did you explore? (Optional)

rojiphil commented 1 year ago

Late to the party but interesting conversation here. Going through the previous comments to see if I can put in my 2 cents here.

rayane-djouah commented 1 year ago

Proposal

Updated

cc @getusha

techievivek commented 1 year ago

@getusha Can you please prioritize reviewing the above proposals? 🙏

getusha commented 1 year ago

@sourcecodedeveloper could you please share your branch so i can test it?

@techievivek based on the complexity of getting the last accessed report from the navigation state, it's must to store a stack of report ids to solve this issue.

jeet-dhandha commented 1 year ago

Proposal

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

We want user to be able to go back to previous accessible chat once they leave the room.
Currently there are two ways user can leave the room:
- Click on the Leave Room from Details
- Or be removed from the Workspace.

What is the root cause of that problem?

Its a new feature that we are adding to the app.
Another issue is that currently lastReadTime only updates when user reads some message by marking it as unread, else it won't update.

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

We will need to create a findLastAccessibleReportID function that will return the last accessible report ID.
- This function will sort all the reports using sortReportsByLastRead function.
- After that we will filter reports using shouldReportBeInOptionList function, as we want user to navigate to an accessible report.
- Finally we will add a parameter to findLastAccessibleReportID function that which will be like filterReportIDs, to filter the current room from the list.
- Also we will need to filter on report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN this condition for notification preference as when user leaves room we set notificationPreference to hidden to mark it as user left room.
- Lastly we will return the last report ID from the list.

We will use this leaveRoom function https://github.com/Expensify/App/blob/d5725c52bead4baa02ecec38915fc7c8c4316a4a/src/libs/actions/Report.js#L2024

and we will add below code in isWorkspaceMemberLeavingWorkspaceRoom condition scope.

const lastAccessibleReportID = ReportUtils.findLastAccessibleReportID( [reportID] );
if (lastAccessibleReportID) {
   Navigation.dismissModal(lastAccessibleReportID);
   return;
}

We will also need to update ReportScreen.js. https://github.com/Expensify/App/blob/d5725c52bead4baa02ecec38915fc7c8c4316a4a/src/pages/home/ReportScreen.js#L303-L312

And we will add same code as above here but without any parameters. As this is triggered without any action from user, and triggered when user is removed of the access from active chat.

Removing User From Workspace

https://github.com/Expensify/App/assets/78416198/eef9b4d0-b41b-402d-a9e3-5cb52b2118ea

Leaving From Room

https://github.com/Expensify/App/assets/78416198/0de73c0a-e11f-4975-af5f-f6be8b0ff182

Leaving From Room1 and Then From Room2

https://github.com/Expensify/App/assets/78416198/6f1244a3-2283-4ba1-aa72-051f2249c416

Refresh chat From Room1 and Then Leave (Sorry For Long Video, main had some scroll bug so wan't able to do it quickly)

https://github.com/Expensify/App/assets/78416198/5812ab54-2769-40fd-83e5-55cfda3a6b68

Please Use Below PR for testing

PR To Test: https://github.com/Expensify/App/pull/31003

What alternative solutions did you explore? (Optional)

N/A

techievivek commented 1 year ago

@getusha

based on the complexity of getting the last accessed report from the navigation state, it's must to store a stack of report ids to solve this issue.

I don't find much benefit in keeping a collection of reportIDs apart from just helping with this particular use case, and it's possible that handling the associated logic could be complicated as well. I would still stick with creating a helper method to retrieve the reportID from the navigation state, even if that's a bit of work.

getusha commented 1 year ago

@rayane-djouah your proposal is not working.

https://github.com/Expensify/App/assets/75031127/62987c2b-84c9-47eb-ba8f-fa619b90c0cc

rayane-djouah commented 1 year ago

@getusha I tested it before and it was working. Could you please check if there is any console error?

getusha commented 1 year ago

@rayane-djouah there are no console errors

rayane-djouah commented 1 year ago

I will check it out during the day

eh2077 commented 1 year ago

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?

The root cause of this issue if that we redirect page to concierge after leave a room, see

https://github.com/Expensify/App/blob/da2c79e52860e6b367e92cda70c827e52c0edd34/src/libs/actions/Report.js#L2080-L2084

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

To drop user to the most recently access chat, we need to trace the access records of opened reportIDs/chats and navigate to the most recent reportID after leaving a room.

To achieve it,

Add a state event listener of the Report screen by changing this line

https://github.com/Expensify/App/blob/da2c79e52860e6b367e92cda70c827e52c0edd34/src/libs/Navigation/AppNavigator/Navigators/CentralPaneNavigator/BaseCentralPaneNavigator.js#L15

to

        <Stack.Navigator
            screenListeners={{
                state: (e) => {
                    const reportID = lodashGet(e, 'data.state.routes[0].params.reportID', '');
                    if (reportID) {
                        console.log('state changed, reportID = ', reportID);
                        Report.updateRecentlyAccessedReportID(reportID);
                    }
                },
            }}
        >

See https://reactnavigation.org/docs/navigation-events#state

Add a method updateRecentlyAccessedReportID in Report.js to trace the more recently accessed reportIDs, like

let recentlyAccessedReportIDs = [];
Onyx.connect({
    key: ONYXKEYS.RECENTLY_ACCESSED_REPORT_ID_LIST,
    waitForCollectionCallback: true,
    callback: (val) => {
        recentlyAccessedReportIDs = val;
    },
});

/**
 * Update recently accessed reportID
 *
 * @param {String} currentReportID
 * @param {Boolean} shouldRemoveCurrentReportID
 * @returns {String} Returns the most recent accessed reportID
 */
function updateRecentlyAccessedReportID(currentReportID, shouldRemoveCurrentReportID = false) {
    const reportIDs = _.filter(recentlyAccessedReportIDs, (reportID) => reportID !== currentReportID);
    if (!shouldRemoveCurrentReportID) {
        reportIDs.push(currentReportID);
    }

    // We can discuss the number of reportIDs need to be saved
    if (reportIDs.length > 10) {
        reportIDs.shift();
    }

    console.log('current recentlyAccessedReportIDs = ', reportIDs);

    // We want to save recently accessed reportIDs to Onyx to be used after refreshing the page
    Onyx.set(ONYXKEYS.RECENTLY_ACCESSED_REPORT_ID_LIST, reportIDs);

    return reportIDs.length > 0 ? reportIDs[reportIDs.length - 1] : '';
}

Change

https://github.com/Expensify/App/blob/da2c79e52860e6b367e92cda70c827e52c0edd34/src/libs/actions/Report.js#L2080-L2084

to

    const lastAccessedReportID = updateRecentlyAccessedReportID(reportID, true);
    if (lastAccessedReportID) {
        Navigation.dismissModal(lastAccessedReportID);
    } 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)

N/A

eh2077 commented 1 year ago

@techievivek @getusha I also proposed a solution which uses state event listener to trace the opened reportID instead of inspecting the state stack by brute force method. The code will be clean and maintainable. For example, in method updateRecentlyAccessedReportID we can get the report by reportID and apply necessary checks of report for future functions.

You can checkout this branch https://github.com/Expensify/App/compare/main...eh2077:App:30778-drop-user-to-most-recently-accessed-report-after-leaving-a-room, if you want to test it.

getusha commented 1 year ago

@eh2077 Please check this comment, we're not looking to store report IDs in Onyx.

jeet-dhandha commented 1 year ago

@getusha any comments on my proposal https://github.com/Expensify/App/issues/30778#issuecomment-1797115161

getusha commented 1 year ago

@jeet-dhandha I will test your proposal, will it handle all of these cases?

rayane-djouah commented 1 year ago

Proposal

Updated

cc @getusha

getusha commented 1 year ago

@sourcecodedeveloper please check this comment https://github.com/Expensify/App/issues/30778#issuecomment-1798298915

eh2077 commented 1 year ago

@getusha

based on the complexity of getting the last accessed report from the navigation state, it's must to store a stack of report ids to solve this issue.

I don't find much benefit in keeping a collection of reportIDs apart from just helping with this particular use case, and it's possible that handling the associated logic could be complicated as well. I would still stick with creating a helper method to retrieve the reportID from the navigation state, even if that's a bit of work.

@techievivek I understand your concern that the solution will be hard to maintain and be complicated extend to new features if we find the reportID by inspecting the navigation state using brute force method.

So, I went through doc of react-native-navigation and find that it provides event listener feature to help us trace the changes of navigator, see https://reactnavigation.org/docs/navigation-events/#state

By using the event listener, we can have an elegant way to trace accessed reportIDs and the stored reportIDs can be easily extended to new future features. See the updateRecentlyAccessedReportID method in my proposal and branch. Given the solution is clean and maintainable, I think we can consider storing reportIDs.

Looking forward to your feedback. Appreciated!

jeet-dhandha commented 1 year ago

I have added videos for two scenarios, i will add for remaining scenarios in few hours. Only one scenario is covered in the videos, rest 2 i will add.

Expensify / App