Page MenuHomePhabricator
Differential D9430

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

Authored by rohan on Oct 9 2023, 9:51 AM.
Tags
None
Referenced Files
F3366766: D9430.id32266.diff
Mon, Nov 25, 12:23 PM
Unknown Object (File)
Sun, Nov 10, 5:00 AM
Unknown Object (File)
Wed, Oct 30, 8:55 AM
Unknown Object (File)
Wed, Oct 30, 8:55 AM
Unknown Object (File)
Wed, Oct 30, 8:55 AM
Unknown Object (File)
Wed, Oct 30, 8:55 AM
Unknown Object (File)
Wed, Oct 30, 8:55 AM
Unknown Object (File)
Wed, Oct 30, 8:55 AM
View All Files
Subscribers
• ted
tomek
will

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
No Lint Coverage
Unit
No Test Coverage

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)
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
ashoat requested changes to this revision.Oct 10 2023, 9:00 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
ashoat added a subscriber: • ted.Oct 10 2023, 9:03 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

rohan updated this revision to Diff 32083.EditedOct 17 2023, 8:38 AM
rohan marked an inline comment as done.

Update useRoleNamesFromCommunityThreadInfo:

  1. We now return a Map of memberID: roleName
  2. 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
  3. 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:

  1. 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
  1. 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
atul resigned from this revision.Oct 17 2023, 9:27 AM

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

ashoat accepted this revision.Oct 18 2023, 1:59 PM
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
rohan added a comment.Oct 19 2023, 12:56 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

rohan updated this revision to Diff 32230.Oct 19 2023, 1:04 PM

Reduce indentation

Harbormaster completed remote builds in B23383: Diff 32230.Oct 19 2023, 1:22 PM
rohan updated this revision to Diff 32266.Oct 20 2023, 8:57 AM

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

rohan updated this revision to Diff 32267.Oct 20 2023, 9:00 AM

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

Revision Contents
Changeset List

PathSize
lib/
utils/
49 lines
native/
chat/
settings/
14 lines
web/
modals/
threads/
members/
8 lines

Diff 32267

View Options

lib/utils/role-utils.js

Loading...
View Options

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

Loading...
View Options

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

Loading...