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.

Sign up for GitHub

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

Jump to bottom

Vueifiy History Grids #17219

Merged
merged 50 commits into from
Jan 15, 2024
Merged

Vueifiy History Grids #17219

merged 50 commits into from
Jan 15, 2024

Conversation

guerler
Copy link
Contributor

@guerler guerler commented Dec 20, 2023 •
edited
Loading

Vuefies the Histories and Shared Histories grids, adds required API endpoints and a manager helpers. These are the last two grids requiring the mako/backbone based grid rendering mechanism.

image

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

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

@guerler guerler added kind/enhancement area/UI-UX kind/refactoring cleanup or refactoring of existing code, no functional changes labels Dec 20, 2023
@guerler guerler force-pushed the grids_history branch 4 times, most recently from 60ac019 to eac8c12 Compare December 20, 2023 04:30
@jmchilton
Copy link
Member

Looks awesome so far!

@guerler guerler force-pushed the grids_history branch 5 times, most recently from 702f2e1 to ca8f9ac Compare December 28, 2023 05:33
@guerler guerler marked this pull request as ready for review December 28, 2023 10:40
@github-actions github-actions bot added this to the 24.0 milestone Dec 28, 2023
@guerler guerler requested a review from jmchilton January 3, 2024 15:38
@itisAliRH itisAliRH mentioned this pull request Jan 8, 2024
Closed
itisAliRH
itisAliRH approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@itisAliRH itisAliRH left a comment

Choose a reason for hiding this comment

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

The UI part looks good to me. Thanks :)

mvdbeek
mvdbeek requested changes Jan 12, 2024
View reviewed changes
Copy link
Member

@mvdbeek mvdbeek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is a step back in functionality. I used to be able to select a bunch of histories and do operations on them (e.g. purge all deleted). This is critical functionality.
Screenshot 2024-01-12 at 15 57 48

@mvdbeek
Copy link
Member

mvdbeek commented Jan 12, 2024

Another thing I've noticed is that the count columns don't line up:
Screenshot 2024-01-12 at 15 59 43
vs
Screenshot 2024-01-12 at 16 00 26

@guerler guerler mentioned this pull request Jan 13, 2024
Merged
4 tasks
@bgruening bgruening mentioned this pull request Jan 14, 2024
Open
@bgruening
Copy link
Member

Given that @guerler is working on the bulk changes in a different PR I think this can go in.

I have added my review comments into this issue: #17283

@mvdbeek I have also added your comment to the follow-up issue.

@mvdbeek mvdbeek merged commit ff173ed into galaxyproject:dev Jan 15, 2024
55 checks passed
@mvdbeek
Copy link
Member

mvdbeek commented Jan 15, 2024 •
edited
Loading

I appreciate that the review is being addressed, but disagree with putting this into a different PR. I can get on board with this practice iff additional bells and whistles are requested, but this is basic functionality that got removed. Now we'll have to do another round of reviewing and hope that #17282 lands.

These partial PRs make it harder to do frequent, timely releases and potentially rolling out updates to main, I hope that'll be a consideration going forward.

@guerler guerler deleted the grids_history branch January 15, 2024 12:55
@guerler
Copy link
Contributor Author

guerler commented Jan 15, 2024 •
edited
Loading

Thanks for the review and comments @mvdbeek. I generally agree, however it does make it easier to review and test features separately and in distinct chunks. Nevertheless this should only be the exception.

mvdbeek
mvdbeek reviewed Jan 15, 2024
View reviewed changes
lib/galaxy/webapps/galaxy/api/histories.py
@@ -131,6 +169,37 @@ def index(
trans, serialization_params, filter_query_params, deleted_only=deleted, all_histories=all
)

@router.get(
"/api/histories/query",
Copy link
Member

Choose a reason for hiding this comment

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

Ehm, that should've just been /api/histories ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do that, but that would require changing/deprecating an existing endpoint.

Copy link
Member

Choose a reason for hiding this comment

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

How so ? You only need to add the additional filters, this is really not worth a separate route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvdbeek mvdbeek mvdbeek requested changes

@itisAliRH itisAliRH itisAliRH approved these changes

@jmchilton jmchilton Awaiting requested review from jmchilton

Assignees
No one assigned
Labels
area/UI-UX kind/bug kind/enhancement kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Milestone
24.0
Development

Successfully merging this pull request may close these issues.
5 participants
@guerler @jmchilton @mvdbeek @bgruening @itisAliRH