[Bug][Table Visualization] Fix first column sort issue

[ananzh]

Description

Currently, the first column of table vis won't sort. This PR fixes the bug.

Issues Resolved

#2827

Check List

• All tests pass
  • yarn test:jest
  • yarn test:jest_integration
  • yarn test:ftr
• New functionality includes testing.
• New functionality has been documented.
• Update CHANGELOG.md
• Commits are signed per the DCO using --signoff

[AMoo-Miki]

Suggested change

	return uiState.sort.colIndex !== null && uiState.sort.direction
	? orderBy(rows, columns[uiState.sort.colIndex]?.id, uiState.sort.direction)
	: rows;
	return columns[uiState.sort?.colIndex]?.id
	? orderBy(rows, columns[uiState.sort.colIndex].id, uiState.sort.direction || <whatever the default is>)
	: rows;

[AMoo-Miki]

@ananzh confirmed that this is not directly user input.

[AMoo-Miki]

@ananzh confirmed that this is not directly user input.

[AMoo-Miki]

I am still not comfortable that we are not checking the validity of id before passing it to orderBy.

PS, I would have preferred for lodash not even be used here.

[joshuarrrr]

So, from the description, it sounds like the bug was that colIndex === 0 evaluates falsey, which was not what you intended to check.

But I agree with @AMoo-Miki - what about the case where columns is an empty array, or where the colIndex is out of bounds? Seems like you also want to check columns[uiState.sort.colIndex].id in your conditional.

[ananzh]

@AMoo-Miki Sort is done on the frontend entirely. When click sort, component will be re-rendered with the new sort index and direction. The data (table.rows) is the same. But sortedRows will be re-calculated and then render in DataGrid --> entirely frontend. For example:

table. rows (always same)
: 
Array(5)
0
: 
{col-0-2: 'New York', col-1-1: 386}
1
: 
{col-0-2: 'Dubai', col-1-1: 191}
2
: 
{col-0-2: 'Cairo', col-1-1: 184}
3
: 
{col-0-2: 'Marrakesh', col-1-1: 164}
4
: 
{col-0-2: 'Cannes', col-1-1: 159}

sortedRows
: 
Array(5)
0
: 
{col-0-2: 'Cairo', col-1-1: 184}
1
: 
{col-0-2: 'Cannes', col-1-1: 159}
2
: 
{col-0-2: 'Dubai', col-1-1: 191}
3
: 
{col-0-2: 'Marrakesh', col-1-1: 164}
4
: 
{col-0-2: 'New York', col-1-1: 386}

[ananzh]

@joshuarrrr From what I know, if there is no data, there are two cases:

• for count metric, table rows

0: 
{col-0-1: 0}

• for other metric, table rows:

0: 
{col-0-1: null}

So no matter what metric we use, we could sort. There is always a column. But if we think this is not 100% percent guaranteed, we could add more check

uiState.sort.colIndex !== null && columns[uiState.sort.colIndex].id && uiState.sort.direction

[AMoo-Miki]

I think uiState.sort.colIndex !== null && columns[uiState.sort.colIndex].id && uiState.sort.direction would be the best.

I understand that there are simpler and more effective checks and TS is being stupid.

PS, is direction really required? if sort is asked for and no direction is provided, it would be safer to just use a default.

[ananzh]

@AMoo-Miki agree. I committed the change.

[codecov-commenter]

Codecov Report

Merging #2828 (f1b868b) into main (5608f82) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2828   +/-   ##
=======================================
  Coverage   66.67%   66.68%           
=======================================
  Files        3219     3219           
  Lines       61445    61445           
  Branches     9417     9417           
=======================================
+ Hits        40966    40972    +6     
+ Misses      18232    18228    -4     
+ Partials     2247     2245    -2     

Impacted Files	Coverage Δ
...rce/components/edit_form/edit_data_source_form.tsx	91.45% <0.00%> (ø)
...ts/update_password_modal/update_password_modal.tsx	95.83% <0.00%> (ø)
...components/create_form/create_data_source_form.tsx	98.36% <0.00%> (ø)
...ic/application/models/sense_editor/sense_editor.ts	64.88% <0.00%> (+0.88%)	⬆️
packages/osd-optimizer/src/node/cache.ts	52.63% <0.00%> (+2.63%)	⬆️
...s/osd-optimizer/src/node/node_auto_tranpilation.ts	87.75% <0.00%> (+4.08%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.