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

Refactor library folder contents api #11686

Merged

Conversation

davelopez
Copy link
Contributor

What did you do?

  • Refactor the folder_contents controller logic to simplify it a bit.
  • Add more tests for the access permission when including deleted items.
  • Move the legacy controller logic to a FolderContentsAPIView class that will be shared with the future FastAPI controller.

Additional details

This time, instead of creating a corresponding FolderContentsManager in lib/galaxy/managers/folder_contents.py, I have tried a new approach creating the analogous FolderContentsAPIView class beside the legacy controller to keep the UsesLibraryMixinItems until it can be removed. I didn't want to remove it yet since is used across various places and will require much more changes.
Anyway, I'm still struggling a bit with how to layout managers, controllers, views, etc. so any advice or recommendations are really appreciated :)

Why did you make this change?

This is part of the efforts to migrate the existing API to FastAPI in relatively small steps.

How to test the changes?

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

@github-actions github-actions bot added this to the 21.05 milestone Mar 19, 2021
@davelopez davelopez added area/libraries Data libraries kind/refactoring cleanup or refactoring of existing code, no functional changes area/testing area/testing/api and removed area/testing area/testing/api labels Mar 19, 2021
@davelopez davelopez force-pushed the refactor_library_folder_contents_api branch from 038954b to 509dc23 Compare March 19, 2021 15:27
Copy link
Member

@jmchilton jmchilton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love all of this but I'd like to see the testing constraints tightened a little as commented in line. Thanks so much!

@davelopez davelopez force-pushed the refactor_library_folder_contents_api branch from 509dc23 to 38f64bd Compare March 22, 2021 18:43
@jmchilton jmchilton merged commit 8b5553c into galaxyproject:dev Mar 23, 2021
@davelopez davelopez deleted the refactor_library_folder_contents_api branch March 23, 2021 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API area/libraries Data libraries area/testing/api area/testing kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants