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

Augment form module separation #4438

Merged
merged 18 commits into from Aug 18, 2017

Conversation

Projects
None yet
3 participants
@guerler
Copy link
Contributor

commented Aug 16, 2017

This PR improves the modularization of tool form variations by delegating model updates out of the tool form base class into the individual variants. One advantage is that the workflow editor form can now reload a tool model of different version (through its own api) without having to create a new module view through the workflow editor manager. This helps to improve readability and code separation. It also streamlines the deferred request handling. ping @mvdbeek can you help me to test this? Thanks a lot for your contributions and help.

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Aug 17, 2017

Is this ready for review @guerler ? Happy to test

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2017

Yes please. Thanks a lot.

@guerler guerler added status/review and removed status/WIP labels Aug 17, 2017

guerler added some commits Aug 17, 2017

@mvdbeek
Copy link
Member

left a comment

I have gone through this a few times and I think I understand all the changes. Great way to learn. It does fix the duplication of labels. Thanks a lot @guerler.

self._buildModel( process, { id : id, version : version } );
};
});
self.model.set( 'id', options.id.replace( options.version, this.version ) );

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 18, 2017

Member

Unfortunately I think this could break for tools that carry "version-like" numbers in their id, like if Bowtie2 version 2 was updated to version 3 (very hypothetical of course). The API is smart enough to ignore the version in the tool id if an explicit version is requested.

It seems that the id gets updated in another place anyway.

I think this get updated here https://github.com/guerler/galaxy/blob/5a913700221c3734364d70546abfa1b9342d8c1b/client/galaxy/scripts/mvc/tool/tool-form.js#L36 in the tool form or here https://github.com/guerler/galaxy/blob/5a913700221c3734364d70546abfa1b9342d8c1b/client/galaxy/scripts/mvc/workflow/workflow-forms.js#L62 in the workflow editor form.

This comment has been minimized.

Copy link
@guerler

guerler Aug 18, 2017

Author Contributor

Yeah, I am not a fan of having to replace the id in the string either. Although this PR does not change that. We introduced it because the tool version is encoded in the tool id / url of tool shed tools. You can see it in the top line of the replaced part.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 18, 2017

Member

Ah, didn't realize that was already here. In any case this isn't necessary anymore with #4119, would be good if we can remove that.

@@ -111,7 +112,7 @@ function( Utils, Portlet, Ui, FormSection, FormData ) {
var new_check = self.data.checksum();
if ( new_check != current_check ) {
current_check = new_check;
self.model.get( 'onchange' ) && self.model.get( 'onchange' )();
self.model.get( 'onchange' )();

This comment has been minimized.

Copy link
@dannon

dannon Aug 18, 2017

Member

Much cleaner, I like this. I feel bad about writing it the other way now ;)

},
error : function( response, status ) {
var error_message = ( response && response.err_msg ) || 'Uncaught error.';
if ( status == 401 ) {

This comment has been minimized.

Copy link
@dannon

dannon Aug 18, 2017

Member

This is not new code, and it's not a problem with this PR, but is this something we can handle elsewhere (in a single place) down the road so we don't have to augment all these requests with login redirects?

@dannon dannon merged commit 7f50a8a into galaxyproject:dev Aug 18, 2017

5 of 6 checks passed

api test Build finished. 281 tests run, 0 skipped, 1 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 153 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 44 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details

dannon added a commit to dannon/galaxy that referenced this pull request Aug 18, 2017

Client rebuild, @guerler and I were trying to figure out what changed…
… in the recently merged galaxyproject#4438 but don't see anything obvious.
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.