Details

Reviewers

ginsu
ashoat
atul

Commits

rCOMMd134c635e839: [lib/web/native] Display community role for users in subchannels/sidebars

Summary

Currently, the members modal will display the correct role for community roots (i.e. 'Members', 'Admins'), but will only display 'Members' for subchannels/sidebars. This is because of the way that ThreadInfo only contains the Members role for these threads.

The solution here is to introspect into the community's threadInfo for that specific thread, and pull the roles from there for each member. This will ensure that we have access to all of the community roles.

I noted in a code comment, but the special case is for GENESIS, where we don't readily have access to the community's members (and also roles are not really supported there), so in that case we'll just read the current ThreadInfo for the role (probably 'Members' for subchannels/sidebars).

Resolves https://linear.app/comm/issue/ENG-5178/ashoat-shows-up-under-admins-column-but-with-members-pill

Test Plan

Please see the test videos below (also tested for custom roles)

Before Web:
Before Native:

After Web:
After Native:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Oct 9 2023, 9:51 AM

Herald added subscribers: will, tomek. · View Herald TranscriptOct 9 2023, 9:51 AM

rohan edited the summary of this revision. (Show Details)Oct 9 2023, 9:57 AM

rohan edited the test plan for this revision. (Show Details)

rohan edited the test plan for this revision. (Show Details)Oct 9 2023, 10:00 AM

Harbormaster completed remote builds in B23111: Diff 31817.Oct 9 2023, 10:09 AM

rohan requested review of this revision.Oct 9 2023, 10:09 AM

It looks like ThreadMembersModalContent is still determining its tabs differently from how the pills are being determined. The fact that this code is different is what got us into this mess, and I'd like to make sure it's unified.

The code there looks at:

memberIsAdmin(member, threadInfo) || memberHasAdminPowers(member)

Another benefit to updating that code will be that we'll have fewer hacky checks... memberIsAdmin checks if the role name is "Admins" (which isn't great) and memberHasAdminPowers checks for CHANGE_ROLE permissions (which is arguably even worse).

(Probably best to handle updating ThreadMembersModalContent in a separate diff... if it doesn't require any further changes to this diff to make that happen, let me know and I'll accept this one.)

lib/utils/role-utils.js
84–95 ↗	(On Diff #31817)	I appreciate you putting a long comment here, but I think it's a little confusing that you don't mention WHY `the roles in the threadInfo will just be "Members"`. Can you try to explain that our in-code permissions system actually has roles for each chat, whereas our user-surfaced permissions system only has roles for each community?
109 ↗	(On Diff #31817)	Usually better to use `??` instead of `&&` in these cases
112 ↗	(On Diff #31817)	Can you move this closer to where it's used?

This revision now requires changes to proceed.Oct 10 2023, 9:00 AM

There is perhaps a design question here around how to handle more than two tabs in ThreadMembersModalContent. Can you create a DES task on Linear and assign it to @ted to take a look? It might also be good to send a message to the Design Team chat in case there's some discussion to be had.

rohan marked 3 inline comments as done.Oct 17 2023, 8:33 AM

rohan added inline comments.

lib/utils/role-utils.js
109 ↗	(On Diff #31817)	I used `&&` since I'm not trying to return `memberInfo.role` (that's just a role ID), it's type is a `?string`. So if `memberInfo.role` is not null, I'd like to introspect into `threadInfo.roles` for that role ID and get it's role name. Let me know if I'm misunderstanding your feedback though

Update useRoleNamesFromCommunityThreadInfo:

We now return a Map of memberID: roleName
For the single member callsites (i.e. getting the role name for the members pills), the Map will contain just one ID mapped to one role name
For the callsite with an array of members (i.e. the Admins section that displays only thread admins), we will have a map of each member's ID mapped to their role name

Tested the following:

Members Pills
- Clicked through to check the pills for non-GENESIS communities (correctly saw Admins Members and custom roles in both community roots and subchannels
- Confirmed that while GENESIS is still unsupportive of roles, the modals do not crash when opening

Admins Filter (this will be the code in the next diff)
- Confirmed that all Admins show up when filtering out the members in both community roots and subchannels

rohan added inline comments.Oct 17 2023, 8:41 AM

lib/utils/role-utils.js
100–116 ↗	(On Diff #32083)	Could reduce indentation

rohan added a child revision: D9513: [web] Call useRoleNamesFromCommunityThreadInfo to help filter Admin members.Oct 17 2023, 8:45 AM

Harbormaster completed remote builds in B23289: Diff 32083.Oct 17 2023, 8:57 AM

Feel free to re-add if there's anything specific I can be helpful w/

In D9430#277058, @ashoat wrote:

There is perhaps a design question here around how to handle more than two tabs in ThreadMembersModalContent. Can you create a DES task on Linear and assign it to @ted to take a look? It might also be good to send a message to the Design Team chat in case there's some discussion to be had.

Did this get done? Please make sure to link the Linear issue before landing!

lib/utils/role-utils.js
100–116 ↗	(On Diff #32083)	+1

This revision is now accepted and ready to land.Oct 18 2023, 1:59 PM

Linear issue here: https://linear.app/comm/issue/DES-158/handling-multiple-tabs-in-the-members-modal-on-web

Also sending a message in the design chat

Reduce indentation

Harbormaster completed remote builds in B23383: Diff 32230.Oct 19 2023, 1:22 PM

Return $ReadOnlyMap<string, ?RoleInfo> instead of $ReadOnlyMap<string, ?string> so I can use roleIsAdminRole in the next diff

Rename hook

This revision was landed with ongoing or failed builds.Oct 20 2023, 9:04 AM

Closed by commit rCOMMd134c635e839: [lib/web/native] Display community role for users in subchannels/sidebars (authored by rohan). · Explain Why

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMMd134c635e839: [lib/web/native] Display community role for users in subchannels/sidebars.

Harbormaster completed remote builds in B23402: Diff 32267.Oct 20 2023, 9:18 AM

[lib/web/native] Display community role for users in subchannels/sidebars
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 32270

lib/utils/role-utils.js

native/chat/settings/thread-settings-member.react.js

web/modals/threads/members/member.react.js

[lib/web/native] Display community role for users in subchannels/sidebarsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 32270

lib/utils/role-utils.js

native/chat/settings/thread-settings-member.react.js

web/modals/threads/members/member.react.js

[lib/web/native] Display community role for users in subchannels/sidebars
ClosedPublic
Actions

Revision Contents
Changeset List