diff --git a/superset-frontend/src/components/Select/Select.test.tsx b/superset-frontend/src/components/Select/Select.test.tsx index 220ad4fe9875c..f55299e3fc7bd 100644 --- a/superset-frontend/src/components/Select/Select.test.tsx +++ b/superset-frontend/src/components/Select/Select.test.tsx @@ -22,11 +22,18 @@ import userEvent from '@testing-library/user-event'; import Select from 'src/components/Select/Select'; import { SELECT_ALL_VALUE } from './utils'; +type Option = { + label: string; + value: number; + gender: string; + disabled?: boolean; +}; + const ARIA_LABEL = 'Test'; const NEW_OPTION = 'Kyle'; const NO_DATA = 'No Data'; const LOADING = 'Loading...'; -const OPTIONS = [ +const OPTIONS: Option[] = [ { label: 'John', value: 1, gender: 'Male' }, { label: 'Liam', value: 2, gender: 'Male' }, { label: 'Olivia', value: 3, gender: 'Female' }, @@ -826,6 +833,56 @@ test('does not render "Select All" when there are 0 or 1 options', async () => { expect(screen.queryByText(selectAllOptionLabel(2))).toBeInTheDocument(); }); +test('do not count unselected disabled options in "Select All"', async () => { + const options = [...OPTIONS]; + options[0].disabled = true; + options[1].disabled = true; + render( + , + ); + await open(); + + // We have 2 options disabled but one is selected initially + expect(await findSelectValue()).toHaveTextContent(options[0].label); + expect(await findSelectValue()).not.toHaveTextContent(options[1].label); + + // Checking Select All shouldn't affect the disabled options + const selectAll = selectAllOptionLabel(OPTIONS.length - 1); + userEvent.click(await findSelectOption(selectAll)); + expect(await findSelectValue()).toHaveTextContent(options[0].label); + expect(await findSelectValue()).not.toHaveTextContent(options[1].label); + + // Unchecking Select All shouldn't affect the disabled options + userEvent.click(await findSelectOption(selectAll)); + expect(await findSelectValue()).toHaveTextContent(options[0].label); + expect(await findSelectValue()).not.toHaveTextContent(options[1].label); +}); + /* 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/Select.tsx b/superset-frontend/src/components/Select/Select.tsx index 68bc60871944d..b1fae69d297ee 100644 --- a/superset-frontend/src/components/Select/Select.tsx +++ b/superset-frontend/src/components/Select/Select.tsx @@ -45,6 +45,8 @@ import { getSuffixIcon, SELECT_ALL_VALUE, selectAllOption, + mapValues, + mapOptions, } from './utils'; import { SelectOptionsType, SelectProps } from './types'; import { @@ -177,23 +179,31 @@ const Select = forwardRef( return result.filter(opt => opt.value !== SELECT_ALL_VALUE); }, [selectOptions, selectValue]); + const enabledOptions = useMemo( + () => fullSelectOptions.filter(option => !option.disabled), + [fullSelectOptions], + ); + + const selectAllEligible = useMemo( + () => + fullSelectOptions.filter( + option => hasOption(option.value, selectValue) || !option.disabled, + ), + [fullSelectOptions, selectValue], + ); + const selectAllEnabled = useMemo( () => !isSingleMode && selectOptions.length > 0 && - fullSelectOptions.length > 1 && + enabledOptions.length > 1 && !inputValue, - [ - isSingleMode, - selectOptions.length, - fullSelectOptions.length, - inputValue, - ], + [isSingleMode, selectOptions.length, enabledOptions.length, inputValue], ); const selectAllMode = useMemo( - () => ensureIsArray(selectValue).length === fullSelectOptions.length + 1, - [selectValue, fullSelectOptions], + () => ensureIsArray(selectValue).length === selectAllEligible.length + 1, + [selectValue, selectAllEligible], ); const handleOnSelect = ( @@ -209,19 +219,19 @@ const Select = forwardRef( if (value === getValue(SELECT_ALL_VALUE)) { if (isLabeledValue(selectedItem)) { return [ - ...fullSelectOptions, + ...selectAllEligible, selectAllOption, ] as AntdLabeledValue[]; } return [ SELECT_ALL_VALUE, - ...fullSelectOptions.map(opt => opt.value), + ...selectAllEligible.map(opt => opt.value), ] as AntdLabeledValue[]; } if (!hasOption(value, array)) { const result = [...array, selectedItem]; if ( - result.length === fullSelectOptions.length && + result.length === selectAllEligible.length && selectAllEnabled ) { return isLabeledValue(selectedItem) @@ -236,12 +246,26 @@ const Select = forwardRef( setInputValue(''); }; + const clear = () => { + setSelectValue( + fullSelectOptions + .filter( + option => option.disabled && hasOption(option.value, selectValue), + ) + .map(option => + labelInValue + ? { label: option.label, value: option.value } + : option.value, + ), + ); + }; + const handleOnDeselect = ( value: string | number | AntdLabeledValue | undefined, ) => { if (Array.isArray(selectValue)) { if (getValue(value) === getValue(SELECT_ALL_VALUE)) { - setSelectValue(undefined); + clear(); } else { let array = selectValue as AntdLabeledValue[]; array = array.filter( @@ -312,7 +336,7 @@ const Select = forwardRef( ); const handleClear = () => { - setSelectValue(undefined); + clear(); if (onClear) { onClear(); } @@ -337,7 +361,7 @@ const Select = forwardRef( // if all values are selected, add select all to value if ( !isSingleMode && - ensureIsArray(value).length === fullSelectOptions.length && + ensureIsArray(value).length === selectAllEligible.length && selectOptions.length > 0 ) { setSelectValue( @@ -353,7 +377,7 @@ const Select = forwardRef( value, isSingleMode, labelInValue, - fullSelectOptions.length, + selectAllEligible.length, selectOptions.length, ]); @@ -362,21 +386,21 @@ const Select = forwardRef( v => getValue(v) === SELECT_ALL_VALUE, ); if (checkSelectAll && !selectAllMode) { - const optionsToSelect = fullSelectOptions.map(option => + const optionsToSelect = selectAllEligible.map(option => labelInValue ? option : option.value, ); optionsToSelect.push(labelInValue ? selectAllOption : SELECT_ALL_VALUE); setSelectValue(optionsToSelect); } - }, [selectValue, selectAllMode, labelInValue, fullSelectOptions]); + }, [selectValue, selectAllMode, labelInValue, selectAllEligible]); const selectAllLabel = useMemo( () => () => `${SELECT_ALL_VALUE} (${formatNumber( NumberFormats.INTEGER, - fullSelectOptions.length, + selectAllEligible.length, )})`, - [fullSelectOptions.length], + [selectAllEligible], ); const handleOnChange = (values: any, options: any) => { @@ -393,30 +417,22 @@ const Select = forwardRef( ) { // send all options to onchange if all are not currently there if (!selectAllMode) { - newValues = labelInValue - ? fullSelectOptions.map(opt => ({ - key: opt.value, - value: opt.value, - label: opt.label, - })) - : fullSelectOptions.map(opt => opt.value); - newOptions = fullSelectOptions.map(opt => ({ - children: opt.label, - key: opt.value, - value: opt.value, - label: opt.label, - })); + newValues = mapValues(selectAllEligible, labelInValue); + newOptions = mapOptions(selectAllEligible); } else { newValues = ensureIsArray(values).filter( (val: any) => getValue(val) !== SELECT_ALL_VALUE, ); } } else if ( - ensureIsArray(values).length === fullSelectOptions.length && + ensureIsArray(values).length === selectAllEligible.length && selectAllMode ) { - newValues = []; - newValues = []; + const array = selectAllEligible.filter( + option => hasOption(option.value, selectValue) && option.disabled, + ); + newValues = mapValues(array, labelInValue); + newOptions = mapOptions(array); } } onChange?.(newValues, newOptions); diff --git a/superset-frontend/src/components/Select/utils.tsx b/superset-frontend/src/components/Select/utils.tsx index e8199358222b5..69606b861572f 100644 --- a/superset-frontend/src/components/Select/utils.tsx +++ b/superset-frontend/src/components/Select/utils.tsx @@ -204,3 +204,21 @@ export const renderSelectOptions = (options: SelectOptionsType) => ); }); + +export const mapValues = (values: SelectOptionsType, labelInValue: boolean) => + labelInValue + ? values.map(opt => ({ + key: opt.value, + value: opt.value, + label: opt.label, + })) + : values.map(opt => opt.value); + +export const mapOptions = (values: SelectOptionsType) => + values.map(opt => ({ + children: opt.label, + key: opt.value, + value: opt.value, + label: opt.label, + disabled: opt.disabled, + }));