From b621ee92c9124e2e2f7c988302eb0f77f00c9fc9 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Fri, 11 Aug 2023 14:22:15 -0300 Subject: [PATCH] fix: Duplicated options in Select when using numerical values (#24906) --- .../cypress/e2e/dashboard/utils.ts | 2 +- .../e2e/explore/advanced_analytics.test.ts | 1 + .../components/Select/AsyncSelect.stories.tsx | 34 ----- .../components/Select/AsyncSelect.test.tsx | 63 ++++++-- .../src/components/Select/AsyncSelect.tsx | 75 ++++++---- .../src/components/Select/Select.stories.tsx | 5 + .../src/components/Select/Select.test.tsx | 135 +++++++++++++----- .../src/components/Select/Select.tsx | 96 ++++++++----- 8 files changed, 260 insertions(+), 151 deletions(-) diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts index 00d3eda45e31f..ca539039cf6e6 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts @@ -322,7 +322,7 @@ export function applyNativeFilterValueWithIndex(index: number, value: string) { cy.get(nativeFilters.filterFromDashboardView.filterValueInput) .eq(index) .should('exist', { timeout: 10000 }) - .type(`${value}{enter}`); + .type(`${value}{enter}`, { force: true }); // click the title to dismiss shown options cy.get(nativeFilters.filterFromDashboardView.filterName) .eq(index) diff --git a/superset-frontend/cypress-base/cypress/e2e/explore/advanced_analytics.test.ts b/superset-frontend/cypress-base/cypress/e2e/explore/advanced_analytics.test.ts index fd207a64e3124..8e52aa6c56b58 100644 --- a/superset-frontend/cypress-base/cypress/e2e/explore/advanced_analytics.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/explore/advanced_analytics.test.ts @@ -39,6 +39,7 @@ describe('Advanced analytics', () => { cy.get('[data-test=time_compare]') .find('input[type=search]') + .clear() .type('1 year{enter}'); cy.get('button[data-test="run-query-button"]').click(); diff --git a/superset-frontend/src/components/Select/AsyncSelect.stories.tsx b/superset-frontend/src/components/Select/AsyncSelect.stories.tsx index 0bdaf43f2c06f..4235008fb2cdc 100644 --- a/superset-frontend/src/components/Select/AsyncSelect.stories.tsx +++ b/superset-frontend/src/components/Select/AsyncSelect.stories.tsx @@ -26,7 +26,6 @@ import React, { import Button from 'src/components/Button'; import AsyncSelect from './AsyncSelect'; import { - SelectOptionsType, AsyncSelectProps, AsyncSelectRef, SelectOptionsTypePage, @@ -39,40 +38,7 @@ export default { const DEFAULT_WIDTH = 200; -const options: SelectOptionsType = [ - { - label: 'Such an incredibly awesome long long label', - value: 'Such an incredibly awesome long long label', - custom: 'Secret custom prop', - }, - { - label: 'Another incredibly awesome long long label', - value: 'Another incredibly awesome long long label', - }, - { - label: 'JSX Label', - customLabel:
JSX Label
, - value: 'JSX Label', - }, - { label: 'A', value: 'A' }, - { label: 'B', value: 'B' }, - { label: 'C', value: 'C' }, - { label: 'D', value: 'D' }, - { label: 'E', value: 'E' }, - { label: 'F', value: 'F' }, - { label: 'G', value: 'G' }, - { label: 'H', value: 'H' }, - { label: 'I', value: 'I' }, -]; - const ARG_TYPES = { - options: { - defaultValue: options, - description: `It defines the options of the Select. - The options can be static, an array of options. - The options can also be async, a promise that returns an array of options. - `, - }, ariaLabel: { description: `It adds the aria-label tag for accessibility standards. Must be plain English and localized. diff --git a/superset-frontend/src/components/Select/AsyncSelect.test.tsx b/superset-frontend/src/components/Select/AsyncSelect.test.tsx index 0b7cafab3a141..b964f48ee78ab 100644 --- a/superset-frontend/src/components/Select/AsyncSelect.test.tsx +++ b/superset-frontend/src/components/Select/AsyncSelect.test.tsx @@ -17,7 +17,14 @@ * under the License. */ import React from 'react'; -import { render, screen, waitFor, within } from 'spec/helpers/testing-library'; +import { + createEvent, + fireEvent, + render, + screen, + waitFor, + within, +} from 'spec/helpers/testing-library'; import userEvent from '@testing-library/user-event'; import { AsyncSelect } from 'src/components'; @@ -93,6 +100,9 @@ const getElementsByClassName = (className: string) => const getSelect = () => screen.getByRole('combobox', { name: ARIA_LABEL }); +const getAllSelectOptions = () => + getElementsByClassName('.ant-select-item-option-content'); + const findSelectOption = (text: string) => waitFor(() => within(getElementByClassName('.rc-virtual-list')).getByText(text), @@ -323,12 +333,14 @@ test('same case should be ranked to the top', async () => { })); render(); await type('Ac'); - const options = await findAllSelectOptions(); - expect(options.length).toBe(4); - expect(options[0]?.textContent).toEqual('acbc'); - expect(options[1]?.textContent).toEqual('CAc'); - expect(options[2]?.textContent).toEqual('abac'); - expect(options[3]?.textContent).toEqual('Cac'); + await waitFor(() => { + const options = getAllSelectOptions(); + expect(options.length).toBe(4); + expect(options[0]?.textContent).toEqual('acbc'); + expect(options[1]?.textContent).toEqual('CAc'); + expect(options[2]?.textContent).toEqual('abac'); + expect(options[3]?.textContent).toEqual('Cac'); + }); }); test('ignores special keys when searching', async () => { @@ -365,7 +377,13 @@ test('searches for custom fields', async () => { test('removes duplicated values', async () => { render(); - await type('a,b,b,b,c,d,d'); + const input = getElementByClassName('.ant-select-selection-search-input'); + const paste = createEvent.paste(input, { + clipboardData: { + getData: () => 'a,b,b,b,c,d,d', + }, + }); + fireEvent(input, paste); const values = await findAllSelectValues(); expect(values.length).toBe(4); expect(values[0]).toHaveTextContent('a'); @@ -601,7 +619,9 @@ test('does not show "No data" when allowNewOptions is true and a new option is e render(); await open(); await type(NEW_OPTION); - expect(screen.queryByText(NO_DATA)).not.toBeInTheDocument(); + await waitFor(() => + expect(screen.queryByText(NO_DATA)).not.toBeInTheDocument(), + ); }); test('sets a initial value in single mode', async () => { @@ -690,12 +710,9 @@ test('does not fire a new request for the same search input', async () => { , ); await type('search'); - expect(await screen.findByText(NO_DATA)).toBeInTheDocument(); - expect(loadOptions).toHaveBeenCalledTimes(1); - clearAll(); + await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(1)); await type('search'); - expect(await screen.findByText(LOADING)).toBeInTheDocument(); - expect(loadOptions).toHaveBeenCalledTimes(1); + await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(1)); }); test('does not fire a new request if all values have been fetched', async () => { @@ -823,6 +840,24 @@ test('does not fire onChange when searching but no selection', async () => { expect(onChange).toHaveBeenCalledTimes(1); }); +test('does not duplicate options when using numeric values', async () => { + render( + ({ + data: [ + { label: '1', value: 1 }, + { label: '2', value: 2 }, + ], + totalCount: 2, + })} + />, + ); + await type('1'); + await waitFor(() => expect(getAllSelectOptions().length).toBe(1)); +}); + /* 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 3453ce2b5f06a..320d6ec3bdb47 100644 --- a/superset-frontend/src/components/Select/AsyncSelect.tsx +++ b/superset-frontend/src/components/Select/AsyncSelect.tsx @@ -27,14 +27,15 @@ import React, { useRef, useCallback, useImperativeHandle, + ClipboardEvent, } from 'react'; import { ensureIsArray, t, usePrevious } from '@superset-ui/core'; import { LabeledValue as AntdLabeledValue } from 'antd/lib/select'; import debounce from 'lodash/debounce'; -import { isEqual } from 'lodash'; +import { isEqual, uniq } 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, @@ -122,6 +123,7 @@ const AsyncSelect = forwardRef( onClear, onDropdownVisibleChange, onDeselect, + onSearch, onSelect, optionFilterProps = ['label', 'value'], options, @@ -129,7 +131,7 @@ const AsyncSelect = forwardRef( placeholder = t('Select ...'), showSearch = true, sortComparator = DEFAULT_SORT_COMPARATOR, - tokenSeparators, + tokenSeparators = TOKEN_SEPARATORS, value, getPopupContainer, oneLine, @@ -150,11 +152,7 @@ const AsyncSelect = forwardRef( const [allValuesLoaded, setAllValuesLoaded] = useState(false); const selectValueRef = useRef(selectValue); const fetchedQueries = useRef(new Map()); - const mappedMode = isSingleMode - ? undefined - : allowNewOptions - ? 'tags' - : 'multiple'; + const mappedMode = isSingleMode ? undefined : 'multiple'; const allowFetch = !fetchOnlyOnSearch || inputValue; const [maxTagCount, setMaxTagCount] = useState( propsMaxTagCount ?? MAX_TAG_COUNT, @@ -253,6 +251,14 @@ const AsyncSelect = forwardRef( const array = selectValue as (string | number)[]; setSelectValue(array.filter(element => element !== value)); } + // removes new option + if (option.isNewOption) { + setSelectOptions( + fullSelectOptions.filter( + option => getValue(option.value) !== getValue(value), + ), + ); + } } fireOnChange(); onDeselect?.(value, option); @@ -341,9 +347,9 @@ const AsyncSelect = forwardRef( [fetchPage], ); - const handleOnSearch = (search: string) => { + const handleOnSearch = debounce((search: string) => { const searchValue = search.trim(); - if (allowNewOptions && isSingleMode) { + if (allowNewOptions) { const newOption = searchValue && !hasOption(searchValue, fullSelectOptions, true) && { label: searchValue, @@ -368,7 +374,10 @@ const AsyncSelect = forwardRef( setIsLoading(!(fetchOnlyOnSearch && !searchValue)); } setInputValue(search); - }; + onSearch?.(searchValue); + }, FAST_DEBOUNCE); + + useEffect(() => () => handleOnSearch.cancel(), [handleOnSearch]); const handlePagination = (e: UIEvent) => { const vScroll = e.currentTarget; @@ -439,19 +448,7 @@ const AsyncSelect = forwardRef( }; const handleOnBlur = (event: React.FocusEvent) => { - const tagsMode = !isSingleMode && allowNewOptions; - const searchValue = inputValue.trim(); - // Searched values will be autoselected during onBlur events when in tags mode. - // We want to make sure a value is only selected if the user has actually selected it - // by pressing Enter or clicking on it. - if ( - tagsMode && - searchValue && - !hasOption(searchValue, selectValue, true) - ) { - // The search value will be added so we revert to the previous value - setSelectValue(selectValue || []); - } + setInputValue(''); onBlur?.(event); }; @@ -526,6 +523,28 @@ const AsyncSelect = forwardRef( [ref], ); + const onPaste = (e: ClipboardEvent) => { + const pastedText = e.clipboardData.getData('text'); + if (isSingleMode) { + setSelectValue({ label: pastedText, value: pastedText }); + } else { + const token = tokenSeparators.find(token => pastedText.includes(token)); + const array = token ? uniq(pastedText.split(token)) : [pastedText]; + setSelectValue(previous => [ + ...((previous || []) as AntdLabeledValue[]), + ...array.map(value => ({ + label: value, + value, + })), + ]); + } + }; + + const shouldRenderChildrenOptions = useMemo( + () => hasCustomLabels(fullSelectOptions), + [fullSelectOptions], + ); + return ( {header && ( @@ -549,17 +568,17 @@ const AsyncSelect = forwardRef( onBlur={handleOnBlur} onDeselect={handleOnDeselect} onDropdownVisibleChange={handleOnDropdownVisibleChange} + // @ts-ignore + onPaste={onPaste} onPopupScroll={handlePagination} onSearch={showSearch ? handleOnSearch : undefined} onSelect={handleOnSelect} onClear={handleClear} - options={ - hasCustomLabels(fullSelectOptions) ? undefined : fullSelectOptions - } + options={shouldRenderChildrenOptions ? undefined : fullSelectOptions} placeholder={placeholder} showSearch={showSearch} showArrow - tokenSeparators={tokenSeparators || TOKEN_SEPARATORS} + tokenSeparators={tokenSeparators} value={selectValue} suffixIcon={getSuffixIcon(isLoading, showSearch, isDropdownVisible)} menuItemSelectedIcon={ diff --git a/superset-frontend/src/components/Select/Select.stories.tsx b/superset-frontend/src/components/Select/Select.stories.tsx index 58fa424700761..d7a9b6e893107 100644 --- a/superset-frontend/src/components/Select/Select.stories.tsx +++ b/superset-frontend/src/components/Select/Select.stories.tsx @@ -103,6 +103,11 @@ const ARG_TYPES = { disable: true, }, }, + mappedMode: { + table: { + disable: true, + }, + }, mode: { description: `It defines whether the Select should allow for the selection of multiple options or single. Single by default. diff --git a/superset-frontend/src/components/Select/Select.test.tsx b/superset-frontend/src/components/Select/Select.test.tsx index bce753604f511..2b204cec1cd52 100644 --- a/superset-frontend/src/components/Select/Select.test.tsx +++ b/superset-frontend/src/components/Select/Select.test.tsx @@ -17,7 +17,14 @@ * under the License. */ import React from 'react'; -import { render, screen, waitFor, within } from 'spec/helpers/testing-library'; +import { + createEvent, + fireEvent, + render, + screen, + waitFor, + within, +} from 'spec/helpers/testing-library'; import userEvent from '@testing-library/user-event'; import Select from 'src/components/Select/Select'; import { SELECT_ALL_VALUE } from './utils'; @@ -68,7 +75,6 @@ const defaultProps = { ariaLabel: ARIA_LABEL, labelInValue: true, options: OPTIONS, - pageSize: 10, showSearch: true, }; @@ -93,6 +99,9 @@ const querySelectOption = (text: string) => within(getElementByClassName('.rc-virtual-list')).queryByText(text), ); +const getAllSelectOptions = () => + getElementsByClassName('.ant-select-item-option-content'); + const findAllSelectOptions = () => waitFor(() => getElementsByClassName('.ant-select-item-option-content')); @@ -134,6 +143,11 @@ const clearTypedText = () => { const open = () => waitFor(() => userEvent.click(getSelect())); +const reopen = async () => { + await type('{esc}'); + await open(); +}; + test('displays a header', async () => { const headerText = 'Header'; render(); await type('Her'); - const options = await findAllSelectOptions(); - expect(options.length).toBe(4); - expect(options[0]?.textContent).toEqual('Her'); - expect(options[1]?.textContent).toEqual('Herme'); - expect(options[2]?.textContent).toEqual('Cher'); - expect(options[3]?.textContent).toEqual('Guilherme'); + await waitFor(() => { + const options = getAllSelectOptions(); + expect(options.length).toBe(4); + expect(options[0]?.textContent).toEqual('Her'); + expect(options[1]?.textContent).toEqual('Herme'); + expect(options[2]?.textContent).toEqual('Cher'); + expect(options[3]?.textContent).toEqual('Guilherme'); + }); }); test('ignores case when searching', async () => { @@ -303,12 +313,14 @@ test('same case should be ranked to the top', async () => { />, ); await type('Ac'); - const options = await findAllSelectOptions(); - expect(options.length).toBe(4); - expect(options[0]?.textContent).toEqual('acbc'); - expect(options[1]?.textContent).toEqual('CAc'); - expect(options[2]?.textContent).toEqual('abac'); - expect(options[3]?.textContent).toEqual('Cac'); + await waitFor(() => { + const options = getAllSelectOptions(); + expect(options.length).toBe(4); + expect(options[0]?.textContent).toEqual('acbc'); + expect(options[1]?.textContent).toEqual('CAc'); + expect(options[2]?.textContent).toEqual('abac'); + expect(options[3]?.textContent).toEqual('Cac'); + }); }); test('ignores special keys when searching', async () => { @@ -338,7 +350,13 @@ test('searches for custom fields', async () => { test('removes duplicated values', async () => { render(); await open(); await type(NEW_OPTION); - expect(screen.queryByText(NO_DATA)).not.toBeInTheDocument(); + await waitFor(() => + expect(screen.queryByText(NO_DATA)).not.toBeInTheDocument(), + ); }); test('does not show "Loading..." when allowNewOptions is false and a new option is entered', async () => { @@ -625,9 +645,11 @@ test('does not render "Select all" when searching', async () => { render(, + ); + await open(); + await type(NEW_OPTION); + expect(await findSelectOption(NEW_OPTION)).toBeInTheDocument(); + await type('{enter}'); + clearTypedText(); + userEvent.click(await findSelectOption(NEW_OPTION)); + expect(await querySelectOption(NEW_OPTION)).not.toBeInTheDocument(); +}); + test('selecting all values also selects "Select all"', async () => { render( , + ); + await open(); + expect(screen.getByText(selectAllOptionLabel(2))).toBeInTheDocument(); }); test('do not count unselected disabled options in "Select All"', async () => { @@ -909,6 +957,21 @@ test('does not fire onChange when searching but no selection', async () => { expect(onChange).toHaveBeenCalledTimes(1); }); +test('does not duplicate options when using numeric values', async () => { + render( +