Arvados

Also, there are currently no multiselect options when more than one user is selected. It might be useful to have the "Remove" button there so that more than one user could be removed at the same time.

22814-group-toolbar @ abf0025cb572424e1b5d286009eb0b7485861281

developer-run-tests-services-workbench2: #1586

• ✅ All agreed upon points are implemented / addressed.
• ✅ Anything not implemented (discovered or discussed during work) has a follow-up story.
• ✅ Code is tested and passing, both automated and manual, what manual testing was done is described
• ✅ The tested code incorporates recent main branch changes.
• n/a New or changed UX/UX and has gotten feedback from stakeholders.
• n/a Documentation has been updated.
• ✅ Behaves appropriately at the intended scale (describe intended scale).
• ✅ Considered backwards and forwards compatibility issues between client and server.
• ✅ Follows our coding standards and GUI style guidelines.

2 bits of feedback:

• In resource-to-menu-kind, it feels a little hacky to return GROUP_MEMBER vs LINK depending solely on the page the user is looking at, it would be nice to have it return a correct result no matter where the user is. Since the link resource is already fetched as resource, is it possible to inspect the head/tail uuid or another attribute of the link object to determine if it's a group membership link? Perhaps if head_uuid is a group/role group type uuid and tail_uuid is a user type uuid - you might be able to use extractUuidKind to check if the UUIDs are ResourceKind.GROUP and ResourceKind.USER
• The logic on renderers.tsx:607 is a little confusing and doesn't read like it lines up with the way the comment was written, can that be reworded to more explicitly match the logic so it's clearer what the reasoning is (behind if(user) then false else !isBuiltIn), ie the comment sounds like it's saying if(user) then (depends on isBuiltIn) but the code reads if(user) always false, otherwise (depends on isBuiltIn).

22814-group-toolbar @ e8c697882c9b685005ca4b52900315eee168ce4b

developer-run-tests-services-workbench2: #1592

• A group can have another group or a project as a member, so checking if the tail is a user won't work.
• A group in the Groups Panel will have a different set of menu items than a group in the Group Details Panel. What I didn't realize at the time is that the groups in the former panel have ResourceKind.GROUP while in the latter, they have ResourceKind.Link (The display name is the same in both places). Once I realized this, the solution became to go back to using the resource to determine the menu simply define a new set of menu items for built-in groups.
• I also created a separate renderer for the delete column in the permissions tab because the tail of those links can be a built-in group. This is necessary because of the backwards way that links permissions are in the group permissions tab (the tail determines the built-in status, not the head).

The comment in the second point became moot in the process.

I have 1 small suggestion: isGroupMember is written like a type guard but doesn't actually narrow types because LinkResource is just an alias of GroupMemberResource. Since we're checking a Resource object which is being cast to LinkResource anyways, we could get rid of the cast and rely more on the type guard by changing it to accept Resource and adding checks for the necessary LinkResource fields:

export const isGroupMember = (resource: Resource): resource is LinkResource => {
    return resource
        && resource.kind === ResourceKind.LINK
        && 'linkClass' in resource
        && 'headKind' in resource
        && resource.linkClass === LinkClass.PERMISSION
        && resource.headKind === ResourceKind.GROUP
};

And it might make sense to rename the method to isGroupMemberLink to make it clearer that it's meant to be passed a link resource.

22814-group-toolbar @ fda5067eb850d5aa5fa6797887c19d46acf7f094

developer-run-tests-services-workbench2: #1596

I reworked it to be an actual type guard, and as such, changed the input type to resource: any. I also differentiated GroupMemberLink from LinkResource so that GroupMemberLink is actually what it says on the tin. Also added the new resourceToMenuKind return value to the unit test.

1 more minor nit - I think this looks much better. I think using resource: Resource as the type guard parameter type would be marginally better since it would warn anyone trying to pass in other resource types. The type guard doesn't really need to be able to narrow any type, and starting with as narrow a type as possible in general makes the code inside the type guard simpler to type check, and would keep it in line with isGroupResource. I believe the only reason we use any for isUserGroup is because it gets passed a resource-shaped type that needs to be checked.

I like the changes to GroupMemberLink, I probably would have been fine with it not technically being a type guard but that does make it clearer about the resulting guarantees.

Aside from this it lgtm!

(After further discussion we landed on using Resource | LinkResource to satisfy type checks and use a narrower type parameter)