Skip to content

Commit

Permalink
fix(sqllab): Floating numbers not sorting correctly in result column (#…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
lyndsiWilliams authored and eschutho committed Jan 27, 2022
1 parent f37525d commit ab32149
Show file tree
Hide file tree
Showing 2 changed files with 201 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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(<FilterableTable {...stringProps} />);

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(<FilterableTable {...integerProps} />);

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(<FilterableTable {...floatProps} />);

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(<FilterableTable {...mixedFloatProps} />);

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');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down

0 comments on commit ab32149

Please sign in to comment.