This is an automated email from the ASF dual-hosted git repository.
msyavuz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new a90928766b fix(theme-crud): add unsaved changes modal (#35254)
a90928766b is described below
commit a90928766b1072d8ca31a3f86ed72ac5fb747055
Author: Gabriel Torres Ruiz <[email protected]>
AuthorDate: Tue Oct 7 12:26:01 2025 -0400
fix(theme-crud): add unsaved changes modal (#35254)
---
.../src/features/themes/ThemeModal.test.tsx | 698 +++++++++++++++------
.../src/features/themes/ThemeModal.tsx | 345 ++++++----
.../src/hooks/useBeforeUnload/index.ts | 43 ++
.../hooks/useBeforeUnload/useBeforeUnload.test.ts | 156 +++++
.../src/hooks/useUnsavedChangesPrompt/index.ts | 17 +-
5 files changed, 920 insertions(+), 339 deletions(-)
diff --git a/superset-frontend/src/features/themes/ThemeModal.test.tsx
b/superset-frontend/src/features/themes/ThemeModal.test.tsx
index 633a8c44bd..995b3c902f 100644
--- a/superset-frontend/src/features/themes/ThemeModal.test.tsx
+++ b/superset-frontend/src/features/themes/ThemeModal.test.tsx
@@ -17,10 +17,12 @@
* under the License.
*/
+import { render, screen } from 'spec/helpers/testing-library';
+import userEvent from '@testing-library/user-event';
import fetchMock from 'fetch-mock';
+import ThemeModal from './ThemeModal';
import { ThemeObject } from './types';
-// Mock theme provider
const mockThemeContext = {
setTemporaryTheme: jest.fn(),
clearLocalOverrides: jest.fn(),
@@ -31,49 +33,17 @@ jest.mock('src/theme/ThemeProvider', () => ({
useThemeContext: () => mockThemeContext,
}));
-// Mock permission utils
jest.mock('src/dashboard/util/permissionUtils', () => ({
isUserAdmin: jest.fn(() => true),
}));
-// Mock bootstrap data
-jest.mock('src/utils/getBootstrapData', () => ({
- __esModule: true,
- default: () => ({
- user: {
- userId: 1,
- firstName: 'Admin',
- lastName: 'User',
- roles: { Admin: [['can_write', 'Dashboard']] },
- permissions: {},
- isActive: true,
- isAnonymous: false,
- username: 'admin',
- email: '[email protected]',
- user_id: 1,
- first_name: 'Admin',
- last_name: 'User',
- },
- common: {
- feature_flags: {},
- conf: {
- SUPERSET_WEBSERVER_DOMAINS: [],
- },
- },
- }),
-}));
-
const mockTheme: ThemeObject = {
id: 1,
theme_name: 'Test Theme',
json_data: JSON.stringify(
{
- colors: {
- primary: '#1890ff',
- secondary: '#52c41a',
- },
- typography: {
- fontSize: 14,
+ token: {
+ colorPrimary: '#1890ff',
},
},
null,
@@ -87,201 +57,527 @@ const mockTheme: ThemeObject = {
},
};
-// Mock theme API endpoints
-fetchMock.get('glob:*/api/v1/theme/1', {
- result: mockTheme,
+const mockSystemTheme: ThemeObject = {
+ ...mockTheme,
+ id: 2,
+ theme_name: 'System Theme',
+ is_system: true,
+};
+
+beforeEach(() => {
+ fetchMock.reset();
+ fetchMock.get('glob:*/api/v1/theme/1', { result: mockTheme });
+ fetchMock.get('glob:*/api/v1/theme/2', { result: mockSystemTheme });
+ fetchMock.get('glob:*/api/v1/theme/*', { result: mockTheme });
+ fetchMock.post('glob:*/api/v1/theme/', { result: { ...mockTheme, id: 3 } });
+ fetchMock.put('glob:*/api/v1/theme/*', { result: mockTheme });
});
-fetchMock.post('glob:*/api/v1/theme/', {
- result: { ...mockTheme, id: 2 },
+afterEach(() => {
+ fetchMock.restore();
+ jest.clearAllMocks();
});
-fetchMock.put('glob:*/api/v1/theme/1', {
- result: mockTheme,
+test('renders modal with add theme dialog when show is true', () => {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ expect(screen.getByRole('dialog')).toBeInTheDocument();
+ expect(screen.getByText('Add theme')).toBeInTheDocument();
});
-// These are defined but not used in the simplified tests
-// const mockUser = {
-// userId: 1,
-// firstName: 'Admin',
-// lastName: 'User',
-// roles: { Admin: [['can_write', 'Dashboard']] },
-// permissions: {},
-// isActive: true,
-// isAnonymous: false,
-// username: 'admin',
-// email: '[email protected]',
-// user_id: 1,
-// first_name: 'Admin',
-// last_name: 'User',
-// };
-
-// const defaultProps = {
-// addDangerToast: jest.fn(),
-// addSuccessToast: jest.fn(),
-// onThemeAdd: jest.fn(),
-// onHide: jest.fn(),
-// show: true,
-// };
-
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
-describe('ThemeModal', () => {
- beforeEach(() => {
- fetchMock.resetHistory();
- jest.clearAllMocks();
- });
+test('does not render modal when show is false', () => {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show={false}
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ expect(screen.queryByRole('dialog')).not.toBeInTheDocument();
+});
- afterEach(() => {
- fetchMock.restore();
- });
+test('renders edit mode title when theme is provided', async () => {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop={false}
+ theme={mockTheme}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ // Wait for theme name to be loaded in the input field
+ expect(await screen.findByDisplayValue('Test Theme')).toBeInTheDocument();
+ expect(screen.getByText('Edit theme properties')).toBeInTheDocument();
+});
- test('should export ThemeModal component', () => {
- const ThemeModalModule = jest.requireActual('./ThemeModal');
- expect(ThemeModalModule.default).toBeDefined();
- expect(typeof ThemeModalModule.default).toBe('object'); // HOC wrapped
component
- });
+test('renders view mode title for system themes', async () => {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop={false}
+ theme={mockSystemTheme}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ // Wait for system theme indicator to appear
+ expect(
+ await screen.findByText('System Theme - Read Only'),
+ ).toBeInTheDocument();
+ expect(screen.getByText('View theme properties')).toBeInTheDocument();
+});
- test('should have correct type definitions', () => {
- expect(mockTheme).toMatchObject({
- id: expect.any(Number),
- theme_name: expect.any(String),
- json_data: expect.any(String),
- changed_on_delta_humanized: expect.any(String),
- changed_by: expect.objectContaining({
- first_name: expect.any(String),
- last_name: expect.any(String),
- }),
- });
- });
+test('renders theme name input field', () => {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ expect(screen.getByPlaceholderText('Enter theme name')).toBeInTheDocument();
+});
- test('should validate JSON data structure', () => {
- const isValidJson = (str: string) => {
- try {
- JSON.parse(str);
- return true;
- } catch (e) {
- return false;
- }
- };
-
- expect(isValidJson(mockTheme.json_data || '')).toBe(true);
- expect(isValidJson('invalid json')).toBe(false);
- expect(isValidJson('{"valid": "json"}')).toBe(true);
- });
+test('renders JSON configuration field', () => {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ expect(screen.getByText('JSON Configuration')).toBeInTheDocument();
+});
- test('should handle theme data parsing', () => {
- const parsedTheme = JSON.parse(mockTheme.json_data || '{}');
- expect(parsedTheme).toMatchObject({
- colors: {
- primary: '#1890ff',
- secondary: '#52c41a',
- },
- typography: {
- fontSize: 14,
- },
- });
- });
+test('disables inputs for read-only system themes', async () => {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop={false}
+ theme={mockSystemTheme}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ const nameInput = await screen.findByPlaceholderText('Enter theme name');
+
+ expect(nameInput).toHaveAttribute('readOnly');
+});
- test('should mock theme context functions', () => {
- expect(mockThemeContext.setTemporaryTheme).toBeDefined();
- expect(mockThemeContext.clearLocalOverrides).toBeDefined();
- expect(mockThemeContext.hasDevOverride).toBeDefined();
- expect(typeof mockThemeContext.setTemporaryTheme).toBe('function');
- expect(typeof mockThemeContext.clearLocalOverrides).toBe('function');
- expect(typeof mockThemeContext.hasDevOverride).toBe('function');
- });
+test('shows Apply button when canDevelop is true and theme exists', async ()
=> {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop
+ theme={mockTheme}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ expect(
+ await screen.findByRole('button', { name: /apply/i }),
+ ).toBeInTheDocument();
+});
- test('should handle API response structure', () => {
- // Test that fetch mock is properly configured
- expect(fetchMock.called()).toBe(false);
+test('does not show Apply button when canDevelop is false', () => {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ expect(
+ screen.queryByRole('button', { name: 'Apply' }),
+ ).not.toBeInTheDocument();
+});
- // Test API structure expectations
- const expectedResponse = {
- result: mockTheme,
- };
+test('disables save button when theme name is empty', () => {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ const saveButton = screen.getByRole('button', { name: 'Add' });
+
+ expect(saveButton).toBeDisabled();
+});
- expect(expectedResponse.result).toMatchObject({
- id: 1,
- theme_name: 'Test Theme',
- });
- });
+test('enables save button when theme name is entered', async () => {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ const nameInput = screen.getByPlaceholderText('Enter theme name');
+ await userEvent.type(nameInput, 'My New Theme');
+
+ const saveButton = await screen.findByRole('button', { name: 'Add' });
+
+ expect(saveButton).toBeEnabled();
+});
- test('should handle create theme API call', () => {
- const newTheme = {
- theme_name: 'New Theme',
- json_data: '{"colors": {"primary": "#ff0000"}}',
- };
+test('validates JSON format and enables save button', async () => {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ const nameInput = screen.getByPlaceholderText('Enter theme name');
+ await userEvent.type(nameInput, 'Test Theme');
+
+ const saveButton = await screen.findByRole('button', { name: 'Add' });
+
+ expect(saveButton).toBeEnabled();
+});
- // Test request structure
- expect(newTheme).toMatchObject({
- theme_name: expect.any(String),
- json_data: expect.any(String),
- });
+test('shows unsaved changes alert when closing modal with modifications',
async () => {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ const nameInput = screen.getByPlaceholderText('Enter theme name');
+ await userEvent.type(nameInput, 'Modified Theme');
+
+ const cancelButton = screen.getByRole('button', { name: 'Cancel' });
+ await userEvent.click(cancelButton);
+
+ expect(
+ await screen.findByText('You have unsaved changes'),
+ ).toBeInTheDocument();
+ expect(
+ screen.getByText('Your changes will be lost if you leave without saving.'),
+ ).toBeInTheDocument();
+ expect(
+ screen.getByRole('button', { name: 'Keep editing' }),
+ ).toBeInTheDocument();
+ expect(screen.getByRole('button', { name: 'Discard' })).toBeInTheDocument();
+ expect(screen.getByRole('button', { name: 'Save' })).toBeInTheDocument();
+});
+
+test('does not show unsaved changes alert when closing without modifications',
async () => {
+ const onHide = jest.fn();
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={onHide}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ const cancelButton = screen.getByRole('button', { name: 'Cancel' });
+ await userEvent.click(cancelButton);
+
+ expect(onHide).toHaveBeenCalled();
+ expect(
+ screen.queryByText('You have unsaved changes'),
+ ).not.toBeInTheDocument();
+});
- // Test that JSON is valid
- expect(() => JSON.parse(newTheme.json_data)).not.toThrow();
+test('allows user to keep editing after triggering cancel alert', async () => {
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ const nameInput = screen.getByPlaceholderText('Enter theme name');
+ await userEvent.type(nameInput, 'Modified Theme');
+
+ const cancelButton = screen.getByRole('button', { name: 'Cancel' });
+ await userEvent.click(cancelButton);
+
+ expect(
+ await screen.findByText('You have unsaved changes'),
+ ).toBeInTheDocument();
+
+ const keepEditingButton = screen.getByRole('button', {
+ name: 'Keep editing',
});
+ await userEvent.click(keepEditingButton);
+
+ expect(
+ screen.queryByText('You have unsaved changes'),
+ ).not.toBeInTheDocument();
+ expect(screen.getByPlaceholderText('Enter theme name')).toHaveValue(
+ 'Modified Theme',
+ );
+});
+
+test('saves changes when clicking Save button in unsaved changes alert', async
() => {
+ const onHide = jest.fn();
+ const onThemeAdd = jest.fn();
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={onThemeAdd}
+ onHide={onHide}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ const nameInput = screen.getByPlaceholderText('Enter theme name');
+ await userEvent.type(nameInput, 'Modified Theme');
+
+ const cancelButton = screen.getByRole('button', { name: 'Cancel' });
+ await userEvent.click(cancelButton);
+
+ expect(
+ await screen.findByText('You have unsaved changes'),
+ ).toBeInTheDocument();
+
+ const saveButton = screen.getByRole('button', { name: 'Save' });
+ await userEvent.click(saveButton);
+
+ // Wait for API call to complete
+ await screen.findByRole('dialog');
+ expect(fetchMock.called()).toBe(true);
+});
- test('should handle update theme API call', () => {
- const updatedTheme = {
- theme_name: 'Updated Theme',
- json_data: '{"colors": {"primary": "#00ff00"}}',
- };
+test('discards changes when clicking Discard button in unsaved changes alert',
async () => {
+ const onHide = jest.fn();
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={onHide}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ const nameInput = screen.getByPlaceholderText('Enter theme name');
+ await userEvent.type(nameInput, 'Modified Theme');
+
+ const cancelButton = screen.getByRole('button', { name: 'Cancel' });
+ await userEvent.click(cancelButton);
+
+ expect(
+ await screen.findByText('You have unsaved changes'),
+ ).toBeInTheDocument();
+
+ const discardButton = screen.getByRole('button', { name: 'Discard' });
+ await userEvent.click(discardButton);
+
+ expect(onHide).toHaveBeenCalled();
+ expect(fetchMock.called('glob:*/api/v1/theme/', 'POST')).toBe(false);
+ expect(fetchMock.called('glob:*/api/v1/theme/*', 'PUT')).toBe(false);
+});
- // Test request structure
- expect(updatedTheme).toMatchObject({
- theme_name: expect.any(String),
- json_data: expect.any(String),
- });
+test('creates new theme when saving', async () => {
+ const onHide = jest.fn();
+ const onThemeAdd = jest.fn();
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={onThemeAdd}
+ onHide={onHide}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ const nameInput = screen.getByPlaceholderText('Enter theme name');
+ await userEvent.type(nameInput, 'New Theme');
+
+ const saveButton = await screen.findByRole('button', { name: 'Add' });
+ await userEvent.click(saveButton);
+
+ expect(await screen.findByRole('dialog')).toBeInTheDocument();
+ expect(fetchMock.called('glob:*/api/v1/theme/', 'POST')).toBe(true);
+});
- // Test that JSON is valid
- expect(() => JSON.parse(updatedTheme.json_data)).not.toThrow();
- });
+test('updates existing theme when saving', async () => {
+ const onHide = jest.fn();
+ const onThemeAdd = jest.fn();
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={onThemeAdd}
+ onHide={onHide}
+ show
+ canDevelop={false}
+ theme={mockTheme}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ const nameInput = await screen.findByDisplayValue('Test Theme');
+ await userEvent.clear(nameInput);
+ await userEvent.type(nameInput, 'Updated Theme');
+
+ const saveButton = screen.getByRole('button', { name: 'Save' });
+ await userEvent.click(saveButton);
+
+ expect(await screen.findByRole('dialog')).toBeInTheDocument();
+ expect(fetchMock.called('glob:*/api/v1/theme/*', 'PUT')).toBe(true);
+});
- test('should validate theme name requirements', () => {
- const validateThemeName = (name: string) => !!(name && name.length > 0);
+test('handles API errors gracefully', async () => {
+ fetchMock.restore();
+ fetchMock.post('glob:*/api/v1/theme/', 500);
- expect(validateThemeName('Valid Theme')).toBe(true);
- expect(validateThemeName('')).toBe(false);
- expect(validateThemeName('Test')).toBe(true);
- });
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop={false}
+ />,
+ { useRedux: true, useRouter: true },
+ );
- test('should validate JSON configuration requirements', () => {
- const validateJsonData = (jsonData: string) => {
- if (!jsonData || jsonData.length === 0) return false;
- try {
- JSON.parse(jsonData);
- return true;
- } catch (e) {
- return false;
- }
- };
-
- expect(validateJsonData(mockTheme.json_data || '')).toBe(true);
- expect(validateJsonData('')).toBe(false);
- expect(validateJsonData('invalid')).toBe(false);
- expect(validateJsonData('{}')).toBe(true);
- });
+ const nameInput = screen.getByPlaceholderText('Enter theme name');
+ await userEvent.type(nameInput, 'New Theme');
- test('should handle permission-based feature availability', () => {
- const permissionUtils = jest.requireMock(
- 'src/dashboard/util/permissionUtils',
- );
+ const saveButton = await screen.findByRole('button', { name: 'Add' });
+ expect(saveButton).toBeEnabled();
- expect(permissionUtils.isUserAdmin).toBeDefined();
- expect(typeof permissionUtils.isUserAdmin).toBe('function');
- expect(permissionUtils.isUserAdmin()).toBe(true);
+ await userEvent.click(saveButton);
- // Test with non-admin user
- (permissionUtils.isUserAdmin as jest.Mock).mockReturnValue(false);
- expect(permissionUtils.isUserAdmin()).toBe(false);
- });
+ await screen.findByRole('dialog');
+ expect(fetchMock.called()).toBe(true);
+});
- test('should handle theme context override state', () => {
- expect(mockThemeContext.hasDevOverride()).toBe(false);
+test('applies theme locally when clicking Apply button', async () => {
+ const onThemeApply = jest.fn();
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop
+ theme={mockTheme}
+ onThemeApply={onThemeApply}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ const applyButton = await screen.findByRole('button', { name: /apply/i });
+ expect(applyButton).toBeEnabled();
+
+ await userEvent.click(applyButton);
+
+ expect(mockThemeContext.setTemporaryTheme).toHaveBeenCalled();
+});
- // Test with override
- mockThemeContext.hasDevOverride.mockReturnValue(true);
- expect(mockThemeContext.hasDevOverride()).toBe(true);
+test('disables Apply button when JSON configuration is invalid', async () => {
+ fetchMock.reset();
+ fetchMock.get('glob:*/api/v1/theme/*', {
+ result: { ...mockTheme, json_data: 'invalid json' },
});
+
+ render(
+ <ThemeModal
+ addDangerToast={jest.fn()}
+ addSuccessToast={jest.fn()}
+ onThemeAdd={jest.fn()}
+ onHide={jest.fn()}
+ show
+ canDevelop
+ theme={mockTheme}
+ />,
+ { useRedux: true, useRouter: true },
+ );
+
+ const applyButton = await screen.findByRole('button', { name: /apply/i });
+ expect(applyButton).toBeDisabled();
});
diff --git a/superset-frontend/src/features/themes/ThemeModal.tsx
b/superset-frontend/src/features/themes/ThemeModal.tsx
index 96a7936bf3..30ef18df4f 100644
--- a/superset-frontend/src/features/themes/ThemeModal.tsx
+++ b/superset-frontend/src/features/themes/ThemeModal.tsx
@@ -16,23 +16,33 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { FunctionComponent, useState, useEffect, ChangeEvent } from 'react';
+import {
+ FunctionComponent,
+ useState,
+ useEffect,
+ useCallback,
+ useMemo,
+ ChangeEvent,
+} from 'react';
+import { omit } from 'lodash';
import { css, styled, t, useTheme } from '@superset-ui/core';
import { useSingleViewResource } from 'src/views/CRUD/hooks';
import { useThemeContext } from 'src/theme/ThemeProvider';
+import { useBeforeUnload } from 'src/hooks/useBeforeUnload';
import SupersetText from 'src/utils/textUtils';
import { Icons } from '@superset-ui/core/components/Icons';
import withToasts from 'src/components/MessageToasts/withToasts';
import {
- Input,
- Modal,
- JsonEditor,
+ Alert,
Button,
Form,
+ Input,
+ JsonEditor,
+ Modal,
+ Space,
Tooltip,
- Alert,
} from '@superset-ui/core/components';
import { useJsonValidation } from
'@superset-ui/core/components/AsyncAceEditor';
import { Typography } from '@superset-ui/core/components/Typography';
@@ -53,7 +63,7 @@ interface ThemeModalProps {
type ThemeStringKeys = keyof Pick<
ThemeObject,
- OnlyKeyWithType<ThemeObject, String>
+ OnlyKeyWithType<ThemeObject, string>
>;
const StyledJsonEditor = styled.div`
@@ -100,7 +110,9 @@ const ThemeModal: FunctionComponent<ThemeModalProps> = ({
const { setTemporaryTheme } = useThemeContext();
const [disableSave, setDisableSave] = useState<boolean>(true);
const [currentTheme, setCurrentTheme] = useState<ThemeObject | null>(null);
+ const [initialTheme, setInitialTheme] = useState<ThemeObject | null>(null);
const [isHidden, setIsHidden] = useState<boolean>(true);
+ const [showConfirmAlert, setShowConfirmAlert] = useState<boolean>(false);
const isEditMode = theme !== null;
const isSystemTheme = currentTheme?.is_system === true;
const isReadOnly = isSystemTheme;
@@ -130,29 +142,35 @@ const ThemeModal: FunctionComponent<ThemeModalProps> = ({
} = useSingleViewResource<ThemeObject>('theme', t('theme'), addDangerToast);
// Functions
- const hide = () => {
+ const hasUnsavedChanges = useCallback(() => {
+ if (!currentTheme || !initialTheme || isReadOnly) return false;
+ return (
+ currentTheme.theme_name !== initialTheme.theme_name ||
+ currentTheme.json_data !== initialTheme.json_data
+ );
+ }, [currentTheme, initialTheme, isReadOnly]);
+
+ const hide = useCallback(() => {
onHide();
setCurrentTheme(null);
- };
+ setInitialTheme(null);
+ setShowConfirmAlert(false);
+ }, [onHide]);
- const onSave = () => {
+ const onSave = useCallback(() => {
if (isEditMode) {
// Edit
if (currentTheme?.id) {
- const update_id = currentTheme.id;
- delete currentTheme.id;
- delete currentTheme.created_by;
- delete currentTheme.changed_by;
- delete currentTheme.changed_on_delta_humanized;
-
- updateResource(update_id, currentTheme).then(response => {
- if (!response) {
- return;
- }
+ const themeData = omit(currentTheme, [
+ 'id',
+ 'created_by',
+ 'changed_by',
+ 'changed_on_delta_humanized',
+ ]);
- if (onThemeAdd) {
- onThemeAdd();
- }
+ updateResource(currentTheme.id, themeData).then(response => {
+ if (!response) return;
+ if (onThemeAdd) onThemeAdd();
hide();
});
@@ -160,69 +178,115 @@ const ThemeModal: FunctionComponent<ThemeModalProps> = ({
} else if (currentTheme) {
// Create
createResource(currentTheme).then(response => {
- if (!response) {
- return;
- }
-
- if (onThemeAdd) {
- onThemeAdd();
- }
+ if (!response) return;
+ if (onThemeAdd) onThemeAdd();
hide();
});
}
- };
+ }, [
+ currentTheme,
+ isEditMode,
+ updateResource,
+ createResource,
+ onThemeAdd,
+ hide,
+ ]);
- const onApply = () => {
+ const handleCancel = useCallback(() => {
+ if (hasUnsavedChanges()) {
+ setShowConfirmAlert(true);
+ } else {
+ hide();
+ }
+ }, [hasUnsavedChanges, hide]);
+
+ const handleConfirmCancel = useCallback(() => {
+ hide();
+ }, [hide]);
+
+ const isValidJson = useCallback((str?: string) => {
+ if (!str) return false;
+ try {
+ JSON.parse(str);
+ return true;
+ } catch (e) {
+ return false;
+ }
+ }, []);
+
+ const onApply = useCallback(() => {
if (currentTheme?.json_data && isValidJson(currentTheme.json_data)) {
try {
const themeConfig = JSON.parse(currentTheme.json_data);
+
setTemporaryTheme(themeConfig);
- if (onThemeApply) {
- onThemeApply();
- }
- if (addSuccessToast) {
- addSuccessToast(t('Local theme set for preview'));
- }
+
+ if (onThemeApply) onThemeApply();
+ if (addSuccessToast) addSuccessToast(t('Local theme set for preview'));
} catch (error) {
addDangerToast(t('Failed to apply theme: Invalid JSON'));
}
}
- };
-
- const onThemeNameChange = (event: ChangeEvent<HTMLInputElement>) => {
- const { target } = event;
-
- const data = {
- ...currentTheme,
- theme_name: currentTheme ? currentTheme.theme_name : '',
- json_data: currentTheme ? currentTheme.json_data : '',
- };
-
- data[target.name as ThemeStringKeys] = target.value;
- setCurrentTheme(data);
- };
-
- const onJsonDataChange = (jsonData: string) => {
- const data = {
- ...currentTheme,
- theme_name: currentTheme ? currentTheme.theme_name : '',
- json_data: jsonData,
- };
- setCurrentTheme(data);
- };
-
- const isValidJson = (str?: string) => {
- if (!str) return false;
- try {
- JSON.parse(str);
- return true;
- } catch (e) {
- return false;
+ }, [
+ currentTheme?.json_data,
+ isValidJson,
+ setTemporaryTheme,
+ onThemeApply,
+ addSuccessToast,
+ addDangerToast,
+ ]);
+
+ const modalTitle = useMemo(() => {
+ if (isEditMode) {
+ return isReadOnly
+ ? t('View theme properties')
+ : t('Edit theme properties');
}
- };
+ return t('Add theme');
+ }, [isEditMode, isReadOnly]);
+
+ const modalIcon = useMemo(() => {
+ const Icon = isEditMode ? Icons.EditOutlined : Icons.PlusOutlined;
+ return (
+ <Icon
+ iconSize="l"
+ css={css`
+ margin: auto ${supersetTheme.sizeUnit * 2}px auto 0;
+ `}
+ />
+ );
+ }, [isEditMode, supersetTheme.sizeUnit]);
+
+ const onThemeNameChange = useCallback(
+ (event: ChangeEvent<HTMLInputElement>) => {
+ const { target } = event;
+
+ const data = {
+ ...currentTheme,
+ theme_name: currentTheme?.theme_name || '',
+ json_data: currentTheme?.json_data || '',
+ };
+
+ data[target.name as ThemeStringKeys] = target.value;
+ setCurrentTheme(data);
+ },
+ [currentTheme],
+ );
+
+ const onJsonDataChange = useCallback(
+ (jsonData: string) => {
+ const data = {
+ ...currentTheme,
+ theme_name: currentTheme?.theme_name || '',
+ json_data: jsonData,
+ };
+ setCurrentTheme(data);
+ },
+ [currentTheme],
+ );
- const validate = () => {
+ const validate = useCallback(() => {
if (isReadOnly) {
setDisableSave(true);
return;
@@ -237,7 +301,12 @@ const ThemeModal: FunctionComponent<ThemeModalProps> = ({
} else {
setDisableSave(true);
}
- };
+ }, [
+ currentTheme?.theme_name,
+ currentTheme?.json_data,
+ isReadOnly,
+ isValidJson,
+ ]);
// Initialize
useEffect(() => {
@@ -247,87 +316,115 @@ const ThemeModal: FunctionComponent<ThemeModalProps> = ({
(theme && theme?.id !== currentTheme.id) ||
(isHidden && show))
) {
- if (theme?.id && !loading) {
- fetchResource(theme.id);
- }
+ if (theme?.id && !loading) fetchResource(theme.id);
} else if (
!isEditMode &&
(!currentTheme || currentTheme.id || (isHidden && show))
) {
- setCurrentTheme({
+ const newTheme = {
theme_name: '',
json_data: JSON.stringify({}, null, 2),
- });
+ };
+ setCurrentTheme(newTheme);
+ setInitialTheme(newTheme);
}
- }, [theme, show]);
+ }, [theme, show, isEditMode, currentTheme, isHidden, loading,
fetchResource]);
useEffect(() => {
if (resource) {
setCurrentTheme(resource);
+ setInitialTheme(resource);
}
}, [resource]);
// Validation
useEffect(() => {
validate();
- }, [
- currentTheme ? currentTheme.theme_name : '',
- currentTheme ? currentTheme.json_data : '',
- isReadOnly,
- ]);
+ }, [validate]);
// Show/hide
- if (isHidden && show) {
- setIsHidden(false);
- }
+ useEffect(() => {
+ if (isHidden && show) setIsHidden(false);
+ }, [isHidden, show]);
+
+ // Handle browser navigation/reload with unsaved changes
+ useBeforeUnload(show && hasUnsavedChanges());
return (
<Modal
disablePrimaryButton={isReadOnly || disableSave}
onHandledPrimaryAction={isReadOnly ? undefined : onSave}
- onHide={hide}
+ onHide={handleCancel}
primaryButtonName={isEditMode ? t('Save') : t('Add')}
show={show}
width="55%"
- footer={[
- <Button key="cancel" onClick={hide} buttonStyle="secondary">
- {isReadOnly ? t('Close') : t('Cancel')}
- </Button>,
- ...(!isReadOnly
- ? [
- <Button
- key="save"
- onClick={onSave}
- disabled={disableSave}
- buttonStyle="primary"
- >
- {isEditMode ? t('Save') : t('Add')}
- </Button>,
- ]
- : []),
- ]}
+ centered
+ footer={
+ showConfirmAlert ? (
+ <Alert
+ closable={false}
+ type="warning"
+ message={t('You have unsaved changes')}
+ description={t(
+ 'Your changes will be lost if you leave without saving.',
+ )}
+ css={{
+ textAlign: 'left',
+ }}
+ action={
+ <Space>
+ <Button
+ key="keep-editing"
+ buttonStyle="tertiary"
+ onClick={() => setShowConfirmAlert(false)}
+ >
+ {t('Keep editing')}
+ </Button>
+ <Button
+ key="discard"
+ buttonStyle="secondary"
+ onClick={handleConfirmCancel}
+ >
+ {t('Discard')}
+ </Button>
+ <Button
+ key="save"
+ buttonStyle="primary"
+ onClick={() => {
+ setShowConfirmAlert(false);
+ onSave();
+ }}
+ disabled={disableSave}
+ >
+ {t('Save')}
+ </Button>
+ </Space>
+ }
+ />
+ ) : (
+ [
+ <Button key="cancel" onClick={handleCancel}
buttonStyle="secondary">
+ {isReadOnly ? t('Close') : t('Cancel')}
+ </Button>,
+ ...(!isReadOnly
+ ? [
+ <Button
+ key="save"
+ onClick={onSave}
+ disabled={disableSave}
+ buttonStyle="primary"
+ >
+ {isEditMode ? t('Save') : t('Add')}
+ </Button>,
+ ]
+ : []),
+ ]
+ )
+ }
title={
<Typography.Title level={4} data-test="theme-modal-title">
- {isEditMode ? (
- <Icons.EditOutlined
- iconSize="l"
- css={css`
- margin: auto ${supersetTheme.sizeUnit * 2}px auto 0;
- `}
- />
- ) : (
- <Icons.PlusOutlined
- iconSize="l"
- css={css`
- margin: auto ${supersetTheme.sizeUnit * 2}px auto 0;
- `}
- />
- )}
- {isEditMode
- ? isReadOnly
- ? t('View theme properties')
- : t('Edit theme properties')
- : t('Add theme')}
+ {modalIcon}
+ {modalTitle}
</Typography.Title>
}
>
@@ -385,7 +482,7 @@ const ThemeModal: FunctionComponent<ThemeModalProps> = ({
onChange={onJsonDataChange}
tabSize={2}
width="100%"
- height="300px"
+ height="250px"
wrapEnabled
readOnly={isReadOnly}
showGutter
diff --git a/superset-frontend/src/hooks/useBeforeUnload/index.ts
b/superset-frontend/src/hooks/useBeforeUnload/index.ts
new file mode 100644
index 0000000000..1befb3b892
--- /dev/null
+++ b/superset-frontend/src/hooks/useBeforeUnload/index.ts
@@ -0,0 +1,43 @@
+/**
+ * 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 { useEffect } from 'react';
+
+/**
+ * Custom hook to handle browser navigation/reload with unsaved changes
+ * @param shouldWarn - Boolean indicating if there are unsaved changes
+ * @param message - Optional custom message (most browsers ignore this and
show their own)
+ */
+export const useBeforeUnload = (
+ shouldWarn: boolean,
+ message?: string,
+): void => {
+ useEffect(() => {
+ const handleBeforeUnload = (event: BeforeUnloadEvent) => {
+ if (!shouldWarn) return;
+
+ event.preventDefault();
+ // Most browsers require returnValue to be set, even though they ignore
custom messages
+ // eslint-disable-next-line no-param-reassign
+ event.returnValue = message || '';
+ };
+
+ window.addEventListener('beforeunload', handleBeforeUnload);
+ return () => window.removeEventListener('beforeunload',
handleBeforeUnload);
+ }, [shouldWarn, message]);
+};
diff --git
a/superset-frontend/src/hooks/useBeforeUnload/useBeforeUnload.test.ts
b/superset-frontend/src/hooks/useBeforeUnload/useBeforeUnload.test.ts
new file mode 100644
index 0000000000..c11abec297
--- /dev/null
+++ b/superset-frontend/src/hooks/useBeforeUnload/useBeforeUnload.test.ts
@@ -0,0 +1,156 @@
+/**
+ * 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 { renderHook } from '@testing-library/react-hooks';
+import { useBeforeUnload } from './index';
+
+function createMockEvent() {
+ return {
+ preventDefault: jest.fn(),
+ returnValue: undefined as string | undefined,
+ } as unknown as BeforeUnloadEvent;
+}
+
+let addEventListenerSpy: jest.SpyInstance;
+let removeEventListenerSpy: jest.SpyInstance;
+let getMockHandler: () => (e: BeforeUnloadEvent) => void;
+let handlers: Array<(e: BeforeUnloadEvent) => void>;
+
+beforeEach(() => {
+ jest.clearAllMocks();
+ jest.restoreAllMocks();
+
+ handlers = [];
+
+ addEventListenerSpy = jest
+ .spyOn(window, 'addEventListener')
+ .mockImplementation((type, handler) => {
+ if (type === 'beforeunload') {
+ handlers.push(handler as (e: BeforeUnloadEvent) => void);
+ }
+ });
+
+ removeEventListenerSpy = jest.spyOn(window, 'removeEventListener');
+
+ getMockHandler = () => handlers[handlers.length - 1];
+});
+
+test('should add event listener when shouldWarn is true', () => {
+ renderHook(() => useBeforeUnload(true));
+
+ expect(addEventListenerSpy).toHaveBeenCalledWith(
+ 'beforeunload',
+ expect.any(Function),
+ );
+});
+
+test('should not prevent default when shouldWarn is false', () => {
+ renderHook(() => useBeforeUnload(false));
+
+ const event = createMockEvent();
+ const handler = getMockHandler();
+ handler(event);
+
+ expect(event.preventDefault).not.toHaveBeenCalled();
+ expect(event.returnValue).toBeUndefined();
+});
+
+test('should prevent default and set returnValue when shouldWarn is true', ()
=> {
+ renderHook(() => useBeforeUnload(true));
+
+ const event = createMockEvent();
+ const handler = getMockHandler();
+ handler(event);
+
+ expect(event.preventDefault).toHaveBeenCalled();
+ expect(event.returnValue).toBe('');
+});
+
+test('should use custom message when provided', () => {
+ const customMessage = 'You have unsaved changes!';
+ renderHook(() => useBeforeUnload(true, customMessage));
+
+ const event = createMockEvent();
+ const handler = getMockHandler();
+ handler(event);
+
+ expect(event.preventDefault).toHaveBeenCalled();
+ expect(event.returnValue).toBe(customMessage);
+});
+
+test('should remove event listener on unmount', () => {
+ const { unmount } = renderHook(() => useBeforeUnload(true));
+
+ unmount();
+
+ expect(removeEventListenerSpy).toHaveBeenCalledWith(
+ 'beforeunload',
+ expect.any(Function),
+ );
+});
+
+test('should update handler when shouldWarn changes', () => {
+ const { rerender } = renderHook(
+ ({ shouldWarn }) => useBeforeUnload(shouldWarn),
+ {
+ initialProps: { shouldWarn: false },
+ },
+ );
+
+ const event = createMockEvent();
+
+ const initialHandler = getMockHandler();
+ initialHandler(event);
+ expect(event.preventDefault).not.toHaveBeenCalled();
+
+ (event.preventDefault as jest.Mock).mockClear();
+ event.returnValue = undefined;
+
+ const initialAddCalls = addEventListenerSpy.mock.calls.length;
+
+ rerender({ shouldWarn: true });
+
+ expect(removeEventListenerSpy).toHaveBeenCalled();
+ expect(addEventListenerSpy.mock.calls.length).toBeGreaterThan(
+ initialAddCalls,
+ );
+
+ const newHandler = getMockHandler();
+ newHandler(event);
+ expect(event.preventDefault).toHaveBeenCalled();
+ expect(event.returnValue).toBe('');
+});
+
+test('should handle multiple instances independently', () => {
+ const { unmount: unmount1 } = renderHook(() => useBeforeUnload(true));
+ const { unmount: unmount2 } = renderHook(() => useBeforeUnload(false));
+
+ const beforeunloadCalls = addEventListenerSpy.mock.calls.filter(
+ call => call[0] === 'beforeunload',
+ );
+ expect(beforeunloadCalls.length).toBeGreaterThanOrEqual(2);
+
+ unmount1();
+ expect(removeEventListenerSpy).toHaveBeenCalled();
+
+ const removalCount = removeEventListenerSpy.mock.calls.length;
+ unmount2();
+ expect(removeEventListenerSpy.mock.calls.length).toBeGreaterThan(
+ removalCount,
+ );
+});
diff --git a/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts
b/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts
index 172e389c24..b79523b72f 100644
--- a/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts
+++ b/superset-frontend/src/hooks/useUnsavedChangesPrompt/index.ts
@@ -19,6 +19,7 @@
import { getClientErrorObject, t } from '@superset-ui/core';
import { useEffect, useRef, useCallback, useState } from 'react';
import { useHistory } from 'react-router-dom';
+import { useBeforeUnload } from 'src/hooks/useBeforeUnload';
type UseUnsavedChangesPromptProps = {
hasUnsavedChanges: boolean;
@@ -94,20 +95,6 @@ export const useUnsavedChangesPrompt = ({
return () => unblock();
}, [blockCallback, hasUnsavedChanges, history]);
- useEffect(() => {
- const handleBeforeUnload = (event: BeforeUnloadEvent) => {
- if (!hasUnsavedChanges) return;
- event.preventDefault();
-
- // Most browsers require a "returnValue" set to empty string
- const evt = event as any;
- evt.returnValue = '';
- };
-
- window.addEventListener('beforeunload', handleBeforeUnload);
- return () => window.removeEventListener('beforeunload',
handleBeforeUnload);
- }, [hasUnsavedChanges]);
-
useEffect(() => {
if (!isSaveModalVisible && manualSaveRef.current) {
setShowModal(false);
@@ -115,6 +102,8 @@ export const useUnsavedChangesPrompt = ({
}
}, [isSaveModalVisible]);
+ useBeforeUnload(hasUnsavedChanges);
+
return {
showModal,
setShowModal,
