diff --git a/.gitignore b/.gitignore index c9699e830dc..644835a533a 100644 --- a/.gitignore +++ b/.gitignore @@ -133,6 +133,7 @@ CLAUDE.local.md PROJECT.md .aider* .claude_rc* +.claude/settings.local.json .env.local oxc-custom-build/ *.code-workspace diff --git a/superset-frontend/playwright/helpers/api/dashboard.ts b/superset-frontend/playwright/helpers/api/dashboard.ts index 52314a31c99..2eb8d092c0d 100644 --- a/superset-frontend/playwright/helpers/api/dashboard.ts +++ b/superset-frontend/playwright/helpers/api/dashboard.ts @@ -44,6 +44,7 @@ export interface DashboardCreatePayload { css?: string; json_metadata?: string; published?: boolean; + theme_id?: number; } /** diff --git a/superset-frontend/playwright/helpers/api/theme.ts b/superset-frontend/playwright/helpers/api/theme.ts new file mode 100644 index 00000000000..1f0152fd15a --- /dev/null +++ b/superset-frontend/playwright/helpers/api/theme.ts @@ -0,0 +1,62 @@ +/** + * 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 { Page, APIResponse } from '@playwright/test'; +import { apiPost, apiDelete, ApiRequestOptions } from './requests'; + +export const ENDPOINTS = { + THEME: 'api/v1/theme/', +} as const; + +/** + * TypeScript interface for theme creation API payload. + * Both fields are required (ThemePostSchema). + */ +export interface ThemeCreatePayload { + theme_name: string; + json_data: string; +} + +/** + * POST request to create a theme + * @param page - Playwright page instance (provides authentication context) + * @param requestBody - Theme configuration object + * @returns API response from theme creation + */ +export async function apiPostTheme( + page: Page, + requestBody: ThemeCreatePayload, +): Promise { + return apiPost(page, ENDPOINTS.THEME, requestBody); +} + +/** + * DELETE request to remove a theme + * @param page - Playwright page instance (provides authentication context) + * @param themeId - ID of the theme to delete + * @param options - Optional request options + * @returns API response from theme deletion + */ +export async function apiDeleteTheme( + page: Page, + themeId: number, + options?: ApiRequestOptions, +): Promise { + return apiDelete(page, `${ENDPOINTS.THEME}${themeId}`, options); +} diff --git a/superset-frontend/playwright/helpers/fixtures/testAssets.ts b/superset-frontend/playwright/helpers/fixtures/testAssets.ts index ada44e5ae55..7a4f0f10c07 100644 --- a/superset-frontend/playwright/helpers/fixtures/testAssets.ts +++ b/superset-frontend/playwright/helpers/fixtures/testAssets.ts @@ -21,6 +21,7 @@ import { test as base } from '@playwright/test'; import { apiDeleteChart } from '../api/chart'; import { apiDeleteDashboard } from '../api/dashboard'; import { apiDeleteDataset } from '../api/dataset'; +import { apiDeleteTheme } from '../api/theme'; import { apiDeleteDatabase } from '../api/database'; /** @@ -31,6 +32,7 @@ export interface TestAssets { trackDashboard(id: number): void; trackChart(id: number): void; trackDataset(id: number): void; + trackTheme(id: number): void; trackDatabase(id: number): void; } @@ -42,16 +44,18 @@ export const test = base.extend<{ testAssets: TestAssets }>({ const dashboardIds = new Set(); const chartIds = new Set(); const datasetIds = new Set(); + const themeIds = new Set(); const databaseIds = new Set(); await use({ trackDashboard: id => dashboardIds.add(id), trackChart: id => chartIds.add(id), trackDataset: id => datasetIds.add(id), + trackTheme: id => themeIds.add(id), trackDatabase: id => databaseIds.add(id), }); - // Cleanup order: dashboards → charts → datasets → databases (respects FK dependencies) + // Cleanup order: dashboards → charts → datasets → themes → databases (respects FK dependencies) // Use failOnStatusCode: false to avoid throwing on 404 (resource already deleted by test) // Warn on unexpected status codes (401/403/500) that may indicate leaked state await Promise.all( @@ -105,6 +109,21 @@ export const test = base.extend<{ testAssets: TestAssets }>({ }), ), ); + await Promise.all( + [...themeIds].map(id => + apiDeleteTheme(page, id, { failOnStatusCode: false }) + .then(response => { + if (!EXPECTED_CLEANUP_STATUSES.has(response.status())) { + console.warn( + `[testAssets] Unexpected status ${response.status()} cleaning up theme ${id}`, + ); + } + }) + .catch(error => { + console.warn(`[testAssets] Failed to cleanup theme ${id}:`, error); + }), + ), + ); await Promise.all( [...databaseIds].map(id => apiDeleteDatabase(page, id, { failOnStatusCode: false }) diff --git a/superset-frontend/playwright/tests/experimental/dashboard/theme.spec.ts b/superset-frontend/playwright/tests/experimental/dashboard/theme.spec.ts new file mode 100644 index 00000000000..edec236e6ca --- /dev/null +++ b/superset-frontend/playwright/tests/experimental/dashboard/theme.spec.ts @@ -0,0 +1,152 @@ +/** + * 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 { test, expect } from '@playwright/test'; +import { AuthPage } from '../../../pages/AuthPage'; +import { DashboardPage } from '../../../pages/DashboardPage'; +import { apiPostTheme, apiDeleteTheme } from '../../../helpers/api/theme'; +import { + apiPostDashboard, + apiPutDashboard, + apiDeleteDashboard, +} from '../../../helpers/api/dashboard'; +import { apiGet } from '../../../helpers/api/requests'; +import { TIMEOUT } from '../../../utils/constants'; + +/** + * Dashboard Theme E2E tests. + * + * Prerequisites: + * - Superset running with example data loaded + * - Admin user authenticated (via global-setup) + * - Non-admin test user created by `superset load-test-users` + * + * Credentials are configurable via environment variables: + * - PLAYWRIGHT_NONADMIN_USERNAME (default: 'gamma') + * - PLAYWRIGHT_NONADMIN_PASSWORD (default: 'general') + */ + +const NONADMIN_USERNAME = process.env.PLAYWRIGHT_NONADMIN_USERNAME || 'gamma'; +const NONADMIN_PASSWORD = process.env.PLAYWRIGHT_NONADMIN_PASSWORD || 'general'; + +// Clear storageState so the default page fixture starts unauthenticated. +// Admin API calls use an explicit admin context with saved auth instead. +test.use({ storageState: { cookies: [], origins: [] } }); + +test('non-admin user can view a themed dashboard without 403 or infinite spinner', async ({ + page, + browser, +}) => { + test.setTimeout(60_000); + + // --- ADMIN SETUP (explicit admin context with saved auth) --- + const adminContext = await browser.newContext({ + baseURL: test.info().project.use.baseURL, + storageState: 'playwright/.auth/user.json', + }); + const adminPage = await adminContext.newPage(); + + let themeId: number | undefined; + let dashboardId: number | undefined; + + try { + // 1. Create a theme + const themeRes = await apiPostTheme(adminPage, { + theme_name: `e2e_theme_test_${Date.now()}`, + json_data: '{}', + }); + expect(themeRes.ok()).toBe(true); + const themeBody = await themeRes.json(); + themeId = themeBody.id; + expect(themeId).toBeTruthy(); + + // 2. Create a published dashboard with the theme assigned + const dashRes = await apiPostDashboard(adminPage, { + dashboard_title: `E2E Theme Test ${Date.now()}`, + published: true, + theme_id: themeId, + }); + expect(dashRes.ok()).toBe(true); + const dashBody = await dashRes.json(); + dashboardId = dashBody.id; + expect(dashboardId).toBeTruthy(); + + // 3. Grant non-admin user access by adding them as a dashboard owner + const usersRes = await apiGet( + adminPage, + `api/v1/dashboard/related/owners?q=(filter:'${NONADMIN_USERNAME}')`, + ); + expect(usersRes.ok()).toBe(true); + const usersBody = await usersRes.json(); + const gammaUserId = usersBody.result?.[0]?.value; + expect(gammaUserId).toBeTruthy(); + const putRes = await apiPutDashboard(adminPage, dashboardId!, { + owners: [gammaUserId], + }); + expect(putRes.ok()).toBe(true); + + // --- NON-ADMIN USER PHASE (page has no cached auth via test.use) --- + + // 4. Instrument network: track any /api/v1/theme/ requests and 403 responses + const themeApiRequests: string[] = []; + const forbiddenResponses: string[] = []; + page.on('response', response => { + const url = response.url(); + if (url.includes('/api/v1/theme/')) { + themeApiRequests.push(url); + } + if (response.status() === 403 && url.includes('/api/v1/theme/')) { + forbiddenResponses.push(url); + } + }); + + // 5. Login as non-admin user + const authPage = new AuthPage(page); + await authPage.goto(); + await authPage.waitForLoginForm(); + await authPage.loginWithCredentials(NONADMIN_USERNAME, NONADMIN_PASSWORD); + await authPage.waitForLoginSuccess(); + + // 6. Navigate to the themed dashboard + const dashboardPage = new DashboardPage(page); + await dashboardPage.gotoById(dashboardId!); + + // 7. Assert dashboard fully loads (not stuck on infinite spinner) + await dashboardPage.waitForLoad({ timeout: TIMEOUT.PAGE_LOAD }); + await dashboardPage.waitForChartsToLoad(); + + // 8. Assert no /api/v1/theme/ requests were made (theme data comes from dashboard response) + expect(themeApiRequests).toHaveLength(0); + // Assert no 403 responses on /api/v1/theme/ (scoped to avoid login/unrelated 403 noise) + expect(forbiddenResponses).toHaveLength(0); + } finally { + // Cleanup: delete test resources using admin context + if (dashboardId) { + await apiDeleteDashboard(adminPage, dashboardId, { + failOnStatusCode: false, + }).catch(() => {}); + } + if (themeId) { + await apiDeleteTheme(adminPage, themeId, { + failOnStatusCode: false, + }).catch(() => {}); + } + await adminContext.close(); + } +}); diff --git a/superset-frontend/src/components/CrudThemeProvider.test.tsx b/superset-frontend/src/components/CrudThemeProvider.test.tsx new file mode 100644 index 00000000000..cd61fbb2999 --- /dev/null +++ b/superset-frontend/src/components/CrudThemeProvider.test.tsx @@ -0,0 +1,326 @@ +/** + * 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 { type ReactNode } from 'react'; +import { render, screen } from 'spec/helpers/testing-library'; +import { logging } from '@apache-superset/core/utils'; +import { + Theme, + normalizeThemeConfig, + isThemeConfigDark, +} from '@apache-superset/core/theme'; +import getBootstrapData from 'src/utils/getBootstrapData'; +import CrudThemeProvider from './CrudThemeProvider'; + +jest.mock('@apache-superset/core/theme', () => ({ + ...jest.requireActual('@apache-superset/core/theme'), + normalizeThemeConfig: jest.fn((config: unknown) => config), + isThemeConfigDark: jest.fn(() => false), +})); + +jest.mock('src/utils/getBootstrapData', () => ({ + __esModule: true, + default: jest.fn(() => ({ + common: { theme: { default: {}, dark: {} } }, + })), +})); + +const mockGetBootstrapData = getBootstrapData as jest.MockedFunction< + typeof getBootstrapData +>; +const mockNormalizeThemeConfig = normalizeThemeConfig as jest.MockedFunction< + typeof normalizeThemeConfig +>; +const mockIsThemeConfigDark = isThemeConfigDark as jest.MockedFunction< + typeof isThemeConfigDark +>; + +type BootstrapData = ReturnType; +type BootstrapThemes = BootstrapData['common']['theme']; + +function mockBootstrap(themes: Partial = {}): BootstrapData { + return { + common: { + theme: { default: {}, dark: {}, ...themes }, + }, + } as unknown as BootstrapData; +} + +const MockSupersetThemeProvider = ({ children }: { children: ReactNode }) => ( +
{children}
+); + +beforeEach(() => { + jest.restoreAllMocks(); + jest.spyOn(Theme, 'fromConfig').mockReturnValue({ + SupersetThemeProvider: MockSupersetThemeProvider, + } as unknown as Theme); + mockNormalizeThemeConfig.mockImplementation( + config => config as ReturnType, + ); + mockIsThemeConfigDark.mockReturnValue(false); + mockGetBootstrapData.mockReturnValue(mockBootstrap()); + // Clean up font style elements from previous tests + document + .querySelectorAll('style[data-superset-fonts]') + .forEach(el => el.remove()); +}); + +test('renders children directly when no theme is provided', () => { + render( + +
Dashboard Content
+ , + ); + expect(screen.getByText('Dashboard Content')).toBeInTheDocument(); + expect( + screen.queryByTestId('dashboard-theme-provider'), + ).not.toBeInTheDocument(); + expect(Theme.fromConfig).not.toHaveBeenCalled(); +}); + +test('renders children directly when theme is null', () => { + render( + +
Dashboard Content
+ , + ); + expect(screen.getByText('Dashboard Content')).toBeInTheDocument(); + expect( + screen.queryByTestId('dashboard-theme-provider'), + ).not.toBeInTheDocument(); + expect(Theme.fromConfig).not.toHaveBeenCalled(); +}); + +test('wraps children with SupersetThemeProvider when valid theme data is provided', () => { + const themeConfig = { token: { colorPrimary: '#ff0000' } }; + render( + +
Dashboard Content
+ , + ); + expect(screen.getByText('Dashboard Content')).toBeInTheDocument(); + expect(screen.getByTestId('dashboard-theme-provider')).toBeInTheDocument(); + expect(Theme.fromConfig).toHaveBeenCalledWith(themeConfig, expect.anything()); +}); + +test('creates theme from inline json_data via Theme.fromConfig', () => { + const themeConfig = { token: { colorPrimary: '#1677ff' } }; + render( + +
Dashboard Content
+ , + ); + expect(screen.getByText('Dashboard Content')).toBeInTheDocument(); + // No API call can happen — CrudThemeProvider has no fetch/SupersetClient imports. + // Theme is created synchronously from the inline json_data prop. + expect(Theme.fromConfig).toHaveBeenCalledWith(themeConfig, expect.anything()); +}); + +test('falls back to rendering children without theme wrapper when json_data is invalid JSON', () => { + jest.spyOn(logging, 'warn').mockImplementation(); + render( + +
Dashboard Content
+ , + ); + expect(screen.getByText('Dashboard Content')).toBeInTheDocument(); + expect( + screen.queryByTestId('dashboard-theme-provider'), + ).not.toBeInTheDocument(); + expect(logging.warn).toHaveBeenCalled(); +}); + +test('falls back to rendering children without theme wrapper when Theme.fromConfig throws', () => { + (Theme.fromConfig as jest.Mock).mockImplementation(() => { + throw new Error('Invalid theme config'); + }); + jest.spyOn(logging, 'warn').mockImplementation(); + render( + +
Dashboard Content
+ , + ); + expect(screen.getByText('Dashboard Content')).toBeInTheDocument(); + expect( + screen.queryByTestId('dashboard-theme-provider'), + ).not.toBeInTheDocument(); + expect(logging.warn).toHaveBeenCalled(); +}); + +test('merges dashboard theme with default base theme from bootstrap data', () => { + const bootstrapDefault = { token: { colorPrimary: '#base-default' } }; + mockGetBootstrapData.mockReturnValue( + mockBootstrap({ default: bootstrapDefault }), + ); + mockIsThemeConfigDark.mockReturnValue(false); + + const themeConfig = { token: { colorPrimary: '#custom' } }; + render( + +
Dashboard Content
+ , + ); + + // Theme.fromConfig should be called with TWO args: normalized config + base theme + expect(Theme.fromConfig).toHaveBeenCalledWith( + expect.objectContaining({ + token: expect.objectContaining({ colorPrimary: '#custom' }), + }), + bootstrapDefault, + ); +}); + +test('uses dark base theme when dashboard theme config is dark', () => { + const bootstrapDark = { + algorithm: 'dark', + token: { colorPrimary: '#base-dark' }, + }; + mockGetBootstrapData.mockReturnValue(mockBootstrap({ dark: bootstrapDark })); + mockIsThemeConfigDark.mockReturnValue(true); + + const themeConfig = { + algorithm: 'dark', + token: { colorPrimary: '#custom-dark' }, + }; + render( + +
Dashboard Content
+ , + ); + + expect(Theme.fromConfig).toHaveBeenCalledWith(themeConfig, bootstrapDark); +}); + +test('injects font URLs as CSS @import rules', () => { + const fontUrl = 'https://fonts.example.com/custom.css'; + const themeConfig = { + token: { colorPrimary: '#ff0000', fontUrls: [fontUrl] }, + }; + render( + +
Dashboard Content
+ , + ); + + const fontStyle = document.querySelector('style[data-superset-fonts]'); + expect(fontStyle).not.toBeNull(); + expect(fontStyle?.textContent).toContain(`@import url("${fontUrl}")`); +}); + +test('does not inject fonts when Theme.fromConfig throws even if fontUrls are present', () => { + (Theme.fromConfig as jest.Mock).mockImplementation(() => { + throw new Error('Invalid theme config'); + }); + jest.spyOn(logging, 'warn').mockImplementation(); + const themeConfig = { + token: { + colorPrimary: '#ff0000', + fontUrls: ['https://fonts.example.com/custom.css'], + }, + }; + render( + +
Dashboard Content
+ , + ); + + const fontStyle = document.querySelector('style[data-superset-fonts]'); + expect(fontStyle).toBeNull(); + expect(logging.warn).toHaveBeenCalled(); +}); + +test('ignores non-array fontUrls in theme config without throwing', () => { + const themeConfig = { + token: { colorPrimary: '#ff0000', fontUrls: 'not-an-array' }, + }; + render( + +
Dashboard Content
+ , + ); + + expect(screen.getByText('Dashboard Content')).toBeInTheDocument(); + expect(screen.getByTestId('dashboard-theme-provider')).toBeInTheDocument(); + const fontStyle = document.querySelector('style[data-superset-fonts]'); + expect(fontStyle).toBeNull(); +}); + +test('does not inject font style element when no fontUrls in config', () => { + const themeConfig = { token: { colorPrimary: '#ff0000' } }; + render( + +
Dashboard Content
+ , + ); + + const fontStyle = document.querySelector('style[data-superset-fonts]'); + expect(fontStyle).toBeNull(); +}); diff --git a/superset-frontend/src/components/CrudThemeProvider.tsx b/superset-frontend/src/components/CrudThemeProvider.tsx index 506d5a64688..d6ed28d0e89 100644 --- a/superset-frontend/src/components/CrudThemeProvider.tsx +++ b/superset-frontend/src/components/CrudThemeProvider.tsx @@ -16,68 +16,80 @@ * specific language governing permissions and limitations * under the License. */ -import { ReactNode, useEffect, useState } from 'react'; -import { useThemeContext } from 'src/theme/ThemeProvider'; -import { Theme } from '@apache-superset/core/theme'; -import { Loading } from '@superset-ui/core/components'; +import { ReactNode, useEffect, useMemo } from 'react'; +import { logging } from '@apache-superset/core/utils'; +import { + Theme, + normalizeThemeConfig, + isThemeConfigDark, +} from '@apache-superset/core/theme'; +import getBootstrapData from 'src/utils/getBootstrapData'; +import type { Dashboard } from 'src/types/Dashboard'; interface CrudThemeProviderProps { children: ReactNode; - themeId?: number | null; + theme?: Dashboard['theme']; } /** - * CrudThemeProvider asks the ThemeController for a dashboard theme provider. - * Flow: Dashboard loads → asks controller → controller fetches theme → - * returns provider → dashboard uses it. - * - * CRITICAL: This does NOT modify the global controller - it creates an isolated dashboard theme. + * CrudThemeProvider applies a dashboard-specific theme using theme data + * from the dashboard API response. Merges with the system's base theme + * (light or dark) and loads custom fonts. Falls back to the global theme + * if the theme data is missing or invalid. */ export default function CrudThemeProvider({ children, - themeId, + theme, }: CrudThemeProviderProps) { - const globalThemeContext = useThemeContext(); - const [dashboardTheme, setDashboardTheme] = useState(null); + const { dashboardTheme, fontUrls } = useMemo(() => { + if (!theme?.json_data) { + return { dashboardTheme: null, fontUrls: undefined }; + } + try { + const themeConfig = JSON.parse(theme.json_data); + const normalizedConfig = normalizeThemeConfig(themeConfig); + const isDark = isThemeConfigDark(normalizedConfig); + const { + common: { theme: bootstrapTheme }, + } = getBootstrapData(); + const baseTheme = isDark ? bootstrapTheme.dark : bootstrapTheme.default; + const createdTheme = Theme.fromConfig( + normalizedConfig, + baseTheme || undefined, + ); + const rawUrls = themeConfig?.token?.fontUrls; + const urls = Array.isArray(rawUrls) ? (rawUrls as string[]) : undefined; + return { dashboardTheme: createdTheme, fontUrls: urls }; + } catch (error) { + logging.warn('Failed to load dashboard theme:', error); + return { dashboardTheme: null, fontUrls: undefined }; + } + }, [theme?.json_data]); useEffect(() => { - if (themeId) { - // Ask the controller to create a SEPARATE dashboard theme provider - // This should NOT affect the global controller or navbar - const loadDashboardTheme = async () => { - try { - const dashboardThemeProvider = - await globalThemeContext.createDashboardThemeProvider( - String(themeId), - ); - setDashboardTheme(dashboardThemeProvider); - } catch (error) { - console.error('Failed to load dashboard theme:', error); - setDashboardTheme(null); - } - }; + if (!dashboardTheme || !fontUrls?.length) return undefined; - loadDashboardTheme(); - } else { - setDashboardTheme(null); - } - }, [themeId, globalThemeContext]); + // JSON.stringify provides safe escaping to prevent CSS injection + const css = fontUrls + .map((url: string) => `@import url(${JSON.stringify(url)});`) + .join('\n'); + const style = document.createElement('style'); + style.setAttribute('data-superset-fonts', 'true'); + style.textContent = css; + document.head.appendChild(style); - // If no themeId, just render children (they use global theme) - if (!themeId) { + return () => { + style.remove(); + }; + }, [dashboardTheme, fontUrls]); + + if (!dashboardTheme) { return <>{children}; } - // If themeId exists, but theme is not loaded yet, return null to prevent re-mounting children - if (!dashboardTheme) { - return ; - } - - // Render children with the dashboard theme provider from controller return ( {children} ); } -// test comment diff --git a/superset-frontend/src/dashboard/components/Header/index.tsx b/superset-frontend/src/dashboard/components/Header/index.tsx index 417c332fd2a..522a22f1eca 100644 --- a/superset-frontend/src/dashboard/components/Header/index.tsx +++ b/superset-frontend/src/dashboard/components/Header/index.tsx @@ -107,7 +107,7 @@ type DashboardPropertiesUpdate = { owners?: Owner[]; roles?: Role[]; tags?: TagType[]; - themeId?: number | null; + theme?: { id: number; theme_name: string; json_data: string } | null; css?: string; title?: string; }; @@ -561,7 +561,10 @@ const Header = (): JSX.Element => { owners: updates.owners, roles: updates.roles, tags: updates.tags, - theme_id: updates.themeId, + // Conditional spread: omit `theme` key entirely when undefined + // to prevent the reducer from overwriting the existing theme. + // `undefined` means "not changed" (e.g., theme not in fetched list). + ...(updates.theme !== undefined && { theme: updates.theme }), css: updates.css, }); boundActionCreators.setUnsavedChanges(true); diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx index d7e4b8e1ec8..fbfd5da51c4 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx @@ -152,10 +152,12 @@ fetchMock.get('glob:*/api/v1/theme/*', { { id: 1, theme_name: 'Test Theme 1', + json_data: '{"token":{"colorPrimary":"#ff0000"}}', }, { id: 2, theme_name: 'Test Theme 2', + json_data: '{"token":{"colorPrimary":"#0000ff"}}', }, ], }, @@ -417,6 +419,74 @@ describe('PropertiesModal', () => { }); }); + test('passes full theme object with json_data to onSubmit when theme is selected', async () => { + mockedIsFeatureEnabled.mockReturnValue(false); + const props = createProps(); + props.onlyApply = true; + // Dashboard has theme id=1, which exists in the fetched themes list + const propsWithTheme = { + ...props, + dashboardInfo: { + ...dashboardInfo, + json_metadata: mockedJsonMetadata, + theme: { id: 1, theme_name: 'Test Theme 1' }, + }, + }; + render(, { + useRedux: true, + }); + + expect( + await screen.findByTestId('dashboard-edit-properties-form'), + ).toBeInTheDocument(); + + userEvent.click(screen.getByRole('button', { name: 'Apply' })); + await waitFor(() => { + expect(props.onSubmit).toHaveBeenCalledTimes(1); + const submitCall = props.onSubmit.mock.calls[0][0]; + // Full theme object (including json_data) should be passed, not just the ID + expect(submitCall.theme).toEqual({ + id: 1, + theme_name: 'Test Theme 1', + json_data: '{"token":{"colorPrimary":"#ff0000"}}', + }); + // themeId removed — derived from theme.id at the save callsite + expect(submitCall).not.toHaveProperty('themeId'); + }); + }); + + test('does not clear theme when selected theme is missing from fetched options', async () => { + mockedIsFeatureEnabled.mockReturnValue(false); + const props = createProps(); + props.onlyApply = true; + // Dashboard has theme id=99, but the theme fetch returns ids 1 and 2 + const propsWithDashboardInfo = { + ...props, + dashboardInfo: { + ...dashboardInfo, + json_metadata: mockedJsonMetadata, + theme: { id: 99, theme_name: 'Deleted Theme' }, + }, + }; + render(, { + useRedux: true, + }); + + expect( + await screen.findByTestId('dashboard-edit-properties-form'), + ).toBeInTheDocument(); + + userEvent.click(screen.getByRole('button', { name: 'Apply' })); + await waitFor(() => { + expect(props.onSubmit).toHaveBeenCalledTimes(1); + const submitCall = props.onSubmit.mock.calls[0][0]; + // theme should be undefined (not null) so Redux is not overwritten + expect(submitCall.theme).toBeUndefined(); + // themeId removed — derived from theme.id at the save callsite + expect(submitCall).not.toHaveProperty('themeId'); + }); + }); + test('Empty "Certified by" should clear "Certification details"', async () => { const props = createProps(); const noCertifiedByProps = { diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx index 8fbcbb690b8..b53bfd6fb66 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx @@ -138,6 +138,7 @@ const PropertiesModal = ({ Array<{ id: number; theme_name: string; + json_data?: string; }> >([]); const categoricalSchemeRegistry = getCategoricalSchemeRegistry(); @@ -385,7 +386,9 @@ const PropertiesModal = ({ colorNamespace, certifiedBy, certificationDetails, - themeId: selectedThemeId, + theme: selectedThemeId + ? themes.find(t => t.id === selectedThemeId) + : null, css: customCss, ...moreOnSubmitProps, }; @@ -441,7 +444,7 @@ const PropertiesModal = ({ // Fetch themes (excluding system themes) const themeQuery = rison.encode({ - columns: ['id', 'theme_name', 'is_system'], + columns: ['id', 'theme_name', 'is_system', 'json_data'], filters: [ { col: 'is_system', diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/sections/StylingSection.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/sections/StylingSection.tsx index 5194b910282..3c710861d3a 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/sections/StylingSection.tsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/sections/StylingSection.tsx @@ -69,6 +69,7 @@ const StyledSwitchContainer = styled.div` interface Theme { id: number; theme_name: string; + json_data?: string; } interface CssTemplate { diff --git a/superset-frontend/src/dashboard/containers/DashboardPage.test.tsx b/superset-frontend/src/dashboard/containers/DashboardPage.test.tsx new file mode 100644 index 00000000000..2eb05eea842 --- /dev/null +++ b/superset-frontend/src/dashboard/containers/DashboardPage.test.tsx @@ -0,0 +1,256 @@ +/** + * 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 type { ReactNode } from 'react'; +import { Suspense } from 'react'; +import { render, screen, waitFor } from 'spec/helpers/testing-library'; +import { + useDashboard, + useDashboardCharts, + useDashboardDatasets, +} from 'src/hooks/apiResources'; +import { SupersetClient } from '@superset-ui/core'; +import CrudThemeProvider from 'src/components/CrudThemeProvider'; +import DashboardPage from './DashboardPage'; + +const mockTheme = { + id: 42, + theme_name: 'Branded', + json_data: '{"token":{"colorPrimary":"#1677ff"}}', +}; + +const mockDashboard = { + id: 1, + slug: null, + url: '/superset/dashboard/1/', + dashboard_title: 'Test Dashboard', + thumbnail_url: null, + published: true, + css: null, + json_metadata: '{}', + position_json: '{}', + changed_by_name: 'admin', + changed_by: { id: 1, first_name: 'Admin', last_name: 'User' }, + changed_on: '2024-01-01T00:00:00', + charts: [], + owners: [], + roles: [], + theme: mockTheme, +}; + +jest.mock('src/hooks/apiResources', () => ({ + useDashboard: jest.fn(), + useDashboardCharts: jest.fn(), + useDashboardDatasets: jest.fn(), +})); + +jest.mock('src/dashboard/actions/hydrate', () => ({ + ...jest.requireActual('src/dashboard/actions/hydrate'), + hydrateDashboard: jest.fn(() => ({ type: 'MOCK_HYDRATE' })), +})); + +jest.mock('src/dashboard/actions/datasources', () => ({ + ...jest.requireActual('src/dashboard/actions/datasources'), + setDatasources: jest.fn(() => ({ type: 'MOCK_SET_DATASOURCES' })), +})); + +jest.mock('src/dashboard/actions/dashboardState', () => ({ + ...jest.requireActual('src/dashboard/actions/dashboardState'), + setDatasetsStatus: jest.fn(() => ({ type: 'MOCK_SET_DATASETS_STATUS' })), +})); + +jest.mock('src/components/CrudThemeProvider', () => ({ + __esModule: true, + default: jest.fn(({ children }: { children: ReactNode }) => ( +
{children}
+ )), +})); + +jest.mock('src/dashboard/components/DashboardBuilder/DashboardBuilder', () => ({ + __esModule: true, + default: () =>
DashboardBuilder
, +})); + +jest.mock('src/dashboard/components/SyncDashboardState', () => ({ + __esModule: true, + default: () => null, + getDashboardContextLocalStorage: () => ({}), +})); + +jest.mock('src/dashboard/containers/Dashboard', () => ({ + __esModule: true, + default: ({ children }: { children: ReactNode }) =>
{children}
, +})); + +jest.mock('src/components/MessageToasts/withToasts', () => ({ + useToasts: () => ({ addDangerToast: jest.fn(), addSuccessToast: jest.fn() }), +})); + +jest.mock('src/dashboard/util/injectCustomCss', () => ({ + __esModule: true, + default: () => () => {}, +})); + +jest.mock('src/dashboard/util/activeAllDashboardFilters', () => ({ + getAllActiveFilters: () => ({}), + getRelevantDataMask: () => ({}), +})); + +jest.mock('src/dashboard/util/activeDashboardFilters', () => ({ + getActiveFilters: () => ({}), +})); + +jest.mock('src/utils/urlUtils', () => ({ + getUrlParam: () => null, +})); + +jest.mock('src/dashboard/components/nativeFilters/FilterBar/keyValue', () => ({ + getFilterValue: jest.fn(), + getPermalinkValue: jest.fn(), +})); + +jest.mock('src/dashboard/contexts/AutoRefreshContext', () => ({ + AutoRefreshProvider: ({ children }: { children: ReactNode }) => ( + <>{children} + ), +})); + +const mockUseDashboard = useDashboard as jest.Mock; +const mockUseDashboardCharts = useDashboardCharts as jest.Mock; +const mockUseDashboardDatasets = useDashboardDatasets as jest.Mock; +const MockCrudThemeProvider = CrudThemeProvider as unknown as jest.Mock; + +afterEach(() => { + jest.restoreAllMocks(); +}); + +beforeEach(() => { + jest.clearAllMocks(); + mockUseDashboard.mockReturnValue({ + result: mockDashboard, + error: null, + }); + mockUseDashboardCharts.mockReturnValue({ + result: [], + error: null, + }); + mockUseDashboardDatasets.mockReturnValue({ + result: [], + error: null, + status: 'complete', + }); +}); + +test('passes full theme object from dashboard API response to CrudThemeProvider', async () => { + const clientGetSpy = jest.spyOn(SupersetClient, 'get'); + + render( + + + , + { + useRedux: true, + useRouter: true, + initialState: { + dashboardInfo: { id: 1, metadata: {} }, + dashboardState: { sliceIds: [] }, + nativeFilters: { filters: {} }, + dataMask: {}, + }, + }, + ); + + await waitFor(() => { + expect(screen.queryByText('loading')).not.toBeInTheDocument(); + }); + + expect(MockCrudThemeProvider).toHaveBeenCalledWith( + expect.objectContaining({ theme: mockTheme }), + expect.anything(), + ); + + // Regression guard: theme data comes from the dashboard API response, + // not a separate /api/v1/theme/:id fetch (which would 403 for non-admin users) + const themeCalls = clientGetSpy.mock.calls.filter(([{ endpoint }]) => + endpoint?.startsWith('/api/v1/theme/'), + ); + expect(themeCalls).toHaveLength(0); +}); + +test('uses theme from Redux dashboardInfo when it differs from API response (Properties modal update)', async () => { + const reduxTheme = { + id: 99, + theme_name: 'Updated Theme', + json_data: '{"token":{"colorPrimary":"#ff0000"}}', + }; + + render( + + + , + { + useRedux: true, + useRouter: true, + initialState: { + dashboardInfo: { id: 1, metadata: {}, theme: reduxTheme }, + dashboardState: { sliceIds: [] }, + nativeFilters: { filters: {} }, + dataMask: {}, + }, + }, + ); + + await waitFor(() => { + expect(screen.queryByText('loading')).not.toBeInTheDocument(); + }); + + // Redux theme should take priority over API response theme + expect(MockCrudThemeProvider).toHaveBeenCalledWith( + expect.objectContaining({ theme: reduxTheme }), + expect.anything(), + ); +}); + +test('passes null theme when Redux dashboardInfo.theme is explicitly null (theme removed)', async () => { + render( + + + , + { + useRedux: true, + useRouter: true, + initialState: { + dashboardInfo: { id: 1, metadata: {}, theme: null }, + dashboardState: { sliceIds: [] }, + nativeFilters: { filters: {} }, + dataMask: {}, + }, + }, + ); + + await waitFor(() => { + expect(screen.queryByText('loading')).not.toBeInTheDocument(); + }); + + // When theme is explicitly null in Redux (removed via Properties modal), + // CrudThemeProvider should receive null + expect(MockCrudThemeProvider).toHaveBeenCalledWith( + expect.objectContaining({ theme: null }), + expect.anything(), + ); +}); diff --git a/superset-frontend/src/dashboard/containers/DashboardPage.tsx b/superset-frontend/src/dashboard/containers/DashboardPage.tsx index e345f5a469d..f6d755adf6d 100644 --- a/superset-frontend/src/dashboard/containers/DashboardPage.tsx +++ b/superset-frontend/src/dashboard/containers/DashboardPage.tsx @@ -120,7 +120,7 @@ export const DashboardPage: FC = ({ idOrSlug }: PageProps) => { ({ dashboardInfo }) => dashboardInfo && Object.keys(dashboardInfo).length > 0, ); - const dashboardTheme = useSelector( + const reduxTheme = useSelector( (state: RootState) => state.dashboardInfo.theme, ); const { addDangerToast } = useToasts(); @@ -295,11 +295,7 @@ export const DashboardPage: FC = ({ idOrSlug }: PageProps) => { ; + +const stateWithScopes = { + id: 1, + metadata: { + native_filter_configuration: [ + { + id: 'FILTER-1', + name: 'Region', + chartsInScope: [10, 20, 30], + tabsInScope: ['TAB-A'], + targets: [{ datasetId: 1 }], + }, + { + id: 'FILTER-2', + name: 'Date Range', + chartsInScope: [40], + tabsInScope: [], + targets: [{ datasetId: 2 }], + }, + ], + chart_customization_config: [ + { + id: 'CUSTOM-1', + chartsInScope: [100], + tabsInScope: ['TAB-X'], + }, + ], + }, +} as unknown as Partial; + +test('preserves existing theme when DASHBOARD_INFO_UPDATED payload omits theme key', () => { + // Simulates the fixed Header behavior: conditional spread omits `theme` + // key when PropertiesModal returns theme: undefined (theme not in fetched list). + const action = dashboardInfoChanged({ slug: 'new-slug' }); + const result = dashboardInfoReducer(stateWithTheme, action); + + expect(result.theme).toBe(existingTheme); + expect(result.slug).toBe('new-slug'); + + // themeId derivation (Header line 299) should produce the original ID + const themeId = result.theme ? result.theme.id : null; + expect(themeId).toBe(99); +}); + +test('clears theme when DASHBOARD_INFO_UPDATED payload has theme: null', () => { + // Simulates intentional theme removal via Properties modal. + const action = dashboardInfoChanged({ theme: null }); + const result = dashboardInfoReducer(stateWithTheme, action); + + expect(result.theme).toBeNull(); + + const themeId = result.theme ? result.theme.id : null; + expect(themeId).toBeNull(); +}); + +test('overwrites theme when DASHBOARD_INFO_UPDATED payload has theme: undefined', () => { + // Documents the dangerous behavior that the Header conditional spread prevents. + // If `theme: undefined` leaks into the action payload, the reducer WILL overwrite + // the existing theme — this is why Header must omit the key entirely. + const action = { + type: DASHBOARD_INFO_UPDATED, + newInfo: { theme: undefined }, + }; + const result = dashboardInfoReducer(stateWithTheme, action); + + // theme is overwritten to undefined — save would compute themeId as null + expect(result.theme).toBeUndefined(); + const themeId = result.theme ? result.theme.id : null; + expect(themeId).toBeNull(); +}); + +test('preserves native_filter_configuration scopes during DASHBOARD_INFO_UPDATED metadata refresh', () => { + // Simulates the save flow: onUpdateSuccess → setDashboardMetadata(serverMetadata) + // → dashboardInfoChanged({ metadata: ... }) → DASHBOARD_INFO_UPDATED. + // The server response contains filters WITHOUT chartsInScope/tabsInScope (client-only). + const serverMetadata = { + native_filter_configuration: [ + { id: 'FILTER-1', name: 'Region (updated)', targets: [{ datasetId: 1 }] }, + { id: 'FILTER-2', name: 'Date Range', targets: [{ datasetId: 2 }] }, + ], + } as unknown as DashboardInfo['metadata']; + + const action = dashboardInfoChanged({ metadata: serverMetadata }); + const result = dashboardInfoReducer(stateWithScopes, action); + + const filters = result.metadata?.native_filter_configuration; + expect(filters).toHaveLength(2); + + // FILTER-1: server updated the name, scopes preserved from existing state + expect(filters![0].name).toBe('Region (updated)'); + expect(filters![0].chartsInScope).toEqual([10, 20, 30]); + expect(filters![0].tabsInScope).toEqual(['TAB-A']); + + // FILTER-2: unchanged, scopes preserved + expect(filters![1].chartsInScope).toEqual([40]); + expect(filters![1].tabsInScope).toEqual([]); +}); + +test('preserves chart_customization_config scopes during DASHBOARD_INFO_UPDATED metadata refresh', () => { + const serverMetadata = { + chart_customization_config: [{ id: 'CUSTOM-1' }], + } as unknown as DashboardInfo['metadata']; + + const action = dashboardInfoChanged({ metadata: serverMetadata }); + const result = dashboardInfoReducer(stateWithScopes, action); + + const customizations = result.metadata?.chart_customization_config; + expect(customizations).toHaveLength(1); + expect(customizations![0].chartsInScope).toEqual([100]); + expect(customizations![0].tabsInScope).toEqual(['TAB-X']); +}); + +test('does not affect metadata when DASHBOARD_INFO_UPDATED has no metadata key', () => { + const action = dashboardInfoChanged({ slug: 'new-slug' }); + const result = dashboardInfoReducer(stateWithScopes, action); + + // Metadata untouched — same reference + expect(result.metadata).toBe(stateWithScopes.metadata); +}); diff --git a/superset-frontend/src/dashboard/reducers/dashboardInfo.ts b/superset-frontend/src/dashboard/reducers/dashboardInfo.ts index 69c4551ce78..f6dbe8b7bd8 100644 --- a/superset-frontend/src/dashboard/reducers/dashboardInfo.ts +++ b/superset-frontend/src/dashboard/reducers/dashboardInfo.ts @@ -46,9 +46,7 @@ import { interface DashboardInfoAction { type: string; - newInfo?: Partial & { - theme_id?: number | null; - }; + newInfo?: Partial; filterBarOrientation?: FilterBarOrientation; crossFiltersEnabled?: boolean; chartCustomization?: (ChartCustomization | ChartCustomizationDivider)[]; @@ -124,22 +122,31 @@ export default function dashboardInfoReducer( case DASHBOARD_INFO_UPDATED: { const dashAction = action as DashboardInfoAction; const newInfo = dashAction.newInfo || {}; - const { theme_id: themeId, ...otherInfo } = newInfo; - const updatedState: DashboardInfoState = { + const incomingMeta = newInfo.metadata; + return { ...state, - ...otherInfo, + ...newInfo, + // Preserve client-only scope data (chartsInScope, tabsInScope) when + // metadata is refreshed from the server, matching HYDRATE_DASHBOARD. + ...(incomingMeta && { + metadata: { + ...incomingMeta, + ...(incomingMeta.native_filter_configuration && { + native_filter_configuration: preserveScopes( + state.metadata?.native_filter_configuration, + incomingMeta.native_filter_configuration, + ), + }), + ...(incomingMeta.chart_customization_config && { + chart_customization_config: preserveScopes( + state.metadata?.chart_customization_config, + incomingMeta.chart_customization_config, + ), + }), + }, + }), last_modified_time: Math.round(new Date().getTime() / 1000), }; - - if (themeId !== undefined) { - if (themeId === null) { - updatedState.theme = null; - } else { - updatedState.theme = { id: themeId, name: `Theme ${themeId}` }; - } - } - - return updatedState; } case DASHBOARD_INFO_FILTERS_CHANGED: { const dashAction = action as DashboardInfoAction; diff --git a/superset-frontend/src/dashboard/types.ts b/superset-frontend/src/dashboard/types.ts index 49b05812468..2a7056bebab 100644 --- a/superset-frontend/src/dashboard/types.ts +++ b/superset-frontend/src/dashboard/types.ts @@ -207,9 +207,9 @@ export type DashboardInfo = { pendingChartCustomizations?: Record; theme?: { id: number; - name: string; + theme_name: string; + json_data: string; } | null; - theme_id?: number | null; css?: string; slug?: string; last_modified_time: number;