From b6cd765840c4524e00102997eaf3d5e8298481c2 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 2 Feb 2023 16:03:00 -0800 Subject: [PATCH 1/3] Fix broken behavior for `multiSelect: false` filters - the diff being returned was catching on the first item (now unflagged) and sending that instead - instead of using an unnecessary `.find`, use our new 3rd `changedOption` arg from `EuiSelectable` to immediately get the changed/selected option - nb: this does require flipping all ternaries on `onOptionClick` b/c the new `changedOption` correctly sends the new updated state --- .../filters/field_value_selection_filter.tsx | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/components/search_bar/filters/field_value_selection_filter.tsx b/src/components/search_bar/filters/field_value_selection_filter.tsx index d908c24884f..16b17e82831 100644 --- a/src/components/search_bar/filters/field_value_selection_filter.tsx +++ b/src/components/search_bar/filters/field_value_selection_filter.tsx @@ -259,23 +259,23 @@ export class FieldValueSelectionFilter extends Component< if (!multiSelect && autoClose) { this.closePopover(); const query = checked - ? this.props.query.removeSimpleFieldClauses(field) - : this.props.query + ? this.props.query .removeSimpleFieldClauses(field) - .addSimpleFieldValue(field, value, true, operator); + .addSimpleFieldValue(field, value, true, operator) + : this.props.query.removeSimpleFieldClauses(field); this.props.onChange(query); } else { if (multiSelect === 'or') { const query = checked - ? this.props.query.removeOrFieldValue(field, value) - : this.props.query.addOrFieldValue(field, value, true, operator); + ? this.props.query.addOrFieldValue(field, value, true, operator) + : this.props.query.removeOrFieldValue(field, value); this.props.onChange(query); } else { const query = checked - ? this.props.query.removeSimpleFieldValue(field, value) - : this.props.query.addSimpleFieldValue(field, value, true, operator); + ? this.props.query.addSimpleFieldValue(field, value, true, operator) + : this.props.query.removeSimpleFieldValue(field, value); this.props.onChange(query); } @@ -404,15 +404,12 @@ export class FieldValueSelectionFilter extends Component< listProps={{ isVirtualized: isOverSearchThreshold || false, }} - onChange={(options) => { - const diff = items.find( - (item, index) => item.checked !== options[index].checked - ); - if (diff) { + onChange={(options, event, changedOption) => { + if (changedOption.data) { this.onOptionClick( - diff.data.optionField, - diff.data.value, - diff.checked + changedOption.data.optionField, + changedOption.data.value, + changedOption.checked ); } }} From 17bca045d94654c56c6c9d09e55a3d552db887d0 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 2 Feb 2023 16:08:32 -0800 Subject: [PATCH 2/3] Add regression tests for both multiselect false/or/true behavior --- .../field_value_selection_filter.spec.tsx | 109 ++++++++++++++---- 1 file changed, 85 insertions(+), 24 deletions(-) diff --git a/src/components/search_bar/filters/field_value_selection_filter.spec.tsx b/src/components/search_bar/filters/field_value_selection_filter.spec.tsx index 23181d23e45..4e196c2c40b 100644 --- a/src/components/search_bar/filters/field_value_selection_filter.spec.tsx +++ b/src/components/search_bar/filters/field_value_selection_filter.spec.tsx @@ -6,7 +6,9 @@ * Side Public License, v 1. */ -import React from 'react'; +/// + +import React, { useState } from 'react'; import { requiredProps } from '../../../test'; import { FieldValueSelectionFilter, @@ -135,31 +137,90 @@ describe('FieldValueSelectionFilter', () => { ); }); - it('uses multi-select OR', () => { - const props: FieldValueSelectionFilterProps = { - ...requiredProps, - index: 0, - onChange: () => {}, - query: Query.parse(''), - config: { - type: 'field_value_selection', - field: 'tag', - name: 'Tag', - multiSelect: 'or', - available: () => false, - loadingMessage: 'loading...', - noOptionsMessage: 'oops...', - searchThreshold: 5, - options: () => Promise.resolve([]), - }, + describe('multi-select testing', () => { + const FieldValueSelectionFilterWithState = ({ + multiSelect, + }: { + multiSelect: 'or' | boolean; + }) => { + const [query, setQuery] = useState(Query.parse('')); + const onChange = (newQuery) => setQuery(newQuery); + + const props: FieldValueSelectionFilterProps = { + ...requiredProps, + index: 0, + onChange, + query, + config: { + type: 'field_value_selection', + field: 'tag', + name: 'Tag', + multiSelect, + options: staticOptions, + }, + }; + + return ; }; - cy.mount(); - cy.get('button').click(); - cy.get('[data-test-subj="euiSelectableMessage"]').should( - 'have.text', - 'oops...' - ); + it('uses multi-select OR', () => { + cy.mount(); + cy.get('button').click(); + + cy.get('li[role="option"][title="feature"]') + .should('have.attr', 'aria-checked', 'false') + .click(); + cy.get('.euiNotificationBadge').should('have.text', '1'); + cy.get('li[role="option"][title="feature"]').should( + 'have.attr', + 'aria-checked', + 'true' + ); + // Popover should still be open when multiselect is true/or + cy.get('li[role="option"][title="Bug"]') + .should('have.attr', 'aria-checked', 'false') + .click(); + cy.get('.euiNotificationBadge').should('have.text', '2'); + cy.get('li[role="option"][title="Bug"]').should( + 'have.attr', + 'aria-checked', + 'true' + ); + }); + + it('uses multi-select false', () => { + cy.mount(); + cy.get('button').click(); + + cy.get('li[role="option"][title="feature"]') + .should('have.attr', 'aria-checked', 'false') + .click(); + cy.get('.euiNotificationBadge').should('have.text', '1'); + cy.get('li[role="option"][title="feature"]').should( + 'have.attr', + 'aria-checked', + 'true' + ); + + // Multiselect false should close the popover, so we need to re-open it + cy.get('button').click(); + cy.get('li[role="option"][title="Bug"]') + .should('have.attr', 'aria-checked', 'false') + .click(); + // Filter count should have remained at 1 + cy.get('.euiNotificationBadge').should('have.text', '1'); + cy.get('li[role="option"][title="Bug"]').should( + 'have.attr', + 'aria-checked', + 'true' + ); + // 'featured' should now be unchecked + cy.get('li[role="option"][title="feature"]').should( + 'have.attr', + 'aria-checked', + 'false' + ); + }); }); it('has inactive filters, field is global', () => { From 4bf0e18eb9d879260e1c882a2baf7e9a28cdeaf5 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 2 Feb 2023 16:20:16 -0800 Subject: [PATCH 3/3] changelog --- upcoming_changelogs/6577.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 upcoming_changelogs/6577.md diff --git a/upcoming_changelogs/6577.md b/upcoming_changelogs/6577.md new file mode 100644 index 00000000000..977b7164592 --- /dev/null +++ b/upcoming_changelogs/6577.md @@ -0,0 +1,3 @@ +**Bug fixes** + +- Fixed a bug with `EuiSearchBar` where filters with `multiSelect: false` were not able to select a new option when an option was already selected