diff --git a/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.useModal.test.tsx b/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.useModal.test.tsx new file mode 100644 index 000000000000..038631ea2719 --- /dev/null +++ b/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.useModal.test.tsx @@ -0,0 +1,110 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { + render, + screen, + fireEvent, + act, + defaultStore as store, +} from 'spec/helpers/testing-library'; +import fetchMock from 'fetch-mock'; +import { Modal } from '@superset-ui/core/components'; +import mockDatasource from 'spec/fixtures/mockDatasource'; +import DatasourceModal from '.'; + +const mockedProps = { + datasource: mockDatasource['7__table'], + addSuccessToast: jest.fn(), + addDangerToast: jest.fn(), + onChange: jest.fn(), + onHide: jest.fn(), + show: true, + onDatasourceSave: jest.fn(), +}; + +beforeEach(() => { + fetchMock.reset(); + fetchMock.put('glob:*/api/v1/dataset/7?override_columns=*', {}); + fetchMock.get('glob:*/api/v1/dataset/7', { result: {} }); + fetchMock.get('glob:*/api/v1/database/?q=*', { result: [] }); +}); + +afterEach(() => { + fetchMock.reset(); + jest.clearAllMocks(); +}); + +test('DatasourceModal - should use Modal.useModal hook instead of Modal.confirm directly', () => { + const useModalSpy = jest.spyOn(Modal, 'useModal'); + const confirmSpy = jest.spyOn(Modal, 'confirm'); + + render(, { store }); + + // Should use the useModal hook + expect(useModalSpy).toHaveBeenCalled(); + + // Should not call Modal.confirm during initial render + expect(confirmSpy).not.toHaveBeenCalled(); + + useModalSpy.mockRestore(); + confirmSpy.mockRestore(); +}); + +test('DatasourceModal - should handle sync columns state without imperative modal updates', async () => { + // Test that we can successfully click the save button without DOM errors + // The actual checkbox is only visible when SQL has changed + render(, { store }); + + const saveButton = screen.getByTestId('datasource-modal-save'); + + // This should not throw any DOM errors + await act(async () => { + fireEvent.click(saveButton); + }); + + // Should show confirmation modal + expect(screen.getByText('Confirm save')).toBeInTheDocument(); + + // Should show the confirmation message + expect( + screen.getByText('Are you sure you want to save and apply changes?'), + ).toBeInTheDocument(); +}); + +test('DatasourceModal - should not store modal instance in state', () => { + // Mock console.warn to catch any warnings about refs or imperatives + const consoleWarn = jest.spyOn(console, 'warn').mockImplementation(); + + render(, { store }); + + // No warnings should be generated about improper React patterns + const reactWarnings = consoleWarn.mock.calls.filter(call => + call.some( + arg => + typeof arg === 'string' && + (arg.includes('findDOMNode') || + arg.includes('ref') || + arg.includes('instance')), + ), + ); + + expect(reactWarnings).toHaveLength(0); + + consoleWarn.mockRestore(); +}); diff --git a/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx b/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx index ecbd8144ddfa..36bd4f815fda 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx +++ b/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx @@ -16,13 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { - FunctionComponent, - useState, - useRef, - useEffect, - useCallback, -} from 'react'; +import { FunctionComponent, useState, useEffect, useCallback } from 'react'; import { useSelector } from 'react-redux'; import { styled, @@ -101,8 +95,7 @@ const DatasourceModal: FunctionComponent = ({ }) => { const theme = useTheme(); const [currentDatasource, setCurrentDatasource] = useState(datasource); - const syncColumnsRef = useRef(false); - const [confirmModal, setConfirmModal] = useState(null); + const [syncColumns, setSyncColumns] = useState(false); const currencies = useSelector< { common: { @@ -114,7 +107,6 @@ const DatasourceModal: FunctionComponent = ({ const [errors, setErrors] = useState([]); const [isSaving, setIsSaving] = useState(false); const [isEditing, setIsEditing] = useState(false); - const dialog = useRef(null); const [modal, contextHolder] = Modal.useModal(); const buildPayload = (datasource: Record) => { const payload: Record = { @@ -196,7 +188,7 @@ const DatasourceModal: FunctionComponent = ({ setIsSaving(true); try { await SupersetClient.put({ - endpoint: `/api/v1/dataset/${currentDatasource.id}?override_columns=${syncColumnsRef.current}`, + endpoint: `/api/v1/dataset/${currentDatasource.id}?override_columns=${syncColumns}`, jsonPayload: buildPayload(currentDatasource), }); @@ -281,14 +273,9 @@ const DatasourceModal: FunctionComponent = ({ impact the column definitions, you might want to skip this step.`)} /> { - syncColumnsRef.current = !syncColumnsRef.current; - if (confirmModal) { - confirmModal.update({ - content: getSaveDialog(), - }); - } + setSyncColumns(!syncColumns); }} /> = ({ {t('Are you sure you want to save and apply changes?')} ), - [currentDatasource.sql, datasource.sql, confirmModal], + [currentDatasource.sql, datasource.sql, syncColumns], ); - useEffect(() => { - if (confirmModal) { - confirmModal.update({ - content: getSaveDialog(), - }); - } - }, [confirmModal, getSaveDialog]); - useEffect(() => { if (datasource.sql !== currentDatasource.sql) { - syncColumnsRef.current = true; + setSyncColumns(true); } }, [datasource.sql, currentDatasource.sql]); const onClickSave = () => { - const modalInstance = modal.confirm({ + modal.confirm({ title: t('Confirm save'), content: getSaveDialog(), onOk: onConfirmSave, @@ -329,8 +308,6 @@ const DatasourceModal: FunctionComponent = ({ okText: t('OK'), cancelText: t('Cancel'), }); - setConfirmModal(modalInstance); - dialog.current = modalInstance; }; return (