-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix leave option does not appear in group chat thread #40529
Conversation
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-app-2024-04-19_11.36.03.mp4Android: mWeb Chromeandroid-chrome-2024-04-19_11.39.44.mp4iOS: Nativeios-app-2024-04-19_11.01.27.mp4iOS: mWeb Safariios-safari-2024-04-19_11.05.12.mp4MacOS: Chrome / Safaridesktop-chrome-2024-04-19_10.24.31.mp4MacOS: Desktopdesktop-app-2024-04-19_10.27.24.mp4 |
src/libs/ReportUtils.ts
Outdated
return false; | ||
} | ||
|
||
return isUserCreatedPolicyRoom(report) || isChatReport(report) || canLeaveRoom(report, !isEmptyObject(policy)) || canLeavePolicyExpenseChat(report, policy); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
😄)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests well! Just a few questions to clarify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar thoughts to @jjcoffee.
src/libs/ReportUtils.ts
Outdated
return false; | ||
} | ||
|
||
return isUserCreatedPolicyRoom(report) || isChatReport(report) || canLeaveRoom(report, !isEmptyObject(policy)) || canLeavePolicyExpenseChat(report, policy); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks seems like we are getting closer. Let's get this one done! 🙇
src/libs/ReportUtils.ts
Outdated
return false; | ||
} | ||
|
||
return isUserCreatedPolicyRoom(report) || isChatReport(report) || canLeaveRoom(report, !isEmptyObject(policy)) || canLeavePolicyExpenseChat(report, policy); |
There was a problem hiding this comment.
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:
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.
@nkdengineer Friendly bump to address the outstanding comments, please! |
src/libs/ReportUtils.ts
Outdated
return false; | ||
} | ||
|
||
if (isRootGroupChat(report) || isSelfDM(report)) { |
There was a problem hiding this comment.
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
src/libs/ReportUtils.ts
Outdated
return false; | ||
} | ||
|
||
return isChatReport(report) || canLeaveRoom(report, !isEmptyObject(policy)) || isNonAdminOrOwnerOfPolicyExpenseChat(report, policy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think we should get rid of canLeaveRoom()
? It's a bit of a mess.
The process I'd take here is to analyze the current code and figure out what conditions would make a chat "joinable" and what conditions would make a chat "leaveable". Actually someone did it here:
So we can go down that list and find what is also true for "join" and then add it to this method instead...
Several of the options say "Nobody can leave". Do we need to consider those cases here? Probably not, because our method is returning true
at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also recommend the structure we are using here for both methods i.e. return early for any disqualifying conditions then return the qualifying conditions at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcaaron After checking canLeaveRoom
function again, I think we can remove this function like this
-
For invoice room case, we can create a separate function
canJoinOrLeaveInvoiceRoom
-
For other cases in this function, because only
POLICY_ROOM
chat type hasreport.visibility
and all other chat types can't leave room as description of this function, we can earlier return false if the report is policyAnnounce room and the user the policy member.
After refactoring, we can remove canLeaveRoom
function and canLeaveChat
function will be like this
function canLeaveChat(report: OnyxEntry<Report>, policy: OnyxEntry<Policy>): boolean {
if (isSelfDM(report) || isRootGroupChat(report)) {
return false;
}
if (isPublicAnnounceRoom(report) && !isEmptyObject(policy)) {
return false;
}
if (canJoinOrLeaveInvoiceRoom(report)) {
return true;
}
return (isChatThread(report) && !!report?.notificationPreference?.length) || isUserCreatedPolicyRoom(report) || isNonAdminOrOwnerOfPolicyExpenseChat(report, policy);
}
cc @jjcoffee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, can you just apply whatever changes you think we should apply and I will review them in the context of the greater PR review?
At first glance... I'm not really loving the canJoinOrLeaveInvoiceRoom()
here. It's not clear to me what case someone can "join" an invoice room, but makes sense that we would not allow someone to leave one.
@nkdengineer Is this ready for re-review/retesting? |
@jjcoffee Waiting for @marcaaron to share the thoughts on this comment #40529 (comment) |
src/libs/ReportUtils.ts
Outdated
/** | ||
* Invoice sender, invoice receiver and auto-invited admins cannot leave | ||
*/ | ||
function canJoinOrLeaveInvoiceRoom(report: OnyxEntry<Report>): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that this has been moved. But confused about why we need to consider it for the "join" case.
Can we actually join an invoice room? What state are things in when we do that? Maybe an example would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. if we are doing this you have a lot of duplicate code happening below that needs to be moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcaaron I checked again and you're right, we don't have the "join" case for invoice room. Updated the function.
src/libs/ReportUtils.ts
Outdated
return false; | ||
} | ||
|
||
return isChatReport(report) || canLeaveRoom(report, !isEmptyObject(policy)) || isNonAdminOrOwnerOfPolicyExpenseChat(report, policy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, can you just apply whatever changes you think we should apply and I will review them in the context of the greater PR review?
At first glance... I'm not really loving the canJoinOrLeaveInvoiceRoom()
here. It's not clear to me what case someone can "join" an invoice room, but makes sense that we would not allow someone to leave one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going OOO, back next Tuesday 14th so I'm provisionally approving for now (it looks like things are roughly on the right track) in case @marcaaron you want to merge whilst I'm away. Happy to re-review and retest when I'm back though!
@jjcoffee Oh, hmm, let's get another C+ to cover for you? There are changes pushed so we should have someone re-testing this. Did you re-test before you approved? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better. Thanks for the changes here.
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.4.74-0 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.4.74-6 🚀
|
/** | ||
* Whether the user can leave a report | ||
*/ | ||
function canLeaveChat(report: OnyxEntry<Report>, policy: OnyxEntry<Policy>): boolean { |
There was a problem hiding this comment.
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
Details
Fix leave option does not appear in group chat thread
Fixed Issues
$ #39660
PROPOSAL: #39660 (comment)
Tests
Offline tests
Same as above
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-04-19.at.11.36.46.mov
Android: mWeb Chrome
Screen.Recording.2024-04-19.at.11.35.06.mov
iOS: Native
Screen.Recording.2024-04-19.at.11.37.25.mov
iOS: mWeb Safari
Screen.Recording.2024-04-19.at.11.32.29.mov
MacOS: Chrome / Safari
Screen.Recording.2024-04-19.at.11.29.17.mov
MacOS: Desktop
Screen.Recording.2024-04-19.at.11.45.12.mov