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

Modular Explore #3657

Closed
scolapasta opened this Issue Mar 1, 2017 · 49 comments

Comments

@scolapasta
Copy link
Contributor

scolapasta commented Mar 1, 2017

Right now we have some external tools that we allow users to "explore" with (TwoRavens, GeoConnect). We'd like to make this more modular to make it easier for installations to add new tools.

Ideally, this would involve adding a new table that could know the url of the tool and other needed info (type of file?), but it may also involve having to write a handler for each. For this, we would want to follow the SPI model, and provide a default handler for simple apps.

@pdurbin

This comment has been minimized.

Copy link
Member

pdurbin commented May 18, 2017

Some more on the modularity and SPI topic:

Also, https://projects.iq.harvard.edu/dcm2017/agenda shows " SPIs and Modularity: Are you interested in learning about Dataverse's emergent modular architecture?"

@djbrooke

This comment has been minimized.

Copy link
Contributor

djbrooke commented Nov 1, 2017

  • We'll limit this to the file level for now
  • Towards to goal of small chunks, the scope of this issue is that we want an installation to be able to run Data Explorer and TwoRavens in parallel.
  • @scolapasta will edit this to add additional details
@scolapasta

This comment has been minimized.

Copy link
Contributor Author

scolapasta commented Nov 2, 2017

Repeating part of comment from #4230:

Besides the more obvious dynamic text and links we need for external tools, a couple of aspects that we need to handle in a modular way are:

When to show the links to the external tool
How to handle the link to the external tool (i.e. wha dynamic info to send)
In both these cases, we can think of building these in incremental ways, with each step building on the previous step, building from simplest, but supporting fewer tools, to most complex, but also most flexible.

For when to show the link, we have 3 steps:
A. simple list of tools, will only support one operation (i.e. Configure)
B. Add "Operation" to list, in order to support multiple operations (i.e. Configure vs. Explore)
C. Add "Subject of Operation" in order to support operations on different kind of objects (i.e. tools that work on tabular files, like DataExplorer, vs tools that work on geospatial, like World Map.

For how to handle link we similarly have 3 steps:

  1. Dictate the exact parameter names and values that an external tool will need.
  2. Define parameters dynamically from a list of "reserved" terms that we define.
  3. Define individual "Handler" Java classes and SPI interface, in order to handle any tool.

In order to support Modular Explore for Two Ravens and DataExplore, the minimum we technically need is A1. However, since we are working on Modular Configure first, that bumps us to B1. In addition, in this scenario we would need to contact the external tools and have them change their parameters and we would rather be more flexible. So the decision is that the definition for done for this issue is B2.

Note, that we would still not be able to support geoConnect / WorldMap in a modular way yet, until we support at least C2, but per the previous comment, that is out of scope for this issue.

@djbrooke

This comment has been minimized.

Copy link
Contributor

djbrooke commented Nov 8, 2017

Thanks @scolapasta for taking the lead on breaking this up before next week's backlog grooming session.

@pdurbin

This comment has been minimized.

Copy link
Member

pdurbin commented Nov 22, 2017

@michbarsinai I pinged you in Slack because I said we'll need to write a more intelligent parser. Please see 272b4c2 for a terrible kludge and tests with examples of what we need. Thanks.

@pdurbin

This comment has been minimized.

Copy link
Member

pdurbin commented Jan 23, 2018

@jggautier after standup we talked about if there's a chance I messed up download counts by differentiating between "TwoRavens" and "Data Explorer" or whatever. I don't think so. As I hoped it looks like the code is simply counting rows like this: "select count(o.id) from GuestbookResponse o where o.datafile_id = " + dataFileId. That's from a method called getCountGuestbookResponsesByDataFileId in GuestbookResponseServiceBean.

@scolapasta that's fine. As you may have noticed, we dragged this issue into QA this morning during standup. Maybe we could tell third-party developers that they need to ask for a UUID from us for their application to be included in our app store (or a least be listed in the guides). I'm still trying to control the scope of this issue.

@kcondon I hope this helps:

Changes to consider testing:

  • TwoRavens: TwoRavens has been converted into an "external tool". Part of the installation instructions have been adjusted to include a curl command operating on a JSON file. Two database settings have been removed: :TwoRavensTabularView and :TwoRavensUrl. Note that the "subset" functionaly still has TwoRavens branding but is independent of TwoRavens.
  • WorldMap: The logic for when to show the WorldMap button has been adjusted to be aware of the presence of external tools.
  • Configure button: The Modular Configure code (#4230) has been refactored and should be retested. Note that external tools can now have a type "configure" vs "explore" as noted in the guides.
  • Download counts and types: Instead of "Explore" we now record the names of the external tools. The rules around download counts should not have changed. I'm not sure if they are documented or not.
  • Guides: The guides have been adjusted to mention external tools as appropriate but further work will be done for #4429 some day.

To think about:

  • What is the definition of done for integrating Data Explorer into Dataverse (#4249)? Should we go ahead and test Data Explorer now? It seems to work for me. 😄

Known issues:

  • When you launch an external tool from a popup the tool will take over the browser window rather than opening in a new tab. When there is no popup, the tool launches in a new tab, which is the desired behavior. This bug exists in production for TwoRavens and has been discussed with Gustavo, Steve, and Mike but we don't have a fix. The inconsistency is unfortunate.
@pdurbin

This comment has been minimized.

@kcondon kcondon self-assigned this Jan 23, 2018

mheppler added a commit that referenced this issue Jan 23, 2018

@kcondon

This comment has been minimized.

Copy link
Contributor

kcondon commented Jan 23, 2018

Notes to self:

  1. db update script is required to run this.
  2. Significant doc changes with instructions on how to add external tools, including new api endpoints?
@mheppler

This comment has been minimized.

Copy link
Contributor

mheppler commented Jan 23, 2018

  • Revised popup title to "Dataset Terms" for download/explore workflows on dataset/file pgs.

screen shot 2018-01-23 at 4 15 36 pm

@scolapasta

This comment has been minimized.

Copy link
Contributor Author

scolapasta commented Jan 24, 2018

@pdurbin to respond to your App store comparison, yes the name is important, but behind the scenes what is used (at least in Android) is the package name. Beyond multiple tools with the same name, another concern is the same tool with a changing name, i.e having two rows that should map to the same thing, but don't (or when we internationalize and remove the name from the db).

I generally prefer to tackle issues which would require future database updates that modify data (as opposed to structure) preemptively. I don't see this as an expansion of scope (rather as a good coding practice); the expansion of scope already occurred when we decided to track the specific tool.

@pdurbin

This comment has been minimized.

Copy link
Member

pdurbin commented Jan 24, 2018

@scolapasta as I mentioned, I'm interested in controlling the scope of this issue. I'm happy to revert c236079 to make it so we go back to persisting "Explore" rather than "Two Ravens" or "Data Explorer" or whatever. That commit was so recent I expect it will revert cleanly and easily.

@djbrooke in the demo you seemed to like that individual tools are being tracked. What would you like?

@djbrooke

This comment has been minimized.

Copy link
Contributor

djbrooke commented Jan 24, 2018

Good discussion, thanks. I understand the reasoning for IDs instead of names, but let's keep this moving for now. On to QA!

pdurbin added a commit that referenced this issue Jan 24, 2018

pdurbin added a commit that referenced this issue Jan 24, 2018

mention external tools as something devs can hack on #3657
Other tweaks. Link to current roadmap, waffle.
@kcondon

This comment has been minimized.

Copy link
Contributor

kcondon commented Jan 24, 2018

Issues found so far:
-last line in db update script is missing a ;

pdurbin added a commit that referenced this issue Jan 25, 2018

@pdurbin

This comment has been minimized.

Copy link
Member

pdurbin commented Jan 25, 2018

@kcondon whoops, good catch. I added the missing semicolon in 92e0a43

As we discussed after standup yesterday there are technically no new API endpoints but the "manifest" format (JSON) has changed in that external tool authors must specify if their tool should appear under the "Explore" button or the "Configure" button.

@kcondon

This comment has been minimized.

Copy link
Contributor

kcondon commented Jan 25, 2018

Thanks. So this is not new?
curl -X POST -H 'Content-type: application/json' --upload-file twoRavens.json http://localhost:8080/api/admin/externalTools

@pdurbin

This comment has been minimized.

Copy link
Member

pdurbin commented Jan 25, 2018

Well, twoRavens.json is new (in the future, we'd like the TwoRavens team to host this file and #4429 is related) but the API endpoint is not new. It shipped with 4.8.5 and is documented at http://guides.dataverse.org/en/4.8.5/installation/external-tools.html#making-an-external-tool-available-in-dataverse

pdurbin added a commit that referenced this issue Jan 25, 2018

pdurbin added a commit that referenced this issue Jan 26, 2018

eliminate buggy non-dropdown version of WorldMap explore button #3657
We were seeing "WorldMap" under the dropdown "Explore" button (good!)
and a non-dropdown "Explore" button was appearing next to it (bad!).

We are now making all these explore tools consistent. They are always
available via the dropdown "Explore" button. No more clicking an
"Explore" button and wondering which tool will launch.
@pdurbin

This comment has been minimized.

Copy link
Member

pdurbin commented Jan 26, 2018

@kcondon just found a bug (thanks!) where two "Explore" buttons were being displayed side by side (one a dropdown and one a non-drop down). I fixed this in c6c0c87 and left a detailed comment about the fix in the commit.

@kcondon

This comment has been minimized.

Copy link
Contributor

kcondon commented Jan 26, 2018

x-Seeing two explore buttons on a shape file that has already been mapped.

@pdurbin

This comment has been minimized.

Copy link
Member

pdurbin commented Jun 6, 2018

Known issues:

  • When you launch an external tool from a popup the tool will take over the browser window rather than opening in a new tab. When there is no popup, the tool launches in a new tab, which is the desired behavior. This bug exists in production for TwoRavens and has been discussed with Gustavo, Steve, and Mike but we don't have a fix. The inconsistency is unfortunate.

This is now being tracked at #4742.

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.