From 7a713e5fd80f847c1100c4d265f671632e0a998d Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Fri, 27 Oct 2023 11:04:33 -0700 Subject: [PATCH] fix: allow for backward compatible errors (#25640) --- .../Datasource/DatasourceEditor.jsx | 2 +- .../Datasource/DatasourceModal.test.jsx | 156 ++++++++++-------- .../components/Datasource/DatasourceModal.tsx | 22 ++- .../src/components/ErrorMessage/types.ts | 9 - superset-frontend/src/utils/errorMessages.ts | 1 + 5 files changed, 107 insertions(+), 83 deletions(-) diff --git a/superset-frontend/src/components/Datasource/DatasourceEditor.jsx b/superset-frontend/src/components/Datasource/DatasourceEditor.jsx index 326b8e606faa..90defaf8dc09 100644 --- a/superset-frontend/src/components/Datasource/DatasourceEditor.jsx +++ b/superset-frontend/src/components/Datasource/DatasourceEditor.jsx @@ -1396,7 +1396,7 @@ class DatasourceEditor extends React.PureComponent { const { theme } = this.props; return ( - + {this.renderErrors()} ({ marginBottom: theme.gridUnit * 4 })} diff --git a/superset-frontend/src/components/Datasource/DatasourceModal.test.jsx b/superset-frontend/src/components/Datasource/DatasourceModal.test.jsx index 24536cac003d..bb6fce7577bd 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal.test.jsx +++ b/superset-frontend/src/components/Datasource/DatasourceModal.test.jsx @@ -18,30 +18,35 @@ */ import React from 'react'; import { act } from 'react-dom/test-utils'; -import { mount } from 'enzyme'; -import { Provider } from 'react-redux'; +import { + render, + screen, + waitFor, + fireEvent, + cleanup, +} from '@testing-library/react'; import fetchMock from 'fetch-mock'; +import { Provider } from 'react-redux'; import sinon from 'sinon'; -import { supersetTheme, ThemeProvider } from '@superset-ui/core'; - -import waitForComponentToPaint from 'spec/helpers/waitForComponentToPaint'; +import { + supersetTheme, + ThemeProvider, + SupersetClient, +} from '@superset-ui/core'; import { defaultStore as store } from 'spec/helpers/testing-library'; -import Modal from 'src/components/Modal'; import { DatasourceModal } from 'src/components/Datasource'; -import DatasourceEditor from 'src/components/Datasource/DatasourceEditor'; import * as uiCore from '@superset-ui/core'; import mockDatasource from 'spec/fixtures/mockDatasource'; -import { api } from 'src/hooks/apiResources/queryApi'; - -const datasource = mockDatasource['7__table']; +// Define your constants here const SAVE_ENDPOINT = 'glob:*/api/v1/dataset/7'; const SAVE_PAYLOAD = { new: 'data' }; const SAVE_DATASOURCE_ENDPOINT = 'glob:*/api/v1/dataset/7'; const GET_DATASOURCE_ENDPOINT = SAVE_DATASOURCE_ENDPOINT; +const GET_DATABASE_ENDPOINT = 'glob:*/api/v1/database/?q=*'; const mockedProps = { - datasource, + datasource: mockDatasource['7__table'], addSuccessToast: () => {}, addDangerToast: () => {}, onChange: () => {}, @@ -50,80 +55,101 @@ const mockedProps = { onDatasourceSave: sinon.spy(), }; -async function mountAndWait(props = mockedProps) { - const mounted = mount( +let container; +let isFeatureEnabledMock; + +async function renderAndWait(props = mockedProps) { + const { container: renderedContainer } = render( - + + + , - { - wrappingComponent: ThemeProvider, - wrappingComponentProps: { theme: supersetTheme }, - }, ); - await waitForComponentToPaint(mounted); - return mounted; + container = renderedContainer; } +beforeEach(() => { + fetchMock.reset(); + cleanup(); + isFeatureEnabledMock = jest.spyOn(uiCore, 'isFeatureEnabled'); + renderAndWait(); + fetchMock.post(SAVE_ENDPOINT, SAVE_PAYLOAD); + fetchMock.put(SAVE_DATASOURCE_ENDPOINT, {}); + fetchMock.get(GET_DATASOURCE_ENDPOINT, { result: {} }); + fetchMock.get(GET_DATABASE_ENDPOINT, { result: [] }); +}); + +afterEach(() => { + isFeatureEnabledMock.mockRestore(); +}); + describe('DatasourceModal', () => { - let wrapper; - let isFeatureEnabledMock; - beforeEach(async () => { - isFeatureEnabledMock = jest.spyOn(uiCore, 'isFeatureEnabled'); - fetchMock.reset(); - wrapper = await mountAndWait(); + it('renders', async () => { + expect(container).toBeDefined(); }); - afterAll(() => { - isFeatureEnabledMock.restore(); - act(() => { - store.dispatch(api.util.resetApiState()); - }); + it('renders the component', () => { + expect(screen.getByText('Edit Dataset')).toBeInTheDocument(); }); - it('renders', () => { - expect(wrapper.find(DatasourceModal)).toExist(); + it('renders a Modal', async () => { + expect(screen.getByRole('dialog')).toBeInTheDocument(); }); - it('renders a Modal', () => { - expect(wrapper.find(Modal)).toExist(); + it('renders a DatasourceEditor', async () => { + expect(screen.getByTestId('datasource-editor')).toBeInTheDocument(); }); - it('renders a DatasourceEditor', () => { - expect(wrapper.find(DatasourceEditor)).toExist(); + it('renders a legacy data source btn', () => { + const button = screen.getByTestId('datasource-modal-legacy-edit'); + expect(button).toBeInTheDocument(); }); - it('saves on confirm', async () => { - const callsP = fetchMock.post(SAVE_ENDPOINT, SAVE_PAYLOAD); - fetchMock.put(SAVE_DATASOURCE_ENDPOINT, {}); - fetchMock.get(GET_DATASOURCE_ENDPOINT, { result: {} }); - act(() => { - wrapper - .find('button[data-test="datasource-modal-save"]') - .props() - .onClick(); + it('disables the save button when the datasource is managed externally', () => { + // the render is currently in a before operation, so it needs to be cleaned up + // we could alternatively move all the renders back into the tests or find a better + // way to automatically render but still allow to pass in props with the tests + cleanup(); + + renderAndWait({ + ...mockedProps, + datasource: { ...mockedProps.datasource, is_managed_externally: true }, }); - await waitForComponentToPaint(wrapper); - act(() => { - const okButton = wrapper.find( - '.ant-modal-confirm .ant-modal-confirm-btns .ant-btn-primary', - ); - okButton.simulate('click'); + const saveButton = screen.getByTestId('datasource-modal-save'); + expect(saveButton).toBeDisabled(); + }); + + it('calls the onDatasourceSave function when the save button is clicked', async () => { + cleanup(); + const onDatasourceSave = jest.fn(); + + renderAndWait({ ...mockedProps, onDatasourceSave }); + const saveButton = screen.getByTestId('datasource-modal-save'); + await act(async () => { + fireEvent.click(saveButton); + const okButton = await screen.findByRole('button', { name: 'OK' }); + okButton.click(); + }); + await waitFor(() => { + expect(onDatasourceSave).toHaveBeenCalled(); }); - await waitForComponentToPaint(wrapper); - // one call to PUT, then one to GET - const expected = [ - 'http://localhost/api/v1/dataset/7', - 'http://localhost/api/v1/dataset/7', - ]; - expect(callsP._calls.map(call => call[0])).toEqual( - expected, - ); /* eslint no-underscore-dangle: 0 */ }); - it('renders a legacy data source btn', () => { - expect( - wrapper.find('button[data-test="datasource-modal-legacy-edit"]'), - ).toExist(); + it('should render error dialog', async () => { + jest + .spyOn(SupersetClient, 'put') + .mockRejectedValue(new Error('Something went wrong')); + await act(async () => { + const saveButton = screen.getByTestId('datasource-modal-save'); + fireEvent.click(saveButton); + const okButton = await screen.findByRole('button', { name: 'OK' }); + okButton.click(); + }); + await act(async () => { + const errorTitle = await screen.findByText('Error saving dataset'); + expect(errorTitle).toBeInTheDocument(); + }); }); }); diff --git a/superset-frontend/src/components/Datasource/DatasourceModal.tsx b/superset-frontend/src/components/Datasource/DatasourceModal.tsx index fed45b43f2e4..3e3acde4b259 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal.tsx +++ b/superset-frontend/src/components/Datasource/DatasourceModal.tsx @@ -31,13 +31,11 @@ import { import Modal from 'src/components/Modal'; import AsyncEsmComponent from 'src/components/AsyncEsmComponent'; -import { - genericSupersetError, - SupersetError, -} from 'src/components/ErrorMessage/types'; +import { SupersetError } from 'src/components/ErrorMessage/types'; import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace'; import withToasts from 'src/components/MessageToasts/withToasts'; import { useSelector } from 'react-redux'; +import { getClientErrorObject } from 'src/utils/getClientErrorObject'; const DatasourceEditor = AsyncEsmComponent(() => import('./DatasourceEditor')); @@ -207,16 +205,24 @@ const DatasourceModal: FunctionComponent = ({ }) .catch(response => { setIsSaving(false); - response.json().then((errorJson: { errors: SupersetError[] }) => { + getClientErrorObject(response).then(error => { + let errorResponse: SupersetError | undefined; + let errorText: string | undefined; + // sip-40 error response + if (error?.errors?.length) { + errorResponse = error.errors[0]; + } else if (typeof error.error === 'string') { + // backward compatible with old error messages + errorText = error.error; + } modal.error({ title: t('Error saving dataset'), okButtonProps: { danger: true, className: 'btn-danger' }, content: ( ), }); diff --git a/superset-frontend/src/components/ErrorMessage/types.ts b/superset-frontend/src/components/ErrorMessage/types.ts index c32435d7f4fb..7c4c3fe94a68 100644 --- a/superset-frontend/src/components/ErrorMessage/types.ts +++ b/superset-frontend/src/components/ErrorMessage/types.ts @@ -107,12 +107,3 @@ export type ErrorMessageComponentProps | null> = export type ErrorMessageComponent = React.ComponentType; - -/* Generic error to be returned when the backend returns an error response that is not - * SIP-41 compliant. */ -export const genericSupersetError = (extra: object) => ({ - error_type: ErrorTypeEnum.GENERIC_BACKEND_ERROR, - extra, - level: 'error' as ErrorLevel, - message: 'An error occurred', -}); diff --git a/superset-frontend/src/utils/errorMessages.ts b/superset-frontend/src/utils/errorMessages.ts index 16a04105c4c8..d5bfbdc17b80 100644 --- a/superset-frontend/src/utils/errorMessages.ts +++ b/superset-frontend/src/utils/errorMessages.ts @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ + // Error messages used in many places across applications const COMMON_ERR_MESSAGES = { SESSION_TIMED_OUT: