This is an automated email from the ASF dual-hosted git repository. sadpandajoe pushed a commit to branch test/native-filter-config-modal in repository https://gitbox.apache.org/repos/asf/superset.git
commit 39d3a699bab40b376c05d0364166241afbcb69c8 Author: Joe Li <[email protected]> AuthorDate: Fri Sep 25 15:22:24 2026 -0700 test(dashboard): cover native filter dependency cycles and config modal save gating Add unit coverage for the native filter config modal: - hasCircularDependency across acyclic, self, 2-node, 3-node and diamond-shaped dependency graphs - dependency picker excludes candidates that would create a cycle and validation flags only filters in the cycle - pending filter removal finalizes after its delay and restore cancels it - save stays blocked while a filter has validation errors and unsaved state stays true while a removal is pending Co-Authored-By: Claude Opus 5.5 <[email protected]> --- .../hooks/useFilterOperations.test.ts | 171 ++++++++++++++++++++- .../hooks/useModalSaveLogic.test.ts | 118 ++++++++++++++ .../nativeFilters/FiltersConfigModal/utils.test.ts | 42 +++++ 3 files changed, 329 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.test.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.test.ts index 42311ccf3c7..87a6d475341 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.test.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.test.ts @@ -16,8 +16,16 @@ * specific language governing permissions and limitations * under the License. */ -import { Behavior } from '@superset-ui/core'; -import { filterSupportsDependencies } from './useFilterOperations'; +import { useState } from 'react'; +import { renderHook, act } from '@testing-library/react'; +import { Behavior, Filter, Divider } from '@superset-ui/core'; +import type { FormInstance } from '@superset-ui/core/components'; +import { + filterSupportsDependencies, + useFilterOperations, +} from './useFilterOperations'; +import { useItemStateManager } from './useItemStateManager'; +import { NativeFiltersForm } from '../types'; const mockItems: Record<string, { value: Record<string, unknown> }> = { filter_select: { @@ -89,3 +97,162 @@ test('filterSupportsDependencies returns false for an unknown or missing filterT expect(filterSupportsDependencies('unknown_type')).toBe(false); expect(filterSupportsDependencies(undefined)).toBe(false); }); + +const buildCascadeConfigMap = (): Record<string, Filter | Divider> => + ({ + f1: { id: 'f1', filterType: 'filter_select', cascadeParentIds: [] }, + f2: { id: 'f2', filterType: 'filter_select', cascadeParentIds: [] }, + f3: { id: 'f3', filterType: 'filter_select', cascadeParentIds: [] }, + }) as unknown as Record<string, Filter | Divider>; + +const buildDependencyForm = ( + dependenciesByFilter: Record<string, string[]>, +): FormInstance<NativeFiltersForm> => { + const filters = Object.fromEntries( + Object.keys(dependenciesByFilter).map(id => [ + id, + { filterType: 'filter_select', dependencies: dependenciesByFilter[id] }, + ]), + ); + return { + getFieldValue: (field: string) => + field === 'filters' ? filters : undefined, + setFields: jest.fn(), + } as unknown as FormInstance<NativeFiltersForm>; +}; + +test('getAvailableFilters excludes a candidate that would create a circular dependency', () => { + // f2 already depends on f1, so offering f2 as a dependency of f1 would cycle + const form = buildDependencyForm({ f1: [], f2: ['f1'], f3: [] }); + const { result } = renderHook(() => + useFilterOperations({ + form, + filterState: { + removedItems: {}, + } as unknown as ReturnType<typeof useItemStateManager>, + filterIds: ['f1', 'f2', 'f3'], + filterConfigMap: buildCascadeConfigMap(), + handleModifyItem: jest.fn(), + setActiveItem: jest.fn(), + setSaveAlertVisible: jest.fn(), + }), + ); + + const availableIds = result.current + .getAvailableFilters('f1', id => id) + .map(option => option.value); + + expect(availableIds).toEqual(['f3']); +}); + +test('validateDependencies flags only the filters that participate in a cycle', () => { + // f1 <-> f2 form a cycle; f3 has no dependencies + const form = buildDependencyForm({ f1: ['f2'], f2: ['f1'], f3: [] }); + const { result } = renderHook(() => + useFilterOperations({ + form, + filterState: { + removedItems: {}, + } as unknown as ReturnType<typeof useItemStateManager>, + filterIds: ['f1', 'f2', 'f3'], + filterConfigMap: buildCascadeConfigMap(), + handleModifyItem: jest.fn(), + setActiveItem: jest.fn(), + setSaveAlertVisible: jest.fn(), + }), + ); + + act(() => { + result.current.validateDependencies(); + }); + + const setFieldsMock = form.setFields as jest.Mock; + const errorsById: Record<string, unknown[]> = {}; + setFieldsMock.mock.calls.forEach(([[field]]) => { + errorsById[field.name[1]] = field.errors; + }); + + expect(errorsById.f1.length).toBeGreaterThan(0); + expect(errorsById.f2.length).toBeGreaterThan(0); + expect(errorsById.f3).toEqual([]); +}); + +// Minimal state harness (rather than the full useItemStateManager) so these +// tests isolate handleRemoveFilter/restoreFilter's own timer handling, +// without useItemStateManager's independent removedItems cleanup effect +// masking a broken guard in the hook under test. +function useFilterOperationsHarness( + filterIds: string[], + form: FormInstance<NativeFiltersForm>, +) { + const [removedItems, setRemovedItems] = useState< + ReturnType<typeof useItemStateManager>['removedItems'] + >({}); + const [changes, setChanges] = useState< + ReturnType<typeof useItemStateManager>['changes'] + >({ modified: [], deleted: [], reordered: [] }); + const filterState = { + removedItems, + setRemovedItems, + changes, + setChanges, + } as unknown as ReturnType<typeof useItemStateManager>; + const filterOperations = useFilterOperations({ + form, + filterState, + filterIds, + filterConfigMap: {}, + handleModifyItem: jest.fn(), + setActiveItem: jest.fn(), + setSaveAlertVisible: jest.fn(), + }); + return { filterState, filterOperations }; +} + +test('handleRemoveFilter finalizes the removal once the pending delay elapses', () => { + jest.useFakeTimers(); + const form = { + getFieldValue: () => undefined, + } as unknown as FormInstance<NativeFiltersForm>; + const { result } = renderHook(() => useFilterOperationsHarness(['f1'], form)); + + act(() => { + result.current.filterOperations.handleRemoveFilter('f1'); + }); + expect(result.current.filterState.removedItems.f1).toEqual( + expect.objectContaining({ isPending: true }), + ); + + act(() => { + jest.advanceTimersByTime(5000); + }); + expect(result.current.filterState.removedItems.f1).toEqual({ + isPending: false, + }); + + jest.useRealTimers(); +}); + +test('restoreFilter cancels the pending removal before the delay elapses', () => { + jest.useFakeTimers(); + const form = { + getFieldValue: () => undefined, + } as unknown as FormInstance<NativeFiltersForm>; + const { result } = renderHook(() => useFilterOperationsHarness(['f1'], form)); + + act(() => { + result.current.filterOperations.handleRemoveFilter('f1'); + }); + act(() => { + result.current.filterOperations.restoreFilter('f1'); + }); + expect(result.current.filterState.removedItems.f1).toBeNull(); + + act(() => { + jest.advanceTimersByTime(5000); + }); + // the timer was cancelled by the restore, so it never finalizes the removal + expect(result.current.filterState.removedItems.f1).toBeNull(); + + jest.useRealTimers(); +}); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useModalSaveLogic.test.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useModalSaveLogic.test.ts new file mode 100644 index 00000000000..f3fdc5be770 --- /dev/null +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useModalSaveLogic.test.ts @@ -0,0 +1,118 @@ +/** + * 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 { renderHook } from '@testing-library/react'; +import type { FormInstance } from '@superset-ui/core/components'; +import { NativeFiltersForm } from '../types'; +import { useModalSaveLogic, ModalSaveLogicParams } from './useModalSaveLogic'; +import { ItemStateManager } from './useItemStateManager'; + +jest.mock('react-redux', () => ({ + useDispatch: () => jest.fn(), +})); + +const buildItemState = ( + overrides: Partial<ItemStateManager> = {}, +): ItemStateManager => ({ + changes: { modified: [], deleted: [], reordered: [] }, + newIds: [], + removedItems: {}, + erroredIds: [], + orderedIds: [], + renderedIds: [], + setChanges: jest.fn(), + setNewIds: jest.fn(), + setRemovedItems: jest.fn(), + setErroredIds: jest.fn(), + setOrderedIds: jest.fn(), + setRenderedIds: jest.fn(), + resetState: jest.fn(), + addToRendered: jest.fn(), + ...overrides, +}); + +const buildParams = ( + overrides: Partial<ModalSaveLogicParams> = {}, +): ModalSaveLogicParams => ({ + form: { + getFieldValue: jest.fn(), + isFieldsTouched: jest.fn(() => false), + getFieldsError: jest.fn(() => []), + } as unknown as FormInstance<NativeFiltersForm>, + configFormRef: { current: null }, + filterState: buildItemState(), + customizationState: buildItemState(), + filterIds: [], + chartCustomizationIds: [], + filterConfigMap: {}, + chartCustomizationConfigMap: {}, + initialFilterOrder: [], + initialCustomizationOrder: [], + unsavedFiltersIds: [], + currentItemId: '', + setActiveItem: jest.fn(), + onSave: jest.fn(), + canBeUsedAsDependency: jest.fn(() => true), + resetForm: jest.fn(), + ...overrides, +}); + +test('canSave is false while a filter has validation errors, even with pending changes', () => { + const params = buildParams({ + filterState: buildItemState({ + erroredIds: ['f1'], + changes: { modified: ['f1'], deleted: [], reordered: [] }, + }), + }); + + const { result } = renderHook(() => useModalSaveLogic(params)); + + expect(result.current.canSave).toBe(false); +}); + +test('canSave is true once a change exists and no filter has validation errors', () => { + const params = buildParams({ + filterState: buildItemState({ + changes: { modified: ['f1'], deleted: [], reordered: [] }, + }), + }); + + const { result } = renderHook(() => useModalSaveLogic(params)); + + expect(result.current.canSave).toBe(true); +}); + +test('hasUnsavedChanges is true while a filter removal is still pending', () => { + const params = buildParams({ + filterState: buildItemState({ + removedItems: { f1: { isPending: true, timerId: 1 } }, + }), + }); + + const { result } = renderHook(() => useModalSaveLogic(params)); + + expect(result.current.hasUnsavedChanges).toBe(true); +}); + +test('hasUnsavedChanges is false when nothing changed and no removal is pending', () => { + const params = buildParams(); + + const { result } = renderHook(() => useModalSaveLogic(params)); + + expect(result.current.hasUnsavedChanges).toBe(false); +}); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.test.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.test.ts index 97c7defca59..2e7f63d95ea 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.test.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.test.ts @@ -23,6 +23,7 @@ import { isChartCustomizationDivider, getItemType, getItemTypeInfo, + hasCircularDependency, NATIVE_FILTER_DIVIDER_PREFIX, CHART_CUSTOMIZATION_DIVIDER_PREFIX, NATIVE_FILTER_PREFIX, @@ -137,3 +138,44 @@ test('getItemTypeInfo returns correct info for customization type', () => { expect(info.dividerType).toBe(ChartCustomizationType.Divider); expect(info.itemTypeName).toBe('customization'); }); + +test('hasCircularDependency returns false for a linear chain with no cycle', () => { + const dependencyMap = new Map<string, string[]>([ + ['a', []], + ['b', ['a']], + ['c', ['b']], + ]); + expect(hasCircularDependency(dependencyMap, 'c')).toBe(false); +}); + +test('hasCircularDependency returns true for a filter that depends on itself', () => { + const dependencyMap = new Map<string, string[]>([['a', ['a']]]); + expect(hasCircularDependency(dependencyMap, 'a')).toBe(true); +}); + +test('hasCircularDependency returns true for a 2-node cycle', () => { + const dependencyMap = new Map<string, string[]>([ + ['a', ['b']], + ['b', ['a']], + ]); + expect(hasCircularDependency(dependencyMap, 'a')).toBe(true); +}); + +test('hasCircularDependency returns true for a longer 3-node cycle', () => { + const dependencyMap = new Map<string, string[]>([ + ['a', ['b']], + ['b', ['c']], + ['c', ['a']], + ]); + expect(hasCircularDependency(dependencyMap, 'a')).toBe(true); +}); + +test('hasCircularDependency returns false for a diamond with a shared ancestor', () => { + const dependencyMap = new Map<string, string[]>([ + ['d', ['b', 'c']], + ['b', ['a']], + ['c', ['a']], + ['a', []], + ]); + expect(hasCircularDependency(dependencyMap, 'd')).toBe(false); +});
