Skip to content
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

Migrate collection elements store to Pinia #16725

Merged

Conversation

davelopez
Copy link
Contributor

@davelopez davelopez commented Sep 22, 2023

Migrates the collectionElementsStore from Vuex to Pinia and modernizes the CollectionPanel component. Also tightens the type annotations in the related API schema models.

TODO

  • Enhance some type annotations in the API
  • Use Fetcher to request data
  • Create a new Pinia store for collection elements
  • Cleanup unused code

BONUS

  • Migrate CollectionPanel component to composition API + Typescript
  • Explore ways to improve the scroll experience for large collections (I'll follow with the changes after experimenting in a different PR)

How to test the changes?

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.

License

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

@davelopez davelopez added kind/enhancement area/UI-UX kind/refactoring cleanup or refactoring of existing code, no functional changes labels Sep 22, 2023
@davelopez davelopez changed the title Migrate collection elements store to Pinia Migrate collection elements store to Pinia Sep 22, 2023
@davelopez davelopez force-pushed the migrate_collection_elements_store_pinia branch from 0aede14 to 8470256 Compare September 25, 2023 12:08
@davelopez davelopez marked this pull request as ready for review September 25, 2023 12:08
@github-actions github-actions bot added this to the 23.2 milestone Sep 25, 2023
@davelopez
Copy link
Contributor Author

I think this is ready for review. I will continue with the collection scrolling improvements in a different PR.

@davelopez
Copy link
Contributor Author

Hmm, test failures seem related... Investigating... 🔎

@davelopez davelopez marked this pull request as draft September 25, 2023 14:35
It always returns a detailed view of the collection since there are no serialization parameters in this route to change the view.
The DCObject information does not contain enough information (like `element_count` or `collection_id`) to handle the sub-collection elements navigation downstream.
@davelopez davelopez force-pushed the migrate_collection_elements_store_pinia branch from 92f7a7d to c029991 Compare September 26, 2023 08:48
None is not a valid extension and it seems to return this when you have a collection containing other sub-collections
@davelopez davelopez force-pushed the migrate_collection_elements_store_pinia branch from 25c3c6c to 4b2e2be Compare September 27, 2023 08:33
@davelopez
Copy link
Contributor Author

I think I found out what was causing these Framework test failures after fixing the response type annotation for GET api/dataset_collections/{id} and making it more accurate with HDCADetailed.

The issue was that the validation for elements_datatypes: Set[str] was failing because those tests were returning [None] instead of an empty list or a proper extension name. The quick fix is in 4b2e2be, but I wanted to make sure this was not hiding another problem and it is the correct fix.

@davelopez
Copy link
Contributor Author

I'm not sure what is going on with test_history_multi_view.py::TestHistoryMultiView::test_list_list_display I would say it is related to these changes, but It consistently passes for me locally.

Can someone try to run it and check?

@davelopez davelopez marked this pull request as ready for review September 27, 2023 11:16
@guerler
Copy link
Contributor

guerler commented Sep 27, 2023

Test passes consistently locally. Seems to be unrelated to this PR. PR looks good to me, works fine, is definitely a step into the right direction. Thank you @davelopez.

@guerler guerler merged commit ab0354b into galaxyproject:dev Sep 27, 2023
43 of 44 checks passed
@davelopez davelopez deleted the migrate_collection_elements_store_pinia branch September 27, 2023 15:08
@davelopez
Copy link
Contributor Author

Thank you @guerler for the quick review!

I'm still a little worried that the selenium test test_history_multi_view.py::TestHistoryMultiView::test_list_list_display might be now flaky as it also fails in the follow-up PR #16738 based on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/UI-UX kind/enhancement kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
Development

Successfully merging this pull request may close these issues.

2 participants