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

Tool-Describing-Tours #4019

Merged
merged 34 commits into from Jun 14, 2017

Conversation

Projects
None yet
9 participants
@anatskiy
Copy link
Contributor

commented May 4, 2017

Description

This PR adds a new plugin "Tool-Describing-Tours" (TDT), which generates interactive tours for a given tool. The plugin looks at the galaxy tool object, reads the information about the tool's test, and uploads all test datasets from it. Then, it generates a tour (using the Tours API) and iteratively asks the user to either entry a value or to select a parameter, using the test parameters.

Also, as a part of this PR, I slightly extended the Webhooks API:

  • pass parameters from the client to the plugin's helper function
  • add two new entry points
    • onload - automatically load all the code from the plugin's script.js to the <head></head> section
    • tool-menu - show a button in the tool's options menu

All other plugins will still work.

Demo

demo

Known issues

  • Sometimes, tour messages are skipped because of the conditional input type. This happens because something creates two DOM elements with the same id, so the Tours cannot decide which one to use to show the message.

screenshot

anatskiy added some commits Mar 5, 2017

Merge branch 'dev' into auto_tour_generation
# Conflicts:
#	static/maps/mvc/tool/tool-form-base.js.map
#	static/maps/mvc/tool/tool-form.js.map
#	static/scripts/bundled/analysis.bundled.js
#	static/scripts/bundled/analysis.bundled.js.map
#	static/scripts/bundled/libs.bundled.js
#	static/scripts/bundled/libs.bundled.js.map
#	static/scripts/bundled/login.bundled.js
#	static/scripts/bundled/login.bundled.js.map
#	static/scripts/mvc/tool/tool-form-base.js
#	static/scripts/mvc/tool/tool-form.js
many improvements
- show warning messages (not errors)
- upload test datasets with a correct type according to the tool's .xml file (no 'auto' detection)
- add more checks to improve the overall stability
- fix tour messages
- other bugfixes
@@ -456,7 +456,8 @@ define([ 'utils/utils', 'utils/deferred', 'mvc/ui/ui-misc', 'mvc/form/form-view'
if ($.isArray( response ) && response.length > 0) {
self.$el.append( $( '<div/>', { id: 'webhook-view' } ) );
var WebhookApp = new Webhooks.WebhookView({
urlRoot: Galaxy.root + 'api/webhooks/workflow'
urlRoot: Galaxy.root + 'api/webhooks/workflow',
toolId: job_def.tool_id
});

This comment has been minimized.

Copy link
@jmchilton

jmchilton May 4, 2017

Member

At an initial glance - it seems like version should be required here as well - if there are multiple tools with the same id but different versions loaded. In the tool shed case they will have separate IDs - but if they are locally added tools I feel like we should handle that case.

This comment has been minimized.

Copy link
@anatskiy

anatskiy May 4, 2017

Author Contributor

Oh, right. Thanks!

This comment has been minimized.

Copy link
@bgruening

bgruening May 4, 2017

Member

Good point!

@bgruening

This comment has been minimized.

Copy link
Member

commented May 4, 2017

I feel that our tests (inside every tools) are not only great because they are tests, but also because they have the potential of demonstrating how a tools works to a User.
Providing a clue about the expected input and outputs, how you can change parameters etc...

If we can leverage this to the community we have a great resource for training with very little effort as all of our tools already contain tests :)

@jmchilton

This comment has been minimized.

Copy link
Member

commented May 4, 2017

I love this idea in general - and because it is an optional plugin as long as the client side focused committers are fine with the interface changes I'm okay with this particular implementation.

I do think we should take it and invest some time in making it more usable though. I'd like to see:

  • Annotations on tests that explicitly describe the test case as a whole and on particular parameters as needed. These things should be presented to users.
  • Only display test cases with annotation - we have test cases that test things fail or instance - they shouldn't be exposed to end users.
  • Extend this to work with repeats, collections, etc...
  • Add a formal API endpoint or abstractions on the tool class in the code to find the test data.
  • Moved out of the webhook framework and into the compiled code base.

@martenson martenson removed the triage label May 4, 2017

dannon and others added some commits May 5, 2017

remove unnecessary whitespace fixes
from otherwise untouched files
@martenson

This comment has been minimized.

Copy link
Member

commented May 5, 2017

@dannon I removed the whitespace fixes from otherwise untouched files

@dannon

This comment has been minimized.

Copy link
Member

commented May 5, 2017

Ok, so I fixed the library inclusion bit with anatskiy#4 (thanks for the merge @bgruening -- I figure having require available will leave this infrastructure flexible enough moving forward to use whatever you need to).

As mentioned in the PR, let me know if you want help with the other bit too (conditionally making all the webhooks requests), which I'd like to have prior to merge of this since we're adding a third request here.

And, thanks @martenson. I think having clean, focused PRs makes review easier for everyone.

@anatskiy

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

@dannon it would be very nice if you could help with loading webhooks only when they are enabled!

@dannon

This comment has been minimized.

Copy link
Member

commented May 15, 2017

@anatskiy Ok, I'll try to get to it later today.

@dannon

This comment has been minimized.

Copy link
Member

commented May 18, 2017

@anatskiy I think we can probably pipe this through the options bootstrapped into Galaxy.config; working on a proof of concept and hopefully we can get this merged prior to the training hackathon next week.

@guerler

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2017

Sorry for chiming in so late. The dom ids should be unique but are generated randomly. This is why we introduced the 'tour_id' attribute. It is user reliable and more readable then the uid's. However, we should look further into this to make sure that it is not a bug. A proof would be to find a case were the same id appears twice in the dom tree.

@dannon

This comment has been minimized.

Copy link
Member

commented Jun 6, 2017

All dom ids should definitely be unique regardless of tour handling. @anatskiy, do you have an example tool form where this error is displayed?

.gitignore Outdated
@@ -136,3 +136,4 @@ doc/source/dev/schema.rst
*.rej
*~
.idea/
.vscode/

This comment has been minimized.

Copy link
@dannon

dannon Jun 6, 2017

Member

Definitely minor, but I think things like this (visual studio, intellij, etc. project caches) should be put in the individual user's global .gitignore instead of here, where it's irrelevant to most other contributors.

This comment has been minimized.

Copy link
@anatskiy

anatskiy Jun 6, 2017

Author Contributor

@dannon sure thing! I didn't know that the user's .gitignore even exists.

@anatskiy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2017

@dannon yeah, in the description there is an example, which shows two dom elements with the same tour_id

592d0a04-30de-11e7-9ee7-6af63193e1dc

@dannon

This comment has been minimized.

Copy link
Member

commented Jun 6, 2017

@anatskiy Ahh, perfect! We should be able to track that down then.

dannon and others added some commits Jun 13, 2017

Remove .idea and .vscode from .gitignore (these are editor specific f…
…iles that contributors can add to their personal global .gitignores)
Merge pull request #5 from dannon/auto_tour_generation
Fixes .gitignore, merges dev.

@dannon dannon merged commit 579b8b0 into galaxyproject:dev Jun 14, 2017

1 check passed

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

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2017

Just for clarification, the screenshot above shows the expanded version of the same line. That is why it appears at first sight that the uid-14 and uid-33 are duplicate. Edit: I see now how the tour-ids are duplicates. Thanks for pointing this out.

@bgruening bgruening deleted the anatskiy:auto_tour_generation branch Jun 14, 2017

@bgruening

This comment has been minimized.

Copy link
Member

commented Jun 14, 2017

@dannon thanks for the merge! I hope this will make tools more useful than ever ... and it should make testing of tools also very easy - e.g. in Docker.

@guerler

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2017

@anatskiy Did you try to use the tour_id with additional child selectors to pick the first or last of the conditional cases? The reason why the tour_id is duplicate is that two conditional cases have the same parameter name. Only one is shown depending on the selected case.

@mblue9

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

Hello,

I only just found this very cool feature and tried it out for testing a tool (DiffBind https://github.com/galaxyproject/tools-iuc/blob/master/tools/diffbind/diffbind.xml) in usegalaxy.eu but it didn't work for this case, it only loaded 2 of the 8 test files into the history, see screenshot below.

screen shot 2018-04-11 at 12 07 22 pm

There is an example history of what that test should generate here: https://galaxy.uni-freiburg.de/u/mblue9/h/testing-diffbind
(I made collections there but just using the datasets outside collections should give the same result)

@anatskiy

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

Hi @mblue9, I'll take a look asap!

@TKlingstrom

This comment has been minimized.

Copy link

commented Aug 17, 2018

It breaks for me as well.

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.