From 5bf605980d9258f9474b23f2b7b4d9c18f8060a6 Mon Sep 17 00:00:00 2001 From: geido Date: Tue, 18 Oct 2022 14:30:59 +0300 Subject: [PATCH 1/4] WIP --- .../controls/DatasourceControl/index.jsx | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx index 9fa4a8a2ba8ef..b30c172204d6b 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx @@ -169,21 +169,31 @@ class DatasourceControl extends React.PureComponent { }; } - onDatasourceSave = datasource => { + onDatasourceSave = (datasource) => { this.props.actions.changeDatasource(datasource); - const timeCol = this.props.form_data?.granularity_sqla; const { columns } = this.props.datasource; - const firstDttmCol = columns.find(column => column.is_dttm); - if ( - datasource.type === 'table' && - !columns.find(({ column_name }) => column_name === timeCol)?.is_dttm - ) { - // set `granularity_sqla` to first datatime column name or null - this.props.actions.setControlValue( - 'granularity_sqla', - firstDttmCol ? firstDttmCol.column_name : null, - ); + const timeCol = this.props.form_data?.granularity_sqla; + const timeColConf = columns.find(({ column_name }) => column_name === timeCol); + const firstDttmCol = columns.find(column => column.is_dttm)?.column_name || null; + const currentMainDttmCol = columns.find(c => c.column_name === datasource.main_dttm_col); + const setDttmCol = currentMainDttmCol?.is_dttm ? currentMainDttmCol.column_name : firstDttmCol; + console.log("firstDttmCol", firstDttmCol); + console.log("currentMainDttmCol", currentMainDttmCol); + + + if (datasource.type === 'table') { + // resets new default time col or picks the first available one + if ( + datasource.main_dttm_col !== timeCol || !timeColConf?.is_dttm + ) { + console.log("setDttmCol", setDttmCol); + this.props.actions.setControlValue( + 'granularity_sqla', + setDttmCol, + ); + } } + if (this.props.onDatasourceSave) { this.props.onDatasourceSave(datasource); } From 0dcac0aa82be1aad9fc275ef5cabc3f751252a8b Mon Sep 17 00:00:00 2001 From: geido Date: Tue, 18 Oct 2022 17:00:23 +0300 Subject: [PATCH 2/4] Fix and add tests --- .../DatasourceControl.test.tsx | 124 +++++++++++++++++- .../controls/DatasourceControl/index.jsx | 31 ++--- 2 files changed, 136 insertions(+), 19 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx index 4d01ec7a79fba..46a0fdaf61d6a 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx @@ -18,9 +18,10 @@ */ import React from 'react'; -import { render, screen, act } from 'spec/helpers/testing-library'; +import { render, screen, act, waitFor } from 'spec/helpers/testing-library'; import userEvent from '@testing-library/user-event'; import { SupersetClient, DatasourceType } from '@superset-ui/core'; +import fetchMock from 'fetch-mock'; import DatasourceControl from '.'; const SupersetClientGet = jest.spyOn(SupersetClient, 'get'); @@ -45,7 +46,10 @@ const createProps = () => ({ }, validationErrors: [], name: 'datasource', - actions: {}, + actions: { + changeDatasource: jest.fn(), + setControlValue: jest.fn(), + }, isEditable: true, user: { createdOn: '2021-04-27T18:12:38.952304', @@ -62,6 +66,17 @@ const createProps = () => ({ onDatasourceSave: jest.fn(), }); +fetchMock.post('glob:*/datasource/save/', { + ...createProps().datasource, +}); + +async function openAndSaveChanges() { + userEvent.click(screen.getByTestId('datasource-menu-trigger')); + userEvent.click(await screen.findByTestId('edit-dataset')); + userEvent.click(await screen.findByTestId('datasource-modal-save')); + userEvent.click(await screen.findByText('OK')); +} + test('Should render', async () => { const props = createProps(); render(); @@ -229,3 +244,108 @@ test('Click on Save as dataset', async () => { expect(screen.getByRole('button', { name: /close/i })).toBeVisible(); expect(dropdownField).toBeVisible(); }); + +test('should set the default temporal column', async () => { + const props = createProps(); + const overrideProps = { + ...props, + form_data: { + granularity_sqla: 'test-col', + }, + datasource: { + ...props.datasource, + main_dttm_col: 'test-default', + columns: [ + { + column_name: 'test-col', + is_dttm: false, + }, + { + column_name: 'test-default', + is_dttm: true, + }, + ], + }, + }; + render(, { + useRedux: true, + }); + + await openAndSaveChanges(); + await waitFor(() => { + expect(props.actions.setControlValue).toHaveBeenCalledWith( + 'granularity_sqla', + 'test-default', + ); + }); +}); + +test('should set the first available temporal column', async () => { + const props = createProps(); + const overrideProps = { + ...props, + form_data: { + granularity_sqla: 'test-col', + }, + datasource: { + ...props.datasource, + main_dttm_col: null, + columns: [ + { + column_name: 'test-col', + is_dttm: false, + }, + { + column_name: 'test-first', + is_dttm: true, + }, + ], + }, + }; + render(, { + useRedux: true, + }); + + await openAndSaveChanges(); + await waitFor(() => { + expect(props.actions.setControlValue).toHaveBeenCalledWith( + 'granularity_sqla', + 'test-first', + ); + }); +}); + +test('should set the temporal column at null', async () => { + const props = createProps(); + const overrideProps = { + ...props, + form_data: { + granularity_sqla: null, + }, + datasource: { + ...props.datasource, + main_dttm_col: null, + columns: [ + { + column_name: 'test-col', + is_dttm: false, + }, + { + column_name: 'test-col-2', + is_dttm: false, + }, + ], + }, + }; + render(, { + useRedux: true, + }); + + await openAndSaveChanges(); + await waitFor(() => { + expect(props.actions.setControlValue).toHaveBeenCalledWith( + 'granularity_sqla', + null, + ); + }); +}); diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx index b30c172204d6b..bca5639c9fdf0 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx @@ -169,28 +169,25 @@ class DatasourceControl extends React.PureComponent { }; } - onDatasourceSave = (datasource) => { + onDatasourceSave = datasource => { this.props.actions.changeDatasource(datasource); const { columns } = this.props.datasource; const timeCol = this.props.form_data?.granularity_sqla; - const timeColConf = columns.find(({ column_name }) => column_name === timeCol); - const firstDttmCol = columns.find(column => column.is_dttm)?.column_name || null; - const currentMainDttmCol = columns.find(c => c.column_name === datasource.main_dttm_col); - const setDttmCol = currentMainDttmCol?.is_dttm ? currentMainDttmCol.column_name : firstDttmCol; - console.log("firstDttmCol", firstDttmCol); - console.log("currentMainDttmCol", currentMainDttmCol); - - + const timeColConf = columns.find( + ({ column_name }) => column_name === timeCol, + ); + const firstDttmCol = + columns.find(column => column.is_dttm)?.column_name || null; + const currentMainDttmCol = columns.find( + c => c.column_name === datasource.main_dttm_col, + ); + const setDttmCol = currentMainDttmCol?.is_dttm + ? currentMainDttmCol.column_name + : firstDttmCol; if (datasource.type === 'table') { // resets new default time col or picks the first available one - if ( - datasource.main_dttm_col !== timeCol || !timeColConf?.is_dttm - ) { - console.log("setDttmCol", setDttmCol); - this.props.actions.setControlValue( - 'granularity_sqla', - setDttmCol, - ); + if (!timeColConf?.is_dttm) { + this.props.actions.setControlValue('granularity_sqla', setDttmCol); } } From 3fdca2293e043fcf57c21f951d1260dc8f39d4f0 Mon Sep 17 00:00:00 2001 From: geido Date: Wed, 19 Oct 2022 17:03:27 +0300 Subject: [PATCH 3/4] Use util --- .../DatasourceControl/DatasourceControl.test.tsx | 4 ++-- .../controls/DatasourceControl/index.jsx | 16 ++++++---------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx index 46a0fdaf61d6a..27efb066dc894 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx @@ -315,7 +315,7 @@ test('should set the first available temporal column', async () => { }); }); -test('should set the temporal column at null', async () => { +test('should not set the temporal column', async () => { const props = createProps(); const overrideProps = { ...props, @@ -345,7 +345,7 @@ test('should set the temporal column at null', async () => { await waitFor(() => { expect(props.actions.setControlValue).toHaveBeenCalledWith( 'granularity_sqla', - null, + undefined, ); }); }); diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx index bca5639c9fdf0..1beaffefa864b 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx @@ -27,7 +27,7 @@ import { t, withTheme, } from '@superset-ui/core'; - +import { getTemporalColumns } from '@superset-ui/chart-controls'; import { getUrlParam } from 'src/utils/urlUtils'; import { AntdDropdown } from 'src/components'; import { Menu } from 'src/components/Menu'; @@ -176,18 +176,14 @@ class DatasourceControl extends React.PureComponent { const timeColConf = columns.find( ({ column_name }) => column_name === timeCol, ); - const firstDttmCol = - columns.find(column => column.is_dttm)?.column_name || null; - const currentMainDttmCol = columns.find( - c => c.column_name === datasource.main_dttm_col, - ); - const setDttmCol = currentMainDttmCol?.is_dttm - ? currentMainDttmCol.column_name - : firstDttmCol; if (datasource.type === 'table') { // resets new default time col or picks the first available one if (!timeColConf?.is_dttm) { - this.props.actions.setControlValue('granularity_sqla', setDttmCol); + const setDttmCol = getTemporalColumns(this.props.datasource); + this.props.actions.setControlValue( + 'granularity_sqla', + setDttmCol.defaultTemporalColumn, + ); } } From 18dda9e0a05005202555b66b6cd96737e784cc18 Mon Sep 17 00:00:00 2001 From: geido Date: Thu, 20 Oct 2022 15:45:48 +0300 Subject: [PATCH 4/4] Fix default temporal --- .../DatasourceControl.test.tsx | 17 +++++----- .../controls/DatasourceControl/index.jsx | 33 ++++++++++++------- 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx index 27efb066dc894..8e41812fb8446 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/DatasourceControl.test.tsx @@ -66,11 +66,10 @@ const createProps = () => ({ onDatasourceSave: jest.fn(), }); -fetchMock.post('glob:*/datasource/save/', { - ...createProps().datasource, -}); - -async function openAndSaveChanges() { +async function openAndSaveChanges(datasource: any) { + fetchMock.post('glob:*/datasource/save/', datasource, { + overwriteRoutes: true, + }); userEvent.click(screen.getByTestId('datasource-menu-trigger')); userEvent.click(await screen.findByTestId('edit-dataset')); userEvent.click(await screen.findByTestId('datasource-modal-save')); @@ -271,7 +270,7 @@ test('should set the default temporal column', async () => { useRedux: true, }); - await openAndSaveChanges(); + await openAndSaveChanges(overrideProps.datasource); await waitFor(() => { expect(props.actions.setControlValue).toHaveBeenCalledWith( 'granularity_sqla', @@ -306,7 +305,7 @@ test('should set the first available temporal column', async () => { useRedux: true, }); - await openAndSaveChanges(); + await openAndSaveChanges(overrideProps.datasource); await waitFor(() => { expect(props.actions.setControlValue).toHaveBeenCalledWith( 'granularity_sqla', @@ -341,11 +340,11 @@ test('should not set the temporal column', async () => { useRedux: true, }); - await openAndSaveChanges(); + await openAndSaveChanges(overrideProps.datasource); await waitFor(() => { expect(props.actions.setControlValue).toHaveBeenCalledWith( 'granularity_sqla', - undefined, + null, ); }); }); diff --git a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx index 1beaffefa864b..266593adaeed1 100644 --- a/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx +++ b/superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx @@ -171,20 +171,29 @@ class DatasourceControl extends React.PureComponent { onDatasourceSave = datasource => { this.props.actions.changeDatasource(datasource); - const { columns } = this.props.datasource; + const { temporalColumns, defaultTemporalColumn } = + getTemporalColumns(datasource); + const { columns } = datasource; + // the current granularity_sqla might not be a temporal column anymore const timeCol = this.props.form_data?.granularity_sqla; - const timeColConf = columns.find( + const isGranularitySqalTemporal = columns.find( ({ column_name }) => column_name === timeCol, - ); - if (datasource.type === 'table') { - // resets new default time col or picks the first available one - if (!timeColConf?.is_dttm) { - const setDttmCol = getTemporalColumns(this.props.datasource); - this.props.actions.setControlValue( - 'granularity_sqla', - setDttmCol.defaultTemporalColumn, - ); - } + )?.is_dttm; + // the current main_dttm_col might not be a temporal column anymore + const isDefaultTemporal = columns.find( + ({ column_name }) => column_name === defaultTemporalColumn, + )?.is_dttm; + + // if the current granularity_sqla is empty or it is not a temporal column anymore + // let's update the control value + if (datasource.type === 'table' && !isGranularitySqalTemporal) { + const temporalColumn = isDefaultTemporal + ? defaultTemporalColumn + : temporalColumns?.[0]; + this.props.actions.setControlValue( + 'granularity_sqla', + temporalColumn || null, + ); } if (this.props.onDatasourceSave) {