From 4b794919ebcaa8a5273c84ab17269908dbf1a7e1 Mon Sep 17 00:00:00 2001 From: Yara Tercero Date: Fri, 17 Jul 2020 12:39:51 -0400 Subject: [PATCH] [Security Solution][Exceptions] - Remove initial add exception item button in builder (#72215) ## Summary This PR addresses two issues in the builder: - **Existing behavior:** if you add a bunch of entries then delete all but one, the indent that shows for when multiple entries exists does not go away - **Updated behavior:** if you add a bunch of entries and delete all but one, the indent that shows for when multiple entries exist goes away - **Existing behavior:** on render of add exception modal, if no exception items exist (or no exception items with entries exist) an `Add Exception` button appears - **Updated behavior:** if only one entry exists, the delete button is disabled for that entry; on initial render of the add exception modal, if no entries exist, an empty entry is shown --- .../builder_button_options.stories.tsx | 17 -- .../builder/builder_button_options.test.tsx | 44 --- .../builder/builder_button_options.tsx | 76 ++--- .../builder/builder_exception_item.test.tsx | 282 ++++++++++++++++++ .../builder/builder_exception_item.tsx | 161 ++++++++++ .../exceptions/builder/exception_item.tsx | 140 --------- .../components/exceptions/builder/index.tsx | 48 ++- .../components/exceptions/translations.ts | 7 - 8 files changed, 493 insertions(+), 282 deletions(-) create mode 100644 x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_exception_item.test.tsx create mode 100644 x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_exception_item.tsx delete mode 100644 x-pack/plugins/security_solution/public/common/components/exceptions/builder/exception_item.tsx diff --git a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_button_options.stories.tsx b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_button_options.stories.tsx index 7e4cbe34f9a64..9486008e708ea 100644 --- a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_button_options.stories.tsx +++ b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_button_options.stories.tsx @@ -16,26 +16,12 @@ addDecorator((storyFn) => ( )); storiesOf('Components|Exceptions|BuilderButtonOptions', module) - .add('init button', () => { - return ( - - ); - }) .add('and/or buttons', () => { return ( { isAndDisabled={false} isOrDisabled={false} showNestedButton={false} - displayInitButton={false} onOrClicked={jest.fn()} onAndClicked={jest.fn()} onNestedClicked={jest.fn()} @@ -31,44 +30,6 @@ describe('BuilderButtonOptions', () => { expect(wrapper.find('[data-test-subj="exceptionsNestedButton"] button')).toHaveLength(0); }); - test('it renders "add exception" button if "displayInitButton" is true', () => { - const wrapper = mount( - - ); - - expect(wrapper.find('[data-test-subj="exceptionsAddNewExceptionButton"] button')).toHaveLength( - 1 - ); - }); - - test('it invokes "onAddExceptionClicked" when "add exception" button is clicked', () => { - const onOrClicked = jest.fn(); - - const wrapper = mount( - - ); - - wrapper.find('[data-test-subj="exceptionsAddNewExceptionButton"] button').simulate('click'); - - expect(onOrClicked).toHaveBeenCalledTimes(1); - }); - test('it invokes "onOrClicked" when "or" button is clicked', () => { const onOrClicked = jest.fn(); @@ -77,7 +38,6 @@ describe('BuilderButtonOptions', () => { isAndDisabled={false} isOrDisabled={false} showNestedButton={false} - displayInitButton={false} onOrClicked={onOrClicked} onAndClicked={jest.fn()} onNestedClicked={jest.fn()} @@ -97,7 +57,6 @@ describe('BuilderButtonOptions', () => { isAndDisabled={false} isOrDisabled={false} showNestedButton={false} - displayInitButton={false} onOrClicked={jest.fn()} onAndClicked={onAndClicked} onNestedClicked={jest.fn()} @@ -113,7 +72,6 @@ describe('BuilderButtonOptions', () => { const wrapper = mount( { const wrapper = mount( { isAndDisabled={false} isOrDisabled={false} showNestedButton - displayInitButton={false} onOrClicked={jest.fn()} onAndClicked={jest.fn()} onNestedClicked={onNestedClicked} diff --git a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_button_options.tsx b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_button_options.tsx index ff1556bcc4d25..eb224b82d756f 100644 --- a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_button_options.tsx +++ b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_button_options.tsx @@ -16,7 +16,6 @@ const MyEuiButton = styled(EuiButton)` interface BuilderButtonOptionsProps { isOrDisabled: boolean; isAndDisabled: boolean; - displayInitButton: boolean; showNestedButton: boolean; onAndClicked: () => void; onOrClicked: () => void; @@ -26,64 +25,47 @@ interface BuilderButtonOptionsProps { export const BuilderButtonOptions: React.FC = ({ isOrDisabled = false, isAndDisabled = false, - displayInitButton, showNestedButton = false, onAndClicked, onOrClicked, onNestedClicked, }) => ( - {displayInitButton ? ( + + + {i18n.AND} + + + + + {i18n.OR} + + + {showNestedButton && ( - {i18n.ADD_EXCEPTION_TITLE} + {i18n.ADD_NESTED_DESCRIPTION} - ) : ( - <> - - - {i18n.AND} - - - - - {i18n.OR} - - - {showNestedButton && ( - - - {i18n.ADD_NESTED_DESCRIPTION} - - - )} - )} ); diff --git a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_exception_item.test.tsx b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_exception_item.test.tsx new file mode 100644 index 0000000000000..9ca7a371ce81b --- /dev/null +++ b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_exception_item.test.tsx @@ -0,0 +1,282 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import React from 'react'; +import { ThemeProvider } from 'styled-components'; +import { mount } from 'enzyme'; +import euiLightVars from '@elastic/eui/dist/eui_theme_light.json'; + +import { ExceptionListItemComponent } from './builder_exception_item'; +import { fields } from '../../../../../../../../src/plugins/data/common/index_patterns/fields/fields.mocks.ts'; +import { getExceptionListItemSchemaMock } from '../../../../../../lists/common/schemas/response/exception_list_item_schema.mock'; +import { + getEntryMatchMock, + getEntryMatchAnyMock, +} from '../../../../../../lists/common/schemas/types/entries.mock'; + +describe('ExceptionListItemComponent', () => { + describe('and badge logic', () => { + test('it renders "and" badge with extra top padding for the first exception item when "andLogicIncluded" is "true"', () => { + const exceptionItem = getExceptionListItemSchemaMock(); + exceptionItem.entries = [getEntryMatchMock(), getEntryMatchMock()]; + const wrapper = mount( + ({ eui: euiLightVars, darkMode: false })}> + + + ); + + expect( + wrapper.find('[data-test-subj="exceptionItemEntryFirstRowAndBadge"]').exists() + ).toBeTruthy(); + }); + + test('it renders "and" badge when more than one exception item entry exists and it is not the first exception item', () => { + const exceptionItem = getExceptionListItemSchemaMock(); + exceptionItem.entries = [getEntryMatchMock(), getEntryMatchMock()]; + const wrapper = mount( + ({ eui: euiLightVars, darkMode: false })}> + + + ); + + expect(wrapper.find('[data-test-subj="exceptionItemEntryAndBadge"]').exists()).toBeTruthy(); + }); + + test('it renders indented "and" badge when "andLogicIncluded" is "true" and only one entry exists', () => { + const exceptionItem = getExceptionListItemSchemaMock(); + exceptionItem.entries = [getEntryMatchMock()]; + const wrapper = mount( + ({ eui: euiLightVars, darkMode: false })}> + + + ); + + expect( + wrapper.find('[data-test-subj="exceptionItemEntryInvisibleAndBadge"]').exists() + ).toBeTruthy(); + }); + + test('it renders no "and" badge when "andLogicIncluded" is "false"', () => { + const exceptionItem = getExceptionListItemSchemaMock(); + exceptionItem.entries = [getEntryMatchMock()]; + const wrapper = mount( + ({ eui: euiLightVars, darkMode: false })}> + + + ); + + expect( + wrapper.find('[data-test-subj="exceptionItemEntryInvisibleAndBadge"]').exists() + ).toBeFalsy(); + expect(wrapper.find('[data-test-subj="exceptionItemEntryAndBadge"]').exists()).toBeFalsy(); + expect( + wrapper.find('[data-test-subj="exceptionItemEntryFirstRowAndBadge"]').exists() + ).toBeFalsy(); + }); + }); + + describe('delete button logic', () => { + test('it renders delete button disabled when it is only entry left in builder', () => { + const exceptionItem = getExceptionListItemSchemaMock(); + exceptionItem.entries = [getEntryMatchMock()]; + const wrapper = mount( + + ); + + expect( + wrapper.find('[data-test-subj="exceptionItemEntryDeleteButton"] button').props().disabled + ).toBeTruthy(); + }); + + test('it does not render delete button disabled when it is not the only entry left in builder', () => { + const exceptionItem = getExceptionListItemSchemaMock(); + exceptionItem.entries = [getEntryMatchMock()]; + + const wrapper = mount( + + ); + + expect( + wrapper.find('[data-test-subj="exceptionItemEntryDeleteButton"] button').props().disabled + ).toBeFalsy(); + }); + + test('it does not render delete button disabled when "exceptionItemIndex" is not "0"', () => { + const exceptionItem = getExceptionListItemSchemaMock(); + exceptionItem.entries = [getEntryMatchMock()]; + const wrapper = mount( + + ); + + expect( + wrapper.find('[data-test-subj="exceptionItemEntryDeleteButton"] button').props().disabled + ).toBeFalsy(); + }); + + test('it does not render delete button disabled when more than one entry exists', () => { + const exceptionItem = getExceptionListItemSchemaMock(); + exceptionItem.entries = [getEntryMatchMock(), getEntryMatchMock()]; + const wrapper = mount( + + ); + + expect( + wrapper.find('[data-test-subj="exceptionItemEntryDeleteButton"] button').at(0).props() + .disabled + ).toBeFalsy(); + }); + + test('it invokes "onChangeExceptionItem" when delete button clicked', () => { + const mockOnDeleteExceptionItem = jest.fn(); + const exceptionItem = getExceptionListItemSchemaMock(); + exceptionItem.entries = [getEntryMatchMock(), getEntryMatchAnyMock()]; + const wrapper = mount( + + ); + + wrapper + .find('[data-test-subj="exceptionItemEntryDeleteButton"] button') + .at(0) + .simulate('click'); + + expect(mockOnDeleteExceptionItem).toHaveBeenCalledWith( + { + ...exceptionItem, + entries: [getEntryMatchAnyMock()], + }, + 0 + ); + }); + }); +}); diff --git a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_exception_item.tsx b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_exception_item.tsx new file mode 100644 index 0000000000000..8e57e83d0e7e4 --- /dev/null +++ b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/builder_exception_item.tsx @@ -0,0 +1,161 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import React, { useMemo, useCallback } from 'react'; +import { EuiButtonIcon, EuiFlexGroup, EuiFlexItem } from '@elastic/eui'; +import styled from 'styled-components'; + +import { IIndexPattern } from '../../../../../../../../src/plugins/data/common'; +import { AndOrBadge } from '../../and_or_badge'; +import { EntryItemComponent } from './entry_item'; +import { getFormattedBuilderEntries } from '../helpers'; +import { FormattedBuilderEntry, ExceptionsBuilderExceptionItem, BuilderEntry } from '../types'; + +const MyInvisibleAndBadge = styled(EuiFlexItem)` + visibility: hidden; +`; + +const MyFirstRowContainer = styled(EuiFlexItem)` + padding-top: 20px; +`; + +interface ExceptionListItemProps { + exceptionItem: ExceptionsBuilderExceptionItem; + exceptionId: string; + exceptionItemIndex: number; + isLoading: boolean; + indexPattern: IIndexPattern; + andLogicIncluded: boolean; + isOnlyItem: boolean; + onDeleteExceptionItem: (item: ExceptionsBuilderExceptionItem, index: number) => void; + onChangeExceptionItem: (item: ExceptionsBuilderExceptionItem, index: number) => void; +} + +export const ExceptionListItemComponent = React.memo( + ({ + exceptionItem, + exceptionId, + exceptionItemIndex, + indexPattern, + isLoading, + isOnlyItem, + andLogicIncluded, + onDeleteExceptionItem, + onChangeExceptionItem, + }) => { + const handleEntryChange = useCallback( + (entry: BuilderEntry, entryIndex: number): void => { + const updatedEntries: BuilderEntry[] = [ + ...exceptionItem.entries.slice(0, entryIndex), + { ...entry }, + ...exceptionItem.entries.slice(entryIndex + 1), + ]; + const updatedExceptionItem: ExceptionsBuilderExceptionItem = { + ...exceptionItem, + entries: updatedEntries, + }; + onChangeExceptionItem(updatedExceptionItem, exceptionItemIndex); + }, + [onChangeExceptionItem, exceptionItem, exceptionItemIndex] + ); + + const handleDeleteEntry = useCallback( + (entryIndex: number): void => { + const updatedEntries: BuilderEntry[] = [ + ...exceptionItem.entries.slice(0, entryIndex), + ...exceptionItem.entries.slice(entryIndex + 1), + ]; + const updatedExceptionItem: ExceptionsBuilderExceptionItem = { + ...exceptionItem, + entries: updatedEntries, + }; + + onDeleteExceptionItem(updatedExceptionItem, exceptionItemIndex); + }, + [exceptionItem, onDeleteExceptionItem, exceptionItemIndex] + ); + + const entries = useMemo( + (): FormattedBuilderEntry[] => + indexPattern != null ? getFormattedBuilderEntries(indexPattern, exceptionItem.entries) : [], + [indexPattern, exceptionItem] + ); + + const andBadge = useMemo((): JSX.Element => { + const badge = ; + if (entries.length > 1 && exceptionItemIndex === 0) { + return ( + + {badge} + + ); + } else if (entries.length > 1) { + return ( + + {badge} + + ); + } else { + return ( + + {badge} + + ); + } + }, [entries.length, exceptionItemIndex]); + + const getDeleteButton = useCallback( + (index: number): JSX.Element => { + const button = ( + handleDeleteEntry(index)} + isDisabled={isOnlyItem && entries.length === 1 && exceptionItemIndex === 0} + aria-label="entryDeleteButton" + className="exceptionItemEntryDeleteButton" + data-test-subj="exceptionItemEntryDeleteButton" + /> + ); + if (index === 0 && exceptionItemIndex === 0) { + return {button}; + } else { + return {button}; + } + }, + [entries.length, exceptionItemIndex, handleDeleteEntry, isOnlyItem] + ); + + return ( + + {andLogicIncluded && andBadge} + + + {entries.map((item, index) => ( + + + + + + {getDeleteButton(index)} + + + ))} + + + + ); + } +); + +ExceptionListItemComponent.displayName = 'ExceptionListItem'; diff --git a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/exception_item.tsx b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/exception_item.tsx deleted file mode 100644 index 5e53ce3ba6578..0000000000000 --- a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/exception_item.tsx +++ /dev/null @@ -1,140 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ - -import React, { useMemo } from 'react'; -import { EuiButtonIcon, EuiFlexGroup, EuiFlexItem } from '@elastic/eui'; -import styled from 'styled-components'; - -import { IIndexPattern } from '../../../../../../../../src/plugins/data/common'; -import { AndOrBadge } from '../../and_or_badge'; -import { EntryItemComponent } from './entry_item'; -import { getFormattedBuilderEntries } from '../helpers'; -import { FormattedBuilderEntry, ExceptionsBuilderExceptionItem, BuilderEntry } from '../types'; - -const MyInvisibleAndBadge = styled(EuiFlexItem)` - visibility: hidden; -`; - -const MyFirstRowContainer = styled(EuiFlexItem)` - padding-top: 20px; -`; - -interface ExceptionListItemProps { - exceptionItem: ExceptionsBuilderExceptionItem; - exceptionId: string; - exceptionItemIndex: number; - isLoading: boolean; - indexPattern: IIndexPattern; - andLogicIncluded: boolean; - onCheckAndLogic: (item: ExceptionsBuilderExceptionItem[]) => void; - onDeleteExceptionItem: (item: ExceptionsBuilderExceptionItem, index: number) => void; - onExceptionItemChange: (item: ExceptionsBuilderExceptionItem, index: number) => void; -} - -export const ExceptionListItemComponent = React.memo( - ({ - exceptionItem, - exceptionId, - exceptionItemIndex, - indexPattern, - isLoading, - andLogicIncluded, - onCheckAndLogic, - onDeleteExceptionItem, - onExceptionItemChange, - }) => { - const handleEntryChange = (entry: BuilderEntry, entryIndex: number): void => { - const updatedEntries: BuilderEntry[] = [ - ...exceptionItem.entries.slice(0, entryIndex), - { ...entry }, - ...exceptionItem.entries.slice(entryIndex + 1), - ]; - const updatedExceptionItem: ExceptionsBuilderExceptionItem = { - ...exceptionItem, - entries: updatedEntries, - }; - onExceptionItemChange(updatedExceptionItem, exceptionItemIndex); - }; - - const handleDeleteEntry = (entryIndex: number): void => { - const updatedEntries: BuilderEntry[] = [ - ...exceptionItem.entries.slice(0, entryIndex), - ...exceptionItem.entries.slice(entryIndex + 1), - ]; - const updatedExceptionItem: ExceptionsBuilderExceptionItem = { - ...exceptionItem, - entries: updatedEntries, - }; - - onDeleteExceptionItem(updatedExceptionItem, exceptionItemIndex); - }; - - const entries = useMemo((): FormattedBuilderEntry[] => { - onCheckAndLogic([exceptionItem]); - return indexPattern != null - ? getFormattedBuilderEntries(indexPattern, exceptionItem.entries) - : []; - }, [indexPattern, exceptionItem, onCheckAndLogic]); - - const andBadge = useMemo((): JSX.Element => { - const badge = ; - if (entries.length > 1 && exceptionItemIndex === 0) { - return {badge}; - } else if (entries.length > 1) { - return {badge}; - } else { - return {badge}; - } - }, [entries.length, exceptionItemIndex]); - - const getDeleteButton = (index: number): JSX.Element => { - const button = ( - handleDeleteEntry(index)} - aria-label="entryDeleteButton" - className="exceptionItemEntryDeleteButton" - data-test-subj="exceptionItemEntryDeleteButton" - /> - ); - if (index === 0 && exceptionItemIndex === 0) { - return {button}; - } else { - return {button}; - } - }; - - return ( - - {andLogicIncluded && andBadge} - - - {entries.map((item, index) => ( - - - - - - {getDeleteButton(index)} - - - ))} - - - - ); - } -); - -ExceptionListItemComponent.displayName = 'ExceptionListItem'; diff --git a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/index.tsx b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/index.tsx index 6bff33afaf70c..08e5b49073ecf 100644 --- a/x-pack/plugins/security_solution/public/common/components/exceptions/builder/index.tsx +++ b/x-pack/plugins/security_solution/public/common/components/exceptions/builder/index.tsx @@ -3,11 +3,11 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -import React, { useMemo, useCallback, useEffect, useState } from 'react'; +import React, { useCallback, useEffect, useState } from 'react'; import { EuiFlexGroup, EuiFlexItem } from '@elastic/eui'; import styled from 'styled-components'; -import { ExceptionListItemComponent } from './exception_item'; +import { ExceptionListItemComponent } from './builder_exception_item'; import { useFetchIndexPatterns } from '../../../../detections/containers/detection_engine/rules/fetch_index_patterns'; import { ExceptionListItemSchema, @@ -80,20 +80,9 @@ export const ExceptionBuilder = ({ ); const handleCheckAndLogic = (items: ExceptionsBuilderExceptionItem[]): void => { - setAndLogicIncluded((includesAnd: boolean): boolean => { - if (includesAnd) { - return true; - } else { - return items.filter(({ entries }) => entries.length > 1).length > 0; - } - }); + setAndLogicIncluded(items.filter(({ entries }) => entries.length > 1).length > 0); }; - // Bubble up changes to parent - useEffect(() => { - onChange({ exceptionItems: filterExceptionItems(exceptions), exceptionsToDelete }); - }, [onChange, exceptionsToDelete, exceptions]); - const handleDeleteExceptionItem = ( item: ExceptionsBuilderExceptionItem, itemIndex: number @@ -164,16 +153,6 @@ export const ExceptionBuilder = ({ setExceptions((existingExceptions) => [...existingExceptions, { ...newException }]); }, [setExceptions, listType, listId, listNamespaceType, ruleName]); - // An exception item can have an empty array for `entries` - const displayInitialAddExceptionButton = useMemo((): boolean => { - return ( - exceptions.length === 0 || - (exceptions.length === 1 && - exceptions[0].entries != null && - exceptions[0].entries.length === 0) - ); - }, [exceptions]); - // Filters index pattern fields by exceptionable fields if list type is endpoint const filterIndexPatterns = useCallback(() => { if (listType === 'endpoint') { @@ -199,6 +178,22 @@ export const ExceptionBuilder = ({ } }; + // Bubble up changes to parent + useEffect(() => { + onChange({ exceptionItems: filterExceptionItems(exceptions), exceptionsToDelete }); + }, [onChange, exceptionsToDelete, exceptions]); + + useEffect(() => { + if ( + exceptions.length === 0 || + (exceptions.length === 1 && + exceptions[0].entries != null && + exceptions[0].entries.length === 0) + ) { + handleAddNewExceptionItem(); + } + }, [exceptions, handleAddNewExceptionItem]); + return ( {(isLoading || indexPatternLoading) && ( @@ -233,9 +228,9 @@ export const ExceptionBuilder = ({ isLoading={indexPatternLoading} exceptionItemIndex={index} andLogicIncluded={andLogicIncluded} - onCheckAndLogic={handleCheckAndLogic} + isOnlyItem={exceptions.length === 1} onDeleteExceptionItem={handleDeleteExceptionItem} - onExceptionItemChange={handleExceptionItemChange} + onChangeExceptionItem={handleExceptionItemChange} /> @@ -253,7 +248,6 @@ export const ExceptionBuilder = ({