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

New Workflow Management Screen UI using JavaScript #4047

Merged
merged 33 commits into from May 11, 2017

Conversation

Projects
None yet
9 participants
@anuprulez
Copy link
Member

commented May 9, 2017

Introduction:

The Pull Request presents a new user interface for workflow's home screen built using JavaScript and thereby retires its mako file (list.mako). It displays list of user's workflows in the center panel of the Galaxy as opposed to previous display in full page.

Visuals:

List of workflows

workflow_1

Search

workflow_2

Details:

The workflow's table merges the shared and user's own workflows and can be identified by text in the 'owner' column ('You' for user's own workflows and an email id for the shared workflows). Moreover, as an enhancement, it includes a client side search which filters the list of workflows using query typed in the search textbox (query needs at least 3 characters to search).

Routing:

It uses client side routing in the same way as the user preferences and custom builds screens.

Credits:

Thanks a lot to @bgruening and @guerler for suggestions and reviews!

Your suggestions are welcome!

@galaxybot galaxybot added the triage label May 9, 2017

@galaxybot galaxybot added this to the 17.09 milestone May 9, 2017

@nsoranzo
Copy link
Member

left a comment

Another mako gone, thanks a lot @anuprulez!

@@ -65,6 +65,8 @@ def index(self, trans, **kwd):
encoded_id = trans.security.encode_id(wf.id)
item['url'] = url_for('workflow', id=encoded_id)
item['owner'] = wf.user.username
item['latest_workflow_steps'] = len( wf.latest_workflow.steps )
item['published'] = wf.published

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo May 9, 2017

Member

This line is not necessary, 'published' field is already filled by the to_dict method.

@@ -74,6 +76,11 @@ def index(self, trans, **kwd):
encoded_id = trans.security.encode_id(wf_sa.stored_workflow.id)
item['url'] = url_for( 'workflow', id=encoded_id )
item['owner'] = wf_sa.stored_workflow.user.username
item['latest_workflow_steps'] = len( wf_sa.stored_workflow.latest_workflow.steps )
item['user_email'] = wf_sa.stored_workflow.user.email

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo May 9, 2017

Member

I think this line should be removed, it's not necessary to expose the user email through the API, the owner should be enough for what you are doing.

item['user_email'] = wf_sa.stored_workflow.user.email
item['user_name'] = wf_sa.stored_workflow.user.username
item['slug'] = wf_sa.stored_workflow.slug
item['published'] = wf_sa.stored_workflow.published

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo May 9, 2017

Member

As above, this is not necessary, 'published' field is already filled by the to_dict method.

wf_obj.text = wf.name;
wf_obj.workflow_steps = wf.latest_workflow_steps;
wf_obj.published = ( wf.published ? "Yes" : "No" );
wf_obj.email = wf.user_email ? wf.user_email : "You";

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo May 9, 2017

Member

Remove

wf_obj.published = ( wf.published ? "Yes" : "No" );
wf_obj.email = wf.user_email ? wf.user_email : "You";
wf_obj.slug = wf.slug ? wf.slug : "";
wf_obj.username = wf.user_name ? wf.user_name : "";

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo May 9, 2017

Member
wf_obj.username = wf.owner === username ? "You" : wf.owner;
@@ -74,6 +76,11 @@ def index(self, trans, **kwd):
encoded_id = trans.security.encode_id(wf_sa.stored_workflow.id)
item['url'] = url_for( 'workflow', id=encoded_id )
item['owner'] = wf_sa.stored_workflow.user.username
item['latest_workflow_steps'] = len( wf_sa.stored_workflow.latest_workflow.steps )
item['user_email'] = wf_sa.stored_workflow.user.email
item['user_name'] = wf_sa.stored_workflow.user.username

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo May 9, 2017

Member

That's the same as the owner, remove.

self._templateActions( wf ) +
'</div>' +
'</td>' +
'<td>' + _.escape( wf.email ) +'</td>' +

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo May 9, 2017

Member

s/email/username/

@bgruening

This comment has been minimized.

Copy link
Member

commented May 9, 2017

@nsoranzo @erasche if you have time can you test this. The UI changes a little bit and we would like to have some more feedback on this side as well! And thanks for the review Nicola!

@erasche

This comment has been minimized.

Copy link
Member

commented May 9, 2017

yep, will do! This looks amazing

@guerler

This comment has been minimized.

Copy link
Contributor

commented May 11, 2017

@anuprulez Yeah, I think we should remove it.

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

@erasche thanks a lot :)

Using those configuration, i could have "prefix" in the url and tested for all the actions like edit, run etc. There were couple of issues with the url again while redirecting the page from server. I have fixed those. Now all actions work fine (with and without prefix in the url).

The visuals you shared come from another mako file (import or upload workflow) and form (create workflow) from server which we have not worked upon in this PR. In future, we plan to transform those mako files as well.

Thanks a lot for your review and help!

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

@guerler I removed "list_for_run.mako" file and its corresponding method in the workflow controller.

@bgruening

This comment has been minimized.

Copy link
Member

commented May 11, 2017

It's green! thanks all for your very nice comments!
Anyone wants to merge this! More to come soon :)

@jmchilton

This comment has been minimized.

Copy link
Member

commented May 11, 2017

Really fantastic contribution and nice work! I've opened a PR with Selenium test fixes for these changes (bgruening#34).

Merge pull request #34 from jmchilton/workflowexport_mako_test_fixes
Fix Selenium test cases for JavaScript-izing workflow management.
else {
$el_workflow.append( self._templateNoWorkflow() );
}
});

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo May 11, 2017

Member

Indentation of lines 18-40 seems wrong.

This comment has been minimized.

Copy link
@dannon

dannon May 11, 2017

Member

No, this is correct, unless I misread your concern. The following is a single statement, not the opening of a block:

var self = this,
    min_query_length = 3;

Personally I think it's less clear than explicit var declarations, but multiple assignment like this is fine.

This comment has been minimized.

Copy link
@anuprulez

anuprulez May 11, 2017

Author Member

The method render starts at column 9
$.getJSON starts at column 13 (line 18) and ends at column 13 (line 40).
Inside $.getJSON , lines start at column 17 and inside if-else lines start at column 21

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo May 11, 2017

Member

Oh how I hate this language! Sorry for the noise!

@@ -11,6 +11,7 @@ var jQuery = require( 'jquery' ),
UserPreferences = require( 'mvc/user/user-preferences' );
CustomBuilds = require( 'mvc/user/user-custom-builds' );
Tours = require( 'mvc/tours' );
Workflows = require( 'mvc/workflow/workflow' );

This comment has been minimized.

Copy link
@dannon

dannon May 11, 2017

Member

This is made global due to the trailing semicolons above. Fixed in bgruening#35.

define( [ 'mvc/form/form-view',
'mvc/ui/ui-misc',
'mvc/ui/ui-select',
'mvc/tool/tool-form-composite' ], function( Form, Ui, Select, ToolForm ) {

This comment has been minimized.

Copy link
@dannon

dannon May 11, 2017

Member

It doesn't look like we're actually using any of these here, or am I missing it?

This comment has been minimized.

Copy link
@anuprulez

anuprulez May 11, 2017

Author Member

I am looking into it. Thanks!

This comment has been minimized.

Copy link
@dannon

dannon May 11, 2017

Member

Sure! Pending that, I'm happy to merge this. Don't worry about rebuilding the client again if there are no additional changes, I can resolve that in the merge.

This comment has been minimized.

Copy link
@anuprulez

anuprulez May 11, 2017

Author Member

Actually I rebuilt the client after removing the imports from the header of workflow.js to verify they are not needed and now I have all those static files modified. Shall I commit all these static files along with conflicting files?

This branch has conflicts that must be resolved

Conflicting files
static/scripts/bundled/libs.bundled.js
static/scripts/bundled/libs.bundled.js.map

This comment has been minimized.

Copy link
@dannon

dannon May 11, 2017

Member

That'd be fine, sure.

This comment has been minimized.

Copy link
@anuprulez

anuprulez May 11, 2017

Author Member

I pushed the changes. The build is running :)

This comment has been minimized.

Copy link
@dannon

dannon May 11, 2017

Member

Ahh, k, I see. That last merge with dev wasn't necessary, but I can wait on the build and re-merge after if you want.

This comment has been minimized.

Copy link
@anuprulez

anuprulez May 11, 2017

Author Member

Ok, sure. Thank you!

bgruening and others added some commits May 11, 2017

Merge pull request #35 from dannon/workflowexport_mako
Fix require inclusion scoping in analysis.js
@dannon

This comment has been minimized.

Copy link
Member

commented May 11, 2017

@galaxybot test this

@dannon

This comment has been minimized.

Copy link
Member

commented May 11, 2017

(probably should have asked Travis to test this before, sorry!)

@dannon dannon merged commit 2f6d959 into galaxyproject:dev May 11, 2017

5 checks passed

api test Build finished. 276 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
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
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@dannon

This comment has been minimized.

Copy link
Member

commented May 11, 2017

Thanks @anuprulez, nice work!

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

Thanks a lot everyone for comments and help!

jmchilton added a commit to jmchilton/galaxy that referenced this pull request May 20, 2017

More workflow test case fixes for galaxyproject#4047.
I think previously this button would be there on page load and now it is loaded after the page in JS.

dannon added a commit that referenced this pull request May 20, 2017

Merge pull request #4093 from jmchilton/selenium_fixes_14
More workflow test case fixes for #4047.

mvdbeek added a commit to mvdbeek/galaxy that referenced this pull request May 22, 2017

More workflow test case fixes for galaxyproject#4047.
I think previously this button would be there on page load and now it is loaded after the page in JS.

mvdbeek added a commit to mvdbeek/galaxy that referenced this pull request May 29, 2017

More workflow test case fixes for galaxyproject#4047.
I think previously this button would be there on page load and now it is loaded after the page in JS.
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.