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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f5b01d5
User is now notified if a table already has a dataset
lyndsiWilliams Nov 8, 2022
16f1f6e
Tables with dataset flow complete, leftPanel tested
lyndsiWilliams Nov 15, 2022
d697bbf
Re-added banner to dataset panel post-rebase
lyndsiWilliams Nov 15, 2022
73c5950
DatasetPanel and Footer tested
lyndsiWilliams Nov 15, 2022
4281a00
Hover styles added to info banner
lyndsiWilliams Nov 16, 2022
b58e689
Set view dataset button to open in new tab
lyndsiWilliams Nov 16, 2022
8f47e55
Adjust View Dataset button hover color
lyndsiWilliams Nov 16, 2022
8654966
Fixed theming calls, bracket formation, logging, and typing issues
lyndsiWilliams Nov 21, 2022
ad13ff4
Move getDatasetList API call to a hook, change naming of linkedDatase…
lyndsiWilliams Nov 22, 2022
955ffc0
optimized setDatasets call
lyndsiWilliams Nov 22, 2022
01f4ffa
Remove console.log
lyndsiWilliams Nov 22, 2022
f4a42c1
Change banner text to be more general and not include Preset or Superset
lyndsiWilliams Nov 22, 2022
d77263d
Fix DatasetPanel test
lyndsiWilliams Nov 22, 2022
0bed85a
Fixed spelling of noreferrer
lyndsiWilliams Nov 22, 2022
5a3f188
Fix loader height so no vertical scrollbar shows
lyndsiWilliams Dec 2, 2022
11e2409
Implement Eric's scrollbar fix
lyndsiWilliams Dec 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.test.tsx
View file
Open in desktop
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}
datasets={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();
});
});
139 changes: 108 additions & 31 deletions superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@
* under the License.
*/
import React from 'react';
import { supersetTheme, t, styled } from '@superset-ui/core';
import { t, styled, useTheme } from '@superset-ui/core';
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 @@ -53,43 +55,51 @@ const HALF = 0.5;
const MARGIN_MULTIPLIER = 3;

const StyledHeader = styled.div<StyledHeaderProps>`
${({ theme }) => `
position: ${(props: StyledHeaderProps) => props.position};
margin: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
margin-top: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
font-size: ${({ theme }) => theme.gridUnit * 6}px;
font-weight: ${({ theme }) => theme.typography.weights.medium};
padding-bottom: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
margin: ${theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px
${theme.gridUnit * MARGIN_MULTIPLIER}px
${theme.gridUnit * MARGIN_MULTIPLIER}px
${theme.gridUnit * (MARGIN_MULTIPLIER + 3)}px;
font-size: ${theme.gridUnit * 6}px;
font-weight: ${theme.typography.weights.medium};
padding-bottom: ${theme.gridUnit * MARGIN_MULTIPLIER}px;

white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;

.anticon:first-of-type {
margin-right: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
margin-right: ${theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
}

.anticon:nth-of-type(2) {
margin-left: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
}
margin-left: ${theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
`}
`;

const StyledTitle = styled.div`
margin-left: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
margin-bottom: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
font-weight: ${({ theme }) => theme.typography.weights.bold};
${({ theme }) => `
margin-left: ${theme.gridUnit * (MARGIN_MULTIPLIER + 3)}px;
margin-bottom: ${theme.gridUnit * MARGIN_MULTIPLIER}px;
font-weight: ${theme.typography.weights.bold};
`}
`;

const LoaderContainer = styled.div`
padding: ${({ theme }) => theme.gridUnit * 8}px
${({ theme }) => theme.gridUnit * 6}px;
${({ theme }) => `
padding: ${theme.gridUnit * 8}px
${theme.gridUnit * 6}px;

display: flex;
align-items: center;
justify-content: center;
height: 100%;
`}
`;

const StyledLoader = styled.div`
${({ theme }) => `
max-width: 50%;
width: ${LOADER_WIDTH}px;

Expand All @@ -100,19 +110,23 @@ const StyledLoader = styled.div`

div {
width: 100%;
margin-top: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
margin-top: ${theme.gridUnit * MARGIN_MULTIPLIER}px;
text-align: center;
font-weight: ${({ theme }) => theme.typography.weights.normal};
font-size: ${({ theme }) => theme.typography.sizes.l}px;
color: ${({ theme }) => theme.colors.grayscale.light1};
font-weight: ${theme.typography.weights.normal};
font-size: ${theme.typography.sizes.l}px;
color: ${theme.colors.grayscale.light1};
}
`}
`;

const TableContainer = styled.div`
${({ theme }) => `
position: relative;
margin: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
margin: ${theme.gridUnit * MARGIN_MULTIPLIER}px;
margin-left: ${theme.gridUnit * (MARGIN_MULTIPLIER + 3)}px;
overflow: scroll;
height: calc(100% - ${({ theme }) => theme.gridUnit * 36}px);
height: calc(100% - ${theme.gridUnit * 36}px);
`}
`;

const StyledTable = styled(Table)`
Expand All @@ -123,6 +137,26 @@ 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.

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

&:hover {
color: ${theme.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 +202,52 @@ export interface IDatasetPanelProps {
* Boolean indicating if the component is in a loading state
*/
loading: boolean;
datasets?: DatasetObject[] | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

nit: I kind of want this to have a better name, maybe existingDatasets? or something along those lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was going off of what @eschutho suggested in this review comment. I'm not really settled on the best name here but I'd be fine changing it, would like to hear what Elizabeth thinks of existingDatasets as well.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense!

Does this need to be a list or can it be boolean? From my, limited, understanding of this feature you're seeing if there already is a dataset linked, so we don't need to be passing in a list of datasets, we could just check to see if there is an associated one.

Copy link
Member Author

@lyndsiWilliams lyndsiWilliams Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We need the whole object so we can cross-reference the list of table names with the table names in the list of datasets here:

superset/superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx

Lines 292 to 295 in d77263d

{datasetNames?.includes(tableName) &&
renderExistingDatasetAlert(
datasets?.find(dataset => dataset.table_name === tableName),
)}

And when it's passed into renderExistingDatasetAlert() we need to be able to pull out the explore_url from the dataset object here:

superset/superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx

Lines 224 to 230 in 0bed85a

onClick={() => {
window.open(
dataset?.explore_url,
'_blank',
'noreferrer noopener popup=false',
);
}}

Copy link
Member

Choose a reason for hiding this comment

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

got it! Makes sense

}

const EXISTING_DATASET_DESCRIPTION = t(
'You can only associate one dataset with one table. This table already has a dataset associated with it in Preset.\n',
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, does this need to be preset specific language? If so, then I think we need to add that in on the Preset side and have a more generic description in superset.

Copy link
Member Author

@lyndsiWilliams lyndsiWilliams Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That makes sense, but I'm not sure about Preset-specific language here. I think we'd need @yousoph 's input on this one.

Copy link
Member

Choose a reason for hiding this comment

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

Updated text: This table already has a dataset associated with it. You can only associate one dataset with a table.

Copy link
Member Author

Choose a reason for hiding this comment

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

I spoke with Sophie and Karan in Slack and we discussed changing the text to be more general and not include Superset or Preset. Changed in this commit.

);
const VIEW_DATASET = t('View Dataset');

const renderExistingDatasetAlert = (dataset?: DatasetObject) => (
<StyledAlert
closable={false}
type="info"
showIcon
message={t('This table already has a dataset')}
description={
<>
{EXISTING_DATASET_DESCRIPTION}
<span
role="button"
onClick={() => {
window.open(
dataset?.explore_url,
'_blank',
'noreferer noopener popup=false',
);
}}
tabIndex={0}
className="view-dataset-button"
>
{VIEW_DATASET}
</span>
</>
}
/>
);

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

let component;
if (loading) {
Expand Down Expand Up @@ -217,17 +288,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>
<>
{datasetNames?.includes(tableName) &&
renderExistingDatasetAlert(
datasets?.find(dataset => dataset.table_name === tableName),
)}
<StyledHeader
position={
!loading && hasColumns ? EPosition.RELATIVE : EPosition.ABSOLUTE
}
title={tableName || ''}
>
{tableName && (
<Icons.Table iconColor={theme.colors.grayscale.base} />
)}
{tableName}
</StyledHeader>
</>
)}
{component}
</>
Expand Down
14 changes: 14 additions & 0 deletions superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/fixtures.ts
View file
Open in desktop
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',
},
];
6 changes: 5 additions & 1 deletion superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/index.tsx
View file
Open in desktop
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;
datasets?: DatasetObject[] | undefined;
}

const DatasetPanelWrapper = ({
tableName,
dbId,
schema,
setHasColumns,
datasets,
}: 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}
datasets={datasets}
/>
);
};
Expand Down
Loading