[lib] Update `threadHasAdminRole` to consider `specialField`
ClosedPublic
Actions

Authored by atul on Mar 1 2024, 1:16 PM.

Details

Reviewers

ginsu
tomek
kamil
michal

Commits

rCOMMcd71e1fc65fe: [lib] Update `threadHasAdminRole` to consider `specialField`

Summary

Context: https://linear.app/comm/issue/ENG-6948/update-threadhasadminrole-to-use-specialrole-field

We're first going through to see if any role has specialRoles.ADMIN_ROLE. If one does, we return true. If one doesn't but has ANY specialRole fields, we assume that it is a Client/"current" type and return false.

However, if there is no specialRole field encountered, we will assume we're dealing with a Server/"legacy" type and fallback to sketchy string search.

Depends on D11208

Test Plan

Going to write some unit tests. Also logged return value at callsites and ensured that things continue to work as expected.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Mar 1 2024, 1:16 PM

Herald added a subscriber: ashoat. · View Herald TranscriptMar 1 2024, 1:16 PM

atul published this revision for review.Mar 1 2024, 1:19 PM

atul added inline comments.

lib/shared/thread-utils.js
1117–1128 ↗	(On Diff #37735)	This logic definitely seems a bit sketchy

Harbormaster completed remote builds in B27258: Diff 37735.Mar 1 2024, 1:32 PM

However, if there is no specialRole field encountered, we will assume we're dealing with a Server/"legacy" type and fallback to sketchy string search.

What is the scenario when this can happen?

This revision now requires changes to proceed.Mar 4 2024, 1:07 AM

atul added a child revision: D11248: [lib] Constrain dep list of `useRoleMemberCountsForCommunity` and `useRoleUserSurfacedPermissions`.Mar 5 2024, 2:22 PM

In D11209#324215, @tomek wrote:

However, if there is no specialRole field encountered, we will assume we're dealing with a Server/"legacy" type and fallback to sketchy string search.

What is the scenario when this can happen?

There are three places where threadHasAdminRole is encountered:

lib/shared/thread-utils: getAvailableThreadMemberActions()
lib/selectors/thread-selectors: baseOtherUsersButNoOtherAdmins()
keyserver/src/updaters/thread-updaters: leaveThread()

getAvailableThreadMemberActions is used in

native/chat/settings/thread-settings-member.react.js
web/modals/threads/members/member.react.js

Which are all expected to have the specialRole field in RoleInfos.

baseOtherUsersButNoOtherAdmins is a Redux selector that is only used on the client, where again we expect the specialRole field in RoleInfos.

leaveThread() is part of thread-updaters on the keyserver and calls threadHasAdminRole with LegacyRawThreadInfo | RawThreadInfo returned from rawThreadInfosFromServerThreadInfos. This is the scenario where we need to handle RoleInfos without the specialRole field patched in.

In D11209#324884, @atul wrote:

In D11209#324215, @tomek wrote:

However, if there is no specialRole field encountered, we will assume we're dealing with a Server/"legacy" type and fallback to sketchy string search.

What is the scenario when this can happen?

There are three places where threadHasAdminRole is encountered:

lib/shared/thread-utils: getAvailableThreadMemberActions()

lib/selectors/thread-selectors: baseOtherUsersButNoOtherAdmins()

keyserver/src/updaters/thread-updaters: leaveThread()

getAvailableThreadMemberActions is used in

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

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

Which are all expected to have the specialRole field in RoleInfos.

baseOtherUsersButNoOtherAdmins is a Redux selector that is only used on the client, where again we expect the specialRole field in RoleInfos.

leaveThread() is part of thread-updaters on the keyserver and calls threadHasAdminRole with LegacyRawThreadInfo | RawThreadInfo returned from rawThreadInfosFromServerThreadInfos. This is the scenario where we need to handle RoleInfos without the specialRole field patched in.

Thanks for checking!

If leaveThread is the only place where we need to support the legacy approach, we can consider having the special logic only there. Overall, I'm looking for a solution where we can make this code a bit more maintainable, but not sure if it is worth it (maybe we can have two variants of this function with different types accepted?).

This revision is now accepted and ready to land.Mar 6 2024, 4:18 AM

Thanks for checking!

If leaveThread is the only place where we need to support the legacy approach, we can consider having the special logic only there. Overall, I'm looking for a solution where we can make this code a bit more maintainable, but not sure if it is worth it (maybe we can have two variants of this function with different types accepted?).

Agree that it would make more sense to split implementations, will keep this in mind later in stack.

rebase around and land

Harbormaster completed remote builds in B27606: Diff 38201.Mar 19 2024, 11:22 AM

Closed by commit rCOMMcd71e1fc65fe: [lib] Update `threadHasAdminRole` to consider `specialField` (authored by atul). · Explain WhyMar 19 2024, 11:28 AM

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMMcd71e1fc65fe: [lib] Update `threadHasAdminRole` to consider `specialField`.