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

[Lens][Dashboard] Adding Lens to Dashboard #53110

Merged
merged 23 commits into from
Jan 13, 2020
Merged
Show file tree
Hide file tree
Changes from 18 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 @@ -37,7 +37,6 @@ import {
KbnUrl,
SavedObjectSaveOpts,
unhashUrl,
VISUALIZE_EMBEDDABLE_TYPE,
} from '../legacy_imports';
import { FilterStateManager } from '../../../../data/public';
import {
Expand Down Expand Up @@ -334,13 +333,12 @@ export class DashboardAppController {
// This code needs to be replaced with a better mechanism for adding new embeddables of
// any type from the add panel. Likely this will happen via creating a visualization "inline",
// without navigating away from the UX.
if ($routeParams[DashboardConstants.NEW_VISUALIZATION_ID_PARAM]) {
container.addSavedObjectEmbeddable(
VISUALIZE_EMBEDDABLE_TYPE,
$routeParams[DashboardConstants.NEW_VISUALIZATION_ID_PARAM]
);
kbnUrl.removeParam(DashboardConstants.ADD_VISUALIZATION_TO_DASHBOARD_MODE_PARAM);
kbnUrl.removeParam(DashboardConstants.NEW_VISUALIZATION_ID_PARAM);
if ($routeParams[DashboardConstants.ADD_EMBEDDABLE_TYPE]) {
const type = $routeParams[DashboardConstants.ADD_EMBEDDABLE_TYPE];
const id = $routeParams[DashboardConstants.ADD_EMBEDDABLE_ID];
container.addSavedObjectEmbeddable(type, id);
kbnUrl.removeParam(DashboardConstants.ADD_EMBEDDABLE_TYPE);
kbnUrl.removeParam(DashboardConstants.ADD_EMBEDDABLE_ID);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@

export const DashboardConstants = {
ADD_VISUALIZATION_TO_DASHBOARD_MODE_PARAM: 'addToDashboard',
NEW_VISUALIZATION_ID_PARAM: 'addVisualization',
LANDING_PAGE_PATH: '/dashboards',
CREATE_NEW_DASHBOARD_URL: '/dashboard',
ADD_EMBEDDABLE_ID: 'addEmbeddableId',
ADD_EMBEDDABLE_TYPE: 'addEmbeddableType',
};

export function createDashboardEditUrl(id: string) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ import { unhashUrl } from '../../../../../../../plugins/kibana_utils/public';

import { initVisEditorDirective } from './visualization_editor';
import { initVisualizationDirective } from './visualization';

import {
VISUALIZE_EMBEDDABLE_TYPE,
subscribeWithScope,
absoluteToParsedUrl,
KibanaParsedUrl,
Expand Down Expand Up @@ -588,7 +588,11 @@ function VisualizeAppController(
getBasePath()
);
dashboardParsedUrl.addQueryParameter(
DashboardConstants.NEW_VISUALIZATION_ID_PARAM,
DashboardConstants.ADD_EMBEDDABLE_TYPE,
VISUALIZE_EMBEDDABLE_TYPE
);
dashboardParsedUrl.addQueryParameter(
DashboardConstants.ADD_EMBEDDABLE_ID,
savedVis.id
);
kbnUrl.change(dashboardParsedUrl.appPath);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ describe('NewVisModal', () => {
expect(window.location.assign).toBeCalledWith('#/visualize/create?type=vis&foo=true&bar=42');
});

it('closes if visualization with aliasUrl and addToDashboard in editorParams', () => {
it('closes and redirects properly if visualization with aliasUrl and addToDashboard in editorParams', () => {
const onClose = jest.fn();
window.location.assign = jest.fn();
const wrapper = mountWithIntl(
Expand All @@ -160,7 +160,7 @@ describe('NewVisModal', () => {
);
const visButton = wrapper.find('button[data-test-subj="visType-visWithAliasUrl"]');
visButton.simulate('click');
expect(window.location.assign).toBeCalledWith('testbasepath/aliasUrl');
expect(window.location.assign).toBeCalledWith('testbasepath/aliasUrl?addToDashboard');
expect(onClose).toHaveBeenCalled();
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,15 +143,18 @@ class NewVisModal extends React.Component<TypeSelectionProps, TypeSelectionState
this.trackUiMetric(METRIC_TYPE.CLICK, visType.name);
}

let params;
if ('aliasUrl' in visType) {
window.location.assign(this.props.addBasePath(visType.aliasUrl));
params = this.props.addBasePath(visType.aliasUrl);
if (this.props.editorParams && this.props.editorParams.includes('addToDashboard')) {
params = `${params}?addToDashboard`;
this.props.onClose();
}
window.location.assign(params);
return;
}

let params = [`type=${encodeURIComponent(visType.name)}`];
params = [`type=${encodeURIComponent(visType.name)}`];

if (searchType) {
params.push(`${searchType === 'search' ? 'savedSearchId' : 'indexPattern'}=${searchId}`);
Expand Down
4 changes: 4 additions & 0 deletions test/functional/page_objects/visualize_page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,10 @@ export function VisualizePageProvider({ getService, getPageObjects }: FtrProvide
async () => (await globalNav.getLastBreadcrumb()) === vizName
);
}

public async clickLensWidget() {
await this.clickVisType('lens');
}
}

return new VisualizePage();
Expand Down
25 changes: 25 additions & 0 deletions x-pack/legacy/plugins/lens/public/app_plugin/app.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ describe('Lens App', () => {
docId?: string;
docStorage: SavedObjectStore;
redirectTo: (id?: string) => void;
addToDashboardMode?: boolean;
}> {
return ({
editorFrame: createMockFrame(),
Expand Down Expand Up @@ -126,6 +127,7 @@ describe('Lens App', () => {
docId?: string;
docStorage: SavedObjectStore;
redirectTo: (id?: string) => void;
addToDashboardMode?: boolean;
}>;
}

Expand Down Expand Up @@ -306,6 +308,7 @@ describe('Lens App', () => {
docId?: string;
docStorage: SavedObjectStore;
redirectTo: (id?: string) => void;
addToDashboardMode?: boolean;
}>;

beforeEach(() => {
Expand Down Expand Up @@ -344,14 +347,19 @@ describe('Lens App', () => {

async function save({
initialDocId,
addToDashboardMode,
...saveProps
}: SaveProps & {
initialDocId?: string;
addToDashboardMode?: boolean;
}) {
const args = {
...defaultArgs,
docId: initialDocId,
};
if (addToDashboardMode) {
args.addToDashboardMode = addToDashboardMode;
}
args.editorFrame = frame;
(args.docStorage.load as jest.Mock).mockResolvedValue({
id: '1234',
Expand Down Expand Up @@ -543,6 +551,23 @@ describe('Lens App', () => {

expect(getButton(instance).disableButton).toEqual(false);
});

it('saves new doc and redirects to dashboard', async () => {
const { args } = await save({
initialDocId: undefined,
addToDashboardMode: true,
newCopyOnSave: false,
newTitle: 'hello there',
});

expect(args.docStorage.save).toHaveBeenCalledWith({
expression: 'kibana 3',
id: undefined,
title: 'hello there',
});

expect(args.redirectTo).toHaveBeenCalledWith('aaa');
});
});
});

Expand Down
14 changes: 12 additions & 2 deletions x-pack/legacy/plugins/lens/public/app_plugin/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { SavedObjectSaveModal } from 'ui/saved_objects/components/saved_object_s
import { AppMountContext, NotificationsStart } from 'src/core/public';
import { IStorageWrapper } from 'src/plugins/kibana_utils/public';
import { npStart } from 'ui/new_platform';
import { FormattedMessage } from '@kbn/i18n/react';
import { KibanaContextProvider } from '../../../../../../src/plugins/kibana_react/public';
import { Document, SavedObjectStore } from '../persistence';
import { EditorFrameInstance } from '../types';
Expand Down Expand Up @@ -50,6 +51,7 @@ export function App({
docId,
docStorage,
redirectTo,
addToDashboardMode,
}: {
editorFrame: EditorFrameInstance;
data: DataPublicPluginStart;
Expand All @@ -58,6 +60,7 @@ export function App({
docId?: string;
docStorage: SavedObjectStore;
redirectTo: (id?: string) => void;
addToDashboardMode?: boolean;
}) {
const language =
storage.get('kibana.userQueryLanguage') || core.uiSettings.get('search:queryLanguage');
Expand Down Expand Up @@ -166,6 +169,13 @@ export function App({

const { TopNavMenu } = npStart.plugins.navigation.ui;

const confirmButton = addToDashboardMode ? (
<FormattedMessage
id="xpack.lens.app.saveAddToDashboard"
defaultMessage="Save and add to dashboard"
/>
) : null;

return (
<I18nProvider>
<KibanaContextProvider
Expand Down Expand Up @@ -320,7 +330,6 @@ export function App({
persistedDoc: newDoc,
lastKnownDoc: newDoc,
}));

if (docId !== id) {
redirectTo(id);
}
Expand All @@ -337,10 +346,11 @@ export function App({
}}
onClose={() => setState(s => ({ ...s, isSaveModalVisible: false }))}
title={lastKnownDoc.title || ''}
showCopyOnSave={true}
showCopyOnSave={!addToDashboardMode}
objectType={i18n.translate('xpack.lens.app.saveModalType', {
defaultMessage: 'Lens visualization',
})}
confirmButtonLabel={confirmButton}
/>
)}
</KibanaContextProvider>
Expand Down
55 changes: 46 additions & 9 deletions x-pack/legacy/plugins/lens/public/app_plugin/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import 'uiExports/visResponseHandlers';
import 'uiExports/savedObjectTypes';

import React from 'react';
import { I18nProvider, FormattedMessage } from '@kbn/i18n/react';
import { HashRouter, Switch, Route, RouteComponentProps } from 'react-router-dom';
import { FormattedMessage, I18nProvider } from '@kbn/i18n/react';
import { HashRouter, Route, RouteComponentProps, Switch } from 'react-router-dom';
import { render, unmountComponentAtNode } from 'react-dom';
import { CoreSetup, CoreStart, SavedObjectsClientContract } from 'src/core/public';
import { DataPublicPluginStart } from 'src/plugins/data/public';
Expand All @@ -41,6 +41,11 @@ import {
import { NOT_INTERNATIONALIZED_PRODUCT_NAME } from '../../common';
import { KibanaLegacySetup } from '../../../../../../src/plugins/kibana_legacy/public';
import { EditorFrameStart } from '../types';
import {
getKibanaBasePathFromDashboardUrl,
addEmbeddableToDashboardUrl,
getDashboardUrlWithoutTime,
} from './url_helper';

export interface LensPluginSetupDependencies {
kibana_legacy: KibanaLegacySetup;
Expand Down Expand Up @@ -94,8 +99,44 @@ export class AppPlugin {
})
);

const redirectTo = (
routeProps: RouteComponentProps<{ id?: string }>,
addToDashboardMode: boolean,
id?: string
) => {
if (!id) {
routeProps.history.push('/lens');
} else if (!addToDashboardMode) {
routeProps.history.push(`/lens/edit/${id}`);
} else if (addToDashboardMode && id) {
routeProps.history.push(`/lens/edit/${id}`);
const url = context.core.chrome.navLinks.get('kibana:dashboard');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is adding more dependencies on context, which is being deprecated. Is there a better way to do this?

if (!url) {
return;
}
const lastDashboardAbsoluteUrl = url.url;
Copy link
Contributor

Choose a reason for hiding this comment

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

This also seems like a bunch of detail that belongs to the dashboard module, not to Lens. Maybe something like this:

  const url = dashboardUrl.addEmbeddable(url.url, id, 'lens');
  if (url) {
    window.history.pushState({}, '', url);
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the original intention, but then we figured the time range is not properly set. So for the hack with setting timerange to work, this won't be so clean.

const lensUrl = `${getKibanaBasePathFromDashboardUrl(
lastDashboardAbsoluteUrl
)}/lens/edit/${id}`;
const dashboardUrlWithoutTime = getDashboardUrlWithoutTime(lastDashboardAbsoluteUrl);
if (lensUrl && dashboardUrlWithoutTime) {
window.history.pushState({}, '', lensUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we generally avoid using pushState directly, and instead use a core service to do this, but I'm not sure. Might be worth looking into, anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it exists in the new platform. I think the pushState is our only option for now.

const dashboardParsedUrl = addEmbeddableToDashboardUrl(
dashboardUrlWithoutTime,
id,
'lens'
);
if (dashboardParsedUrl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this returns null, we silently fail. Is that the right thing? It seems like a null here is a legit error that should be surfaced in some way.

window.history.pushState({}, '', dashboardParsedUrl);
}
}
}
};

const renderEditor = (routeProps: RouteComponentProps<{ id?: string }>) => {
trackUiEvent('loaded');
const addToDashboardMode =
!!routeProps.location.search && routeProps.location.search.includes('addToDashboard');
return (
<App
core={context.core}
Expand All @@ -104,13 +145,8 @@ export class AppPlugin {
storage={new Storage(localStorage)}
docId={routeProps.match.params.id}
docStorage={new SavedObjectIndexStore(savedObjectsClient)}
redirectTo={id => {
if (!id) {
routeProps.history.push('/lens');
} else {
routeProps.history.push(`/lens/edit/${id}`);
}
}}
redirectTo={id => redirectTo(routeProps, addToDashboardMode, id)}
addToDashboardMode={addToDashboardMode}
/>
);
};
Expand All @@ -119,6 +155,7 @@ export class AppPlugin {
trackUiEvent('loaded_404');
return <FormattedMessage id="xpack.lens.app404" defaultMessage="404 Not Found" />;
}

render(
<I18nProvider>
<HashRouter>
Expand Down
Loading