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 7d2eff9a4d8d918e7483e0eb6270dfb11b3a871e Author: Evan Rusackas <[email protected]> AuthorDate: Tue Jan 21 14:12:03 2025 -0700 Revert "getting tests to pass" This reverts commit d6f78f0f7c7ade0f08a80e822855f1e6485f46df. --- .../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, 47 insertions(+), 191 deletions(-) diff --git a/superset-frontend/src/dashboard/actions/dashboardState.test.js b/superset-frontend/src/dashboard/actions/dashboardState.test.js index 0bb4fbffda..4e30c38fb1 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.test.js +++ b/superset-frontend/src/dashboard/actions/dashboardState.test.js @@ -57,7 +57,6 @@ describe('dashboardState actions', () => { present: mockDashboardData.positions, future: {}, }, - charts: {}, }; const newDashboardData = mockDashboardData; @@ -154,12 +153,7 @@ describe('dashboardState actions', () => { it('dispatches SET_OVERRIDE_CONFIRM when an inspect value has diff', async () => { const id = 192; - const { getState, dispatch } = setup({ - charts: { - 123: { id: 123 }, - 456: { id: 456 }, - }, - }); + const { getState, dispatch } = setup(); const thunk = saveDashboardRequest( newDashboardData, id, @@ -178,12 +172,7 @@ describe('dashboardState actions', () => { it('should post dashboard data with after confirm the overwrite values', async () => { const id = 192; - const { getState, dispatch } = setup({ - charts: { - 123: { id: 123 }, - 456: { id: 456 }, - }, - }); + const { getState, dispatch } = setup(); 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 3d841661b1..e3421ee040 100644 --- a/superset-frontend/src/dashboard/components/Dashboard.test.jsx +++ b/superset-frontend/src/dashboard/components/Dashboard.test.jsx @@ -48,7 +48,6 @@ describe('Dashboard', () => { removeSliceFromDashboard() {}, triggerQuery() {}, logEvent() {}, - clearDataMaskState() {}, }, dashboardState, dashboardInfo, @@ -62,7 +61,6 @@ describe('Dashboard', () => { userId: dashboardInfo.userId, impressionId: 'id', loadStats: {}, - dashboardId: 123, }; const ChildrenComponent = () => <div>Test</div>; @@ -127,30 +125,9 @@ describe('Dashboard', () => { let refreshSpy; beforeEach(() => { - 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: {}, - }; - + wrapper = setup({ activeFilters: OVERRIDE_FILTERS }); + wrapper.instance().appliedFilters = OVERRIDE_FILTERS; + prevProps = wrapper.instance().props; refreshSpy = sinon.spy(wrapper.instance(), 'refreshCharts'); }); @@ -171,36 +148,27 @@ describe('Dashboard', () => { }); it('should not call refresh when there is no change', () => { - const sameFilters = JSON.parse(JSON.stringify(OVERRIDE_FILTERS)); wrapper.setProps({ - activeFilters: sameFilters, + activeFilters: OVERRIDE_FILTERS, }); wrapper.instance().componentDidUpdate(prevProps); expect(refreshSpy.callCount).toBe(0); - expect(wrapper.instance().appliedFilters).toEqual(sameFilters); + expect(wrapper.instance().appliedFilters).toBe(OVERRIDE_FILTERS); }); 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({ - ...wrapper.instance().props, - activeFilters: newFilters, - editMode: false, - chartConfiguration: {}, + activeFilters: { + ...OVERRIDE_FILTERS, + ...getAllActiveFilters({ + dataMask: dataMaskWith1Filter, + nativeFilters: singleNativeFiltersState.filters, + allSliceIds: [227, 229, 230], + }), + }, }); - wrapper.instance().componentDidUpdate(prevProps); - expect(refreshSpy.callCount).toBe(1); expect(wrapper.instance().appliedFilters).toEqual({ ...OVERRIDE_FILTERS, @@ -222,51 +190,34 @@ describe('Dashboard', () => { it('should call refresh if a filter is added', () => { getRelatedCharts.mockReturnValue([1]); - const newFilter = { gender: { values: ['boy', 'girl'], scope: [1] }, }; - - const newProps = { - ...wrapper.instance().props, + wrapper.setProps({ 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([]); - - const newProps = { - ...wrapper.instance().props, + wrapper.setProps({ 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] }, }; - - const newProps = { - ...wrapper.instance().props, + wrapper.setProps({ 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]); @@ -274,58 +225,41 @@ 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] }, }; - - const newProps = { - ...wrapper.instance().props, + wrapper.setProps({ 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] }, }; - const newProps = { - ...wrapper.instance().props, + wrapper.setProps({ 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: [] }, }; - const newProps = { - ...wrapper.instance().props, + wrapper.setProps({ 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 a28f85f779..5d93fd6900 100644 --- a/superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.test.tsx +++ b/superset-frontend/src/dashboard/components/OverwriteConfirm/OverwriteConfirmModal.test.tsx @@ -20,7 +20,6 @@ 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'; @@ -62,24 +61,10 @@ 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: { - present: {}, - }, + dashboardLayout: {}, dashboardFilters: {}, - dashboardInfo: { - metadata: {}, - crossFiltersEnabled: false, - }, - charts: {}, - dataMask: {}, - nativeFilters: { - filters: {}, - }, }); - const { findByTestId } = render( <OverwriteConfirmModal overwriteConfirmMetadata={overwriteConfirmMetadata} @@ -89,23 +74,17 @@ 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); - - 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 + 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 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 6b4e94a0c8..2863a07931 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,18 +20,12 @@ import fetchMock from 'fetch-mock'; import { waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; -import { render, screen } from 'spec/helpers/testing-library'; +import { render, screen, within } 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, @@ -141,14 +135,10 @@ 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( - menuItems[2].querySelector('[aria-label="check"]'), + within(screen.getAllByRole('menuitem')[2]).getByLabelText('check'), ).toBeInTheDocument(); }); @@ -160,13 +150,10 @@ 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( - menuItems[3].querySelector('[aria-label="check"]'), + within(screen.getAllByRole('menuitem')[3]).getByLabelText('check'), ).toBeInTheDocument(); }); @@ -189,9 +176,10 @@ test('On selection change, send request and update checked value', async () => { userEvent.click(screen.getByLabelText('gear')); userEvent.hover(screen.getByText('Orientation of filter bar')); - const menuItems = screen.getAllByRole('menuitem'); + expect(await screen.findByText('Vertical (Left)')).toBeInTheDocument(); + expect(screen.getByText('Horizontal (Top)')).toBeInTheDocument(); expect( - menuItems[2].querySelector('[aria-label="check"]'), + within(screen.getAllByRole('menuitem')[2]).getByLabelText('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 fa27873732..0f402ee134 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/utils.test.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/utils.test.ts @@ -45,12 +45,6 @@ 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); @@ -62,20 +56,6 @@ 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 868803254c..ef577b354b 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/utils.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/utils.ts @@ -27,8 +27,6 @@ import { getChartMetadataRegistry, QueryFormData, t, - isFeatureEnabled, - FeatureFlag, } from '@superset-ui/core'; import { LayoutItem } from 'src/dashboard/types'; import extractUrlParams from 'src/dashboard/util/extractUrlParams'; @@ -148,22 +146,10 @@ export function getExtraFormData( } export function nativeFilterGate(behaviors: Behavior[]): boolean { - 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); + return ( + !behaviors.includes(Behavior.NativeFilter) || + behaviors.includes(Behavior.InteractiveChart) + ); } export const findTabsWithChartsInScope = (
