From fbf99527fe39d683bd5733f80bce49a84975dab0 Mon Sep 17 00:00:00 2001 From: Justin Park Date: Wed, 5 Jul 2023 15:44:09 -0700 Subject: [PATCH 1/9] fix(datasets): Replace left panel layout by TableSelector --- .../AddDataset/LeftPanel/LeftPanel.test.tsx | 311 +++++++++--------- .../datasets/AddDataset/LeftPanel/index.tsx | 248 +------------- 2 files changed, 165 insertions(+), 394 deletions(-) diff --git a/superset-frontend/src/features/datasets/AddDataset/LeftPanel/LeftPanel.test.tsx b/superset-frontend/src/features/datasets/AddDataset/LeftPanel/LeftPanel.test.tsx index 604e3e9d9e7eb..31208e5e6c5c2 100644 --- a/superset-frontend/src/features/datasets/AddDataset/LeftPanel/LeftPanel.test.tsx +++ b/superset-frontend/src/features/datasets/AddDataset/LeftPanel/LeftPanel.test.tsx @@ -27,122 +27,128 @@ const databasesEndpoint = 'glob:*/api/v1/database/?q*'; const schemasEndpoint = 'glob:*/api/v1/database/*/schemas*'; const tablesEndpoint = 'glob:*/api/v1/database/*/tables/?q*'; -fetchMock.get(databasesEndpoint, { - count: 2, - description_columns: {}, - ids: [1, 2], - label_columns: { - allow_file_upload: 'Allow Csv Upload', - allow_ctas: 'Allow Ctas', - allow_cvas: 'Allow Cvas', - allow_dml: 'Allow Dml', - allow_multi_schema_metadata_fetch: 'Allow Multi Schema Metadata Fetch', - allow_run_async: 'Allow Run Async', - allows_cost_estimate: 'Allows Cost Estimate', - allows_subquery: 'Allows Subquery', - allows_virtual_table_explore: 'Allows Virtual Table Explore', - disable_data_preview: 'Disables SQL Lab Data Preview', - backend: 'Backend', - changed_on: 'Changed On', - changed_on_delta_humanized: 'Changed On Delta Humanized', - 'created_by.first_name': 'Created By First Name', - 'created_by.last_name': 'Created By Last Name', - database_name: 'Database Name', - explore_database_id: 'Explore Database Id', - expose_in_sqllab: 'Expose In Sqllab', - force_ctas_schema: 'Force Ctas Schema', - id: 'Id', - }, - list_columns: [ - 'allow_file_upload', - 'allow_ctas', - 'allow_cvas', - 'allow_dml', - 'allow_multi_schema_metadata_fetch', - 'allow_run_async', - 'allows_cost_estimate', - 'allows_subquery', - 'allows_virtual_table_explore', - 'disable_data_preview', - 'backend', - 'changed_on', - 'changed_on_delta_humanized', - 'created_by.first_name', - 'created_by.last_name', - 'database_name', - 'explore_database_id', - 'expose_in_sqllab', - 'force_ctas_schema', - 'id', - ], - list_title: 'List Database', - order_columns: [ - 'allow_file_upload', - 'allow_dml', - 'allow_run_async', - 'changed_on', - 'changed_on_delta_humanized', - 'created_by.first_name', - 'database_name', - 'expose_in_sqllab', - ], - result: [ - { - allow_file_upload: false, - allow_ctas: false, - allow_cvas: false, - allow_dml: false, - allow_multi_schema_metadata_fetch: false, - allow_run_async: false, - allows_cost_estimate: null, - allows_subquery: true, - allows_virtual_table_explore: true, - disable_data_preview: false, - backend: 'postgresql', - changed_on: '2021-03-09T19:02:07.141095', - changed_on_delta_humanized: 'a day ago', - created_by: null, - database_name: 'test-postgres', - explore_database_id: 1, - expose_in_sqllab: true, - force_ctas_schema: null, - id: 1, +beforeEach(() => { + fetchMock.get(databasesEndpoint, { + count: 2, + description_columns: {}, + ids: [1, 2], + label_columns: { + allow_file_upload: 'Allow Csv Upload', + allow_ctas: 'Allow Ctas', + allow_cvas: 'Allow Cvas', + allow_dml: 'Allow Dml', + allow_multi_schema_metadata_fetch: 'Allow Multi Schema Metadata Fetch', + allow_run_async: 'Allow Run Async', + allows_cost_estimate: 'Allows Cost Estimate', + allows_subquery: 'Allows Subquery', + allows_virtual_table_explore: 'Allows Virtual Table Explore', + disable_data_preview: 'Disables SQL Lab Data Preview', + backend: 'Backend', + changed_on: 'Changed On', + changed_on_delta_humanized: 'Changed On Delta Humanized', + 'created_by.first_name': 'Created By First Name', + 'created_by.last_name': 'Created By Last Name', + database_name: 'Database Name', + explore_database_id: 'Explore Database Id', + expose_in_sqllab: 'Expose In Sqllab', + force_ctas_schema: 'Force Ctas Schema', + id: 'Id', }, - { - allow_csv_upload: false, - allow_ctas: false, - allow_cvas: false, - allow_dml: false, - allow_multi_schema_metadata_fetch: false, - allow_run_async: false, - allows_cost_estimate: null, - allows_subquery: true, - allows_virtual_table_explore: true, - disable_data_preview: false, - backend: 'mysql', - changed_on: '2021-03-09T19:02:07.141095', - changed_on_delta_humanized: 'a day ago', - created_by: null, - database_name: 'test-mysql', - explore_database_id: 1, - expose_in_sqllab: true, - force_ctas_schema: null, - id: 2, - }, - ], -}); + list_columns: [ + 'allow_file_upload', + 'allow_ctas', + 'allow_cvas', + 'allow_dml', + 'allow_multi_schema_metadata_fetch', + 'allow_run_async', + 'allows_cost_estimate', + 'allows_subquery', + 'allows_virtual_table_explore', + 'disable_data_preview', + 'backend', + 'changed_on', + 'changed_on_delta_humanized', + 'created_by.first_name', + 'created_by.last_name', + 'database_name', + 'explore_database_id', + 'expose_in_sqllab', + 'force_ctas_schema', + 'id', + ], + list_title: 'List Database', + order_columns: [ + 'allow_file_upload', + 'allow_dml', + 'allow_run_async', + 'changed_on', + 'changed_on_delta_humanized', + 'created_by.first_name', + 'database_name', + 'expose_in_sqllab', + ], + result: [ + { + allow_file_upload: false, + allow_ctas: false, + allow_cvas: false, + allow_dml: false, + allow_multi_schema_metadata_fetch: false, + allow_run_async: false, + allows_cost_estimate: null, + allows_subquery: true, + allows_virtual_table_explore: true, + disable_data_preview: false, + backend: 'postgresql', + changed_on: '2021-03-09T19:02:07.141095', + changed_on_delta_humanized: 'a day ago', + created_by: null, + database_name: 'test-postgres', + explore_database_id: 1, + expose_in_sqllab: true, + force_ctas_schema: null, + id: 1, + }, + { + allow_csv_upload: false, + allow_ctas: false, + allow_cvas: false, + allow_dml: false, + allow_multi_schema_metadata_fetch: false, + allow_run_async: false, + allows_cost_estimate: null, + allows_subquery: true, + allows_virtual_table_explore: true, + disable_data_preview: false, + backend: 'mysql', + changed_on: '2021-03-09T19:02:07.141095', + changed_on_delta_humanized: 'a day ago', + created_by: null, + database_name: 'test-mysql', + explore_database_id: 1, + expose_in_sqllab: true, + force_ctas_schema: null, + id: 2, + }, + ], + }); + + fetchMock.get(schemasEndpoint, { + result: ['information_schema', 'public'], + }); -fetchMock.get(schemasEndpoint, { - result: ['information_schema', 'public'], + fetchMock.get(tablesEndpoint, { + count: 3, + result: [ + { value: 'Sheet1', type: 'table', extra: null }, + { value: 'Sheet2', type: 'table', extra: null }, + { value: 'Sheet3', type: 'table', extra: null }, + ], + }); }); -fetchMock.get(tablesEndpoint, { - count: 3, - result: [ - { value: 'Sheet1', type: 'table', extra: null }, - { value: 'Sheet2', type: 'table', extra: null }, - { value: 'Sheet3', type: 'table', extra: null }, - ], +afterEach(() => { + fetchMock.reset(); }); const mockFun = jest.fn(); @@ -152,14 +158,16 @@ test('should render', async () => { useRedux: true, }); expect( - await screen.findByText(/select database & schema/i), + await screen.findByText(/Select database or type to search databases/i), ).toBeInTheDocument(); }); test('should render schema selector, database selector container, and selects', async () => { render(, { useRedux: true }); - expect(await screen.findByText(/select database & schema/i)).toBeVisible(); + expect( + await screen.findByText(/Select database or type to search databases/i), + ).toBeVisible(); const databaseSelect = screen.getByRole('combobox', { name: 'Select database or type to search databases', @@ -175,7 +183,7 @@ test('does not render blank state if there is nothing selected', async () => { render(, { useRedux: true }); expect( - await screen.findByText(/select database & schema/i), + await screen.findByText(/Select database or type to search databases/i), ).toBeInTheDocument(); const emptyState = screen.queryByRole('img', { name: /empty/i }); expect(emptyState).not.toBeInTheDocument(); @@ -218,66 +226,41 @@ test('searches for a table name', async () => { const schemaSelect = screen.getByRole('combobox', { name: /select schema or type to search schemas/i, }); + const tableSelect = screen.getByRole('combobox', { + name: /select table or type to search tables/i, + }); await waitFor(() => expect(schemaSelect).toBeEnabled()); // Click 'public' schema to access tables userEvent.click(schemaSelect); userEvent.click(screen.getAllByText('public')[1]); + await waitFor(() => expect(fetchMock.calls(tablesEndpoint).length).toBe(1)); + userEvent.click(tableSelect); await waitFor(() => { - expect(screen.getByText('Sheet1')).toBeInTheDocument(); - expect(screen.getByText('Sheet2')).toBeInTheDocument(); - expect(screen.getByText('Sheet3')).toBeInTheDocument(); - }); - - userEvent.type(screen.getByRole('textbox'), 'Sheet2'); - - await waitFor(() => { - expect(screen.queryByText('Sheet1')).not.toBeInTheDocument(); - expect(screen.getByText('Sheet2')).toBeInTheDocument(); - expect(screen.queryByText('Sheet3')).not.toBeInTheDocument(); - }); -}); - -test('renders a warning icon when a table name has a pre-existing dataset', async () => { - render( - , - { - useRedux: true, - }, - ); - - // Click 'test-postgres' database to access schemas - const databaseSelect = screen.getByRole('combobox', { - name: /select database or type to search databases/i, + expect( + screen.queryByRole('option', { + name: /Sheet1/i, + }), + ).toBeInTheDocument(); + expect( + screen.queryByRole('option', { + name: /Sheet2/i, + }), + ).toBeInTheDocument(); }); - userEvent.click(databaseSelect); - userEvent.click(await screen.findByText('test-postgres')); - const schemaSelect = screen.getByRole('combobox', { - name: /select schema or type to search schemas/i, - }); - - await waitFor(() => expect(schemaSelect).toBeEnabled()); - - // Warning icon should not show yet - expect( - screen.queryByRole('img', { name: 'warning' }), - ).not.toBeInTheDocument(); - - // Click 'public' schema to access tables - userEvent.click(schemaSelect); - userEvent.click(screen.getAllByText('public')[1]); + userEvent.type(tableSelect, 'Sheet2'); await waitFor(() => { - expect(screen.getByText('Sheet2')).toBeInTheDocument(); + expect( + screen.queryByRole('option', { name: /Sheet1/i }), + ).not.toBeInTheDocument(); + expect( + screen.queryByRole('option', { + name: /Sheet2/i, + }), + ).toBeInTheDocument(); }); - - // Sheet2 should now show the warning icon - expect(screen.getByRole('img', { name: 'warning' })).toBeVisible(); }); diff --git a/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx b/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx index 90ec555833ff5..7d64ad7450f31 100644 --- a/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx +++ b/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx @@ -16,36 +16,11 @@ * specific language governing permissions and limitations * under the License. */ -import React, { - useEffect, - useState, - SetStateAction, - Dispatch, - useCallback, -} from 'react'; -import rison from 'rison'; -import { - SupersetClient, - t, - styled, - css, - useTheme, - logging, -} from '@superset-ui/core'; -import { Input } from 'src/components/Input'; -import { Form } from 'src/components/Form'; -import Icons from 'src/components/Icons'; -import { TableOption } from 'src/components/TableSelector'; -import RefreshLabel from 'src/components/RefreshLabel'; -import { Table } from 'src/hooks/apiResources'; -import Loading from 'src/components/Loading'; -import DatabaseSelector, { - DatabaseObject, -} from 'src/components/DatabaseSelector'; -import { - EmptyStateMedium, - emptyStateComponent, -} from 'src/components/EmptyState'; +import React, { useEffect, SetStateAction, Dispatch, useCallback } from 'react'; +import { styled } from '@superset-ui/core'; +import TableSelector from 'src/components/TableSelector'; +import { DatabaseObject } from 'src/components/DatabaseSelector'; +import { emptyStateComponent } from 'src/components/EmptyState'; import { useToasts } from 'src/components/MessageToasts/withToasts'; import { LocalStorageKeys, getItem } from 'src/utils/localStorageHelpers'; import { @@ -59,10 +34,6 @@ interface LeftPanelProps { datasetNames?: (string | null | undefined)[] | undefined; } -const SearchIcon = styled(Icons.Search)` - color: ${({ theme }) => theme.colors.grayscale.light1}; -`; - const LeftPanelStyle = styled.div` ${({ theme }) => ` max-width: ${theme.gridUnit * 87.5}px; @@ -74,14 +45,6 @@ const LeftPanelStyle = styled.div` height: auto; margin-top: ${theme.gridUnit * 17.5}px; } - .refresh { - position: absolute; - top: ${theme.gridUnit * 38.75}px; - left: ${theme.gridUnit * 16.75}px; - span[role="button"]{ - font-size: ${theme.gridUnit * 4.25}px; - } - } .section-title { margin-top: ${theme.gridUnit * 5.5}px; margin-bottom: ${theme.gridUnit * 11}px; @@ -153,82 +116,29 @@ const LeftPanelStyle = styled.div` `} `; -export default function LeftPanel({ - setDataset, - dataset, - datasetNames, -}: LeftPanelProps) { - const theme = useTheme(); - - const [tableOptions, setTableOptions] = useState>([]); - const [resetTables, setResetTables] = useState(false); - const [loadTables, setLoadTables] = useState(false); - const [searchVal, setSearchVal] = useState(''); - const [refresh, setRefresh] = useState(false); - const [selectedTable, setSelectedTable] = useState(null); - +export default function LeftPanel({ setDataset, dataset }: LeftPanelProps) { const { addDangerToast } = useToasts(); const setDatabase = useCallback( (db: Partial) => { setDataset({ type: DatasetActionType.selectDatabase, payload: { db } }); - setSelectedTable(null); - setResetTables(true); }, [setDataset], ); - - const setTable = (tableName: string, index: number) => { - setSelectedTable(index); - setDataset({ - type: DatasetActionType.selectTable, - payload: { name: 'table_name', value: tableName }, - }); - }; - - const getTablesList = useCallback( - (url: string) => { - SupersetClient.get({ url }) - .then(({ json }) => { - const options: TableOption[] = json.result.map((table: Table) => { - const option: TableOption = { - value: table.value, - label: , - text: table.label, - }; - - return option; - }); - - setTableOptions(options); - setLoadTables(false); - setResetTables(false); - setRefresh(false); - }) - .catch(error => { - addDangerToast(t('There was an error fetching tables')); - logging.error(t('There was an error fetching tables'), error); - }); - }, - [addDangerToast], - ); - const setSchema = (schema: string) => { if (schema) { setDataset({ type: DatasetActionType.selectSchema, payload: { name: 'schema', value: schema }, }); - setLoadTables(true); } - setSelectedTable(null); - setResetTables(true); }; - - const encodedSchema = dataset?.schema - ? encodeURIComponent(dataset?.schema) - : undefined; - + const setTable = (tableName: string) => { + setDataset({ + type: DatasetActionType.selectTable, + payload: { name: 'table_name', value: tableName }, + }); + }; useEffect(() => { const currentUserSelectedDb = getItem( LocalStorageKeys.db, @@ -239,140 +149,18 @@ export default function LeftPanel({ } }, [setDatabase]); - useEffect(() => { - if (loadTables) { - const params = rison.encode({ - force: refresh, - schema_name: encodedSchema, - }); - - const endpoint = `/api/v1/database/${dataset?.db?.id}/tables/?q=${params}`; - getTablesList(endpoint); - } - }, [loadTables, dataset?.db?.id, encodedSchema, getTablesList, refresh]); - - useEffect(() => { - if (resetTables) { - setTableOptions([]); - setResetTables(false); - } - }, [resetTables]); - - const filteredOptions = tableOptions.filter(option => - option?.value?.toLowerCase().includes(searchVal.toLowerCase()), - ); - - const Loader = (inline: string) => ( -
- -

{inline}

-
- ); - - const SELECT_DATABASE_AND_SCHEMA_TEXT = t('Select database & schema'); - const TABLE_LOADING_TEXT = t('Table loading'); - const NO_TABLES_FOUND_TITLE = t('No database tables found'); - const NO_TABLES_FOUND_DESCRIPTION = t('Try selecting a different schema'); - const SELECT_DATABASE_TABLE_TEXT = t('Select database table'); - const REFRESH_TABLE_LIST_TOOLTIP = t('Refresh table list'); - const REFRESH_TABLES_TEXT = t('Refresh tables'); - const SEARCH_TABLES_PLACEHOLDER_TEXT = t('Search tables'); - - const optionsList = document.getElementsByClassName('options-list'); - const scrollableOptionsList = - optionsList[0]?.scrollHeight > optionsList[0]?.clientHeight; - const [emptyResultsWithSearch, setEmptyResultsWithSearch] = useState(false); - - const onEmptyResults = (searchText?: string) => { - setEmptyResultsWithSearch(!!searchText); - }; - return ( -

- {SELECT_DATABASE_AND_SCHEMA_TEXT} -

- - {loadTables && !refresh && Loader(TABLE_LOADING_TEXT)} - {dataset?.schema && !loadTables && !tableOptions.length && !searchVal && ( -
- -
- )} - - {dataset?.schema && (tableOptions.length > 0 || searchVal.length > 0) && ( - <> -
-

{SELECT_DATABASE_TABLE_TEXT}

- { - setLoadTables(true); - setRefresh(true); - }} - tooltipContent={REFRESH_TABLE_LIST_TOOLTIP} - /> - {refresh && Loader(REFRESH_TABLES_TEXT)} - {!refresh && ( - } - onChange={evt => { - setSearchVal(evt.target.value); - }} - className="table-form" - placeholder={SEARCH_TABLES_PLACEHOLDER_TEXT} - allowClear - /> - )} - -
- {!refresh && - filteredOptions.map((option, i) => ( -
setTable(option.value, i)} - > - {option.label} - {datasetNames?.includes(option.value) && ( - - )} -
- ))} -
- - )}
); } From b443f8b7b968899afaff1c49e2329d66b764e215 Mon Sep 17 00:00:00 2001 From: Justin Park Date: Wed, 5 Jul 2023 17:14:32 -0700 Subject: [PATCH 2/9] spec update for new table selector --- .../src/features/datasets/DatasetLayout/DatasetLayout.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/features/datasets/DatasetLayout/DatasetLayout.test.tsx b/superset-frontend/src/features/datasets/DatasetLayout/DatasetLayout.test.tsx index a851b2b3bc18b..66cbf6f0c44ff 100644 --- a/superset-frontend/src/features/datasets/DatasetLayout/DatasetLayout.test.tsx +++ b/superset-frontend/src/features/datasets/DatasetLayout/DatasetLayout.test.tsx @@ -59,7 +59,7 @@ describe('DatasetLayout', () => { ); expect( - await screen.findByText(/select database & schema/i), + await screen.findByText(/Select database or type to search databases/i), ).toBeInTheDocument(); expect(LeftPanel).toBeTruthy(); }); From d3af5d6810ee559e55c20607b349ed0f8c22a4ce Mon Sep 17 00:00:00 2001 From: Justin Park Date: Fri, 7 Jul 2023 11:46:23 -0700 Subject: [PATCH 3/9] Add warning indicator using customLabel renderder --- .../src/components/TableSelector/index.tsx | 7 +++- .../datasets/AddDataset/LeftPanel/index.tsx | 34 +++++++++++++++++-- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/components/TableSelector/index.tsx b/superset-frontend/src/components/TableSelector/index.tsx index 7c2a809e5cf36..380145d18d61d 100644 --- a/superset-frontend/src/components/TableSelector/index.tsx +++ b/superset-frontend/src/components/TableSelector/index.tsx @@ -104,6 +104,7 @@ interface TableSelectorProps { tableValue?: string | string[]; onTableSelectChange?: (value?: string | string[], schema?: string) => void; tableSelectMode?: 'single' | 'multiple'; + customTableOptionLabelRenderer?: (table: Table) => JSX.Element; } export interface TableOption { @@ -164,6 +165,7 @@ const TableSelector: FunctionComponent = ({ tableSelectMode = 'single', tableValue = undefined, onTableSelectChange, + customTableOptionLabelRenderer, }) => { const { addSuccessToast } = useToasts(); const [currentSchema, setCurrentSchema] = useState( @@ -203,9 +205,12 @@ const TableSelector: FunctionComponent = ({ value: table.value, label: , text: table.value, + ...(customTableOptionLabelRenderer && { + customLabel: customTableOptionLabelRenderer(table), + }), })) : [], - [data], + [data, customTableOptionLabelRenderer], ); useEffect(() => { diff --git a/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx b/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx index 7d64ad7450f31..07d188056915d 100644 --- a/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx +++ b/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx @@ -17,8 +17,8 @@ * under the License. */ import React, { useEffect, SetStateAction, Dispatch, useCallback } from 'react'; -import { styled } from '@superset-ui/core'; -import TableSelector from 'src/components/TableSelector'; +import { styled, t } from '@superset-ui/core'; +import TableSelector, { TableOption } from 'src/components/TableSelector'; import { DatabaseObject } from 'src/components/DatabaseSelector'; import { emptyStateComponent } from 'src/components/EmptyState'; import { useToasts } from 'src/components/MessageToasts/withToasts'; @@ -27,6 +27,7 @@ import { DatasetActionType, DatasetObject, } from 'src/features/datasets/AddDataset/types'; +import { Table } from 'src/hooks/apiResources'; interface LeftPanelProps { setDataset: Dispatch>; @@ -116,7 +117,13 @@ const LeftPanelStyle = styled.div` `} `; -export default function LeftPanel({ setDataset, dataset }: LeftPanelProps) { +const divider = ':@:'; + +export default function LeftPanel({ + setDataset, + dataset, + datasetNames, +}: LeftPanelProps) { const { addDangerToast } = useToasts(); const setDatabase = useCallback( @@ -149,6 +156,26 @@ export default function LeftPanel({ setDataset, dataset }: LeftPanelProps) { } }, [setDatabase]); + const datasetNamesSet = datasetNames?.join(divider); + + const customTableOptionLabelRenderer = useCallback( + (table: Table) => ( + + ), + [datasetNamesSet], + ); + return ( From f3f14c08df6bacb1de171ba15bc45a609d4aa8c2 Mon Sep 17 00:00:00 2001 From: Justin Park Date: Fri, 7 Jul 2023 11:56:52 -0700 Subject: [PATCH 4/9] restore warning icon test spec --- .../AddDataset/LeftPanel/LeftPanel.test.tsx | 57 ++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/features/datasets/AddDataset/LeftPanel/LeftPanel.test.tsx b/superset-frontend/src/features/datasets/AddDataset/LeftPanel/LeftPanel.test.tsx index 31208e5e6c5c2..5156073281e8a 100644 --- a/superset-frontend/src/features/datasets/AddDataset/LeftPanel/LeftPanel.test.tsx +++ b/superset-frontend/src/features/datasets/AddDataset/LeftPanel/LeftPanel.test.tsx @@ -251,16 +251,71 @@ test('searches for a table name', async () => { ).toBeInTheDocument(); }); - userEvent.type(tableSelect, 'Sheet2'); + userEvent.type(tableSelect, 'Sheet3'); await waitFor(() => { expect( screen.queryByRole('option', { name: /Sheet1/i }), ).not.toBeInTheDocument(); + expect( + screen.queryByRole('option', { name: /Sheet2/i }), + ).not.toBeInTheDocument(); + expect( + screen.queryByRole('option', { + name: /Sheet3/i, + }), + ).toBeInTheDocument(); + }); +}); + +test('renders a warning icon when a table name has a pre-existing dataset', async () => { + render( + , + { + useRedux: true, + }, + ); + + // Click 'test-postgres' database to access schemas + const databaseSelect = screen.getByRole('combobox', { + name: /select database or type to search databases/i, + }); + userEvent.click(databaseSelect); + userEvent.click(await screen.findByText('test-postgres')); + + const schemaSelect = screen.getByRole('combobox', { + name: /select schema or type to search schemas/i, + }); + const tableSelect = screen.getByRole('combobox', { + name: /select table or type to search tables/i, + }); + + await waitFor(() => expect(schemaSelect).toBeEnabled()); + + // Warning icon should not show yet + expect( + screen.queryByRole('img', { name: 'warning' }), + ).not.toBeInTheDocument(); + + // Click 'public' schema to access tables + userEvent.click(schemaSelect); + userEvent.click(screen.getAllByText('public')[1]); + userEvent.click(tableSelect); + + await waitFor(() => { expect( screen.queryByRole('option', { name: /Sheet2/i, }), ).toBeInTheDocument(); }); + + userEvent.type(tableSelect, 'Sheet2'); + + // Sheet2 should now show the warning icon + expect(screen.getByRole('img', { name: 'alert-solid' })).toBeInTheDocument(); }); From 4b0f59859bfffe4a3abe9322157e73191df7ac7d Mon Sep 17 00:00:00 2001 From: Justin Park Date: Fri, 7 Jul 2023 12:31:08 -0700 Subject: [PATCH 5/9] adjust margin right for warning icon --- .../src/components/WarningIconWithTooltip/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/components/WarningIconWithTooltip/index.tsx b/superset-frontend/src/components/WarningIconWithTooltip/index.tsx index f732554e15aaa..a52d932831586 100644 --- a/superset-frontend/src/components/WarningIconWithTooltip/index.tsx +++ b/superset-frontend/src/components/WarningIconWithTooltip/index.tsx @@ -39,7 +39,7 @@ function WarningIconWithTooltip({ ); From 478db72aeee225e58bedc5e441d212ddb7114ef5 Mon Sep 17 00:00:00 2001 From: Justin Park Date: Tue, 11 Jul 2023 09:41:58 -0700 Subject: [PATCH 6/9] remove bottom margin on tooltip --- superset-frontend/src/components/Tooltip/index.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/superset-frontend/src/components/Tooltip/index.tsx b/superset-frontend/src/components/Tooltip/index.tsx index 64af6b06a0ef7..8237356690bf8 100644 --- a/superset-frontend/src/components/Tooltip/index.tsx +++ b/superset-frontend/src/components/Tooltip/index.tsx @@ -41,6 +41,9 @@ export const Tooltip = (props: TooltipProps) => { display: block; } } + .ant-tooltip-inner > p { + margin: 0; + } `} /> Date: Thu, 13 Jul 2023 15:12:20 -0700 Subject: [PATCH 7/9] configurable marginRight on warning icon --- superset-frontend/src/components/TableSelector/index.tsx | 1 + .../src/components/WarningIconWithTooltip/index.tsx | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/components/TableSelector/index.tsx b/superset-frontend/src/components/TableSelector/index.tsx index 380145d18d61d..84556d9beec9f 100644 --- a/superset-frontend/src/components/TableSelector/index.tsx +++ b/superset-frontend/src/components/TableSelector/index.tsx @@ -133,6 +133,7 @@ export const TableOption = ({ table }: { table: Table }) => { )} {value} diff --git a/superset-frontend/src/components/WarningIconWithTooltip/index.tsx b/superset-frontend/src/components/WarningIconWithTooltip/index.tsx index a52d932831586..bb3591b97d57a 100644 --- a/superset-frontend/src/components/WarningIconWithTooltip/index.tsx +++ b/superset-frontend/src/components/WarningIconWithTooltip/index.tsx @@ -24,11 +24,13 @@ import { Tooltip } from 'src/components/Tooltip'; export interface WarningIconWithTooltipProps { warningMarkdown: string; size?: IconType['iconSize']; + indentSize?: number; } function WarningIconWithTooltip({ warningMarkdown, size, + indentSize = 2, }: WarningIconWithTooltipProps) { const theme = useTheme(); return ( @@ -39,7 +41,7 @@ function WarningIconWithTooltip({ ); From 3c0c2102d4ed0f0e07eac91a061d673fc96f82f3 Mon Sep 17 00:00:00 2001 From: justinpark Date: Wed, 19 Jul 2023 18:45:50 -0700 Subject: [PATCH 8/9] remove redundant join/split --- .../src/features/datasets/AddDataset/LeftPanel/index.tsx | 8 ++------ .../src/features/datasets/hooks/useDatasetLists.ts | 7 +++++-- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx b/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx index 07d188056915d..715bf2deeed0a 100644 --- a/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx +++ b/superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx @@ -117,8 +117,6 @@ const LeftPanelStyle = styled.div` `} `; -const divider = ':@:'; - export default function LeftPanel({ setDataset, dataset, @@ -156,13 +154,11 @@ export default function LeftPanel({ } }, [setDatabase]); - const datasetNamesSet = datasetNames?.join(divider); - const customTableOptionLabelRenderer = useCallback( (table: Table) => ( ), - [datasetNamesSet], + [datasetNames], ); return ( diff --git a/superset-frontend/src/features/datasets/hooks/useDatasetLists.ts b/superset-frontend/src/features/datasets/hooks/useDatasetLists.ts index 1c4b00df2d121..373d98946f8a2 100644 --- a/superset-frontend/src/features/datasets/hooks/useDatasetLists.ts +++ b/superset-frontend/src/features/datasets/hooks/useDatasetLists.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { useState, useEffect, useCallback } from 'react'; +import { useState, useEffect, useCallback, useMemo } from 'react'; import { SupersetClient, logging, t } from '@superset-ui/core'; import rison from 'rison'; import { addDangerToast } from 'src/components/MessageToasts/actions'; @@ -83,7 +83,10 @@ const useDatasetsList = ( } }, [db?.id, schema, encodedSchema, getDatasetsList]); - const datasetNames = datasets?.map(dataset => dataset.table_name); + const datasetNames = useMemo( + () => datasets?.map(dataset => dataset.table_name), + [datasets], + ); return { datasets, datasetNames }; }; From e5e0358a301be1c780a51e1a0bb59b47db45c27e Mon Sep 17 00:00:00 2001 From: justinpark Date: Wed, 19 Jul 2023 18:48:18 -0700 Subject: [PATCH 9/9] replace to marginRight --- superset-frontend/src/components/TableSelector/index.tsx | 2 +- .../src/components/WarningIconWithTooltip/index.tsx | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/superset-frontend/src/components/TableSelector/index.tsx b/superset-frontend/src/components/TableSelector/index.tsx index 84556d9beec9f..f36056a4a218b 100644 --- a/superset-frontend/src/components/TableSelector/index.tsx +++ b/superset-frontend/src/components/TableSelector/index.tsx @@ -133,7 +133,7 @@ export const TableOption = ({ table }: { table: Table }) => { )} {value} diff --git a/superset-frontend/src/components/WarningIconWithTooltip/index.tsx b/superset-frontend/src/components/WarningIconWithTooltip/index.tsx index bb3591b97d57a..82b3ac7e00662 100644 --- a/superset-frontend/src/components/WarningIconWithTooltip/index.tsx +++ b/superset-frontend/src/components/WarningIconWithTooltip/index.tsx @@ -24,13 +24,13 @@ import { Tooltip } from 'src/components/Tooltip'; export interface WarningIconWithTooltipProps { warningMarkdown: string; size?: IconType['iconSize']; - indentSize?: number; + marginRight?: number; } function WarningIconWithTooltip({ warningMarkdown, size, - indentSize = 2, + marginRight, }: WarningIconWithTooltipProps) { const theme = useTheme(); return ( @@ -41,7 +41,7 @@ function WarningIconWithTooltip({ );