Skip to content

Commit

Permalink
[Security Solution][Lens] Check filter action type to hide default fi…
Browse files Browse the repository at this point in the history
…lter actions in visualizations (#171284)

## Summary

Bug: #171167

The [previous
implementation](#170127) solved a
different bug using a new `shouldShowLegendAction` property. This
approach had a limitation on the Security Dashboards page since the
Security app has no control over the properties passed to the
visualization components when they are rendered through portable
Dashboards.

This PR fixes the problem by checking if any of the registered actions
is a "filter" action `type` in the visualizations. If customized filter
actions are found, the default filter actions hardcoded in the
visualizations code are not added, preventing duplication of filter
actions.

The specific action `type` used for the check is the
`FILTER_CELL_ACTION_TYPE = 'cellAction-filter'` constant exported by the
`@kbn/cell-actions` package.

This new approach uses a property stored in the registered actions
themselves, so we don't need to pass any extra property to the
visualization components, it works transparently. So the
`shouldShowLegendAction` property has also been cleaned.

## Demos

Timeline using `showTopN` visualization:


https://github.com/elastic/kibana/assets/17747913/491c08e8-0740-4c9e-9cb7-81267b9ba040


Alerts page using `Counts` table visualization and `showTopN`
visualization


https://github.com/elastic/kibana/assets/17747913/683eec6c-382e-4ae9-9400-c1022922e488


Portable Dashboard visualizations:


https://github.com/elastic/kibana/assets/17747913/50f09a65-488d-41f2-a5b8-3882d9c80678


Security actions are "compatible" only inside the Security app, in the
Lens app the default filter actions are displayed:

<img width="1682" alt="lens-actions-screenshot"
src="https://github.com/elastic/kibana/assets/17747913/1e7ce929-129d-45a9-ba71-8d28e3726454">

---------

Co-authored-by: kibanamachine <[email protected]>
  • Loading branch information
semd and kibanamachine authored Nov 20, 2023
1 parent 4871162 commit 9bf7e38
Show file tree
Hide file tree
Showing 40 changed files with 128 additions and 261 deletions.
9 changes: 9 additions & 0 deletions packages/kbn-cell-actions/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

export * from './src/constants';
2 changes: 1 addition & 1 deletion packages/kbn-cell-actions/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export type {
export type { UseDataGridColumnsCellActions, UseDataGridColumnsCellActionsProps } from './hooks';

// Constants
export { CellActionsMode, FILTER_CELL_ACTION_TYPE, COPY_CELL_ACTION_TYPE } from './constants';
export { CellActionsMode } from './constants';

// Components and hooks
export { CellActionsProvider } from './context';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export interface MultiFilterEvent {

export interface CellValueAction {
id: string;
type?: string;
iconType: string;
displayName: string;
execute: (data: CellValueContext['data']) => void;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,15 @@ import { LegendAction, SeriesIdentifier, useLegendAction } from '@elastic/charts
import { DataPublicPluginStart } from '@kbn/data-plugin/public';
import { Datatable } from '@kbn/expressions-plugin/public';
import { FieldFormatsStart } from '@kbn/field-formats-plugin/public';
import { FILTER_CELL_ACTION_TYPE } from '@kbn/cell-actions/constants';
import { PartitionVisParams } from '../../common/types';
import { ColumnCellValueActions, FilterEvent } from '../types';
import { CellValueAction, ColumnCellValueActions, FilterEvent } from '../types';
import { getSeriesValueColumnIndex, getFilterPopoverTitle } from './filter_helpers';

const hasFilterCellAction = (actions: CellValueAction[]) => {
return actions.some(({ type }) => type === FILTER_CELL_ACTION_TYPE);
};

export const getLegendActions = (
canFilter: (
data: FilterEvent | null,
Expand Down Expand Up @@ -58,9 +63,10 @@ export const getLegendActions = (
pieSeries.key
);

const panelItems: EuiContextMenuPanelDescriptor['items'] = [];
const compatibleCellActions = columnCellValueActions[columnIndex] ?? [];

if (isFilterable && filterData) {
const panelItems: EuiContextMenuPanelDescriptor['items'] = [];
if (!hasFilterCellAction(compatibleCellActions) && isFilterable && filterData) {
panelItems.push(
{
name: i18n.translate('expressionPartitionVis.legend.filterForValueButtonAriaLabel', {
Expand All @@ -87,20 +93,18 @@ export const getLegendActions = (
);
}

if (columnCellValueActions[columnIndex]) {
const columnMeta = visData.columns[columnIndex].meta;
columnCellValueActions[columnIndex].forEach((action) => {
panelItems.push({
name: action.displayName,
'data-test-subj': `legend-${title}-${action.id}`,
icon: <EuiIcon type={action.iconType} size="m" />,
onClick: () => {
action.execute([{ columnMeta, value: pieSeries.key }]);
setPopoverOpen(false);
},
});
const columnMeta = visData.columns[columnIndex].meta;
compatibleCellActions.forEach((action) => {
panelItems.push({
name: action.displayName,
'data-test-subj': `legend-${title}-${action.id}`,
icon: <EuiIcon type={action.iconType} size="m" />,
onClick: () => {
action.execute([{ columnMeta, value: pieSeries.key }]);
setPopoverOpen(false);
},
});
}
});

if (panelItems.length === 0) {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"@kbn/analytics",
"@kbn/chart-icons",
"@kbn/chart-expressions-common",
"@kbn/cell-actions",
],
"exclude": [
"target/**/*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { layeredXyVisFunction } from '.';
import { createMockExecutionContext } from '@kbn/expressions-plugin/common/mocks';
import { sampleArgs, sampleExtendedLayer } from '../__mocks__';
import { XY_VIS } from '../constants';
import { shouldShowLegendActionDefault } from '../helpers/visualization';

describe('layeredXyVis', () => {
test('it renders with the specified data and args', async () => {
Expand All @@ -31,7 +30,6 @@ describe('layeredXyVis', () => {
syncTooltips: false,
syncCursor: true,
canNavigateToLens: false,
shouldShowLegendAction: shouldShowLegendActionDefault,
},
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {
validateAxes,
} from './validate';
import { appendLayerIds, getDataLayers } from '../helpers';
import { shouldShowLegendActionDefault } from '../helpers/visualization';

export const layeredXyVisFn: LayeredXyVisFn['fn'] = async (data, args, handlers) => {
const layers = appendLayerIds(args.layers ?? [], 'layers');
Expand Down Expand Up @@ -67,7 +66,6 @@ export const layeredXyVisFn: LayeredXyVisFn['fn'] = async (data, args, handlers)
syncTooltips: handlers?.isSyncTooltipsEnabled?.() ?? false,
syncCursor: handlers?.isSyncCursorEnabled?.() ?? true,
overrides: handlers.variables?.overrides as XYRender['value']['overrides'],
shouldShowLegendAction: handlers?.shouldShowLegendAction ?? shouldShowLegendActionDefault,
},
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { xyVisFunction } from '.';
import { createMockExecutionContext } from '@kbn/expressions-plugin/common/mocks';
import { sampleArgs, sampleLayer } from '../__mocks__';
import { XY_VIS } from '../constants';
import { shouldShowLegendActionDefault } from '../helpers/visualization';

describe('xyVis', () => {
test('it renders with the specified data and args', async () => {
Expand Down Expand Up @@ -43,7 +42,6 @@ describe('xyVis', () => {
syncColors: false,
syncTooltips: false,
syncCursor: true,
shouldShowLegendAction: shouldShowLegendActionDefault,
},
});
});
Expand Down Expand Up @@ -354,7 +352,6 @@ describe('xyVis', () => {
syncColors: false,
syncTooltips: false,
syncCursor: true,
shouldShowLegendAction: shouldShowLegendActionDefault,
},
});
});
Expand Down Expand Up @@ -404,7 +401,6 @@ describe('xyVis', () => {
syncTooltips: false,
syncCursor: true,
overrides,
shouldShowLegendAction: shouldShowLegendActionDefault,
},
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import {
validateAxes,
} from './validate';
import { logDatatable } from '../utils';
import { shouldShowLegendActionDefault } from '../helpers/visualization';

const createDataLayer = (args: XYArgs, table: Datatable): DataLayerConfigResult => {
const accessors = getAccessors<string | ExpressionValueVisDimension, XYArgs>(args, table);
Expand Down Expand Up @@ -140,7 +139,6 @@ export const xyVisFn: XyVisFn['fn'] = async (data, args, handlers) => {
syncTooltips: handlers?.isSyncTooltipsEnabled?.() ?? false,
syncCursor: handlers?.isSyncCursorEnabled?.() ?? true,
overrides: handlers.variables?.overrides as XYRender['value']['overrides'],
shouldShowLegendAction: handlers?.shouldShowLegendAction ?? shouldShowLegendActionDefault,
},
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,3 @@ export function isTimeChart(layers: CommonXYDataLayerConfigResult[]) {
(!l.xScaleType || l.xScaleType === XScaleTypes.TIME)
);
}

export const shouldShowLegendActionDefault = () => true;
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,7 @@ describe('getLegendAction', function () {
formattedColumns: {},
},
},
{},
() => true
{}
);
let wrapper: ReactWrapper<LegendActionProps>;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ export const getLegendAction = (
fieldFormats: LayersFieldFormats,
formattedDatatables: DatatablesWithFormatInfo,
titles: LayersAccessorsTitles,
shouldShowLegendAction?: (actionId: string) => boolean,
singleTable?: boolean
): LegendAction =>
React.memo(({ series: [xySeries] }) => {
Expand Down Expand Up @@ -110,7 +109,6 @@ export const getLegendAction = (
}
onFilter={filterHandler}
legendCellValueActions={legendCellValueActions}
shouldShowLegendAction={shouldShowLegendAction}
/>
);
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,14 @@

import React, { useState, useMemo } from 'react';
import { i18n } from '@kbn/i18n';
import {
EuiContextMenuPanelDescriptor,
EuiIcon,
EuiPopover,
EuiContextMenu,
EuiContextMenuPanelItemDescriptor,
} from '@elastic/eui';
import { FILTER_CELL_ACTION_TYPE } from '@kbn/cell-actions/constants';
import { EuiContextMenuPanelDescriptor, EuiIcon, EuiPopover, EuiContextMenu } from '@elastic/eui';
import { useLegendAction } from '@elastic/charts';
import type { CellValueAction } from '../types';
import { shouldShowLegendActionDefault } from '../../common/helpers/visualization';

const hasFilterCellAction = (actions: CellValueAction[]) => {
return actions.some(({ type }) => type === FILTER_CELL_ACTION_TYPE);
};

export type LegendCellValueActions = Array<
Omit<CellValueAction, 'execute'> & { execute: () => void }
Expand All @@ -36,20 +34,18 @@ export interface LegendActionPopoverProps {
* Compatible actions to be added to the popover actions
*/
legendCellValueActions?: LegendCellValueActions;
shouldShowLegendAction?: (actionId: string) => boolean;
}

export const LegendActionPopover: React.FunctionComponent<LegendActionPopoverProps> = ({
label,
onFilter,
legendCellValueActions = [],
shouldShowLegendAction = shouldShowLegendActionDefault,
}) => {
const [popoverOpen, setPopoverOpen] = useState(false);
const [ref, onClose] = useLegendAction<HTMLDivElement>();

const panels: EuiContextMenuPanelDescriptor[] = useMemo(() => {
const defaultActions = [
const defaultFilterActions = [
{
id: 'filterIn',
displayName: i18n.translate('expressionXY.legend.filterForValueButtonAriaLabel', {
Expand All @@ -76,30 +72,29 @@ export const LegendActionPopover: React.FunctionComponent<LegendActionPopoverPro
},
];

const legendCellValueActionPanelItems = [...defaultActions, ...legendCellValueActions].reduce<
EuiContextMenuPanelItemDescriptor[]
>((acc, action) => {
if (shouldShowLegendAction(action.id)) {
acc.push({
name: action.displayName,
'data-test-subj': `legend-${label}-${action.id}`,
icon: <EuiIcon type={action.iconType} size="m" />,
onClick: () => {
action.execute();
setPopoverOpen(false);
},
});
}
return acc;
}, []);
const allActions = [
...(!hasFilterCellAction(legendCellValueActions) ? defaultFilterActions : []),
...legendCellValueActions,
];

const legendCellValueActionPanelItems = allActions.map((action) => ({
name: action.displayName,
'data-test-subj': `legend-${label}-${action.id}`,
icon: <EuiIcon type={action.iconType} size="m" />,
onClick: () => {
action.execute();
setPopoverOpen(false);
},
}));

return [
{
id: 'main',
title: label,
items: legendCellValueActionPanelItems,
},
];
}, [label, legendCellValueActions, onFilter, shouldShowLegendAction]);
}, [label, legendCellValueActions, onFilter]);

const Button = (
<div
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ export type XYChartRenderProps = Omit<XYChartProps, 'canNavigateToLens'> & {
renderComplete: () => void;
uiState?: PersistedState;
timeFormat: string;
shouldShowLegendAction?: (actionId: string) => boolean;
};

function nonNullable<T>(v: T): v is NonNullable<T> {
Expand Down Expand Up @@ -208,7 +207,6 @@ export function XYChart({
uiState,
timeFormat,
overrides,
shouldShowLegendAction,
}: XYChartRenderProps) {
const {
legend,
Expand Down Expand Up @@ -841,7 +839,6 @@ export function XYChart({
fieldFormats,
formattedDatatables,
titles,
shouldShowLegendAction,
singleTable
)
: undefined
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,6 @@ export const getXyChartRenderer = ({
syncCursor={config.syncCursor}
uiState={handlers.uiState as PersistedState}
renderComplete={renderComplete}
shouldShowLegendAction={handlers.shouldShowLegendAction}
/>
</div>
</I18nProvider>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ export interface VisualizationType {

export interface CellValueAction {
id: string;
type?: string;
iconType: string;
displayName: string;
execute: (data: CellValueContext['data']) => void;
Expand Down
1 change: 1 addition & 0 deletions src/plugins/chart_expressions/expression_xy/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"@kbn/event-annotation-common",
"@kbn/visualization-ui-components",
"@kbn/es-query",
"@kbn/cell-actions",
],
"exclude": [
"target/**/*",
Expand Down
2 changes: 0 additions & 2 deletions src/plugins/expressions/common/execution/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,6 @@ export interface ExecutionContext<
* Logs datatable.
*/
logDatatable?(name: string, datatable: Datatable): void;

shouldShowLegendAction?: (actionId: string) => boolean;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,5 +105,4 @@ export interface IInterpreterRenderHandlers {
uiState?: unknown;

getExecutionContext(): KibanaExecutionContext | undefined;
shouldShowLegendAction?: (actionId: string) => boolean;
}
1 change: 0 additions & 1 deletion src/plugins/expressions/public/loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ export class ExpressionLoader {
hasCompatibleActions: params?.hasCompatibleActions,
getCompatibleCellValueActions: params?.getCompatibleCellValueActions,
executionContext: params?.executionContext,
shouldShowLegendAction: params?.shouldShowLegendAction,
});
this.render$ = this.renderHandler.render$;
this.update$ = this.renderHandler.update$;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ export interface ReactExpressionRendererProps
error?: ExpressionRenderError | null
) => React.ReactElement | React.ReactElement[];
padding?: 'xs' | 's' | 'm' | 'l' | 'xl';
shouldShowLegendAction?: (actionId: string) => boolean;
}

export type ReactExpressionRendererType = React.ComponentType<ReactExpressionRendererProps>;
Expand Down
Loading

0 comments on commit 9bf7e38

Please sign in to comment.