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

Merging configure menu with main workflow view #4353

Merged
merged 10 commits into from Aug 2, 2017

Conversation

Projects
None yet
5 participants
@anuprulez
Copy link
Member

commented Jul 28, 2017

This PR merges the configure workflow menu view with the main workflow view (workflow management screen).

newconfigworkflow

When a workflow is checked in the "Show in Tools panel", it gets listed in the Tools panel and the home page refreshes.

Thanks @guerler for suggestings!

@galaxybot galaxybot added the triage label Jul 28, 2017

@galaxybot galaxybot added this to the 17.09 milestone Jul 28, 2017

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Jul 28, 2017

The intent is awesome! Could you perhaps also remove the 'Create New Workflow' page ? Maybe the user can directly enter a workflow name after clicking on the '+'. Also, Workflow in the masthead does not become white anymore, it's always Analyze Data ... could we address that?

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Jul 28, 2017

@galaxybot test this

@@ -52,6 +53,30 @@ define( [ 'utils/utils' ], function( Utils ) {
});
},

// Save the workflow as an item in Tool panel
register_show_tool_menu: function( self ) {

This comment has been minimized.

Copy link
@guerler

guerler Jul 28, 2017

Contributor

Parsing 'self' is not necessary here or at other locations within this module. You may just use 'this' directly within the method.

@@ -52,6 +53,30 @@ define( [ 'utils/utils' ], function( Utils ) {
});
},

// Save the workflow as an item in Tool panel
register_show_tool_menu: function( self ) {
var $el_checkboxes = self.$el.find( '.show-in-tool-panel' );

This comment has been minimized.

Copy link
@guerler

guerler Jul 28, 2017

Contributor

Here and at similar locations you can just write this.$( '.show-in-tool-panel' ). el.find is not necessary.

'</tr></thead>';
_.each( workflows, function( wf ) {
var checkbox_html = wf.show_in_tool_panel ? '<input type="checkbox" class="show-in-tool-panel" value="'+ wf.wf_id +'" checked="'+ wf.show_in_tool_panel +'">' : '<input type="checkbox" class="show-in-tool-panel" value="'+ wf.wf_id +'"';

This comment has been minimized.

Copy link
@guerler

guerler Jul 29, 2017

Contributor

Can you remove the redundancy in this line? I t hink it is also missing a > the end for the second case.

@guerler

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2017

This is great. It makes this feature more apparent and easier to use. Thanks for implementing it.

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2017

@mvdbeek Thanks a lot for your suggestions. Can we take these changes to a separate PR?

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Jul 30, 2017

Sure, no problem.

item['owner'] = wf.user.username
item['number_of_steps'] = len( wf.latest_workflow.steps )
item['show_in_tool_panel'] = False
item['wf_id'] = wf.id

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Jul 30, 2017

Member

This seems a little strange to me (but I'm not an expert, so feel free to dismiss this), you are adding both the encoded (item['id']) and unencoded workflow id (item['wf_id']).

This comment has been minimized.

Copy link
@guerler

guerler Jul 31, 2017

Contributor

I agree. The unencoded id should not be returned through the api.

This comment has been minimized.

Copy link
@anuprulez

anuprulez Aug 1, 2017

Author Member

@mvdbeek @guerler Thanks for the review!
I removed the unencoded workflow id from the api and also removed get_workflow_menu api call which fetched workflow ids present in the tools panel. Now, there is a flag in the workflows set itself (boolean - show_in_tool_panel) which determines whether any workflow is to be shown in the tools panel. Now, we just use encoded workflow id.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 1, 2017

Member

I don't think this is a good change, the separation between GET and PUT was fine and we should not remove things from the API. I'd say whatever consumed unencoded workflow ids should be changed to work with encoded workflow ids, which is get_workflow_menu, right?

This comment has been minimized.

Copy link
@anuprulez

anuprulez Aug 1, 2017

Author Member

get_workflow_menu api is nowhere used and it used to display the unencoded workflow id. But the workflows which are present in the tools menu already have a boolean flag show_in_tool_panel in the list of workflows itself. But, PUT api is used to save the list using the encoded workflow ids. That's why I removed this Get API. There is already a separate api to list workflows. It was not required anymore.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 1, 2017

Member

How would you know this is unused ? That we don't use it in the UI doesn't mean it's not used. public APIs should not just disappear like that, without announcement or deprecation. If it is unused, you can leave a comment in the code that it is deprecated and that another endpoint should be used.

This comment has been minimized.

Copy link
@anuprulez

anuprulez Aug 1, 2017

Author Member

I will restore the API as I am not completely sure what are all places it is used.

But what we should do about the exposed unencoded workflow ids in the api? Shall we comment them or just remove these lines:

ids_in_menu = [ x.stored_workflow_id for x in user.stored_workflow_menu_entries ]
'ids_in_menu': ids_in_menu

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 1, 2017

Member

So we're not actively using this endpoint ? Then I'd leave it as it is.

m = model.StoredWorkflowMenuEntry()
m.stored_workflow = q.get( id )
user.stored_workflow_menu_entries.append( m )
for wf_id in workflow_ids:

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 1, 2017

Member

I'd probably just decode all workflow ids using trans.security.decode_id as early as you can (before sess = trans.sa_session). The rest of the code can stay the same then.

@bgruening

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

@mvdbeek thanks for your review. Is this ready to go?

@@ -82,6 +82,10 @@ def set_workflow_menu( self, trans, **kwd ):
workflow_ids = []
elif type( workflow_ids ) != list:
workflow_ids = [ workflow_ids ]
workflow_ids_decoded = []

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 2, 2017

Member

I'd prefer the compacter workflow_ids = [trans.security.decode_id(wf_id) for wf_id in workflow_ids], but that's not a must. I do prefer not to use the plural (ids) in the below for loop though.

@mvdbeek

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

The python part looks OK, @guerler is the rest OK for you now ?

$.ajax({
type: 'PUT',
url: Galaxy.root + 'api/workflows/menu/',
data: JSON.stringify( { 'workflow_ids': ids } ),

This comment has been minimized.

Copy link
@guerler

guerler Aug 2, 2017

Contributor

Is stringify necessary here?

This comment has been minimized.

Copy link
@anuprulez

anuprulez Aug 2, 2017

Author Member

It gives bad request without it (JSON.stringify)

@guerler

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2017

It looks good. I think we can make additions with follow up PRs if necessary.

@guerler guerler merged commit 14360d6 into galaxyproject:dev Aug 2, 2017

5 checks passed

api test Build finished. 279 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 150 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 37 tests run, 0 skipped, 0 failed.
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
You can’t perform that action at this time.