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

Hide add to group button for notification and chronos account #47770

Merged
merged 3 commits into from
Aug 30, 2024
Merged

Hide add to group button for notification and chronos account #47770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
00b5c06
hide add to group button for notification and chronos account
bernhardoj Aug 21, 2024
8689ad3
Merge branch 'main' into fix/43841-dont-allow-adding-notifications-to…
bernhardoj Aug 23, 2024
140dcff
lint
bernhardoj Aug 23, 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
4 changes: 4 additions & 0 deletions src/CONST.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2484,6 +2484,10 @@ const CONST = {
get RESTRICTED_ACCOUNT_IDS() {
return [this.ACCOUNT_ID.NOTIFICATIONS];
},
// Account IDs that can't be added as a group member
get NON_ADDABLE_ACCOUNT_IDS() {
return [this.ACCOUNT_ID.NOTIFICATIONS, this.ACCOUNT_ID.CHRONOS];
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on this comment, we don't want the add to group button to show for Chronos too, so I created a new list here.

But, I see that on the group invite list page, we exclude all expensify accounts, including Concierge, so searching for those accounts won't show up in the invitation search list.

App/src/pages/InviteReportParticipantsPage.tsx

Lines 52 to 53 in 685bb4c

const excludedUsers = useMemo(
() => [...PersonalDetailsUtils.getLoginsByAccountIDs(ReportUtils.getParticipantsAccountIDsForDisplay(report, false, true)), ...CONST.EXPENSIFY_EMAILS],

Should we hide the add to group button for all expensify accounts?

@iwiznia @trjExpensify @abdulrahuman5196

Copy link
Contributor

Choose a reason for hiding this comment

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

Historically, people have been able to have a groupDM with Concierge for support. It's the same as emailing [email protected] with additional email addresses. That said, now we have transitioned to "Groups" where membership isn't fixed on creation (meaning, everyone else in the group can leave freely), does that cause problems and someone can end up with two Concierge DMs if that happens? 🤔 CC: @marcaaron @JmillsExpensify

Copy link

Choose a reason for hiding this comment

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

We ended up covering this case in the Group Chats design doc. The intended design spec should work like this.

A goal of this doc is to completely deprecate Group DMs in favor of Group Chats. In doing that, the behavior of chatting with Concierge via email will change a bit. When a user emails Concierge and CC’s another user we will create a new Group Chat instead of trying to locate an existing chat between the participants. The consequence is that some historical chat messages between these participants can exist across several different reports vs. just one. This is a case we can potentially optimize for in the future, but decided to completely deprecate the Group DM pattern for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, so in product you should be able to Add to group Concierge still? If everyone else is removed/leaves, it'll still be a "Group" technically and not be confused with the Concierge DM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If everyone else is removed/leaves, it'll still be a "Group" technically and not be confused with the Concierge DM?

Removing all group members won't convert it to a DM. Leaving the group chat will remove the report.

Based on the design spec, then we should be able to add a concierge to the group. We can update the non-addable to group account list later if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah basically Group Chats are not defined by the number or makeup of their members. You can have a Group Chat with yourself as the only user. You can have a Group Chat with you and Concierge. You can remove Concierge from that chat later if you want. The 1:1 DM with Concierge will always be a 1:1 DM with Concierge though and you should not have more than 1 of those. Hope that makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks for the confirmation!

@abdulrahuman5196 we can start the review here then


// Auth limit is 60k for the column but we store edits and other metadata along the html so let's use a lower limit to accommodate for it.
MAX_COMMENT_LENGTH: 10000,
Expand Down
2 changes: 1 addition & 1 deletion src/pages/NewChatPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ function NewChatPage({isGroupChat}: NewChatPageProps) {

const itemRightSideComponent = useCallback(
(item: ListItem & OptionsListUtils.Option, isFocused?: boolean) => {
if (item.isSelfDM) {
if (!!item.isSelfDM || (item.accountID && CONST.NON_ADDABLE_ACCOUNT_IDS.includes(item.accountID))) {
return null;
}
/**
Expand Down
Loading