Wasn't really sure if I needed to modify this file? cc @atul

getRolePermissionBlobs isn't used here since it has preset permissions for roles in different communities. With custom roles, each role starts off with none of the user-surfaced permissions (all checkboxes client-side are unticked), and the user is free to add/remove as many permissions as they'd like when editing the role as well.

If I called getRolePermissionBlobs when a user edits a role, wouldn't that just force the permissions to however the default Members blob looks like for the community type?

There's not currently, but I can call threadTypeIsCommunityRoot on the thread type when it's fetched possibly

This wasn't just for Members, it was for the creation of custom roles and editing all roles.

Members are already given voiced in getRolePermissionBlobs for COMMUNITY_ROOTs, though that means we need to handle custom roles ourselves:

• For custom roles, how do we confer voiced to them in COMMUNITY_ROOT
• For both members and custom roles in COMMUNITY_ANNOUNCEMENT_ROOT, if the user ticks the user-surfaced option VOICED_IN_ANNOUNCEMENT_CHANNELS client-side, then how do we give voiced to those roles

The code here was hoping to deal with these, so for all roles:

• If the thread is a COMMUNITY_ROOT, push the voiced permission in their permissions blob (especially important for custom roles)
• If the thread is not a COMMUNITY_ANNOUNCEMENT_ROOT, then the previous behavior was that ticking the client-side option to voice a role in an announcement channel corresponded with the voiced thread permission. This was the root cause of ENG-5183 since it didn't extend support to announcement channels

makePermissionsBlob seems good, I'll work on the change!

Though I tried looking at parseThreadPermissionString, and I wonder if it's going to do the right thing here? Feel free to correct me, but it seems to parse permission strings that have propagation / filter prefixes in them (like descendant_open_voiced).

threadPermissions. VOICED_IN_ANNOUNCEMENT_CHANNELS maps to voiced_in_announcement_channels as a thread permission string, so I'm not sure if parseThreadPermissionString would help here?

The general feedback is that we shouldn't have two sources of truth. You're making a fair argument against using getRolePermissionBlobs here, but it still doesn't explain why we need to use two completely separate methods for determining permissions blobs – one for role creation, and another for role modification.

Have you considered deprecating getRolePermissionBlobs in favor of programmatically computing those blobs based on a set of default userSurfacedPermissions?

Yes please. This should've been done from the start...

My intuition is that this hacky workaround should not be here. We should find a way to handle this without polluting business logic like this.

If the thread is a COMMUNITY_ROOT, push the voiced permission in their permissions blob (especially important for custom roles)

getRolePermissionBlobs has the ability to vary the set of permissions based on ThreadType. It sounds like we need the same thing for your user-surfaced permissions code. If your user-surfaced permissions code had access to the community root ThreadType, it could use a different set of universalCommunityPermissions depending on whether it is an announcement root or not.

I don't feel strongly about using parseThreadPermissionString. I'm open to using any solution that goes through makePermissionsBlob.

Feel free to correct me, but it seems to parse permission strings that have propagation / filter prefixes in them

Yeah, that's what it currently does. You could of course modify it to do more, but I again I don't feel strongly.

Typing this here for flow, otherwise I get the following:

Could not decide which case to select, since  case 1 [1] may work but if it doesn't  case 2 [2] looks promising too.

where case 1 and case 2 are

function permissionLookup(
  permissions: ?ThreadPermissionsBlob | ?ThreadPermissionsInfo, // here
  permission: ThreadPermission,
): boolean {