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 option does not appear in group chat thread #40529

Merged
merged 23 commits into from
May 13, 2024
Merged

Fix leave option does not appear in group chat thread #40529

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
d7eedc7
Fix leave option does not appear in group chat thread
nkdengineer Apr 19, 2024
f2adb93
Merge branch 'main' into fix/39660
nkdengineer Apr 22, 2024
10f3079
create canLeaveReport and canJoinReport util
nkdengineer Apr 22, 2024
1ce4e9f
renam function
nkdengineer Apr 22, 2024
9e2db78
Merge branch 'main' into fix/39660
nkdengineer Apr 23, 2024
212dcf0
Merge branch 'main' into fix/39660
nkdengineer Apr 24, 2024
6f130b3
rename function
nkdengineer Apr 24, 2024
9baf84c
add selfDM check
nkdengineer Apr 24, 2024
ccf3030
add explain comment
nkdengineer Apr 24, 2024
0a7e601
Merge branch 'main' into fix/39660
nkdengineer Apr 25, 2024
7299772
add explain comment for whisper check
nkdengineer Apr 25, 2024
53323da
Merge branch 'main' into fix/39660
nkdengineer Apr 26, 2024
31ed70b
remove unnecessary check
nkdengineer Apr 26, 2024
9b01946
Merge branch 'main' into fix/39660
nkdengineer May 1, 2024
f656e9c
add explain comment for isRootGroupChat function
nkdengineer May 1, 2024
c08fec1
Merge branch 'main' into fix/39660
nkdengineer May 2, 2024
5ed6873
remove canLeaveRoom function
nkdengineer May 2, 2024
403bd4a
fix lint
nkdengineer May 2, 2024
c81ccf3
edit the comment
nkdengineer May 2, 2024
8346da5
add comment
nkdengineer May 2, 2024
8407f6d
merge main
nkdengineer May 4, 2024
535848c
rename function
nkdengineer May 4, 2024
03f51ea
merge main
nkdengineer May 9, 2024
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
45 changes: 45 additions & 0 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5675,6 +5675,10 @@ function isDeprecatedGroupDM(report: OnyxEntry<Report>): boolean {
);
}

function isRootGroupChat(report: OnyxEntry<Report>): boolean {
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
return !isChatThread(report) && (isGroupChat(report) || isDeprecatedGroupDM(report));
}

/**
* Assume any report without a reportID is unusable.
*/
Expand Down Expand Up @@ -6077,6 +6081,44 @@ function canLeavePolicyExpenseChat(report: OnyxEntry<Report>, policy: OnyxEntry<
return isPolicyExpenseChat(report) && !(PolicyUtils.isPolicyAdmin(policy) || PolicyUtils.isPolicyOwner(policy, currentUserAccountID ?? -1) || isReportOwner(report));
}

/**
* Whether the user can join a report
*/
function canJoinChat(report: OnyxEntry<Report>, parentReportAction: OnyxEntry<ReportAction>, policy: OnyxEntry<Policy>): boolean {
// We disabled thread functions for whisper action in this PR https://github.com/Expensify/App/pull/31676
// So we should not show join option for existing thread on whisper message that has already been left, or manually leave it
if (ReportActionsUtils.isWhisperAction(parentReportAction)) {
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

// If the notification preference of the chat is not hidden that means we have already joined the chat
if (report?.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

if (isRootGroupChat(report) || isSelfDM(report)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment here like:

Anyone viewing these chat types is already a participant and therefore cannot join

return false;
}

return isUserCreatedPolicyRoom(report) || isChatReport(report) || canLeaveRoom(report, !isEmptyObject(policy)) || canLeavePolicyExpenseChat(report, policy);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we check for isChatReport here and not isChatThread?

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you explore if canLeaveRoom can be left out here? On the face of it, it seems like it shouldn't be here 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Let's get rid of all the canLeaveWhatever logic and just clearly document all the reasons we can leave or join (even if we have to repeat some stuff). I think that will be much less confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we check for isChatReport here and not isChatThread?

You're right, that is my mistake when I bring the logic from HeaderView into util.

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 agree. Let's get rid of all the canLeaveWhatever logic and just clearly document all the reasons we can leave or join (even if we have to repeat some stuff). I think that will be much less confusing.

In fact, we will only be able to join a chat if we can leave it. If we have left the room, then its notification is hidden, the join option will appear, and vice versa. That is the reason we have the canLeave... condition here.

@jjcoffee @marcaaron What do you think if I update the canJoinChat like this

/**
 * Whether the user can join a report
 * - We can leave the chat
 * - The chat isn't a thread of a whisper action
 * - The user left the chat (the notification of its is hidden)
 */
function canJoinChat(report: OnyxEntry<Report>, parentReportAction: OnyxEntry<ReportAction>, policy: OnyxEntry<Policy>): boolean {
    if (ReportActionsUtils.isWhisperAction(parentReportAction)) {
        return false;
    }

    if (report?.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
        return false;
    }

    return canLeaveChat(report, policy);

}

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest renaming canLeavePolicyExpenseChat to isNonAdminOrOwnerOfPolicyExpenseChat().

I also find this method a bit weird:

App/src/libs/ReportUtils.ts

Lines 5261 to 5278 in 9a727de

function canLeaveRoom(report: OnyxEntry<Report>, isPolicyEmployee: boolean): boolean {
if (!report?.visibility) {
if (
report?.chatType === CONST.REPORT.CHAT_TYPE.POLICY_ADMINS ||
report?.chatType === CONST.REPORT.CHAT_TYPE.POLICY_ANNOUNCE ||
report?.chatType === CONST.REPORT.CHAT_TYPE.POLICY_EXPENSE_CHAT ||
report?.chatType === CONST.REPORT.CHAT_TYPE.DOMAIN_ALL ||
report?.chatType === CONST.REPORT.CHAT_TYPE.SELF_DM ||
!report?.chatType
) {
// DM chats don't have a chatType
return false;
}
} else if (isPublicAnnounceRoom(report) && isPolicyEmployee) {
return false;
}
return true;
}

It has a default of true. But aren't there specific types of reports we can leave? We should check if it's one of them. Instead we are checking if it's NOT one of the rooms we "can't" leave.

I bet once we figure out what those are then the logic is much simpler and we might not even need a canLeaveRoom() method at all.

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 would suggest renaming canLeavePolicyExpenseChat to isNonAdminOrOwnerOfPolicyExpenseChat().

I checked this function and agree isNonAdminOrOwnerOfPolicyExpenseChat is good, it's what we do in this function.

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 bet once we figure out what those are then the logic is much simpler and we might not even need a canLeaveRoom() method at all.

@marcaaron canLeaveRoom function just updated the case for invoice room, I think we can keep this logic as it is now. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcaaron Here's the latest canLeaveRoom for context. I'm starting to wonder if it's better to open a separate issue to tidy up the overall logic here 😅

That being said, the core issue here seems to be that 95% of canLeaveRoom is actually "is the room of a type that we can join or leave" with "is this a room we can leave" tacked on. Unless there are cases where you can leave a room, but not join again? (Unlikely, but maybe I've missed something!)

Does it then make sense to clear things up a bit by creating a separate method, canChangeRoomSubscription or something, and then we just call that in both canLeaveRoom and canJoinRoom (along with notificationPreference checks)? (Though I realise this is almost going back to what we had before with canLeaveOrJoinRoom 😄)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love if we could stick with the current plan and do the tidying here. More bugs and confusion will arise from this if we don't make an effort to improve what we've got.

}

/**
* Whether the user can leave a report
*/
function canLeaveChat(report: OnyxEntry<Report>, policy: OnyxEntry<Policy>): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave chat doesn't make sense for anonymous users since they haven't actually joined.
Issue: #43404

if (isSelfDM(report) || isRootGroupChat(report)) {
return false;
}

return (
(isChatThread(report) && !!report?.notificationPreference?.length) ||
isUserCreatedPolicyRoom(report) ||
canLeaveRoom(report, !isEmptyObject(policy)) ||
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
canLeavePolicyExpenseChat(report, policy)
);
}

function getReportActionActorAccountID(reportAction: OnyxEntry<ReportAction>, iouReport: OnyxEntry<Report> | undefined): number | undefined {
switch (reportAction?.actionName) {
case CONST.REPORT.ACTIONS.TYPE.REPORT_PREVIEW:
Expand Down Expand Up @@ -6202,6 +6244,8 @@ export {
canFlagReportAction,
canLeavePolicyExpenseChat,
canLeaveRoom,
canJoinChat,
canLeaveChat,
canReportBeMentionedWithinPolicy,
canRequestMoney,
canSeeDefaultRoom,
Expand Down Expand Up @@ -6327,6 +6371,7 @@ export {
isDM,
isDefaultRoom,
isDeprecatedGroupDM,
isRootGroupChat,
isExpenseReport,
isExpenseRequest,
isExpensifyOnlyParticipantInReport,
Expand Down
11 changes: 2 additions & 9 deletions src/pages/home/HeaderView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import useWindowDimensions from '@hooks/useWindowDimensions';
import * as HeaderUtils from '@libs/HeaderUtils';
import Navigation from '@libs/Navigation/Navigation';
import * as OptionsListUtils from '@libs/OptionsListUtils';
import * as ReportActionsUtils from '@libs/ReportActionsUtils';
import * as ReportUtils from '@libs/ReportUtils';
import * as Link from '@userActions/Link';
import * as Report from '@userActions/Report';
Expand Down Expand Up @@ -92,13 +91,9 @@ function HeaderView({report, personalDetails, parentReport, parentReportAction,
const parentNavigationSubtitleData = ReportUtils.getParentNavigationSubtitle(reportHeaderData);
const isConcierge = ReportUtils.hasSingleParticipant(report) && participants.includes(CONST.ACCOUNT_ID.CONCIERGE);
const isCanceledTaskReport = ReportUtils.isCanceledTaskReport(report, parentReportAction);
const isWhisperAction = ReportActionsUtils.isWhisperAction(parentReportAction);
const isUserCreatedPolicyRoom = ReportUtils.isUserCreatedPolicyRoom(report);
const isPolicyEmployee = useMemo(() => !isEmptyObject(policy), [policy]);
const canLeaveRoom = ReportUtils.canLeaveRoom(report, isPolicyEmployee);
const reportDescription = ReportUtils.getReportDescriptionText(report);
const policyName = ReportUtils.getPolicyName(report, true);
const canLeavePolicyExpenseChat = ReportUtils.canLeavePolicyExpenseChat(report, policy);
const policyDescription = ReportUtils.getPolicyDescriptionText(policy);
const isPersonalExpenseChat = isPolicyExpenseChat && ReportUtils.isCurrentUserSubmitter(report.reportID);
const shouldShowSubtitle = () => {
Expand Down Expand Up @@ -143,16 +138,14 @@ function HeaderView({report, personalDetails, parentReport, parentReportAction,
Report.updateNotificationPreference(reportID, report.notificationPreference, CONST.REPORT.NOTIFICATION_PREFERENCE.ALWAYS, false, report.parentReportID, report.parentReportActionID),
);

const canJoinOrLeave = !isSelfDM && !isGroupChat && (isChatThread || isUserCreatedPolicyRoom || canLeaveRoom || canLeavePolicyExpenseChat);
const canJoin = canJoinOrLeave && !isWhisperAction && report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
const canLeave = canJoinOrLeave && ((isChatThread && !!report.notificationPreference?.length) || isUserCreatedPolicyRoom || canLeaveRoom || canLeavePolicyExpenseChat);
const canJoin = ReportUtils.canJoinChat(report, parentReportAction, policy);
if (canJoin) {
threeDotMenuItems.push({
icon: Expensicons.ChatBubbles,
text: translate('common.join'),
onSelected: join,
});
} else if (canLeave) {
} else if (ReportUtils.canLeaveChat(report, policy)) {
const isWorkspaceMemberLeavingWorkspaceRoom = !isChatThread && (report.visibility === CONST.REPORT.VISIBILITY.RESTRICTED || isPolicyExpenseChat) && isPolicyEmployee;
threeDotMenuItems.push({
icon: Expensicons.ChatBubbles,
Expand Down
Loading