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

Show tags, import by drag-and-drop and avoid refreshed in workflow page #4476

Merged
merged 26 commits into from Sep 1, 2017

Conversation

Projects
None yet
4 participants
@mvdbeek
Copy link
Member

commented Aug 22, 2017

This adds support for tagging workflows directly on the overview page,
allows downloading, renaming and copying workflows without refreshing the page
and allows dragging workflow files into the center frame to import them.

This workflow also includes #4500, once that is merged it should be clearer.

@@ -133,7 +135,70 @@ var TagsEditor = Backbone.View
});

// =============================================================================

var WorkflowTagsEditor = TagsEditor.extend({

This comment has been minimized.

Copy link
@guerler

guerler Aug 22, 2017

Contributor

Any chance we can re-use or extend the tags editor without introducing a workflow specific tag editor? I am asking because tag editors will be needed for other grids in the future too.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 22, 2017

Author Member

Yeah, I guess we could change the behavior of the tag editor (it's used for datasets and collections as well) to be the same as the WorkflowTagsEditor (i.e click on the tag to edit it, and close the editor with Escape instead of a separate button for opening/closing the editor).

This comment has been minimized.

Copy link
@guerler

guerler Aug 22, 2017

Contributor

Yeah or you can add flags to govern the closing behavior or any other preferences, we have something similar here:
https://github.com/galaxyproject/galaxy/blob/dev/client/galaxy/scripts/mvc/form/form-view.js#L12
This would avoid inheritance and would let us use combinations of options. It would be nice to have one customizable tag editor we can extend in this fashion and use throughout the ui.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 29, 2017

Author Member

@guerler I merged the WorkflowTagsEditor with the TagsEditor in 3ca147f, is that what you had in mind ?

@mvdbeek mvdbeek referenced this pull request Aug 28, 2017

Closed

Prototype published history grid using Vue.js #4425

0 of 7 tasks complete

@mvdbeek mvdbeek force-pushed the mvdbeek:workflow_view_tags branch from 323032e to 2874973 Aug 28, 2017

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2017

We can now download and rename workflows without leaving the overview page or refreshing the whole page, and we can drop (one or many) workflow files to (batch) import them, but there is still some work left do.

@mvdbeek mvdbeek force-pushed the mvdbeek:workflow_view_tags branch 3 times, most recently from 6d44419 to 3ca147f Aug 29, 2017

@mvdbeek mvdbeek changed the title [WIP] Workflow view tags Show tags, import by drag-and-drop and avoid refreshed in workflow page Aug 31, 2017

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

@mvdbeek mvdbeek force-pushed the mvdbeek:workflow_view_tags branch 4 times, most recently from e956abd to cc3a0fa Aug 31, 2017

@jmchilton

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

I missed this at first - I thought it was just some small tag change - this is so incredible! This eliminates so many clicks. I've merged your other PR, can you rebase to resolve conflicts?

@martenson
Copy link
Member

left a comment

Looks great. Few comments inline. It would be nice to also show only one notification when importing multiple workflows but that is minor.

initialize: function(){
_.bindAll(this, 'render', '_rowTemplate', 'renderTagEditor', '_templateActions', 'removeWorkflow', 'copyWorkflow'); // every function that uses 'this' as the current object should be in here
mod_toastr.options.timeOut = 1500;
mod_toastr.options.positionClass = "toast-top-left";

This comment has been minimized.

Copy link
@martenson

martenson Sep 1, 2017

Member

Could we please stick to top-right? I think all other implementations show notifications there.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Sep 1, 2017

Author Member

Sure, np.

},

/** Template for user actions for workflows */
_templateActions: function( ) {

This comment has been minimized.

Copy link
@martenson

martenson Sep 1, 2017

Member

I think using the underscore templating would be helpful here but not necessary.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Sep 1, 2017

Author Member

Can you point me to an example in the codebase ?


var logNamespace = 'workflow';
//==============================================================================
/** @class model for tool citations.

This comment has been minimized.

Copy link
@martenson

martenson Sep 1, 2017

Member

copypasta?

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Sep 1, 2017

Author Member

indeed

url: '/api/workflows',

initialize: function () {
this.fetch();

This comment has been minimized.

Copy link
@martenson

martenson Sep 1, 2017

Member

I am not sure this belongs to the model, would it be better served in view with the rest of the controller's logic?

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Sep 1, 2017

Author Member

Makes sense, I'll have a look

mvdbeek added some commits Aug 21, 2017

Allow updating workflows from partial information
The workflow overview page gets workflow information from the
/api/workflows, which returns a reduced amount of information, most
notably the steps are not being returned. This commit skips updating the
actual workflow steps if the steps are not included in the workflow
payload. We can now update name, tags and whether a workflow should be
shown in the tool panel. We also return owner and number of steps in the
response to importing workflows.

@mvdbeek mvdbeek force-pushed the mvdbeek:workflow_view_tags branch from cc3a0fa to bbaf59a Sep 1, 2017

mvdbeek and others added some commits Sep 1, 2017

More workflow index page Selenium tests.
- Fix rename test for recent change to using alert.
- Test basic adding a tag.
- Outline of a test for downloading workflows.
- Add test for publishing display on index.
- Add test for using the search box on the workflow index page.
@jmchilton

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

@mvdbeek I pushed some tests into your branch - hope that is cool - feel free to strip them out if not.

@martenson

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

@mvdbeek once you OK the tests that @jmchilton added I will merge. This is splendid.

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2017

@martenson

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

@mvdbeek your templating approach is not wrong, it is more of personal preference at this use case I guess. For a more complex template the underscore templating is a better choice I believe.

@martenson martenson merged commit 47efd80 into galaxyproject:dev Sep 1, 2017

5 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
api test Build finished. 284 tests run, 0 skipped, 0 failed.
Details
framework test Build finished. 161 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
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.