[23.1] Fix delete collection + elements

[davelopez]

Fixes #16866

Before this fix, deleting a collection will only mark the HDCA as deleted but not the contained elements. There are currently 2 ways of deleting collections from the History Panel:

Using the "Trash" icon in a collection

We used to display (in the old history) a menu when deleting a single collection to choose to also delete its contents, this menu is now back to help mitigate this issue #16866 (comment)

Using the "bulk delete" operation and including collections in the selection

When deleting collections in bulk, we cannot present this choice for the user, we need to decide the default behavior (delete elements too vs only HCDA). Since using the "bulk purge" when there are collections in the selection will also purge its items, for consistency, using the "bulk delete" will now also mark elements as deleted in the same way.

How to test the changes?

• I've included appropriate automated tests.
• Instructions for manual testing are as follows:
  • Follow the instructions in Deleting a collection doesn't delete the elements #16866

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[jmchilton]

I get that this is probably the desired user experience 99% of the time, but I still hate it.

I do not love the API behavior is conditional on the state of the collection and not of the structure of the request. Also this seems... not performant. Would it be better to hit the API to see if any HDAs belong to other collections or are visible at the top level of the history and then gray out delete "collection and datasets" with an explanation if they are. That way you don't have to issue an expensive query for each dataset - you can just issue an aggregate query across all datasets in the collection once (or once per level of the collection). Then the delete datasets part of the API response could be always respected for applications and power users that might want to script complex behaviors but the UI protects the 99%.

I would feel so much better about this - but I admit it is a lot of extra work potentially so I'm going accept this as is if we've like to move on to working on less niche problems and more grounded concerns than mine.

[davelopez]

@jmchilton yeah, I don't love it either, I just tried to implement what was discussed in #16866

I do not love the API behavior is conditional on the state of the collection and not of the structure of the request.

Sorry I don't understand very well this sentence 😓

Would it be better to hit the API to see if any HDAs belong to other collections or are visible at the top level of the history and then gray out delete "collection and datasets" with an explanation if they are. That way you don't have to issue an expensive query for each dataset

I might be confusing things here, but I want to clarify that the menu showing collection and elements doesn't use this inefficient _is_hda_used_in_multiple_collections (that I would like to improve with some help :) ). Using the menu will just call the same API that was called in the old history api/histories/${history_id}/contents/dataset_collections/${id}?recursive=true, so basically passing recursive true or false depending on the option.

The problematic part is when deleting from the "bulk selection" but now that I think of it... maybe I could just try to use the same delete (recursive) from the DatasetCollectionManager... and completely replace this inefficient attempt or trying to delete the elements in this situation is not even worth it and we should just avoid it altogether 🤔

I would feel so much better about this - but I admit it is a lot of extra work potentially so I'm going accept this as is if we've like to move on to working on less niche problems and more grounded concerns than mine.

I'm always willing to put in all the work and effort I can, especially if it helps you feel better about it 😄 I really value all your comments 👍

[davelopez]

Ok, in the context of "bulk deletions", I've completely removed the odd and inefficient logic to check if an element of a collection is orphaned to decide to delete it or not. Now it behaves exactly as we were doing with the "bulk purge", meaning we will mark all elements as deleted too.

I don't know if this way is more performant, but at least now it is consistent with both options and there is no custom logic involved.

The only question that remains is if this is the default behavior that we want for bulk operations or not i.e. affecting all elements of a collection always vs only the HDCA.

I've also updated the PR description to clearly differentiate between the 2 ways of deleting collections from the History Panel.

What do you think? I'm happy to keep tinkering if this is still a less ideal solution.

[jdavcs]

I think this is a simple and clean solution which addresses the referenced issue in straightforward way.

[jmchilton]

This seems more clean than the version I was responding on. I don't see anything questionable here on the backend anymore. Thanks so much.

[simonbray]

Thanks @davelopez for working on this 🎉