From b6b996398148a99dcc83e4754c430d50e24a1204 Mon Sep 17 00:00:00 2001 From: Spenser Hale Date: Tue, 17 Sep 2024 11:52:50 -0700 Subject: [PATCH 1/2] Add null handling to sorting logic and update tests Adjusted sorting functions to handle null values by placing them at the end. Updated test cases to validate the new sorting behavior, and included new fixture data for testing scenarios with null values. --- .../components/dataviews/stories/fixtures.tsx | 19 ++++++ packages/dataviews/src/field-types/index.tsx | 7 ++- packages/dataviews/src/field-types/text.tsx | 2 + .../src/test/filter-and-sort-data-view.js | 61 ++++++++++++++++++- 4 files changed, 86 insertions(+), 3 deletions(-) diff --git a/packages/dataviews/src/components/dataviews/stories/fixtures.tsx b/packages/dataviews/src/components/dataviews/stories/fixtures.tsx index ff098209b34684..b179601de0376a 100644 --- a/packages/dataviews/src/components/dataviews/stories/fixtures.tsx +++ b/packages/dataviews/src/components/dataviews/stories/fixtures.tsx @@ -696,3 +696,22 @@ export const fields: Field< SpaceObject >[] = [ enableSorting: false, }, ]; + +export const sortDataWithNull = [ + { + id: 1, + message: null, + }, + { + id: 2, + message: 'Something went wrong', + }, + { + id: 3, + message: null, + }, + { + id: 4, + message: 'An error has occurred', + }, +]; diff --git a/packages/dataviews/src/field-types/index.tsx b/packages/dataviews/src/field-types/index.tsx index eb9dada479c6bf..df1773af5ddcb1 100644 --- a/packages/dataviews/src/field-types/index.tsx +++ b/packages/dataviews/src/field-types/index.tsx @@ -27,13 +27,16 @@ export default function getFieldTypeDefinition( type?: FieldType ) { return { sort: ( a: any, b: any, direction: SortDirection ) => { + if (a === null) return 1; + if (b === null) return -1; + if ( typeof a === 'number' && typeof b === 'number' ) { return direction === 'asc' ? a - b : b - a; } return direction === 'asc' - ? a.localeCompare( b ) - : b.localeCompare( a ); + ? a.localeCompare(b) + : b.localeCompare(a); }, isValid: ( value: any, context?: ValidationContext ) => { if ( context?.elements ) { diff --git a/packages/dataviews/src/field-types/text.tsx b/packages/dataviews/src/field-types/text.tsx index 76ff699d0848c4..57b89237e90a6e 100644 --- a/packages/dataviews/src/field-types/text.tsx +++ b/packages/dataviews/src/field-types/text.tsx @@ -4,6 +4,8 @@ import type { SortDirection, ValidationContext } from '../types'; function sort( valueA: any, valueB: any, direction: SortDirection ) { + if (valueA === null) return 1; + if (valueB === null) return -1; return direction === 'asc' ? valueA.localeCompare( valueB ) : valueB.localeCompare( valueA ); diff --git a/packages/dataviews/src/test/filter-and-sort-data-view.js b/packages/dataviews/src/test/filter-and-sort-data-view.js index 7f0b3a30e8587c..b0deed7881d38d 100644 --- a/packages/dataviews/src/test/filter-and-sort-data-view.js +++ b/packages/dataviews/src/test/filter-and-sort-data-view.js @@ -2,7 +2,11 @@ * Internal dependencies */ import { filterSortAndPaginate } from '../filter-and-sort-data-view'; -import { data, fields } from '../components/dataviews/stories/fixtures'; +import { + data, + fields, + sortDataWithNull, +} from '../components/dataviews/stories/fixtures'; describe( 'filters', () => { it( 'should return empty if the data is empty', () => { @@ -338,6 +342,61 @@ describe( 'sorting', () => { expect( result[ 0 ].title ).toBe( 'Uranus' ); expect( result[ 1 ].title ).toBe( 'Neptune' ); } ); + + const testSorting = ( data, fields, sortDirection, expectedResults ) => { + const { data: result } = filterSortAndPaginate( + data, + { + sort: { field: fields[ 0 ].id, direction: sortDirection }, + }, + fields + ); + + result.forEach( ( item, index ) => { + expect( item[ fields[ 0 ].id ] ).toBe( expectedResults[ index ] ); + } ); + }; + + const expectedNullableResults = { + asc: [ 'An error has occurred', 'Something went wrong', null, null ], + desc: [ 'Something went wrong', 'An error has occurred', null, null ], + }; + + it( 'should asc sort null values to end for fields without type definition', () => { + testSorting( + sortDataWithNull, + [ { label: 'Error Message', id: 'message' } ], + 'asc', + expectedNullableResults.asc + ); + } ); + + it( 'should desc sort null values to end for fields without type definition', () => { + testSorting( + sortDataWithNull, + [ { label: 'Error Message', id: 'message' } ], + 'desc', + expectedNullableResults.desc + ); + } ); + + it( 'should asc sort null values to end for text fields type definition', () => { + testSorting( + sortDataWithNull, + [ { id: 'message', type: 'text' } ], + 'asc', + expectedNullableResults.asc + ); + } ); + + it( 'should desc sort null values to end for text fields type definition', () => { + testSorting( + sortDataWithNull, + [ { id: 'message', type: 'text' } ], + 'desc', + expectedNullableResults.desc + ); + } ); } ); describe( 'pagination', () => { From 83d33c914fb8504768315d58d50955def99850ac Mon Sep 17 00:00:00 2001 From: Spenser Hale Date: Tue, 17 Sep 2024 12:11:18 -0700 Subject: [PATCH 2/2] Add null handling to sorting logic and update tests Adjusted sorting functions to handle null values by placing them at the end. Updated test cases to validate the new sorting behavior, and included new fixture data for testing scenarios with null values. --- .../components/dataviews/stories/fixtures.tsx | 8 ++++ .../dataviews/src/field-types/datetime.tsx | 3 ++ .../dataviews/src/field-types/integer.tsx | 3 ++ .../src/test/filter-and-sort-data-view.js | 40 ++++++++++++++++++- 4 files changed, 52 insertions(+), 2 deletions(-) diff --git a/packages/dataviews/src/components/dataviews/stories/fixtures.tsx b/packages/dataviews/src/components/dataviews/stories/fixtures.tsx index b179601de0376a..52eb764e4822eb 100644 --- a/packages/dataviews/src/components/dataviews/stories/fixtures.tsx +++ b/packages/dataviews/src/components/dataviews/stories/fixtures.tsx @@ -701,17 +701,25 @@ export const sortDataWithNull = [ { id: 1, message: null, + count: null, + login_at: null, }, { id: 2, message: 'Something went wrong', + count: 1, + login_at: '2021-01-01T00:00:00Z', }, { id: 3, message: null, + count: null, + login_at: null, }, { id: 4, message: 'An error has occurred', + count: 5, + login_at: '2024-01-01T00:00:00Z', }, ]; diff --git a/packages/dataviews/src/field-types/datetime.tsx b/packages/dataviews/src/field-types/datetime.tsx index aa97fc86c318c2..7530a3983b57f7 100644 --- a/packages/dataviews/src/field-types/datetime.tsx +++ b/packages/dataviews/src/field-types/datetime.tsx @@ -4,6 +4,9 @@ import type { SortDirection, ValidationContext } from '../types'; function sort( a: any, b: any, direction: SortDirection ) { + if (a === null) return 1; + if (b === null) return -1; + const timeA = new Date( a ).getTime(); const timeB = new Date( b ).getTime(); diff --git a/packages/dataviews/src/field-types/integer.tsx b/packages/dataviews/src/field-types/integer.tsx index f57c8e382db816..e860a4192bd133 100644 --- a/packages/dataviews/src/field-types/integer.tsx +++ b/packages/dataviews/src/field-types/integer.tsx @@ -4,6 +4,9 @@ import type { SortDirection, ValidationContext } from '../types'; function sort( a: any, b: any, direction: SortDirection ) { + if (a === null) return 1; + if (b === null) return -1; + return direction === 'asc' ? a - b : b - a; } diff --git a/packages/dataviews/src/test/filter-and-sort-data-view.js b/packages/dataviews/src/test/filter-and-sort-data-view.js index b0deed7881d38d..da3e3b1e57e8d9 100644 --- a/packages/dataviews/src/test/filter-and-sort-data-view.js +++ b/packages/dataviews/src/test/filter-and-sort-data-view.js @@ -365,7 +365,7 @@ describe( 'sorting', () => { it( 'should asc sort null values to end for fields without type definition', () => { testSorting( sortDataWithNull, - [ { label: 'Error Message', id: 'message' } ], + [ { id: 'message' } ], 'asc', expectedNullableResults.asc ); @@ -374,7 +374,7 @@ describe( 'sorting', () => { it( 'should desc sort null values to end for fields without type definition', () => { testSorting( sortDataWithNull, - [ { label: 'Error Message', id: 'message' } ], + [ { id: 'message' } ], 'desc', expectedNullableResults.desc ); @@ -397,6 +397,42 @@ describe( 'sorting', () => { expectedNullableResults.desc ); } ); + + it( 'should asc sort null values to end for integer fields type definition', () => { + testSorting( + sortDataWithNull, + [ { id: 'count', type: 'integer' } ], + 'asc', + [ 1, 5, null, null ] + ); + } ); + + it( 'should desc sort null values to end for integer fields type definition', () => { + testSorting( + sortDataWithNull, + [ { id: 'count', type: 'integer' } ], + 'desc', + [ 5, 1, null, null ] + ); + } ); + + it( 'should asc sort null values to end for datetime fields type definition', () => { + testSorting( + sortDataWithNull, + [ { id: 'login_at', type: 'datetime' } ], + 'asc', + [ '2021-01-01T00:00:00Z', '2024-01-01T00:00:00Z', null, null ] + ); + } ); + + it( 'should desc sort null values to end for datetime fields type definition', () => { + testSorting( + sortDataWithNull, + [ { id: 'login_at', type: 'datetime' } ], + 'desc', + [ '2024-01-01T00:00:00Z', '2021-01-01T00:00:00Z', null, null ] + ); + } ); } ); describe( 'pagination', () => {