-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[24.0] do not expand datasets that are known to be inaccessible #17818
[24.0] do not expand datasets that are known to be inaccessible #17818
Conversation
I assume the union_mode does not trickle down... type accessible as literal False
that does not fetch dataset details or allow other interactions minor adjustments
dffedef
to
ac7dbb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Sorry, I forgot this was still pending!
Alright, I was too eager to get this in and overlooked your updated comments in the PR description 😅
I think I have a potential fix for this. Should I push some commits to this branch to test it out? |
Fixes typescript error: Argument of type '"edit"' is not assignable to parameter of type '"toggleHighlights"'
ba84945
to
810b246
Compare
Alright, I pushed a couple of commits to fix the remaining outstanding issue with inaccessible elements in collections. Basically, I just serialized the Before the changesAfter the changesAlso, I patched the |
Investigating test failures... of course, It couldn't be so easy... 😆 |
It is probably enough to serialize the `accessible` property only when requesting collection contents displayed in the UI.
All green now, It wasn't that bad after all 😅 |
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 ! |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Fixes galaxyproject#17818 (comment): ``` AttributeError 'NoneType' object has no attribute 'dataset' ```
Fixes #17818 (comment): ``` AttributeError 'NoneType' object has no attribute 'dataset' ```
Nice catch Sentry! |
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.
Notes:
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 #17757
How to test the changes?
(Select all options that apply)
License