diff --git a/superset-frontend/src/explore/components/controls/SelectControl.jsx b/superset-frontend/src/explore/components/controls/SelectControl.jsx index f75d67dedca9..efe1989e7f63 100644 --- a/superset-frontend/src/explore/components/controls/SelectControl.jsx +++ b/superset-frontend/src/explore/components/controls/SelectControl.jsx @@ -87,6 +87,44 @@ const defaultProps = { valueKey: 'value', }; +const numberComparator = (a, b) => a.value - b.value; + +export const areAllValuesNumbers = (items, valueKey = 'value') => { + if (!items || items.length === 0) { + return false; + } + return items.every(item => { + if (Array.isArray(item)) { + const [value] = item; + return typeof value === 'number'; + } + if (typeof item === 'object' && item !== null) { + return typeof item[valueKey] === 'number'; + } + return typeof item === 'number'; + }); +}; + +export const getSortComparator = ( + choices, + options, + valueKey, + explicitComparator, +) => { + if (explicitComparator) { + return explicitComparator; + } + + if ( + (options && areAllValuesNumbers(options, valueKey)) || + (choices && areAllValuesNumbers(choices, valueKey)) + ) { + return numberComparator; + } + + return undefined; +}; + export const innerGetOptions = props => { const { choices, optionRenderer, valueKey } = props; let options = []; @@ -252,7 +290,12 @@ export default class SelectControl extends PureComponent { onDeselect, options: this.state.options, placeholder, - sortComparator: this.props.sortComparator, + sortComparator: getSortComparator( + this.props.choices, + this.props.options, + this.props.valueKey, + this.props.sortComparator, + ), value: getValue(), tokenSeparators, notFoundContent, diff --git a/superset-frontend/src/explore/components/controls/SelectControl.test.jsx b/superset-frontend/src/explore/components/controls/SelectControl.test.jsx index b4b2b0b8d1ad..25d88a8eb2e4 100644 --- a/superset-frontend/src/explore/components/controls/SelectControl.test.jsx +++ b/superset-frontend/src/explore/components/controls/SelectControl.test.jsx @@ -27,6 +27,8 @@ import { } from 'spec/helpers/testing-library'; import SelectControl, { innerGetOptions, + areAllValuesNumbers, + getSortComparator, } from 'src/explore/components/controls/SelectControl'; const defaultProps = { @@ -235,4 +237,210 @@ describe('SelectControl', () => { expect(innerGetOptions(defaultProps)).toEqual(options); }); }); + + describe('areAllValuesNumbers', () => { + it('returns true when all values are numbers (array format)', () => { + const items = [ + [1, 'One'], + [2, 'Two'], + [3, 'Three'], + ]; + expect(areAllValuesNumbers(items)).toBe(true); + }); + + it('returns false when some values are not numbers (array format)', () => { + const items = [ + [1, 'One'], + ['two', 'Two'], + [3, 'Three'], + ]; + expect(areAllValuesNumbers(items)).toBe(false); + }); + + it('returns true when all values are numbers (object format)', () => { + const items = [ + { value: 1, label: 'One' }, + { value: 2, label: 'Two' }, + { value: 3, label: 'Three' }, + ]; + expect(areAllValuesNumbers(items)).toBe(true); + }); + + it('returns false when some values are not numbers (object format)', () => { + const items = [ + { value: 1, label: 'One' }, + { value: 'two', label: 'Two' }, + { value: 3, label: 'Three' }, + ]; + expect(areAllValuesNumbers(items)).toBe(false); + }); + + it('returns true when all values are numbers (primitive format)', () => { + const items = [1, 2, 3]; + expect(areAllValuesNumbers(items)).toBe(true); + }); + + it('returns false when some values are not numbers (primitive format)', () => { + const items = [1, 'two', 3]; + expect(areAllValuesNumbers(items)).toBe(false); + }); + + it('works with custom valueKey', () => { + const items = [ + { id: 1, label: 'One' }, + { id: 2, label: 'Two' }, + { id: 3, label: 'Three' }, + ]; + expect(areAllValuesNumbers(items, 'id')).toBe(true); + }); + + it('returns false for empty items', () => { + expect(areAllValuesNumbers([])).toBe(false); + expect(areAllValuesNumbers(null)).toBe(false); + expect(areAllValuesNumbers(undefined)).toBe(false); + }); + }); + + describe('getSortComparator', () => { + const mockExplicitComparator = (a, b) => a.label.localeCompare(b.label); + + it('returns explicit comparator when provided', () => { + const choices = [ + [1, 'One'], + [2, 'Two'], + ]; + const result = getSortComparator( + choices, + null, + 'value', + mockExplicitComparator, + ); + expect(result).toBe(mockExplicitComparator); + }); + + it('returns number comparator for numeric choices', () => { + const choices = [ + [1, 'One'], + [2, 'Two'], + ]; + const result = getSortComparator(choices, null, 'value', null); + expect(typeof result).toBe('function'); + expect(result).not.toBe(mockExplicitComparator); + }); + + it('returns number comparator for numeric options', () => { + const options = [ + { value: 1, label: 'One' }, + { value: 2, label: 'Two' }, + ]; + const result = getSortComparator(null, options, 'value', null); + expect(typeof result).toBe('function'); + expect(result).not.toBe(mockExplicitComparator); + }); + + it('prioritizes options over choices when both are numeric', () => { + const choices = [ + [1, 'One'], + [2, 'Two'], + ]; + const options = [ + { value: 3, label: 'Three' }, + { value: 4, label: 'Four' }, + ]; + const result = getSortComparator(choices, options, 'value', null); + expect(typeof result).toBe('function'); + }); + + it('returns undefined for non-numeric choices', () => { + const choices = [ + ['one', 'One'], + ['two', 'Two'], + ]; + const result = getSortComparator(choices, null, 'value', null); + expect(result).toBeUndefined(); + }); + + it('returns undefined for non-numeric options', () => { + const options = [ + { value: 'one', label: 'One' }, + { value: 'two', label: 'Two' }, + ]; + const result = getSortComparator(null, options, 'value', null); + expect(result).toBeUndefined(); + }); + + it('returns undefined when no choices or options provided', () => { + const result = getSortComparator(null, null, 'value', null); + expect(result).toBeUndefined(); + }); + }); + + describe('numeric sorting integration', () => { + it('applies numeric sorting to choices automatically', () => { + const numericChoices = [ + [3, 'Three'], + [1, 'One'], + [2, 'Two'], + ]; + renderSelectControl({ choices: numericChoices }); + + // The SelectControl should receive a sortComparator for numeric values + // This is tested by verifying the component renders without errors + const selectorWrapper = screen.getByLabelText('Row Limit', { + selector: 'div', + }); + expect(selectorWrapper).toBeInTheDocument(); + }); + + it('applies numeric sorting to options automatically', () => { + const numericOptions = [ + { value: 3, label: 'Three' }, + { value: 1, label: 'One' }, + { value: 2, label: 'Two' }, + ]; + renderSelectControl({ options: numericOptions, choices: undefined }); + + // The SelectControl should receive a sortComparator for numeric values + const selectorWrapper = screen.getByLabelText('Row Limit', { + selector: 'div', + }); + expect(selectorWrapper).toBeInTheDocument(); + }); + + it('does not apply numeric sorting to mixed-type choices', () => { + const mixedChoices = [ + [1, 'One'], + ['two', 'Two'], + [3, 'Three'], + ]; + renderSelectControl({ choices: mixedChoices }); + + // Should render without errors and not apply numeric sorting + const selectorWrapper = screen.getByLabelText('Row Limit', { + selector: 'div', + }); + expect(selectorWrapper).toBeInTheDocument(); + }); + + it('respects explicit sortComparator over automatic numeric sorting', () => { + const numericChoices = [ + [3, 'Three'], + [1, 'One'], + [2, 'Two'], + ]; + const explicitComparator = jest.fn((a, b) => + a.label.localeCompare(b.label), + ); + + renderSelectControl({ + choices: numericChoices, + sortComparator: explicitComparator, + }); + + const selectorWrapper = screen.getByLabelText('Row Limit', { + selector: 'div', + }); + expect(selectorWrapper).toBeInTheDocument(); + }); + }); });