From 49d4a66308eabf56dfacc5af6f04615f5e4c827c Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Thu, 15 Sep 2022 10:24:06 -0300 Subject: [PATCH 1/2] fix: Duplicated numeric values in Select --- .../src/components/Select/AsyncSelect.tsx | 79 ++++++++++++------- .../src/components/Select/Select.tsx | 51 +++++++----- .../src/components/Select/utils.tsx | 16 ++-- 3 files changed, 90 insertions(+), 56 deletions(-) diff --git a/superset-frontend/src/components/Select/AsyncSelect.tsx b/superset-frontend/src/components/Select/AsyncSelect.tsx index e3118f228498d..96650110874ce 100644 --- a/superset-frontend/src/components/Select/AsyncSelect.tsx +++ b/superset-frontend/src/components/Select/AsyncSelect.tsx @@ -34,7 +34,7 @@ import debounce from 'lodash/debounce'; import { isEqual } from 'lodash'; import Icons from 'src/components/Icons'; import { getClientErrorObject } from 'src/utils/getClientErrorObject'; -import { SLOW_DEBOUNCE } from 'src/constants'; +import { FAST_DEBOUNCE, SLOW_DEBOUNCE } from 'src/constants'; import { getValue, hasOption, @@ -369,34 +369,53 @@ const AsyncSelect = forwardRef( [fetchPage], ); - const handleOnSearch = (search: string) => { - const searchValue = search.trim(); - if (allowNewOptions && isSingleMode) { - const newOption = searchValue && - !hasOption(searchValue, fullSelectOptions, true) && { - label: searchValue, - value: searchValue, - isNewOption: true, - }; - const cleanSelectOptions = fullSelectOptions.filter( - opt => !opt.isNewOption || hasOption(opt.value, selectValue), - ); - const newOptions = newOption - ? [newOption, ...cleanSelectOptions] - : cleanSelectOptions; - setSelectOptions(newOptions); - } - if ( - !allValuesLoaded && - loadingEnabled && - !fetchedQueries.current.has(getQueryCacheKey(searchValue, 0, pageSize)) - ) { - // if fetch only on search but search value is empty, then should not be - // in loading state - setIsLoading(!(fetchOnlyOnSearch && !searchValue)); - } - setInputValue(search); - }; + const handleOnSearch = useCallback( + (search: string) => { + const searchValue = search.trim(); + if (allowNewOptions && isSingleMode) { + const newOption = searchValue && + !hasOption(searchValue, fullSelectOptions, true) && { + label: searchValue, + value: searchValue, + isNewOption: true, + }; + const cleanSelectOptions = fullSelectOptions.filter( + opt => !opt.isNewOption || hasOption(opt.value, selectValue), + ); + const newOptions = newOption + ? [newOption, ...cleanSelectOptions] + : cleanSelectOptions; + setSelectOptions(newOptions); + } + if ( + !allValuesLoaded && + loadingEnabled && + !fetchedQueries.current.has( + getQueryCacheKey(searchValue, 0, pageSize), + ) + ) { + // if fetch only on search but search value is empty, then should not be + // in loading state + setIsLoading(!(fetchOnlyOnSearch && !searchValue)); + } + setInputValue(search); + }, + [ + allValuesLoaded, + allowNewOptions, + fetchOnlyOnSearch, + fullSelectOptions, + isSingleMode, + loadingEnabled, + pageSize, + selectValue, + ], + ); + + const debouncedOnSearch = useMemo( + () => debounce(handleOnSearch, FAST_DEBOUNCE), + [handleOnSearch], + ); const handlePagination = (e: UIEvent) => { const vScroll = e.currentTarget; @@ -535,7 +554,7 @@ const AsyncSelect = forwardRef( onDeselect={handleOnDeselect} onDropdownVisibleChange={handleOnDropdownVisibleChange} onPopupScroll={handlePagination} - onSearch={showSearch ? handleOnSearch : undefined} + onSearch={showSearch ? debouncedOnSearch : undefined} onSelect={handleOnSelect} onClear={handleClear} onChange={onChange} diff --git a/superset-frontend/src/components/Select/Select.tsx b/superset-frontend/src/components/Select/Select.tsx index 6b6713852d542..4f964b4c9b020 100644 --- a/superset-frontend/src/components/Select/Select.tsx +++ b/superset-frontend/src/components/Select/Select.tsx @@ -27,7 +27,8 @@ import React, { } from 'react'; import { ensureIsArray, t } from '@superset-ui/core'; import { LabeledValue as AntdLabeledValue } from 'antd/lib/select'; -import { isEqual } from 'lodash'; +import { debounce, isEqual } from 'lodash'; +import { FAST_DEBOUNCE } from 'src/constants'; import { getValue, hasOption, @@ -198,25 +199,33 @@ const Select = forwardRef( setInputValue(''); }; - const handleOnSearch = (search: string) => { - const searchValue = search.trim(); - if (allowNewOptions && isSingleMode) { - const newOption = searchValue && - !hasOption(searchValue, fullSelectOptions, true) && { - label: searchValue, - value: searchValue, - isNewOption: true, - }; - const cleanSelectOptions = fullSelectOptions.filter( - opt => !opt.isNewOption || hasOption(opt.value, selectValue), - ); - const newOptions = newOption - ? [newOption, ...cleanSelectOptions] - : cleanSelectOptions; - setSelectOptions(newOptions); - } - setInputValue(search); - }; + const handleOnSearch = useCallback( + (search: string) => { + const searchValue = search.trim(); + if (allowNewOptions && isSingleMode) { + const newOption = searchValue && + !hasOption(searchValue, fullSelectOptions, true) && { + label: searchValue, + value: searchValue, + isNewOption: true, + }; + const cleanSelectOptions = fullSelectOptions.filter( + opt => !opt.isNewOption || hasOption(opt.value, selectValue), + ); + const newOptions = newOption + ? [newOption, ...cleanSelectOptions] + : cleanSelectOptions; + setSelectOptions(newOptions); + } + setInputValue(search); + }, + [allowNewOptions, fullSelectOptions, isSingleMode, selectValue], + ); + + const debouncedOnSearch = useMemo( + () => debounce(handleOnSearch, FAST_DEBOUNCE), + [handleOnSearch], + ); const handleFilterOption = (search: string, option: AntdLabeledValue) => handleFilterOptionHelper(search, option, optionFilterProps, filterOption); @@ -288,7 +297,7 @@ const Select = forwardRef( onDeselect={handleOnDeselect} onDropdownVisibleChange={handleOnDropdownVisibleChange} onPopupScroll={undefined} - onSearch={shouldShowSearch ? handleOnSearch : undefined} + onSearch={shouldShowSearch ? debouncedOnSearch : undefined} onSelect={handleOnSelect} onClear={handleClear} onChange={onChange} diff --git a/superset-frontend/src/components/Select/utils.tsx b/superset-frontend/src/components/Select/utils.tsx index 9916ef07e2698..27a97ac8bd466 100644 --- a/superset-frontend/src/components/Select/utils.tsx +++ b/superset-frontend/src/components/Select/utils.tsx @@ -96,20 +96,26 @@ export function getValue( return isLabeledValue(option) ? option.value : option; } -type LabeledValue = { label?: ReactNode; value?: V }; +type V = string | number | null | undefined; -export function hasOption( +type LabeledValue = { label?: ReactNode; value?: V }; + +export function hasOption( value: V, - options?: V | LabeledValue | (V | LabeledValue)[], + options?: V | LabeledValue | (V | LabeledValue)[], checkLabel = false, ): boolean { const optionsArray = ensureIsArray(options); + // When comparing the values we use the equality + // operator to automatically convert different types return ( optionsArray.find( x => - x === value || + // eslint-disable-next-line eqeqeq + x == value || (isObject(x) && - (('value' in x && x.value === value) || + // eslint-disable-next-line eqeqeq + (('value' in x && x.value == value) || (checkLabel && 'label' in x && x.label === value))), ) !== undefined ); From b1028d95f4eee771cc4838bf38f55335335790b7 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Thu, 15 Sep 2022 16:02:57 -0300 Subject: [PATCH 2/2] Adds tests --- .../components/Select/AsyncSelect.test.tsx | 20 +++++ .../src/components/Select/AsyncSelect.tsx | 79 +++++++------------ .../src/components/Select/Select.test.tsx | 17 ++++ .../src/components/Select/Select.tsx | 51 +++++------- 4 files changed, 88 insertions(+), 79 deletions(-) diff --git a/superset-frontend/src/components/Select/AsyncSelect.test.tsx b/superset-frontend/src/components/Select/AsyncSelect.test.tsx index e5569f1be0fc4..b11dcde017c48 100644 --- a/superset-frontend/src/components/Select/AsyncSelect.test.tsx +++ b/superset-frontend/src/components/Select/AsyncSelect.test.tsx @@ -98,6 +98,11 @@ const findSelectOption = (text: string) => within(getElementByClassName('.rc-virtual-list')).getByText(text), ); +const querySelectOption = (text: string) => + waitFor(() => + within(getElementByClassName('.rc-virtual-list')).queryByText(text), + ); + const findAllSelectOptions = () => waitFor(() => getElementsByClassName('.ant-select-item-option-content')); @@ -736,6 +741,21 @@ test('renders a helper text when one is provided', async () => { expect(screen.queryByText(helperText)).toBeInTheDocument(); }); +test('finds an element with a numeric value and does not duplicate the options', async () => { + const options = jest.fn(async () => ({ + data: [ + { label: 'a', value: 11 }, + { label: 'b', value: 12 }, + ], + totalCount: 2, + })); + render(); + await open(); + await type('11'); + expect(await findSelectOption('a')).toBeInTheDocument(); + expect(await querySelectOption('11')).not.toBeInTheDocument(); +}); + /* TODO: Add tests that require scroll interaction. Needs further investigation. - Fetches more data when scrolling and more data is available diff --git a/superset-frontend/src/components/Select/AsyncSelect.tsx b/superset-frontend/src/components/Select/AsyncSelect.tsx index 96650110874ce..e3118f228498d 100644 --- a/superset-frontend/src/components/Select/AsyncSelect.tsx +++ b/superset-frontend/src/components/Select/AsyncSelect.tsx @@ -34,7 +34,7 @@ import debounce from 'lodash/debounce'; import { isEqual } from 'lodash'; import Icons from 'src/components/Icons'; import { getClientErrorObject } from 'src/utils/getClientErrorObject'; -import { FAST_DEBOUNCE, SLOW_DEBOUNCE } from 'src/constants'; +import { SLOW_DEBOUNCE } from 'src/constants'; import { getValue, hasOption, @@ -369,53 +369,34 @@ const AsyncSelect = forwardRef( [fetchPage], ); - const handleOnSearch = useCallback( - (search: string) => { - const searchValue = search.trim(); - if (allowNewOptions && isSingleMode) { - const newOption = searchValue && - !hasOption(searchValue, fullSelectOptions, true) && { - label: searchValue, - value: searchValue, - isNewOption: true, - }; - const cleanSelectOptions = fullSelectOptions.filter( - opt => !opt.isNewOption || hasOption(opt.value, selectValue), - ); - const newOptions = newOption - ? [newOption, ...cleanSelectOptions] - : cleanSelectOptions; - setSelectOptions(newOptions); - } - if ( - !allValuesLoaded && - loadingEnabled && - !fetchedQueries.current.has( - getQueryCacheKey(searchValue, 0, pageSize), - ) - ) { - // if fetch only on search but search value is empty, then should not be - // in loading state - setIsLoading(!(fetchOnlyOnSearch && !searchValue)); - } - setInputValue(search); - }, - [ - allValuesLoaded, - allowNewOptions, - fetchOnlyOnSearch, - fullSelectOptions, - isSingleMode, - loadingEnabled, - pageSize, - selectValue, - ], - ); - - const debouncedOnSearch = useMemo( - () => debounce(handleOnSearch, FAST_DEBOUNCE), - [handleOnSearch], - ); + const handleOnSearch = (search: string) => { + const searchValue = search.trim(); + if (allowNewOptions && isSingleMode) { + const newOption = searchValue && + !hasOption(searchValue, fullSelectOptions, true) && { + label: searchValue, + value: searchValue, + isNewOption: true, + }; + const cleanSelectOptions = fullSelectOptions.filter( + opt => !opt.isNewOption || hasOption(opt.value, selectValue), + ); + const newOptions = newOption + ? [newOption, ...cleanSelectOptions] + : cleanSelectOptions; + setSelectOptions(newOptions); + } + if ( + !allValuesLoaded && + loadingEnabled && + !fetchedQueries.current.has(getQueryCacheKey(searchValue, 0, pageSize)) + ) { + // if fetch only on search but search value is empty, then should not be + // in loading state + setIsLoading(!(fetchOnlyOnSearch && !searchValue)); + } + setInputValue(search); + }; const handlePagination = (e: UIEvent) => { const vScroll = e.currentTarget; @@ -554,7 +535,7 @@ const AsyncSelect = forwardRef( onDeselect={handleOnDeselect} onDropdownVisibleChange={handleOnDropdownVisibleChange} onPopupScroll={handlePagination} - onSearch={showSearch ? debouncedOnSearch : undefined} + onSearch={showSearch ? handleOnSearch : undefined} onSelect={handleOnSelect} onClear={handleClear} onChange={onChange} diff --git a/superset-frontend/src/components/Select/Select.test.tsx b/superset-frontend/src/components/Select/Select.test.tsx index 0ee1f409e423d..06f5f7b8e57c1 100644 --- a/superset-frontend/src/components/Select/Select.test.tsx +++ b/superset-frontend/src/components/Select/Select.test.tsx @@ -77,6 +77,11 @@ const findSelectOption = (text: string) => within(getElementByClassName('.rc-virtual-list')).getByText(text), ); +const querySelectOption = (text: string) => + waitFor(() => + within(getElementByClassName('.rc-virtual-list')).queryByText(text), + ); + const findAllSelectOptions = () => waitFor(() => getElementsByClassName('.ant-select-item-option-content')); @@ -549,6 +554,18 @@ test('renders a helper text when one is provided', async () => { expect(screen.queryByText(helperText)).toBeInTheDocument(); }); +test('finds an element with a numeric value and does not duplicate the options', async () => { + const options = [ + { label: 'a', value: 11 }, + { label: 'b', value: 12 }, + ]; + render(