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

[Drilldowns] import dashboard url generator from plugin contract #64628

Merged
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
2 changes: 1 addition & 1 deletion src/plugins/dashboard/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export {
} from './application';
export { DashboardConstants, createDashboardEditUrl } from './dashboard_constants';

export { DashboardStart } from './plugin';
export { DashboardStart, DashboardUrlGenerator } from './plugin';
export { DASHBOARD_APP_URL_GENERATOR } from './url_generator';

export function plugin(initializerContext: PluginInitializerContext) {
Expand Down
19 changes: 15 additions & 4 deletions src/plugins/dashboard/public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ import {
DataPublicPluginSetup,
esFilters,
} from '../../../plugins/data/public';
import { SharePluginSetup, SharePluginStart } from '../../../plugins/share/public';
import {
SharePluginSetup,
SharePluginStart,
UrlGeneratorContract,
} from '../../../plugins/share/public';
import { UiActionsSetup, UiActionsStart } from '../../../plugins/ui_actions/public';

import { Start as InspectorStartContract } from '../../../plugins/inspector/public';
Expand Down Expand Up @@ -77,7 +81,7 @@ import {
import {
DashboardAppLinkGeneratorState,
DASHBOARD_APP_URL_GENERATOR,
createDirectAccessDashboardLinkGenerator,
createDashboardUrlGenerator,
} from './url_generator';
import { createSavedDashboardLoader } from './saved_dashboards';
import { DashboardConstants } from './dashboard_constants';
Expand All @@ -89,6 +93,8 @@ declare module '../../share/public' {
}
}

export type DashboardUrlGenerator = UrlGeneratorContract<typeof DASHBOARD_APP_URL_GENERATOR>;

interface SetupDependencies {
data: DataPublicPluginSetup;
embeddable: EmbeddableSetup;
Expand All @@ -111,8 +117,10 @@ interface StartDependencies {
}

export type Setup = void;

export interface DashboardStart {
getSavedDashboardLoader: () => SavedObjectLoader;
dashboardUrlGenerator?: DashboardUrlGenerator;
}

declare module '../../../plugins/ui_actions/public' {
Expand All @@ -130,6 +138,8 @@ export class DashboardPlugin
private appStateUpdater = new BehaviorSubject<AngularRenderedAppUpdater>(() => ({}));
private stopUrlTracking: (() => void) | undefined = undefined;

private dashboardUrlGenerator?: DashboardUrlGenerator;

public setup(
core: CoreSetup<StartDependencies, DashboardStart>,
{ share, uiActions, embeddable, home, kibanaLegacy, data, usageCollection }: SetupDependencies
Expand All @@ -140,8 +150,8 @@ export class DashboardPlugin
const startServices = core.getStartServices();

if (share) {
share.urlGenerators.registerUrlGenerator(
createDirectAccessDashboardLinkGenerator(async () => {
this.dashboardUrlGenerator = share.urlGenerators.registerUrlGenerator(
createDashboardUrlGenerator(async () => {
const [coreStart, , selfStart] = await startServices;
return {
appBasePath: coreStart.application.getUrlForApp('dashboard'),
Expand Down Expand Up @@ -325,6 +335,7 @@ export class DashboardPlugin
});
return {
getSavedDashboardLoader: () => savedDashboardLoader,
dashboardUrlGenerator: this.dashboardUrlGenerator,
};
}

Expand Down
24 changes: 12 additions & 12 deletions src/plugins/dashboard/public/url_generator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { createDirectAccessDashboardLinkGenerator } from './url_generator';
import { createDashboardUrlGenerator } from './url_generator';
import { hashedItemStore } from '../../kibana_utils/public';
// eslint-disable-next-line
import { mockStorage } from '../../kibana_utils/public/storage/hashed_item_store/mock';
Expand Down Expand Up @@ -55,7 +55,7 @@ describe('dashboard url generator', () => {
});

test('creates a link to a saved dashboard', async () => {
const generator = createDirectAccessDashboardLinkGenerator(() =>
const generator = createDashboardUrlGenerator(() =>
Promise.resolve({
appBasePath: APP_BASE_PATH,
useHashedUrl: false,
Expand All @@ -67,7 +67,7 @@ describe('dashboard url generator', () => {
});

test('creates a link with global time range set up', async () => {
const generator = createDirectAccessDashboardLinkGenerator(() =>
const generator = createDashboardUrlGenerator(() =>
Promise.resolve({
appBasePath: APP_BASE_PATH,
useHashedUrl: false,
Expand All @@ -83,7 +83,7 @@ describe('dashboard url generator', () => {
});

test('creates a link with filters, time range, refresh interval and query to a saved object', async () => {
const generator = createDirectAccessDashboardLinkGenerator(() =>
const generator = createDashboardUrlGenerator(() =>
Promise.resolve({
appBasePath: APP_BASE_PATH,
useHashedUrl: false,
Expand Down Expand Up @@ -123,7 +123,7 @@ describe('dashboard url generator', () => {
});

test('if no useHash setting is given, uses the one was start services', async () => {
const generator = createDirectAccessDashboardLinkGenerator(() =>
const generator = createDashboardUrlGenerator(() =>
Promise.resolve({
appBasePath: APP_BASE_PATH,
useHashedUrl: true,
Expand All @@ -137,7 +137,7 @@ describe('dashboard url generator', () => {
});

test('can override a false useHash ui setting', async () => {
const generator = createDirectAccessDashboardLinkGenerator(() =>
const generator = createDashboardUrlGenerator(() =>
Promise.resolve({
appBasePath: APP_BASE_PATH,
useHashedUrl: false,
Expand All @@ -152,7 +152,7 @@ describe('dashboard url generator', () => {
});

test('can override a true useHash ui setting', async () => {
const generator = createDirectAccessDashboardLinkGenerator(() =>
const generator = createDashboardUrlGenerator(() =>
Promise.resolve({
appBasePath: APP_BASE_PATH,
useHashedUrl: true,
Expand Down Expand Up @@ -195,7 +195,7 @@ describe('dashboard url generator', () => {
};

test('attaches filters from destination dashboard', async () => {
const generator = createDirectAccessDashboardLinkGenerator(() =>
const generator = createDashboardUrlGenerator(() =>
Promise.resolve({
appBasePath: APP_BASE_PATH,
useHashedUrl: false,
Expand Down Expand Up @@ -224,7 +224,7 @@ describe('dashboard url generator', () => {
});

test("doesn't fail if can't retrieve filters from destination dashboard", async () => {
const generator = createDirectAccessDashboardLinkGenerator(() =>
const generator = createDashboardUrlGenerator(() =>
Promise.resolve({
appBasePath: APP_BASE_PATH,
useHashedUrl: false,
Expand All @@ -246,7 +246,7 @@ describe('dashboard url generator', () => {
});

test('can enforce empty filters', async () => {
const generator = createDirectAccessDashboardLinkGenerator(() =>
const generator = createDashboardUrlGenerator(() =>
Promise.resolve({
appBasePath: APP_BASE_PATH,
useHashedUrl: false,
Expand All @@ -270,7 +270,7 @@ describe('dashboard url generator', () => {
});

test('no filters in result url if no filters applied', async () => {
const generator = createDirectAccessDashboardLinkGenerator(() =>
const generator = createDashboardUrlGenerator(() =>
Promise.resolve({
appBasePath: APP_BASE_PATH,
useHashedUrl: false,
Expand All @@ -288,7 +288,7 @@ describe('dashboard url generator', () => {
});

test('can turn off preserving filters', async () => {
const generator = createDirectAccessDashboardLinkGenerator(() =>
const generator = createDashboardUrlGenerator(() =>
Promise.resolve({
appBasePath: APP_BASE_PATH,
useHashedUrl: false,
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/dashboard/public/url_generator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export type DashboardAppLinkGeneratorState = UrlGeneratorState<{
preserveSavedFilters?: boolean;
}>;

export const createDirectAccessDashboardLinkGenerator = (
export const createDashboardUrlGenerator = (
getStartServices: () => Promise<{
appBasePath: string;
useHashedUrl: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ test('Asking for a generator that does not exist throws an error', () => {
});

test('Registering and retrieving a generator', async () => {
setup.registerUrlGenerator({
const generator = setup.registerUrlGenerator({
id: 'TEST_GENERATOR',
createUrl: () => Promise.resolve('myurl'),
});
const generator = start.getUrlGenerator('TEST_GENERATOR');

expect(generator).toMatchInlineSnapshot(`
Object {
"createUrl": [Function],
Expand All @@ -47,6 +47,20 @@ test('Registering and retrieving a generator', async () => {
new Error('You cannot call migrate on a non-deprecated generator.')
);
expect(await generator.createUrl({})).toBe('myurl');

const retrievedGenerator = start.getUrlGenerator('TEST_GENERATOR');
expect(retrievedGenerator).toMatchInlineSnapshot(`
Object {
"createUrl": [Function],
"id": "TEST_GENERATOR",
"isDeprecated": false,
"migrate": [Function],
}
`);
await expect(generator.migrate({})).rejects.toEqual(
new Error('You cannot call migrate on a non-deprecated generator.')
);
expect(await generator.createUrl({})).toBe('myurl');
});

test('Registering a generator with a createUrl function that is deprecated throws an error', () => {
Expand Down
11 changes: 6 additions & 5 deletions src/plugins/share/public/url_generators/url_generator_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ export interface UrlGeneratorsStart {
}

export interface UrlGeneratorsSetup {
registerUrlGenerator: <Id extends UrlGeneratorId>(generator: UrlGeneratorsDefinition<Id>) => void;
registerUrlGenerator: <Id extends UrlGeneratorId>(
generator: UrlGeneratorsDefinition<Id>
) => UrlGeneratorContract<Id>;
}

export class UrlGeneratorsService implements Plugin<UrlGeneratorsSetup, UrlGeneratorsStart> {
Expand All @@ -43,10 +45,9 @@ export class UrlGeneratorsService implements Plugin<UrlGeneratorsSetup, UrlGener
registerUrlGenerator: <Id extends UrlGeneratorId>(
generatorOptions: UrlGeneratorsDefinition<Id>
) => {
this.urlGenerators.set(
generatorOptions.id,
new UrlGeneratorInternal<Id>(generatorOptions, this.getUrlGenerator)
);
const generator = new UrlGeneratorInternal<Id>(generatorOptions, this.getUrlGenerator);
this.urlGenerators.set(generatorOptions.id, generator);
return generator.getPublicContract();
},
};
return setup;
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/dashboard_enhanced/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { DashboardDrilldownsService } from './services';
import { DataPublicPluginStart } from '../../../../src/plugins/data/public';
import { AdvancedUiActionsSetup, AdvancedUiActionsStart } from '../../advanced_ui_actions/public';
import { DrilldownsSetup, DrilldownsStart } from '../../drilldowns/public';
import { DashboardStart } from '../../../../src/plugins/dashboard/public';

export interface SetupDependencies {
advancedUiActions: AdvancedUiActionsSetup;
Expand All @@ -25,6 +26,7 @@ export interface StartDependencies {
drilldowns: DrilldownsStart;
embeddable: EmbeddableStart;
share: SharePluginStart;
dashboard: DashboardStart;
}

// eslint-disable-next-line
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,23 @@ export class DashboardDrilldownsService {
{ advancedUiActions: uiActions }: SetupDependencies
) {
const start = createStartServicesGetter(core.getStartServices);
const getDashboardUrlGenerator = () => {
const urlGenerator = start().plugins.dashboard.dashboardUrlGenerator;
if (!urlGenerator)
throw new Error('dashboardUrlGenerator is required for dashboard to dashboard drilldown');
Copy link
Contributor

Choose a reason for hiding this comment

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

This message contains error message specific for drilldowns.

... required for dashboard to dashboard drilldown

When somebody else in this plugin will start using the dashboard URL generator, this error message will not make sense for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it is inside /dashboard_drilldowns_services.ts 🤔

return urlGenerator;
};

const actionFlyoutCreateDrilldown = new FlyoutCreateDrilldownAction({ start });
uiActions.addTriggerAction(CONTEXT_MENU_TRIGGER, actionFlyoutCreateDrilldown);

const actionFlyoutEditDrilldown = new FlyoutEditDrilldownAction({ start });
uiActions.addTriggerAction(CONTEXT_MENU_TRIGGER, actionFlyoutEditDrilldown);

const dashboardToDashboardDrilldown = new DashboardToDashboardDrilldown({ start });
const dashboardToDashboardDrilldown = new DashboardToDashboardDrilldown({
start,
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 this is a move in the wrong direction. While it's very easy to just pass down start it encourages tight coupling to core. See https://www.notion.so/Tight-Coupling-in-Kibana-fcee0454319140d39aa5e83052bba901

Copy link
Contributor

Choose a reason for hiding this comment

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

Had a similar comment in a PR from Vadim, we can move this discussion outside this PR. Don't let it block you.

getDashboardUrlGenerator,
});
uiActions.registerDrilldown(dashboardToDashboardDrilldown);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
*/

import { DashboardToDashboardDrilldown } from './drilldown';
import { UrlGeneratorContract } from '../../../../../../../src/plugins/share/public';
import { savedObjectsServiceMock } from '../../../../../../../src/core/public/mocks';
import { savedObjectsServiceMock, coreMock } from '../../../../../../../src/core/public/mocks';
import { dataPluginMock } from '../../../../../../../src/plugins/data/public/mocks';
import { ActionContext, Config } from './types';
import {
Expand All @@ -19,15 +18,16 @@ import {
import { esFilters } from '../../../../../../../src/plugins/data/public';

// convenient to use real implementation here.
import { createDirectAccessDashboardLinkGenerator } from '../../../../../../../src/plugins/dashboard/public/url_generator';
import { createDashboardUrlGenerator } from '../../../../../../../src/plugins/dashboard/public/url_generator';
import { UrlGeneratorsService } from '../../../../../../../src/plugins/share/public/url_generators';
import { VisualizeEmbeddableContract } from '../../../../../../../src/plugins/visualizations/public';
import {
RangeSelectTriggerContext,
ValueClickTriggerContext,
} from '../../../../../../../src/plugins/embeddable/public';
import { StartDependencies } from '../../../plugin';
import { SavedObjectLoader } from '../../../../../../../src/plugins/saved_objects/public';
import { StartServicesGetter } from '../../../../../../../src/plugins/kibana_utils/public/core';
import { StartDependencies } from '../../../plugin';

describe('.isConfigValid()', () => {
const drilldown = new DashboardToDashboardDrilldown({} as any);
Expand Down Expand Up @@ -105,23 +105,19 @@ describe('.execute() & getHref', () => {
data: {
actions: dataPluginActions,
},
share: {
urlGenerators: {
getUrlGenerator: () =>
createDirectAccessDashboardLinkGenerator(() =>
Promise.resolve({
appBasePath: 'test',
useHashedUrl: false,
savedDashboardLoader: ({} as unknown) as SavedObjectLoader,
})
) as UrlGeneratorContract<string>,
},
},
},
self: {},
})) as unknown) as StartServicesGetter<
Pick<StartDependencies, 'data' | 'advancedUiActions' | 'share'>
>,
})) as unknown) as StartServicesGetter<Pick<StartDependencies, 'data' | 'advancedUiActions'>>,
getDashboardUrlGenerator: () =>
new UrlGeneratorsService().setup(coreMock.createSetup()).registerUrlGenerator(
createDashboardUrlGenerator(() =>
Promise.resolve({
appBasePath: 'test',
useHashedUrl: false,
savedDashboardLoader: ({} as unknown) as SavedObjectLoader,
})
)
),
});
const selectRangeFiltersSpy = jest
.spyOn(dataPluginActions, 'createFiltersFromRangeSelectAction')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import React from 'react';
import { reactToUiComponent } from '../../../../../../../src/plugins/kibana_react/public';
import { DASHBOARD_APP_URL_GENERATOR } from '../../../../../../../src/plugins/dashboard/public';
import { DashboardUrlGenerator } from '../../../../../../../src/plugins/dashboard/public';
import { ActionContext, Config } from './types';
import { CollectConfigContainer } from './components';
import { DASHBOARD_TO_DASHBOARD_DRILLDOWN } from './constants';
Expand All @@ -22,7 +22,8 @@ import { StartServicesGetter } from '../../../../../../../src/plugins/kibana_uti
import { StartDependencies } from '../../../plugin';

export interface Params {
start: StartServicesGetter<Pick<StartDependencies, 'data' | 'advancedUiActions' | 'share'>>;
start: StartServicesGetter<Pick<StartDependencies, 'data' | 'advancedUiActions'>>;
getDashboardUrlGenerator: () => DashboardUrlGenerator;
}

export class DashboardToDashboardDrilldown
Expand Down Expand Up @@ -142,9 +143,7 @@ export class DashboardToDashboardDrilldown
}
}

const { plugins } = this.params.start();

return plugins.share.urlGenerators.getUrlGenerator(DASHBOARD_APP_URL_GENERATOR).createUrl({
return this.params.getDashboardUrlGenerator().createUrl({
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks much cleaner!

dashboardId: config.dashboardId,
query: config.useCurrentFilters ? query : undefined,
timeRange,
Expand Down