rusackas commented on code in PR #37141: URL: https://github.com/apache/superset/pull/37141#discussion_r3615670720
########## superset-frontend/src/pages/MobileUnsupported/MobileUnsupported.test.tsx: ########## @@ -0,0 +1,172 @@ +/** Review Comment: That hint (and the whole Continue anyway bypass) got removed a few commits back... growing the viewport past the breakpoint unblocks the route automatically now, so there's nothing left to test here. ########## superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts: ########## @@ -0,0 +1,344 @@ +/** + * 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, devices } from '@playwright/test'; +import { TIMEOUT } from '../../utils/constants'; + +/** + * Mobile dashboard viewing tests verify that dashboards can be viewed + * and interacted with on mobile devices. + * + * These tests assume the World Bank's Health sample dashboard exists. + */ + +// Use iPhone 12 viewport for mobile tests +const mobileViewport = devices['iPhone 12']; + +test.describe('Mobile Dashboard Viewing', () => { + test.use({ + viewport: mobileViewport.viewport, + userAgent: mobileViewport.userAgent, + }); + + test.beforeEach(async ({ page }) => { + // Navigate to dashboard list to find a dashboard + await page.goto('dashboard/list/'); + await page.waitForLoadState('networkidle'); + }); + + test('dashboard list renders in card view on mobile', async ({ page }) => { + // On mobile, dashboard list should show cards, not table + // Look for card elements + const cards = page.locator('[data-test="styled-card"]'); + + // Should have at least one card if dashboards exist + // (This test may need adjustment based on test data availability) + const cardCount = await cards.count(); + + // Either cards are visible, or the empty state is shown + if (cardCount > 0) { + await expect(cards.first()).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + } else { + // No dashboards - that's OK for the test environment + await expect( + page + .getByText('No dashboards yet') + .or(page.locator('[data-test="listview-table"]')), Review Comment: Good catch, fixed! The fallback now asserts the `empty-state` container, and the table view is asserted absent either way. ########## superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx: ########## @@ -521,3 +536,82 @@ test('should maintain layout when switching between tabs', async () => { expect(gridContainer).toBeInTheDocument(); expect(tabPanels.length).toBeGreaterThan(0); }); + +// Mobile support tests +// Note: The main mobile tests require mocking useBreakpoint to return mobile breakpoints +// which is done at the module level. These tests verify mobile-related component behavior. + +test('should not render filter bar panel on desktop when nativeFiltersEnabled is false', () => { + (useStoredSidebarWidth as jest.Mock).mockImplementation(() => [ + 100, + jest.fn(), + ]); + (fetchFaveStar as jest.Mock).mockReturnValue({ type: 'mock-action' }); + (setActiveTab as jest.Mock).mockReturnValue({ type: 'mock-action' }); + + jest.spyOn(useNativeFiltersModule, 'useNativeFilters').mockReturnValue({ + showDashboard: true, + missingInitialFilters: [], + dashboardFiltersOpen: true, + toggleDashboardFiltersOpen: jest.fn(), + nativeFiltersEnabled: false, + hasFilters: false, + }); + + const { queryByTestId } = render(<DashboardBuilder />, { + useRedux: true, + store: storeWithState({ + ...mockState, + dashboardLayout: undoableDashboardLayout, + }), + useDnd: true, + useTheme: true, + }); + + // Filter panel should not be present when native filters are disabled + expect(queryByTestId('dashboard-filters-panel')).not.toBeInTheDocument(); +}); + +test('should render dashboard content wrapper', () => { + (useStoredSidebarWidth as jest.Mock).mockImplementation(() => [ + 100, + jest.fn(), + ]); + (fetchFaveStar as jest.Mock).mockReturnValue({ type: 'mock-action' }); + (setActiveTab as jest.Mock).mockReturnValue({ type: 'mock-action' }); + + const { getByTestId } = render(<DashboardBuilder />, { + useRedux: true, + store: storeWithState({ + ...mockState, + dashboardLayout: undoableDashboardLayout, + }), + useDnd: true, + useTheme: true, + }); + + // Dashboard content wrapper should always be present + expect(getByTestId('dashboard-content-wrapper')).toBeInTheDocument(); +}); + +test('should render header container', () => { + (useStoredSidebarWidth as jest.Mock).mockImplementation(() => [ + 100, + jest.fn(), + ]); + (fetchFaveStar as jest.Mock).mockReturnValue({ type: 'mock-action' }); + (setActiveTab as jest.Mock).mockReturnValue({ type: 'mock-action' }); + + const { queryByTestId } = render(<DashboardBuilder />, { + useRedux: true, + store: storeWithState({ + ...mockState, + dashboardLayout: undoableDashboardLayout, + }), + useDnd: true, + useTheme: true, + }); + + // Header container should be present + expect(queryByTestId('dashboard-header-container')).toBeInTheDocument(); Review Comment: I think this one's a miss... `dashboard-header-container` comes from `Header/index.tsx`, and the `-wrapper` id is a different element. The pre-existing tests on `master` use the same id. ########## superset-frontend/src/components/MobileRouteGuard/MobileRouteGuard.test.tsx: ########## @@ -0,0 +1,124 @@ +/** + * 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 MobileRouteGuard, { + isMobileSupportedRoute, + MOBILE_SUPPORTED_ROUTES, +} from './index'; + +// Store the original sessionStorage +const originalSessionStorage = window.sessionStorage; + +// Clean up sessionStorage before each test +beforeEach(() => { + sessionStorage.clear(); + jest.clearAllMocks(); +}); + +afterEach(() => { + // Restore sessionStorage if it was mocked + Object.defineProperty(window, 'sessionStorage', { + value: originalSessionStorage, + writable: true, + }); +}); + +// Unit tests for isMobileSupportedRoute helper function +test('isMobileSupportedRoute returns true for dashboard list', () => { + expect(isMobileSupportedRoute('/dashboard/list/')).toBe(true); + expect(isMobileSupportedRoute('/dashboard/list')).toBe(true); +}); + +test('isMobileSupportedRoute returns true for individual dashboards', () => { + expect(isMobileSupportedRoute('/superset/dashboard/123/')).toBe(true); + expect(isMobileSupportedRoute('/superset/dashboard/my-dashboard/')).toBe( + true, + ); + expect(isMobileSupportedRoute('/superset/dashboard/abc-123/')).toBe(true); +}); + +test('isMobileSupportedRoute returns true for welcome page', () => { + expect(isMobileSupportedRoute('/superset/welcome/')).toBe(true); + expect(isMobileSupportedRoute('/superset/welcome')).toBe(true); +}); + +test('isMobileSupportedRoute returns true for auth routes', () => { + expect(isMobileSupportedRoute('/login/')).toBe(true); + expect(isMobileSupportedRoute('/logout/')).toBe(true); + expect(isMobileSupportedRoute('/register/')).toBe(true); +}); + +test('isMobileSupportedRoute returns false for chart routes', () => { + expect(isMobileSupportedRoute('/chart/list/')).toBe(false); + expect(isMobileSupportedRoute('/explore/')).toBe(false); + expect(isMobileSupportedRoute('/superset/explore/')).toBe(false); +}); + +test('isMobileSupportedRoute returns false for SQL Lab', () => { + expect(isMobileSupportedRoute('/sqllab/')).toBe(false); + expect(isMobileSupportedRoute('/superset/sqllab/')).toBe(false); +}); + +test('isMobileSupportedRoute returns false for database/dataset routes', () => { + expect(isMobileSupportedRoute('/database/list/')).toBe(false); + expect(isMobileSupportedRoute('/dataset/list/')).toBe(false); +}); + +test('isMobileSupportedRoute strips query params and hash', () => { + expect(isMobileSupportedRoute('/dashboard/list/?page=1')).toBe(true); + expect(isMobileSupportedRoute('/dashboard/list/#section')).toBe(true); + expect(isMobileSupportedRoute('/chart/list/?page=1')).toBe(false); +}); + +test('MOBILE_SUPPORTED_ROUTES includes expected patterns', () => { + // Verify the constant is exported and has expected patterns + expect(MOBILE_SUPPORTED_ROUTES).toBeInstanceOf(Array); + expect(MOBILE_SUPPORTED_ROUTES.length).toBeGreaterThan(0); + + // Check some patterns exist + const hasLoginPattern = MOBILE_SUPPORTED_ROUTES.some(p => p.test('/login/')); + const hasDashboardListPattern = MOBILE_SUPPORTED_ROUTES.some(p => + p.test('/dashboard/list/'), + ); + const hasWelcomePattern = MOBILE_SUPPORTED_ROUTES.some(p => + p.test('/superset/welcome/'), + ); + + expect(hasLoginPattern).toBe(true); + expect(hasDashboardListPattern).toBe(true); + expect(hasWelcomePattern).toBe(true); +}); + +// Integration tests for MobileRouteGuard component +// Note: These tests require mocking at the module level which is complex +// The tests below verify the component structure and exports + +test('MobileRouteGuard exports the component as default', () => { + expect(MobileRouteGuard).toBeDefined(); + expect(typeof MobileRouteGuard).toBe('function'); +}); + +test('isMobileSupportedRoute is exported', () => { + expect(isMobileSupportedRoute).toBeDefined(); + expect(typeof isMobileSupportedRoute).toBe('function'); +}); + +test('MOBILE_SUPPORTED_ROUTES is exported', () => { + expect(MOBILE_SUPPORTED_ROUTES).toBeDefined(); + expect(Array.isArray(MOBILE_SUPPORTED_ROUTES)).toBe(true); +}); Review Comment: The regex allowlist is gone entirely... routes declare `mobileSupported` in `routes.tsx` now, so there are no patterns left to anchor. ########## superset-frontend/packages/superset-ui-core/src/components/PageHeaderWithActions/index.tsx: ########## @@ -81,6 +81,15 @@ const headerStyles = (theme: SupersetTheme) => css` display: flex; align-items: center; } + + /* Mobile: center the title between left and right panels */ + @media (max-width: 767px) { + .title-panel { + flex: 1; + justify-content: center; + margin-right: 0; + } + } Review Comment: The filter trigger and the kebab roughly balance each other in practice, so the title lands close enough to center on the screens we've captured. Happy to revisit if it looks off on a real dashboard. ########## superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts: ########## @@ -0,0 +1,344 @@ +/** + * 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, devices } from '@playwright/test'; +import { TIMEOUT } from '../../utils/constants'; + +/** + * Mobile dashboard viewing tests verify that dashboards can be viewed + * and interacted with on mobile devices. + * + * These tests assume the World Bank's Health sample dashboard exists. + */ + +// Use iPhone 12 viewport for mobile tests +const mobileViewport = devices['iPhone 12']; + +test.describe('Mobile Dashboard Viewing', () => { + test.use({ + viewport: mobileViewport.viewport, + userAgent: mobileViewport.userAgent, + }); + + test.beforeEach(async ({ page }) => { + // Navigate to dashboard list to find a dashboard + await page.goto('dashboard/list/'); + await page.waitForLoadState('networkidle'); + }); + + test('dashboard list renders in card view on mobile', async ({ page }) => { + // On mobile, dashboard list should show cards, not table + // Look for card elements + const cards = page.locator('[data-test="styled-card"]'); + + // Should have at least one card if dashboards exist + // (This test may need adjustment based on test data availability) + const cardCount = await cards.count(); + + // Either cards are visible, or the empty state is shown + if (cardCount > 0) { + await expect(cards.first()).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + } else { + // No dashboards - that's OK for the test environment + await expect( + page + .getByText('No dashboards yet') + .or(page.locator('[data-test="listview-table"]')), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); Review Comment: Good catch, fixed! The no-dashboards branch asserts the empty state now, and `listview-table` is asserted absent in both branches. ########## superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx: ########## @@ -106,6 +111,11 @@ export const useHeaderActionsMenu = ({ case MenuKeys.ManageEmbedded: manageEmbedded(); break; + case 'toggle-favorite': + if (saveFaveStar && isStarred !== undefined) { + saveFaveStar(dashboardId, isStarred); + } + break; Review Comment: The action expects the current value and inverts internally (`isStarred ? delete : post`, then `toggleFaveStar(!isStarred)`), same convention as the `FaveStar` component. Passing `!isStarred` would double-invert. ########## superset-frontend/src/components/ListView/ListView.test.tsx: ########## @@ -350,3 +350,225 @@ describe('ListView', () => { expect(mockedPropsComprehensive.fetchData).toHaveBeenCalled(); }); }); + +// Mobile support tests +test('respects forceViewMode prop and hides view toggle', () => { + // Omit cardSortSelectOptions to avoid CardSortSelect needing initialSort + const { cardSortSelectOptions, ...propsWithoutSort } = + mockedPropsComprehensive; + render( + <MemoryRouter> + <QueryParamProvider adapter={ReactRouter5Adapter}> + <ListView + {...propsWithoutSort} + renderCard={() => <div>Card</div>} + forceViewMode="card" + /> + </QueryParamProvider> + </MemoryRouter>, + { store: mockStore() }, + ); + + // View toggle should not be present when forceViewMode is set + expect(screen.queryByLabelText('card-view')).not.toBeInTheDocument(); + expect(screen.queryByLabelText('list-view')).not.toBeInTheDocument(); +}); + +test('shows card view when forceViewMode is card', () => { + // Omit cardSortSelectOptions to avoid CardSortSelect needing initialSort + const { cardSortSelectOptions, ...propsWithoutSort } = + mockedPropsComprehensive; + render( + <MemoryRouter> + <QueryParamProvider adapter={ReactRouter5Adapter}> + <ListView + {...propsWithoutSort} + renderCard={() => <div data-test="test-card">Card Content</div>} + forceViewMode="card" + /> + </QueryParamProvider> + </MemoryRouter>, + { store: mockStore() }, + ); + + // Should render cards, not table rows + expect(screen.getAllByTestId('test-card')).toHaveLength(2); +}); + +test('renders mobile filter drawer when mobileFiltersOpen is true', () => { + const setMobileFiltersOpen = jest.fn(); + // Omit cardSortSelectOptions to avoid CardSortSelect needing initialSort + const { cardSortSelectOptions, ...propsWithoutSort } = + mockedPropsComprehensive; + render( + <MemoryRouter> + <QueryParamProvider adapter={ReactRouter5Adapter}> + <ListView + {...propsWithoutSort} + mobileFiltersOpen + setMobileFiltersOpen={setMobileFiltersOpen} + mobileFiltersDrawerTitle="Search Dashboards" + /> + </QueryParamProvider> + </MemoryRouter>, + { store: mockStore() }, + ); + + // Drawer should be visible with custom title + expect(screen.getByText('Search Dashboards')).toBeInTheDocument(); +}); + +test('calls setMobileFiltersOpen(false) when drawer is closed', async () => { + const setMobileFiltersOpen = jest.fn(); + // Omit cardSortSelectOptions to avoid CardSortSelect needing initialSort + const { cardSortSelectOptions, ...propsWithoutSort } = + mockedPropsComprehensive; + render( + <MemoryRouter> + <QueryParamProvider adapter={ReactRouter5Adapter}> + <ListView + {...propsWithoutSort} + mobileFiltersOpen + setMobileFiltersOpen={setMobileFiltersOpen} + mobileFiltersDrawerTitle="Search" + /> + </QueryParamProvider> + </MemoryRouter>, + { store: mockStore() }, + ); + + // Click the close button on the drawer + const closeButton = screen.getByLabelText('Close'); + await userEvent.click(closeButton); + + expect(setMobileFiltersOpen).toHaveBeenCalledWith(false); +}); + +test('mobile drawer contains FilterControls', () => { + const setMobileFiltersOpen = jest.fn(); + const { cardSortSelectOptions, ...propsWithoutSort } = + mockedPropsComprehensive; + render( + <MemoryRouter> + <QueryParamProvider adapter={ReactRouter5Adapter}> + <ListView + {...propsWithoutSort} + mobileFiltersOpen + setMobileFiltersOpen={setMobileFiltersOpen} + /> + </QueryParamProvider> + </MemoryRouter>, + { store: mockStore() }, + ); + + // The drawer should contain filter controls (comboboxes for select filters) + const comboboxes = screen.getAllByRole('combobox'); + expect(comboboxes.length).toBeGreaterThan(0); +}); + +test('mobile drawer contains CardSortSelect when in card view with sort options', () => { + const setMobileFiltersOpen = jest.fn(); + render( + <MemoryRouter> + <QueryParamProvider adapter={ReactRouter5Adapter}> + <ListView + {...mockedPropsComprehensive} + renderCard={() => <div>Card</div>} + forceViewMode="card" + mobileFiltersOpen + setMobileFiltersOpen={setMobileFiltersOpen} + initialSort={[{ id: 'something' }]} + /> + </QueryParamProvider> + </MemoryRouter>, + { store: mockStore() }, + ); + + // Sort select should be present (may be multiple - one in drawer, one in header) + const sortSelects = screen.getAllByTestId('card-sort-select'); + expect(sortSelects.length).toBeGreaterThan(0); +}); + +test('uses default drawer title when mobileFiltersDrawerTitle not provided', () => { + const setMobileFiltersOpen = jest.fn(); + const { cardSortSelectOptions, ...propsWithoutSort } = + mockedPropsComprehensive; + render( + <MemoryRouter> + <QueryParamProvider adapter={ReactRouter5Adapter}> + <ListView + {...propsWithoutSort} + mobileFiltersOpen + setMobileFiltersOpen={setMobileFiltersOpen} + /> + </QueryParamProvider> + </MemoryRouter>, + { store: mockStore() }, + ); + + // Default title should be 'Search' + expect(screen.getByText('Search')).toBeInTheDocument(); +}); + +test('does not render drawer when mobileFiltersOpen is false', () => { + const setMobileFiltersOpen = jest.fn(); + const { cardSortSelectOptions, ...propsWithoutSort } = + mockedPropsComprehensive; + render( + <MemoryRouter> + <QueryParamProvider adapter={ReactRouter5Adapter}> + <ListView + {...propsWithoutSort} + mobileFiltersOpen={false} + setMobileFiltersOpen={setMobileFiltersOpen} + mobileFiltersDrawerTitle="Search" + /> + </QueryParamProvider> + </MemoryRouter>, + { store: mockStore() }, + ); + + // Drawer should not be visible (title not in visible content) + // Note: Ant Design drawer might still be in DOM but hidden + const drawer = document.querySelector('.ant-drawer-open'); + expect(drawer).toBeNull(); Review Comment: antd only applies `.ant-drawer-open` when the drawer is actually open, so asserting it's absent does cover the drawer being shown... the drawer content isn't even mounted before first open. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
