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 png report as panel action #26476

Closed

Conversation

bgaddis56
Copy link
Contributor

Summary

Added a PNG report panel action to each dashboard panel if it is a visualization which produces a PNG report for the visualization selected. It utilized the current filters and time range for the report.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@stacey-gammon stacey-gammon added the WIP Work in progress label Nov 30, 2018
appquerystring = appquerystring.replace(new RegExp('#', 'g'), '%23');

// mode of quick is hard coded and works ok even if time is relative or absolute and
// refreshinterval is not used in the report
const visualizationURL =
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about pushing this into the embeddable? Accessing embeddable urls will be useful in many other places. There actually is already an embeddable.metadata.editUrl I think, but it lacks the query parameters. Kind of like how I suggest doing in here: #25247. But rather than an API service, just put it in the embeddable layer, and then later, if we want to use an API service, we can replace it there.

class VisualizeEmbeddable {

public generateAccessLink({ containerState }) {
  const id = linkData.id;
  const appState = `(filters:!(${containerState.filters})`;
  const globalState = `(${containerState.globalTime})`;
  // Use a url service to ensure everything is escaped properly
  const url = url.parse({
    path: `kibana`,
    hash: `#visualizations/edit/${id}`,
    queryParameters: {
      '_g': globalState,
      '_a': appState,
    }
  })
  return url.toString();
}
}

I think if you use a url service, you will get the encoding for free, that's why I used url to take the string pieces into a url, then back into string again. https://nodejs.org/api/url.html#url_url_parse_urlstring_parsequerystring_slashesdenotehost

You will have access to Private in that class as well to grab the state object so this class can probably not be concerned about it.

@@ -63,6 +63,10 @@ interface EmbeddableOptions {
export abstract class Embeddable {
public readonly metadata: EmbeddableMetadata = {};

public readonly filters: Filters = [];
public readonly timeRange: TimeRange = { to: '', from: '' };
public readonly savedVisualization: SavedVisualization = { id: '' };
Copy link
Contributor

Choose a reason for hiding this comment

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

don't want this here, since not all embeddables will have a savedVisualization. Saved Search has savedSearch object.

We could store the embeddable type on the embeddable, the one that the Factory is registered under, and do something like embeddable.type == 'visualization' to check. I don't love that idea though... what if another plugin author wants to create an embeddable and it supplies a generateAccessUrl function, so it wants to take advantage of the reporting system? We should be able to handle that, without Reporting needing to know about it. But then we also don't really want the embeddable layer to know about the plugin reporting, so I also don't really want to expose on the embeddable something like canGenerateReport.

So I have put some thought into this, about using another registry where embeddables can say "I support this action" . Let's sync on this later, I'll share some of my docs/thoughts and get your input on it.

@tsullivan
Copy link
Member

I pulled the branch up and ran the source, and saw some optimizations failures:

optmzr    log   [17:44:23.562] [fatal][optimize] Optimization failed in 23.82 secondsError: Optimizations failure.
   4378 modules

    ERROR in ./x-pack/plugins/reporting/public/panel_actions/get_csv_report_panel_action.tsx
    Module not found: Error: Can't resolve '../../../../../src/core_plugins/kibana/public/discover/embeddable/search_embeddable' in '/Users/tsullivan/elastic/kibana/x-pack/plugins/reporting/public/panel_actions'

    ERROR in ./x-pack/plugins/reporting/public/panel_actions/get_png_report_panel_action.tsx
    Module not found: Error: Can't resolve '../../../../../src/core_plugins/kibana/public/visualize/embeddable/visualize_embeddable' in '/Users/tsullivan/elastic/kibana/x-pack/plugins/reporting/public/panel_actions'


// Use a url service to ensure everything is escaped properly
const myurl = `/app/kibana#` + url.format({
pathname: '/visualize/edit/' + id,
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't work, the url for saved searches I think is discover/edit .

Tests should probably be added as part of this PR too, so if the url format changes, the ci will fail.

@@ -58,6 +60,32 @@ export class SearchEmbeddable extends Embeddable {
};
}

/**
* Generates an access URL that will be used in creating a PNG report.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Generates an access URL that will be used in creating a PNG report.
* Generates an access URL that can be used to open up the saved search directly in the Discover App, with the settings applied from the given containerState.

Don't mention PNG report in here, this is on the base embeddable layer now, reporting just happens to use it, but it's not really here specifically for reporting (I think it will be useful in other scenarios as well).

@bgaddis56
Copy link
Contributor Author

bgaddis56 commented Dec 5, 2018 via email

} from 'ui/embeddable';
import { kfetch } from 'ui/kfetch';
import { toastNotifications } from 'ui/notify';
import { SearchEmbeddable } from '../../../../../src/legacy/core_plugins/kibana/public/discover/embeddable/search_embeddable';
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 not entirely sure this will work in a build, because of how x-pack gets relocated as a plugin

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, you are right, it won't.

@joelgriffith
Copy link
Contributor

joelgriffith commented Feb 12, 2019

Going to close this in favor of the work and I are doing. PR link incoming

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants