Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Flow for tables that already have a dataset #22136

Merged
merged 16 commits into from
Dec 5, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import DatasetPanel, {
tableColumnDefinition,
COLUMN_TITLE,
} from './DatasetPanel';
import { exampleColumns } from './fixtures';
import { exampleColumns, exampleDataset } from './fixtures';
import {
SELECT_MESSAGE,
CREATE_MESSAGE,
Expand All @@ -44,7 +44,7 @@ jest.mock(
);

describe('DatasetPanel', () => {
it('renders a blank state DatasetPanel', () => {
test('renders a blank state DatasetPanel', () => {
render(<DatasetPanel hasError={false} columnList={[]} loading={false} />);

const blankDatasetImg = screen.getByRole('img', { name: /empty/i });
Expand All @@ -65,7 +65,7 @@ describe('DatasetPanel', () => {
expect(sqlLabLink).toBeVisible();
});

it('renders a no columns screen', () => {
test('renders a no columns screen', () => {
render(
<DatasetPanel
tableName="Name"
Expand All @@ -83,7 +83,7 @@ describe('DatasetPanel', () => {
expect(noColumnsDescription).toBeVisible();
});

it('renders a loading screen', () => {
test('renders a loading screen', () => {
render(
<DatasetPanel
tableName="Name"
Expand All @@ -99,7 +99,7 @@ describe('DatasetPanel', () => {
expect(blankDatasetTitle).toBeVisible();
});

it('renders an error screen', () => {
test('renders an error screen', () => {
render(
<DatasetPanel
tableName="Name"
Expand All @@ -115,7 +115,7 @@ describe('DatasetPanel', () => {
expect(errorDescription).toBeVisible();
});

it('renders a table with columns displayed', async () => {
test('renders a table with columns displayed', async () => {
const tableName = 'example_name';
render(
<DatasetPanel
Expand All @@ -138,4 +138,23 @@ describe('DatasetPanel', () => {
expect(screen.getByText(row.type)).toBeInTheDocument();
});
});

test('renders an info banner if table already has a dataset', async () => {
render(
<DatasetPanel
tableName="example_table"
hasError={false}
columnList={exampleColumns}
loading={false}
linkedDatasets={exampleDataset}
/>,
);

// This is text in the info banner
expect(
await screen.findByText(
/you can only associate one dataset with one table. this table already has a dataset associated with it in preset./i,
),
).toBeVisible();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@
import React from 'react';
import { supersetTheme, t, styled } from '@superset-ui/core';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyndsiWilliams I missed this when we originally worked on DatasetPanel but using supersetTheme directly is an anti-pattern. We should remove this import, and instead bring in useTheme
import { useTheme, t, styled } from '@superset-ui/core';
At line 131-148, and line 288, anywhere else we directly use supersetTheme should be changed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIxed in this commit.

import Icons from 'src/components/Icons';
import Alert from 'src/components/Alert';
import Table, { ColumnsType, TableSize } from 'src/components/Table';
import { alphabeticalSort } from 'src/components/Table/sorters';
// @ts-ignore
import LOADING_GIF from 'src/assets/images/loading.gif';
import { DatasetObject } from 'src/views/CRUD/data/dataset/AddDataset/types';
import { ITableColumn } from './types';
import MessageContent from './MessageContent';

Expand Down Expand Up @@ -54,8 +56,10 @@ const MARGIN_MULTIPLIER = 3;

const StyledHeader = styled.div<StyledHeaderProps>`
position: ${(props: StyledHeaderProps) => props.position};
margin: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
margin-top: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
margin: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px
${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px
${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px
${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 3)}px;
eschutho marked this conversation as resolved.
Show resolved Hide resolved
font-size: ${({ theme }) => theme.gridUnit * 6}px;
font-weight: ${({ theme }) => theme.typography.weights.medium};
padding-bottom: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
Expand All @@ -74,7 +78,7 @@ const StyledHeader = styled.div<StyledHeaderProps>`
`;

const StyledTitle = styled.div`
margin-left: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
margin-left: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 3)}px;
margin-bottom: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
font-weight: ${({ theme }) => theme.typography.weights.bold};
`;
Expand Down Expand Up @@ -111,6 +115,7 @@ const StyledLoader = styled.div`
const TableContainer = styled.div`
position: relative;
margin: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
margin-left: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 3)}px;
overflow: scroll;
height: calc(100% - ${({ theme }) => theme.gridUnit * 36}px);
`;
Expand All @@ -123,6 +128,24 @@ const StyledTable = styled(Table)`
right: 0;
`;

const StyledAlert = styled(Alert)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to coment on line 20, we shuld repelase use of supertThem with:
const StyledAlert = styled(Alert)( ({ theme }) =>

and replace all the supersetTheme with theme
for example:
`border: 1px solid ${theme.colors.info.base};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIxed in this commit.

border: 1px solid ${supersetTheme.colors.info.base};
padding: ${supersetTheme.gridUnit * 4}px;
margin: ${supersetTheme.gridUnit * 6}px ${supersetTheme.gridUnit * 6}px
${supersetTheme.gridUnit * 8}px;
.view-dataset-button {
position: absolute;
top: ${supersetTheme.gridUnit * 4}px;
right: ${supersetTheme.gridUnit * 4}px;
font-weight: ${supersetTheme.typography.weights.normal};

&:hover {
color: ${supersetTheme.colors.secondary.dark3};
text-decoration: underline;
}
}
`;

export const REFRESHING = t('Refreshing columns');
export const COLUMN_TITLE = t('Table columns');
export const ALT_LOADING = t('Loading');
Expand Down Expand Up @@ -168,15 +191,48 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
linkedDatasets?: DatasetObject[] | undefined;
}

const renderExistingDatasetAlert = (linkedDataset: any) => (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put a type on this linkeddataset?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, good catch! Typed in this commit.

<StyledAlert
closable={false}
type="info"
showIcon
message={t('This table already has a dataset')}
description={
<>
{t(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend defining all text using t() as const outside of functions so that the text only gets translated once, not per render / function call. It also makes the text exportable for use in test files so you don't have to keep string literals defined in multiple files in sync manually.

I am commenting once but recommend for anywhere t() is being called inside a function / functional component.

Example can be seen on line 149, 150, 151

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation on this, I didn't realize it was running on each render/function call but it definitely makes sense! I fixed these spots and some others that I found in this commit.

'You can only associate one dataset with one table. This table already has a dataset associated with it in Preset.\n',
)}
<span
role="button"
onClick={() => {
window.open(
linkedDataset.explore_url,
'_blank',
'noreferer noopener popup=false',
);
}}
tabIndex={0}
className="view-dataset-button"
>
{t('View Dataset')}
</span>
</>
}
/>
);

const DatasetPanel = ({
tableName,
columnList,
loading,
hasError,
linkedDatasets,
}: IDatasetPanelProps) => {
const hasColumns = columnList?.length > 0 ?? false;
const linkedDatasetNames = linkedDatasets?.map(dataset => dataset.table_name);

let component;
if (loading) {
Expand Down Expand Up @@ -217,17 +273,23 @@ const DatasetPanel = ({
return (
<>
{tableName && (
<StyledHeader
position={
!loading && hasColumns ? EPosition.RELATIVE : EPosition.ABSOLUTE
}
title={tableName || ''}
>
{tableName && (
<Icons.Table iconColor={supersetTheme.colors.grayscale.base} />
)}
{tableName}
</StyledHeader>
<>
{linkedDatasetNames?.includes(tableName) &&
renderExistingDatasetAlert(
linkedDatasets?.find(dataset => dataset.table_name === tableName),
)}
<StyledHeader
position={
!loading && hasColumns ? EPosition.RELATIVE : EPosition.ABSOLUTE
}
title={tableName || ''}
>
{tableName && (
<Icons.Table iconColor={supersetTheme.colors.grayscale.base} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where I commented earlier use of supersetTheme directly is an anit pattern we did not catch in initial creation of this file.

Above in the functional component we should have
const theme = useTheme();

Then here use
<Icons.Table iconColor={theme.colors.grayscale.base} />`

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIxed in this commit.

)}
{tableName}
</StyledHeader>
</>
)}
{component}
</>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { DatasetObject } from 'src/views/CRUD/data/dataset/AddDataset/types';
import { ITableColumn } from './types';

export const exampleColumns: ITableColumn[] = [
Expand All @@ -32,3 +33,16 @@ export const exampleColumns: ITableColumn[] = [
type: 'DATE',
},
];

export const exampleDataset: DatasetObject[] = [
{
db: {
id: 1,
database_name: 'test_database',
owners: [1],
},
schema: 'test_schema',
dataset_name: 'example_dataset',
table_name: 'example_table',
},
];
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
import React, { useEffect, useState, useRef } from 'react';
import { SupersetClient } from '@superset-ui/core';
import { DatasetObject } from 'src/views/CRUD/data/dataset/AddDataset/types';
import DatasetPanel from './DatasetPanel';
import { ITableColumn, IDatabaseTable, isIDatabaseTable } from './types';

Expand Down Expand Up @@ -53,13 +54,15 @@ export interface IDatasetPanelWrapperProps {
*/
schema?: string | null;
setHasColumns?: Function;
linkedDatasets?: DatasetObject[] | undefined;
}

const DatasetPanelWrapper = ({
tableName,
dbId,
schema,
setHasColumns,
linkedDatasets,
}: IDatasetPanelWrapperProps) => {
const [columnList, setColumnList] = useState<ITableColumn[]>([]);
const [loading, setLoading] = useState(false);
Expand Down Expand Up @@ -110,7 +113,7 @@ const DatasetPanelWrapper = ({
if (tableName && schema && dbId) {
getTableMetadata({ tableName, dbId, schema });
}
// getTableMetadata is a const and should not be independency array
// getTableMetadata is a const and should not be in dependency array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 😁

// eslint-disable-next-line react-hooks/exhaustive-deps
}, [tableName, dbId, schema]);

Expand All @@ -120,6 +123,7 @@ const DatasetPanelWrapper = ({
hasError={hasError}
loading={loading}
tableName={tableName}
linkedDatasets={linkedDatasets}
/>
);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ const mockPropsWithDataset = {
};

describe('Footer', () => {
it('renders a Footer with a cancel button and a disabled create button', () => {
test('renders a Footer with a cancel button and a disabled create button', () => {
render(<Footer {...mockedProps} />, { useRedux: true });

const saveButton = screen.getByRole('button', {
Expand All @@ -55,7 +55,7 @@ describe('Footer', () => {
expect(createButton).toBeDisabled();
});

it('renders a Create Dataset button when a table is selected', () => {
test('renders a Create Dataset button when a table is selected', () => {
render(<Footer {...mockPropsWithDataset} />, { useRedux: true });

const createButton = screen.getByRole('button', {
Expand All @@ -64,4 +64,17 @@ describe('Footer', () => {

expect(createButton).toBeEnabled();
});

test('create button becomes disabled when table already has a dataset', () => {
render(
<Footer linkedDatasets={['real_info']} {...mockPropsWithDataset} />,
{ useRedux: true },
);

const createButton = screen.getByRole('button', {
name: /Create/i,
});

expect(createButton).toBeDisabled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ interface FooterProps {
datasetObject?: Partial<DatasetObject> | null;
onDatasetAdd?: (dataset: DatasetObject) => void;
hasColumns?: boolean;
linkedDatasets?: (string | null | undefined)[] | undefined;
}

const INPUT_FIELDS = ['db', 'schema', 'table_name'];
Expand All @@ -52,6 +53,7 @@ function Footer({
datasetObject,
addDangerToast,
hasColumns = false,
linkedDatasets,
}: FooterProps) {
const { createResource } = useSingleViewResource<Partial<DatasetObject>>(
'dataset',
Expand Down Expand Up @@ -113,7 +115,11 @@ function Footer({
<Button onClick={cancelButtonOnClick}>Cancel</Button>
<Button
buttonStyle="primary"
disabled={!datasetObject?.table_name || !hasColumns}
disabled={
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nit, but since this disabled check is getting kind of long it may be worth defining a variable doing this check before the return JSX block which will make the code easier to debug in future and keep the actual JSX portion cleaner.

Example:

const disabled = !datasetObject?.table_name ||
          !hasColumns ||
          linkedDatasets?.includes(datasetObject?.table_name);

  return (
    <>
      <Button onClick={cancelButtonOnClick}>Cancel</Button>
      <Button
        buttonStyle="primary"
        disabled
        tooltip={!datasetObject?.table_name ? tooltipText : undefined}
        onClick={onSave}
      >

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah good call, much cleaner! Changed in this commit

!datasetObject?.table_name ||
!hasColumns ||
linkedDatasets?.includes(datasetObject?.table_name)
}
tooltip={!datasetObject?.table_name ? tooltipText : undefined}
onClick={onSave}
>
Expand Down