Can we add a comment explaining the reasoning for this "block" of code?

That's fair, I only considered the client-side of changing roles where it's one at a time at the moment (haven't checked the new role creation flow) - a better approach would to check 1) the number of admin roles in the change request and 2) the number of current admins in the community. Then we can throw an error if all admins are losing their role

If we take this approach, we'll need another task (or two) to make sure that it's not possible to rename a role to "Admins", and that it's not possible to rename the "Admins" role.

The first could probably be addressed in the role creation stack when I work on that by adding a unique key on the role names (we'd probably want this anyways so we don't have several roles with the same name with different permissions).

The second I believe is already covered as an edge case in the designs.

I wonder if we could add an isAdmin parameter to RoleInfo, similar to how we have isDefault today. This would require changes to the MySQL database: probably a new admin_role column on threads, similar to how we have the default_role column now.

I'm open to looking into this, just wanted to gauge if it would be better to handle now or towards the end of roles since I already plan to prevent duplicate role names and prevent the admin role from being renamed

I'm open to looking into this, just wanted to gauge if it would be better to handle now or towards the end of roles since I already plan to prevent duplicate role names and prevent the admin role from being renamed

Not sure, that's a good question...

I'd like to hear from @atul RE this:

I vaguely recall chatting with @atul recently about some other reason that it would be good to have this parameter, but I don't recall the exact details (I think it was related to permissions?)

If that's not an urgent thing we want to address, then I'm open to sequencing this later. But it does feel to me that determining whether a role is an admin role based on the name is a bit "sketchy"...

This could probably be simplified into something like the below, but I preferred the loop to keep readability

const changedAdminsCount = memberIDs.filter(
  memberID =>
    threadInfo.members.find(member => member.id === memberID)?.role ===
    adminRoleID,
).length;

We could by throwing a invalid_parameters error here, in the event that a threadID not associated to a thread is for some reason passed in

adminRoleID can be falsey if there is no admin role

That's fair

memberRole can be falsey if a new user is being added

I feel like that makes sense. If a new user is added and they don't have a role, that just leaves an undefined value in memberRoles. When we calculate changedAdminsCount and iterate through memberRoles, this instance of undefined === adminRoleID would just be false and it makes sense since they aren't an admin losing their role.

Am I missing something there?

I'm not objecting to them being undefined – I'm pointing out that if both of these things happen, changedAdminsCount can be artificially inflated by undefined === undefined

Ah I misunderstood your original comment. Makes sense! The best solution I think would be just to wrap this all in an if (adminRoleID) { ... }