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

Fix visualization link in import success message #4952

Merged
merged 2 commits into from Nov 8, 2017

Conversation

Projects
None yet
2 participants
@guerler
Contributor

guerler commented Nov 7, 2017

No description provided.

@guerler guerler added this to the 18.01 milestone Nov 7, 2017

@guerler guerler added status/review and removed status/WIP labels Nov 7, 2017

@guerler guerler changed the title from Fix visualization import link in success message to Fix visualization link in import success message Nov 7, 2017

@guerler

This comment has been minimized.

Contributor

guerler commented Nov 7, 2017

@galaxybot test this

@@ -404,7 +404,7 @@ def imp(self, trans, id):
# Redirect to load galaxy frames.
return trans.show_ok_message(
message="""Visualization "%s" has been imported. <br>You can <a href="%s">start using this visualization</a> or %s."""
% (visualization.title, web.url_for(controller='visualization'), referer_message), use_panels=True)
% (visualization.title, web.url_for(controller='visualizations/list'), referer_message), use_panels=True)

This comment has been minimized.

@dannon

dannon Nov 8, 2017

Member

Since it's not actually a controller endpoint anymore, this should probably just be web.url_for('visualizations/list'), right?

This comment has been minimized.

@guerler

guerler Nov 8, 2017

Contributor

Currently we often refer to the client routes also as controllers. Changing this without resetting the controller would redirect to visualization/visualizations/list right?

This comment has been minimized.

@dannon

dannon Nov 8, 2017

Member

/visualizations/list and not visualizations/list, I should have said, which hopefully has the right behavior?

(and, can you elaborate on what you mean regarding the client routes and controllers? I'm not sure where that came from; wasn't my intent when setting up the first client route redirects)

This comment has been minimized.

@guerler

guerler Nov 8, 2017

Contributor

I changed it for this particular case. I mean that we usually kept the form controller=ROUTE in the backend even when we transitioned them to front end routes. I am in favor of changing this but I think it should be done consistently for all client routes together.

This comment has been minimized.

@dannon

dannon Nov 8, 2017

Member

@guerler Got it, thanks. We should dig into this more, later, and see what the pros and cons are. To me, it doesn't seem like there's a need to use the controller/action form when there aren't actual controllers to represent; just specific routes -- my concern (maybe unfounded, though, we should look) was that Routes is going to make extra assumptions based on the controller argument vs absolute paths.

@dannon dannon merged commit 92c6f36 into galaxyproject:dev Nov 8, 2017

1 of 7 checks passed

api test Test started.
Details
framework test Test started.
Details
integration test Test started.
Details
lgtm analysis: JavaScript Running analyses for revisions
Details
selenium test Test started.
Details
toolshed test Test started.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@guerler

This comment has been minimized.

Contributor

guerler commented Nov 8, 2017

Thanks for the review @dannon

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