Skip to content

Commit

Permalink
[Dashboard Navigation] Make locator generic (#167928)
Browse files Browse the repository at this point in the history
Closes #164748

## Summary

Previously, the link embeddable **always** used the Dashboard plugin's
locator - this meant that, for portable dashboards (such as those in
Security or in APM), navigation through the link embeddable would always
send the user to the Dashboard plugin rather than staying in whatever
context the portable dashboard was in. This PR fixes that by ensuring
that each `DashboardRenderer` consumer can provide their **own** locator
as a prop - this means that the Dashboard app can still use its own
locator, while the Security/APM plugins (for example) can **also**
define a locator so that navigation remains in the security/APM app.

### Security @elastic/security-solution-platform 

While I did my best to modify each portable dashboard implementation to
include this new locator prop, without the full context on how each
plugin wants to handle the link embeddable, I've had to make a few
assumptions about the behaviour. For example, in security....

- The Security dashboards **not** respond to the query bar in the same
way that they do in the Dashboard app - therefore, the link embeddable
settings for "Use filters and query from origin dashboard" and "Use date
range from origin dashboard" do not make sense in this context.

For example, in Security, it is (from my understanding) **expected**
that the query bar would always remain constant, regardless of these
settings; therefore, these settings are more-or-less ignored.
Unfortunately, this opens up a potential confusion for users, especially
if the are editing/creating a link embeddable in the security context.

- The Security app does not currently use real locators, and my attempts
to remedy this were unsuccessful. Therefore, rather than requiring a
**true** `Locator` object to be passed in as a prop to the
`DashboardRenderer`, I had to instead accept any object that has both a
`navigate` and `getRedirectUrl` method defined.

It would be **much** cleaner for the Security plugin to define its own
locator - that way, the `DashboardRenderer` could instead just accept a
locator ID as a prop, and the link embeddable could use that ID fetch
the appropriate locator as necessary; however, this is a pretty major
refactor, since the Security app handles URLs/navigation in a much
different way than any other Kibana app.


**Before:**


https://github.com/elastic/kibana/assets/8698078/67fdf34f-60e3-47fc-b205-8a6443a0452d

**After:**


https://github.com/elastic/kibana/assets/8698078/f92f1eb0-1467-4408-8792-f881e355188b

### APM @elastic/apm-ui 

While I did my best to modify each portable dashboard implementation to
include this new locator prop, without the full context on how each
plugin wants to handle the link embeddable, I've had to make a few
assumptions about the behaviour. For example, in APM....

- Similar to the Security implementation, the APM dashboards **not**
respond to the query bar in the same way that they do in the Dashboard
app - therefore, the link embeddable settings for "Use filters and query
from origin dashboard" and "Use date range from origin dashboard" do not
make sense in this context.

For example, in APM, it is (from my understanding) **expected** that the
query bar would always remain constant, regardless of these settings;
therefore, these settings are more-or-less ignored. That being said,
because APM does not currently support dashboard editing, I believe this
is less of a concern than it is for other implementations.

- Because of the unique content management system that APM is using,
where Dashboards must be linked in order for them to be viewed, the link
embeddable would not work **unless** the user was navigating to a
dashboard that was added to the APM Dashboard CM. I've had to change
this so that **any** dashboard can be viewed in the APM context - that
way, even if someone is trying to load a dashboard that is **not**
linked, it will still load.

This goes against how APM typically handles its dashboards, so I am open
to suggestions if this behaviour is undesirable. It just felt odd to me
that I would have to link every single referenced dashboard in the APM
context if I wanted the link embeddable to work....

**Before:**


https://github.com/elastic/kibana/assets/8698078/59397d42-2289-4721-9d4a-74c3e7a4d871

**After:**


https://github.com/elastic/kibana/assets/8698078/87924826-e766-4b50-87c6-132ae936f2fc


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
  • Loading branch information
Heenawter and kibanamachine authored Nov 6, 2023
1 parent 6ac15df commit a7728e4
Show file tree
Hide file tree
Showing 27 changed files with 396 additions and 305 deletions.
5 changes: 5 additions & 0 deletions src/plugins/dashboard/public/dashboard_app/dashboard_app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
} from './url/search_sessions_integration';
import { DashboardAPI, DashboardRenderer } from '..';
import { type DashboardEmbedSettings } from './types';
import { DASHBOARD_APP_LOCATOR } from './locator/locator';
import { pluginServices } from '../services/plugin_services';
import { AwaitingDashboardAPI } from '../dashboard_container';
import { DashboardRedirect } from '../dashboard_container/types';
Expand Down Expand Up @@ -82,6 +83,7 @@ export function DashboardApp({
settings: { uiSettings },
data: { search },
customBranding,
share: { url },
} = pluginServices.getServices();
const showPlainSpinner = useObservable(customBranding.hasCustomBranding$, false);
const { scopedHistory: getScopedHistory } = useDashboardMountContext();
Expand Down Expand Up @@ -188,6 +190,8 @@ export function DashboardApp({
return () => stopWatchingAppStateInUrl();
}, [dashboardAPI, kbnUrlStateStorage, savedDashboardId]);

const locator = useMemo(() => url?.locators.get(DASHBOARD_APP_LOCATOR), [url]);

return (
<>
{showNoDataPage && (
Expand All @@ -206,6 +210,7 @@ export function DashboardApp({
{getLegacyConflictWarning?.()}

<DashboardRenderer
locator={locator}
ref={setDashboardAPI}
dashboardRedirect={redirectTo}
savedObjectId={savedDashboardId}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,19 @@ import { isQuery, isTimeRange } from '@kbn/data-plugin/common';
import { Filter, isFilterPinned, Query, TimeRange } from '@kbn/es-query';
import { EmbeddableInput, IEmbeddable } from '@kbn/embeddable-plugin/public';
import { DashboardDrilldownOptions } from '@kbn/presentation-util-plugin/public';

import { DashboardAppLocatorParams } from './locator';
import { DashboardLocatorParams } from '../../dashboard_container';

interface EmbeddableQueryInput extends EmbeddableInput {
query?: Query;
filters?: Filter[];
timeRange?: TimeRange;
}

export const getEmbeddableParams = (
export const getDashboardLocatorParamsFromEmbeddable = (
source: IEmbeddable<EmbeddableQueryInput>,
options: DashboardDrilldownOptions
): Partial<DashboardAppLocatorParams> => {
const params: DashboardAppLocatorParams = {};
): Partial<DashboardLocatorParams> => {
const params: DashboardLocatorParams = {};

const input = source.getInput();
if (isQuery(input.query) && options.useCurrentFilters) {
Expand Down
55 changes: 5 additions & 50 deletions src/plugins/dashboard/public/dashboard_app/locator/locator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,11 @@ import { flow } from 'lodash';

import type { Filter } from '@kbn/es-query';
import { setStateToKbnUrl } from '@kbn/kibana-utils-plugin/public';
import { SerializableControlGroupInput } from '@kbn/controls-plugin/common';
import type { LocatorDefinition, LocatorPublic } from '@kbn/share-plugin/public';
import type { GlobalQueryStateFromUrl } from '@kbn/data-plugin/public';

import type { DashboardContainerInput } from '../../../common';
import { SavedDashboardPanel } from '../../../common/content_management';
import { DASHBOARD_APP_ID, SEARCH_SESSION_ID } from '../../dashboard_constants';
import { DashboardLocatorParams } from '../..';

/**
* Useful for ensuring that we don't pass any non-serializable values to history.push (for example, functions).
Expand All @@ -35,67 +33,24 @@ export const cleanEmptyKeys = (stateObj: Record<string, unknown>) => {

export const DASHBOARD_APP_LOCATOR = 'DASHBOARD_APP_LOCATOR';

export type DashboardAppLocatorParams = Partial<
Omit<
DashboardContainerInput,
'panels' | 'controlGroupInput' | 'executionContext' | 'isEmbeddedExternally'
>
> & {
/**
* If given, the dashboard saved object with this id will be loaded. If not given,
* a new, unsaved dashboard will be loaded up.
*/
dashboardId?: string;

/**
* If not given, will use the uiSettings configuration for `storeInSessionStorage`. useHash determines
* whether to hash the data in the url to avoid url length issues.
*/
useHash?: boolean;

/**
* When `true` filters from saved filters from destination dashboard as merged with applied filters
* When `false` applied filters take precedence and override saved filters
*
* true is default
*/
preserveSavedFilters?: boolean;

/**
* Search search session ID to restore.
* (Background search)
*/
searchSessionId?: string;

/**
* List of dashboard panels
*/
panels?: Array<SavedDashboardPanel & SerializableRecord>; // used SerializableRecord here to force the GridData type to be read as serializable

/**
* Control group input
*/
controlGroupInput?: SerializableControlGroupInput;
};

export type DashboardAppLocator = LocatorPublic<DashboardAppLocatorParams>;
export type DashboardAppLocator = LocatorPublic<DashboardLocatorParams>;

export interface DashboardAppLocatorDependencies {
useHashedUrl: boolean;
getDashboardFilterFields: (dashboardId: string) => Promise<Filter[]>;
}

export type ForwardedDashboardState = Omit<
DashboardAppLocatorParams,
DashboardLocatorParams,
'dashboardId' | 'preserveSavedFilters' | 'useHash' | 'searchSessionId'
>;

export class DashboardAppLocatorDefinition implements LocatorDefinition<DashboardAppLocatorParams> {
export class DashboardAppLocatorDefinition implements LocatorDefinition<DashboardLocatorParams> {
public readonly id = DASHBOARD_APP_LOCATOR;

constructor(protected readonly deps: DashboardAppLocatorDependencies) {}

public readonly getLocation = async (params: DashboardAppLocatorParams) => {
public readonly getLocation = async (params: DashboardLocatorParams) => {
const {
filters,
useHash: paramsUseHash,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
*/

import { Capabilities } from '@kbn/core/public';
import { DashboardLocatorParams } from '../../../dashboard_container';
import { convertPanelMapToSavedPanels, DashboardContainerInput } from '../../../../common';

import { DashboardAppLocatorParams } from '../../..';
import { pluginServices } from '../../../services/plugin_services';
import { showPublicUrlSwitch, ShowShareModal, ShowShareModalProps } from './show_share_modal';

Expand Down Expand Up @@ -56,7 +56,7 @@ describe('showPublicUrlSwitch', () => {

describe('ShowShareModal', () => {
const unsavedStateKeys = ['query', 'filters', 'options', 'savedQuery', 'panels'] as Array<
keyof DashboardAppLocatorParams
keyof DashboardLocatorParams
>;
const toggleShareMenuSpy = jest.spyOn(
pluginServices.getServices().share,
Expand All @@ -83,7 +83,7 @@ describe('ShowShareModal', () => {
expect(toggleShareMenuSpy).toHaveBeenCalledTimes(1);
const shareLocatorParams = (
toggleShareMenuSpy.mock.calls[0][0].sharingData as {
locatorParams: { params: DashboardAppLocatorParams };
locatorParams: { params: DashboardLocatorParams };
}
).locatorParams.params;
unsavedStateKeys.forEach((key) => {
Expand Down Expand Up @@ -125,7 +125,7 @@ describe('ShowShareModal', () => {
expect(toggleShareMenuSpy).toHaveBeenCalledTimes(1);
const shareLocatorParams = (
toggleShareMenuSpy.mock.calls[0][0].sharingData as {
locatorParams: { params: DashboardAppLocatorParams };
locatorParams: { params: DashboardLocatorParams };
}
).locatorParams.params;
const rawDashboardState = {
Expand All @@ -134,7 +134,7 @@ describe('ShowShareModal', () => {
};
unsavedStateKeys.forEach((key) => {
expect(shareLocatorParams[key]).toStrictEqual(
(rawDashboardState as unknown as Partial<DashboardAppLocatorParams>)[key]
(rawDashboardState as unknown as Partial<DashboardLocatorParams>)[key]
);
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ import { dashboardUrlParams } from '../../dashboard_router';
import { shareModalStrings } from '../../_dashboard_app_strings';
import { pluginServices } from '../../../services/plugin_services';
import { convertPanelMapToSavedPanels } from '../../../../common';
import { DashboardAppLocatorParams, DASHBOARD_APP_LOCATOR } from '../../locator/locator';
import { DASHBOARD_APP_LOCATOR } from '../../locator/locator';
import { DashboardLocatorParams } from '../../../dashboard_container';

const showFilterBarId = 'showFilterBar';

Expand Down Expand Up @@ -120,7 +121,7 @@ export function ShowShareModal({
);
};

let unsavedStateForLocator: DashboardAppLocatorParams = {};
let unsavedStateForLocator: DashboardLocatorParams = {};
const unsavedDashboardState = dashboardBackup.getState(savedObjectId);

if (unsavedDashboardState) {
Expand All @@ -131,7 +132,7 @@ export function ShowShareModal({
panels: unsavedDashboardState.panels
? (convertPanelMapToSavedPanels(
unsavedDashboardState.panels
) as DashboardAppLocatorParams['panels'])
) as DashboardLocatorParams['panels'])
: undefined,

// options
Expand All @@ -143,7 +144,7 @@ export function ShowShareModal({
};
}

const locatorParams: DashboardAppLocatorParams = {
const locatorParams: DashboardLocatorParams = {
dashboardId: savedObjectId,
preserveSavedFilters: true,
refreshInterval: undefined, // We don't share refresh interval externally
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ import type { Query } from '@kbn/es-query';
import { SearchSessionInfoProvider } from '@kbn/data-plugin/public';

import { SEARCH_SESSION_ID } from '../../dashboard_constants';
import { DashboardContainer } from '../../dashboard_container';
import { DashboardContainer, DashboardLocatorParams } from '../../dashboard_container';
import { convertPanelMapToSavedPanels } from '../../../common';
import { pluginServices } from '../../services/plugin_services';
import { DashboardAppLocatorParams, DASHBOARD_APP_LOCATOR } from '../locator/locator';
import { DASHBOARD_APP_LOCATOR } from '../locator/locator';

export const removeSearchSessionIdFromURL = (kbnUrlStateStorage: IKbnUrlStateStorage) => {
kbnUrlStateStorage.kbnUrlControls.updateAsync((nextUrl) => {
Expand All @@ -46,7 +46,7 @@ export const getSessionURLObservable = (history: History) =>

export function createSessionRestorationDataProvider(
container: DashboardContainer
): SearchSessionInfoProvider<DashboardAppLocatorParams> {
): SearchSessionInfoProvider<DashboardLocatorParams> {
return {
getName: async () => container.getTitle(),
getLocatorData: async () => ({
Expand All @@ -67,7 +67,7 @@ function getLocatorParams({
}: {
container: DashboardContainer;
shouldRestoreSearchSession: boolean;
}): DashboardAppLocatorParams {
}): DashboardLocatorParams {
const {
data: {
query: {
Expand Down Expand Up @@ -101,6 +101,6 @@ function getLocatorParams({
: undefined,
panels: lastSavedId
? undefined
: (convertPanelMapToSavedPanels(panels) as DashboardAppLocatorParams['panels']),
: (convertPanelMapToSavedPanels(panels) as DashboardLocatorParams['panels']),
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import { reportPerformanceMetricEvent } from '@kbn/ebt-tools';
import { KibanaThemeProvider } from '@kbn/kibana-react-plugin/public';
import type { ControlGroupContainer } from '@kbn/controls-plugin/public';
import type { KibanaExecutionContext, OverlayRef } from '@kbn/core/public';
import { LocatorPublic } from '@kbn/share-plugin/common';
import { ExitFullScreenButtonKibanaProvider } from '@kbn/shared-ux-button-exit-full-screen';
import { ReduxToolsPackage, ReduxEmbeddableTools } from '@kbn/presentation-util-plugin/public';

Expand All @@ -49,13 +50,13 @@ import {
DashboardReduxState,
DashboardRenderPerformanceStats,
} from '../types';
import { DASHBOARD_CONTAINER_TYPE } from '../..';
import { placePanel } from '../component/panel_placement';
import { pluginServices } from '../../services/plugin_services';
import { initializeDashboard } from './create/create_dashboard';
import { DASHBOARD_APP_ID, DASHBOARD_LOADED_EVENT } from '../../dashboard_constants';
import { DashboardCreationOptions } from './dashboard_container_factory';
import { DashboardAnalyticsService } from '../../services/analytics/types';
import { DashboardLocatorParams, DASHBOARD_CONTAINER_TYPE } from '../..';
import { DashboardViewport } from '../component/viewport/dashboard_viewport';
import { DashboardPanelState, DashboardContainerInput } from '../../../common';
import { dashboardContainerReducers } from '../state/dashboard_container_reducers';
Expand Down Expand Up @@ -107,6 +108,8 @@ export class DashboardContainer extends Container<InheritedChildInput, Dashboard
public controlGroup?: ControlGroupContainer;

public searchSessionId?: string;
public locator?: Pick<LocatorPublic<DashboardLocatorParams>, 'navigate' | 'getRedirectUrl'>;

// cleanup
public stopSyncingWithUnifiedSearch?: () => void;
private cleanupStateTools: () => void;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,48 +8,50 @@

import '../_dashboard_container.scss';

import classNames from 'classnames';
import React, {
useRef,
useMemo,
useState,
useEffect,
forwardRef,
useEffect,
useImperativeHandle,
useLayoutEffect,
useMemo,
useRef,
useState,
} from 'react';
import { v4 as uuidv4 } from 'uuid';
import classNames from 'classnames';
import useUnmount from 'react-use/lib/useUnmount';
import { v4 as uuidv4 } from 'uuid';

import { EuiLoadingElastic, EuiLoadingSpinner } from '@elastic/eui';
import { SavedObjectNotFound } from '@kbn/kibana-utils-plugin/common';
import { ErrorEmbeddable, isErrorEmbeddable } from '@kbn/embeddable-plugin/public';
import { SavedObjectNotFound } from '@kbn/kibana-utils-plugin/common';

import { LocatorPublic } from '@kbn/share-plugin/common';
import { DASHBOARD_CONTAINER_TYPE } from '..';
import { DashboardContainerInput } from '../../../common';
import type { DashboardContainer } from '../embeddable/dashboard_container';
import {
DashboardAPI,
AwaitingDashboardAPI,
buildApiFromDashboardContainer,
} from './dashboard_api';
import {
DashboardCreationOptions,
DashboardContainerFactory,
DashboardContainerFactoryDefinition,
DashboardCreationOptions,
} from '../embeddable/dashboard_container_factory';
import { DashboardRedirect } from '../types';
import { DASHBOARD_CONTAINER_TYPE } from '..';
import { DashboardContainerInput } from '../../../common';
import type { DashboardContainer } from '../embeddable/dashboard_container';
import { DashboardLocatorParams, DashboardRedirect } from '../types';
import { Dashboard404Page } from './dashboard_404';
import {
AwaitingDashboardAPI,
buildApiFromDashboardContainer,
DashboardAPI,
} from './dashboard_api';

export interface DashboardRendererProps {
savedObjectId?: string;
showPlainSpinner?: boolean;
dashboardRedirect?: DashboardRedirect;
getCreationOptions?: () => Promise<DashboardCreationOptions>;
locator?: Pick<LocatorPublic<DashboardLocatorParams>, 'navigate' | 'getRedirectUrl'>;
}

export const DashboardRenderer = forwardRef<AwaitingDashboardAPI, DashboardRendererProps>(
({ savedObjectId, getCreationOptions, dashboardRedirect, showPlainSpinner }, ref) => {
({ savedObjectId, getCreationOptions, dashboardRedirect, showPlainSpinner, locator }, ref) => {
const dashboardRoot = useRef(null);
const dashboardViewport = useRef(null);
const [loading, setLoading] = useState(true);
Expand Down Expand Up @@ -77,6 +79,11 @@ export const DashboardRenderer = forwardRef<AwaitingDashboardAPI, DashboardRende

const id = useMemo(() => uuidv4(), []);

useEffect(() => {
/* In case the locator prop changes, we need to reassign the value in the container */
if (dashboardContainer) dashboardContainer.locator = locator;
}, [dashboardContainer, locator]);

useEffect(() => {
/**
* Here we attempt to build a dashboard or navigate to a new dashboard. Clear all error states
Expand Down
1 change: 1 addition & 0 deletions src/plugins/dashboard/public/dashboard_container/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,4 @@ export {

export { DashboardRenderer } from './external_api/dashboard_renderer';
export type { DashboardAPI, AwaitingDashboardAPI } from './external_api/dashboard_api';
export type { DashboardLocatorParams } from './types';
Loading

0 comments on commit a7728e4

Please sign in to comment.