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 Configure Workflow Menu UI #4092

Merged
merged 16 commits into from May 30, 2017

Conversation

Projects
None yet
6 participants
@anuprulez
Copy link
Member

commented May 20, 2017

Introduction:

This Pull Request replaces "configure_menu.mako" by JavaScript. It is another step in the line of the #4047 to remove mako files for Workflows.

User Interface:

configure_menu

Details:

It merges both the types of workflows (user's own and shared) into one table which differ by the text in the "owner" column. Again, the workflows are shown in the center panel. After saving the preferences, user is redirected to workflow home screen. As an enhancement, the workflow's table has two buttons (top right) for checking and unchecking all workflows before saving.

Credits:

Thanks a lot @bgruening

All suggestions are welcome. Thank you!

@bgruening

This comment has been minimized.

Copy link
Member

commented May 20, 2017

Thanks a lot @anuprulez!

@galaxybot galaxybot added the triage label May 20, 2017

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

"""
payload = kwd.get( 'payload' )
user = trans.get_user()
if trans.request.method == "PUT":

This comment has been minimized.

Copy link
@guerler

guerler May 22, 2017

Contributor

Methods should have their own function. I would also suggest to name the endpoint just workflow_menu.

@martenson

This comment has been minimized.

Copy link
Member

commented May 22, 2017

@galaxybot test this

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented May 22, 2017

@guerler Is it ok now to be merged? thanks :)

@guerler

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

Minor but I think the endpoint should be 'workflow/menu' where 'PUT' saves settings and a 'GET' returns the settings. So the url should always be /api/workflows/menu. The functions can be named set_/get_ just like for the user endpoints. Sorry if I was not clear before.

@guerler

This comment has been minimized.

Copy link
Contributor

commented May 23, 2017

@anuprulez please checkout the comment above. Everything else looks very nice to me. Pretty cool. Thanks a lot for your contributions.

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented May 23, 2017

@guerler Yes, I have made the required changes. Thanks a lot for the review :)

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented May 23, 2017

@guerler does it look ok now? thanks :)

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented May 24, 2017

@guerler Thank you for the changes and sorry for not understanding the changes in the way you wished.

One doubt I have may be for future reference. How is it beneficial to have one api url (/api/workflows/menu) for two api methods/calls (get and set here) instead of having one api url and one method/call (like the way I did before)? Is it required for api tests during build or do users directly access the api through the url?

Thanks!

@guerler

This comment has been minimized.

Copy link
Contributor

commented May 25, 2017

I think /api/workflows/menu is fine. Its somewhat restful. GET requests data and PUT saves it to the entity which is a menu.

@bgruening

This comment has been minimized.

Copy link
Member

commented May 30, 2017

@guerler can we merge this?

@dannon

This comment has been minimized.

Copy link
Member

commented May 30, 2017

Just catching up here, and it's not a major change really (nor required, just throwing the idea out there), but to me this might make more sense under /api/user/preferences or the like. (That is, it seems it's more a user preference than an attribute of workflows at its core -- it's a purely application display artifact)

@guerler

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

I agree.

@bgruening

This comment has been minimized.

Copy link
Member

commented May 30, 2017

@dannon @guerler should this be changed before the merge? Or can we change this later. @anuprulez has already more to come.

@guerler

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

I think it can be changed in a follow-up if @dannon is ok with that.

@dannon

This comment has been minimized.

Copy link
Member

commented May 30, 2017

Generally speaking I think we should probably nail down the endpoints first, but I'm fine with it being a followup in this case -- we just need to have the API endpoint in its final form before the next release.

May have just run into a problem with security on this one though, hang on a sec.

@dannon

This comment has been minimized.

Copy link
Member

commented May 30, 2017

(following up with separate PR as the security bit is unrelated to this specific code -- just this feature)

@dannon dannon merged commit 9896fc0 into galaxyproject:dev May 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@anuprulez

This comment has been minimized.

Copy link
Member Author

commented May 30, 2017

@dannon Thanks for merging. Will follow up the changes suggested in the next PR

@bgruening bgruening deleted the bgruening:workflowexport_mako branch May 30, 2017

@anuprulez

This comment has been minimized.

Copy link
Member Author

commented May 30, 2017

@dannon Do you wish to change just the url to /api/user/preferences? This will involve moving the api calls (which make call to other methods defined in WorkflowsAPIController controller to fetch a list of workflows) to /users api file. So, changing the url would involve not only the change in urls but moving some methods to /users api file. Is it ok?
@bgruening

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.