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

[17.05] fix image proxy prefix in tool form #5237

Merged
merged 2 commits into from Jan 22, 2018

Conversation

Projects
None yet
4 participants
@martenson
Member

martenson commented Dec 20, 2017

This fixes #4490

The proxy prefix has never been added to the URLs of images in repositories, this adds the prefix dynamically at render time.
Alternatively I think this could be fixed at the point when this image url is being generated on the backend.

Given that most tools do not have images in help, and those who have have usually one or two this shouldn't slow down things.

I also believe this is a more robust fix since it solves precisely the bug reported and can hardly have any side effects.

fix image proxy prefix in tool form
alternatively I think this could be fixed at the point
 when this image url is being generated on backend
@dannon

This comment has been minimized.

Member

dannon commented Dec 20, 2017

Given that most tools do not have images in help, and those who have have usually one or two this shouldn't slow down things.

This runs an extra underscore _.each loop on the results of a jquery-find on the toolform help template regardless of whether there are images or not every time we build a tool form for display, or rerender. All so we can put a bandaid on a backbone bug that is fixed, correctly, in the other PRs in question. If we decide to use tool help elsewhere, we'll have to apply a similar bandaid there, as well. Lastly, when we swap to a correctly functioning router library, we'll have to remember to take this bandaid back out.

The correct fix also has the benefit of always constructing correct URLs, across the board, for uniform behavior everywhere, so we won't see this same class of bug in other places. If we do want that applied to 17.05, I'll backport it, but I think this bandaid is a bad idea.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Dec 20, 2017

Thanks @martenson. I'm more comfortable with this fix than with forking bootstrap. Any chance we can restrict this to just URLs that contain admin_toolshed? External absolute URLs and deployment-local relative URLs should work IMO and look like they might be broken by this? If I'm wrong and they do not work currently - I'm +1 on this as is.

@dannon

This comment has been minimized.

Member

dannon commented Dec 20, 2017

It's not bootstrap, it's backbone, and it's a 2 line tweak to url handling in a dead project that we're phasing out, not a massive fork that we'll need to maintain. Buying into wrangling our own URLs here, instead of fixing them across the board, is a bad idea for the exact types of bugs that @jmchilton has pointed out above.

If you two are going to shove this through even though I spent all the time actually debugging and fixing the problem, please at least use jquery selection result's .each(). There's no need to throw two javascript libraries at this.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Dec 20, 2017

Responding to a comment from private chat:

Nobody yet has explained why a minor patch to the library is bad, or pointed out anything it will harm.

@dannon I generally prefer reuse of existing libraries instead using forked ones - even on JS side. I have a clear record on this I think - I preferred the citations rendered with the previous library but I didn't hold back your changes because I admitted it might be worth it to use the more popular, standard library on balance (#4978 (comment)). To me this situation is pretty analogous.

Update: Spelling fix.

@dannon

This comment has been minimized.

Member

dannon commented Dec 20, 2017

Edit: This is a waste of my time; ya'll do what you're going to do. I'm going to move on and fix more things.

@jmchilton

This comment has been minimized.

Member

jmchilton commented Dec 20, 2017

The citations rendered with the previous library were forked, and I changed that to use unmodified libraries explicitly so we did not have to fork them, could install from npm, and just use them.

Exactly - I thought there was enough value in using unmodified upstream libraries I allowed this change even though I thought the rendered citations were degraded. In weighing things - I agreed with you at the time that it was better to not fork if all else is equal.

@dannon

This comment has been minimized.

Member

dannon commented Dec 20, 2017

@jmchilton I said I'm moving on, do what you want to do. I don't get a vote here, and I'm tired of wasting so much time arguing with you two constantly.

Also, copying messages from an intentionally private chat and posting them in public? Classy.

@martenson martenson added this to the 18.01 milestone Jan 2, 2018

@martenson

This comment has been minimized.

Member

martenson commented Jan 17, 2018

the travis (macOS) fails are unrelated

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 22, 2018

I'm not sure why going with the more performant fix is a bad idea here when both are hacks to some extent, if backbone is really dead and being phased out. If we were keeping it, or it was installed at runtime and not shipped, then hacking it would be a much worse idea. And I think we all agree that hacking external libraries with changes that won't be accepted upstream should not be done without good reason.

I spoke privately with multiple people involved in this and I think there was a general feeling about a lack of public awareness or discussion of future direction, and frustration over the rapid breakdown of discussion. I've disagreed with all of you before on various principles and I think we all want things done properly, with the project's future in mind. So, I hate to see things devolve this way and over a relatively minor technical issue. Cooler heads can prevail.

Concerning this fix, since @dannon has moved on and this is a working solution, I think we should merge this.

@dannon

This comment has been minimized.

Member

dannon commented Jan 22, 2018

Just to be clear, they are both working solutions. We're just apparently split as a team on what the better one is. I still contend my approach is the right one, and that this is a worse approach, but in the interest of moving on as mentioned above I'm just going to merge this now. I'm rewriting it all using a new router anyway.

@dannon dannon merged commit 4288e96 into galaxyproject:release_17.05 Jan 22, 2018

5 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
api test Build finished. 275 tests run, 0 skipped, 0 failed.
Details
framework test Build finished. 148 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 34 tests run, 0 skipped, 0 failed.
Details
selenium test Build finished. No test results found.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment