Arvados

If we're going to need to support bulk operations from the client side, maybe there'll be a need to create some UI element to hint the user the progress of those operations.

15768-multi-select-operations @ commit: bdcaea37455b72c345c7305e6a7bc4ba63d5c1b7

developer-tests-workbench2: #1348

• All agreed upon points are implemented / addressed.
  - done
• 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
  - manual testing included all described operations done repeatedly on all resource types
• Documentation has been updated.
  - N/A
• Behaves appropriately at the intended scale (describe intended scale).
  - testing was done with nearly a hundred items, and while slowdown was noted, wb2 remained responsive
• Considered backwards and forwards compatibility issues between client and server.
  - all communications between client and server use existing functionality and asynchronous effects are properly handled
• Follows our coding standards and GUI style guidelines.
  -done

Here are some things I noticed:

• It would be good to add confirmation dialogs to delete for collections and projects, similar to the process delete context menu
• The hover ripple effect on the toolbar items is squished instead of circular, you could probably move some of the margin into padding (to allow the ripple effect to take up more space since it can use padding space inside each button but can't use space in margin outside the button) could help fix that. You could also potentially get creative with negative margins if necessary
• It seems there's some extraneous arguments to waitForDom on line 1014 of the collection spec
• The verifyIOParameterImage is commented out of the process spec, that check should be passing to verify that inline image previews work - I'm guessing this is because of the arvados change from localhost to 127.0.0.1 - there is a fix for that in main that switches the integration tests to also load from 127.0.0.1 to fix cors issues so merging main into your branch should let you uncomment that again
• In the search spec, the federated item copy to clipboard and open in new tab should use the federated WB2 url instead of the local WB2 instance url - this should be working in main so possibly there was some cause of breakage that was fixed in main, merging main into your branch might also fix this too
• In data table multiselect popover, there's some commented out code in submit - I think we typically try to remove commented out stuff unless there's a reason it needs to stick around. Also, does the submit function serve a purpose since it seems to be empty otherwise?
• I noticed backend 422 errors are being suppressed globally in the index.tsx error handler - I think it would be better to catch the error in your action with a try/catch so that 422 errors elsewhere in the application can still show a toast. I think the rationale behind that is so that errors that are handled or are displayed in a user friendly way can choose to hide the nasty raw error while unexpected errors in code that doesn't have error handling will still show the toast by default
• I've been thinking about the strategy of converting all context menu actions to handle resource arrays even though all the existing actions are designed for single items - it seems weird to foreach the array of resources for something like the share or edit options which if given multiple resources would try to open the same dialog multiple times. I think the main issue is it gives the appearance that it can handle multiple items. I don't want to create a bunch of work, but assuming you didn't make those actions handle multiple items, I think at least indexing the first item for actions and scrapping the loop for existing context menu items would make it clearer that those actions only work for single items. I think a nicer implementation would be to have a separate execute function or a separate type for context menu multiple item actions where the execute function has a resource array type signature, but that could be a lot more work so I don't think it's necessary (a better solution could be done when actual multiple item support is added to regular context menu items)
• I'm curious about the change to the navigation handler to accept the any type and whether it's being used somewhere with an indexable type - I think it's nice to have the string type constraint vs any, and if there's somewhere that passes in an array maybe that side could be changed to index the uuid out of the array so that navigateTo can keep its typed param, wdyt?

It would be good to add confirmation dialogs to delete for collections and projects, similar to the process delete context menu

• Behavior on this branch is the same as on main, so I think adding confirmation dialogs should go on another ticket

The hover ripple effect on the toolbar items...

• fixed

It seems there's some extraneous arguments...

• fixed

The verifyIOParameterImage is commented out of the process spec...

• switched it back, hopefully the fixes on main will address

In the search spec, the federated item copy to clipboard...

• restored to previous

In data table multiselect popover...

• Removed entirely. Meant to do it before, but missed it in my cleanup

I noticed backend 422 errors are being suppressed globally...

• restored global 422 handling and implemented the intended handling changes closer to where they are thrown

I've been thinking about the strategy...

• You're absolutely right and those changes have been made.

I'm curious about the change to the navigation handler...

• This was actually changed so that I could test something and I forgot to change it back :)

Currently waiting for CI changes to complete to bounce back for review

Just a few more cleanup tasks but it lgtm once that's done

• There's a duplicate renames a file to a different directory cypress test in collection.spec.js that probably just got duplicated during merge resolution
• There's a stray console.log in renderers.tsx L524
• It looks like the ContextMenuResource type has an unused isMulti field that I think can be removed
• For removing the for loops for actions that are for single items, these places still have single item open-dialog actions that are applied in a loop and probably should also be changed to indexing off the first element:
  • api-client-authorization-action-set
  • group-action-set, group-member-action-set
  • keep-service-action-set
  • link-action-set
  • repository-action-set
  • root-project-action-set
  • ssh-key-action-set
  • user-action-set
  • virtual-machine-action-set
  • and the Api Details action in search-results-action-set / trashed-collection-action-set

There's a duplicate renames a file to a different directory cypress test in collection.spec.js that probably just got duplicated during merge resolution

• removed

There's a stray console.log in renderers.tsx L524

• removed

It looks like the ContextMenuResource type has an unused isMulti field that I think can be removed

• That is used by the toggleProjectTrashed function to determine whether or not to navigate to the restored item UUID after restoration. Originally it was supposed to do more, but this is its only function now.

edit: renamed isSingle to avoid confusion with isMulti

For removing the for loops...

• removed all