Arvados

I tested this, and while the "delete wokflow" icon now shows up in the toolbar when multiple workflows are selected, when you click on the delete icon it only deletes the first workflow. So it appears that the actual delete callback also needs to handle multi-select.

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

Changed delete method to handle multiple resources.

I am seeing a related issue on 2.7.2 where if I multiselect heterogeneous resources including workflow definitions, then trash them, it trashes the non-workflow resources but not the workflows. Does this branch resolve that issue too? If not I can file a separate ticket.

Reviewing fd7d23d

• Just curious: why is it necessary to deconstruct the array and reconstruct it on line 29?: for (const resource of [...resources]) { ... }
• Also, I was curious as to why we need to pass the owner UUID to the workflow delete call, and digging a bit, I'm seeing that for some reason, navigateTo(ownerUuid) is called before deleting the object. I know this is unrelated to this branch, but do you happen to know why this is necessary?
• Brett's comment above was one of the topics we talked about yesterday. Do you think it would be a good opportunity to change the delete icon to something different (maybe a big X icon?) so that it's less confusing for the user?
• Related to the above, I realize that there's a third case of "trash icon is not exactly the same action": for processes, the tooltip says "Remove", and if I select a workflow definition and a process, I don't get the trash icon, so I'm guessing the code is treating them as different actions.
  • Upon further investigation, it seems that for some reason when we delete processes, we show a confirmation dialog first, but we don't do the same for workflow definitions, they get deleted immediately. I think we need to have consistency on this, if this happens to be an overlooked issue. We either ask for confirmation on destructive operations or we don't, and doing so would allow us to treat "delete workflow" and "remove process" actions as the same thing, enabling the Delete icon on multi-selections. WDYT?
• If the above comment doesn't make sense, I think we'll need a different icon for "Remove process" too.

Forgot to comment it would be nice to have this fix tested. Haven't checked if we already have tests for the multi-select feature, if not I think it's really important to have them as the feature is likely to be used a lot by heavy users.

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

21535-multi-wf-delete @ 9078027722e70c82d972a41e416620c9d8428b8b

• Just curious: why is it necessary to deconstruct...

The idea is to create a copy of the array because some multiselect options can mutate the array, causing unknown bugs from side effects. It's not strictly necessary on every multiselect operation, but I left it in as a best practices practice.

• Also, I was curious as to why we need to pass the owner UUID...

It's necessary when deleting a workflow while in the workflow-view. If the redirect didn't happen, you would wind up seeing an "item not found" screen and then navigating from there. It is not needed in multiselect operations, so I made that parameter optional, the redirect conditional, and removed it where it's not appropriate.

• Brett's comment above was one of the topics we talked about yesterday...
• Related to the above, I realize that there's a third case of "trash icon...

I changed the icon for removing a process or workflow permanently with this icon: https://pictogrammers.com/library/mdi/icon/delete-forever/ . The two icons (this and "trash") will never appear beside each other, which is why the distinction was never considered too important, but the fact that confusion has occurred means that it actually is important.

• Upon further investigation, it seems that for some reason when we delete processes, we show a confirmation dialog...

Added a confirmation dialog for workflow deletion.

it would be nice to have this fix tested

Added testing for deleting multiple workflows, and created ticket #21776 to prevent future devs from dealing with the same problems I had writing these tests.