diff --git a/.gitignore b/.gitignore index def7322c649..fce133adee4 100644 --- a/.gitignore +++ b/.gitignore @@ -132,3 +132,5 @@ superset/static/stats/statistics.html CLAUDE.local.md .aider* .claude_rc* +.env.local +PROJECT.md diff --git a/docs/docs/configuration/theming.mdx b/docs/docs/configuration/theming.mdx index 5064e2e87c9..13ee29d39fe 100644 --- a/docs/docs/configuration/theming.mdx +++ b/docs/docs/configuration/theming.mdx @@ -13,9 +13,9 @@ apache-superset>=6.0 Superset now rides on **Ant Design v5's token-based theming**. Every Antd token works, plus a handful of Superset-specific ones for charts and dashboard chrome. -## Managing Themes via CRUD Interface +## Managing Themes via UI -Superset now includes a built-in **Theme Management** interface accessible from the admin menu under **Settings > Themes**. +Superset includes a built-in **Theme Management** interface accessible from the admin menu under **Settings > Themes**. ### Creating a New Theme @@ -29,22 +29,38 @@ Superset now includes a built-in **Theme Management** interface accessible from You can also extend with Superset-specific tokens (documented in the default theme object) before you import. +### System Theme Administration + +When `ENABLE_UI_THEME_ADMINISTRATION = True` is configured, administrators can manage system-wide themes directly from the UI: + +#### Setting System Themes +- **System Default Theme**: Click the sun icon on any theme to set it as the system-wide default +- **System Dark Theme**: Click the moon icon on any theme to set it as the system dark mode theme +- **Automatic OS Detection**: When both default and dark themes are set, Superset automatically detects and applies the appropriate theme based on OS preferences + +#### Managing System Themes +- System themes are indicated with special badges in the theme list +- Only administrators with write permissions can modify system theme settings +- Removing a system theme designation reverts to configuration file defaults + ### Applying Themes to Dashboards Once created, themes can be applied to individual dashboards: - Edit any dashboard and select your custom theme from the theme dropdown - Each dashboard can have its own theme, allowing for branded or context-specific styling -## Alternative: Instance-wide Configuration +## Configuration Options -For system-wide theming, you can configure default themes via Python configuration: +### Python Configuration -### Setting Default Themes +Configure theme behavior via `superset_config.py`: ```python -# superset_config.py +# Enable UI-based theme administration for admins +ENABLE_UI_THEME_ADMINISTRATION = True -# Default theme (light mode) +# Optional: Set initial default themes via configuration +# These can be overridden via the UI when ENABLE_UI_THEME_ADMINISTRATION = True THEME_DEFAULT = { "token": { "colorPrimary": "#2893B3", @@ -53,7 +69,7 @@ THEME_DEFAULT = { } } -# Dark theme configuration +# Optional: Dark theme configuration THEME_DARK = { "algorithm": "dark", "token": { @@ -62,23 +78,28 @@ THEME_DARK = { } } -# Theme behavior settings -THEME_SETTINGS = { - "enforced": False, # If True, forces default theme always - "allowSwitching": True, # Allow users to switch between themes - "allowOSPreference": True, # Auto-detect system theme preference -} +# To force a single theme on all users, set THEME_DARK = None +# When both themes are defined (via UI or config): +# - Users can manually switch between themes +# - OS preference detection is automatically enabled ``` -### Copying Themes from CRUD Interface +### Migration from Configuration to UI -To use a theme created via the CRUD interface as your system default: +When `ENABLE_UI_THEME_ADMINISTRATION = True`: -1. Navigate to **Settings > Themes** and edit your desired theme -2. Copy the complete JSON configuration from the theme definition field -3. Paste it directly into your `superset_config.py` as shown above +1. System themes set via the UI take precedence over configuration file settings +2. The UI shows which themes are currently set as system defaults +3. Administrators can change system themes without restarting Superset +4. Configuration file themes serve as fallbacks when no UI themes are set -Restart Superset to apply changes. +### Copying Themes Between Systems + +To export a theme for use in configuration files or another instance: + +1. Navigate to **Settings > Themes** and click the export icon on your desired theme +2. Extract the JSON configuration from the exported YAML file +3. Use this JSON in your `superset_config.py` or import it into another Superset instance ## Theme Development Workflow @@ -146,7 +167,26 @@ This feature works with the stock Docker image - no custom build required! ## Advanced Features -- **System Themes**: Superset includes built-in light and dark themes +- **System Themes**: Manage system-wide default and dark themes via UI or configuration - **Per-Dashboard Theming**: Each dashboard can have its own visual identity - **JSON Editor**: Edit theme configurations directly within Superset's interface - **Custom Fonts**: Load external fonts via configuration without rebuilding +- **OS Dark Mode Detection**: Automatically switches themes based on system preferences +- **Theme Import/Export**: Share themes between instances via YAML files + +## API Access + +For programmatic theme management, Superset provides REST endpoints: + +- `GET /api/v1/theme/` - List all themes +- `POST /api/v1/theme/` - Create a new theme +- `PUT /api/v1/theme/{id}` - Update a theme +- `DELETE /api/v1/theme/{id}` - Delete a theme +- `PUT /api/v1/theme/{id}/set_system_default` - Set as system default theme (admin only) +- `PUT /api/v1/theme/{id}/set_system_dark` - Set as system dark theme (admin only) +- `DELETE /api/v1/theme/unset_system_default` - Remove system default designation +- `DELETE /api/v1/theme/unset_system_dark` - Remove system dark designation +- `GET /api/v1/theme/export/` - Export themes as YAML +- `POST /api/v1/theme/import/` - Import themes from YAML + +These endpoints require appropriate permissions and are subject to RBAC controls. diff --git a/superset-frontend/packages/superset-ui-core/src/theme/types.ts b/superset-frontend/packages/superset-ui-core/src/theme/types.ts index b3cf0ae4112..ec260dafc91 100644 --- a/superset-frontend/packages/superset-ui-core/src/theme/types.ts +++ b/superset-frontend/packages/superset-ui-core/src/theme/types.ts @@ -431,14 +431,9 @@ export interface ThemeContextType { } /** - * Configuration object for complete theme setup including default, dark themes and settings + * Configuration object for complete theme setup including default and dark themes */ export interface SupersetThemeConfig { theme_default: AnyThemeConfig; theme_dark?: AnyThemeConfig; - theme_settings?: { - enforced?: boolean; - allowSwitching?: boolean; - allowOSPreference?: boolean; - }; } diff --git a/superset-frontend/src/constants.ts b/superset-frontend/src/constants.ts index e5293324472..4ea7c276d93 100644 --- a/superset-frontend/src/constants.ts +++ b/superset-frontend/src/constants.ts @@ -143,7 +143,6 @@ export const DEFAULT_COMMON_BOOTSTRAP_DATA: CommonBootstrapData = { theme: { default: {}, dark: {}, - settings: {}, }, menu_data: { menu: [], diff --git a/superset-frontend/src/features/themes/api.test.ts b/superset-frontend/src/features/themes/api.test.ts new file mode 100644 index 00000000000..6545705e24e --- /dev/null +++ b/superset-frontend/src/features/themes/api.test.ts @@ -0,0 +1,122 @@ +/** + * 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 fetchMock from 'fetch-mock'; +import { + setSystemDefaultTheme, + setSystemDarkTheme, + unsetSystemDefaultTheme, + unsetSystemDarkTheme, +} from './api'; + +describe('Theme API', () => { + beforeEach(() => { + fetchMock.reset(); + }); + + afterEach(() => { + fetchMock.restore(); + }); + + describe('setSystemDefaultTheme', () => { + it('should call the correct endpoint with theme id', async () => { + const mockResponse = { id: 1, result: 'success' }; + fetchMock.put('glob:*/api/v1/theme/1/set_system_default', mockResponse); + + await setSystemDefaultTheme(1); + + expect(fetchMock.called('glob:*/api/v1/theme/1/set_system_default')).toBe( + true, + ); + }); + + it('should handle errors properly', async () => { + fetchMock.put('glob:*/api/v1/theme/1/set_system_default', { + throws: new Error('API Error'), + }); + + await expect(setSystemDefaultTheme(1)).rejects.toThrow('API Error'); + }); + }); + + describe('setSystemDarkTheme', () => { + it('should call the correct endpoint with theme id', async () => { + const mockResponse = { id: 2, result: 'success' }; + fetchMock.put('glob:*/api/v1/theme/2/set_system_dark', mockResponse); + + await setSystemDarkTheme(2); + + expect(fetchMock.called('glob:*/api/v1/theme/2/set_system_dark')).toBe( + true, + ); + }); + + it('should handle errors properly', async () => { + fetchMock.put('glob:*/api/v1/theme/2/set_system_dark', { + throws: new Error('API Error'), + }); + + await expect(setSystemDarkTheme(2)).rejects.toThrow('API Error'); + }); + }); + + describe('unsetSystemDefaultTheme', () => { + it('should call the correct endpoint', async () => { + const mockResponse = { result: 'success' }; + fetchMock.delete( + 'glob:*/api/v1/theme/unset_system_default', + mockResponse, + ); + + await unsetSystemDefaultTheme(); + + expect(fetchMock.called('glob:*/api/v1/theme/unset_system_default')).toBe( + true, + ); + }); + + it('should handle errors properly', async () => { + fetchMock.delete('glob:*/api/v1/theme/unset_system_default', { + throws: new Error('API Error'), + }); + + await expect(unsetSystemDefaultTheme()).rejects.toThrow('API Error'); + }); + }); + + describe('unsetSystemDarkTheme', () => { + it('should call the correct endpoint', async () => { + const mockResponse = { result: 'success' }; + fetchMock.delete('glob:*/api/v1/theme/unset_system_dark', mockResponse); + + await unsetSystemDarkTheme(); + + expect(fetchMock.called('glob:*/api/v1/theme/unset_system_dark')).toBe( + true, + ); + }); + + it('should handle errors properly', async () => { + fetchMock.delete('glob:*/api/v1/theme/unset_system_dark', { + throws: new Error('API Error'), + }); + + await expect(unsetSystemDarkTheme()).rejects.toThrow('API Error'); + }); + }); +}); diff --git a/superset-frontend/src/features/themes/api.ts b/superset-frontend/src/features/themes/api.ts new file mode 100644 index 00000000000..2392af2c861 --- /dev/null +++ b/superset-frontend/src/features/themes/api.ts @@ -0,0 +1,39 @@ +/** + * 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 { SupersetClient } from '@superset-ui/core'; + +export const setSystemDefaultTheme = (themeId: number) => + SupersetClient.put({ + endpoint: `/api/v1/theme/${themeId}/set_system_default`, + }); + +export const setSystemDarkTheme = (themeId: number) => + SupersetClient.put({ + endpoint: `/api/v1/theme/${themeId}/set_system_dark`, + }); + +export const unsetSystemDefaultTheme = () => + SupersetClient.delete({ + endpoint: `/api/v1/theme/unset_system_default`, + }); + +export const unsetSystemDarkTheme = () => + SupersetClient.delete({ + endpoint: `/api/v1/theme/unset_system_dark`, + }); diff --git a/superset-frontend/src/features/themes/types.test.ts b/superset-frontend/src/features/themes/types.test.ts new file mode 100644 index 00000000000..cc80b6f709a --- /dev/null +++ b/superset-frontend/src/features/themes/types.test.ts @@ -0,0 +1,167 @@ +/** + * 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 { ThemeObject } from './types'; + +describe('Theme Types', () => { + describe('ThemeObject', () => { + it('should accept valid theme objects', () => { + const validTheme: ThemeObject = { + id: 1, + theme_name: 'Test Theme', + json_data: '{"colors": {"primary": "#1890ff"}}', + uuid: '123e4567-e89b-12d3-a456-426614174000', + is_system: false, + is_system_default: false, + is_system_dark: false, + changed_on_delta_humanized: '1 day ago', + created_on: '2023-01-01T00:00:00Z', + changed_by: { + id: 1, + first_name: 'John', + last_name: 'Doe', + }, + created_by: { + id: 2, + first_name: 'Jane', + last_name: 'Smith', + }, + }; + + // TypeScript will ensure this compiles + expect(validTheme).toBeDefined(); + expect(validTheme.is_system).toBe(false); + expect(validTheme.is_system_default).toBe(false); + expect(validTheme.is_system_dark).toBe(false); + }); + + it('should handle optional fields', () => { + const minimalTheme: ThemeObject = { + theme_name: 'Minimal Theme', + json_data: '{}', + }; + + // TypeScript allows optional fields + expect(minimalTheme.id).toBeUndefined(); + expect(minimalTheme.is_system).toBeUndefined(); + expect(minimalTheme.is_system_default).toBeUndefined(); + expect(minimalTheme.is_system_dark).toBeUndefined(); + }); + + it('should handle system theme fields correctly', () => { + const systemTheme: ThemeObject = { + id: 2, + theme_name: 'System Theme', + json_data: '{"colors": {"primary": "#000000"}}', + is_system: true, + is_system_default: true, + is_system_dark: false, + }; + + expect(systemTheme.is_system).toBe(true); + expect(systemTheme.is_system_default).toBe(true); + expect(systemTheme.is_system_dark).toBe(false); + }); + + it('should handle both system default and dark flags', () => { + // This should not happen in practice (a theme shouldn't be both) + // but the type allows it for flexibility + const theme: ThemeObject = { + id: 3, + theme_name: 'Dual System Theme', + json_data: '{}', + is_system: true, + is_system_default: true, + is_system_dark: true, + }; + + expect(theme.is_system_default).toBe(true); + expect(theme.is_system_dark).toBe(true); + }); + }); + + describe('Theme JSON Data', () => { + it('should parse valid JSON data', () => { + const theme: ThemeObject = { + theme_name: 'Parse Test', + json_data: '{"colors": {"primary": "#1890ff", "secondary": "#52c41a"}}', + }; + + const parsed = JSON.parse(theme.json_data!); + expect(parsed.colors.primary).toBe('#1890ff'); + expect(parsed.colors.secondary).toBe('#52c41a'); + }); + + it('should handle empty JSON', () => { + const theme: ThemeObject = { + theme_name: 'Empty Theme', + json_data: '{}', + }; + + const parsed = JSON.parse(theme.json_data!); + expect(parsed).toEqual({}); + }); + }); + + describe('Type Guards', () => { + it('should identify system themes', () => { + const isSystemTheme = (theme: ThemeObject): boolean => + theme.is_system === true; + + const systemTheme: ThemeObject = { + theme_name: 'System', + json_data: '{}', + is_system: true, + }; + + const userTheme: ThemeObject = { + theme_name: 'User', + json_data: '{}', + is_system: false, + }; + + expect(isSystemTheme(systemTheme)).toBe(true); + expect(isSystemTheme(userTheme)).toBe(false); + }); + + it('should identify deletable themes', () => { + const isDeletable = (theme: ThemeObject): boolean => + !theme.is_system && !theme.is_system_default && !theme.is_system_dark; + + const deletableTheme: ThemeObject = { + theme_name: 'Deletable', + json_data: '{}', + is_system: false, + is_system_default: false, + is_system_dark: false, + }; + + const systemDefaultTheme: ThemeObject = { + theme_name: 'System Default', + json_data: '{}', + is_system: false, + is_system_default: true, + is_system_dark: false, + }; + + expect(isDeletable(deletableTheme)).toBe(true); + expect(isDeletable(systemDefaultTheme)).toBe(false); + }); + }); +}); diff --git a/superset-frontend/src/features/themes/types.ts b/superset-frontend/src/features/themes/types.ts index 3f4b3417ef3..87d1df98b2b 100644 --- a/superset-frontend/src/features/themes/types.ts +++ b/superset-frontend/src/features/themes/types.ts @@ -22,6 +22,8 @@ export type ThemeObject = { id?: number; uuid?: string; is_system?: boolean; + is_system_default?: boolean; + is_system_dark?: boolean; changed_on_delta_humanized?: string; created_on?: string; changed_by?: Owner; diff --git a/superset-frontend/src/pages/ThemeList/index.test.tsx b/superset-frontend/src/pages/ThemeList/index.test.tsx new file mode 100644 index 00000000000..f495c9596ac --- /dev/null +++ b/superset-frontend/src/pages/ThemeList/index.test.tsx @@ -0,0 +1,287 @@ +/** + * 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, waitFor } from 'spec/helpers/testing-library'; +import fetchMock from 'fetch-mock'; +import * as hooks from 'src/views/CRUD/hooks'; +import * as themeApi from 'src/features/themes/api'; +import * as getBootstrapData from 'src/utils/getBootstrapData'; +import ThemesList from './index'; + +// Mock the getBootstrapData function +jest.mock('src/utils/getBootstrapData', () => ({ + __esModule: true, + default: () => ({ + common: { + theme: { + enableUiThemeAdministration: true, + }, + }, + user: { + userId: 1, + username: 'admin', + roles: { + Admin: [['can_write', 'Theme']], + }, + }, + }), +})); + +// Mock theme API functions +jest.mock('src/features/themes/api', () => ({ + setSystemDefaultTheme: jest.fn(() => Promise.resolve()), + setSystemDarkTheme: jest.fn(() => Promise.resolve()), + unsetSystemDefaultTheme: jest.fn(() => Promise.resolve()), + unsetSystemDarkTheme: jest.fn(() => Promise.resolve()), +})); + +// Mock the CRUD hooks +jest.mock('src/views/CRUD/hooks', () => ({ + ...jest.requireActual('src/views/CRUD/hooks'), + useListViewResource: jest.fn(), +})); + +// Mock the useThemeContext hook +jest.mock('src/theme/ThemeProvider', () => ({ + ...jest.requireActual('src/theme/ThemeProvider'), + useThemeContext: jest.fn().mockReturnValue({ + getCurrentCrudThemeId: jest.fn().mockReturnValue('1'), + appliedTheme: { theme_name: 'Light Theme', id: 1 }, + }), +})); + +const mockThemes = [ + { + id: 1, + theme_name: 'Light Theme', + is_system_default: true, + is_system_dark: false, + created_by: { id: 1, first_name: 'Admin', last_name: 'User' }, + changed_on_delta_humanized: '1 day ago', + }, + { + id: 2, + theme_name: 'Dark Theme', + is_system_default: false, + is_system_dark: true, + created_by: { id: 1, first_name: 'Admin', last_name: 'User' }, + changed_on_delta_humanized: '2 days ago', + }, + { + id: 3, + theme_name: 'Custom Theme', + is_system_default: false, + is_system_dark: false, + created_by: { id: 2, first_name: 'Test', last_name: 'User' }, + changed_on_delta_humanized: '3 days ago', + }, +]; + +describe('ThemesList', () => { + beforeEach(() => { + // Mock the useListViewResource hook + (hooks.useListViewResource as jest.Mock).mockReturnValue({ + state: { + loading: false, + resourceCollection: mockThemes, + resourceCount: 3, + bulkSelectEnabled: false, + }, + setResourceCollection: jest.fn(), + hasPerm: jest.fn().mockReturnValue(true), + refreshData: jest.fn(), + fetchData: jest.fn(), + toggleBulkSelect: jest.fn(), + }); + + fetchMock.reset(); + fetchMock.get('glob:*/api/v1/theme/*', { + count: 3, + result: mockThemes, + }); + }); + + afterEach(() => { + fetchMock.restore(); + jest.clearAllMocks(); + }); + + it('renders the themes list with proper structure', async () => { + render( + , + { + useRedux: true, + useDnd: true, + useQueryParams: true, + }, + ); + + await waitFor(() => { + expect(screen.getByText('Light Theme')).toBeInTheDocument(); + expect(screen.getByText('Dark Theme')).toBeInTheDocument(); + expect(screen.getByText('Custom Theme')).toBeInTheDocument(); + }); + }); + + it('shows system theme badges for default and dark themes', async () => { + render( + , + { + useRedux: true, + useDnd: true, + useQueryParams: true, + }, + ); + + await waitFor(() => { + // Check for system badges + expect(screen.getByText('Light Theme')).toBeInTheDocument(); + expect(screen.getByText('Dark Theme')).toBeInTheDocument(); + }); + + // Verify that themes with system flags are marked appropriately + expect(mockThemes[0].is_system_default).toBe(true); + expect(mockThemes[1].is_system_dark).toBe(true); + expect(mockThemes[2].is_system_default).toBe(false); + expect(mockThemes[2].is_system_dark).toBe(false); + }); + + it('uses flat theme structure for enableUiThemeAdministration', () => { + // Verify the component accesses the correct bootstrap data structure + const { common } = getBootstrapData.default(); + + // Should access flat structure + expect(common.theme.enableUiThemeAdministration).toBe(true); + + // Should NOT have nested settings structure + expect((common.theme as any).settings).toBeUndefined(); + }); + + it('shows admin controls when user has proper permissions', async () => { + render( + , + { + useRedux: true, + useDnd: true, + useQueryParams: true, + }, + ); + + await waitFor(() => { + expect(screen.getByText('Light Theme')).toBeInTheDocument(); + }); + + // Admin controls should be visible + const adminElements = screen.queryAllByRole('button'); + expect(adminElements.length).toBeGreaterThan(0); + }); + + it('calls setSystemDefaultTheme API when setting a theme as default', async () => { + const { setSystemDefaultTheme } = themeApi; + const addSuccessToast = jest.fn(); + const refreshData = jest.fn(); + + (hooks.useListViewResource as jest.Mock).mockReturnValue({ + state: { + loading: false, + resourceCollection: mockThemes, + resourceCount: 3, + bulkSelectEnabled: false, + }, + setResourceCollection: jest.fn(), + hasPerm: jest.fn().mockReturnValue(true), + refreshData, + fetchData: jest.fn(), + toggleBulkSelect: jest.fn(), + }); + + render( + , + { + useRedux: true, + useDnd: true, + useQueryParams: true, + }, + ); + + await waitFor(() => { + expect(screen.getByText('Custom Theme')).toBeInTheDocument(); + }); + + // Since the actual UI interaction would be complex to test, + // we verify the API function exists and can be called + expect(setSystemDefaultTheme).toBeDefined(); + + // Test calling the API directly + await setSystemDefaultTheme(3); + expect(setSystemDefaultTheme).toHaveBeenCalledWith(3); + }); + + it('configures theme deletion endpoint', async () => { + const addDangerToast = jest.fn(); + const addSuccessToast = jest.fn(); + const refreshData = jest.fn(); + + // Setup delete mock before rendering + fetchMock.delete('glob:*/api/v1/theme/*', 200); + + // Mock the useListViewResource hook with refreshData + (hooks.useListViewResource as jest.Mock).mockReturnValue({ + state: { + loading: false, + resourceCollection: mockThemes, + resourceCount: 3, + bulkSelectEnabled: false, + }, + setResourceCollection: jest.fn(), + hasPerm: jest.fn().mockReturnValue(true), + refreshData, + fetchData: jest.fn(), + toggleBulkSelect: jest.fn(), + }); + + render( + , + { + useRedux: true, + useDnd: true, + useQueryParams: true, + }, + ); + + await waitFor(() => { + expect(screen.getByText('Custom Theme')).toBeInTheDocument(); + }); + + // Verify that themes are rendered correctly + expect(screen.getByText('Light Theme')).toBeInTheDocument(); + expect(screen.getByText('Dark Theme')).toBeInTheDocument(); + expect(screen.getByText('Custom Theme')).toBeInTheDocument(); + + // Verify that action buttons are present (which include delete for non-system themes) + const actionButtons = screen.getAllByRole('button'); + expect(actionButtons.length).toBeGreaterThan(0); + }); +}); diff --git a/superset-frontend/src/pages/ThemeList/index.tsx b/superset-frontend/src/pages/ThemeList/index.tsx index 2733926fd4e..8c570f46cf5 100644 --- a/superset-frontend/src/pages/ThemeList/index.tsx +++ b/superset-frontend/src/pages/ThemeList/index.tsx @@ -27,6 +27,7 @@ import { Alert, Tooltip, Space, + Modal, } from '@superset-ui/core/components'; import rison from 'rison'; @@ -36,6 +37,7 @@ import withToasts from 'src/components/MessageToasts/withToasts'; import { useThemeContext } from 'src/theme/ThemeProvider'; import SubMenu, { SubMenuProps } from 'src/features/home/SubMenu'; import handleResourceExport from 'src/utils/export'; +import getBootstrapData from 'src/utils/getBootstrapData'; import { ModifiedInfo, ListView, @@ -51,6 +53,12 @@ import ThemeModal from 'src/features/themes/ThemeModal'; import { ThemeObject } from 'src/features/themes/types'; import { QueryObjectColumns } from 'src/views/CRUD/types'; import { Icons } from '@superset-ui/core/components/Icons'; +import { + setSystemDefaultTheme, + setSystemDarkTheme, + unsetSystemDefaultTheme, + unsetSystemDarkTheme, +} from 'src/features/themes/api'; const PAGE_SIZE = 25; @@ -111,6 +119,13 @@ function ThemesList({ const canImport = hasPerm('can_write'); const canApply = hasPerm('can_write'); // Only users with write permission can apply themes + // Get theme settings from bootstrap data + const bootstrapData = getBootstrapData(); + const themeData = bootstrapData?.common?.theme || {}; + + const canSetSystemThemes = + canEdit && (themeData as any)?.enableUiThemeAdministration; + const [themeCurrentlyDeleting, setThemeCurrentlyDeleting] = useState(null); @@ -132,8 +147,11 @@ function ThemesList({ }; const handleBulkThemeDelete = (themesToDelete: ThemeObject[]) => { - // Filter out system themes from deletion - const deletableThemes = themesToDelete.filter(theme => !theme.is_system); + // Filter out system themes and themes that are set as system themes + const deletableThemes = themesToDelete.filter( + theme => + !theme.is_system && !theme.is_system_default && !theme.is_system_dark, + ); if (deletableThemes.length === 0) { addDangerToast(t('Cannot delete system themes')); @@ -216,6 +234,84 @@ function ThemesList({ addSuccessToast(t('Theme imported')); }; + // Generic confirmation modal utility to reduce code duplication + const showThemeConfirmation = (config: { + title: string; + content: string; + onConfirm: () => Promise; + successMessage: string; + errorMessage: string; + }) => { + Modal.confirm({ + title: config.title, + content: config.content, + onOk: () => { + config + .onConfirm() + .then(() => { + refreshData(); + addSuccessToast(config.successMessage); + }) + .catch(err => { + addDangerToast(t(config.errorMessage, err.message)); + }); + }, + }); + }; + + const handleSetSystemDefault = (theme: ThemeObject) => { + showThemeConfirmation({ + title: t('Set System Default Theme'), + content: t( + 'Are you sure you want to set "%s" as the system default theme? This will apply to all users who haven\'t set a personal preference.', + theme.theme_name, + ), + onConfirm: () => setSystemDefaultTheme(theme.id!), + successMessage: t( + '"%s" is now the system default theme', + theme.theme_name, + ), + errorMessage: 'Failed to set system default theme: %s', + }); + }; + + const handleSetSystemDark = (theme: ThemeObject) => { + showThemeConfirmation({ + title: t('Set System Dark Theme'), + content: t( + 'Are you sure you want to set "%s" as the system dark theme? This will apply to all users who haven\'t set a personal preference.', + theme.theme_name, + ), + onConfirm: () => setSystemDarkTheme(theme.id!), + successMessage: t('"%s" is now the system dark theme', theme.theme_name), + errorMessage: 'Failed to set system dark theme: %s', + }); + }; + + const handleUnsetSystemDefault = () => { + showThemeConfirmation({ + title: t('Remove System Default Theme'), + content: t( + 'Are you sure you want to remove the system default theme? The application will fall back to the configuration file default.', + ), + onConfirm: () => unsetSystemDefaultTheme(), + successMessage: t('System default theme removed'), + errorMessage: 'Failed to remove system default theme: %s', + }); + }; + + const handleUnsetSystemDark = () => { + showThemeConfirmation({ + title: t('Remove System Dark Theme'), + content: t( + 'Are you sure you want to remove the system dark theme? The application will fall back to the configuration file dark theme.', + ), + onConfirm: () => unsetSystemDarkTheme(), + successMessage: t('System dark theme removed'), + errorMessage: 'Failed to remove system dark theme: %s', + }); + }; + const initialSort = [{ id: 'theme_name', desc: true }]; const columns = useMemo( () => [ @@ -234,12 +330,26 @@ function ThemesList({ - {t('Local')} + {t('Local')} )} {original.is_system && ( - {t('System')} + {t('System')} + + )} + {original.is_system_default && ( + + + {t('Default')} + + + )} + {original.is_system_dark && ( + + + {t('Dark')} + )} @@ -267,7 +377,17 @@ function ThemesList({ { Cell: ({ row: { original } }: any) => { const handleEdit = () => handleThemeEdit(original); - const handleDelete = () => setThemeCurrentlyDeleting(original); + const handleDelete = () => { + if (original.is_system_default || original.is_system_dark) { + addDangerToast( + t( + 'Cannot delete theme that is set as system default or dark theme', + ), + ); + return; + } + setThemeCurrentlyDeleting(original); + }; const handleApply = () => handleThemeApply(original); const handleExport = () => handleBulkThemeExport([original]); @@ -303,6 +423,42 @@ function ThemesList({ onClick: handleExport, } : null, + canSetSystemThemes && !original.is_system_default + ? { + label: 'set-default-action', + tooltip: t('Set as system default theme'), + placement: 'bottom', + icon: 'SunOutlined', + onClick: () => handleSetSystemDefault(original), + } + : null, + canSetSystemThemes && original.is_system_default + ? { + label: 'unset-default-action', + tooltip: t('Remove as system default theme'), + placement: 'bottom', + icon: 'StopOutlined', + onClick: () => handleUnsetSystemDefault(), + } + : null, + canSetSystemThemes && !original.is_system_dark + ? { + label: 'set-dark-action', + tooltip: t('Set as system dark theme'), + placement: 'bottom', + icon: 'MoonOutlined', + onClick: () => handleSetSystemDark(original), + } + : null, + canSetSystemThemes && original.is_system_dark + ? { + label: 'unset-dark-action', + tooltip: t('Remove as system dark theme'), + placement: 'bottom', + icon: 'StopOutlined', + onClick: () => handleUnsetSystemDark(), + } + : null, canDelete && !original.is_system ? { label: 'delete-action', @@ -330,7 +486,14 @@ function ThemesList({ id: QueryObjectColumns.ChangedBy, }, ], - [canDelete, canCreate, canApply, canExport, appliedThemeId], + [ + canDelete, + canCreate, + canApply, + canExport, + canSetSystemThemes, + appliedThemeId, + ], ); const menuData: SubMenuProps = { diff --git a/superset-frontend/src/theme/ThemeController.ts b/superset-frontend/src/theme/ThemeController.ts index 5a1851e8f5f..a40dafca5e6 100644 --- a/superset-frontend/src/theme/ThemeController.ts +++ b/superset-frontend/src/theme/ThemeController.ts @@ -33,16 +33,9 @@ import { import type { BootstrapThemeData, BootstrapThemeDataConfig, - SerializableThemeSettings, } from 'src/types/bootstrapTypes'; import getBootstrapData from 'src/utils/getBootstrapData'; -const DEFAULT_THEME_SETTINGS = { - enforced: false, - allowSwitching: true, - allowOSPreference: true, -} as const; - const STORAGE_KEYS = { THEME_MODE: 'superset-theme-mode', CRUD_THEME_ID: 'superset-crud-theme-id', @@ -90,8 +83,6 @@ export class ThemeController { private darkTheme: AnyThemeConfig | null; - private themeSettings: SerializableThemeSettings; - private systemMode: ThemeMode.DARK | ThemeMode.DEFAULT; private currentMode: ThemeMode; @@ -128,18 +119,15 @@ export class ThemeController { const { bootstrapDefaultTheme, bootstrapDarkTheme, - bootstrapThemeSettings, hasCustomThemes, }: BootstrapThemeData = this.loadBootstrapData(); this.hasCustomThemes = hasCustomThemes; - this.themeSettings = bootstrapThemeSettings || {}; // Set themes based on bootstrap data availability if (this.hasCustomThemes) { - this.darkTheme = bootstrapDarkTheme || bootstrapDefaultTheme || null; - this.defaultTheme = - bootstrapDefaultTheme || bootstrapDarkTheme || defaultTheme; + this.darkTheme = bootstrapDarkTheme; + this.defaultTheme = bootstrapDefaultTheme || defaultTheme; } else { this.darkTheme = null; this.defaultTheme = defaultTheme; @@ -186,16 +174,18 @@ export class ThemeController { /** * Check if the user can update the theme. + * Always true now - theme enforcement is done via THEME_DARK = None */ public canSetTheme(): boolean { - return !this.themeSettings?.enforced; + return true; } /** * Check if the user can update the theme mode. + * Only possible if dark theme is available */ public canSetMode(): boolean { - return this.isModeUpdatable(); + return this.darkTheme !== null; } /** @@ -417,11 +407,10 @@ export class ThemeController { /** * Checks if OS preference detection is allowed. + * Allowed when both themes are available */ public canDetectOSPreference(): boolean { - const { allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference } = - this.themeSettings || {}; - return allowOSPreference === true; + return this.darkTheme !== null; } /** @@ -435,12 +424,6 @@ export class ThemeController { this.darkTheme = config.theme_dark || null; this.hasCustomThemes = true; - this.themeSettings = { - enforced: config.theme_settings?.enforced ?? false, - allowSwitching: config.theme_settings?.allowSwitching ?? true, - allowOSPreference: config.theme_settings?.allowOSPreference ?? true, - }; - let newMode: ThemeMode; try { this.validateModeUpdatePermission(this.currentMode); @@ -539,14 +522,11 @@ export class ThemeController { /** * Determines whether the MediaQueryList listener for system theme changes should be initialized. - * This checks if OS preference detection is enabled in the theme settings. + * This checks if both themes are available to enable OS preference detection. * @returns {boolean} True if the media query listener should be initialized, false otherwise */ private shouldInitializeMediaQueryListener(): boolean { - const { allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference } = - this.themeSettings || {}; - - return allowOSPreference === true; + return this.darkTheme !== null; } /** @@ -569,20 +549,14 @@ export class ThemeController { common: { theme = {} as BootstrapThemeDataConfig }, } = getBootstrapData(); - const { - default: defaultTheme, - dark: darkTheme, - settings: themeSettings, - } = theme; + const { default: defaultTheme, dark: darkTheme } = theme; const hasValidDefault: boolean = this.isNonEmptyObject(defaultTheme); const hasValidDark: boolean = this.isNonEmptyObject(darkTheme); - const hasValidSettings: boolean = this.isNonEmptyObject(themeSettings); return { bootstrapDefaultTheme: hasValidDefault ? defaultTheme : null, bootstrapDarkTheme: hasValidDark ? darkTheme : null, - bootstrapThemeSettings: hasValidSettings ? themeSettings : null, hasCustomThemes: hasValidDefault || hasValidDark, }; } @@ -598,18 +572,6 @@ export class ThemeController { ); } - /** - * Determines if mode updates are allowed. - */ - private isModeUpdatable(): boolean { - const { - enforced = DEFAULT_THEME_SETTINGS.enforced, - allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching, - } = this.themeSettings || {}; - - return !enforced && allowSwitching; - } - /** * Normalizes the theme configuration to ensure it has a valid algorithm. * @param theme - The theme configuration to normalize @@ -633,13 +595,11 @@ export class ThemeController { } // Priority 2: System theme based on mode (applies to all contexts) - const { allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference } = - this.themeSettings; - let resolvedMode: ThemeMode = mode; if (mode === ThemeMode.SYSTEM) { - if (!allowOSPreference) return null; + // OS preference is allowed when dark theme exists + if (this.darkTheme === null) return null; resolvedMode = ThemeController.getSystemPreferredMode(); } @@ -661,35 +621,18 @@ export class ThemeController { * Determines the initial theme mode with error recovery. */ private determineInitialMode(): ThemeMode { - const { - enforced = DEFAULT_THEME_SETTINGS.enforced, - allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference, - allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching, - } = this.themeSettings; - - // Enforced mode always takes precedence - if (enforced) { + // If no dark theme is available, force default mode + if (this.darkTheme === null) { this.storage.removeItem(this.modeStorageKey); return ThemeMode.DEFAULT; } - // When OS preference is allowed but switching is not - // This means the user MUST follow OS preference and cannot override it - if (allowOSPreference && !allowSwitching) { - // Clear any saved preference since switching is not allowed - this.storage.removeItem(this.modeStorageKey); - return ThemeMode.SYSTEM; - } - // Try to restore saved mode const savedMode: ThemeMode | null = this.loadSavedMode(); if (savedMode && this.isValidThemeMode(savedMode)) return savedMode; - // Fallback to system preference if allowed and available - if (allowOSPreference && this.getThemeForMode(this.systemMode)) - return ThemeMode.SYSTEM; - - return ThemeMode.DEFAULT; + // Default to system preference when both themes are available + return ThemeMode.SYSTEM; } /** @@ -724,7 +667,7 @@ export class ThemeController { case ThemeMode.DEFAULT: return !!this.defaultTheme; case ThemeMode.SYSTEM: - return this.themeSettings?.allowOSPreference !== false; + return this.darkTheme !== null; default: return true; } @@ -742,28 +685,13 @@ export class ThemeController { * Validates permission to update mode. * @param newMode - The new mode to validate * @throws {Error} If the user does not have permission to update the theme mode - * @throws {Error} If the new mode is SYSTEM and OS preference is not allowed */ private validateModeUpdatePermission(newMode: ThemeMode): void { - const { - allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference, - allowSwitching = DEFAULT_THEME_SETTINGS.allowSwitching, - } = this.themeSettings; - - // If OS preference is allowed but switching is not, - // don't allow any mode changes - if (allowOSPreference && !allowSwitching) - throw new Error( - 'Theme mode changes are not allowed when OS preference is enforced', - ); - - // Check if user can set a new theme mode + // Check if user can set a new theme mode (dark theme must exist) if (!this.canSetMode()) - throw new Error('User does not have permission to update the theme mode'); - - // Check if user has permissions to set OS preference as a theme mode - if (newMode === ThemeMode.SYSTEM && !allowOSPreference) - throw new Error('System theme mode is not allowed'); + throw new Error( + 'Theme mode changes are not allowed when only one theme is available', + ); } /** diff --git a/superset-frontend/src/theme/tests/ThemeController.test.ts b/superset-frontend/src/theme/tests/ThemeController.test.ts index 1851dfabd9c..ae5d66e7b09 100644 --- a/superset-frontend/src/theme/tests/ThemeController.test.ts +++ b/superset-frontend/src/theme/tests/ThemeController.test.ts @@ -73,18 +73,11 @@ const DARK_THEME: AnyThemeConfig = { algorithm: ThemeAlgorithm.DARK, }; -const THEME_SETTINGS = { - enforced: false, - allowSwitching: true, - allowOSPreference: true, -}; - // BootstrapData common template generator const createMockBootstrapData = ( themeConfig: BootstrapThemeDataConfig = { default: DEFAULT_THEME, dark: DARK_THEME, - settings: THEME_SETTINGS, }, ): { common: CommonBootstrapData } => ({ common: { @@ -256,7 +249,6 @@ describe('ThemeController', () => { createMockBootstrapData({ default: {}, dark: {}, - settings: {}, }), ); @@ -281,32 +273,11 @@ describe('ThemeController', () => { ); }); - it('should respect enforced theme settings', () => { - mockGetBootstrapData.mockReturnValue( - createMockBootstrapData({ - default: {}, - dark: {}, - settings: { enforced: true, allowSwitching: false }, - }), - ); - - mockLocalStorage.getItem.mockReturnValue(ThemeMode.DARK); - - controller = new ThemeController({ - themeObject: mockThemeObject, - }); - - expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT); - expect(controller.canSetTheme()).toBe(false); - expect(controller.canSetMode()).toBe(false); - }); - it('should handle system theme preference', () => { mockGetBootstrapData.mockReturnValue( createMockBootstrapData({ - default: {}, - dark: {}, - settings: { allowOSPreference: true }, + default: DEFAULT_THEME, + dark: DARK_THEME, }), ); @@ -322,7 +293,6 @@ describe('ThemeController', () => { createMockBootstrapData({ default: DEFAULT_THEME, dark: {}, - settings: {}, }), ); @@ -333,12 +303,13 @@ describe('ThemeController', () => { // Clear the call from initialization jest.clearAllMocks(); - controller.setThemeMode(ThemeMode.DARK); - - expect(mockSetConfig).toHaveBeenCalledTimes(1); - expect(mockSetConfig).toHaveBeenCalledWith( - expect.objectContaining(DEFAULT_THEME), + // Should throw when trying to change mode with only one theme + expect(() => controller.setThemeMode(ThemeMode.DARK)).toThrow( + 'Theme mode changes are not allowed when only one theme is available', ); + + // Config should not have been called since the error was thrown + expect(mockSetConfig).not.toHaveBeenCalled(); }); it('should handle only dark theme', () => { @@ -346,7 +317,6 @@ describe('ThemeController', () => { createMockBootstrapData({ default: {}, dark: DARK_THEME, - settings: {}, }), ); @@ -354,47 +324,22 @@ describe('ThemeController', () => { themeObject: mockThemeObject, }); + // When only dark theme is available, controller uses the default fallback theme initially expect(mockSetConfig).toHaveBeenCalledTimes(1); - expect(mockSetConfig).toHaveBeenCalledWith( - expect.objectContaining({ - ...DARK_THEME, - algorithm: antdThemeImport.darkAlgorithm, - }), - ); - }); - it('should handle only settings', () => { - const fallbackTheme = { - token: { - colorBgBase: '#ffffff', - colorPrimary: '#1890ff', - }, - }; + const calledWith = mockSetConfig.mock.calls[0][0]; - mockGetBootstrapData.mockReturnValue( - createMockBootstrapData({ - default: {}, - dark: {}, - settings: { enforced: true }, - }), - ); + // Should use the default theme fallback (not dark) for initial load + expect(calledWith.colorBgBase).toBe('#fff'); + expect(calledWith.colorTextBase).toBe('#000'); - controller = new ThemeController({ - themeObject: mockThemeObject, - defaultTheme: fallbackTheme, - }); + // Should allow mode changes since dark theme exists + expect(controller.canSetMode()).toBe(true); - expect(controller.canSetTheme()).toBe(false); + // Should be able to switch to dark mode + jest.clearAllMocks(); + controller.setThemeMode(ThemeMode.DARK); expect(mockSetConfig).toHaveBeenCalledTimes(1); - expect(mockSetConfig).toHaveBeenCalledWith( - expect.objectContaining({ - token: expect.objectContaining({ - colorBgBase: '#ffffff', - colorPrimary: '#1890ff', - }), - algorithm: antdThemeImport.defaultAlgorithm, - }), - ); }); it('should handle completely empty BootstrapData', () => { @@ -409,7 +354,6 @@ describe('ThemeController', () => { createMockBootstrapData({ default: {}, dark: {}, - settings: {}, }), ); @@ -466,42 +410,6 @@ describe('ThemeController', () => { ); }); - it('should allow theme switching if there is no bootstrap themes', () => { - const fallbackTheme = { - token: { - colorBgBase: '#ffffff', - colorPrimary: '#1890ff', - }, - }; - - mockGetBootstrapData.mockReturnValue( - createMockBootstrapData({ - default: {}, - dark: {}, - settings: {}, - }), - ); - - controller = new ThemeController({ - themeObject: mockThemeObject, - defaultTheme: fallbackTheme, - }); - - // Clear initialization calls - jest.clearAllMocks(); - - // Switch to dark mode - controller.setThemeMode(ThemeMode.DARK); - - expect(mockSetConfig).toHaveBeenCalledTimes(1); - expect(mockSetConfig).toHaveBeenCalledWith( - expect.objectContaining({ - ...fallbackTheme, - algorithm: antdThemeImport.darkAlgorithm, - }), - ); - }); - describe('Theme Management', () => { beforeEach(() => { controller = new ThemeController({ @@ -524,29 +432,6 @@ describe('ThemeController', () => { ); }); - it('should throw error when theme updates are not allowed', () => { - mockGetBootstrapData.mockReturnValue( - createMockBootstrapData({ - default: {}, - dark: {}, - settings: { enforced: true }, - }), - ); - - controller = new ThemeController({ - themeObject: mockThemeObject, - }); - - expect(() => { - controller.setTheme({ - token: { - colorBgBase: '#000000', - colorPrimary: '#ff0000', - }, - }); - }).toThrow('User does not have permission to update the theme'); - }); - it('should change theme mode when allowed', () => { // Clear initialization calls jest.clearAllMocks(); @@ -561,72 +446,11 @@ describe('ThemeController', () => { ); }); - it('should throw error when mode updates are not allowed but OS preference is', () => { - mockGetBootstrapData.mockReturnValue( - createMockBootstrapData({ - default: DEFAULT_THEME, - dark: DARK_THEME, - settings: { allowSwitching: false }, - }), - ); - - controller = new ThemeController({ - themeObject: mockThemeObject, - }); - - expect(() => { - controller.setThemeMode(ThemeMode.DARK); - }).toThrow( - 'Theme mode changes are not allowed when OS preference is enforced', - ); - }); - - it('should throw error when mode updates and OS preference are not allowed', () => { - mockGetBootstrapData.mockReturnValue( - createMockBootstrapData({ - default: DEFAULT_THEME, - dark: DARK_THEME, - settings: { allowOSPreference: false, allowSwitching: false }, - }), - ); - - controller = new ThemeController({ - themeObject: mockThemeObject, - }); - - expect(() => { - controller.setThemeMode(ThemeMode.DARK); - }).toThrow('User does not have permission to update the theme mode'); - }); - - it('should throw error when system mode is not allowed', () => { - mockGetBootstrapData.mockReturnValue( - createMockBootstrapData({ - default: DEFAULT_THEME, - dark: DARK_THEME, - settings: { - allowOSPreference: false, - allowSwitching: true, - enforced: false, - }, - }), - ); - - controller = new ThemeController({ - themeObject: mockThemeObject, - }); - - expect(() => { - controller.setThemeMode(ThemeMode.SYSTEM); - }).toThrow('System theme mode is not allowed'); - }); - it('should handle missing theme gracefully', () => { mockGetBootstrapData.mockReturnValue( createMockBootstrapData({ default: DEFAULT_THEME, dark: {}, - settings: THEME_SETTINGS, }), ); @@ -634,9 +458,13 @@ describe('ThemeController', () => { themeObject: mockThemeObject, }); - controller.setThemeMode(ThemeMode.DARK); + // Should throw when trying to set mode with only one theme + expect(() => controller.setThemeMode(ThemeMode.DARK)).toThrow( + 'Theme mode changes are not allowed when only one theme is available', + ); - expect(controller.getCurrentMode()).toBe(ThemeMode.DARK); + // Mode should remain unchanged + expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT); expect(consoleSpy).not.toHaveBeenCalled(); }); @@ -757,7 +585,6 @@ describe('ThemeController', () => { createMockBootstrapData({ default: {}, dark: {}, - settings: { allowOSPreference: false }, }), ); @@ -875,7 +702,6 @@ describe('ThemeController', () => { algorithm: [ThemeAlgorithm.DARK, ThemeAlgorithm.COMPACT], }, dark: DARK_THEME, - settings: THEME_SETTINGS, }), ); @@ -1061,7 +887,6 @@ describe('ThemeController', () => { createMockBootstrapData({ default: {}, dark: {}, - settings: {}, }), ); @@ -1077,11 +902,6 @@ describe('ThemeController', () => { const themeConfig = { theme_default: DEFAULT_THEME, theme_dark: DARK_THEME, - theme_settings: { - enforced: false, - allowSwitching: true, - allowOSPreference: true, - }, }; controller.setThemeConfig(themeConfig); @@ -1094,7 +914,7 @@ describe('ThemeController', () => { }), ); - expect(controller.getCurrentMode()).toBe(ThemeMode.SYSTEM); + expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT); expect(controller.canSetTheme()).toBe(true); expect(controller.canSetMode()).toBe(true); }); @@ -1115,7 +935,7 @@ describe('ThemeController', () => { ); expect(controller.canSetTheme()).toBe(true); - expect(controller.canSetMode()).toBe(true); + expect(controller.canSetMode()).toBe(false); }); it('should handle theme_default and theme_dark without settings', () => { @@ -1145,74 +965,7 @@ describe('ThemeController', () => { ); }); - it('should handle enforced theme settings', () => { - const themeConfig = { - theme_default: DEFAULT_THEME, - theme_dark: DARK_THEME, - theme_settings: { - enforced: true, - allowSwitching: false, - allowOSPreference: false, - }, - }; - - controller.setThemeConfig(themeConfig); - - expect(controller.canSetTheme()).toBe(false); - expect(controller.canSetMode()).toBe(false); - expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT); - - expect(() => { - controller.setThemeMode(ThemeMode.DARK); - }).toThrow('User does not have permission to update the theme mode'); - }); - - it('should handle allowOSPreference: false setting', () => { - const themeConfig = { - theme_default: DEFAULT_THEME, - theme_dark: DARK_THEME, - theme_settings: { - enforced: false, - allowSwitching: true, - allowOSPreference: false, - }, - }; - - controller.setThemeConfig(themeConfig); - - expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT); - expect(controller.canSetMode()).toBe(true); - - expect(() => { - controller.setThemeMode(ThemeMode.SYSTEM); - }).toThrow('System theme mode is not allowed'); - }); - - it('should re-determine initial mode based on new settings', () => { - mockMatchMedia.mockReturnValue({ - matches: true, - addEventListener: jest.fn(), - removeEventListener: jest.fn(), - }); - - const themeConfig = { - theme_default: DEFAULT_THEME, - theme_dark: DARK_THEME, - theme_settings: { - enforced: false, - allowSwitching: false, - allowOSPreference: true, - }, - }; - - controller.setThemeConfig(themeConfig); - - expect(controller.getCurrentMode()).toBe(ThemeMode.SYSTEM); - expect(controller.canSetMode()).toBe(false); - }); - it('should apply appropriate theme after configuration', () => { - controller.setThemeMode(ThemeMode.DARK); jest.clearAllMocks(); const themeConfig = { @@ -1236,10 +989,9 @@ describe('ThemeController', () => { expect(mockSetConfig).toHaveBeenCalledWith( expect.objectContaining({ token: expect.objectContaining({ - colorPrimary: '#ff0000', - colorBgBase: '#000000', + colorPrimary: '#00ff00', }), - algorithm: antdThemeImport.darkAlgorithm, + algorithm: antdThemeImport.defaultAlgorithm, }), ); }); @@ -1247,26 +999,27 @@ describe('ThemeController', () => { it('should handle missing theme_dark gracefully', () => { const themeConfig = { theme_default: DEFAULT_THEME, - theme_settings: { - allowSwitching: true, - }, }; controller.setThemeConfig(themeConfig); - jest.clearAllMocks(); - controller.setThemeMode(ThemeMode.DARK); - - expect(mockSetConfig).toHaveBeenCalledTimes(1); - expect(mockSetConfig).toHaveBeenCalledWith( - expect.objectContaining({ - token: expect.objectContaining(DEFAULT_THEME.token), - algorithm: antdThemeImport.defaultAlgorithm, - }), - ); + // Can't set dark mode when there's no dark theme + expect(controller.canSetMode()).toBe(false); }); it('should preserve existing theme mode when possible', () => { + // First create controller with dark theme available + mockGetBootstrapData.mockReturnValue( + createMockBootstrapData({ + default: DEFAULT_THEME, + dark: DARK_THEME, + }), + ); + + controller = new ThemeController({ + themeObject: mockThemeObject, + }); + controller.setThemeMode(ThemeMode.DARK); const initialMode = controller.getCurrentMode(); @@ -1275,10 +1028,6 @@ describe('ThemeController', () => { const themeConfig = { theme_default: DEFAULT_THEME, theme_dark: DARK_THEME, - theme_settings: { - allowSwitching: true, - allowOSPreference: false, - }, }; controller.setThemeConfig(themeConfig); @@ -1301,20 +1050,6 @@ describe('ThemeController', () => { expect(changeCallback).toHaveBeenCalledWith(mockThemeObject); }); - it('should handle partial theme_settings', () => { - const themeConfig = { - theme_default: DEFAULT_THEME, - theme_settings: { - enforced: true, - }, - }; - - controller.setThemeConfig(themeConfig); - - expect(controller.canSetTheme()).toBe(false); - expect(controller.canSetMode()).toBe(false); - }); - it('should handle error in theme application', () => { mockSetConfig.mockImplementationOnce(() => { throw new Error('Theme application error'); diff --git a/superset-frontend/src/types/bootstrapTypes.ts b/superset-frontend/src/types/bootstrapTypes.ts index e64921900d8..abe32d65834 100644 --- a/superset-frontend/src/types/bootstrapTypes.ts +++ b/superset-frontend/src/types/bootstrapTypes.ts @@ -145,16 +145,10 @@ export interface MenuData { }; } -export interface SerializableThemeSettings { - enforced?: boolean; - allowSwitching?: boolean; - allowOSPreference?: boolean; -} - export interface BootstrapThemeDataConfig { default: SerializableThemeConfig | {}; dark: SerializableThemeConfig | {}; - settings: SerializableThemeSettings | {}; + enableUiThemeAdministration?: boolean; } export interface CommonBootstrapData { @@ -186,7 +180,6 @@ export interface BootstrapData { export interface BootstrapThemeData { bootstrapDefaultTheme: AnyThemeConfig | null; bootstrapDarkTheme: AnyThemeConfig | null; - bootstrapThemeSettings: SerializableThemeSettings | null; hasCustomThemes: boolean; } diff --git a/superset-frontend/src/views/theme.bootstrap.test.ts b/superset-frontend/src/views/theme.bootstrap.test.ts new file mode 100644 index 00000000000..4add3ea7172 --- /dev/null +++ b/superset-frontend/src/views/theme.bootstrap.test.ts @@ -0,0 +1,132 @@ +/** + * 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. + */ + +/** + * Tests for theme bootstrap data loading logic + * + * These tests validate the behavior of get_theme_bootstrap_data() in base.py + * by testing the expected bootstrap data structure + */ + +describe('Theme Bootstrap Data', () => { + describe('when UI theme administration is enabled', () => { + const mockBootstrapData = { + theme: { + default: { colors: { primary: '#1890ff' } }, + dark: { colors: { primary: '#000000' } }, + enableUiThemeAdministration: true, + }, + }; + + it('should load themes from database when available', () => { + // This tests that when enableUiThemeAdministration is true, + // the system attempts to load themes from the database + expect(mockBootstrapData.theme.enableUiThemeAdministration).toBe(true); + expect(mockBootstrapData.theme.default).toBeDefined(); + expect(mockBootstrapData.theme.dark).toBeDefined(); + }); + + it('should have proper theme structure', () => { + expect(mockBootstrapData.theme).toHaveProperty('default'); + expect(mockBootstrapData.theme).toHaveProperty('dark'); + expect(mockBootstrapData.theme).toHaveProperty( + 'enableUiThemeAdministration', + ); + }); + }); + + describe('when UI theme administration is disabled', () => { + const mockBootstrapData = { + theme: { + default: { colors: { primary: '#1890ff' } }, + dark: { colors: { primary: '#000000' } }, + enableUiThemeAdministration: false, + }, + }; + + it('should use config-based themes', () => { + // When enableUiThemeAdministration is false, + // themes should come from configuration files + expect(mockBootstrapData.theme.enableUiThemeAdministration).toBe(false); + expect(mockBootstrapData.theme.default).toBeDefined(); + expect(mockBootstrapData.theme.dark).toBeDefined(); + }); + }); + + describe('edge cases', () => { + it('should handle missing theme gracefully', () => { + const mockBootstrapData = { + theme: { + default: {}, + dark: {}, + enableUiThemeAdministration: true, + }, + }; + + // Empty theme objects should be valid + expect(mockBootstrapData.theme.default).toEqual({}); + expect(mockBootstrapData.theme.dark).toEqual({}); + }); + + it('should handle invalid theme settings', () => { + const mockBootstrapData = { + theme: { + default: {}, + dark: {}, + enableUiThemeAdministration: false, + }, + }; + + // Should fall back to defaults when settings are invalid + expect(mockBootstrapData.theme.enableUiThemeAdministration).toBeDefined(); + expect(mockBootstrapData.theme.enableUiThemeAdministration).toBe(false); + }); + }); + + describe('permissions integration', () => { + it('should respect admin-only access for system themes', () => { + const mockBootstrapData = { + theme: { + default: {}, + dark: {}, + enableUiThemeAdministration: true, + }, + }; + + // When UI theme administration is enabled, + // only admins should be able to modify system themes + expect(mockBootstrapData.theme.enableUiThemeAdministration).toBe(true); + }); + + it('should allow all users to view themes', () => { + const mockBootstrapData = { + theme: { + default: { colors: { primary: '#1890ff' } }, + dark: { colors: { primary: '#000000' } }, + enableUiThemeAdministration: true, + }, + }; + + // All users should be able to see theme data in bootstrap + expect(mockBootstrapData.theme).toBeDefined(); + expect(mockBootstrapData.theme.default).toBeDefined(); + expect(mockBootstrapData.theme.dark).toBeDefined(); + }); + }); +}); diff --git a/superset/commands/theme/delete.py b/superset/commands/theme/delete.py index f149eb7b45f..6894a56e2df 100644 --- a/superset/commands/theme/delete.py +++ b/superset/commands/theme/delete.py @@ -20,6 +20,7 @@ from typing import Optional from superset.commands.base import BaseCommand from superset.commands.theme.exceptions import ( + SystemThemeInUseError, SystemThemeProtectedError, ThemeDeleteFailedError, ThemeNotFoundError, @@ -58,6 +59,9 @@ class DeleteThemeCommand(BaseCommand): for theme in self._models: if theme.is_system: raise SystemThemeProtectedError() + # Check if theme is in use as system default or dark + if theme.is_system_default or theme.is_system_dark: + raise SystemThemeInUseError() # Check for dashboard usage self._dashboard_usage = self._get_dashboard_usage() diff --git a/superset/commands/theme/exceptions.py b/superset/commands/theme/exceptions.py index ec0529bce7f..24d8a142f53 100644 --- a/superset/commands/theme/exceptions.py +++ b/superset/commands/theme/exceptions.py @@ -33,3 +33,11 @@ class ThemeNotFoundError(CommandException): class SystemThemeProtectedError(CommandException): message = _("Cannot modify system themes.") + + +class SystemThemeInUseError(CommandException): + message = _("Cannot delete theme that is set as system default or dark theme.") + + +class ThemeAdministrationDisabledError(CommandException): + message = _("UI theme administration is not enabled.") diff --git a/superset/commands/theme/set_system_theme.py b/superset/commands/theme/set_system_theme.py new file mode 100644 index 00000000000..23001868d15 --- /dev/null +++ b/superset/commands/theme/set_system_theme.py @@ -0,0 +1,126 @@ +# 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 logging +from functools import partial +from typing import Optional + +from sqlalchemy import update + +from superset.commands.base import BaseCommand +from superset.commands.theme.exceptions import ThemeNotFoundError +from superset.daos.theme import ThemeDAO +from superset.extensions import db +from superset.models.core import Theme +from superset.utils.decorators import on_error, transaction + +logger = logging.getLogger(__name__) + + +class SetSystemDefaultThemeCommand(BaseCommand): + def __init__(self, theme_id: int): + self._theme_id = theme_id + self._theme: Optional[Theme] = None + + @transaction(on_error=partial(on_error, reraise=Exception)) + def run(self) -> Theme: + self.validate() + assert self._theme + + # Clear all existing system defaults in a single query + db.session.execute( + update(Theme) + .where(Theme.is_system_default.is_(True)) + .values(is_system_default=False) + ) + + # Set the new system default + self._theme.is_system_default = True + db.session.add(self._theme) + + logger.info(f"Set theme {self._theme_id} as system default") + + return self._theme + + def validate(self) -> None: + self._theme = ThemeDAO.find_by_id(self._theme_id) + if not self._theme: + raise ThemeNotFoundError() + + +class SetSystemDarkThemeCommand(BaseCommand): + def __init__(self, theme_id: int): + self._theme_id = theme_id + self._theme: Optional[Theme] = None + + @transaction(on_error=partial(on_error, reraise=Exception)) + def run(self) -> Theme: + self.validate() + assert self._theme + + # Clear all existing system dark themes in a single query + db.session.execute( + update(Theme) + .where(Theme.is_system_dark.is_(True)) + .values(is_system_dark=False) + ) + + # Set the new system dark theme + self._theme.is_system_dark = True + db.session.add(self._theme) + + logger.info(f"Set theme {self._theme_id} as system dark") + + return self._theme + + def validate(self) -> None: + self._theme = ThemeDAO.find_by_id(self._theme_id) + if not self._theme: + raise ThemeNotFoundError() + + +class ClearSystemDefaultThemeCommand(BaseCommand): + @transaction(on_error=partial(on_error, reraise=Exception)) + def run(self) -> None: + # Clear all system default themes + db.session.execute( + update(Theme) + .where(Theme.is_system_default.is_(True)) + .values(is_system_default=False) + ) + + logger.info("Cleared system default theme") + + def validate(self) -> None: + # No validation needed for clearing + pass + + +class ClearSystemDarkThemeCommand(BaseCommand): + @transaction(on_error=partial(on_error, reraise=Exception)) + def run(self) -> None: + # Clear all system dark themes + db.session.execute( + update(Theme) + .where(Theme.is_system_dark.is_(True)) + .values(is_system_dark=False) + ) + + logger.info("Cleared system dark theme") + + def validate(self) -> None: + # No validation needed for clearing + pass diff --git a/superset/config.py b/superset/config.py index 27782c1a75c..4e3034e8648 100644 --- a/superset/config.py +++ b/superset/config.py @@ -755,17 +755,13 @@ THEME_DEFAULT: Theme = {"algorithm": "default"} THEME_DARK: Theme = {"algorithm": "dark"} # Theme behavior and user preference settings -# Controls how themes are applied and what options users have -# - enforced: Forces the default theme always, overriding all other settings -# - allowSwitching: Allows users to manually switch between default and dark themes. -# - allowOSPreference: Allows the app to automatically use the system's preferred theme mode # noqa: E501 +# To force a single theme on all users, set THEME_DARK = None +# When both THEME_DEFAULT and THEME_DARK are defined: +# - Users can manually switch between themes +# - OS preference detection is automatically enabled # -# Example: -THEME_SETTINGS = { - "enforced": False, # If True, forces the default theme and ignores user preferences # noqa: E501 - "allowSwitching": True, # Allows user to switch between themes (default and dark) # noqa: E501 - "allowOSPreference": True, # Allows the app to Auto-detect and set system theme preference # noqa: E501 -} +# Enable UI-based theme administration for admins +ENABLE_UI_THEME_ADMINISTRATION = True # Allows admins to set system themes via UI # Custom font configuration # Load external fonts at runtime without rebuilding the application diff --git a/superset/daos/theme.py b/superset/daos/theme.py index 3430567c4f6..024d698ff0d 100644 --- a/superset/daos/theme.py +++ b/superset/daos/theme.py @@ -29,3 +29,59 @@ class ThemeDAO(BaseDAO[Theme]): def find_by_uuid(cls, uuid: str) -> Optional[Theme]: """Find theme by UUID.""" return db.session.query(Theme).filter(Theme.uuid == uuid).first() + + @classmethod + def find_system_default(cls) -> Optional[Theme]: + """Find the current system default theme. + + First looks for a theme with is_system_default=True. + If not found or multiple found, falls back to is_system=True theme + with name 'THEME_DEFAULT'. + """ + system_defaults = ( + db.session.query(Theme).filter(Theme.is_system_default.is_(True)).all() + ) + + if len(system_defaults) == 1: + return system_defaults[0] + + if len(system_defaults) > 1: + logger.warning( + f"Multiple system default themes found ({len(system_defaults)}), " + "falling back to config theme" + ) + + # Fallback to is_system=True theme with name 'THEME_DEFAULT' + return ( + db.session.query(Theme) + .filter(Theme.is_system.is_(True), Theme.theme_name == "THEME_DEFAULT") + .first() + ) + + @classmethod + def find_system_dark(cls) -> Optional[Theme]: + """Find the current system dark theme. + + First looks for a theme with is_system_dark=True. + If not found or multiple found, falls back to is_system=True theme + with name 'THEME_DARK'. + """ + system_darks = ( + db.session.query(Theme).filter(Theme.is_system_dark.is_(True)).all() + ) + + if len(system_darks) == 1: + return system_darks[0] + + if len(system_darks) > 1: + logger.warning( + f"Multiple system dark themes found ({len(system_darks)}), " + "falling back to config theme" + ) + + # Fallback to is_system=True theme with name 'THEME_DARK' + return ( + db.session.query(Theme) + .filter(Theme.is_system.is_(True), Theme.theme_name == "THEME_DARK") + .first() + ) diff --git a/superset/migrations/versions/2025-08-05_14-38_c233f5365c9e_is_system_default_is_system_dark.py b/superset/migrations/versions/2025-08-05_14-38_c233f5365c9e_is_system_default_is_system_dark.py new file mode 100644 index 00000000000..65d1e6fbff7 --- /dev/null +++ b/superset/migrations/versions/2025-08-05_14-38_c233f5365c9e_is_system_default_is_system_dark.py @@ -0,0 +1,87 @@ +# 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. +"""is_system_default-is_system_dark + +Revision ID: c233f5365c9e +Revises: cd1fb11291f2 +Create Date: 2025-08-05 14:38:55.782777 + +""" + +import sqlalchemy as sa +from alembic import op +from sqlalchemy import false + +from superset.migrations.shared.utils import ( + add_columns, + create_index, + drop_columns, + drop_index, +) + +# revision identifiers, used by Alembic. +revision = "c233f5365c9e" +down_revision = "cd1fb11291f2" + + +def upgrade(): + # Add columns as nullable first + add_columns( + "themes", + sa.Column("is_system_default", sa.Boolean(), nullable=True), + sa.Column("is_system_dark", sa.Boolean(), nullable=True), + ) + + # Update existing rows to have False values using SQLAlchemy + connection = op.get_bind() + themes_table = sa.table( + "themes", + sa.column("is_system_default", sa.Boolean()), + sa.column("is_system_dark", sa.Boolean()), + ) + + connection.execute( + themes_table.update() + .where(themes_table.c.is_system_default.is_(None)) + .values(is_system_default=false()) + ) + + connection.execute( + themes_table.update() + .where(themes_table.c.is_system_dark.is_(None)) + .values(is_system_dark=false()) + ) + + # Now make the columns non-nullable + # MySQL requires explicit type specification for CHANGE/MODIFY operations + with op.batch_alter_table("themes") as batch_op: + batch_op.alter_column( + "is_system_default", existing_type=sa.Boolean(), nullable=False + ) + batch_op.alter_column( + "is_system_dark", existing_type=sa.Boolean(), nullable=False + ) + + # Create indexes + create_index("themes", "idx_theme_is_system_default", ["is_system_default"]) + create_index("themes", "idx_theme_is_system_dark", ["is_system_dark"]) + + +def downgrade(): + drop_index("themes", "idx_theme_is_system_default") + drop_index("themes", "idx_theme_is_system_dark") + drop_columns("themes", "is_system_dark", "is_system_default") diff --git a/superset/models/core.py b/superset/models/core.py index f20806457fd..05267e457df 100755 --- a/superset/models/core.py +++ b/superset/models/core.py @@ -119,10 +119,17 @@ class Theme(AuditMixinNullable, ImportExportMixin, Model): """Themes for dashboards""" __tablename__ = "themes" + __table_args__ = ( + sqla.Index("idx_theme_is_system_default", "is_system_default"), + sqla.Index("idx_theme_is_system_dark", "is_system_dark"), + ) + id = Column(Integer, primary_key=True) theme_name = Column(String(250)) json_data = Column(utils.MediumText(), default="") is_system = Column(Boolean, default=False, nullable=False) + is_system_default = Column(Boolean, default=False, nullable=False) + is_system_dark = Column(Boolean, default=False, nullable=False) export_fields = ["theme_name", "json_data"] diff --git a/superset/themes/api.py b/superset/themes/api.py index ff104e8dde3..b98d3df4308 100644 --- a/superset/themes/api.py +++ b/superset/themes/api.py @@ -20,7 +20,7 @@ from io import BytesIO from typing import Any from zipfile import ZipFile -from flask import request, Response, send_file +from flask import current_app as app, request, Response, send_file from flask_appbuilder.api import expose, protect, rison, safe from flask_appbuilder.models.sqla.interface import SQLAInterface from flask_babel import ngettext @@ -30,12 +30,19 @@ from werkzeug.datastructures import FileStorage from superset.commands.importers.v1.utils import get_contents_from_bundle from superset.commands.theme.delete import DeleteThemeCommand from superset.commands.theme.exceptions import ( + SystemThemeInUseError, SystemThemeProtectedError, ThemeDeleteFailedError, ThemeNotFoundError, ) from superset.commands.theme.export import ExportThemesCommand from superset.commands.theme.importers.dispatcher import ImportThemesCommand +from superset.commands.theme.set_system_theme import ( + ClearSystemDarkThemeCommand, + ClearSystemDefaultThemeCommand, + SetSystemDarkThemeCommand, + SetSystemDefaultThemeCommand, +) from superset.commands.theme.update import UpdateThemeCommand from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.extensions import event_logger @@ -67,9 +74,19 @@ class ThemeRestApi(BaseSupersetModelRestApi): RouteMethod.IMPORT, RouteMethod.RELATED, "bulk_delete", # not using RouteMethod since locally defined + "set_system_default", + "set_system_dark", + "unset_system_default", + "unset_system_dark", } class_permission_name = "Theme" - method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP + method_permission_name = { + **MODEL_API_RW_METHOD_PERMISSION_MAP, + "set_system_default": "write", + "set_system_dark": "write", + "unset_system_default": "write", + "unset_system_dark": "write", + } resource_name = "theme" allow_browser_login = True @@ -85,6 +102,8 @@ class ThemeRestApi(BaseSupersetModelRestApi): "json_data", "id", "is_system", + "is_system_default", + "is_system_dark", "theme_name", "uuid", ] @@ -101,6 +120,8 @@ class ThemeRestApi(BaseSupersetModelRestApi): "json_data", "id", "is_system", + "is_system_default", + "is_system_dark", "theme_name", "uuid", ] @@ -183,6 +204,8 @@ class ThemeRestApi(BaseSupersetModelRestApi): return self.response_404() except SystemThemeProtectedError: return self.response_403() + except SystemThemeInUseError as ex: + return self.response_422(message=str(ex)) except ThemeDeleteFailedError as ex: logger.exception(f"Theme delete failed for ID: {pk}") return self.response_422(message=str(ex)) @@ -242,6 +265,8 @@ class ThemeRestApi(BaseSupersetModelRestApi): return self.response_404() except SystemThemeProtectedError: return self.response_403() + except SystemThemeInUseError as ex: + return self.response_422(message=str(ex)) except ThemeDeleteFailedError as ex: logger.exception(f"Theme delete failed for IDs: {item_ids}") return self.response_422(message=str(ex)) @@ -532,3 +557,237 @@ class ThemeRestApi(BaseSupersetModelRestApi): except Exception as ex: logger.exception("Unexpected error importing themes") return self.response_422(message=str(ex)) + + @expose("//set_system_default", methods=("PUT",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, + *args, + **kwargs: f"{self.__class__.__name__}.set_system_default", + log_to_statsd=False, + ) + def set_system_default(self, pk: int) -> Response: + """Set a theme as the system default theme. + --- + put: + summary: Set a theme as the system default theme + parameters: + - in: path + schema: + type: integer + description: The theme id + name: pk + responses: + 200: + description: Theme successfully set as system default + content: + application/json: + schema: + type: object + properties: + id: + type: integer + result: + type: string + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + # Check if user is admin + from superset import security_manager + + if not security_manager.is_admin(): + return self.response( + 403, message="Only administrators can set system themes" + ) + + # Check if UI theme administration is enabled + if not app.config.get("ENABLE_UI_THEME_ADMINISTRATION", False): + return self.response(403, message="UI theme administration is not enabled") + + try: + command = SetSystemDefaultThemeCommand(pk) + theme = command.run() + return self.response(200, id=theme.id, result="success") + except ThemeNotFoundError: + return self.response_404() + except Exception as ex: + return self.response_422(message=str(ex)) + + @expose("//set_system_dark", methods=("PUT",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, + *args, + **kwargs: f"{self.__class__.__name__}.set_system_dark", + log_to_statsd=False, + ) + def set_system_dark(self, pk: int) -> Response: + """Set a theme as the system dark theme. + --- + put: + summary: Set a theme as the system dark theme + parameters: + - in: path + schema: + type: integer + description: The theme id + name: pk + responses: + 200: + description: Theme successfully set as system dark + content: + application/json: + schema: + type: object + properties: + id: + type: integer + result: + type: string + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + # Check if user is admin + from superset import security_manager + + if not security_manager.is_admin(): + return self.response( + 403, message="Only administrators can set system themes" + ) + + # Check if UI theme administration is enabled + if not app.config.get("ENABLE_UI_THEME_ADMINISTRATION", False): + return self.response(403, message="UI theme administration is not enabled") + + try: + command = SetSystemDarkThemeCommand(pk) + theme = command.run() + return self.response(200, id=theme.id, result="success") + except ThemeNotFoundError: + return self.response_404() + except Exception as ex: + return self.response_422(message=str(ex)) + + @expose("/unset_system_default", methods=("DELETE",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, + *args, + **kwargs: f"{self.__class__.__name__}.unset_system_default", + log_to_statsd=False, + ) + def unset_system_default(self) -> Response: + """Clear the system default theme. + --- + delete: + summary: Clear the system default theme + responses: + 200: + description: System default theme cleared + content: + application/json: + schema: + type: object + properties: + result: + type: string + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 500: + $ref: '#/components/responses/500' + """ + # Check if user is admin + from superset import security_manager + + if not security_manager.is_admin(): + return self.response( + 403, message="Only administrators can set system themes" + ) + + # Check if UI theme administration is enabled + if not app.config.get("ENABLE_UI_THEME_ADMINISTRATION", False): + return self.response(403, message="UI theme administration is not enabled") + + try: + ClearSystemDefaultThemeCommand().run() + return self.response(200, result="success") + except Exception as ex: + return self.response_422(message=str(ex)) + + @expose("/unset_system_dark", methods=("DELETE",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, + *args, + **kwargs: f"{self.__class__.__name__}.unset_system_dark", + log_to_statsd=False, + ) + def unset_system_dark(self) -> Response: + """Clear the system dark theme. + --- + delete: + summary: Clear the system dark theme + responses: + 200: + description: System dark theme cleared + content: + application/json: + schema: + type: object + properties: + result: + type: string + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 500: + $ref: '#/components/responses/500' + """ + # Check if user is admin + from superset import security_manager + + if not security_manager.is_admin(): + return self.response( + 403, message="Only administrators can set system themes" + ) + + # Check if UI theme administration is enabled + if not app.config.get("ENABLE_UI_THEME_ADMINISTRATION", False): + return self.response(403, message="UI theme administration is not enabled") + + try: + ClearSystemDarkThemeCommand().run() + return self.response(200, result="success") + except Exception as ex: + return self.response_422(message=str(ex)) diff --git a/superset/themes/types.py b/superset/themes/types.py index 06153443cf8..5ffb01e5263 100644 --- a/superset/themes/types.py +++ b/superset/themes/types.py @@ -44,24 +44,8 @@ class Theme(TypedDict, total=False): inherit: Optional[bool] -class ThemeSettings(TypedDict, total=False): - """ - Represents the settings for themes in the application. - """ - - enforced: Optional[bool] - allowSwitching: Optional[bool] - allowOSPreference: Optional[bool] - - class ThemeMode(str, Enum): DEFAULT = "default" DARK = "dark" SYSTEM = "system" COMPACT = "compact" - - -class ThemeSettingsKey(str, Enum): - ENFORCED = "enforced" - ALLOW_SWITCHING = "allowSwitching" - ALLOW_OS_PREFERENCE = "allowOSPreference" diff --git a/superset/themes/utils.py b/superset/themes/utils.py index 3b2bebeab1f..7e92b564f4d 100644 --- a/superset/themes/utils.py +++ b/superset/themes/utils.py @@ -16,7 +16,7 @@ # under the License. from typing import Any, Dict -from superset.themes.types import ThemeMode, ThemeSettingsKey +from superset.themes.types import ThemeMode def _is_valid_theme_mode(mode: str) -> bool: @@ -87,29 +87,3 @@ def is_valid_theme(theme: Dict[str, Any]) -> bool: return True except Exception: return False - - -def is_valid_theme_settings(settings: Dict[str, Any]) -> bool: - """Check if theme settings are valid""" - try: - if not isinstance(settings, dict): - return False - - # Empty dict is valid - if not settings: - return True - - # Check if all keys are valid - valid_keys = {setting.value for setting in ThemeSettingsKey} - for key in settings.keys(): - if key not in valid_keys: - return False - - # Type check values - all must be booleans - for _key, value in settings.items(): - if not isinstance(value, bool): - return False - - return True - except Exception: - return False diff --git a/superset/views/base.py b/superset/views/base.py index 2318efe1f90..393e9ac574a 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -56,6 +56,7 @@ from superset import ( security_manager, ) from superset.connectors.sqla import models +from superset.daos.theme import ThemeDAO from superset.db_engine_specs import get_available_engine_specs from superset.db_engine_specs.gsheets import GSheetsEngineSpec from superset.extensions import cache_manager @@ -63,7 +64,6 @@ from superset.reports.models import ReportRecipientType from superset.superset_typing import FlaskResponse from superset.themes.utils import ( is_valid_theme, - is_valid_theme_settings, ) from superset.utils import core as utils, json from superset.utils.filters import get_dataset_access_filters @@ -71,12 +71,6 @@ from superset.views.error_handling import json_error_response from .utils import bootstrap_user_data, get_config_value -DEFAULT_THEME_SETTINGS = { - "enforced": False, - "allowSwitching": True, - "allowOSPreference": True, -} - FRONTEND_CONF_KEYS = ( "SUPERSET_WEBSERVER_TIMEOUT", "SUPERSET_DASHBOARD_POSITION_DATA_LIMIT", @@ -308,39 +302,65 @@ def get_theme_bootstrap_data() -> dict[str, Any]: """ Returns the theme data to be sent to the client. """ - # Get theme configs - default_theme_config = get_config_value("THEME_DEFAULT") - dark_theme_config = get_config_value("THEME_DARK") - theme_settings = get_config_value("THEME_SETTINGS") + # Check if UI theme administration is enabled + enable_ui_admin = app.config.get("ENABLE_UI_THEME_ADMINISTRATION", False) + + if enable_ui_admin: + # Try to load themes from database + default_theme_model = ThemeDAO.find_system_default() + dark_theme_model = ThemeDAO.find_system_dark() + + # Parse theme JSON from database models + default_theme = {} + if default_theme_model: + try: + default_theme = json.loads(default_theme_model.json_data) + except json.JSONDecodeError: + logger.error( + f"Invalid JSON in system default theme {default_theme_model.id}" + ) + # Fallback to config + default_theme = get_config_value("THEME_DEFAULT") + else: + # No system default theme in database, use config + default_theme = get_config_value("THEME_DEFAULT") + + dark_theme = {} + if dark_theme_model: + try: + dark_theme = json.loads(dark_theme_model.json_data) + except json.JSONDecodeError: + logger.error(f"Invalid JSON in system dark theme {dark_theme_model.id}") + # Fallback to config + dark_theme = get_config_value("THEME_DARK") + else: + # No system dark theme in database, use config + dark_theme = get_config_value("THEME_DARK") + else: + # UI theme administration disabled, use config-based themes + default_theme = get_config_value("THEME_DEFAULT") + dark_theme = get_config_value("THEME_DARK") # Validate theme configurations - default_theme = default_theme_config if not is_valid_theme(default_theme): logger.warning( - "Invalid THEME_DEFAULT configuration: %s, using empty theme", - default_theme_config, + "Invalid default theme configuration: %s, using empty theme", + default_theme, ) default_theme = {} - dark_theme = dark_theme_config if not is_valid_theme(dark_theme): logger.warning( - "Invalid THEME_DARK configuration: %s, using empty theme", - dark_theme_config, + "Invalid dark theme configuration: %s, using empty theme", + dark_theme, ) dark_theme = {} - if not is_valid_theme_settings(theme_settings): - logger.warning( - "Invalid THEME_SETTINGS configuration: %s, using defaults", theme_settings - ) - theme_settings = DEFAULT_THEME_SETTINGS - return { "theme": { "default": default_theme, "dark": dark_theme, - "settings": theme_settings, + "enableUiThemeAdministration": enable_ui_admin, } } diff --git a/tests/integration_tests/themes/api_tests.py b/tests/integration_tests/themes/api_tests.py index 4581752a154..7828101fcfe 100644 --- a/tests/integration_tests/themes/api_tests.py +++ b/tests/integration_tests/themes/api_tests.py @@ -92,13 +92,14 @@ class TestThemeApi(SupersetTestCase): "created_on", "id", "is_system", + "is_system_default", + "is_system_dark", "json_data", "theme_name", "uuid", ] result_columns = list(data["result"][0].keys()) - result_columns.sort() - assert expected_columns == result_columns + assert set(expected_columns) == set(result_columns) @pytest.mark.usefixtures("create_themes") def test_get_list_sort_theme(self): @@ -207,6 +208,8 @@ class TestThemeApi(SupersetTestCase): "theme_name": "theme_name1", "json_data": '{"color": "theme1"}', "is_system": False, + "is_system_default": False, + "is_system_dark": False, "uuid": str(theme.uuid), "changed_by": { "first_name": theme.created_by.first_name, diff --git a/tests/integration_tests/themes/test_theme_api_permissions.py b/tests/integration_tests/themes/test_theme_api_permissions.py new file mode 100644 index 00000000000..d444421d148 --- /dev/null +++ b/tests/integration_tests/themes/test_theme_api_permissions.py @@ -0,0 +1,260 @@ +# 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. +"""Integration tests for theme API permissions""" + +import uuid + +from superset import db +from superset.models.core import Theme +from superset.utils import json +from tests.conftest import with_config +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ADMIN_USERNAME, GAMMA_USERNAME + + +class TestThemeAPIPermissions(SupersetTestCase): + """Test theme API permissions and system theme functionality""" + + def setUp(self): + """Set up test fixtures""" + super().setUp() + + # Generate unique identifier for this test + self.test_id = str(uuid.uuid4())[:8] + + # Create test themes + self.regular_theme = Theme( + theme_name=f"Regular Theme {self.test_id}", + json_data=json.dumps({"colors": {"primary": "#1890ff"}}), + is_system=False, + created_by=self.get_user("admin"), + changed_by=self.get_user("admin"), + ) + + self.system_theme = Theme( + theme_name=f"System Theme {self.test_id}", + json_data=json.dumps({"colors": {"primary": "#000000"}}), + is_system=True, + created_by=self.get_user("admin"), + changed_by=self.get_user("admin"), + ) + + db.session.add_all([self.regular_theme, self.system_theme]) + db.session.commit() + + def tearDown(self): + """Clean up test fixtures""" + # Clean up themes + for theme in [self.regular_theme, self.system_theme]: + if theme: + try: + db.session.delete(theme) + except Exception: + db.session.rollback() + + try: + db.session.commit() + except Exception: + db.session.rollback() + + super().tearDown() + + def test_admin_can_set_system_default(self): + """Test that admin can set a theme as system default""" + # Login as admin + self.login(ADMIN_USERNAME) + + # Set theme as system default + response = self.client.put( + f"/api/v1/theme/{self.regular_theme.id}/set_system_default" + ) + + # Should succeed + assert response.status_code == 200 + + # Verify theme is now system default + theme = db.session.query(Theme).filter_by(id=self.regular_theme.id).first() + assert theme.is_system_default is True + + @with_config({"ENABLE_UI_THEME_ADMINISTRATION": True}) + def test_non_admin_cannot_set_system_default(self): + """Test that non-admin users cannot set system themes""" + # Login as gamma user + self.login(GAMMA_USERNAME) + + # Try to set theme as system default + response = self.client.put( + f"/api/v1/theme/{self.regular_theme.id}/set_system_default" + ) + + # Should be forbidden + assert response.status_code == 403 + data = response.get_json() + assert "Only administrators can set system themes" in data["message"] + + # Verify theme is not system default + theme = db.session.query(Theme).filter_by(id=self.regular_theme.id).first() + assert theme.is_system_default is False + + @with_config({"ENABLE_UI_THEME_ADMINISTRATION": False}) + def test_system_theme_requires_config_enabled(self): + """Test that system theme APIs require configuration to be enabled""" + # Login as admin + self.login(ADMIN_USERNAME) + + # Try to set theme as system default + response = self.client.put( + f"/api/v1/theme/{self.regular_theme.id}/set_system_default" + ) + + # Should be forbidden + assert response.status_code == 403 + data = response.get_json() + assert "UI theme administration is not enabled" in data["message"] + + @with_config({"ENABLE_UI_THEME_ADMINISTRATION": True}) + def test_admin_can_set_system_dark(self): + """Test that admin can set a theme as system dark""" + # Login as admin + self.login(ADMIN_USERNAME) + + # Set theme as system dark + response = self.client.put( + f"/api/v1/theme/{self.regular_theme.id}/set_system_dark" + ) + + # Should succeed + assert response.status_code == 200 + + # Verify theme is now system dark + theme = db.session.query(Theme).filter_by(id=self.regular_theme.id).first() + assert theme.is_system_dark is True + + @with_config({"ENABLE_UI_THEME_ADMINISTRATION": True}) + def test_admin_can_unset_system_default(self): + """Test that admin can unset system default theme""" + # First set a theme as system default + self.regular_theme.is_system_default = True + db.session.commit() + + # Login as admin + self.login(ADMIN_USERNAME) + + # Unset system default + response = self.client.delete("/api/v1/theme/unset_system_default") + + # Should succeed + assert response.status_code == 200 + + # Verify no theme is system default + theme = db.session.query(Theme).filter_by(id=self.regular_theme.id).first() + assert theme.is_system_default is False + + @with_config({"ENABLE_UI_THEME_ADMINISTRATION": True}) + def test_admin_can_unset_system_dark(self): + """Test that admin can unset system dark theme""" + # First set a theme as system dark + self.regular_theme.is_system_dark = True + db.session.commit() + + # Login as admin + self.login(ADMIN_USERNAME) + + # Unset system dark + response = self.client.delete("/api/v1/theme/unset_system_dark") + + # Should succeed + assert response.status_code == 200 + + # Verify no theme is system dark + theme = db.session.query(Theme).filter_by(id=self.regular_theme.id).first() + assert theme.is_system_dark is False + + @with_config({"ENABLE_UI_THEME_ADMINISTRATION": True}) + def test_only_one_system_default_allowed(self): + """Test that only one theme can be system default at a time""" + # Create another theme + theme2 = Theme( + theme_name=f"Another Theme {self.test_id}", + json_data=json.dumps({"colors": {"primary": "#ff0000"}}), + created_by=self.get_user("admin"), + changed_by=self.get_user("admin"), + ) + db.session.add(theme2) + db.session.commit() + + try: + # Login as admin + self.login(ADMIN_USERNAME) + + # Set first theme as system default + response = self.client.put( + f"/api/v1/theme/{self.regular_theme.id}/set_system_default" + ) + assert response.status_code == 200 + + # Set second theme as system default + response = self.client.put(f"/api/v1/theme/{theme2.id}/set_system_default") + assert response.status_code == 200 + + # Verify only the second theme is system default + theme1 = db.session.query(Theme).filter_by(id=self.regular_theme.id).first() + theme2_check = db.session.query(Theme).filter_by(id=theme2.id).first() + + assert theme1.is_system_default is False + assert theme2_check.is_system_default is True + + finally: + # Clean up + db.session.delete(theme2) + db.session.commit() + + def test_system_theme_cannot_be_deleted(self): + """Test that themes set as system themes cannot be deleted""" + # Set theme as system default + self.regular_theme.is_system_default = True + db.session.commit() + + # Login as admin + self.login(ADMIN_USERNAME) + + # Try to delete the theme + response = self.client.delete(f"/api/v1/theme/{self.regular_theme.id}") + + # Should fail with appropriate error + assert response.status_code == 422 + data = response.get_json() + assert ( + "Cannot delete theme" in data["message"] + or "system" in data["message"].lower() + ) + + def test_gamma_user_can_read_themes(self): + """Test that gamma users can read themes""" + # Login as gamma user + self.login(GAMMA_USERNAME) + + # Should be able to read themes + response = self.client.get("/api/v1/theme/") + assert response.status_code == 200 + + # Should be able to read individual theme + response = self.client.get(f"/api/v1/theme/{self.regular_theme.id}") + assert response.status_code == 200 + + # Note: Gamma users' ability to create/update/delete themes + # depends on the specific permissions configuration diff --git a/tests/unit_tests/commands/theme/test_set_system_theme.py b/tests/unit_tests/commands/theme/test_set_system_theme.py new file mode 100644 index 00000000000..0645409d085 --- /dev/null +++ b/tests/unit_tests/commands/theme/test_set_system_theme.py @@ -0,0 +1,231 @@ +# 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. + +from unittest.mock import MagicMock, patch + +import pytest + +from superset.commands.theme.exceptions import ThemeNotFoundError +from superset.commands.theme.set_system_theme import ( + ClearSystemDarkThemeCommand, + ClearSystemDefaultThemeCommand, + SetSystemDarkThemeCommand, + SetSystemDefaultThemeCommand, +) +from superset.models.core import Theme + + +class TestSetSystemDefaultThemeCommand: + """Test SetSystemDefaultThemeCommand""" + + @patch("superset.commands.theme.set_system_theme.db.session") + @patch("superset.commands.theme.set_system_theme.ThemeDAO") + def test_set_system_default_success(self, mock_dao, mock_session): + """Test successfully setting a theme as system default""" + # Create a mock theme + mock_theme = MagicMock(spec=Theme) + mock_theme.id = 1 + mock_dao.find_by_id.return_value = mock_theme + + # Mock the update query + mock_execute = MagicMock() + mock_session.execute.return_value = mock_execute + + # Create and run command + command = SetSystemDefaultThemeCommand(1) + result = command.run() + + # Verify the theme was found + mock_dao.find_by_id.assert_called_once_with(1) + + # Verify all existing system defaults were cleared + mock_session.execute.assert_called_once() + + # Verify the theme was set as system default + assert mock_theme.is_system_default is True + + # Verify the theme was added to session and committed + mock_session.add.assert_called_once_with(mock_theme) + mock_session.commit.assert_called_once() + + # Verify the result + assert result == mock_theme + + @patch("superset.commands.theme.set_system_theme.ThemeDAO") + def test_set_system_default_theme_not_found(self, mock_dao): + """Test setting system default when theme doesn't exist""" + # Mock theme not found + mock_dao.find_by_id.return_value = None + + # Create command and expect exception + command = SetSystemDefaultThemeCommand(999) + + with pytest.raises(ThemeNotFoundError): + command.run() + + # Verify the theme was searched for + mock_dao.find_by_id.assert_called_once_with(999) + + +class TestSetSystemDarkThemeCommand: + """Test SetSystemDarkThemeCommand""" + + @patch("superset.commands.theme.set_system_theme.db.session") + @patch("superset.commands.theme.set_system_theme.ThemeDAO") + def test_set_system_dark_success(self, mock_dao, mock_session): + """Test successfully setting a theme as system dark""" + # Create a mock theme + mock_theme = MagicMock(spec=Theme) + mock_theme.id = 2 + mock_dao.find_by_id.return_value = mock_theme + + # Mock the update query + mock_execute = MagicMock() + mock_session.execute.return_value = mock_execute + + # Create and run command + command = SetSystemDarkThemeCommand(2) + result = command.run() + + # Verify the theme was found + mock_dao.find_by_id.assert_called_once_with(2) + + # Verify all existing system dark themes were cleared + mock_session.execute.assert_called_once() + + # Verify the theme was set as system dark + assert mock_theme.is_system_dark is True + + # Verify the theme was added to session and committed + mock_session.add.assert_called_once_with(mock_theme) + mock_session.commit.assert_called_once() + + # Verify the result + assert result == mock_theme + + @patch("superset.commands.theme.set_system_theme.ThemeDAO") + def test_set_system_dark_theme_not_found(self, mock_dao): + """Test setting system dark when theme doesn't exist""" + # Mock theme not found + mock_dao.find_by_id.return_value = None + + # Create command and expect exception + command = SetSystemDarkThemeCommand(999) + + with pytest.raises(ThemeNotFoundError): + command.run() + + # Verify the theme was searched for + mock_dao.find_by_id.assert_called_once_with(999) + + +class TestClearSystemDefaultThemeCommand: + """Test ClearSystemDefaultThemeCommand""" + + @patch("superset.commands.theme.set_system_theme.db.session") + def test_clear_system_default_success(self, mock_session): + """Test successfully clearing system default theme""" + # Mock the update query + mock_execute = MagicMock() + mock_session.execute.return_value = mock_execute + + # Create and run command + command = ClearSystemDefaultThemeCommand() + command.run() + + # Verify the update was executed + mock_session.execute.assert_called_once() + + # Verify commit was called + mock_session.commit.assert_called_once() + + +class TestClearSystemDarkThemeCommand: + """Test ClearSystemDarkThemeCommand""" + + @patch("superset.commands.theme.set_system_theme.db.session") + def test_clear_system_dark_success(self, mock_session): + """Test successfully clearing system dark theme""" + # Mock the update query + mock_execute = MagicMock() + mock_session.execute.return_value = mock_execute + + # Create and run command + command = ClearSystemDarkThemeCommand() + command.run() + + # Verify the update was executed + mock_session.execute.assert_called_once() + + # Verify commit was called + mock_session.commit.assert_called_once() + + +class TestSystemThemeConstraints: + """Test system theme uniqueness constraints""" + + @patch("superset.commands.theme.set_system_theme.db.session") + @patch("superset.commands.theme.set_system_theme.ThemeDAO") + def test_only_one_system_default(self, mock_dao, mock_session): + """Test that setting a new system default clears all others""" + # Create mock themes + mock_theme_new = MagicMock(spec=Theme) + mock_theme_new.id = 3 + mock_dao.find_by_id.return_value = mock_theme_new + + # Mock update to track the SQL + mock_update = MagicMock() + mock_session.execute.return_value = mock_update + + # Create and run command + command = SetSystemDefaultThemeCommand(3) + command.run() + + # Verify the SQL update was called to clear all is_system_default flags + mock_session.execute.assert_called_once() + + # Get the actual SQL expression that was executed + sql_expr = mock_session.execute.call_args[0][0] + + # Verify it's an update statement (can't directly check SQL content with mocks) + assert sql_expr is not None + + @patch("superset.commands.theme.set_system_theme.db.session") + @patch("superset.commands.theme.set_system_theme.ThemeDAO") + def test_only_one_system_dark(self, mock_dao, mock_session): + """Test that setting a new system dark clears all others""" + # Create mock themes + mock_theme_new = MagicMock(spec=Theme) + mock_theme_new.id = 4 + mock_dao.find_by_id.return_value = mock_theme_new + + # Mock update to track the SQL + mock_update = MagicMock() + mock_session.execute.return_value = mock_update + + # Create and run command + command = SetSystemDarkThemeCommand(4) + command.run() + + # Verify the SQL update was called to clear all is_system_dark flags + mock_session.execute.assert_called_once() + + # Get the actual SQL expression that was executed + sql_expr = mock_session.execute.call_args[0][0] + + # Verify it's an update statement (can't directly check SQL content with mocks) + assert sql_expr is not None diff --git a/tests/unit_tests/daos/test_theme_dao.py b/tests/unit_tests/daos/test_theme_dao.py new file mode 100644 index 00000000000..546dee7991a --- /dev/null +++ b/tests/unit_tests/daos/test_theme_dao.py @@ -0,0 +1,233 @@ +# 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. + +from unittest.mock import MagicMock, patch + +from superset.daos.theme import ThemeDAO +from superset.models.core import Theme + + +class TestThemeDAO: + """Test ThemeDAO functionality""" + + @patch("superset.daos.theme.db.session") + def test_find_system_default_single(self, mock_session): + """Test finding system default theme when exactly one exists""" + # Create a mock theme + mock_theme = MagicMock(spec=Theme) + mock_theme.is_system_default = True + + # Mock the query chain + mock_query = MagicMock() + mock_session.query.return_value = mock_query + mock_filter = MagicMock() + mock_query.filter.return_value = mock_filter + mock_filter.all.return_value = [mock_theme] + + # Call the method + result = ThemeDAO.find_system_default() + + # Verify the result + assert result == mock_theme + + @patch("superset.daos.theme.db.session") + @patch("superset.daos.theme.logger") + def test_find_system_default_multiple(self, mock_logger, mock_session): + """Test finding system default theme when multiple exist""" + # Create mock themes + mock_theme1 = MagicMock(spec=Theme) + mock_theme1.is_system_default = True + mock_theme2 = MagicMock(spec=Theme) + mock_theme2.is_system_default = True + + # Create mock fallback theme + mock_fallback = MagicMock(spec=Theme) + mock_fallback.is_system = True + mock_fallback.theme_name = "THEME_DEFAULT" + + # Mock the query chains - need separate mocks for each query call + mock_query1 = MagicMock() + mock_query2 = MagicMock() + mock_session.query.side_effect = [mock_query1, mock_query2] + + # First query returns multiple themes + mock_filter1 = MagicMock() + mock_query1.filter.return_value = mock_filter1 + mock_filter1.all.return_value = [mock_theme1, mock_theme2] + + # Second query returns fallback theme + mock_filter2 = MagicMock() + mock_query2.filter.return_value = mock_filter2 + mock_filter2.first.return_value = mock_fallback + + # Call the method + result = ThemeDAO.find_system_default() + + # Verify warning was logged + mock_logger.warning.assert_called_once() + assert ( + "Multiple system default themes found (2)" + in mock_logger.warning.call_args[0][0] + ) + + # Verify the result is the fallback theme + assert result == mock_fallback + + @patch("superset.daos.theme.db.session") + def test_find_system_default_none(self, mock_session): + """Test finding system default theme when none exist""" + # Create mock fallback theme + mock_fallback = MagicMock(spec=Theme) + mock_fallback.is_system = True + mock_fallback.theme_name = "THEME_DEFAULT" + + # Mock the query chains - need separate mocks for each query call + mock_query1 = MagicMock() + mock_query2 = MagicMock() + mock_session.query.side_effect = [mock_query1, mock_query2] + + # First query returns no themes + mock_filter1 = MagicMock() + mock_query1.filter.return_value = mock_filter1 + mock_filter1.all.return_value = [] + + # Second query returns fallback theme + mock_filter2 = MagicMock() + mock_query2.filter.return_value = mock_filter2 + mock_filter2.first.return_value = mock_fallback + + # Call the method + result = ThemeDAO.find_system_default() + + # Verify the result is the fallback theme + assert result == mock_fallback + + @patch("superset.daos.theme.db.session") + def test_find_system_dark_single(self, mock_session): + """Test finding system dark theme when exactly one exists""" + # Create a mock theme + mock_theme = MagicMock(spec=Theme) + mock_theme.is_system_dark = True + + # Mock the query chain + mock_query = MagicMock() + mock_session.query.return_value = mock_query + mock_filter = MagicMock() + mock_query.filter.return_value = mock_filter + mock_filter.all.return_value = [mock_theme] + + # Call the method + result = ThemeDAO.find_system_dark() + + # Verify the result + assert result == mock_theme + + @patch("superset.daos.theme.db.session") + @patch("superset.daos.theme.logger") + def test_find_system_dark_multiple(self, mock_logger, mock_session): + """Test finding system dark theme when multiple exist""" + # Create mock themes + mock_theme1 = MagicMock(spec=Theme) + mock_theme1.is_system_dark = True + mock_theme2 = MagicMock(spec=Theme) + mock_theme2.is_system_dark = True + + # Create mock fallback theme + mock_fallback = MagicMock(spec=Theme) + mock_fallback.is_system = True + mock_fallback.theme_name = "THEME_DARK" + + # Mock the query chains - need separate mocks for each query call + mock_query1 = MagicMock() + mock_query2 = MagicMock() + mock_session.query.side_effect = [mock_query1, mock_query2] + + # First query returns multiple themes + mock_filter1 = MagicMock() + mock_query1.filter.return_value = mock_filter1 + mock_filter1.all.return_value = [mock_theme1, mock_theme2] + + # Second query returns fallback theme + mock_filter2 = MagicMock() + mock_query2.filter.return_value = mock_filter2 + mock_filter2.first.return_value = mock_fallback + + # Call the method + result = ThemeDAO.find_system_dark() + + # Verify warning was logged + mock_logger.warning.assert_called_once() + assert ( + "Multiple system dark themes found (2)" + in mock_logger.warning.call_args[0][0] + ) + + # Verify the result is the fallback theme + assert result == mock_fallback + + @patch("superset.daos.theme.db.session") + def test_find_system_dark_none_with_fallback(self, mock_session): + """Test finding system dark theme when none exist but fallback does""" + # Create mock fallback theme + mock_fallback = MagicMock(spec=Theme) + mock_fallback.is_system = True + mock_fallback.theme_name = "THEME_DARK" + + # Mock the query chains - need separate mocks for each query call + mock_query1 = MagicMock() + mock_query2 = MagicMock() + mock_session.query.side_effect = [mock_query1, mock_query2] + + # First query returns no themes + mock_filter1 = MagicMock() + mock_query1.filter.return_value = mock_filter1 + mock_filter1.all.return_value = [] + + # Second query returns fallback theme + mock_filter2 = MagicMock() + mock_query2.filter.return_value = mock_filter2 + mock_filter2.first.return_value = mock_fallback + + # Call the method + result = ThemeDAO.find_system_dark() + + # Verify the result is the fallback theme + assert result == mock_fallback + + @patch("superset.daos.theme.db.session") + def test_find_system_dark_none_without_fallback(self, mock_session): + """Test finding system dark theme when none exist and no fallback""" + # Mock the query chains - need separate mocks for each query call + mock_query1 = MagicMock() + mock_query2 = MagicMock() + mock_session.query.side_effect = [mock_query1, mock_query2] + + # First query returns no themes + mock_filter1 = MagicMock() + mock_query1.filter.return_value = mock_filter1 + mock_filter1.all.return_value = [] + + # Second query returns no fallback + mock_filter2 = MagicMock() + mock_query2.filter.return_value = mock_filter2 + mock_filter2.first.return_value = None + + # Call the method + result = ThemeDAO.find_system_dark() + + # Verify the result is None + assert result is None diff --git a/tests/unit_tests/themes/api_test.py b/tests/unit_tests/themes/api_test.py index 75d94f9b40a..fad439fcceb 100644 --- a/tests/unit_tests/themes/api_test.py +++ b/tests/unit_tests/themes/api_test.py @@ -58,10 +58,12 @@ class TestThemeRestApi: "json_data", "id", "is_system", + "is_system_default", + "is_system_dark", "theme_name", "uuid", ] - assert ThemeRestApi.show_columns == expected_columns + assert set(ThemeRestApi.show_columns) == set(expected_columns) def test_list_columns_configuration(self): """Test that list columns are configured correctly""" @@ -78,10 +80,12 @@ class TestThemeRestApi: "json_data", "id", "is_system", + "is_system_default", + "is_system_dark", "theme_name", "uuid", ] - assert ThemeRestApi.list_columns == expected_columns + assert set(ThemeRestApi.list_columns) == set(expected_columns) def test_order_columns_configuration(self): """Test that order columns are configured correctly""" diff --git a/tests/unit_tests/themes/test_utils.py b/tests/unit_tests/themes/test_utils.py index 01ec1eb5139..b576604a998 100644 --- a/tests/unit_tests/themes/test_utils.py +++ b/tests/unit_tests/themes/test_utils.py @@ -18,12 +18,11 @@ import pytest -from superset.themes.types import ThemeMode, ThemeSettingsKey +from superset.themes.types import ThemeMode from superset.themes.utils import ( _is_valid_algorithm, _is_valid_theme_mode, is_valid_theme, - is_valid_theme_settings, ) @@ -76,18 +75,3 @@ def test_is_valid_algorithm(algorithm, expected): ) def test_is_valid_theme(theme, expected): assert is_valid_theme(theme) is expected - - -@pytest.mark.parametrize( - "settings, expected", - [ - ([], False), # not a dict - ("string", False), - ({}, True), # empty - ({key.value: True for key in ThemeSettingsKey}, True), - ({"enforced": True, "foo": False}, False), # invalid key - ({"enforced": "yes"}, False), # invalid value type - ], -) -def test_is_valid_theme_settings(settings, expected): - assert is_valid_theme_settings(settings) is expected