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

[Cloud Security] [Findings] Adding grouping component #169884

Merged
merged 38 commits into from
Nov 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
8bbb01a
wip: grouping
opauloh Oct 13, 2023
c40e2fc
group selector fix
opauloh Oct 17, 2023
0ac7e9e
WIP: group selector on findings page
opauloh Oct 17, 2023
bb7f308
adding more group by fields
opauloh Oct 18, 2023
1bacb6a
Merge branch 'main' into grouping/foundation
opauloh Oct 18, 2023
c9f251e
add non url additional filters
opauloh Oct 23, 2023
9432b87
Merge branch 'main' into grouping/foundation
opauloh Oct 23, 2023
4fd1510
adding custom title
opauloh Oct 25, 2023
fd4e071
additional filters
opauloh Oct 25, 2023
40a7797
datatable group component
opauloh Oct 25, 2023
bd80f34
clean up code
opauloh Oct 25, 2023
befa489
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine Oct 25, 2023
2f761d5
add more information to disabled
opauloh Oct 25, 2023
ec0dbdf
code clean up
opauloh Oct 26, 2023
d55081c
hiding take action button
opauloh Oct 26, 2023
4d8e9bc
Merge branch 'main' into grouping/foundation
opauloh Nov 9, 2023
59cf7e3
addressing PR review suggestions
opauloh Nov 11, 2023
5abda98
splitting and optimizing code
opauloh Nov 13, 2023
dcde804
splitting code into hooks and components
opauloh Nov 13, 2023
97c19a3
addressing pr comments, removing anys
opauloh Nov 13, 2023
be967d2
findings grouping FTR tests
opauloh Nov 15, 2023
199f938
reverting change
opauloh Nov 15, 2023
f51a462
adding comments and time filtering
opauloh Nov 15, 2023
1af2377
revert use posture table
opauloh Nov 15, 2023
1eb9ef2
add use posture data table
opauloh Nov 15, 2023
a145184
update FTR tests
opauloh Nov 15, 2023
24188a3
Merge branch 'main' into grouping/foundation
opauloh Nov 15, 2023
975fd13
update dashboard navigation
opauloh Nov 15, 2023
1c0308c
fix ci errors
opauloh Nov 15, 2023
7bacb48
fixing ci and FTR tests
opauloh Nov 16, 2023
ca4713c
fix CI types error
opauloh Nov 16, 2023
95f8cf4
remove unused service from ftr
opauloh Nov 16, 2023
70e9539
Merge branch 'main' into grouping/foundation
opauloh Nov 21, 2023
2c35de7
Merge branch 'main' into grouping/foundation
opauloh Nov 21, 2023
3d866de
Merge branch 'main' into grouping/foundation
opauloh Nov 21, 2023
dca18ba
Merge branch 'main' into grouping/foundation
opauloh Nov 22, 2023
af4d6b5
Merge branch 'main' into grouping/foundation
opauloh Nov 27, 2023
6ff86ab
adding maxGroupingLevels tests
opauloh Nov 27, 2023
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 @@ -124,4 +124,52 @@ describe('group selector', () => {
);
expect(getByTestId('group-selector-dropdown').title).toEqual('Rule name, Host name');
});
describe('when maxGroupingLevels is 1', () => {
it('Presents single option selector label when dropdown is clicked', () => {
const { getByTestId } = render(
<GroupSelector {...testProps} maxGroupingLevels={1} groupsSelected={[]} />
);
fireEvent.click(getByTestId('group-selector-dropdown'));
expect(getByTestId('contextMenuPanelTitle').textContent).toMatch(/select grouping/i);
});
it('Does not disable any options when maxGroupingLevels is 1 and one option is selected', () => {
const groupSelected = ['kibana.alert.rule.name'];

const { getByTestId } = render(
<GroupSelector {...testProps} maxGroupingLevels={1} groupsSelected={groupSelected} />
);

fireEvent.click(getByTestId('group-selector-dropdown'));

[...testProps.options, { key: 'custom', label: 'Custom field' }].forEach((o) => {
expect(getByTestId(`panel-${o.key}`)).not.toHaveAttribute('disabled');
});
});
});
describe('when maxGroupingLevels is greater than 1', () => {
it('Presents select up to "X" groupings when dropdown is clicked', () => {
const { getByTestId } = render(
<GroupSelector {...testProps} maxGroupingLevels={3} groupsSelected={[]} />
);
fireEvent.click(getByTestId('group-selector-dropdown'));
expect(getByTestId('contextMenuPanelTitle').textContent).toMatch(/select up to 3 groupings/i);
});
it('Disables non-selected options when maxGroupingLevels is greater than 1 and the selects items reaches the maxGroupingLevels', () => {
const groupSelected = ['kibana.alert.rule.name', 'user.name'];

const { getByTestId } = render(
<GroupSelector {...testProps} maxGroupingLevels={2} groupsSelected={groupSelected} />
);

fireEvent.click(getByTestId('group-selector-dropdown'));

[...testProps.options, { key: 'custom', label: 'Custom field' }].forEach((o) => {
if (groupSelected.includes(o.key) || o.key === 'none') {
expect(getByTestId(`panel-${o.key}`)).not.toHaveAttribute('disabled');
} else {
expect(getByTestId(`panel-${o.key}`)).toHaveAttribute('disabled');
}
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,21 @@ const GroupSelectorComponent = ({
[groupsSelected]
);

const panels: EuiContextMenuPanelDescriptor[] = useMemo(
() => [
const panels: EuiContextMenuPanelDescriptor[] = useMemo(() => {
const isOptionDisabled = (key?: string) => {
// Do not disable when maxGroupingLevels is 1 to allow toggling between groups
if (maxGroupingLevels === 1) {
opauloh marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
// Disable all non selected options when the maxGroupingLevels is reached
return groupsSelected.length === maxGroupingLevels && (key ? !isGroupSelected(key) : true);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we keep the behaviour of disabling currently selected option when the max level is 1? Right now to remove grouping, you can either switch to none or "unselect" the option. If we change the behavior to toggle when max level is 1, maybe it makes sense to disable this "unselect" behaviour

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great point, I'll adjust it in the code and bring it to discussion with the code owners

Copy link
Contributor

Choose a reason for hiding this comment

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

thats fine, can you please just add some unit tests? We never get to line 53 in the current tests

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 added the unit tests related to the changes introduced in this PR, thanks a lot for reviewing it @stephmilovic!

};

return [
{
id: 'firstPanel',
title: i18n.SELECT_FIELD(maxGroupingLevels),
title:
maxGroupingLevels === 1 ? i18n.SELECT_SINGLE_FIELD : i18n.SELECT_FIELD(maxGroupingLevels),
items: [
{
'data-test-subj': 'panel-none',
Expand All @@ -57,7 +67,7 @@ const GroupSelectorComponent = ({
},
...options.map<EuiContextMenuPanelItemDescriptor>((o) => ({
'data-test-subj': `panel-${o.key}`,
disabled: groupsSelected.length === maxGroupingLevels && !isGroupSelected(o.key),
disabled: isOptionDisabled(o.key),
name: o.label,
onClick: () => onGroupChange(o.key),
icon: isGroupSelected(o.key) ? 'check' : 'empty',
Expand All @@ -66,7 +76,7 @@ const GroupSelectorComponent = ({
'data-test-subj': `panel-custom`,
name: i18n.CUSTOM_FIELD,
icon: 'empty',
disabled: groupsSelected.length === maxGroupingLevels,
disabled: isOptionDisabled(),
panel: 'customPanel',
hasPanel: true,
},
Expand All @@ -87,9 +97,8 @@ const GroupSelectorComponent = ({
/>
),
},
],
[fields, groupsSelected.length, isGroupSelected, maxGroupingLevels, onGroupChange, options]
);
];
}, [fields, groupsSelected.length, isGroupSelected, maxGroupingLevels, onGroupChange, options]);
const selectedOptions = useMemo(
() => options.filter((groupOption) => isGroupSelected(groupOption.key)),
[isGroupSelected, options]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ export const SELECT_FIELD = (groupingLevelsCount: number) =>
defaultMessage: 'Select up to {groupingLevelsCount} groupings',
});

export const SELECT_SINGLE_FIELD = i18n.translate('grouping.groupBySingleField', {
defaultMessage: 'Select grouping',
});

export const NONE = i18n.translate('grouping.noneGroupByOptionName', {
defaultMessage: 'None',
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ type RunTimeMappings =
| Record<string, Omit<RuntimeFieldSpec, 'type'> & { type: RuntimePrimitiveTypes }>
| undefined;

interface BoolAgg {
export interface BoolAgg {
bool: BoolQuery;
}

interface RangeAgg {
export interface RangeAgg {
opauloh marked this conversation as resolved.
Show resolved Hide resolved
range: { '@timestamp': { gte: string; lte: string } };
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,34 @@ describe('Group Selector Hooks', () => {
});
});

it('On group change when maxGroupingLevels is 1, remove previously selected group', () => {
const testGroup = {
[groupingId]: {
...defaultGroup,
options: defaultGroupingOptions,
activeGroups: ['host.name'],
},
};
const { result } = renderHook((props) => useGetGroupSelector(props), {
initialProps: {
...defaultArgs,
maxGroupingLevels: 1,
groupingState: {
groupById: testGroup,
},
},
});
act(() => result.current.props.onGroupChange('user.name'));

expect(dispatch).toHaveBeenCalledWith({
payload: {
id: groupingId,
activeGroups: ['user.name'],
},
type: ActionType.updateActiveGroups,
});
});

it('On group change, resets active page, sets active group, and leaves options alone', () => {
const testGroup = {
[groupingId]: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export interface UseGetGroupSelectorArgs {
event: string | string[],
count?: number | undefined
) => void;
title?: string;
}

interface UseGetGroupSelectorStateless
Expand Down Expand Up @@ -84,6 +85,7 @@ export const useGetGroupSelector = ({
onGroupChange,
onOptionsChange,
tracker,
title,
}: UseGetGroupSelectorArgs) => {
const { activeGroups: selectedGroups, options } =
groupByIdSelector({ groups: groupingState }, groupingId) ?? defaultGroup;
Expand All @@ -110,20 +112,25 @@ export const useGetGroupSelector = ({

const onChange = useCallback(
(groupSelection: string) => {
if (selectedGroups.find((selected) => selected === groupSelection)) {
const groups = selectedGroups.filter((selectedGroup) => selectedGroup !== groupSelection);
if (groups.length === 0) {
setSelectedGroups(['none']);
} else {
setSelectedGroups(groups);
// Simulate a toggle behavior when maxGroupingLevels is 1
opauloh marked this conversation as resolved.
Show resolved Hide resolved
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 kbn-securitysolution-grouping lib would benefit from having some unit tests. As it didn't have any, I wouldn't block this PR on that, but I'd vote for adding some of the tests which are testing your changes to the lib

Copy link
Contributor

@stephmilovic stephmilovic Nov 27, 2023

Choose a reason for hiding this comment

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

I think kbn-securitysolution-grouping lib would benefit from having some unit tests. As it didn't have any...

@maxcold Why do you think there are not unit tests? We do have tests that should have been added to :

  • packages/kbn-securitysolution-grouping/src/components/group_selector/index.test.tsx
  • packages/kbn-securitysolution-grouping/src/hooks/use_get_group_selector.test.tsx

Please add unit tests

Copy link
Contributor

@maxcold maxcold Nov 27, 2023

Choose a reason for hiding this comment

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

@stephmilovic totally my bad, I have no idea where I looked tbh, but I remember checking the codebase in my code editor and not finding the unit test files. Some kind of a blackout from my side I guess :) for sure we need to cover new cases with unit tests

if (maxGroupingLevels === 1) {
setSelectedGroups([groupSelection]);
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 to be the only untested condition here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test added 🙌

} else {
if (selectedGroups.find((selected) => selected === groupSelection)) {
const groups = selectedGroups.filter((selectedGroup) => selectedGroup !== groupSelection);
if (groups.length === 0) {
setSelectedGroups(['none']);
} else {
setSelectedGroups(groups);
}
return;
}
return;
}

const newSelectedGroups = isNoneGroup([groupSelection])
? [groupSelection]
: [...selectedGroups.filter((selectedGroup) => selectedGroup !== 'none'), groupSelection];
setSelectedGroups(newSelectedGroups);
const newSelectedGroups = isNoneGroup([groupSelection])
? [groupSelection]
: [...selectedGroups.filter((selectedGroup) => selectedGroup !== 'none'), groupSelection];
setSelectedGroups(newSelectedGroups);
}

// built-in telemetry: UI-counter
tracker?.(
Expand All @@ -133,7 +140,7 @@ export const useGetGroupSelector = ({

onGroupChange?.({ tableId: groupingId, groupByField: groupSelection });
},
[groupingId, onGroupChange, selectedGroups, setSelectedGroups, tracker]
[groupingId, maxGroupingLevels, onGroupChange, selectedGroups, setSelectedGroups, tracker]
);

useEffect(() => {
Expand Down Expand Up @@ -184,6 +191,7 @@ export const useGetGroupSelector = ({
fields,
maxGroupingLevels,
options,
title,
}}
/>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { Grouping as GroupingComponent } from '../components/grouping';
/** Interface for grouping object where T is the `GroupingAggregation`
* @interface GroupingArgs<T>
*/
interface Grouping<T> {
export interface UseGrouping<T> {
getGrouping: (props: DynamicGroupingProps<T>) => React.ReactElement;
groupSelector: React.ReactElement<GroupSelectorProps>;
selectedGroups: string[];
Expand Down Expand Up @@ -72,6 +72,7 @@ interface GroupingArgs<T> {
event: string | string[],
count?: number | undefined
) => void;
title?: string;
}

/**
Expand All @@ -85,6 +86,7 @@ interface GroupingArgs<T> {
* @param onGroupChange callback executed when selected group is changed, used for tracking
* @param onOptionsChange callback executed when grouping options are changed, used for consumer grouping selector
* @param tracker telemetry handler
* @param title of the grouping selector component
* @returns {@link Grouping} the grouping constructor { getGrouping, groupSelector, pagination, selectedGroups }
*/
export const useGrouping = <T,>({
Expand All @@ -96,7 +98,8 @@ export const useGrouping = <T,>({
onGroupChange,
onOptionsChange,
tracker,
}: GroupingArgs<T>): Grouping<T> => {
title,
}: GroupingArgs<T>): UseGrouping<T> => {
const [groupingState, dispatch] = useReducer(groupsReducerWithStorage, initialState);
const { activeGroups: selectedGroups } = useMemo(
() => groupByIdSelector({ groups: groupingState }, groupingId) ?? defaultGroup,
Expand Down Expand Up @@ -125,6 +128,7 @@ export const useGrouping = <T,>({
onGroupChange,
onOptionsChange,
tracker,
title,
});

const getGrouping = useCallback(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
* 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; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

export * from './use_cloud_posture_data_table';
Loading
Loading