Arvados

Currently, there are several sets of actions and filters for them (menuKinds), with a complicated series of if statements to determine which action set to use in each situation. The factors involved in determining which action are the user, the selected resource(s), the permissions the user has on the selected resource(s), and the cluster config.

I suggest that we employ Peter's suggestion instead, making one large set of actions which can be selected from as needed. We could re-use most of the existing menuKinds in principle, but in practice each menuKind would be a set of action names, which could be used to select from (NOT filter) the set of all actions.

I suggest, in place of the complicated switch statement full of ifs and ternaries, we define an object representing the the current values of each piece of state that goes to determine which menuKind gets selected. This object could then be given to a modified switch statement that would simply see if the object matches certain criteria and return the proper menuKind. this would be, at the very least, a vast readability improvement.

Also, it's probably a good idea to revisit the Documentation for accuracy and build the new model based on that:

• https://doc.arvados.org/v2.7/api/permission-model.html
• Groups_Projects_Ownership_and_Permissions_Specification

For example, in the permission model, no mention is made of the Admin level of permission, which is basically the can_manage level plus the ability to add a resource to the Public Favorites

1. removing the duplicated resourceUuidTo*MenuKind code from MultiselectToollbar and from context-menu-actions in favor of a single, simplified version
2. aligning the menu action sets

22051-integrate-resource-to-menu-kind @ 6ea731ad35c3e554ee7318979b9f9b81665db5fe

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

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

• The context menu component test had a couple of mistakes around readonly/canmanage situations, fixed
• The getResourceWithEditable function was removed, along with the associated test. The functionality of that test is still covered in the renamed resource-to-menukind test.

• I think it would be better to keep the types of resources more distinct, and rather than use MenuKindResource as kind of a super type that you can put any resource in and be able to access any member you're interested in, we would use getResource<Resource> and then use type guards where the switch case is to check and safely convert Resource to WhateverResource before using it. As it is currently, MenuKindResource is kind of acting like a global cast which makes it less obvious everywhere the resource is accessed that the members might not be present.
• Unfortunately, switch cases don't seem to support type narrowing in TS, so you would need to change it to an if/else chain

So overall it would look like:

const resource = getResource<Resource>(uuid)(resources);
if (isGroupResource(resource)) {
  // resource is now GroupResource allowing access to resource.groupClass
} else if...

And the type guards would look like: (it would be good to keep all the type guards in their respective model definition files, so in models/group.ts and models/collection.ts respectively)

export const isGroupResource = (resource: Resource): resource is GroupResource => {
    return resource.kind === ResourceKind.GROUP;
};
export const isCollectionResource = (resource: Resource): resource is CollectionResource => {
    return resource.kind === ResourceKind.COLLECTION;
};

• Only the Project and Collection cases really need the type guard/narrowing since the rest of the cases aren't accessing resource, so it's fine if it stays as a generic Resource, so just doing resource.kind === ResourceKind.PROCESS for Process/user/link/workflow is fine.

There would need to be a couple more tweaks to make this work:

• getIsEditable could accept Resource if it used the isGroupResource type guard like so:
  

      const isEditable = (resources[resource.ownerUuid] as GroupResource)?.canWrite
                          || (isGroupResource(resource) && resource.canWrite);
  

• resourceIsFrozen could be locked down a little more to accept Resource if you use another type guard like so:
  

  export const isProjectResource = (resource: Resource): resource is ProjectResource => {
      return isGroupResource(resource) && 'frozenByUuid' in resource;
  };
  
  let isFrozen: boolean = isProjectResource(resource) ? !!resource.frozenByUuid : false;
  

• Finally, since canManage and canWrite are only accessed in the GroupResource case, you can avoid type issues by moving accessing that to within the GroupResource block where resource has already been type narrowed to a GroupResource.
• Also, since there is already an isGroupResource in MultiselectToolbar.tsx which checks for "Role" groups (user groups), it would help clarify if that one was renamed to isRoleGroupResource or something similar.

22051-integrate-resource-to-menu-kind @ 05a386a5bc9fe16b73b47340fcec0122f6adcc59

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

• I think it would be better to keep the types of resources more distinct...

Done, MenukindResource is removed

• Unfortunately, switch cases don't seem to support...

Created type guards in the appropriate files, changed the switch case to use type guards for projects and collections, left it intact for other ResourceKinds

• getIsEditable could accept Resource if it used the isGroupResource type guard like so:

Done

• resourceIsFrozen could be locked down a little more to accept Resource...

Done

• Finally, since canManage and canWrite...

Done

• Also, since there is already an isGroupResource...

Done

• There was some misalignment in the Groups panel menus, now fixed.
• I had to add a Resource type guard so that resourceIsFrozen() in DetailsPanel would work
• I also added a filter for a malformed link to link-update-name.ts. This change will be part of #22473, but it is necessary for me to use here because I have a malformed link in the DB associated with me.

One final comment: I think isRoleGroupResource can just use isUserGroup to reduce code duplication.

Aside from that LGTM!

22051-integrate-menu-actions @ 51fdfe56278f35e8b2ac66ab0f8f54efb5736b18

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

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

• One item that might be appropriate for this ticket is the fact that context menu actions have two option fields (icon and component) that could be combined into one required field that is either type. I don't know if it's worth the effort or not, especially considering this would require rewrites of every action and both menu components.

Just 1 nit, in trash-action.tsx we should use matchTrashRoute, and in other places we get the pathname from state.router.location.pathname, so using that instead of window.location might be preferable - for example: matchCollectionRoute(location ? location.pathname : "")

Aside from that it lgtm!