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.

Sign up for GitHub

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

Fix: Leave thread chat when has no comment #27849

Closed
wants to merge 6 commits into from
Closed

Fix: Leave thread chat when has no comment #27849

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
08337c3
leave room when has no comment
dukenv0307 Sep 20, 2023
844752d
fix: merge main
dukenv0307 Sep 20, 2023
1555286
merge main
dukenv0307 Sep 20, 2023
3a8f8f0
fix: resolve conflict
dukenv0307 Sep 25, 2023
475a7e0
add comment
dukenv0307 Sep 25, 2023
6269ba7
add dependencies to useEffect
dukenv0307 Sep 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1838,8 +1838,9 @@ function getCurrentUserAccountID() {
* Leave a report by setting the state to submitted and closed
*
* @param {String} reportID
* @param {Boolean} shouldNavigate should navigate after leaving room or not
*/
function leaveRoom(reportID) {
function leaveRoom(reportID, shouldNavigate = true) {
const report = lodashGet(allReports, [reportID], {});
const reportKeys = _.keys(report);

Expand Down Expand Up @@ -1886,6 +1887,9 @@ function leaveRoom(reportID) {
},
);
Navigation.dismissModal();
if (!shouldNavigate) {
return;
}
Comment on lines +1890 to +1892
Copy link
Contributor

@fedirjh fedirjh Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this line above the modal dismissal .

Bug: User is navigated to concierge after leaving the thread

  1. Open report
  2. Navigate to thread
  3. Click back button
  4. User should go back to main report
  5. User is navigate to concierge
CleanShot.2023-09-26.at.16.07.37.mp4

Copy link
Contributor Author

@dukenv0307 dukenv0307 Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fedirjh I see the problem, this PR changed the logic

so I don't think just moving that IF condition will work since when we call LeaveRoom, we set the optimistic data of that report statusNum to CLOSED

App/src/libs/actions/Report.js

Line 1868 in fff2dd4

statusNum: CONST.REPORT.STATUS.CLOSED,

In ReportScreen, we have a logic to navigate to Concierge once statusNum is CLOSED

App/src/pages/home/ReportScreen.js

Lines 312 to 321 in fff2dd4

if (
// non-optimistic case
(!prevUserLeavingStatus && userLeavingStatus) ||
// optimistic case
(prevOnyxReportID && prevOnyxReportID === routeReportID && !onyxReportID && prevReport.statusNum === CONST.REPORT.STATUS.OPEN && report.statusNum === CONST.REPORT.STATUS.CLOSED)
) {
Navigation.goBack();
Report.navigateToConciergeChat();
return;
}

Working on this to find a solution

Copy link
Contributor

@fedirjh fedirjh Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one simple solution for this bug is that we add an optimistic prop let's say shouldNavigate and use it to prevent redirection inside ReportScreen.

optimisticData: [
    {
        onyxMethod: Onyx.METHOD.SET,
        key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
        value: {
            stateNum: CONST.REPORT.STATE_NUM.SUBMITTED,
            statusNum: CONST.REPORT.STATUS.CLOSED,                        
            shouldNavigate, // Add this prop
        },
    },
],

// Update the condition to 

if ( 
     // non-optimistic case 
     (!prevUserLeavingStatus && userLeavingStatus) || 
     // optimistic case 
     (... && report.shouldNavigate) 
 ) {

if (Navigation.getTopmostReportId() === reportID) {
Navigation.goBack(ROUTES.HOME);
}
Expand Down
15 changes: 14 additions & 1 deletion src/pages/home/ReportScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, {useRef, useState, useEffect, useMemo, useCallback} from 'react';
import {withOnyx} from 'react-native-onyx';
import {useFocusEffect} from '@react-navigation/native';
import {useFocusEffect, useIsFocused} from '@react-navigation/native';
import PropTypes from 'prop-types';
import {View} from 'react-native';
import lodashGet from 'lodash/get';
Expand Down Expand Up @@ -153,6 +153,7 @@ function ReportScreen({
const reactionListRef = useRef();
const prevReport = usePrevious(report);
const prevUserLeavingStatus = usePrevious(userLeavingStatus);
const isFocused = useIsFocused();

const [skeletonViewContainerHeight, setSkeletonViewContainerHeight] = useState(0);
const [isBannerVisible, setIsBannerVisible] = useState(true);
Expand Down Expand Up @@ -350,6 +351,18 @@ function ReportScreen({
[report, isLoading, shouldHideReport, isDefaultReport, isOptimisticDelete, userLeavingStatus],
);

useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this hook runs when component unmounts ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will

if (isFocused) {
return;
}
// When the report screen is unmounted or no longer in focus, and the user has no comments on that thread, call LeaveRoom
if (ReportUtils.isThread(report) && report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a comment explaining this hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should prefer early return here:

if (!ReportUtils.isThread(report) || report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
    return;
}

Report.leaveRoom(report.reportID, false);

Report.leaveRoom(report.reportID, false);
}

// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why didn’t we pass all deps , basically the report ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should trigger this useEffect once the report loses focus

Copy link
Contributor

@roryabraham roryabraham Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree we don't want to run this whenever any property on the report changes. However, I think we probably want to make sure that the correct report data is used and want to avoid suppressing the lint rule.

So maybe we can workaround the lint rule with something like this:

const isThread = ReportUtils.isThread(report);
const isNotificationPreferenceHidden = report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
const reportID = report.reportID;

// When the report screen is unmounted or no longer in focus, and the user has no comments on that thread, call LeaveRoom
useEffect(() => {
    if (isFocused || !isThread || !isNotificationPreferenceHidden) {
        return;
    }
    Report.leaveRoom(reportID, false);
}, [isFocused, isThread, isNotificationPreferenceHidden, reportID]);

}, [isFocused]);

return (
<ReportScreenContext.Provider
value={{
Expand Down
Loading