From f0be0ade196e9651cc419232846a430b7ff2c038 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Thu, 21 Jan 2021 08:33:49 -0500 Subject: [PATCH] [Uptime] Improve filter group (#88185) * Unskip "Observer location" test block. * Commit temp "describe.only" to make flaky test runner go faster. * Add optional chain for some potentially-null props. * Make overview filters type partial. * Repair broken types. * Remove only call from test. * Add unit tests and mark areas for improvement in \`FilterGroup\` component. * Add aria-label translations and new labels. * Refactor existing tests and add tests for new labels. * Fix bug in event handler and update tests. * Delete a comment. * Delete a comment. * Add some line breaks to help readability. * Add additional tests, fix a bug. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../filters_expression_select.test.tsx | 264 ++++++++++-------- .../filters_expression_select.tsx | 14 +- .../monitor_expressions/translations.ts | 21 ++ .../filter_popover.test.tsx.snap | 129 --------- .../filter_group/filter_group.test.tsx | 73 +++++ .../overview/filter_group/filter_group.tsx | 9 +- .../filter_group/filter_popover.test.tsx | 94 ++++--- .../overview/filter_group/filter_popover.tsx | 9 +- .../overview/filter_group/translations.tsx | 4 +- .../filter_group/uptime_filter_button.tsx | 5 + 10 files changed, 330 insertions(+), 292 deletions(-) delete mode 100644 x-pack/plugins/uptime/public/components/overview/filter_group/__snapshots__/filter_popover.test.tsx.snap create mode 100644 x-pack/plugins/uptime/public/components/overview/filter_group/filter_group.test.tsx diff --git a/x-pack/plugins/uptime/public/components/overview/alerts/monitor_expressions/filters_expression_select.test.tsx b/x-pack/plugins/uptime/public/components/overview/alerts/monitor_expressions/filters_expression_select.test.tsx index 3ebd828604463..21b1a067a4e9d 100644 --- a/x-pack/plugins/uptime/public/components/overview/alerts/monitor_expressions/filters_expression_select.test.tsx +++ b/x-pack/plugins/uptime/public/components/overview/alerts/monitor_expressions/filters_expression_select.test.tsx @@ -6,9 +6,18 @@ import React from 'react'; import { shallowWithIntl } from '@kbn/test/jest'; +import { fireEvent, waitFor } from '@testing-library/react'; import { FiltersExpressionsSelect } from './filters_expression_select'; +import { render } from '../../../../lib/helper/rtl_helpers'; +import { filterAriaLabels as aria } from './translations'; +import { filterLabels } from '../../filter_group/translations'; + +describe('FiltersExpressionSelect', () => { + const LOCATION_FIELD_NAME = 'observer.geo.name'; + const PORT_FIELD_NAME = 'url.port'; + const SCHEME_FIELD_NAME = 'monitor.type'; + const TAG_FIELD_NAME = 'tags'; -describe('filters expression select component', () => { it('is empty when no filters available', () => { const component = shallowWithIntl( { `); }); - it('contains provided new filter values', () => { - const component = shallowWithIntl( + it.each([ + [[LOCATION_FIELD_NAME], [aria.LOCATION], [aria.TAG, aria.PORT, aria.SCHEME]], + [ + [LOCATION_FIELD_NAME, TAG_FIELD_NAME], + [aria.LOCATION, aria.TAG], + [aria.PORT, aria.SCHEME], + ], + [ + [PORT_FIELD_NAME, SCHEME_FIELD_NAME], + [aria.PORT, aria.SCHEME], + [aria.LOCATION, aria.TAG], + ], + [[TAG_FIELD_NAME], [aria.TAG], [aria.LOCATION, aria.PORT, aria.SCHEME]], + ])('contains provided new filter values', (newFilters, expectedLabels, absentLabels) => { + const { getByLabelText, queryByLabelText } = render( { shouldUpdateUrl={false} /> ); - expect(component).toMatchInlineSnapshot(` - - - - - } - disabled={true} - fieldName="observer.geo.name" - forceOpen={false} - id="filter_location" - items={Array []} - loading={false} - onFilterFieldChange={[Function]} - selectedItems={Array []} - setForceOpen={[Function]} - title="Scheme" - /> - - - - - - - - - `); + expectedLabels.forEach((label) => expect(getByLabelText(label))); + absentLabels.forEach((label) => expect(queryByLabelText(label)).toBeNull()); }); - it('contains provided selected filter values', () => { - const component = shallowWithIntl( + it.each([ + ['Remove filter Location', LOCATION_FIELD_NAME], + ['Remove filter Scheme', SCHEME_FIELD_NAME], + ['Remove filter Port', PORT_FIELD_NAME], + ['Remove filter Tag', TAG_FIELD_NAME], + ])('fires remove filter handler', async (removeButtonLabel, expectedFieldName) => { + const onRemoveFilterMock = jest.fn(); + const setAlertParamsMock = jest.fn(); + const { getByLabelText } = render( ); - expect(component).toMatchInlineSnapshot(` - - - - - } - disabled={false} - fieldName="tags" - forceOpen={false} - id="filter_tags" - items={ - Array [ - "foo", - "bar", - ] - } - loading={false} - onFilterFieldChange={[Function]} - selectedItems={Array []} - setForceOpen={[Function]} - title="Tags" - /> - - - - - - - - - `); + + const removeButton = getByLabelText(removeButtonLabel); + fireEvent.click(removeButton); + expect(onRemoveFilterMock).toHaveBeenCalledTimes(1); + expect(onRemoveFilterMock).toHaveBeenCalledWith(expectedFieldName); + expect(setAlertParamsMock).toHaveBeenCalledTimes(1); + expect(setAlertParamsMock).toHaveBeenCalledWith('filters', { + [SCHEME_FIELD_NAME]: [], + [LOCATION_FIELD_NAME]: [], + [TAG_FIELD_NAME]: [], + [PORT_FIELD_NAME]: [], + }); }); + + const TEST_TAGS = ['foo', 'bar']; + const TEST_PORTS = [5601, 9200]; + const TEST_SCHEMES = ['http', 'tcp']; + const TEST_LOCATIONS = ['nyc', 'fairbanks']; + + it.each([ + [ + { + tags: TEST_TAGS, + ports: [5601, 9200], + schemes: ['http', 'tcp'], + locations: ['nyc', 'fairbanks'], + }, + [TAG_FIELD_NAME], + aria.TAG, + filterLabels.TAG, + TEST_TAGS, + ], + [ + { + tags: [], + ports: TEST_PORTS, + schemes: [], + locations: [], + }, + [PORT_FIELD_NAME], + aria.PORT, + filterLabels.PORT, + TEST_PORTS, + ], + [ + { + tags: [], + ports: [], + schemes: TEST_SCHEMES, + locations: [], + }, + [SCHEME_FIELD_NAME], + aria.SCHEME, + filterLabels.SCHEME, + TEST_SCHEMES, + ], + [ + { + tags: [], + ports: [], + schemes: [], + locations: TEST_LOCATIONS, + }, + [LOCATION_FIELD_NAME], + aria.LOCATION, + filterLabels.LOCATION, + TEST_LOCATIONS, + ], + ])( + 'applies accessible label to filter expressions, and contains selected filters', + /** + * @param filters the set of filters the test should supply as props + * @param newFilters the set of filter item types the component should render + * @param expectedFilterButtonAriaLabel the aria label for the popover button for the targeted filter + * @param filterLabel the name of the filter label expected in each item's aria-label + * @param expectedFilterItems the set of filter options the component should render + */ + async ( + filters, + newFilters, + expectedFilterButtonAriaLabel, + filterLabel, + expectedFilterItems + ) => { + const { getByLabelText } = render( + + ); + + const filterButton = getByLabelText(expectedFilterButtonAriaLabel); + + fireEvent.click(filterButton); + + await waitFor(() => { + expectedFilterItems.forEach((filterItem: string | number) => + expect(getByLabelText(`Filter by ${filterLabel} ${filterItem}.`)) + ); + }); + } + ); }); diff --git a/x-pack/plugins/uptime/public/components/overview/alerts/monitor_expressions/filters_expression_select.tsx b/x-pack/plugins/uptime/public/components/overview/alerts/monitor_expressions/filters_expression_select.tsx index 64862a8b748d8..63dcf31fd8111 100644 --- a/x-pack/plugins/uptime/public/components/overview/alerts/monitor_expressions/filters_expression_select.tsx +++ b/x-pack/plugins/uptime/public/components/overview/alerts/monitor_expressions/filters_expression_select.tsx @@ -8,7 +8,7 @@ import React, { useState } from 'react'; import { EuiButtonIcon, EuiExpression, EuiFlexGroup, EuiFlexItem, EuiSpacer } from '@elastic/eui'; import { FilterPopover } from '../../filter_group/filter_popover'; import { filterLabels } from '../../filter_group/translations'; -import { alertFilterLabels } from './translations'; +import { alertFilterLabels, filterAriaLabels } from './translations'; import { FilterExpressionsSelectProps } from './filters_expression_select_container'; import { OverviewFiltersState } from '../../../../state/reducers/overview_filters'; @@ -58,6 +58,7 @@ export const FiltersExpressionsSelect: React.FC = ({ const monitorFilters = [ { + 'aria-label': filterAriaLabels.PORT, onFilterFieldChange, loading: false, fieldName: 'url.port', @@ -71,6 +72,7 @@ export const FiltersExpressionsSelect: React.FC = ({ value: selectedPorts.length === 0 ? alertFilterLabels.ANY_PORT : selectedPorts?.join(','), }, { + 'aria-label': filterAriaLabels.TAG, onFilterFieldChange, loading: false, fieldName: 'tags', @@ -78,11 +80,12 @@ export const FiltersExpressionsSelect: React.FC = ({ disabled: tags?.length === 0, items: tags ?? [], selectedItems: selectedTags, - title: filterLabels.TAGS, + title: filterLabels.TAG, description: selectedTags.length === 0 ? alertFilterLabels.WITH : alertFilterLabels.WITH_TAG, value: selectedTags.length === 0 ? alertFilterLabels.ANY_TAG : selectedTags?.join(','), }, { + 'aria-label': filterAriaLabels.SCHEME, onFilterFieldChange, loading: false, fieldName: 'monitor.type', @@ -95,6 +98,7 @@ export const FiltersExpressionsSelect: React.FC = ({ value: selectedSchemes.length === 0 ? alertFilterLabels.ANY_TYPE : selectedSchemes?.join(','), }, { + 'aria-label': filterAriaLabels.LOCATION, onFilterFieldChange, loading: false, fieldName: 'observer.geo.name', @@ -102,7 +106,7 @@ export const FiltersExpressionsSelect: React.FC = ({ disabled: locations?.length === 0, items: locations ?? [], selectedItems: selectedLocations, - title: filterLabels.SCHEME, + title: filterLabels.LOCATION, description: selectedLocations.length === 0 ? alertFilterLabels.FROM : alertFilterLabels.FROM_LOCATION, value: @@ -132,7 +136,7 @@ export const FiltersExpressionsSelect: React.FC = ({ {...item} btnContent={ = ({ { diff --git a/x-pack/plugins/uptime/public/components/overview/alerts/monitor_expressions/translations.ts b/x-pack/plugins/uptime/public/components/overview/alerts/monitor_expressions/translations.ts index 64c082dc51334..919095d411fae 100644 --- a/x-pack/plugins/uptime/public/components/overview/alerts/monitor_expressions/translations.ts +++ b/x-pack/plugins/uptime/public/components/overview/alerts/monitor_expressions/translations.ts @@ -54,6 +54,12 @@ export const alertFilterLabels = { ANY_LOCATION: i18n.translate('xpack.uptime.alerts.monitorStatus.filters.anyLocation', { defaultMessage: 'any location', }), + + REMOVE_FILTER_LABEL: (title: string) => + i18n.translate('xpack.uptime.alerts.monitorExpression.label', { + defaultMessage: 'Remove filter {title}', + values: { title }, + }), }; export const statusExpLabels = { @@ -82,3 +88,18 @@ export const timeExpLabels = { } ), }; + +export const filterAriaLabels = { + PORT: i18n.translate('xpack.uptime.alerts.monitorStatus.filters.port.label', { + defaultMessage: `Select port filters to apply to the alert's query.`, + }), + TAG: i18n.translate('xpack.uptime.alerts.monitorStatus.filters.tag.label', { + defaultMessage: `Select tag filters to apply to the alert's query.`, + }), + SCHEME: i18n.translate('xpack.uptime.alerts.monitorStatus.filters.scheme.label', { + defaultMessage: `Select protocol scheme filters to apply to the alert's query.`, + }), + LOCATION: i18n.translate('xpack.uptime.alerts.monitorStatus.filters.location.label', { + defaultMessage: `Select location filters to apply to the alert's query.`, + }), +}; diff --git a/x-pack/plugins/uptime/public/components/overview/filter_group/__snapshots__/filter_popover.test.tsx.snap b/x-pack/plugins/uptime/public/components/overview/filter_group/__snapshots__/filter_popover.test.tsx.snap deleted file mode 100644 index 47efe8946d0b6..0000000000000 --- a/x-pack/plugins/uptime/public/components/overview/filter_group/__snapshots__/filter_popover.test.tsx.snap +++ /dev/null @@ -1,129 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`FilterPopover component does not show item list when loading 1`] = ` - - } - closePopover={[Function]} - data-test-subj="filter-popover_test" - display="inlineBlock" - hasArrow={true} - id="test" - isOpen={false} - ownFocus={true} - panelPaddingSize="m" - zIndex={10000} -> - - - - -`; - -exports[`FilterPopover component renders without errors for valid props 1`] = ` - - } - closePopover={[Function]} - data-test-subj="filter-popover_test" - display="inlineBlock" - hasArrow={true} - id="test" - isOpen={false} - ownFocus={true} - panelPaddingSize="m" - zIndex={10000} -> - - - - -`; - -exports[`FilterPopover component returns selected items on popover close 1`] = ` -
-
- Some text -
-
-
- -
-
-
-`; diff --git a/x-pack/plugins/uptime/public/components/overview/filter_group/filter_group.test.tsx b/x-pack/plugins/uptime/public/components/overview/filter_group/filter_group.test.tsx new file mode 100644 index 0000000000000..0fc413011fa51 --- /dev/null +++ b/x-pack/plugins/uptime/public/components/overview/filter_group/filter_group.test.tsx @@ -0,0 +1,73 @@ +/* + * 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 { fireEvent, waitFor } from '@testing-library/react'; +import { render } from '../../../lib/helper/rtl_helpers'; +import { FilterGroupComponent } from './filter_group'; + +describe('FilterGroupComponent', () => { + const overviewFilters = { + locations: ['nyc', 'fairbanks'], + ports: [5601, 9200], + schemes: ['http', 'tcp'], + tags: ['prod', 'dev'], + }; + it.each([ + ['expands filter group for Location filter', 'Search for location'], + ['expands filter group for Port filter', 'Search for port'], + ['expands filter group for Scheme filter', 'Search for scheme'], + ['expands filter group for Tag filter', 'Search for tag'], + ])('handles loading', async (popoverButtonLabel, searchInputLabel) => { + const { getByLabelText } = render( + + ); + + const popoverButton = getByLabelText(popoverButtonLabel); + fireEvent.click(popoverButton); + await waitFor(() => { + const searchInput = getByLabelText(searchInputLabel); + expect(searchInput).toHaveAttribute('placeholder', 'Loading...'); + }); + }); + + it.each([ + [ + 'expands filter group for Location filter', + 'Search for location', + ['Filter by Location nyc.', 'Filter by Location fairbanks.'], + ], + [ + 'expands filter group for Port filter', + 'Search for port', + ['Filter by Port 5601.', 'Filter by Port 9200.'], + ], + [ + 'expands filter group for Scheme filter', + 'Search for scheme', + ['Filter by Scheme http.', 'Filter by Scheme tcp.'], + ], + [ + 'expands filter group for Tag filter', + 'Search for tag', + ['Filter by Tag prod.', 'Filter by Tag dev.'], + ], + ])( + 'displays filter items when clicked', + async (popoverButtonLabel, searchInputLabel, filterItemButtonLabels) => { + const { getByLabelText } = render( + + ); + + const popoverButton = getByLabelText(popoverButtonLabel); + fireEvent.click(popoverButton); + await waitFor(() => { + expect(getByLabelText(searchInputLabel)); + filterItemButtonLabels.forEach((itemLabel) => expect(getByLabelText(itemLabel))); + }); + } + ); +}); diff --git a/x-pack/plugins/uptime/public/components/overview/filter_group/filter_group.tsx b/x-pack/plugins/uptime/public/components/overview/filter_group/filter_group.tsx index a5256ee1e2626..a4a03ec5587a0 100644 --- a/x-pack/plugins/uptime/public/components/overview/filter_group/filter_group.tsx +++ b/x-pack/plugins/uptime/public/components/overview/filter_group/filter_group.tsx @@ -15,7 +15,7 @@ import { useFilterUpdate } from '../../../hooks/use_filter_update'; import { MONITOR_ROUTE } from '../../../../common/constants'; import { useSelectedFilters } from '../../../hooks/use_selected_filters'; -interface PresentationalComponentProps { +interface Props { loading: boolean; overviewFilters: OverviewFilters; } @@ -28,10 +28,7 @@ function isDisabled(array?: T[]) { return array ? array.length === 0 : true; } -export const FilterGroupComponent: React.FC = ({ - overviewFilters, - loading, -}) => { +export const FilterGroupComponent: React.FC = ({ overviewFilters, loading }) => { const { locations, ports, schemes, tags } = overviewFilters; const [updatedFieldValues, setUpdatedFieldValues] = useState<{ @@ -90,7 +87,7 @@ export const FilterGroupComponent: React.FC = ({ disabled: isDisabled(tags), items: tags ?? [], selectedItems: selectedTags, - title: filterLabels.TAGS, + title: filterLabels.TAG, }, ] : []), diff --git a/x-pack/plugins/uptime/public/components/overview/filter_group/filter_popover.test.tsx b/x-pack/plugins/uptime/public/components/overview/filter_group/filter_popover.test.tsx index d08d56dc2e2a0..dc45077901c10 100644 --- a/x-pack/plugins/uptime/public/components/overview/filter_group/filter_popover.test.tsx +++ b/x-pack/plugins/uptime/public/components/overview/filter_group/filter_popover.test.tsx @@ -5,23 +5,22 @@ */ import React from 'react'; -import { shallowWithIntl, mountWithIntl } from '@kbn/test/jest'; +import { fireEvent, waitFor, waitForElementToBeRemoved } from '@testing-library/react'; import { FilterPopoverProps, FilterPopover } from './filter_popover'; -import { UptimeFilterButton } from './uptime_filter_button'; -import { EuiFilterSelectItem } from '@elastic/eui'; +import { render } from '../../../lib/helper/rtl_helpers'; describe('FilterPopover component', () => { let props: FilterPopoverProps; beforeEach(() => { props = { - fieldName: 'foo', + fieldName: 'test-fieldName', id: 'test', loading: false, items: ['first', 'second', 'third', 'fourth'], onFilterFieldChange: jest.fn(), selectedItems: ['first', 'third'], - title: 'bar', + title: 'test-title', }; }); @@ -29,43 +28,68 @@ describe('FilterPopover component', () => { jest.clearAllMocks(); }); - it('renders without errors for valid props', () => { - const wrapper = shallowWithIntl(); - expect(wrapper).toMatchSnapshot(); - }); - it('expands on button click', () => { - const wrapper = mountWithIntl(); + const { getByRole, getByLabelText, getByText, queryByLabelText, queryByText } = render( + + ); - // ensure the popover isn't open - expect(wrapper.find('EuiPopoverTitle')).toHaveLength(0); + const screenReaderOnlyText = 'You are in a dialog. To close this dialog, hit escape.'; - expect(wrapper.find(UptimeFilterButton)).toHaveLength(1); - wrapper.find(UptimeFilterButton).simulate('click'); + expect(queryByText(screenReaderOnlyText)).toBeNull(); + expect(queryByLabelText('Filter by bar fourth.')).toBeNull(); - // check that the popover is now open - expect(wrapper.find('EuiPopoverTitle')).toHaveLength(1); + fireEvent.click(getByRole('button')); + + expect(getByText(screenReaderOnlyText)); + expect(getByLabelText('Filter by test-title fourth.')); }); - it('does not show item list when loading', () => { + it('does not show item list when loading, and displays placeholder', async () => { props.loading = true; - const wrapper = shallowWithIntl(); - expect(wrapper).toMatchSnapshot(); - }); + const { getByRole, queryByText, getByLabelText } = render(); - it('returns selected items on popover close', () => { - const wrapper = mountWithIntl( -
-
Some text
- -
- ); - expect(wrapper.find(UptimeFilterButton)).toHaveLength(1); - wrapper.find(UptimeFilterButton).simulate('click'); - expect(wrapper.find(EuiFilterSelectItem)).toHaveLength(props.items.length); - wrapper.find(EuiFilterSelectItem).at(1).simulate('click'); - wrapper.find('#foo').simulate('click'); - const rendered = wrapper.render(); - expect(rendered).toMatchSnapshot(); + fireEvent.click(getByRole('button')); + + await waitFor(() => { + const search = getByLabelText('Search for test-title'); + expect(search).toHaveAttribute('placeholder', 'Loading...'); + }); + + expect(queryByText('Filter by test-title second.')).toBeNull(); }); + + it.each([ + [[], ['third'], ['third']], + [['first', 'third'], ['first'], ['third']], + [['fourth'], ['first', 'second'], ['first', 'second', 'fourth']], + ])( + 'returns selected items on popover close', + async (selectedPropsItems, expectedSelections, itemsToClick) => { + if (itemsToClick.length < 1) { + throw new Error('This test assumes at least one item will be clicked'); + } + props.selectedItems = selectedPropsItems; + + const { getByLabelText, queryByLabelText } = render(); + + const uptimeFilterButton = getByLabelText(`expands filter group for ${props.title} filter`); + + fireEvent.click(uptimeFilterButton); + + const generateLabelText = (item: string) => `Filter by ${props.title} ${item}.`; + + itemsToClick.forEach((item) => { + const optionButtonLabelText = generateLabelText(item); + const optionButton = getByLabelText(optionButtonLabelText); + fireEvent.click(optionButton); + }); + + fireEvent.click(uptimeFilterButton); + + await waitForElementToBeRemoved(() => queryByLabelText(generateLabelText(itemsToClick[0]))); + + expect(props.onFilterFieldChange).toHaveBeenCalledTimes(1); + expect(props.onFilterFieldChange).toHaveBeenCalledWith(props.fieldName, expectedSelections); + } + ); }); diff --git a/x-pack/plugins/uptime/public/components/overview/filter_group/filter_popover.tsx b/x-pack/plugins/uptime/public/components/overview/filter_group/filter_popover.tsx index e79c036d54e0e..d5b6efbf9ded7 100644 --- a/x-pack/plugins/uptime/public/components/overview/filter_group/filter_popover.tsx +++ b/x-pack/plugins/uptime/public/components/overview/filter_group/filter_popover.tsx @@ -76,8 +76,11 @@ export const FilterPopover = ({ numFilters={items.length} numActiveFilters={isOpen ? tempSelectedItems.length : selectedItems.length} onClick={() => { + if (isOpen) { + // only update these values on close + onFilterFieldChange(fieldName, tempSelectedItems); + } setIsOpen(!isOpen); - onFilterFieldChange(fieldName, tempSelectedItems); }} title={title} /> @@ -124,6 +127,10 @@ export const FilterPopover = ({ {!loading && itemsToDisplay.map((item) => ( (