From e0e6c65cd251e547ade49dafb7f95bdf9a203886 Mon Sep 17 00:00:00 2001 From: Marcos Iglesias Date: Tue, 29 Sep 2020 22:43:24 -0700 Subject: [PATCH 01/10] Testing types for table input Signed-off-by: Marcos Iglesias --- .../static/js/components/ColumnList/index.tsx | 2 +- .../js/components/common/Table/index.tsx | 12 +++++--- .../components/common/Table/table.story.tsx | 7 +++++ .../common/Table/testDataBuilder.tsx | 30 +++++++++++++++++++ 4 files changed, 46 insertions(+), 5 deletions(-) diff --git a/amundsen_application/static/js/components/ColumnList/index.tsx b/amundsen_application/static/js/components/ColumnList/index.tsx index 9c63f01b8..61b62ab31 100644 --- a/amundsen_application/static/js/components/ColumnList/index.tsx +++ b/amundsen_application/static/js/components/ColumnList/index.tsx @@ -234,7 +234,7 @@ const ColumnList: React.FC = ({ formattedAndOrderedData = formattedAndOrderedData.reverse(); } - let formattedColumns: ReusableTableColumn[] = [ + let formattedColumns: ReusableTableColumn[] = [ { title: 'Name', field: 'content', diff --git a/amundsen_application/static/js/components/common/Table/index.tsx b/amundsen_application/static/js/components/common/Table/index.tsx index 3cb7c93a6..53507b4c6 100644 --- a/amundsen_application/static/js/components/common/Table/index.tsx +++ b/amundsen_application/static/js/components/common/Table/index.tsx @@ -16,15 +16,19 @@ export enum TextAlignmentValues { right = 'right', center = 'center', } -export interface TableColumn { +export interface TableColumn { title: string; - field: string; + field: T; horAlign?: TextAlignmentValues; component?: (value: any, index: number) => React.ReactNode; width?: number; // sortable?: bool (false) } +interface RowData { + [key: string]: any; +} + export interface TableOptions { tableClassName?: string; isLoading?: boolean; @@ -37,8 +41,8 @@ export interface TableOptions { } export interface TableProps { - columns: TableColumn[]; - data: any[]; + data: RowData[]; + columns: TableColumn[]; options?: TableOptions; } diff --git a/amundsen_application/static/js/components/common/Table/table.story.tsx b/amundsen_application/static/js/components/common/Table/table.story.tsx index e14eeb80c..2047addeb 100644 --- a/amundsen_application/static/js/components/common/Table/table.story.tsx +++ b/amundsen_application/static/js/components/common/Table/table.story.tsx @@ -33,6 +33,10 @@ const { columns: columnsWithCollapsedRow, data: dataWithCollapsedRow, } = dataBuilder.withCollapsedRow().build(); +const { + columns: columnsWithWrongData, + data: dataWithWrongData, +} = dataBuilder.withWrongData().build(); const expandRowComponent = (rowValue, index) => ( {index}:{rowValue.value} @@ -62,6 +66,9 @@ export const TableStates = () => ( options={object('options', { isLoading: true })} /> + + + ); diff --git a/amundsen_application/static/js/components/common/Table/testDataBuilder.tsx b/amundsen_application/static/js/components/common/Table/testDataBuilder.tsx index 9a6413ac1..f53dc78fe 100644 --- a/amundsen_application/static/js/components/common/Table/testDataBuilder.tsx +++ b/amundsen_application/static/js/components/common/Table/testDataBuilder.tsx @@ -44,6 +44,36 @@ function TestDataBuilder(config = {}) { ...config, }; + this.withWrongData = () => { + const attr = { + data: [ + { name: 'rowName', type: 'rowType', value: 1 }, + { name: 'rowName2', type: 'rowType2', value: 2 }, + { name: 'rowName3', type: 'rowType3', value: 3 }, + ], + columns: [ + { + title: 'Name', + field: 'name', + }, + { + title: 'Type', + field: 'type', + }, + { + title: 'Value', + field: 'value', + }, + { + title: 'Bad Field', + field: 'badfield', + }, + ], + }; + + return new this.Klass(attr); + }; + this.withUsageRow = () => { const attr = { data: [ From 5e4a095a388c2cae3d8856c890834114788fe29c Mon Sep 17 00:00:00 2001 From: Marcos Iglesias Date: Wed, 30 Sep 2020 18:20:48 -0700 Subject: [PATCH 02/10] Removing generic Signed-off-by: Marcos Iglesias --- .../static/js/components/ColumnList/index.tsx | 2 +- .../static/js/components/common/Table/index.tsx | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/amundsen_application/static/js/components/ColumnList/index.tsx b/amundsen_application/static/js/components/ColumnList/index.tsx index 61b62ab31..9c63f01b8 100644 --- a/amundsen_application/static/js/components/ColumnList/index.tsx +++ b/amundsen_application/static/js/components/ColumnList/index.tsx @@ -234,7 +234,7 @@ const ColumnList: React.FC = ({ formattedAndOrderedData = formattedAndOrderedData.reverse(); } - let formattedColumns: ReusableTableColumn[] = [ + let formattedColumns: ReusableTableColumn[] = [ { title: 'Name', field: 'content', diff --git a/amundsen_application/static/js/components/common/Table/index.tsx b/amundsen_application/static/js/components/common/Table/index.tsx index 53507b4c6..7a702e985 100644 --- a/amundsen_application/static/js/components/common/Table/index.tsx +++ b/amundsen_application/static/js/components/common/Table/index.tsx @@ -16,9 +16,9 @@ export enum TextAlignmentValues { right = 'right', center = 'center', } -export interface TableColumn { +export interface TableColumn { title: string; - field: T; + field: string; horAlign?: TextAlignmentValues; component?: (value: any, index: number) => React.ReactNode; width?: number; @@ -42,7 +42,7 @@ export interface TableOptions { export interface TableProps { data: RowData[]; - columns: TableColumn[]; + columns: TableColumn[]; options?: TableOptions; } From 46c6160a80ee2360ca4da6f299768d1886c4a8f1 Mon Sep 17 00:00:00 2001 From: Marcos Iglesias Date: Wed, 30 Sep 2020 21:02:01 -0700 Subject: [PATCH 03/10] Adds data validation to table Signed-off-by: Marcos Iglesias --- amundsen_application/static/.betterer.results | 3 --- amundsen_application/static/.storybook/manager.ts | 2 +- .../js/components/common/Table/index.spec.tsx | 13 +++++++++++++ .../static/js/components/common/Table/index.tsx | 13 +++++++++++++ .../js/components/common/Table/table.story.tsx | 9 --------- 5 files changed, 27 insertions(+), 13 deletions(-) diff --git a/amundsen_application/static/.betterer.results b/amundsen_application/static/.betterer.results index b8b70735c..a9617207d 100644 --- a/amundsen_application/static/.betterer.results +++ b/amundsen_application/static/.betterer.results @@ -99,9 +99,6 @@ exports[`strict null compilation`] = { [255, 6, 11, "No overload matches this call.\\n The last overload gave the following error.\\n Type \'(() => SubmitSearchRequest) | null\' is not assignable to type \'ActionCreator\'.\\n Type \'null\' is not assignable to type \'ActionCreator\'.", "2296208050"], [270, 4, 18, "No overload matches this call.\\n The last overload gave the following error.\\n Argument of type \'(dispatch: any, ownProps: any) => ActionCreator\' is not assignable to parameter of type \'DispatchFromProps\'.\\n Type \'(dispatch: any, ownProps: any) => ActionCreator\' is missing the following properties from type \'DispatchFromProps\': submitSearch, onInputChange, onSelectInlineResult", "2926224796"] ], - "js/components/common/Table/index.tsx:489040313": [ - [235, 22, 13, "Type \'unknown\' is not assignable to type \'ReactNode\'.\\n Type \'unknown\' is not assignable to type \'ReactPortal\'.", "971959308"] - ], "js/components/common/Tags/TagInput/index.tsx:3754832290": [ [63, 22, 6, "Type \'undefined\' is not assignable to type \'GetAllTagsRequest\'.", "1979467425"], [66, 4, 4, "Type \'undefined\' is not assignable to type \'Tag[]\'.", "2087952548"], diff --git a/amundsen_application/static/.storybook/manager.ts b/amundsen_application/static/.storybook/manager.ts index 8b75d125d..a26d9b09d 100644 --- a/amundsen_application/static/.storybook/manager.ts +++ b/amundsen_application/static/.storybook/manager.ts @@ -6,7 +6,7 @@ addons.setConfig({ isFullscreen: false, showNav: true, showPanel: true, - panelPosition: 'bottom', + panelPosition: 'right', sidebarAnimations: true, enableShortcuts: true, isToolshown: true, diff --git a/amundsen_application/static/js/components/common/Table/index.spec.tsx b/amundsen_application/static/js/components/common/Table/index.spec.tsx index f26e4daaf..e7fc6d226 100644 --- a/amundsen_application/static/js/components/common/Table/index.spec.tsx +++ b/amundsen_application/static/js/components/common/Table/index.spec.tsx @@ -400,6 +400,19 @@ describe('Table', () => { }); }); }); + + describe('when columns specify fields not in the data', () => { + const { columns, data } = dataBuilder.withWrongData().build(); + + it('throws an error', () => { + expect(() => { + setup({ + data, + columns, + }); + }).toThrow(); + }); + }); }); describe('options', () => { diff --git a/amundsen_application/static/js/components/common/Table/index.tsx b/amundsen_application/static/js/components/common/Table/index.tsx index 7a702e985..c5a22d52a 100644 --- a/amundsen_application/static/js/components/common/Table/index.tsx +++ b/amundsen_application/static/js/components/common/Table/index.tsx @@ -48,6 +48,8 @@ export interface TableProps { const DEFAULT_EMPTY_MESSAGE = 'No Results'; const EXPAND_ROW_TEXT = 'Expand Row'; +const INVALID_DATA_ERROR_MESSAGE = + 'Error: Invalid data! Your data does not contain the fields specified on the columns property.'; const DEFAULT_LOADING_ITEMS = 3; const DEFAULT_ROW_HEIGHT = 30; const EXPANDING_CELL_WIDTH = '70px'; @@ -72,6 +74,15 @@ type EmptyRowProps = { const getCellAlignmentClass = (alignment: TextAlignmentValues) => ALIGNEMENT_TO_CLASS_MAP[alignment]; +const fieldIsDefined = (field, row) => row[field] !== undefined; +const checkValidData = (data, fields) => { + fields.forEach((field) => { + if (data.filter(fieldIsDefined.bind(null, field)).length === 0) { + throw new Error(INVALID_DATA_ERROR_MESSAGE); + } + }); +}; + const EmptyRow: React.FC = ({ colspan, rowStyles, @@ -186,6 +197,8 @@ const Table: React.FC = ({ ); if (data.length) { + checkValidData(data, fields); + body = data.map((item, index) => { return ( diff --git a/amundsen_application/static/js/components/common/Table/table.story.tsx b/amundsen_application/static/js/components/common/Table/table.story.tsx index 2047addeb..7a0563fbf 100644 --- a/amundsen_application/static/js/components/common/Table/table.story.tsx +++ b/amundsen_application/static/js/components/common/Table/table.story.tsx @@ -33,18 +33,12 @@ const { columns: columnsWithCollapsedRow, data: dataWithCollapsedRow, } = dataBuilder.withCollapsedRow().build(); -const { - columns: columnsWithWrongData, - data: dataWithWrongData, -} = dataBuilder.withWrongData().build(); const expandRowComponent = (rowValue, index) => ( {index}:{rowValue.value} ); -// const stories = storiesOf('Components/Table', module); - export const TableStates = () => ( <> @@ -66,9 +60,6 @@ export const TableStates = () => ( options={object('options', { isLoading: true })} /> - -
- ); From 0bcc23cbe2d08a1ffe6a19c7d323424ab677ff48 Mon Sep 17 00:00:00 2001 From: Marcos Iglesias Date: Wed, 30 Sep 2020 21:19:52 -0700 Subject: [PATCH 04/10] Removing verbose Error string Signed-off-by: Marcos Iglesias --- .../static/js/components/common/Table/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/amundsen_application/static/js/components/common/Table/index.tsx b/amundsen_application/static/js/components/common/Table/index.tsx index c5a22d52a..9cb37b83b 100644 --- a/amundsen_application/static/js/components/common/Table/index.tsx +++ b/amundsen_application/static/js/components/common/Table/index.tsx @@ -49,7 +49,7 @@ export interface TableProps { const DEFAULT_EMPTY_MESSAGE = 'No Results'; const EXPAND_ROW_TEXT = 'Expand Row'; const INVALID_DATA_ERROR_MESSAGE = - 'Error: Invalid data! Your data does not contain the fields specified on the columns property.'; + 'Invalid data! Your data does not contain the fields specified on the columns property.'; const DEFAULT_LOADING_ITEMS = 3; const DEFAULT_ROW_HEIGHT = 30; const EXPANDING_CELL_WIDTH = '70px'; From 47981e983381d15fac9de330b250295169a612da Mon Sep 17 00:00:00 2001 From: Marcos Iglesias Date: Wed, 30 Sep 2020 21:45:32 -0700 Subject: [PATCH 05/10] Adding Gago's recommendation Signed-off-by: Marcos Iglesias --- .../js/components/common/Table/index.tsx | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/amundsen_application/static/js/components/common/Table/index.tsx b/amundsen_application/static/js/components/common/Table/index.tsx index 9cb37b83b..4ba7deecb 100644 --- a/amundsen_application/static/js/components/common/Table/index.tsx +++ b/amundsen_application/static/js/components/common/Table/index.tsx @@ -73,14 +73,24 @@ type EmptyRowProps = { const getCellAlignmentClass = (alignment: TextAlignmentValues) => ALIGNEMENT_TO_CLASS_MAP[alignment]; - const fieldIsDefined = (field, row) => row[field] !== undefined; -const checkValidData = (data, fields) => { + +type Some = string | number | boolean | symbol | bigint | object; +type ValidData = Record; // Removes the undefined | null values + +const checkValidData = ( + data: unknown[], + fields: string[] +): data is ValidData[] => { + let isValid = true; + fields.forEach((field) => { if (data.filter(fieldIsDefined.bind(null, field)).length === 0) { - throw new Error(INVALID_DATA_ERROR_MESSAGE); + isValid = false; } }); + + return isValid; }; const EmptyRow: React.FC = ({ @@ -197,7 +207,9 @@ const Table: React.FC = ({ ); if (data.length) { - checkValidData(data, fields); + if (!checkValidData(data, fields)) { + throw new Error(INVALID_DATA_ERROR_MESSAGE); + } body = data.map((item, index) => { return ( From d2b49f0fd67f3ae0eb4e94cf55eaff70728942e4 Mon Sep 17 00:00:00 2001 From: Marcos Iglesias Date: Wed, 30 Sep 2020 21:47:39 -0700 Subject: [PATCH 06/10] Cleanup and reorganization Signed-off-by: Marcos Iglesias --- .../js/components/common/Table/index.tsx | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/amundsen_application/static/js/components/common/Table/index.tsx b/amundsen_application/static/js/components/common/Table/index.tsx index 4ba7deecb..a6f52abcf 100644 --- a/amundsen_application/static/js/components/common/Table/index.tsx +++ b/amundsen_application/static/js/components/common/Table/index.tsx @@ -24,9 +24,11 @@ export interface TableColumn { width?: number; // sortable?: bool (false) } +type Some = string | number | boolean | symbol | bigint | object; +type ValidData = Record; // Removes the undefined | null values interface RowData { - [key: string]: any; + [key: string]: Some; } export interface TableOptions { @@ -46,6 +48,16 @@ export interface TableProps { options?: TableOptions; } +type RowStyles = { + height: string; +}; + +type EmptyRowProps = { + colspan: number; + rowStyles: RowStyles; + emptyMessage?: string; +}; + const DEFAULT_EMPTY_MESSAGE = 'No Results'; const EXPAND_ROW_TEXT = 'Expand Row'; const INVALID_DATA_ERROR_MESSAGE = @@ -61,24 +73,12 @@ const ALIGNEMENT_TO_CLASS_MAP = { center: 'is-center-aligned', }; -type RowStyles = { - height: string; -}; - -type EmptyRowProps = { - colspan: number; - rowStyles: RowStyles; - emptyMessage?: string; -}; - const getCellAlignmentClass = (alignment: TextAlignmentValues) => ALIGNEMENT_TO_CLASS_MAP[alignment]; -const fieldIsDefined = (field, row) => row[field] !== undefined; -type Some = string | number | boolean | symbol | bigint | object; -type ValidData = Record; // Removes the undefined | null values +const fieldIsDefined = (field, row) => row[field] !== undefined; -const checkValidData = ( +const checkIfValidData = ( data: unknown[], fields: string[] ): data is ValidData[] => { @@ -207,7 +207,7 @@ const Table: React.FC = ({ ); if (data.length) { - if (!checkValidData(data, fields)) { + if (!checkIfValidData(data, fields)) { throw new Error(INVALID_DATA_ERROR_MESSAGE); } From ed1b4777306a20bf2cfd9c409336f74e202a3980 Mon Sep 17 00:00:00 2001 From: Marcos Iglesias Date: Wed, 30 Sep 2020 22:12:25 -0700 Subject: [PATCH 07/10] Using a for loop Signed-off-by: Marcos Iglesias --- .../static/js/components/common/Table/index.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/amundsen_application/static/js/components/common/Table/index.tsx b/amundsen_application/static/js/components/common/Table/index.tsx index a6f52abcf..db71eb51e 100644 --- a/amundsen_application/static/js/components/common/Table/index.tsx +++ b/amundsen_application/static/js/components/common/Table/index.tsx @@ -84,12 +84,12 @@ const checkIfValidData = ( ): data is ValidData[] => { let isValid = true; - fields.forEach((field) => { - if (data.filter(fieldIsDefined.bind(null, field)).length === 0) { + for (let i = 0; i < fields.length; i++) { + if (data.filter(fieldIsDefined.bind(null, fields[i])).length === 0) { isValid = false; + break; } - }); - + } return isValid; }; From a2167440ce1c900ca32b4a9e07fab902c68d7550 Mon Sep 17 00:00:00 2001 From: Marcos Iglesias Date: Wed, 30 Sep 2020 22:21:31 -0700 Subject: [PATCH 08/10] Reducing noise on tests, using some Signed-off-by: Marcos Iglesias --- .../static/js/components/common/Table/index.spec.tsx | 9 +++++++++ .../static/js/components/common/Table/index.tsx | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/amundsen_application/static/js/components/common/Table/index.spec.tsx b/amundsen_application/static/js/components/common/Table/index.spec.tsx index e7fc6d226..3e60e3615 100644 --- a/amundsen_application/static/js/components/common/Table/index.spec.tsx +++ b/amundsen_application/static/js/components/common/Table/index.spec.tsx @@ -404,6 +404,15 @@ describe('Table', () => { describe('when columns specify fields not in the data', () => { const { columns, data } = dataBuilder.withWrongData().build(); + beforeEach(() => { + jest.spyOn(console, 'error'); + console.error.mockImplementation(() => {}); + }); + + afterEach(() => { + console.error.mockRestore(); + }); + it('throws an error', () => { expect(() => { setup({ diff --git a/amundsen_application/static/js/components/common/Table/index.tsx b/amundsen_application/static/js/components/common/Table/index.tsx index db71eb51e..6d2118f0d 100644 --- a/amundsen_application/static/js/components/common/Table/index.tsx +++ b/amundsen_application/static/js/components/common/Table/index.tsx @@ -85,7 +85,7 @@ const checkIfValidData = ( let isValid = true; for (let i = 0; i < fields.length; i++) { - if (data.filter(fieldIsDefined.bind(null, fields[i])).length === 0) { + if (!data.some(fieldIsDefined.bind(null, fields[i]))) { isValid = false; break; } From 55100852e6f5272cf1f8d01a18fe9cabbbb3579b Mon Sep 17 00:00:00 2001 From: Marcos Iglesias Date: Wed, 30 Sep 2020 22:44:04 -0700 Subject: [PATCH 09/10] Moving editUrl and editText to null on the table Signed-off-by: Marcos Iglesias --- .../static/js/components/ColumnList/index.tsx | 12 ++++++------ .../static/js/components/common/Table/index.spec.tsx | 5 +++-- .../static/js/components/common/Table/index.tsx | 4 ++-- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/amundsen_application/static/js/components/ColumnList/index.tsx b/amundsen_application/static/js/components/ColumnList/index.tsx index 9c63f01b8..2dfb5f80e 100644 --- a/amundsen_application/static/js/components/ColumnList/index.tsx +++ b/amundsen_application/static/js/components/ColumnList/index.tsx @@ -76,8 +76,8 @@ type FormattedDataType = { usage: number | null; stats: StatType | null; action: string; - editText?: string; - editUrl?: string; + editText: string | null; + editUrl: string | null; index: number; name: string; sort_order: string; @@ -168,8 +168,8 @@ const ExpandedRowComponent: React.FC = ( = ({ action: item.name, name: item.name, isEditable: item.is_editable, - editText, - editUrl, + editText: editText || null, + editUrl: editUrl || null, index, }; }); diff --git a/amundsen_application/static/js/components/common/Table/index.spec.tsx b/amundsen_application/static/js/components/common/Table/index.spec.tsx index 3e60e3615..cf59724b8 100644 --- a/amundsen_application/static/js/components/common/Table/index.spec.tsx +++ b/amundsen_application/static/js/components/common/Table/index.spec.tsx @@ -3,6 +3,7 @@ import * as React from 'react'; import { mount } from 'enzyme'; +import { mocked } from 'ts-jest/utils'; import Table, { TableProps } from '.'; @@ -406,11 +407,11 @@ describe('Table', () => { beforeEach(() => { jest.spyOn(console, 'error'); - console.error.mockImplementation(() => {}); + mocked(console.error).mockImplementation(() => {}); }); afterEach(() => { - console.error.mockRestore(); + mocked(console.error).mockRestore(); }); it('throws an error', () => { diff --git a/amundsen_application/static/js/components/common/Table/index.tsx b/amundsen_application/static/js/components/common/Table/index.tsx index 6d2118f0d..8410a1ff2 100644 --- a/amundsen_application/static/js/components/common/Table/index.tsx +++ b/amundsen_application/static/js/components/common/Table/index.tsx @@ -25,10 +25,10 @@ export interface TableColumn { // sortable?: bool (false) } type Some = string | number | boolean | symbol | bigint | object; -type ValidData = Record; // Removes the undefined | null values +type ValidData = Record; // Removes the undefined values interface RowData { - [key: string]: Some; + [key: string]: Some | null; } export interface TableOptions { From 59b448217709a41b2eb2443339f224c5630fb76b Mon Sep 17 00:00:00 2001 From: Marcos Iglesias <190833+Golodhros@users.noreply.github.com> Date: Wed, 30 Sep 2020 22:47:26 -0700 Subject: [PATCH 10/10] Update amundsen_application/static/js/components/common/Table/index.spec.tsx Signed-off-by: Marcos Iglesias Valle Co-authored-by: Gago --- .../static/js/components/common/Table/index.spec.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/amundsen_application/static/js/components/common/Table/index.spec.tsx b/amundsen_application/static/js/components/common/Table/index.spec.tsx index cf59724b8..8b33e0618 100644 --- a/amundsen_application/static/js/components/common/Table/index.spec.tsx +++ b/amundsen_application/static/js/components/common/Table/index.spec.tsx @@ -407,7 +407,7 @@ describe('Table', () => { beforeEach(() => { jest.spyOn(console, 'error'); - mocked(console.error).mockImplementation(() => {}); + mocked(console.error).mockImplementation(jest.fn); }); afterEach(() => {