From c6aee630ab2781f0856466a95b0f4e9b5fb64210 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Tue, 26 Aug 2025 18:10:41 +0200 Subject: [PATCH 1/2] fix: SelectControl default sort numeric choices by value --- .../components/controls/SelectControl.jsx | 57 +++- .../controls/SelectControl.test.jsx | 244 ++++++++++++++++++ 2 files changed, 300 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/explore/components/controls/SelectControl.jsx b/superset-frontend/src/explore/components/controls/SelectControl.jsx index f75d67dedca9..1267d394f1ed 100644 --- a/superset-frontend/src/explore/components/controls/SelectControl.jsx +++ b/superset-frontend/src/explore/components/controls/SelectControl.jsx @@ -87,6 +87,56 @@ const defaultProps = { valueKey: 'value', }; +const numberComparator = (a, b) => { + const aVal = typeof a.value === 'number' ? a.value : parseFloat(a.value); + const bVal = typeof b.value === 'number' ? b.value : parseFloat(b.value); + return aVal - bVal; +}; + +export const areAllChoiceValuesNumbers = (choices, valueKey = 'value') => { + if (!choices || choices.length === 0) { + return false; + } + return choices.every(c => { + if (Array.isArray(c)) { + const [value] = c; + return typeof value === 'number'; + } + if (typeof c === 'object' && c !== null) { + return typeof c[valueKey] === 'number'; + } + return typeof c === 'number'; + }); +}; + +export const areAllOptionValuesNumbers = (options, valueKey = 'value') => { + if (!options || options.length === 0) { + return false; + } + return options.every(o => typeof o[valueKey] === 'number'); +}; + +export const getSortComparator = ( + choices, + options, + valueKey, + explicitComparator, +) => { + if (explicitComparator) { + return explicitComparator; + } + + if (options && areAllOptionValuesNumbers(options, valueKey)) { + return numberComparator; + } + + if (choices && areAllChoiceValuesNumbers(choices, valueKey)) { + return numberComparator; + } + + return undefined; +}; + export const innerGetOptions = props => { const { choices, optionRenderer, valueKey } = props; let options = []; @@ -252,7 +302,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..c5a1a72fed75 100644 --- a/superset-frontend/src/explore/components/controls/SelectControl.test.jsx +++ b/superset-frontend/src/explore/components/controls/SelectControl.test.jsx @@ -27,6 +27,9 @@ import { } from 'spec/helpers/testing-library'; import SelectControl, { innerGetOptions, + areAllChoiceValuesNumbers, + areAllOptionValuesNumbers, + getSortComparator, } from 'src/explore/components/controls/SelectControl'; const defaultProps = { @@ -235,4 +238,245 @@ describe('SelectControl', () => { expect(innerGetOptions(defaultProps)).toEqual(options); }); }); + + describe('areAllChoiceValuesNumbers', () => { + it('returns true when all choice values are numbers (array format)', () => { + const choices = [ + [1, 'One'], + [2, 'Two'], + [3, 'Three'], + ]; + expect(areAllChoiceValuesNumbers(choices)).toBe(true); + }); + + it('returns false when some choice values are not numbers (array format)', () => { + const choices = [ + [1, 'One'], + ['two', 'Two'], + [3, 'Three'], + ]; + expect(areAllChoiceValuesNumbers(choices)).toBe(false); + }); + + it('returns true when all choice values are numbers (object format)', () => { + const choices = [ + { value: 1, label: 'One' }, + { value: 2, label: 'Two' }, + { value: 3, label: 'Three' }, + ]; + expect(areAllChoiceValuesNumbers(choices)).toBe(true); + }); + + it('returns false when some choice values are not numbers (object format)', () => { + const choices = [ + { value: 1, label: 'One' }, + { value: 'two', label: 'Two' }, + { value: 3, label: 'Three' }, + ]; + expect(areAllChoiceValuesNumbers(choices)).toBe(false); + }); + + it('returns true when all choice values are numbers (primitive format)', () => { + const choices = [1, 2, 3]; + expect(areAllChoiceValuesNumbers(choices)).toBe(true); + }); + + it('returns false when some choice values are not numbers (primitive format)', () => { + const choices = [1, 'two', 3]; + expect(areAllChoiceValuesNumbers(choices)).toBe(false); + }); + + it('works with custom valueKey', () => { + const choices = [ + { id: 1, label: 'One' }, + { id: 2, label: 'Two' }, + { id: 3, label: 'Three' }, + ]; + expect(areAllChoiceValuesNumbers(choices, 'id')).toBe(true); + }); + + it('returns false for empty choices', () => { + expect(areAllChoiceValuesNumbers([])).toBe(false); + expect(areAllChoiceValuesNumbers(null)).toBe(false); + expect(areAllChoiceValuesNumbers(undefined)).toBe(false); + }); + }); + + describe('areAllOptionValuesNumbers', () => { + it('returns true when all option values are numbers', () => { + const options = [ + { value: 1, label: 'One' }, + { value: 2, label: 'Two' }, + { value: 3, label: 'Three' }, + ]; + expect(areAllOptionValuesNumbers(options)).toBe(true); + }); + + it('returns false when some option values are not numbers', () => { + const options = [ + { value: 1, label: 'One' }, + { value: 'two', label: 'Two' }, + { value: 3, label: 'Three' }, + ]; + expect(areAllOptionValuesNumbers(options)).toBe(false); + }); + + it('works with custom valueKey', () => { + const options = [ + { id: 1, label: 'One' }, + { id: 2, label: 'Two' }, + { id: 3, label: 'Three' }, + ]; + expect(areAllOptionValuesNumbers(options, 'id')).toBe(true); + }); + + it('returns false for empty options', () => { + expect(areAllOptionValuesNumbers([])).toBe(false); + expect(areAllOptionValuesNumbers(null)).toBe(false); + expect(areAllOptionValuesNumbers(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(); + }); + }); }); From 8582b73e0f72b875d124f68511324ab8ea61043d Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Thu, 28 Aug 2025 12:17:02 +0200 Subject: [PATCH 2/2] Simplify logic --- .../components/controls/SelectControl.jsx | 38 +++----- .../controls/SelectControl.test.jsx | 88 ++++++------------- 2 files changed, 39 insertions(+), 87 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/SelectControl.jsx b/superset-frontend/src/explore/components/controls/SelectControl.jsx index 1267d394f1ed..efe1989e7f63 100644 --- a/superset-frontend/src/explore/components/controls/SelectControl.jsx +++ b/superset-frontend/src/explore/components/controls/SelectControl.jsx @@ -87,35 +87,24 @@ const defaultProps = { valueKey: 'value', }; -const numberComparator = (a, b) => { - const aVal = typeof a.value === 'number' ? a.value : parseFloat(a.value); - const bVal = typeof b.value === 'number' ? b.value : parseFloat(b.value); - return aVal - bVal; -}; +const numberComparator = (a, b) => a.value - b.value; -export const areAllChoiceValuesNumbers = (choices, valueKey = 'value') => { - if (!choices || choices.length === 0) { +export const areAllValuesNumbers = (items, valueKey = 'value') => { + if (!items || items.length === 0) { return false; } - return choices.every(c => { - if (Array.isArray(c)) { - const [value] = c; + return items.every(item => { + if (Array.isArray(item)) { + const [value] = item; return typeof value === 'number'; } - if (typeof c === 'object' && c !== null) { - return typeof c[valueKey] === 'number'; + if (typeof item === 'object' && item !== null) { + return typeof item[valueKey] === 'number'; } - return typeof c === 'number'; + return typeof item === 'number'; }); }; -export const areAllOptionValuesNumbers = (options, valueKey = 'value') => { - if (!options || options.length === 0) { - return false; - } - return options.every(o => typeof o[valueKey] === 'number'); -}; - export const getSortComparator = ( choices, options, @@ -126,11 +115,10 @@ export const getSortComparator = ( return explicitComparator; } - if (options && areAllOptionValuesNumbers(options, valueKey)) { - return numberComparator; - } - - if (choices && areAllChoiceValuesNumbers(choices, valueKey)) { + if ( + (options && areAllValuesNumbers(options, valueKey)) || + (choices && areAllValuesNumbers(choices, valueKey)) + ) { return numberComparator; } diff --git a/superset-frontend/src/explore/components/controls/SelectControl.test.jsx b/superset-frontend/src/explore/components/controls/SelectControl.test.jsx index c5a1a72fed75..25d88a8eb2e4 100644 --- a/superset-frontend/src/explore/components/controls/SelectControl.test.jsx +++ b/superset-frontend/src/explore/components/controls/SelectControl.test.jsx @@ -27,8 +27,7 @@ import { } from 'spec/helpers/testing-library'; import SelectControl, { innerGetOptions, - areAllChoiceValuesNumbers, - areAllOptionValuesNumbers, + areAllValuesNumbers, getSortComparator, } from 'src/explore/components/controls/SelectControl'; @@ -239,101 +238,66 @@ describe('SelectControl', () => { }); }); - describe('areAllChoiceValuesNumbers', () => { - it('returns true when all choice values are numbers (array format)', () => { - const choices = [ + describe('areAllValuesNumbers', () => { + it('returns true when all values are numbers (array format)', () => { + const items = [ [1, 'One'], [2, 'Two'], [3, 'Three'], ]; - expect(areAllChoiceValuesNumbers(choices)).toBe(true); + expect(areAllValuesNumbers(items)).toBe(true); }); - it('returns false when some choice values are not numbers (array format)', () => { - const choices = [ + it('returns false when some values are not numbers (array format)', () => { + const items = [ [1, 'One'], ['two', 'Two'], [3, 'Three'], ]; - expect(areAllChoiceValuesNumbers(choices)).toBe(false); + expect(areAllValuesNumbers(items)).toBe(false); }); - it('returns true when all choice values are numbers (object format)', () => { - const choices = [ + 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(areAllChoiceValuesNumbers(choices)).toBe(true); + expect(areAllValuesNumbers(items)).toBe(true); }); - it('returns false when some choice values are not numbers (object format)', () => { - const choices = [ + 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(areAllChoiceValuesNumbers(choices)).toBe(false); - }); - - it('returns true when all choice values are numbers (primitive format)', () => { - const choices = [1, 2, 3]; - expect(areAllChoiceValuesNumbers(choices)).toBe(true); - }); - - it('returns false when some choice values are not numbers (primitive format)', () => { - const choices = [1, 'two', 3]; - expect(areAllChoiceValuesNumbers(choices)).toBe(false); - }); - - it('works with custom valueKey', () => { - const choices = [ - { id: 1, label: 'One' }, - { id: 2, label: 'Two' }, - { id: 3, label: 'Three' }, - ]; - expect(areAllChoiceValuesNumbers(choices, 'id')).toBe(true); + expect(areAllValuesNumbers(items)).toBe(false); }); - it('returns false for empty choices', () => { - expect(areAllChoiceValuesNumbers([])).toBe(false); - expect(areAllChoiceValuesNumbers(null)).toBe(false); - expect(areAllChoiceValuesNumbers(undefined)).toBe(false); + it('returns true when all values are numbers (primitive format)', () => { + const items = [1, 2, 3]; + expect(areAllValuesNumbers(items)).toBe(true); }); - }); - describe('areAllOptionValuesNumbers', () => { - it('returns true when all option values are numbers', () => { - const options = [ - { value: 1, label: 'One' }, - { value: 2, label: 'Two' }, - { value: 3, label: 'Three' }, - ]; - expect(areAllOptionValuesNumbers(options)).toBe(true); - }); - - it('returns false when some option values are not numbers', () => { - const options = [ - { value: 1, label: 'One' }, - { value: 'two', label: 'Two' }, - { value: 3, label: 'Three' }, - ]; - expect(areAllOptionValuesNumbers(options)).toBe(false); + 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 options = [ + const items = [ { id: 1, label: 'One' }, { id: 2, label: 'Two' }, { id: 3, label: 'Three' }, ]; - expect(areAllOptionValuesNumbers(options, 'id')).toBe(true); + expect(areAllValuesNumbers(items, 'id')).toBe(true); }); - it('returns false for empty options', () => { - expect(areAllOptionValuesNumbers([])).toBe(false); - expect(areAllOptionValuesNumbers(null)).toBe(false); - expect(areAllOptionValuesNumbers(undefined)).toBe(false); + it('returns false for empty items', () => { + expect(areAllValuesNumbers([])).toBe(false); + expect(areAllValuesNumbers(null)).toBe(false); + expect(areAllValuesNumbers(undefined)).toBe(false); }); });