This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch removing-DASHBOARD_CROSS_FILTERS-flag in repository https://gitbox.apache.org/repos/asf/superset.git
commit aaaa957bedb178c14907cd584fa9ced92f70c5b4 Author: Evan Rusackas <[email protected]> AuthorDate: Mon Jan 13 17:09:19 2025 -0700 getting tests to pass --- .../src/dashboard/actions/dashboardState.test.js | 15 ++- .../src/dashboard/components/Dashboard.test.jsx | 116 ++++++++++++++++----- .../OverwriteConfirmModal.test.tsx | 41 ++++++-- .../FilterBarSettings/FilterBarSettings.test.tsx | 24 +++-- .../components/nativeFilters/utils.test.ts | 20 ++++ .../dashboard/components/nativeFilters/utils.ts | 22 +++- 6 files changed, 191 insertions(+), 47 deletions(-) diff --git a/superset-frontend/src/dashboard/actions/dashboardState.test.js b/superset-frontend/src/dashboard/actions/dashboardState.test.js index 4e30c38fb1..0bb4fbffda 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.test.js +++ b/superset-frontend/src/dashboard/actions/dashboardState.test.js @@ -57,6 +57,7 @@ describe('dashboardState actions', () => { present: mockDashboardData.positions, future: {}, }, + charts: {}, }; const newDashboardData = mockDashboardData; @@ -153,7 +154,12 @@ describe('dashboardState actions', () => { it('dispatches SET_OVERRIDE_CONFIRM when an inspect value has diff', async () => { const id = 192; - const { getState, dispatch } = setup(); + const { getState, dispatch } = setup({ + charts: { + 123: { id: 123 }, + 456: { id: 456 }, + }, + }); const thunk = saveDashboardRequest( newDashboardData, id, @@ -172,7 +178,12 @@ describe('dashboardState actions', () => { it('should post dashboard data with after confirm the overwrite values', async () => { const id = 192; - const { getState, dispatch } = setup(); + const { getState, dispatch } = setup({ + charts: { + 123: { id: 123 }, + 456: { id: 456 }, + }, + }); const confirmedDashboardData = { ...newDashboardData, css: updatedCss, diff --git a/superset-frontend/src/dashboard/components/Dashboard.test.jsx b/superset-frontend/src/dashboard/components/Dashboard.test.jsx index e3421ee040..3d841661b1 100644 --- a/superset-frontend/src/dashboard/components/Dashboard.test.jsx +++ b/superset-frontend/src/dashboard/components/Dashboard.test.jsx @@ -48,6 +48,7 @@ describe('Dashboard', () => { removeSliceFromDashboard() {}, triggerQuery() {}, logEvent() {}, + clearDataMaskState() {}, }, dashboardState, dashboardInfo, @@ -61,6 +62,7 @@ describe('Dashboard', () => { userId: dashboardInfo.userId, impressionId: 'id', loadStats: {}, + dashboardId: 123, }; const ChildrenComponent = () => <div>Test</div>; @@ -125,9 +127,30 @@ describe('Dashboard', () => { let refreshSpy; beforeEach(() => { - wrapper = setup({ activeFilters: OVERRIDE_FILTERS }); - wrapper.instance().appliedFilters = OVERRIDE_FILTERS; - prevProps = wrapper.instance().props; + jest.clearAllMocks(); + getRelatedCharts.mockReset(); + + // Create a deep copy of OVERRIDE_FILTERS for prevProps + const initialFilters = JSON.parse(JSON.stringify(OVERRIDE_FILTERS)); + + // Setup with required props + wrapper = setup({ + activeFilters: initialFilters, + editMode: false, // Ensure not in edit mode + chartConfiguration: {}, // Ensure chartConfiguration exists + ownDataCharts: {}, + }); + wrapper.instance().appliedFilters = initialFilters; + + // Set up prevProps with the initial filters + prevProps = { + ...wrapper.instance().props, + activeFilters: initialFilters, + editMode: false, + chartConfiguration: {}, + ownDataCharts: {}, + }; + refreshSpy = sinon.spy(wrapper.instance(), 'refreshCharts'); }); @@ -148,27 +171,36 @@ describe('Dashboard', () => { }); it('should not call refresh when there is no change', () => { + const sameFilters = JSON.parse(JSON.stringify(OVERRIDE_FILTERS)); wrapper.setProps({ - activeFilters: OVERRIDE_FILTERS, + activeFilters: sameFilters, }); wrapper.instance().componentDidUpdate(prevProps); expect(refreshSpy.callCount).toBe(0); - expect(wrapper.instance().appliedFilters).toBe(OVERRIDE_FILTERS); + expect(wrapper.instance().appliedFilters).toEqual(sameFilters); }); it('should call refresh when native filters changed', () => { getRelatedCharts.mockReturnValue([230]); + + const newFilters = { + ...OVERRIDE_FILTERS, + ...getAllActiveFilters({ + dataMask: dataMaskWith1Filter, + nativeFilters: singleNativeFiltersState.filters, + allSliceIds: [227, 229, 230], + }), + }; + wrapper.setProps({ - activeFilters: { - ...OVERRIDE_FILTERS, - ...getAllActiveFilters({ - dataMask: dataMaskWith1Filter, - nativeFilters: singleNativeFiltersState.filters, - allSliceIds: [227, 229, 230], - }), - }, + ...wrapper.instance().props, + activeFilters: newFilters, + editMode: false, + chartConfiguration: {}, }); + wrapper.instance().componentDidUpdate(prevProps); + expect(refreshSpy.callCount).toBe(1); expect(wrapper.instance().appliedFilters).toEqual({ ...OVERRIDE_FILTERS, @@ -190,34 +222,51 @@ describe('Dashboard', () => { it('should call refresh if a filter is added', () => { getRelatedCharts.mockReturnValue([1]); + const newFilter = { gender: { values: ['boy', 'girl'], scope: [1] }, }; - wrapper.setProps({ + + const newProps = { + ...wrapper.instance().props, activeFilters: newFilter, - }); + }; + wrapper.setProps(newProps); + wrapper.instance().componentDidUpdate(prevProps); + expect(refreshSpy.callCount).toBe(1); expect(wrapper.instance().appliedFilters).toEqual(newFilter); }); it('should call refresh if a filter is removed', () => { getRelatedCharts.mockReturnValue([]); - wrapper.setProps({ + + const newProps = { + ...wrapper.instance().props, activeFilters: {}, - }); + }; + wrapper.setProps(newProps); + wrapper.instance().componentDidUpdate(prevProps); + expect(refreshSpy.callCount).toBe(1); expect(wrapper.instance().appliedFilters).toEqual({}); }); it('should call refresh if a filter is changed', () => { getRelatedCharts.mockReturnValue([1]); + const newFilters = { ...OVERRIDE_FILTERS, '1_region': { values: ['Canada'], scope: [1] }, }; - wrapper.setProps({ + + const newProps = { + ...wrapper.instance().props, activeFilters: newFilters, - }); + }; + wrapper.setProps(newProps); + wrapper.instance().componentDidUpdate(prevProps); + expect(refreshSpy.callCount).toBe(1); expect(wrapper.instance().appliedFilters).toEqual(newFilters); expect(refreshSpy.getCall(0).args[0]).toEqual([1]); @@ -225,41 +274,58 @@ describe('Dashboard', () => { it('should call refresh with multiple chart ids', () => { getRelatedCharts.mockReturnValue([1, 2]); + const newFilters = { ...OVERRIDE_FILTERS, '2_country_name': { values: ['New Country'], scope: [1, 2] }, }; - wrapper.setProps({ + + const newProps = { + ...wrapper.instance().props, activeFilters: newFilters, - }); + }; + wrapper.setProps(newProps); + wrapper.instance().componentDidUpdate(prevProps); + expect(refreshSpy.callCount).toBe(1); expect(wrapper.instance().appliedFilters).toEqual(newFilters); expect(refreshSpy.getCall(0).args[0]).toEqual([1, 2]); }); it('should call refresh if a filter scope is changed', () => { + getRelatedCharts.mockReturnValue([2]); + const newFilters = { ...OVERRIDE_FILTERS, '3_country_name': { values: ['USA'], scope: [2] }, }; - wrapper.setProps({ + const newProps = { + ...wrapper.instance().props, activeFilters: newFilters, - }); + }; + wrapper.setProps(newProps); + wrapper.instance().componentDidUpdate(prevProps); + expect(refreshSpy.callCount).toBe(1); expect(refreshSpy.getCall(0).args[0]).toEqual([2]); }); it('should call refresh with empty [] if a filter is changed but scope is not applicable', () => { getRelatedCharts.mockReturnValue([]); + const newFilters = { ...OVERRIDE_FILTERS, '3_country_name': { values: ['CHINA'], scope: [] }, }; - wrapper.setProps({ + const newProps = { + ...wrapper.instance().props, activeFilters: newFilters, - }); + }; + wrapper.setProps(newProps); + wrapper.instance().componentDidUpdate(prevProps); + expect(refreshSpy.callCount).toBe(1); expect(refreshSpy.getCall(0).args[0]).toEqual([]); }); diff --git a/superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.test.tsx b/superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.test.tsx index 5d93fd6900..a28f85f779 100644 --- a/superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.test.tsx +++ b/superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.test.tsx @@ -20,6 +20,7 @@ import thunk from 'redux-thunk'; import configureStore from 'redux-mock-store'; import fetchMock from 'fetch-mock'; import { mockAllIsIntersecting } from 'react-intersection-observer/test-utils'; +import { act } from 'react-test-renderer'; import { fireEvent, render, waitFor } from 'spec/helpers/testing-library'; import { overwriteConfirmMetadata } from 'spec/fixtures/mockDashboardState'; @@ -61,10 +62,24 @@ test('requests update dashboard api when save button is clicked', async () => { last_modified_time: +new Date(), result: overwriteConfirmMetadata.data, }); + + // Initialize store with required state const store = mockStore({ - dashboardLayout: {}, + dashboardLayout: { + present: {}, + }, dashboardFilters: {}, + dashboardInfo: { + metadata: {}, + crossFiltersEnabled: false, + }, + charts: {}, + dataMask: {}, + nativeFilters: { + filters: {}, + }, }); + const { findByTestId } = render( <OverwriteConfirmModal overwriteConfirmMetadata={overwriteConfirmMetadata} @@ -74,17 +89,23 @@ test('requests update dashboard api when save button is clicked', async () => { store, }, ); + const saveButton = await findByTestId('overwrite-confirm-save-button'); expect(fetchMock.calls(updateDashboardEndpoint)).toHaveLength(0); - fireEvent.click(saveButton); - expect(fetchMock.calls(updateDashboardEndpoint)).toHaveLength(0); - mockAllIsIntersecting(true); - fireEvent.click(saveButton); - await waitFor(() => - expect(fetchMock.calls(updateDashboardEndpoint)?.[0]?.[1]?.body).toEqual( - JSON.stringify(overwriteConfirmMetadata.data), - ), - ); + + await act(async () => { + mockAllIsIntersecting(true); + fireEvent.click(saveButton); + }); + + // Wait for the fetch mock to be called and verify the request body + await waitFor(() => { + const calls = fetchMock.calls(updateDashboardEndpoint); + expect(calls).toHaveLength(1); + expect(JSON.parse(calls[0][1].body)).toEqual(overwriteConfirmMetadata.data); + }); + + // Verify the action was dispatched await waitFor(() => expect(store.getActions()).toContainEqual({ type: 'SET_OVERRIDE_CONFIRM', diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/FilterBarSettings.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/FilterBarSettings.test.tsx index 2863a07931..6b4e94a0c8 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/FilterBarSettings.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/FilterBarSettings.test.tsx @@ -20,12 +20,18 @@ import fetchMock from 'fetch-mock'; import { waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; -import { render, screen, within } from 'spec/helpers/testing-library'; +import { render, screen } from 'spec/helpers/testing-library'; import { DashboardInfo, FilterBarOrientation } from 'src/dashboard/types'; import * as mockedMessageActions from 'src/components/MessageToasts/actions'; import { FeatureFlag } from '@superset-ui/core'; import FilterBarSettings from '.'; +// Mock the number formatter registration to avoid duplicate warnings +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + getNumberFormatter: jest.fn(), +})); + const initialState: { dashboardInfo: DashboardInfo } = { dashboardInfo: { id: 1, @@ -135,10 +141,14 @@ test('Popover opens with "Vertical" selected', async () => { await setup(); userEvent.click(screen.getByLabelText('gear')); userEvent.hover(screen.getByText('Orientation of filter bar')); + expect(await screen.findByText('Vertical (Left)')).toBeInTheDocument(); expect(screen.getByText('Horizontal (Top)')).toBeInTheDocument(); + + // Look for the check icon in a different way + const menuItems = screen.getAllByRole('menuitem'); expect( - within(screen.getAllByRole('menuitem')[2]).getByLabelText('check'), + menuItems[2].querySelector('[aria-label="check"]'), ).toBeInTheDocument(); }); @@ -150,10 +160,13 @@ test('Popover opens with "Horizontal" selected', async () => { await setup({ filterBarOrientation: FilterBarOrientation.Horizontal }); userEvent.click(screen.getByLabelText('gear')); userEvent.hover(screen.getByText('Orientation of filter bar')); + expect(await screen.findByText('Vertical (Left)')).toBeInTheDocument(); expect(screen.getByText('Horizontal (Top)')).toBeInTheDocument(); + + const menuItems = screen.getAllByRole('menuitem'); expect( - within(screen.getAllByRole('menuitem')[3]).getByLabelText('check'), + menuItems[3].querySelector('[aria-label="check"]'), ).toBeInTheDocument(); }); @@ -176,10 +189,9 @@ test('On selection change, send request and update checked value', async () => { userEvent.click(screen.getByLabelText('gear')); userEvent.hover(screen.getByText('Orientation of filter bar')); - expect(await screen.findByText('Vertical (Left)')).toBeInTheDocument(); - expect(screen.getByText('Horizontal (Top)')).toBeInTheDocument(); + const menuItems = screen.getAllByRole('menuitem'); expect( - within(screen.getAllByRole('menuitem')[2]).getByLabelText('check'), + menuItems[2].querySelector('[aria-label="check"]'), ).toBeInTheDocument(); userEvent.click(screen.getByText('Horizontal (Top)')); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/utils.test.ts b/superset-frontend/src/dashboard/components/nativeFilters/utils.test.ts index 0f402ee134..fa27873732 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/utils.test.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/utils.test.ts @@ -45,6 +45,12 @@ describe('nativeFilterGate', () => { }); it('should return false for native filter chart with cross filter support', () => { + isFeatureEnabledMock.mockImplementation(feature => { + if (feature === FeatureFlag.DASHBOARD_CROSS_FILTERS) return false; + if (feature === FeatureFlag.DASHBOARD_NATIVE_FILTERS) return false; + return false; + }); + expect( nativeFilterGate([Behavior.NativeFilter, Behavior.InteractiveChart]), ).toEqual(false); @@ -56,6 +62,20 @@ describe('nativeFilterGate', () => { }); describe('with cross filters and experimental feature flag enabled', () => { + beforeAll(() => { + isFeatureEnabledMock = jest + .spyOn(uiCore, 'isFeatureEnabled') + .mockImplementation(feature => { + if (feature === FeatureFlag.DASHBOARD_CROSS_FILTERS) return true; + if (feature === FeatureFlag.DASHBOARD_NATIVE_FILTERS) return true; + return false; + }); + }); + + afterAll(() => { + isFeatureEnabledMock.mockRestore(); + }); + it('should return true for regular chart', () => { expect(nativeFilterGate([])).toEqual(true); }); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/utils.ts index ef577b354b..868803254c 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/utils.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/utils.ts @@ -27,6 +27,8 @@ import { getChartMetadataRegistry, QueryFormData, t, + isFeatureEnabled, + FeatureFlag, } from '@superset-ui/core'; import { LayoutItem } from 'src/dashboard/types'; import extractUrlParams from 'src/dashboard/util/extractUrlParams'; @@ -146,10 +148,22 @@ export function getExtraFormData( } export function nativeFilterGate(behaviors: Behavior[]): boolean { - return ( - !behaviors.includes(Behavior.NativeFilter) || - behaviors.includes(Behavior.InteractiveChart) - ); + const hasNativeFilter = behaviors.includes(Behavior.NativeFilter); + + // If it's not a native filter, always allow + if (!hasNativeFilter) { + return true; + } + + // Native filters are not allowed when feature flags are disabled + if (!isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS)) { + return false; + } + + // When feature flag is enabled: + // - Pure native filters are not allowed + // - Native filters with interactive chart behavior are allowed + return behaviors.includes(Behavior.InteractiveChart); } export const findTabsWithChartsInScope = (
