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

Jump to bottom

fix: Address database options prepopulated with stale option, in dataset component #27797

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: Address database options prepopulated with stale option, in dataset component #27797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5805578
fix(dataset): Address database options prepopulated with stale databa…
jzhao62 Feb 28, 2024
2342587
(test): add MemoryRouter to testCases related with dataset/Leftpanel,…
jzhao62 Apr 6, 2024
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
60 changes: 44 additions & 16 deletions superset-frontend/src/features/datasets/AddDataset/LeftPanel/LeftPanel.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import userEvent from '@testing-library/user-event';
import { render, screen, waitFor } from 'spec/helpers/testing-library';
import LeftPanel from 'src/features/datasets/AddDataset/LeftPanel';
import { exampleDataset } from 'src/features/datasets/AddDataset/DatasetPanel/fixtures';
import { MemoryRouter } from 'react-router-dom';

const databasesEndpoint = 'glob:*/api/v1/database/?q*';
const schemasEndpoint = 'glob:*/api/v1/database/*/schemas*';
Expand Down Expand Up @@ -154,16 +155,26 @@ afterEach(() => {
const mockFun = jest.fn();

test('should render', async () => {
render(<LeftPanel setDataset={mockFun} />, {
useRedux: true,
});
render(
<MemoryRouter>
Copy link
Member

Choose a reason for hiding this comment

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

render(<LeftPanel setDataset={mockFun} />, {
    useRedux: true,
    useRouter: true,
});

<LeftPanel setDataset={mockFun} />
</MemoryRouter>,
{
useRedux: true,
},
);
expect(
await screen.findByText(/Select database or type to search databases/i),
).toBeInTheDocument();
});

test('should render schema selector, database selector container, and selects', async () => {
render(<LeftPanel setDataset={mockFun} />, { useRedux: true });
render(
<MemoryRouter>
<LeftPanel setDataset={mockFun} />
</MemoryRouter>,
{ useRedux: true },
);

expect(
await screen.findByText(/Select database or type to search databases/i),
Expand All @@ -180,7 +191,12 @@ test('should render schema selector, database selector container, and selects',
});

test('does not render blank state if there is nothing selected', async () => {
render(<LeftPanel setDataset={mockFun} />, { useRedux: true });
render(
<MemoryRouter>
<LeftPanel setDataset={mockFun} />{' '}
</MemoryRouter>,
{ useRedux: true },
);

expect(
await screen.findByText(/Select database or type to search databases/i),
Expand All @@ -190,9 +206,14 @@ 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} dataset={exampleDataset[0]} />, {
useRedux: true,
});
render(
<MemoryRouter>
<LeftPanel setDataset={mockFun} dataset={exampleDataset[0]} />
</MemoryRouter>,
{
useRedux: true,
},
);

// Click 'test-postgres' database to access schemas
const databaseSelect = screen.getByRole('combobox', {
Expand All @@ -212,9 +233,14 @@ test('renders list of options when user clicks on schema', async () => {
});

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

// Click 'test-postgres' database to access schemas
const databaseSelect = screen.getByRole('combobox', {
Expand Down Expand Up @@ -270,11 +296,13 @@ test('searches for a table name', async () => {

test('renders a warning icon when a table name has a pre-existing dataset', async () => {
render(
<LeftPanel
setDataset={mockFun}
dataset={exampleDataset[0]}
datasetNames={['Sheet2']}
/>,
<MemoryRouter>
<LeftPanel
setDataset={mockFun}
dataset={exampleDataset[0]}
datasetNames={['Sheet2']}
/>
</MemoryRouter>,
{
useRedux: true,
},
Expand Down
7 changes: 6 additions & 1 deletion superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
DatasetObject,
} from 'src/features/datasets/AddDataset/types';
import { Table } from 'src/hooks/apiResources';
import { useLocation } from 'react-router-dom';

interface LeftPanelProps {
setDataset: Dispatch<SetStateAction<object>>;
Expand Down Expand Up @@ -122,6 +123,9 @@ export default function LeftPanel({
datasetNames,
}: LeftPanelProps) {
const { addDangerToast } = useToasts();
const location = useLocation();

const isAddingDataSet = location.pathname.includes('/dataset/add') ?? false;
Copy link
Member

Choose a reason for hiding this comment

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

This is not the correct way of handling this case. This component receives the dataset as a parameter and it should be the responsibility of the caller to update the dataset accordingly, which will trigger a re-render of this component. If you check the call stack, you'll see the dataset is coming from a hook. We need to invalidate the cached results of that hook if the database changes. @justinpark

Copy link
Author

Choose a reason for hiding this comment

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

i see, i will check if i can patch a fix on the backend


const setDatabase = useCallback(
(db: Partial<DatabaseObject>) => {
Expand All @@ -144,6 +148,7 @@ export default function LeftPanel({
});
};
useEffect(() => {
// TODO: storing and fetching the last selected database from local storage is potentially technical debt
const currentUserSelectedDb = getItem(
LocalStorageKeys.Database,
null,
Expand Down Expand Up @@ -174,7 +179,7 @@ export default function LeftPanel({
return (
<LeftPanelStyle>
<TableSelector
database={dataset?.db}
database={isAddingDataSet ? null : dataset?.db}
handleError={addDangerToast}
emptyState={emptyStateComponent(false)}
onDbChange={setDatabase}
Expand Down
Loading