From ab3214935bdf8dac58712e749e426e826144a870 Mon Sep 17 00:00:00 2001 From: Lyndsi Kay Williams <55605634+lyndsiWilliams@users.noreply.github.com> Date: Fri, 3 Dec 2021 18:31:46 -0600 Subject: [PATCH] fix(sqllab): Floating numbers not sorting correctly in result column (#17573) * Floating nums now sorting correctly with parseFloatingNums function * Floating numbers AND strings now sorting correctly, +locale comparison * Added NULL handling back to sort function * Moved parseFloatingNums outside of sortResults * Removed localeCompare and added testing * Add equality check back to sort function * Added floatValue nit --- .../FilterableTable/FilterableTable.test.tsx | 188 ++++++++++++++++++ .../FilterableTable/FilterableTable.tsx | 17 +- 2 files changed, 201 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx b/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx index 5690f5f9e1a57..77f7f712f837b 100644 --- a/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx +++ b/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx @@ -22,6 +22,8 @@ import { styledMount as mount } from 'spec/helpers/theming'; import FilterableTable, { MAX_COLUMNS_FOR_TABLE, } from 'src/components/FilterableTable/FilterableTable'; +import { render, screen } from 'spec/helpers/testing-library'; +import userEvent from '@testing-library/user-event'; describe('FilterableTable', () => { const mockedProps = { @@ -84,3 +86,189 @@ describe('FilterableTable', () => { expect(wrapper.find('.ReactVirtualized__Table__row')).toExist(); }); }); + +describe('FilterableTable sorting - RTL', () => { + it('sorts strings correctly', () => { + const stringProps = { + orderedColumnKeys: ['a'], + data: [{ a: 'Bravo' }, { a: 'Alpha' }, { a: 'Charlie' }], + height: 500, + }; + render(); + + const stringColumn = screen.getByRole('columnheader', { name: 'a' }); + const gridCells = screen.getAllByRole('gridcell'); + + // Original order + expect(gridCells[0]).toHaveTextContent('Bravo'); + expect(gridCells[1]).toHaveTextContent('Alpha'); + expect(gridCells[2]).toHaveTextContent('Charlie'); + + // First click to sort ascending + userEvent.click(stringColumn); + expect(gridCells[0]).toHaveTextContent('Alpha'); + expect(gridCells[1]).toHaveTextContent('Bravo'); + expect(gridCells[2]).toHaveTextContent('Charlie'); + + // Second click to sort descending + userEvent.click(stringColumn); + expect(gridCells[0]).toHaveTextContent('Charlie'); + expect(gridCells[1]).toHaveTextContent('Bravo'); + expect(gridCells[2]).toHaveTextContent('Alpha'); + + // Third click to sort ascending again + userEvent.click(stringColumn); + expect(gridCells[0]).toHaveTextContent('Alpha'); + expect(gridCells[1]).toHaveTextContent('Bravo'); + expect(gridCells[2]).toHaveTextContent('Charlie'); + }); + + it('sorts integers correctly', () => { + const integerProps = { + orderedColumnKeys: ['b'], + data: [{ b: 10 }, { b: 0 }, { b: 100 }], + height: 500, + }; + render(); + + const integerColumn = screen.getByRole('columnheader', { name: 'b' }); + const gridCells = screen.getAllByRole('gridcell'); + + // Original order + expect(gridCells[0]).toHaveTextContent('10'); + expect(gridCells[1]).toHaveTextContent('0'); + expect(gridCells[2]).toHaveTextContent('100'); + + // First click to sort ascending + userEvent.click(integerColumn); + expect(gridCells[0]).toHaveTextContent('0'); + expect(gridCells[1]).toHaveTextContent('10'); + expect(gridCells[2]).toHaveTextContent('100'); + + // Second click to sort descending + userEvent.click(integerColumn); + expect(gridCells[0]).toHaveTextContent('100'); + expect(gridCells[1]).toHaveTextContent('10'); + expect(gridCells[2]).toHaveTextContent('0'); + + // Third click to sort ascending again + userEvent.click(integerColumn); + expect(gridCells[0]).toHaveTextContent('0'); + expect(gridCells[1]).toHaveTextContent('10'); + expect(gridCells[2]).toHaveTextContent('100'); + }); + + it('sorts floating numbers correctly', () => { + const floatProps = { + orderedColumnKeys: ['c'], + data: [{ c: 45.67 }, { c: 1.23 }, { c: 89.0000001 }], + height: 500, + }; + render(); + + const floatColumn = screen.getByRole('columnheader', { name: 'c' }); + const gridCells = screen.getAllByRole('gridcell'); + + // Original order + expect(gridCells[0]).toHaveTextContent('45.67'); + expect(gridCells[1]).toHaveTextContent('1.23'); + expect(gridCells[2]).toHaveTextContent('89.0000001'); + + // First click to sort ascending + userEvent.click(floatColumn); + expect(gridCells[0]).toHaveTextContent('1.23'); + expect(gridCells[1]).toHaveTextContent('45.67'); + expect(gridCells[2]).toHaveTextContent('89.0000001'); + + // Second click to sort descending + userEvent.click(floatColumn); + expect(gridCells[0]).toHaveTextContent('89.0000001'); + expect(gridCells[1]).toHaveTextContent('45.67'); + expect(gridCells[2]).toHaveTextContent('1.23'); + + // Third click to sort ascending again + userEvent.click(floatColumn); + expect(gridCells[0]).toHaveTextContent('1.23'); + expect(gridCells[1]).toHaveTextContent('45.67'); + expect(gridCells[2]).toHaveTextContent('89.0000001'); + }); + + it('sorts rows properly when floating numbers have mixed types', () => { + const mixedFloatProps = { + orderedColumnKeys: ['d'], + data: [ + { d: 48710.92 }, + { d: 145776.56 }, + { d: 72212.86 }, + { d: '144729.96000000002' }, + { d: '26260.210000000003' }, + { d: '152718.97999999998' }, + { d: 28550.59 }, + { d: '24078.610000000004' }, + { d: '98089.08000000002' }, + { d: '3439718.0300000007' }, + { d: '4528047.219999993' }, + ], + height: 500, + }; + render(); + + const mixedFloatColumn = screen.getByRole('columnheader', { name: 'd' }); + const gridCells = screen.getAllByRole('gridcell'); + + // Original order + expect(gridCells[0]).toHaveTextContent('48710.92'); + expect(gridCells[1]).toHaveTextContent('145776.56'); + expect(gridCells[2]).toHaveTextContent('72212.86'); + expect(gridCells[3]).toHaveTextContent('144729.96000000002'); + expect(gridCells[4]).toHaveTextContent('26260.210000000003'); + expect(gridCells[5]).toHaveTextContent('152718.97999999998'); + expect(gridCells[6]).toHaveTextContent('28550.59'); + expect(gridCells[7]).toHaveTextContent('24078.610000000004'); + expect(gridCells[8]).toHaveTextContent('98089.08000000002'); + expect(gridCells[9]).toHaveTextContent('3439718.0300000007'); + expect(gridCells[10]).toHaveTextContent('4528047.219999993'); + + // First click to sort ascending + userEvent.click(mixedFloatColumn); + expect(gridCells[0]).toHaveTextContent('24078.610000000004'); + expect(gridCells[1]).toHaveTextContent('26260.210000000003'); + expect(gridCells[2]).toHaveTextContent('28550.59'); + expect(gridCells[3]).toHaveTextContent('48710.92'); + expect(gridCells[4]).toHaveTextContent('72212.86'); + expect(gridCells[5]).toHaveTextContent('98089.08000000002'); + expect(gridCells[6]).toHaveTextContent('144729.96000000002'); + expect(gridCells[7]).toHaveTextContent('145776.56'); + expect(gridCells[8]).toHaveTextContent('152718.97999999998'); + expect(gridCells[9]).toHaveTextContent('3439718.0300000007'); + expect(gridCells[10]).toHaveTextContent('4528047.219999993'); + + // Second click to sort descending + userEvent.click(mixedFloatColumn); + expect(gridCells[0]).toHaveTextContent('4528047.219999993'); + expect(gridCells[1]).toHaveTextContent('3439718.0300000007'); + expect(gridCells[2]).toHaveTextContent('152718.97999999998'); + expect(gridCells[3]).toHaveTextContent('145776.56'); + expect(gridCells[4]).toHaveTextContent('144729.96000000002'); + expect(gridCells[5]).toHaveTextContent('98089.08000000002'); + expect(gridCells[6]).toHaveTextContent('72212.86'); + expect(gridCells[7]).toHaveTextContent('48710.92'); + expect(gridCells[8]).toHaveTextContent('28550.59'); + expect(gridCells[9]).toHaveTextContent('26260.210000000003'); + expect(gridCells[10]).toHaveTextContent('24078.610000000004'); + + // Third click to sort ascending again + userEvent.click(mixedFloatColumn); + expect(gridCells[0]).toHaveTextContent('24078.610000000004'); + expect(gridCells[1]).toHaveTextContent('26260.210000000003'); + expect(gridCells[2]).toHaveTextContent('28550.59'); + expect(gridCells[3]).toHaveTextContent('48710.92'); + expect(gridCells[4]).toHaveTextContent('72212.86'); + expect(gridCells[5]).toHaveTextContent('98089.08000000002'); + expect(gridCells[6]).toHaveTextContent('144729.96000000002'); + expect(gridCells[7]).toHaveTextContent('145776.56'); + expect(gridCells[8]).toHaveTextContent('152718.97999999998'); + expect(gridCells[9]).toHaveTextContent('3439718.0300000007'); + expect(gridCells[10]).toHaveTextContent('4528047.219999993'); + }); +}); diff --git a/superset-frontend/src/components/FilterableTable/FilterableTable.tsx b/superset-frontend/src/components/FilterableTable/FilterableTable.tsx index e6605fcfc0de2..96bef2d73e3a4 100644 --- a/superset-frontend/src/components/FilterableTable/FilterableTable.tsx +++ b/superset-frontend/src/components/FilterableTable/FilterableTable.tsx @@ -321,21 +321,30 @@ export default class FilterableTable extends PureComponent< ); } + // Parse any floating numbers so they'll sort correctly + parseFloatingNums = (value: any) => { + const floatValue = parseFloat(value); + return Number.isNaN(floatValue) ? value : floatValue; + }; + sortResults(sortBy: string, descending: boolean) { return (a: Datum, b: Datum) => { - const aValue = a[sortBy]; - const bValue = b[sortBy]; + const aValue = this.parseFloatingNums(a[sortBy]); + const bValue = this.parseFloatingNums(b[sortBy]); + + // equal items sort equally if (aValue === bValue) { - // equal items sort equally return 0; } + + // nulls sort after anything else if (aValue === null) { - // nulls sort after anything else return 1; } if (bValue === null) { return -1; } + if (descending) { return aValue < bValue ? 1 : -1; }