From 04557225a0a9e9c3987ba70b2a01ef40282ffc98 Mon Sep 17 00:00:00 2001 From: cccs-Dustin <96579982+cccs-Dustin@users.noreply.github.com> Date: Thu, 10 Mar 2022 10:12:52 -0500 Subject: [PATCH 1/6] Added new feature flag, all that is left to do is add and test unit tests --- RESOURCES/FEATURE_FLAGS.md | 1 + .../src/utils/featureFlags.ts | 1 + .../Datasource/DatasourceEditor.jsx | 1 + .../Datasource/DatasourceEditor.test.jsx | 45 ++++++++++++++++++- superset/config.py | 2 + superset/views/base.py | 1 + 6 files changed, 50 insertions(+), 1 deletion(-) diff --git a/RESOURCES/FEATURE_FLAGS.md b/RESOURCES/FEATURE_FLAGS.md index c19fac7b069..32cdf2c9161 100644 --- a/RESOURCES/FEATURE_FLAGS.md +++ b/RESOURCES/FEATURE_FLAGS.md @@ -26,6 +26,7 @@ These features are considered **unfinished** and should only be used on developm - DASHBOARD_CACHE - DASHBOARD_NATIVE_FILTERS_SET - DISABLE_DATASET_SOURCE_EDIT +- ENABLE_EDIT_COLUMN_TYPE - ENABLE_EXPLORE_JSON_CSRF_PROTECTION - KV_STORE - PRESTO_EXPAND_DATA diff --git a/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts b/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts index cab4f727411..8f13afe5ca2 100644 --- a/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts +++ b/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts @@ -34,6 +34,7 @@ export enum FeatureFlag { DISABLE_LEGACY_DATASOURCE_EDITOR = 'DISABLE_LEGACY_DATASOURCE_EDITOR', ENABLE_REACT_CRUD_VIEWS = 'ENABLE_REACT_CRUD_VIEWS', DISABLE_DATASET_SOURCE_EDIT = 'DISABLE_DATASET_SOURCE_EDIT', + ENABLE_EDIT_COLUMN_TYPE = 'ENABLE_EDIT_COLUMN_TYPE', DISPLAY_MARKDOWN_HTML = 'DISPLAY_MARKDOWN_HTML', ESCAPE_MARKDOWN_HTML = 'ESCAPE_MARKDOWN_HTML', DASHBOARD_NATIVE_FILTERS = 'DASHBOARD_NATIVE_FILTERS', diff --git a/superset-frontend/src/components/Datasource/DatasourceEditor.jsx b/superset-frontend/src/components/Datasource/DatasourceEditor.jsx index 1b6de1c7a90..017a8c8599f 100644 --- a/superset-frontend/src/components/Datasource/DatasourceEditor.jsx +++ b/superset-frontend/src/components/Datasource/DatasourceEditor.jsx @@ -1272,6 +1272,7 @@ class DatasourceEditor extends React.PureComponent { this.setColumns({ databaseColumns }) } onDatasourceChange={this.onDatasourceChange} + allowEditDataType={isFeatureEnabled(FeatureFlag.ENABLE_EDIT_COLUMN_TYPE)} /> {this.state.metadataLoading && } diff --git a/superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx b/superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx index dec75afdc33..7e443f79f8d 100644 --- a/superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx +++ b/superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx @@ -69,7 +69,11 @@ describe('DatasourceEditor', () => { })); // to add, remove and modify columns accordingly - it('can modify columns', async () => { + it('can modify columns except data type when feature flag is disabled', async () => { + const isFeatureEnabledMock = jest + .spyOn(featureFlags, 'isFeatureEnabled') + .mockImplementation(() => false); + const columnsTab = screen.getByTestId('collection-tab-Columns'); userEvent.click(columnsTab); @@ -93,6 +97,45 @@ describe('DatasourceEditor', () => { userEvent.type(await inputDtmFormat, 'test'); userEvent.type(await inputCertifiedBy, 'test'); userEvent.type(await inputCertDetails, 'test'); + + isFeatureEnabledMock.mockRestore(); + }); + + // to add, remove and modify columns accordingly + it('can modify column data type when feature flag enabled', async () => { + const isFeatureEnabledMock = jest + .spyOn(featureFlags, 'isFeatureEnabled') + .mockImplementation(() => true); + + const columnsTab = screen.getByTestId('collection-tab-Columns'); + userEvent.click(columnsTab); + + const getToggles = screen.getAllByRole('button', { + name: /toggle expand/i, + }); + userEvent.click(getToggles[0]); + const getTextboxes = screen.getAllByRole('textbox'); + expect(getTextboxes.length).toEqual(6); + + const inputLabel = screen.getByPlaceholderText('Label'); + const inputDescription = screen.getByPlaceholderText('Description'); + // const inputDataType = screen.getByPlaceholderTest(''); + const inputDtmFormat = screen.getByPlaceholderText('%Y/%m/%d'); + const inputCertifiedBy = screen.getByPlaceholderText('Certified by'); + const inputCertDetails = screen.getByPlaceholderText( + 'Certification details', + ); + + screen.debug(); + + userEvent.type(await inputLabel, 'test_lable'); + userEvent.type(await inputDescription, 'test'); + // userEvent.type(await inputDataType, 'test'); + userEvent.type(await inputDtmFormat, 'test'); + userEvent.type(await inputCertifiedBy, 'test'); + userEvent.type(await inputCertDetails, 'test'); + + isFeatureEnabledMock.mockRestore(); }); it('can delete columns', async () => { diff --git a/superset/config.py b/superset/config.py index c6f2269b94e..9511d6916b8 100644 --- a/superset/config.py +++ b/superset/config.py @@ -357,6 +357,8 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: # Experimental feature introducing a client (browser) cache "CLIENT_CACHE": False, "DISABLE_DATASET_SOURCE_EDIT": False, + # Allow users to modify column data types in new dataset editor + "ENABLE_EDIT_COLUMN_TYPE": True, # When using a recent version of Druid that supports JOINs turn this on "DRUID_JOINS": False, "DYNAMIC_PLUGINS": False, diff --git a/superset/views/base.py b/superset/views/base.py index 1249bc43cc4..363841ea6ee 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -88,6 +88,7 @@ "SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT", "SUPERSET_DASHBOARD_PERIODICAL_REFRESH_WARNING_MESSAGE", "DISABLE_DATASET_SOURCE_EDIT", + "ENABLE_EDIT_COLUMN_TYPE", "DRUID_IS_ACTIVE", "ENABLE_JAVASCRIPT_CONTROLS", "DEFAULT_SQLLAB_LIMIT", From 8b47568ce1d28ecca1771d142a2374ee1c6fcbfb Mon Sep 17 00:00:00 2001 From: cccs-Dustin <96579982+cccs-Dustin@users.noreply.github.com> Date: Thu, 10 Mar 2022 10:16:54 -0500 Subject: [PATCH 2/6] Modifications made by the pre-commit hook --- .../src/components/Datasource/DatasourceEditor.jsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/components/Datasource/DatasourceEditor.jsx b/superset-frontend/src/components/Datasource/DatasourceEditor.jsx index 017a8c8599f..c0731f37179 100644 --- a/superset-frontend/src/components/Datasource/DatasourceEditor.jsx +++ b/superset-frontend/src/components/Datasource/DatasourceEditor.jsx @@ -1272,7 +1272,9 @@ class DatasourceEditor extends React.PureComponent { this.setColumns({ databaseColumns }) } onDatasourceChange={this.onDatasourceChange} - allowEditDataType={isFeatureEnabled(FeatureFlag.ENABLE_EDIT_COLUMN_TYPE)} + allowEditDataType={isFeatureEnabled( + FeatureFlag.ENABLE_EDIT_COLUMN_TYPE, + )} /> {this.state.metadataLoading && } From eb1ebad48b35e25335f13fef5e98343001ffdec2 Mon Sep 17 00:00:00 2001 From: cccs-Dustin <96579982+cccs-Dustin@users.noreply.github.com> Date: Thu, 10 Mar 2022 11:59:40 -0500 Subject: [PATCH 3/6] Added in the outline of the unit tests that need to be created --- .../Datasource/DatasourceEditor.test.jsx | 153 ++++++++++-------- 1 file changed, 83 insertions(+), 70 deletions(-) diff --git a/superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx b/superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx index 7e443f79f8d..f7c9dcc7150 100644 --- a/superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx +++ b/superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx @@ -68,76 +68,6 @@ describe('DatasourceEditor', () => { }, 0); })); - // to add, remove and modify columns accordingly - it('can modify columns except data type when feature flag is disabled', async () => { - const isFeatureEnabledMock = jest - .spyOn(featureFlags, 'isFeatureEnabled') - .mockImplementation(() => false); - - const columnsTab = screen.getByTestId('collection-tab-Columns'); - userEvent.click(columnsTab); - - const getToggles = screen.getAllByRole('button', { - name: /toggle expand/i, - }); - userEvent.click(getToggles[0]); - const getTextboxes = screen.getAllByRole('textbox'); - expect(getTextboxes.length).toEqual(5); - - const inputLabel = screen.getByPlaceholderText('Label'); - const inputDescription = screen.getByPlaceholderText('Description'); - const inputDtmFormat = screen.getByPlaceholderText('%Y/%m/%d'); - const inputCertifiedBy = screen.getByPlaceholderText('Certified by'); - const inputCertDetails = screen.getByPlaceholderText( - 'Certification details', - ); - - userEvent.type(await inputLabel, 'test_lable'); - userEvent.type(await inputDescription, 'test'); - userEvent.type(await inputDtmFormat, 'test'); - userEvent.type(await inputCertifiedBy, 'test'); - userEvent.type(await inputCertDetails, 'test'); - - isFeatureEnabledMock.mockRestore(); - }); - - // to add, remove and modify columns accordingly - it('can modify column data type when feature flag enabled', async () => { - const isFeatureEnabledMock = jest - .spyOn(featureFlags, 'isFeatureEnabled') - .mockImplementation(() => true); - - const columnsTab = screen.getByTestId('collection-tab-Columns'); - userEvent.click(columnsTab); - - const getToggles = screen.getAllByRole('button', { - name: /toggle expand/i, - }); - userEvent.click(getToggles[0]); - const getTextboxes = screen.getAllByRole('textbox'); - expect(getTextboxes.length).toEqual(6); - - const inputLabel = screen.getByPlaceholderText('Label'); - const inputDescription = screen.getByPlaceholderText('Description'); - // const inputDataType = screen.getByPlaceholderTest(''); - const inputDtmFormat = screen.getByPlaceholderText('%Y/%m/%d'); - const inputCertifiedBy = screen.getByPlaceholderText('Certified by'); - const inputCertDetails = screen.getByPlaceholderText( - 'Certification details', - ); - - screen.debug(); - - userEvent.type(await inputLabel, 'test_lable'); - userEvent.type(await inputDescription, 'test'); - // userEvent.type(await inputDataType, 'test'); - userEvent.type(await inputDtmFormat, 'test'); - userEvent.type(await inputCertifiedBy, 'test'); - userEvent.type(await inputCertDetails, 'test'); - - isFeatureEnabledMock.mockRestore(); - }); - it('can delete columns', async () => { const columnsTab = screen.getByTestId('collection-tab-Columns'); userEvent.click(columnsTab); @@ -183,6 +113,89 @@ describe('DatasourceEditor', () => { expect(screen.getByText(/template parameters/i)).toBeInTheDocument(); }); + describe('disable edit column data type', () => { + + beforeAll(() => { + isFeatureEnabledMock = jest + .spyOn(featureFlags, 'isFeatureEnabled') + .mockImplementation(() => false); + }); + + afterAll(() => { + isFeatureEnabledMock.mockRestore(); + }); + + // to add, remove and modify columns accordingly + it('can modify columns except data type when feature flag is disabled', async () => { + const columnsTab = screen.getByTestId('collection-tab-Columns'); + userEvent.click(columnsTab); + + const getToggles = screen.getAllByRole('button', { + name: /toggle expand/i, + }); + userEvent.click(getToggles[0]); + const getTextboxes = screen.getAllByRole('textbox'); + expect(getTextboxes.length).toEqual(5); + + const inputLabel = screen.getByPlaceholderText('Label'); + const inputDescription = screen.getByPlaceholderText('Description'); + const inputDtmFormat = screen.getByPlaceholderText('%Y/%m/%d'); + const inputCertifiedBy = screen.getByPlaceholderText('Certified by'); + const inputCertDetails = screen.getByPlaceholderText( + 'Certification details', + ); + + userEvent.type(await inputLabel, 'test_lable'); + userEvent.type(await inputDescription, 'test'); + userEvent.type(await inputDtmFormat, 'test'); + userEvent.type(await inputCertifiedBy, 'test'); + userEvent.type(await inputCertDetails, 'test'); + }); + }); + + describe('enable edit column data type', () => { + + beforeAll(() => { + isFeatureEnabledMock = jest + .spyOn(featureFlags, 'isFeatureEnabled') + .mockImplementation(() => true); + }); + + afterAll(() => { + isFeatureEnabledMock.mockRestore(); + }); + + // to add, remove and modify columns accordingly + it('can modify columns including data type when feature flag enabled', async () => { + const columnsTab = screen.getByTestId('collection-tab-Columns'); + userEvent.click(columnsTab); + + const getToggles = screen.getAllByRole('button', { + name: /toggle expand/i, + }); + + userEvent.click(getToggles[0]); + const getTextboxes = screen.getAllByRole('textbox'); + expect(getTextboxes.length).toEqual(6); + + const inputLabel = screen.getByPlaceholderText('Label'); + const inputDescription = screen.getByPlaceholderText('Description'); + // const inputDataType = screen.getByPlaceholderTest(''); + const inputDtmFormat = screen.getByPlaceholderText('%Y/%m/%d'); + const inputCertifiedBy = screen.getByPlaceholderText('Certified by'); + const inputCertDetails = screen.getByPlaceholderText( + 'Certification details', + ); + + userEvent.type(await inputLabel, 'test_lable'); + userEvent.type(await inputDescription, 'test'); + // userEvent.type(await inputDataType, 'test'); + userEvent.type(await inputDtmFormat, 'test'); + userEvent.type(await inputCertifiedBy, 'test'); + userEvent.type(await inputCertDetails, 'test'); + }); + }); + describe('enable edit Source tab', () => { beforeAll(() => { isFeatureEnabledMock = jest From cea45aae8bb2dbdb55a842fe3e116f03dde5a385 Mon Sep 17 00:00:00 2001 From: cccs-Dustin <96579982+cccs-Dustin@users.noreply.github.com> Date: Thu, 10 Mar 2022 13:05:37 -0500 Subject: [PATCH 4/6] Added logging for debugging purposes --- .../src/components/Datasource/DatasourceEditor.test.jsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx b/superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx index f7c9dcc7150..9849a46c6d5 100644 --- a/superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx +++ b/superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx @@ -174,6 +174,8 @@ describe('DatasourceEditor', () => { name: /toggle expand/i, }); + // log screen here + userEvent.click(getToggles[0]); const getTextboxes = screen.getAllByRole('textbox'); expect(getTextboxes.length).toEqual(6); From 5774a872773a09708458ba7912c3a98ae3740bda Mon Sep 17 00:00:00 2001 From: cccs-Dustin <96579982+cccs-Dustin@users.noreply.github.com> Date: Thu, 10 Mar 2022 14:16:14 -0500 Subject: [PATCH 5/6] Finalized the unit tests for this new feature flag --- .../src/components/Datasource/DatasourceEditor.jsx | 1 + .../components/Datasource/DatasourceEditor.test.jsx | 13 ++++++------- superset/config.py | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/superset-frontend/src/components/Datasource/DatasourceEditor.jsx b/superset-frontend/src/components/Datasource/DatasourceEditor.jsx index c0731f37179..87a4ba96283 100644 --- a/superset-frontend/src/components/Datasource/DatasourceEditor.jsx +++ b/superset-frontend/src/components/Datasource/DatasourceEditor.jsx @@ -234,6 +234,7 @@ function ColumnCollectionTable({ label={t('Data type')} control={ { userEvent.click(getToggles[0]); const getTextboxes = screen.getAllByRole('textbox'); expect(getTextboxes.length).toEqual(5); - expect(screen.queryByTestId('column-data-type-select')).toBeNull(); + expect( + screen.queryByTestId('column-data-type-select'), + ).not.toBeInTheDocument(); const inputLabel = screen.getByPlaceholderText('Label'); const inputDescription = screen.getByPlaceholderText('Description'); @@ -177,7 +179,9 @@ describe('DatasourceEditor', () => { userEvent.click(getToggles[0]); const getTextboxes = screen.getAllByRole('textbox'); expect(getTextboxes.length).toEqual(5); - expect(screen.queryByTestId('column-data-type-select')).toBeTruthy(); + expect( + screen.queryByTestId('column-data-type-select'), + ).toBeInTheDocument(); const inputLabel = screen.getByPlaceholderText('Label'); const inputDescription = screen.getByPlaceholderText('Description');