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

Dashboard: Fix plugin dashboard save as button #55197

Merged
merged 3 commits into from Sep 22, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
@@ -0,0 +1,95 @@
import { render, waitFor, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import React from 'react';
import { Provider } from 'react-redux';

import { configureStore } from 'app/store/configureStore';

import { DashboardModel } from '../../state';

import { SaveDashboardDrawer } from './SaveDashboardDrawer';

jest.mock('app/core/core', () => ({
...jest.requireActual('app/core/core'),
contextSrv: {},
}));

jest.mock('@grafana/runtime', () => ({
...jest.requireActual('@grafana/runtime'),
getBackendSrv: () => ({
post: mockPost,
}),
}));

jest.mock('app/core/services/backend_srv', () => ({
backendSrv: {
getDashboardByUid: jest.fn().mockResolvedValue({ dashboard: {} }),
},
}));
const mockOnDismiss = jest.fn();
const mockPost = jest.fn();

const setup = async () => {
const mockDashboard = new DashboardModel({
uid: 'mockDashboardUid',
version: 1,
});
const store = configureStore();

const { rerender } = await waitFor(() =>
render(
<Provider store={store}>
<SaveDashboardDrawer dashboard={mockDashboard} onDismiss={mockOnDismiss} />{' '}
</Provider>
)
);

return { rerender, mockDashboard, store };
Copy link
Contributor

Choose a reason for hiding this comment

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

setup() is doing too many things: creating a dashboard mock and returning it, configuring store and returning it, and rendering a component and returning it. I think it would be better to have mockDashboard as a part of a single describe() or even inside a single test whenever it's needed.
The configuration of the store can also just happen as a separate thing once for the whole test module.

};

describe('SaveDashboardDrawer', () => {
beforeEach(() => {
mockPost.mockClear();
});

it("renders SaveDashboardErrorProxy if there's an error and it not yet handled", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are never actually looking for the name SaveDashboardErrorProxy in the test and it will have to be changed here if the component is changed, maybe it's better to just say "renders a corresponding modal if there's an error..."

jest.spyOn(console, 'error').mockImplementation();
mockPost.mockRejectedValueOnce({
status: 412,
data: {
status: 'plugin-dashboard',
},
config: {},
});

const { rerender, store, mockDashboard } = await setup();

await userEvent.click(screen.getByRole('button', { name: /save/i }));

expect(screen.getByRole('button', { name: /cancel/i })).toBeInTheDocument();
expect(screen.getByRole('button', { name: /save as/i })).toBeInTheDocument();
expect(screen.getByRole('button', { name: /overwrite/i })).toBeInTheDocument();

rerender(
Copy link
Contributor

Choose a reason for hiding this comment

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

From here down it's confusing what is happening, as it seems to be a different test: "renders a corresponding Save new modal if there's an error that has already been handled".

<Provider store={store}>
<SaveDashboardDrawer dashboard={mockDashboard} onDismiss={mockOnDismiss} />{' '}
</Provider>
);

mockPost.mockClear();
mockPost.mockRejectedValueOnce({
status: 412,
data: {
status: 'plugin-dashboard',
},
isHandled: true,
config: {},
});

await userEvent.click(screen.getByRole('button', { name: /save/i }));

expect(screen.getByText(/save dashboard/i)).toBeInTheDocument();
expect(screen.queryByRole('button', { name: /save as/i })).not.toBeInTheDocument();
expect(screen.queryByRole('button', { name: /overwrite/i })).not.toBeInTheDocument();
});
});
Expand Up @@ -126,7 +126,7 @@ export const SaveDashboardDrawer = ({ dashboard, onDismiss, onSaveSuccess, isCop
);
};

if (state.error && isFetchError(state.error)) {
if (state.error && isFetchError(state.error) && !state.error.isHandled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That's neat 🧉

return (
<SaveDashboardErrorProxy
error={state.error}
Expand Down
Expand Up @@ -36,7 +36,7 @@ export const useDashboardSave = (dashboard: DashboardModel) => {

const notifyApp = useAppNotification();
useEffect(() => {
if (state.error) {
if (state.error && !state.loading) {
notifyApp.error(state.error.message ?? 'Error saving dashboard');
}
if (state.value) {
Expand Down