This is an automated email from the ASF dual-hosted git repository. jli pushed a commit to branch fix-app-root-logos in repository https://gitbox.apache.org/repos/asf/superset.git
commit 396bcc23639e661976e1beada009f01c597155b4 Author: Joe Li <[email protected]> AuthorDate: Sun Mar 1 10:57:54 2026 -0800 test(frontend): add failing tests for ensureAppRoot absolute URL passthrough TDD RED phase for fix of brandLogoHref being double-prefixed with app root when LOGO_TARGET_PATH is an absolute URL in subdirectory deployments. - pathUtils.test.ts: 4 failing tests covering https://, http://, ftp://, mailto:, and protocol-relative (//...) passthrough for both ensureAppRoot and makeUrl; adds loadPathUtils() helper to reduce bootstrap boilerplate - Menu.test.tsx: 2 failing tests for absolute and protocol-relative brandLogoHref; 3 passing guardrails for brand.path subdir prefixing, fallback branch behavior, and useTheme mock infrastructure Co-Authored-By: Claude Sonnet 4.6 <[email protected]> --- superset-frontend/src/features/home/Menu.test.tsx | 115 +++++++++++++++++++++- superset-frontend/src/utils/pathUtils.test.ts | 63 ++++++++++++ 2 files changed, 177 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/features/home/Menu.test.tsx b/superset-frontend/src/features/home/Menu.test.tsx index 4b7e744188..2c07963c7b 100644 --- a/superset-frontend/src/features/home/Menu.test.tsx +++ b/superset-frontend/src/features/home/Menu.test.tsx @@ -21,9 +21,15 @@ import fetchMock from 'fetch-mock'; import { render, screen, userEvent } from 'spec/helpers/testing-library'; import setupCodeOverrides from 'src/setup/setupCodeOverrides'; import { getExtensionsRegistry } from '@superset-ui/core'; +import * as CoreUI from '@apache-superset/core/ui'; import { Menu } from './Menu'; import * as getBootstrapData from 'src/utils/getBootstrapData'; +jest.mock('@apache-superset/core/ui', () => ({ + ...jest.requireActual('@apache-superset/core/ui'), + useTheme: jest.fn(), +})); + const dropdownItems = [ { label: 'Data', @@ -243,6 +249,8 @@ const staticAssetsPrefixMock = jest.spyOn( getBootstrapData, 'staticAssetsPrefix', ); +const applicationRootMock = jest.spyOn(getBootstrapData, 'applicationRoot'); +const useThemeMock = CoreUI.useTheme as jest.Mock; fetchMock.get( 'glob:*api/v1/database/?q=(filters:!((col:allow_file_upload,opr:upload_is_enabled,value:!t)))', @@ -252,8 +260,11 @@ fetchMock.get( beforeEach(() => { // setup a DOM element as a render target useSelectorMock.mockClear(); - // By default use empty static assets prefix + // By default use empty static assets prefix and default app root staticAssetsPrefixMock.mockReturnValue(''); + applicationRootMock.mockReturnValue(''); + // By default useTheme returns the real default theme (brandLogoUrl is falsy) + useThemeMock.mockReturnValue(CoreUI.supersetTheme); }); test('should render', async () => { @@ -675,3 +686,105 @@ test('should not render the brand text if not available', async () => { const brandText = screen.queryByText(text); expect(brandText).not.toBeInTheDocument(); }); + +test('brand logo href should not be prefixed with app root when brandLogoHref is an absolute URL', async () => { + applicationRootMock.mockReturnValue('/superset'); + useThemeMock.mockReturnValue({ + ...CoreUI.supersetTheme, + brandLogoUrl: '/static/assets/images/custom-logo.png', + brandLogoHref: 'https://external.example.com', + }); + useSelectorMock.mockReturnValue({ roles: user.roles }); + + render(<Menu {...mockedProps} />, { + useRedux: true, + useQueryParams: true, + useRouter: true, + useTheme: true, + }); + + const brandLink = await screen.findByRole('link', { + name: /apache superset/i, + }); + expect(brandLink).toHaveAttribute('href', 'https://external.example.com'); +}); + +test('brand logo href should not be prefixed with app root when brandLogoHref is protocol-relative', async () => { + applicationRootMock.mockReturnValue('/superset'); + useThemeMock.mockReturnValue({ + ...CoreUI.supersetTheme, + brandLogoUrl: '/static/assets/images/custom-logo.png', + brandLogoHref: '//external.example.com', + }); + useSelectorMock.mockReturnValue({ roles: user.roles }); + + render(<Menu {...mockedProps} />, { + useRedux: true, + useQueryParams: true, + useRouter: true, + useTheme: true, + }); + + const brandLink = await screen.findByRole('link', { + name: /apache superset/i, + }); + expect(brandLink).toHaveAttribute('href', '//external.example.com'); +}); + +test('brand path should be prefixed with app root in subdirectory deployment', async () => { + applicationRootMock.mockReturnValue('/superset'); + useSelectorMock.mockReturnValue({ roles: user.roles }); + + const propsWithSimplePath = { + ...mockedProps, + data: { + ...mockedProps.data, + brand: { + ...mockedProps.data.brand, + path: '/welcome/', + }, + }, + }; + + render(<Menu {...propsWithSimplePath} />, { + useRedux: true, + useQueryParams: true, + useRouter: true, + useTheme: true, + }); + + const brandLink = await screen.findByRole('link', { + name: new RegExp(propsWithSimplePath.data.brand.alt, 'i'), + }); + expect(brandLink).toHaveAttribute('href', '/superset/welcome/'); +}); + +test('brand link falls back to brand.path when theme brandLogoUrl is absent', async () => { + // useThemeMock default returns supersetTheme with brandLogoUrl undefined (falsy) + applicationRootMock.mockReturnValue('/superset'); + useSelectorMock.mockReturnValue({ roles: user.roles }); + + const propsWithFallbackPath = { + ...mockedProps, + data: { + ...mockedProps.data, + brand: { + ...mockedProps.data.brand, + path: '/welcome/', + }, + }, + }; + + render(<Menu {...propsWithFallbackPath} />, { + useRedux: true, + useQueryParams: true, + useRouter: true, + useTheme: true, + }); + + const brandLink = await screen.findByRole('link', { + name: new RegExp(propsWithFallbackPath.data.brand.alt, 'i'), + }); + // ensureAppRoot must have been applied: /welcome/ → /superset/welcome/ + expect(brandLink).toHaveAttribute('href', '/superset/welcome/'); +}); diff --git a/superset-frontend/src/utils/pathUtils.test.ts b/superset-frontend/src/utils/pathUtils.test.ts index bf24053e67..5c8aed9df0 100644 --- a/superset-frontend/src/utils/pathUtils.test.ts +++ b/superset-frontend/src/utils/pathUtils.test.ts @@ -158,3 +158,66 @@ test('makeUrl should handle URLs with anchors', async () => { '/superset/dashboard/123#anchor', ); }); + +// Sets up bootstrap data and returns a fresh pathUtils module instance. +// Passing appRoot='' (default) simulates no subdirectory deployment. +async function loadPathUtils(appRoot = '') { + const bootstrapData = { common: { application_root: appRoot } }; + document.body.innerHTML = `<div id="app" data-bootstrap='${JSON.stringify(bootstrapData)}'></div>`; + jest.resetModules(); + await import('./getBootstrapData'); + return import('./pathUtils'); +} + +test('ensureAppRoot should preserve absolute and protocol-relative URLs unchanged with default root', async () => { + const { ensureAppRoot } = await loadPathUtils(); + + expect(ensureAppRoot('https://external.example.com')).toBe( + 'https://external.example.com', + ); + expect(ensureAppRoot('http://external.example.com')).toBe( + 'http://external.example.com', + ); + expect(ensureAppRoot('//external.example.com')).toBe( + '//external.example.com', + ); +}); + +test('ensureAppRoot should preserve absolute URLs unchanged with custom subdirectory', async () => { + const { ensureAppRoot } = await loadPathUtils('/superset/'); + + expect(ensureAppRoot('https://external.example.com')).toBe( + 'https://external.example.com', + ); + expect(ensureAppRoot('http://external.example.com')).toBe( + 'http://external.example.com', + ); + // Non-http absolute schemes: implementation must not rely on http/https check alone + expect(ensureAppRoot('ftp://files.example.com/data')).toBe( + 'ftp://files.example.com/data', + ); + expect(ensureAppRoot('mailto:[email protected]')).toBe( + 'mailto:[email protected]', + ); +}); + +test('ensureAppRoot should preserve protocol-relative URLs unchanged', async () => { + const { ensureAppRoot } = await loadPathUtils('/superset/'); + + expect(ensureAppRoot('//external.example.com')).toBe( + '//external.example.com', + ); +}); + +test('makeUrl should preserve absolute and protocol-relative URLs unchanged', async () => { + const { makeUrl } = await loadPathUtils('/superset/'); + + expect(makeUrl('https://external.example.com')).toBe( + 'https://external.example.com', + ); + expect(makeUrl('//external.example.com')).toBe('//external.example.com'); + // Non-http absolute scheme parity with ensureAppRoot + expect(makeUrl('ftp://files.example.com/data')).toBe( + 'ftp://files.example.com/data', + ); +});
