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

CSV panel actions for SavedSearch embedabbles #33709

Merged
merged 9 commits into from
Mar 29, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ class PanelActionsStore {
*/
public initializeFromRegistry(panelActionsRegistry: ContextMenuAction[]) {
panelActionsRegistry.forEach(panelAction => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some reason panel actions are duped everytime you re-enter a page when client-side routing occurs. This fixes that.

this.actions.push(panelAction);
if (!this.actions.includes(panelAction)) {
this.actions.push(panelAction);
}
});
}
}
Expand Down
9 changes: 8 additions & 1 deletion src/legacy/ui/public/kfetch/kfetch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,19 @@ describe('kfetch', () => {
});
});

it('should return response', async () => {
it('should return JSON responses by default', async () => {
fetchMock.get('*', { foo: 'bar' });
const res = await kfetch({ pathname: 'my/path' });
expect(res).toEqual({ foo: 'bar' });
});

it('should not return JSON responses by defaul when `parseJson` is `false`', async () => {
fetchMock.get('*', { foo: 'bar' });
const raw = await kfetch({ pathname: 'my/path' }, { parseJson: false });
const res = await raw.text();
expect(res).toEqual('{"foo":"bar"}');
});

it('should prepend url with basepath by default', async () => {
fetchMock.get('*', {});
await kfetch({ pathname: 'my/path' });
Expand Down
8 changes: 5 additions & 3 deletions src/legacy/ui/public/kfetch/kfetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export interface KFetchOptions extends RequestInit {

export interface KFetchKibanaOptions {
prependBasePath?: boolean;
parseJson?: boolean;
}

export interface Interceptor {
Expand All @@ -50,7 +51,7 @@ export const addInterceptor = (interceptor: Interceptor) => interceptors.push(in

export async function kfetch(
options: KFetchOptions,
{ prependBasePath = true }: KFetchKibanaOptions = {}
{ prependBasePath = true, parseJson = true }: KFetchKibanaOptions = {}
) {
const combinedOptions = withDefaultOptions(options);
const promise = requestInterceptors(combinedOptions).then(
Expand All @@ -61,10 +62,11 @@ export async function kfetch(
});

return window.fetch(fullUrl, restOptions).then(async res => {
const body = await getBodyAsJson(res);
const body = parseJson ? await getBodyAsJson(res) : null;
if (res.ok) {
return body;
return parseJson ? body : res;
}

joelgriffith marked this conversation as resolved.
Show resolved Hide resolved
throw new KFetchError(res, body);
});
}
Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugins/reporting/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ export const reporting = (kibana) => {
'plugins/reporting/share_context_menu/register_csv_reporting',
'plugins/reporting/share_context_menu/register_reporting',
],
contextMenuActions: [
'plugins/reporting/panel_actions/get_csv_panel_action',
],
hacks: ['plugins/reporting/hacks/job_completion_notifier'],
home: ['plugins/reporting/register_feature'],
managementSections: ['plugins/reporting/views/management'],
Expand Down
120 changes: 120 additions & 0 deletions x-pack/plugins/reporting/public/panel_actions/get_csv_panel_action.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import dateMath from '@elastic/datemath';
import { i18n } from '@kbn/i18n';

import { ContextMenuAction, ContextMenuActionsRegistryProvider } from 'ui/embeddable';
import { PanelActionAPI } from 'ui/embeddable/context_menu_actions/types';
import { kfetch } from 'ui/kfetch';
import { toastNotifications } from 'ui/notify';
import { API_BASE_URL_V1 } from '../../common/constants';

const API_BASE_URL = `${API_BASE_URL_V1}/generate/immediate/csv/saved-object/`;

class GetCsvReportPanelAction extends ContextMenuAction {
constructor() {
super(
{
displayName: i18n.translate('xpack.reporting.dashboard.downloadCsvPanelTitle', {
defaultMessage: 'Download CSV',
}),
id: 'downloadCsvReport',
parentPanelId: 'mainMenu',
},
{
icon: 'document',
}
);
}

public async generateJobParams({ searchEmbeddable }: { searchEmbeddable: any }) {
const adapters = searchEmbeddable.getInspectorAdapters();
if (!adapters) {
return {};
}

if (adapters.requests.requests.length === 0) {
return {};
}

return searchEmbeddable.searchScope.searchSource.getSearchRequestBody();
}

// @TODO: Clean this up once we update SavedSearch's interface
// and location in the file-system. `viewMode` also has an enum
// buried inside of the dashboard folder we could use vs a bare string
public isVisible = (panelActionAPI: PanelActionAPI): boolean => {
const { embeddable, containerState } = panelActionAPI;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to go with duck-type-typing here since it was proving to be impossible to import the SavedSearchEmbedable component :/

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so the actual class we want is from src/legacy/core_plugins/kibana/public/discover/embeddable/search_embeddable.ts, which is an extension of the Embeddable class. That's why we have to check for the savedSearch property to make sure it's valid?

In that case, I think this could use a comment to marker where we might be able to improve things down the road.

return (
containerState.viewMode !== 'edit' && !!embeddable && embeddable.hasOwnProperty('savedSearch')
);
};

// TODO: Need to expose some of the interface here from SavedSearch
// as well as pass things like columns into the API call
public onClick = async (panelActionAPI: PanelActionAPI) => {
const { embeddable } = panelActionAPI as any;
const {
timeRange: { from, to },
} = embeddable;

if (!embeddable) {
return;
}

const searchEmbeddable = embeddable;
const state = await this.generateJobParams({ searchEmbeddable });
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be wrapped in _.pick to whitelist fields that we care about. For example, we don't care about highlight, size, and script_fields (since they can't create script fields on-the-fly)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, this is just a comment of something we can figure out later, because I'm not sure what we need at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

Another thing (that doesn't need to be figured out right now) - this object doesn't seem to contain the columns.

  1. Create a saved search
  2. Add it to a dashboard
  3. Change the columns in the panel of the dashboard
  4. We need to figure out how to get the columns to show, it's not in this state (but it could be)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!


const id = `search:${embeddable.savedSearch.id}`;
const filename = embeddable.savedSearch.title;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe next, we should work on exposing the SearchEmbeddable interface here, because .savedSearch and .$rootScope are typed as private in 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.

In addition to the other headaches with the SearchEmbeddable interface (😁) it seems too limited for the definition to be in /discover. The Discover app is used for creating a saved search... but having the defined structure of a saved search is important all over the Kibana codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think I'm going to add another issue where we do some cleanup/migration of this component so we can better use it in our x-pack code.

const timezone = embeddable.$rootScope.chrome.getUiSettingsClient().get('dateFormat:tz');
const fromTime = dateMath.parse(from);
const toTime = dateMath.parse(to);

if (!fromTime || !toTime) {
return this.onGenerationFail(
new Error(`Invalid time range: From: ${fromTime}, To: ${toTime}`)
);
}

const body = JSON.stringify({
timerange: {
min: fromTime.valueOf(),
max: toTime.valueOf(),
timezone,
},
state,
});

await kfetch({ method: 'POST', pathname: `${API_BASE_URL}${id}`, body }, { parseJson: false })
.then(r => r.text())
.then(csv => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kind of a nasty client-side mechanism to do CSV downloads browser-side. Might want to consolidate this into some sort of util elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like typical JS to me 😁

const blob = new Blob([csv], { type: 'text/csv' });
const a = window.document.createElement('a');
const downloadObject = window.URL.createObjectURL(blob);
a.href = downloadObject;
a.download = `${filename}.csv`;
a.click();
window.URL.revokeObjectURL(downloadObject);
})
.catch(this.onGenerationFail);
};

private onGenerationFail(error: Error) {
toastNotifications.addDanger({
Copy link
Member

Choose a reason for hiding this comment

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

💚

title: i18n.translate('xpack.reporting.dashboard.failedCsvDownloadTitle', {
defaultMessage: `CSV download failed`,
}),
text: i18n.translate('xpack.reporting.dashboard.failedCsvDownloadMessage', {
defaultMessage: `We couldn't download your CSV at this time.`,
}),
'data-test-subj': 'downloadCsvFail',
});
}
}

ContextMenuActionsRegistryProvider.register(() => new GetCsvReportPanelAction());
2 changes: 1 addition & 1 deletion x-pack/test/reporting/api/generate/csv_saved_search.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import expect from 'expect.js';
import expect from '@kbn/expect';
import supertest from 'supertest';
import { CSV_RESULT_TIMEBASED, CSV_RESULT_TIMELESS } from './fixtures';

Expand Down