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

Remove histories grid mako #4167

Merged
merged 21 commits into from Jun 20, 2017

Conversation

Projects
None yet
2 participants
@guerler
Copy link
Contributor

commented Jun 7, 2017

Replaces the history grid mako with js. Resolves the way the copy operation was implemented previously. Instead of searching and re-assigning the final dom-elements with new click handlers, the dialog handling is added to the options dictionary. Requires #4157 and #4163.

guerler added some commits Jun 7, 2017

@guerler guerler added status/review and removed status/WIP labels Jun 13, 2017

@@ -83,6 +84,7 @@ window.app = function app( options, bootstrapped ){
'(/)user(/)(:form_id)' : 'show_user_form',
'(/)workflow(/)' : 'show_workflows',
'(/)pages(/)(:action_id)' : 'show_pages',
'(/)histories(/)(:action_id)' : 'show_histories',

This comment has been minimized.

Copy link
@dannon

dannon Jun 13, 2017

Member

Similar to the comment here #4163 (comment), this routes /history/*/ to /history/list/. This isn't good practice, invalid URLs should return error codes, instead of routing to something that wasn't requested.

This comment has been minimized.

Copy link
@guerler

guerler Jun 13, 2017

Author Contributor

Sorry, should have commented earlier. This is a preparation for published_lists. However you are right that in the earlier PR for datasets it is not necessary since there is no published datasets grid. I will remove it from #4163. Is this fine with you?

This comment has been minimized.

Copy link
@dannon

dannon Jun 14, 2017

Member

Edits to comments don't show up in notifications ;)

If it's just two routes, histories/list and histories/published_lists, both should be defined in buildapp.py as explicit routes instead of catchall actions on a base route. This is probably the easiest way to ensure they get the correct HTTP response.

This comment has been minimized.

Copy link
@guerler

guerler Jun 14, 2017

Author Contributor

Ok.

@guerler guerler force-pushed the guerler:grid_histories branch from ef997df to c2ebb57 Jun 14, 2017

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2017

I removed the wildcard routes and replaced them with explicit routes.

@dannon

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

Thanks @guerler

@dannon dannon merged commit 6a0401e into galaxyproject:dev Jun 20, 2017

5 checks passed

api test Build finished. 279 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 150 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 34 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2017

Cool. Thank you for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.