Skip to content

Commit

Permalink
[RAC] [Security Solution] Hides the Show top <field> action in char…
Browse files Browse the repository at this point in the history
…t legends (elastic#109566)

## Summary

Fixes <elastic#108910> , which allowed users to launch a new `Top <field>` popover from an existing popover, to infinity (and beyond)

The `Show top <field>` action is now hidden in the `Top <field>` popover's chart legend, and also:

- In the legend items of charts throughout the Security Solution (e.g. on the `Overview` page, and in the `Trend` chart on the `Alerts` page)
- For items in the `Count` aggregation table on the `Alerts` page

## Screenshots

### Before (Top `signal.rule.name` popover)

![before-top-signal-rule-name](https://user-images.githubusercontent.com/4459398/130302784-00a6c24d-17c8-4361-979e-01b8467f100e.png)

_Above: It was possible to launch another `Top <field>` popover from the legend of an existing popover_

### After (Top `signal.rule.name` popover)

![after-top-signal-rule-name](https://user-images.githubusercontent.com/4459398/130302925-d5aaa1ff-9565-4374-aa87-bde5880cb64f.png)

_Above: It is no longer possible to launch another `Top <field>` popover from the legend of an existing popover_

### Before (Chart legends)

![before-overview](https://user-images.githubusercontent.com/4459398/130303169-dc6c6de3-a2ba-40fe-a1f0-fe0d78b9638c.png)

_Above: It was possible to launch a `Top <field>` popover from chart legends_

### After (Chart legends)

![after-overview](https://user-images.githubusercontent.com/4459398/130303519-2eb0a60e-c6cd-4659-b6b2-d5ba234f668f.png)

_Above: It is no longer possible to launch a `Top <field>` popover from chart legends_

### Before (`Count` items)

![before-count](https://user-images.githubusercontent.com/4459398/130304111-b37373cf-1afb-41b8-9f38-b5d9b37cdb2d.png)

_Above: It was possible to launch a `Top <field>` popover from `Count` items_

### After (`Count` items)

![after-count](https://user-images.githubusercontent.com/4459398/130304166-fb641fa2-b52e-44ff-8210-0e228a43330c.png)

_Above: It is no longer possible to launch a `Top <field>` popover from `Count` items_

cc @mdefazio
  • Loading branch information
andrew-goldstein authored and kibanamachine committed Aug 23, 2021
1 parent 69ecbed commit 4371cad
Show file tree
Hide file tree
Showing 15 changed files with 78 additions and 99 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,10 @@ describe('DraggableLegendItem', () => {
wrapper.find(`[data-test-subj="legend-item-${legendItem.dataProviderId}"]`).first().text()
).toEqual(legendItem.value);
});

it('always hides the Top N action for legend items', () => {
expect(
wrapper.find(`[data-test-subj="legend-item-${legendItem.dataProviderId}"]`).prop('hideTopN')
).toEqual(true);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const DraggableLegendItemComponent: React.FC<{
<DefaultDraggable
data-test-subj={`legend-item-${dataProviderId}`}
field={field}
hideTopN={true}
id={dataProviderId}
isDraggable={false}
timelineId={timelineId}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ type RenderFunctionProp = (
interface Props {
dataProvider: DataProvider;
disabled?: boolean;
hideTopN?: boolean;
isDraggable?: boolean;
inline?: boolean;
render: RenderFunctionProp;
Expand Down Expand Up @@ -125,6 +126,7 @@ export const getStyle = (

const DraggableOnWrapperComponent: React.FC<Props> = ({
dataProvider,
hideTopN = false,
onFilterAdded,
render,
timelineId,
Expand All @@ -147,6 +149,7 @@ const DraggableOnWrapperComponent: React.FC<Props> = ({
showTopN,
} = useHoverActions({
dataProvider,
hideTopN,
onFilterAdded,
render,
timelineId,
Expand Down Expand Up @@ -304,6 +307,7 @@ const DraggableOnWrapperComponent: React.FC<Props> = ({

const DraggableWrapperComponent: React.FC<Props> = ({
dataProvider,
hideTopN = false,
isDraggable = false,
onFilterAdded,
render,
Expand All @@ -319,6 +323,7 @@ const DraggableWrapperComponent: React.FC<Props> = ({
showTopN,
} = useHoverActions({
dataProvider,
hideTopN,
isDraggable,
onFilterAdded,
render,
Expand Down Expand Up @@ -363,6 +368,7 @@ const DraggableWrapperComponent: React.FC<Props> = ({
return (
<DraggableOnWrapperComponent
dataProvider={dataProvider}
hideTopN={hideTopN}
onFilterAdded={onFilterAdded}
render={render}
timelineId={timelineId}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -655,6 +655,7 @@ describe('helpers', () => {
allowTopN({
browserField: aggregatableAllowedType,
fieldName: aggregatableAllowedType.name,
hideTopN: false,
})
).toBe(true);
});
Expand All @@ -664,6 +665,7 @@ describe('helpers', () => {
allowTopN({
browserField: undefined,
fieldName: 'signal.rule.name',
hideTopN: false,
})
).toBe(true);
});
Expand All @@ -678,6 +680,7 @@ describe('helpers', () => {
allowTopN({
browserField: nonAggregatableAllowedType,
fieldName: nonAggregatableAllowedType.name,
hideTopN: false,
})
).toBe(false);
});
Expand All @@ -692,6 +695,7 @@ describe('helpers', () => {
allowTopN({
browserField: aggregatableNotAllowedType,
fieldName: aggregatableNotAllowedType.name,
hideTopN: false,
})
).toBe(false);
});
Expand All @@ -703,6 +707,7 @@ describe('helpers', () => {
allowTopN({
browserField: missingAggregatable,
fieldName: missingAggregatable.name,
hideTopN: false,
})
).toBe(false);
});
Expand All @@ -714,6 +719,7 @@ describe('helpers', () => {
allowTopN({
browserField: missingType,
fieldName: missingType.name,
hideTopN: false,
})
).toBe(false);
});
Expand All @@ -723,6 +729,17 @@ describe('helpers', () => {
allowTopN({
browserField: undefined,
fieldName: 'non-allowlisted',
hideTopN: false,
})
).toBe(false);
});

test('it returns false when hideTopN is true', () => {
expect(
allowTopN({
browserField: aggregatableAllowedType,
fieldName: aggregatableAllowedType.name,
hideTopN: true, // <-- the Top N action shall not be shown for this (otherwise valid) field
})
).toBe(false);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,11 @@ export const addProviderToTimeline = ({
export const allowTopN = ({
browserField,
fieldName,
hideTopN,
}: {
browserField: Partial<BrowserField> | undefined;
fieldName: string;
hideTopN: boolean;
}): boolean => {
const isAggregatable = browserField?.aggregatable ?? false;
const fieldType = browserField?.type ?? '';
Expand Down Expand Up @@ -181,5 +183,9 @@ export const allowTopN = ({
'signal.status',
].includes(fieldName);

if (hideTopN) {
return false;
}

return isAllowlistedNonBrowserField || (isAggregatable && isAllowedType);
};

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
import { Provider } from '../../../timelines/components/timeline/data_providers/provider';

export interface DefaultDraggableType {
hideTopN?: boolean;
id: string;
isDraggable?: boolean;
field: string;
Expand Down Expand Up @@ -88,9 +89,11 @@ Content.displayName = 'Content';
* @param tooltipContent - defaults to displaying `field`, pass `null` to
* prevent a tooltip from being displayed, or pass arbitrary content
* @param queryValue - defaults to `value`, this query overrides the `queryMatch.value` used by the `DataProvider` that represents the data
* @param hideTopN - defaults to `false`, when true, the option to aggregate this field will be hidden
*/
export const DefaultDraggable = React.memo<DefaultDraggableType>(
({
hideTopN = false,
id,
isDraggable = true,
field,
Expand Down Expand Up @@ -137,6 +140,7 @@ export const DefaultDraggable = React.memo<DefaultDraggableType>(
return (
<DraggableWrapper
dataProvider={dataProviderProp}
hideTopN={hideTopN}
isDraggable={isDraggable}
render={renderCallback}
timelineId={timelineId}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,10 @@ AdditionalContent.displayName = 'AdditionalContent';
const StyledHoverActionsContainer = styled.div<{
$showTopN: boolean;
$showOwnFocus: boolean;
$hideTopN: boolean;
$isActive: boolean;
}>`
min-width: 138px;
min-width: ${({ $hideTopN }) => `${$hideTopN ? '112px' : '138px'}`};
padding: ${(props) => `0 ${props.theme.eui.paddingSizes.s}`};
display: flex;
Expand Down Expand Up @@ -91,6 +92,7 @@ interface Props {
enableOverflowButton?: boolean;
field: string;
goGetTimelineId?: (args: boolean) => void;
hideTopN?: boolean;
isObjectArray: boolean;
onFilterAdded?: () => void;
ownFocus: boolean;
Expand Down Expand Up @@ -129,6 +131,7 @@ export const HoverActions: React.FC<Props> = React.memo(
field,
goGetTimelineId,
isObjectArray,
hideTopN = false,
onFilterAdded,
ownFocus,
showOwnFocus = true,
Expand Down Expand Up @@ -207,6 +210,7 @@ export const HoverActions: React.FC<Props> = React.memo(
enableOverflowButton,
field,
handleHoverActionClicked,
hideTopN,
isObjectArray,
isOverflowPopoverOpen,
onFilterAdded,
Expand All @@ -231,6 +235,7 @@ export const HoverActions: React.FC<Props> = React.memo(
onKeyDown={onKeyDown}
$showTopN={showTopN}
$showOwnFocus={showOwnFocus}
$hideTopN={hideTopN}
$isActive={isActive}
className={isActive ? 'hoverActions-active' : ''}
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ describe('useHoverActionItems', () => {
defaultFocusedButtonRef: null,
field: 'signal.rule.name',
handleHoverActionClicked: jest.fn(),
hideTopN: false,
isObjectArray: true,
ownFocus: false,
showTopN: false,
Expand Down Expand Up @@ -112,4 +113,21 @@ describe('useHoverActionItems', () => {
);
});
});

test('it should hide the Top N action when hideTopN is true', async () => {
await act(async () => {
const { result, waitForNextUpdate } = renderHook(() => {
const testProps = {
...defaultProps,
hideTopN: true, // <-- hide the Top N action
};
return useHoverActionItems(testProps);
});
await waitForNextUpdate();

result.current.allActionItems.forEach((item) => {
expect(item.props['data-test-subj']).not.toEqual('hover-actions-show-top-n');
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { isEmpty } from 'lodash';

import { useKibana } from '../../lib/kibana';
import { getAllFieldsByName } from '../../containers/source';
import { allowTopN } from './utils';
import { allowTopN } from '../drag_and_drop/helpers';
import { useDeepEqualSelector } from '../../hooks/use_selector';
import { ColumnHeaderOptions, DataProvider, TimelineId } from '../../../../common/types/timeline';
import { SourcererScopeName } from '../../store/sourcerer/model';
Expand All @@ -29,6 +29,7 @@ export interface UseHoverActionItemsProps {
enableOverflowButton?: boolean;
field: string;
handleHoverActionClicked: () => void;
hideTopN: boolean;
isObjectArray: boolean;
isOverflowPopoverOpen?: boolean;
itemsToShow?: number;
Expand Down Expand Up @@ -56,6 +57,7 @@ export const useHoverActionItems = ({
enableOverflowButton,
field,
handleHoverActionClicked,
hideTopN,
isObjectArray,
isOverflowPopoverOpen,
itemsToShow = 2,
Expand Down Expand Up @@ -182,6 +184,7 @@ export const useHoverActionItems = ({
allowTopN({
browserField: getAllFieldsByName(browserFields)[field],
fieldName: field,
hideTopN,
}) ? (
<ShowTopNButton
Component={enableOverflowButton ? EuiContextMenuItem : undefined}
Expand Down Expand Up @@ -229,6 +232,7 @@ export const useHoverActionItems = ({
getFilterForValueButton,
getFilterOutValueButton,
handleHoverActionClicked,
hideTopN,
isObjectArray,
onFilterAdded,
ownFocus,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ type RenderFunctionProp = (
interface Props {
dataProvider: DataProvider;
disabled?: boolean;
hideTopN: boolean;
isDraggable?: boolean;
inline?: boolean;
render: RenderFunctionProp;
Expand All @@ -38,6 +39,7 @@ interface Props {

export const useHoverActions = ({
dataProvider,
hideTopN,
isDraggable,
onFilterAdded,
render,
Expand Down Expand Up @@ -101,6 +103,7 @@ export const useHoverActions = ({
dataProvider={dataProvider}
draggableId={isDraggable ? getDraggableId(dataProvider.id) : undefined}
field={dataProvider.queryMatch.field}
hideTopN={hideTopN}
isObjectArray={false}
goGetTimelineId={setGoGetTimelineId}
onFilterAdded={onFilterAdded}
Expand All @@ -120,6 +123,7 @@ export const useHoverActions = ({
closeTopN,
dataProvider,
handleClosePopOverTrigger,
hideTopN,
hoverActionsOwnFocus,
isDraggable,
onFilterAdded,
Expand Down
Loading

0 comments on commit 4371cad

Please sign in to comment.