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

add report icon to data panel data object toolbar [SCT-930] #1321

Merged
merged 5 commits into from
Jun 1, 2018

Conversation

eapearson
Copy link
Collaborator

No description provided.

- adds call to workspace to fetch referencing objects when gathering object info
- adds menu button to toolbar: enabled if a report is available, disabled otherwise
- refactored a bit to move button creation out of main toolbar method
- refactored buttons to use the (already generated) object-ified object info
- self -> _this; self is a global variable (property of window)

var reportRegex = /^KBaseReport.Report-/;

return narrativeService.callFunc('list_objects_with_sets', [{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do, exactly? It looks like you're stitching in to the Data Panel populator and looking up everything that references each object, and filtering for reports? That should either just be part of the NarrativeService call. Or, to make it faster, just find the report when the it's requested. In a large narrative, this might take forever.

Also, copied objects (i.e. everything in a copied narrative) lose that link. You can find it by tracing back up the copy chain, but that's also something that shouldn't be applied to every object in a Narrative, and only on request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on a function in NarrativeService right now that should do what you need.

@eapearson
Copy link
Collaborator Author

Yes, you got it. I should have added a note, but presumed because of the earlier conversation that you might pick up the NS side of it if you had time, as you have. Otherwise, it works well enough as is to iterate upon.

@briehl
Copy link
Member

briehl commented May 25, 2018

I'll have to pick this up later in the weekend. Got a function mostly there, but not entirely tested. In the meantime, would this work? Think anything else would be helpful?
https://github.com/briehl/NarrativeService/blob/report_lookup/NarrativeService.spec#L393-L423

I figure the service itself should return a list of reports (if there's more than one), and the interface can do with that what it wants.

@coveralls
Copy link

coveralls commented May 26, 2018

Coverage Status

Coverage decreased (-0.2%) to 13.161% when pulling af7672a on eapearson:SCT930 into b2dc91f on kbase:develop.

@eapearson
Copy link
Collaborator Author

Took a look at the NS changes finally. Looks like it covers the needs just fine ...
I'm wondering about including the error within the result itself. Might it be better to include it as a second return value? I've used this pattern, and it makes it very easy to check for the outcome of the call ... either an exception that falls through and generates a 500, or a result, error pair. The presence of a non-null error indicates an error, and the value some error value.
Also, it isn't altogether clear to me how an object can be referenced by more than one report. Could you explain?

@briehl
Copy link
Member

briehl commented May 30, 2018

Hey, before merging this, would you mind looking at (and maybe merging) #1323 ? I suspect lingering nonsense with PhantomJS is one of the causes of our recent testing woes.

@briehl briehl merged commit cbceeb9 into kbase:develop Jun 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants