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

Allow switching tool versions in workflow editor #4373

Merged
merged 15 commits into from Aug 15, 2017

Conversation

Projects
None yet
7 participants
@mvdbeek
Copy link
Member

commented Aug 3, 2017

This will finally close #557 !

@mvdbeek mvdbeek added this to the 17.09 milestone Aug 3, 2017

@mvdbeek mvdbeek changed the title [WIP] Allow switching versions in workflows [WIP] Allow switching tool versions in workflows Aug 3, 2017

@mvdbeek mvdbeek changed the title [WIP] Allow switching tool versions in workflows [WIP] Allow switching tool versions in workflow editor Aug 3, 2017

@mvdbeek mvdbeek changed the title [WIP] Allow switching tool versions in workflow editor Allow switching tool versions in workflow editor Aug 3, 2017

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2017

This works nicely: If the input or output connections change, the connection is removed and the user can re-connect.

@nsoranzo nsoranzo requested a review from guerler Aug 3, 2017

@gregvonkuster

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2017

Awesome! Thanks @mvdbeek ;)

@jmchilton

This comment has been minimized.

Copy link
Member

commented Aug 3, 2017

@mvdbeek You are a wizard who packs so much awesome in such tiny commits.

@mvdbeek mvdbeek changed the title Allow switching tool versions in workflow editor [WIP] Allow switching tool versions in workflow editor Aug 3, 2017

@mvdbeek mvdbeek added status/WIP and removed status/review labels Aug 3, 2017

@bgruening

This comment has been minimized.

Copy link
Member

commented Aug 3, 2017

So cool! 🍻 for Marius!

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Aug 3, 2017

@mvdbeek There's a conflict in static/ and qunit tests are failing, can you take a look?

@mvdbeek mvdbeek force-pushed the mvdbeek:switch_versions_in_workflows branch 2 times, most recently from 7f52f13 to 27bce82 Aug 3, 2017

@@ -37,7 +37,7 @@ define( [ 'utils/utils', 'mvc/form/form-view', 'mvc/tool/tool-form-base' ], func
text_disable : 'Set at Runtime',
narrow : true,
initial_errors : true,
sustain_version : true,
sustain_version : false,

This comment has been minimized.

Copy link
@guerler

guerler Aug 6, 2017

Contributor

This line can be removed. The default is false.

@@ -185,6 +186,12 @@ define(['mvc/workflow/workflow-view-node'], function( NodeView ) {
var nodeView = node.nodeView;
this.tool_state = data.tool_state;
this.config_form = data.config_form;
if (this.config_form) {
this.tool_version = this.config_form.version;

This comment has been minimized.

Copy link
@guerler

guerler Aug 6, 2017

Contributor

Can be a one-liner: this.tool_version = this.config_form && this.config_form.version;.

@@ -156,7 +156,14 @@ define( [ 'utils/utils', 'utils/deferred', 'mvc/ui/ui-misc', 'mvc/form/form-view
// queue model request
self.deferred.reset();
self.deferred.execute( function( process ) {
self._buildModel( process, { id : id, version : version } )
if ( options.hasOwnProperty( "workflow" ) ) {

This comment has been minimized.

Copy link
@guerler

guerler Aug 6, 2017

Contributor

This should be moved out of the base class. I can help with this tho. Is this PR already functional?

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 6, 2017

Author Member

Not entirely, I'm still struggling with getting the connections right when the number and type of output changes.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 7, 2017

Author Member

@guerler I think this is fully functional now, but I'm sure there are some optimizations to do. Feel free to push to this branch if you want.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Aug 11, 2017

Member

@guerler Do you object to me merging as is so you can follow up with that change - or do you want this change made first?

This comment has been minimized.

Copy link
@guerler

guerler Aug 15, 2017

Contributor

Thanks @mvdbeek. Yeah I can make those changes later.

@mvdbeek mvdbeek force-pushed the mvdbeek:switch_versions_in_workflows branch 2 times, most recently from e3340cc to f1842da Aug 7, 2017

@mvdbeek mvdbeek added status/review and removed status/WIP labels Aug 7, 2017

@mvdbeek mvdbeek changed the title [WIP] Allow switching tool versions in workflow editor Allow switching tool versions in workflow editor Aug 7, 2017

this.tool_state = data.tool_state;
this.config_form = data.config_form;
this.force_refesh = this.config_form && this.tool_version !== this.config_form.version;

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 7, 2017

Member

s/force_refesh/force_refresh/ ? Also below.

this.tool_state = data.tool_state;
this.errors = data.errors;
this.tooltip = data.tooltip ? data.tooltip : "";
this.annotation = data.annotation;
// this.output_terminals = {}; // removes outputs when switching tool versions

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 7, 2017

Member

Leftover?

@mvdbeek mvdbeek force-pushed the mvdbeek:switch_versions_in_workflows branch from f1842da to 14c7c98 Aug 7, 2017

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2017

@martenson

This comment has been minimized.

Copy link
Member

commented Aug 11, 2017

So in my testing I have FastQC Galaxy version 0.67 and 0.68 and switching in between them looks fine, however the default selected when adding to the WF canvas is 0.67, shouldn't it be the latest instead?

@martenson

This comment has been minimized.

Copy link
Member

commented Aug 11, 2017

This implementation seems to select whatever revision was installed the last as the default.

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2017

@martenson

This comment has been minimized.

Copy link
Member

commented Aug 12, 2017

@mvdbeek when selecting fastqc in standard analysis interface, I get the latest as default

mvdbeek added a commit to mvdbeek/galaxy that referenced this pull request Aug 13, 2017

Don't cast tool_version to string if tool_version is None-type
If a version is not specifically requested a wrong version (`None` cast to
`'None'`) of a tool will be loaded in the workflow editor instead of the
latest version.

That should address @martenson's remark in
galaxyproject#4373 (comment)
(and maybe also
galaxyproject#557 (comment)).

@mvdbeek mvdbeek force-pushed the mvdbeek:switch_versions_in_workflows branch from d3a3fa5 to 73f4141 Aug 13, 2017

@martenson martenson merged commit 27be249 into galaxyproject:dev Aug 15, 2017

0 of 5 checks passed

api test Test started.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
framework test Test started.
Details
integration test Test started.
Details
toolshed test Test started.
Details
@martenson

This comment has been minimized.

Copy link
Member

commented Aug 15, 2017

Thanks a lot @mvdbeek !

@mvdbeek mvdbeek deleted the mvdbeek:switch_versions_in_workflows branch Sep 3, 2017

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.