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: Enable new dataset creation flow II #22835

Merged
merged 29 commits into from
Feb 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
5c5e211
optimized setDatasets call
lyndsiWilliams Nov 22, 2022
e822b94
Remove console.log
lyndsiWilliams Nov 22, 2022
9ec9c99
Create dataset polish/bug fix
lyndsiWilliams Nov 29, 2022
7dc42ef
Make defaultPageSize a const and move emptyStateComponent into src/co…
lyndsiWilliams Dec 19, 2022
2a42fda
Change all add dataset entrances to use new create page
lyndsiWilliams Jan 3, 2023
047e25e
Cancel now redirects to the previous page
lyndsiWilliams Jan 4, 2023
fc65ffe
Create dataset returns user to dataset list
lyndsiWilliams Jan 5, 2023
6538236
Delete AddDatasetModal
lyndsiWilliams Jan 5, 2023
7e167c2
Fix tests
lyndsiWilliams Jan 5, 2023
d2fbc98
Use useHistory instead of window.location.href
lyndsiWilliams Jan 9, 2023
ee40bdb
Address Cody's review comments
lyndsiWilliams Jan 11, 2023
64749a2
Add dangerToasts to error handling
lyndsiWilliams Jan 11, 2023
a31a141
Fix lint
lyndsiWilliams Jan 11, 2023
b03efda
Fix useEffect warnings and autosetting db from local storage, add tra…
lyndsiWilliams Jan 12, 2023
00feed8
fix lint
lyndsiWilliams Jan 12, 2023
3b0d32a
Add path back into error message
lyndsiWilliams Jan 12, 2023
c3673ec
Fix DatabaseModal test by mocking useHistory hook
lyndsiWilliams Jan 13, 2023
585b915
Add table_name query to /chart/add/ redirect
lyndsiWilliams Jan 13, 2023
fbd2f2d
Change text on footer create button
lyndsiWilliams Jan 13, 2023
c31363e
Add filter by database ID to rison in queryParams
lyndsiWilliams Jan 17, 2023
de429a9
Remove user-selected db from local storage when db is deleted
lyndsiWilliams Jan 17, 2023
211d8a1
Fix bugs
lyndsiWilliams Jan 23, 2023
dff5f07
Fix bug: Create dataset disabled in topnav + menu
lyndsiWilliams Jan 24, 2023
ee0f2f0
Better solution for top nav create dataset disabled bug
lyndsiWilliams Jan 26, 2023
4bafc1a
Fix bug: already existing datasets are not marked as "already exists"…
lyndsiWilliams Jan 27, 2023
e80adab
Fix CREATE_DATASET_TEXT and tooltip in footer, add todo note to histo…
lyndsiWilliams Jan 31, 2023
9fa7ccb
Merge branch 'master' into lyndsi/enable-dataset-creation-fixed
lyndsiWilliams Feb 1, 2023
fcf1546
Fix rebase
lyndsiWilliams Feb 1, 2023
bb94687
Fix LeftPanel test
lyndsiWilliams Feb 1, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -186,27 +186,6 @@ test('should render a create dataset infobox', async () => {
expect(infoboxText).toBeVisible();
});

test('should render a save dataset modal when "Create a dataset" is clicked', async () => {
const newProps = {
...props,
datasource: {
...datasource,
type: DatasourceType.Query,
},
};
render(<DatasourcePanel {...newProps} />, { useRedux: true, useDnd: true });

const createButton = await screen.findByRole('button', {
name: /create a dataset/i,
});

userEvent.click(createButton);

const saveDatasetModalTitle = screen.getByText(/save or overwrite dataset/i);

expect(saveDatasetModalTitle).toBeVisible();
});

test('should not render a save dataset modal when datasource is not query or dataset', async () => {
const newProps = {
...props,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,9 @@ const ColumnSelectPopover = ({
}, []);

const setDatasetAndClose = () => {
if (setDatasetModal) setDatasetModal(true);
if (setDatasetModal) {
setDatasetModal(true);
}
onClose();
};

Expand Down
5 changes: 1 addition & 4 deletions superset-frontend/src/pages/ChartCreation/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -337,10 +337,7 @@ export class ChartCreation extends React.PureComponent<
const isButtonDisabled = this.isBtnDisabled();
const datasetHelpText = this.state.canCreateDataset ? (
<span data-test="dataset-write">
<Link
to="/tablemodelview/list/#create"
data-test="add-chart-new-dataset"
>
<Link to="/dataset/add/" data-test="add-chart-new-dataset">
{t('Add a dataset')}
</Link>
{` ${t('or')} `}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import React, { useState, useMemo, useEffect } from 'react';
import rison from 'rison';
import { useSelector } from 'react-redux';
import { useQueryParams, BooleanParam } from 'use-query-params';
import { LocalStorageKeys, setItem } from 'src/utils/localStorageHelpers';

import Loading from 'src/components/Loading';
import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
Expand Down Expand Up @@ -157,6 +158,9 @@ function DatabaseList({ addDangerToast, addSuccessToast }: DatabaseListProps) {
refreshData();
addSuccessToast(t('Deleted: %s', dbName));

// Delete user-selected db from local storage
setItem(LocalStorageKeys.db, null);

// Close delete modal
setDatabaseCurrentlyDeleting(null);
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,14 @@ jest.mock('src/components/Icons/Icon', () => ({
StyledIcon: () => <span />,
}));

const mockHistoryPush = jest.fn();
jest.mock('react-router-dom', () => ({
...jest.requireActual('react-router-dom'),
useHistory: () => ({
push: mockHistoryPush,
}),
}));

const dbProps = {
show: true,
database_name: 'my database',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ interface DatabaseModalProps {
show: boolean;
databaseId: number | undefined; // If included, will go into edit mode
dbEngine: string | undefined; // if included goto step 2 with engine already set
history?: any;
}

export enum ActionType {
Expand Down Expand Up @@ -521,6 +522,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
show,
databaseId,
dbEngine,
history,
}) => {
const [db, setDB] = useReducer<
Reducer<Partial<DatabaseObject> | null, DBReducerActionType>
Expand Down Expand Up @@ -653,6 +655,16 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
onHide();
};

const redirectURL = (url: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding a TODO comment noting that this check and passing history as a prop can be removed once SQL Lab is in the SPA, as history will then be globally available? Assuming that describes the situation correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! Added the comment in this commit.

/* TODO (lyndsiWilliams): This check and passing history
as a prop can be removed once SQL Lab is in the SPA */
if (!isEmpty(history)) {
history?.push(url);
} else {
window.location.href = url;
}
};

// Database import logic
const {
state: {
Expand Down Expand Up @@ -1345,23 +1357,21 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
const renderCTABtns = () => (
<StyledBtns>
<Button
// eslint-disable-next-line no-return-assign
buttonStyle="secondary"
onClick={() => {
setLoading(true);
fetchAndSetDB();
window.location.href = '/tablemodelview/list#create';
redirectURL('/dataset/add/');
}}
>
{t('CREATE DATASET')}
</Button>
<Button
buttonStyle="secondary"
// eslint-disable-next-line no-return-assign
onClick={() => {
setLoading(true);
fetchAndSetDB();
window.location.href = `/superset/sqllab/?db=true`;
redirectURL(`/superset/sqllab/?db=true`);
}}
>
{t('QUERY DATA IN SQL LAB')}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ import React from 'react';
import { render, screen } from 'spec/helpers/testing-library';
import AddDataset from 'src/views/CRUD/data/dataset/AddDataset';

const mockHistoryPush = jest.fn();
jest.mock('react-router-dom', () => ({
...jest.requireActual('react-router-dom'),
useHistory: () => ({
push: mockHistoryPush,
}),
}));

describe('AddDataset', () => {
it('renders a blank state AddDataset', async () => {
render(<AddDataset />, { useRedux: true });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export const exampleDataset: DatasetObject[] = [
id: 1,
database_name: 'test_database',
owners: [1],
backend: 'test_backend',
},
schema: 'test_schema',
dataset_name: 'example_dataset',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@
* under the License.
*/
import React, { useEffect, useState, useRef } from 'react';
import { SupersetClient } from '@superset-ui/core';
import { SupersetClient, logging, t } from '@superset-ui/core';
import { DatasetObject } from 'src/views/CRUD/data/dataset/AddDataset/types';
import { addDangerToast } from 'src/components/MessageToasts/actions';
import DatasetPanel from './DatasetPanel';
import { ITableColumn, IDatabaseTable, isIDatabaseTable } from './types';

Expand Down Expand Up @@ -94,9 +95,17 @@ const DatasetPanelWrapper = ({
setColumnList([]);
setHasColumns?.(false);
setHasError(true);
// eslint-disable-next-line no-console
console.error(
`The API response from ${path} does not match the IDatabaseTable interface.`,
addDangerToast(
t(
'The API response from %s does not match the IDatabaseTable interface.',
path,
),
);
logging.error(
t(
'The API response from %s does not match the IDatabaseTable interface.',
path,
),
);
}
} catch (error) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ import React from 'react';
import { render, screen } from 'spec/helpers/testing-library';
import Footer from 'src/views/CRUD/data/dataset/AddDataset/Footer';

const mockHistoryPush = jest.fn();
jest.mock('react-router-dom', () => ({
...jest.requireActual('react-router-dom'),
useHistory: () => ({
push: mockHistoryPush,
}),
}));

const mockedProps = {
url: 'realwebsite.com',
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/
import React from 'react';
import { useHistory } from 'react-router-dom';
import Button from 'src/components/Button';
import { t } from '@superset-ui/core';
import { useSingleViewResource } from 'src/views/CRUD/hooks';
Expand Down Expand Up @@ -49,12 +50,12 @@ const LOG_ACTIONS = [
];

function Footer({
url,
datasetObject,
addDangerToast,
hasColumns = false,
datasets,
}: FooterProps) {
const history = useHistory();
const { createResource } = useSingleViewResource<Partial<DatasetObject>>(
'dataset',
t('dataset'),
Expand All @@ -72,11 +73,6 @@ function Footer({

return LOG_ACTIONS[value];
};
const goToPreviousUrl = () => {
// this is a placeholder url until the final feature gets implemented
// at that point we will be passing in the url of the previous location.
window.location.href = url;
};

const cancelButtonOnClick = () => {
if (!datasetObject) {
Expand All @@ -85,7 +81,7 @@ function Footer({
const logAction = createLogAction(datasetObject);
logEvent(logAction, datasetObject);
}
goToPreviousUrl();
history.goBack();
};

const tooltipText = t('Select a database table.');
Expand All @@ -104,13 +100,13 @@ function Footer({
if (typeof response === 'number') {
logEvent(LOG_ACTIONS_DATASET_CREATION_SUCCESS, datasetObject);
// When a dataset is created the response we get is its ID number
goToPreviousUrl();
history.push(`/chart/add/?dataset=${datasetObject.table_name}`);
}
});
}
};

const CREATE_DATASET_TEXT = t('Create Dataset');
const CREATE_DATASET_TEXT = t('Create dataset and create chart');
const disabledCheck =
!datasetObject?.table_name ||
!hasColumns ||
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import fetchMock from 'fetch-mock';
import userEvent from '@testing-library/user-event';
import { render, screen, waitFor } from 'spec/helpers/testing-library';
import LeftPanel from 'src/views/CRUD/data/dataset/AddDataset/LeftPanel';
import { exampleDataset } from 'src/views/CRUD/data/dataset/AddDataset/DatasetPanel/fixtures';

const databasesEndpoint = 'glob:*/api/v1/database/?q*';
const schemasEndpoint = 'glob:*/api/v1/database/*/schemas*';
Expand Down Expand Up @@ -136,8 +137,8 @@ fetchMock.get(schemasEndpoint, {
});

fetchMock.get(tablesEndpoint, {
count: 3,
result: [
tableLength: 3,
options: [
{ value: 'Sheet1', type: 'table', extra: null },
{ value: 'Sheet2', type: 'table', extra: null },
{ value: 'Sheet3', type: 'table', extra: null },
Expand Down Expand Up @@ -181,31 +182,29 @@ test('does not render blank state if there is nothing selected', async () => {
});

test('renders list of options when user clicks on schema', async () => {
render(<LeftPanel setDataset={mockFun} schema="schema_a" dbId={1} />, {
render(<LeftPanel setDataset={mockFun} dataset={exampleDataset[0]} />, {
useRedux: true,
});

// Click 'test-postgres' database to access schemas
const databaseSelect = screen.getByRole('combobox', {
name: 'Select database or type database name',
});
// Schema select should be disabled until database is selected
const schemaSelect = screen.getByRole('combobox', {
name: /select schema or type schema name/i,
});
userEvent.click(databaseSelect);
expect(await screen.findByText('test-postgres')).toBeInTheDocument();
expect(schemaSelect).toBeDisabled();
userEvent.click(screen.getByText('test-postgres'));

// Wait for schema field to be enabled
// Schema select will be automatically populated if there is only one schema
const schemaSelect = screen.getByRole('combobox', {
name: /select schema or type schema name/i,
});
await waitFor(() => {
expect(schemaSelect).toBeEnabled();
});
});

test('searches for a table name', async () => {
render(<LeftPanel setDataset={mockFun} schema="schema_a" dbId={1} />, {
render(<LeftPanel setDataset={mockFun} dataset={exampleDataset[0]} />, {
useRedux: true,
});

Expand Down Expand Up @@ -245,9 +244,8 @@ test('renders a warning icon when a table name has a pre-existing dataset', asyn
render(
<LeftPanel
setDataset={mockFun}
schema="schema_a"
dbId={1}
datasets={['Sheet2']}
dataset={exampleDataset[0]}
datasetNames={['Sheet2']}
/>,
{
useRedux: true,
Expand Down
Loading