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: Displaying details to Dataset/Database deletion modals #30016

Merged
merged 16 commits into from
Sep 10, 2024
Merged
1 change: 1 addition & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ repos:
- id: debug-statements
- id: end-of-file-fixer
- id: trailing-whitespace
exclude: ^.*\.(snap)
args: ["--markdown-linebreak-ext=md"]
- repo: https://github.com/pre-commit/mirrors-prettier
rev: v3.1.0 # Use the sha or tag you want to point at
Expand Down
3 changes: 3 additions & 0 deletions .rat-excludes
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ tsconfig.tsbuildinfo
generator-superset/*
temporary_superset_ui/*

# skip license checks for auto-generated test snapshots
.*snap

# docs overrides for third party logos we don't have the rights to
google-big-query.svg
google-sheets.svg
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/.prettierignore
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,6 @@ CHANGELOG/
*.geojson
*-topo.json
storybook-static/
*.snap

/.nx/workspace-data
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ test('calls the function on confirm', async () => {
const confirmInput = getByTestId('delete-modal-input');
fireEvent.change(confirmInput, { target: { value: 'DELETE' } });

const confirmButton = getByRole('button', { name: 'delete' });
const confirmButton = getByRole('button', { name: 'Delete' });
fireEvent.click(confirmButton);

await waitFor(() => expect(mockedProps.onConfirm).toHaveBeenCalledTimes(1));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,12 @@ test('Calling "onConfirm" only after typing "delete" in the input', () => {
expect(props.onConfirm).toBeCalledTimes(0);

// do not execute "onConfirm" if you have not typed "delete"
userEvent.click(screen.getByText('delete'));
userEvent.click(screen.getByText('Delete'));
expect(props.onConfirm).toBeCalledTimes(0);

// execute "onConfirm" if you have typed "delete"
userEvent.type(screen.getByTestId('delete-modal-input'), 'delete');
userEvent.click(screen.getByText('delete'));
userEvent.click(screen.getByText('Delete'));
expect(props.onConfirm).toBeCalledTimes(1);

// confirm input has been cleared
Expand Down
3 changes: 2 additions & 1 deletion superset-frontend/src/components/DeleteModal/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,11 @@ export default function DeleteModal({
disablePrimaryButton={disableChange}
onHide={hide}
onHandledPrimaryAction={confirm}
primaryButtonName={t('delete')}
primaryButtonName={t('Delete')}
rusackas marked this conversation as resolved.
Show resolved Hide resolved
primaryButtonType="danger"
show={open}
title={title}
centered
>
<DescriptionContainer>{description}</DescriptionContainer>
<StyledDiv>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,14 +164,7 @@ describe('Admin DatabaseList', () => {
});
await waitForComponentToPaint(wrapper);

expect(wrapper.find(DeleteModal).props().description)
.toMatchInlineSnapshot(`
<React.Fragment>
<p>
The database db 0 is linked to 0 charts that appear on 0 dashboards and users have 0 SQL Lab tabs using this database open. Are you sure you want to continue? Deleting the database will break those objects.
</p>
</React.Fragment>
`);
expect(wrapper.find(DeleteModal).props().description).toMatchSnapshot();
Copy link
Member Author

Choose a reason for hiding this comment

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

InlineSnapshots are no longer supported by Jest. A few changed files in this PR allow us to use regular snapshots, which remain supported.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that I also loosened constraints (prettier, license check) on those snapshot files, so they can be committed as-generated.


act(() => {
wrapper
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Admin DatabaseList deletes 1`] = `
<React.Fragment>
<p>
The database

<b>
db 0
</b>

is linked to 0 charts that appear on 0 dashboards and users have 0 SQL Lab tabs using this database open. Are you sure you want to continue? Deleting the database will break those objects.
</p>
</React.Fragment>
`;
85 changes: 76 additions & 9 deletions superset-frontend/src/pages/DatabaseList/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ 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 { useListViewResource } from 'src/views/CRUD/hooks';
import {
Expand Down Expand Up @@ -65,8 +64,8 @@ const dbConfigExtraExtension = extensionsRegistry.get(
const PAGE_SIZE = 25;

interface DatabaseDeleteObject extends DatabaseObject {
chart_count: number;
dashboard_count: number;
charts: any;
dashboards: any;
sqllab_tab_count: number;
}
interface DatabaseListProps {
Expand Down Expand Up @@ -170,8 +169,8 @@ function DatabaseList({
.then(({ json = {} }) => {
setDatabaseCurrentlyDeleting({
...database,
chart_count: json.charts.count,
dashboard_count: json.dashboards.count,
charts: json.charts,
dashboards: json.dashboards,
sqllab_tab_count: json.sqllab_tab_states.count,
});
})
Expand Down Expand Up @@ -609,14 +608,82 @@ function DatabaseList({
description={
<>
<p>
{t('The database')}{' '}
<b>{databaseCurrentlyDeleting.database_name}</b>{' '}
{t(
'The database %s is linked to %s charts that appear on %s dashboards and users have %s SQL Lab tabs using this database open. Are you sure you want to continue? Deleting the database will break those objects.',
databaseCurrentlyDeleting.database_name,
databaseCurrentlyDeleting.chart_count,
databaseCurrentlyDeleting.dashboard_count,
'is linked to %s charts that appear on %s dashboards and users have %s SQL Lab tabs using this database open. Are you sure you want to continue? Deleting the database will break those objects.',
databaseCurrentlyDeleting.charts.count,
databaseCurrentlyDeleting.dashboards.count,
databaseCurrentlyDeleting.sqllab_tab_count,
)}
</p>
{databaseCurrentlyDeleting.dashboards.count >= 1 && (
<>
<h4>{t('Affected Dashboards')}</h4>
<ul>
{databaseCurrentlyDeleting.dashboards.result
.slice(0, 10)
.map(
(
result: { id: number; title: string },
index: number,
) => (
<li key={result.id}>
<a
href={`/superset/dashboard/${result.id}`}
target="_atRiskItem"
>
{result.title}
</a>
</li>
),
)}
{databaseCurrentlyDeleting.dashboards.result.length >
10 && (
<li>
{t(
'... and %s others',
databaseCurrentlyDeleting.dashboards.result.length -
10,
)}
</li>
)}
</ul>
</>
)}
{databaseCurrentlyDeleting.charts.count >= 1 && (
<>
<h4>{t('Affected Charts')}</h4>
<ul>
{databaseCurrentlyDeleting.charts.result.slice(0, 10).map(
(
result: {
id: number;
slice_name: string;
},
index: number,
) => (
<li key={result.id}>
<a
href={`/explore/?slice_id=${result.id}`}
target="_atRiskItem"
>
{result.slice_name}
</a>
</li>
),
)}
{databaseCurrentlyDeleting.charts.result.length > 10 && (
<li>
{t(
'... and %s others',
databaseCurrentlyDeleting.charts.result.length - 10,
)}
</li>
)}
</ul>
</>
)}
{DatabaseDeleteRelatedExtension && (
<DatabaseDeleteRelatedExtension
database={databaseCurrentlyDeleting}
Expand Down
87 changes: 79 additions & 8 deletions superset-frontend/src/pages/DatasetList/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {
SupersetClient,
t,
} from '@superset-ui/core';
import { FunctionComponent, useState, useMemo, useCallback } from 'react';
import { FunctionComponent, useState, useMemo, useCallback, Key } from 'react';
import { Link, useHistory } from 'react-router-dom';
import rison from 'rison';
import {
Expand Down Expand Up @@ -154,7 +154,11 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
} = useListViewResource<Dataset>('dataset', t('dataset'), addDangerToast);

const [datasetCurrentlyDeleting, setDatasetCurrentlyDeleting] = useState<
(Dataset & { chart_count: number; dashboard_count: number }) | null
| (Dataset & {
charts: any;
dashboards: any;
})
| null
>(null);

const [datasetCurrentlyEditing, setDatasetCurrentlyEditing] =
Expand Down Expand Up @@ -243,8 +247,8 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
.then(({ json = {} }) => {
setDatasetCurrentlyDeleting({
...dataset,
chart_count: json.charts.count,
dashboard_count: json.dashboards.count,
charts: json.charts,
dashboards: json.dashboards,
});
})
.catch(
Expand Down Expand Up @@ -742,13 +746,80 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
description={
<>
<p>
{t('The dataset')}
<b> {datasetCurrentlyDeleting.table_name} </b>
{t(
'The dataset %s is linked to %s charts that appear on %s dashboards. Are you sure you want to continue? Deleting the dataset will break those objects.',
datasetCurrentlyDeleting.table_name,
datasetCurrentlyDeleting.chart_count,
datasetCurrentlyDeleting.dashboard_count,
'is linked to %s charts that appear on %s dashboards. Are you sure you want to continue? Deleting the dataset will break those objects.',
datasetCurrentlyDeleting.charts.count,
datasetCurrentlyDeleting.dashboards.count,
)}
</p>
{datasetCurrentlyDeleting.dashboards.count >= 1 && (
<>
<h4>{t('Affected Dashboards')}</h4>
<ul>
{datasetCurrentlyDeleting.dashboards.result
.slice(0, 10)
.map(
(
result: { id: Key | null | undefined; title: string },
index: number,
) => (
<li key={result.id}>
<a
href={`/superset/dashboard/${result.id}`}
target="_atRiskItem"
>
{result.title}
</a>
</li>
),
)}
{datasetCurrentlyDeleting.dashboards.result.length > 10 && (
<li>
{t(
'... and %s others',
datasetCurrentlyDeleting.dashboards.result.length -
10,
)}
</li>
)}
</ul>
</>
)}
{datasetCurrentlyDeleting.charts.count >= 1 && (
<>
<h4>{t('Affected Charts')}</h4>
<ul>
{datasetCurrentlyDeleting.charts.result.slice(0, 10).map(
(
result: {
id: Key | null | undefined;
slice_name: string;
},
index: number,
) => (
<li key={result.id}>
<a
href={`/explore/?slice_id=${result.id}`}
target="_atRiskItem"
>
{result.slice_name}
</a>
</li>
),
)}
{datasetCurrentlyDeleting.charts.result.length > 10 && (
<li>
{t(
'... and %s others',
datasetCurrentlyDeleting.charts.result.length - 10,
)}
</li>
)}
</ul>
</>
)}
{DatasetDeleteRelatedExtension && (
<DatasetDeleteRelatedExtension
dataset={datasetCurrentlyDeleting}
Expand Down
Loading