galaxy icon indicating copy to clipboard operation
galaxy copied to clipboard

[24.0] do not expand datasets that are known to be inaccessible

Open martenson opened this issue 1 year ago • 5 comments

This now works better for hdas.

It does not fix existing bug for the corner case scenario of dataset collection that have some inaccessible element in it. This is due DCAs not having the accessible property and it is unclear to me whether including it would 1) make sense 2) impose a high performance penalty.

I was experimenting with making the inaccessible items unselectable, but there are actual operations like hide or delete that apply to these hdas (and are pretty likely to be invoked) even though you do not have access to its contents. Therefore I left it untouched. Note other operations will fail ungracefully - like collection building or tags.

also closes #16903


update: I've started reimplementing the inaccessible dataset state.

Screenshot 2024-03-27 at 5 33 13 PM

Notes:

  • This is still broken for inaccessible datasets within collections
  • Disable dragging of inaccessible items
  • Investigate how this affects multiselect and our fancy keyboard selection

This is to show one approach how to address this -- I'd like to discuss this wider before going further.

I am not sure how much we should rely on the accessible property here, but if we do it makes it easier to catch, however we could possibly run into caching problems. We could also alter the dataset store so the API call is not directly tied to a computed property (this may be an unwanted vue pattern?) which I hope would prevent the loop, and then catch the 403 and give user the latest information.

I have noticed similar issue also happens to data collections that contain some datasets inaccessible to current user. However in that case the collection is not supposed to expand, but open a different view, and that fails to load completely. So you do not even see the datasets you have access to.

edit: I forgot about it but @davelopez reminded me that ideally the history items would already render as inaccessible (white with padlock).


eventually closes https://github.com/galaxyproject/galaxy/issues/17757

How to test the changes?

(Select all options that apply)

  • [ ] I've included appropriate automated tests.
  • [ ] This is a refactoring of components with existing test coverage.
  • [ ] Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

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

martenson avatar Mar 23 '24 02:03 martenson

Alright, I was too eager to get this in and overlooked your updated comments in the PR description :sweat_smile:

This is still broken for inaccessible datasets within collections

I think I have a potential fix for this. Should I push some commits to this branch to test it out?

davelopez avatar May 23 '24 08:05 davelopez

Alright, I pushed a couple of commits to fix the remaining outstanding issue with inaccessible elements in collections.

Basically, I just serialized the accessible attribute for HDAObjects and make sure we don't even try to request details for inaccessible HDAs in the client. I don't know why this accessible field was never serialized before (maybe for performance reasons?) but it looks to me like it is the right thing to do because the UI state will be incorrect otherwise :thinking:

Before the changes

CollectionInaccessibleBug

After the changes

CollectionInaccessibleBugFixed

Also, I patched the openapi_to_schema.mjs script to handle boolean constants.

davelopez avatar May 24 '24 09:05 davelopez

Investigating test failures... of course, It couldn't be so easy... :laughing:

davelopez avatar May 24 '24 11:05 davelopez

All green now, It wasn't that bad after all :sweat_smile:

davelopez avatar May 24 '24 13:05 davelopez

lgtm, the only issue I still see is that the animation for expanding/collapsing looks clunky for the grey items. I assume that is because it is bound to use the same-length animation, but has less height, so everything looks sluggish. Not a big deal at all, I'd just ignore it for now.

thanks for pushing it over the finish line @davelopez !

martenson avatar May 24 '24 21:05 martenson

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ AttributeError: 'NoneType' object has no attribute 'dataset' /api/dataset_collections/{hdca_id}/contents/{pa... View Issue

Did you find this useful? React with a 👍 or 👎

Nice catch Sentry!

davelopez avatar May 28 '24 07:05 davelopez