codeant-ai-for-open-source[bot] commented on code in PR #37681:
URL: https://github.com/apache/superset/pull/37681#discussion_r2776518822
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts:
##########
@@ -55,37 +66,63 @@ export const checkIsValidateError = (dataMask:
DataMaskStateWithId) => {
export const checkIsApplyDisabled = (
dataMaskSelected: DataMaskStateWithId,
dataMaskApplied: DataMaskStateWithId,
- filters: Filter[],
+ filtersInScope: Filter[],
+ allFilters?: Filter[],
) => {
- if (!checkIsValidateError(dataMaskSelected)) {
- return true;
- }
-
- const dataSelectedValues = Object.values(dataMaskSelected);
- const dataAppliedValues = Object.values(dataMaskApplied);
-
- const hasMissingRequiredFilter = filters.some(filter =>
- checkIsMissingRequiredValue(
- filter,
- dataMaskSelected?.[filter?.id]?.filterState,
- ),
- );
+ if (!checkIsValidateError(dataMaskSelected)) return true;
const selectedExtraFormData = getOnlyExtraFormData(dataMaskSelected);
const appliedExtraFormData = getOnlyExtraFormData(dataMaskApplied);
- const areEqual = areObjectsEqual(
+ // Check counts first
+ const selectedCount = Object.keys(selectedExtraFormData).length;
+ const appliedCount = Object.keys(appliedExtraFormData).length;
+
+ if (selectedCount !== appliedCount) return true;
+
+ // Check for changes
+ const dataEqual = areObjectsEqual(
selectedExtraFormData,
appliedExtraFormData,
{ ignoreUndefined: true },
);
- const result =
- areEqual ||
- dataSelectedValues.length !== dataAppliedValues.length ||
- hasMissingRequiredFilter;
+ // If no changes at all, Apply should be disabled
+ if (dataEqual) return true;
+
+ // Determine which filters to validate for required values
+ const inScopeFilterIds = new Set(filtersInScope.map(f => f.id));
+
+ // Check if changes are in-scope or out-of-scope
+ const hasInScopeChanges = filtersInScope.some(filter => {
+ const selected = selectedExtraFormData[filter.id];
+ const applied = appliedExtraFormData[filter.id];
+ return !isEqual(selected, applied);
+ });
+
+ // Determine which filters to validate for required values
+ const hasOutOfScopeChanges =
+ !hasInScopeChanges &&
+ allFilters?.some(filter => {
+ if (inScopeFilterIds.has(filter.id)) return false;
+ const selected = selectedExtraFormData[filter.id];
+ const applied = appliedExtraFormData[filter.id];
+ return !isEqual(selected, applied);
+ });
+
+ const shouldValidateAllRequired = hasOutOfScopeChanges && allFilters;
+ const filtersToValidateRequired = shouldValidateAllRequired
+ ? allFilters
+ : filtersInScope;
Review Comment:
**Suggestion:** The logic for determining when to validate all required
filters only considers out-of-scope changes if there are no in-scope changes
(`!hasInScopeChanges`), so if the user changes an in-scope filter while some
required out-of-scope filters remain empty (e.g., after "Clear all"), those
out-of-scope required filters are never validated and the Apply button can
become enabled prematurely, violating the intended validation behavior and the
PR's described scenario. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Apply enabled while required filters on other tabs empty.
- ❌ Intended cross-tab required-filter validation remains partially broken.
- ⚠️ Risk of charts rendering without required security filters.
```
</details>
```suggestion
// Check for out-of-scope changes independently of in-scope changes
const hasOutOfScopeChanges =
allFilters?.some(filter => {
if (inScopeFilterIds.has(filter.id)) return false;
const selected = selectedExtraFormData[filter.id];
const applied = appliedExtraFormData[filter.id];
return !isEqual(selected, applied);
}) || false;
const filtersToValidateRequired =
hasOutOfScopeChanges && allFilters ? allFilters : filtersInScope;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a dashboard with two tabs and two required native filters
following the PR's own
testing instructions: Filter A (required, scoped to Tab A) and Filter B
(required, scoped
to Tab B), both with default values, then save and refresh the dashboard.
2. On Tab A, click the "Clear all" button in the native filters bar so that
both Filter A
and Filter B are cleared in `dataMaskSelected`, while `dataMaskApplied`
still contains the
previously applied (non-empty) values for both filters (this is the exact
scenario the PR
description is targeting).
3. Still on Tab A, select a new value for Filter A only, leaving Filter B
(on Tab B)
empty. At this point, the code in `checkIsApplyDisabled` at
`superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts:66-126`
runs
with:
- `filtersInScope` containing Filter A only (active tab),
- `allFilters` containing both Filter A and Filter B,
- `selectedExtraFormData` and `appliedExtraFormData` differing for both
Filter A and
Filter B.
4. Inside `checkIsApplyDisabled`, the existing logic (lines 93-116 in
`utils.ts`)
computes:
- `hasInScopeChanges === true` because Filter A changed,
- `hasOutOfScopeChanges === false` because it is gated by
`!hasInScopeChanges`,
- `filtersToValidateRequired` is therefore set to `filtersInScope`
(Filter A only), so
`checkIsMissingRequiredValue` is never run for Filter B, and
`hasMissingRequiredFilter
=== false`; consequently `checkIsApplyDisabled` returns `false` and the
Apply button
becomes enabled even though required Filter B (out-of-scope) is empty,
contradicting
the PR's stated requirement that Apply must remain disabled until all
required filters
across tabs are filled.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/utils.ts
**Line:** 103:116
**Comment:**
*Logic Error: The logic for determining when to validate all required
filters only considers out-of-scope changes if there are no in-scope changes
(`!hasInScopeChanges`), so if the user changes an in-scope filter while some
required out-of-scope filters remain empty (e.g., after "Clear all"), those
out-of-scope required filters are never validated and the Apply button can
become enabled prematurely, violating the intended validation behavior and the
PR's described scenario.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37681&comment_hash=7cfc545c16cf35d58117a9b309e7fb6cd475fcb3a888b3c1d7fb6fae58540566&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37681&comment_hash=7cfc545c16cf35d58117a9b309e7fb6cd475fcb3a888b3c1d7fb6fae58540566&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -127,364 +104,488 @@ describe('FilterBar', () => {
],
};
});
+}
+
+function createFilter(overrides: Record<string, unknown> = {}) {
+ const id = (overrides.id as string) || 'test-filter';
+ return {
+ id,
+ name: 'Test Filter',
+ filterType: 'filter_select',
+ targets: [{ datasetId: 1, column: { name: 'test_column' } }],
+ defaultDataMask: { filterState: { value: null }, extraFormData: {} },
+ controlValues: {},
+ cascadeParentIds: [],
+ scope: { rootPath: ['ROOT_ID'], excluded: [] },
+ type: 'NATIVE_FILTER',
+ description: '',
+ chartsInScope: [],
+ tabsInScope: [],
+ ...overrides,
+ };
+}
+
+function createDataMask(
+ filterId: string,
+ value: unknown = undefined,
+ extraFormData: Record<string, unknown> = {},
+) {
+ return {
+ id: filterId,
+ filterState: { value },
+ extraFormData,
+ };
+}
+
+function createDivider(overrides: Record<string, unknown> = {}) {
+ return {
+ id: 'NATIVE_FILTER_DIVIDER-1',
+ type: 'DIVIDER',
+ scope: { rootPath: ['ROOT_ID'], excluded: [] },
+ title: 'Select time range',
+ description: 'Select year/month etc..',
+ chartsInScope: [],
+ tabsInScope: [],
+ ...overrides,
+ };
+}
- const getTimeRangeNoFilterMockUrl =
- 'glob:*/api/v1/time_range/?q=%27No%20filter%27';
- const getTimeRangeLastDayMockUrl =
- 'glob:*/api/v1/time_range/?q=%27Last%20day%27';
- const getTimeRangeLastWeekMockUrl =
- 'glob:*/api/v1/time_range/?q=%27Last%20week%27';
-
- beforeEach(() => {
- jest.clearAllMocks();
-
- fetchMock.removeRoute(getTimeRangeNoFilterMockUrl);
- fetchMock.get(
- getTimeRangeNoFilterMockUrl,
- {
- result: { since: '', until: '', timeRange: 'No filter' },
+function createStateWithFilter(
+ filter: ReturnType<typeof createFilter>,
+ dataMask: ReturnType<typeof createDataMask>,
+ dashboardInfoOverrides: Record<string, unknown> = {},
+) {
+ return {
+ ...stateWithoutNativeFilters,
+ dashboardInfo: {
+ id: 1,
+ dash_edit_perm: true,
+ metadata: {
+ native_filter_configuration: [filter],
},
- { name: getTimeRangeNoFilterMockUrl },
- );
-
- fetchMock.removeRoute(getTimeRangeLastDayMockUrl);
- fetchMock.get(
- getTimeRangeLastDayMockUrl,
- {
- result: {
- since: '2021-04-13T00:00:00',
- until: '2021-04-14T00:00:00',
- timeRange: 'Last day',
- },
+ ...dashboardInfoOverrides,
+ },
+ dataMask: { [filter.id]: dataMask },
+ nativeFilters: {
+ filters: { [filter.id]: filter },
+ filtersState: {},
+ },
+ };
+}
+
+function setupTimeRangeMocks() {
+ const urls = {
+ noFilter: 'glob:*/api/v1/time_range/?q=%27No%20filter%27',
+ lastDay: 'glob:*/api/v1/time_range/?q=%27Last%20day%27',
+ lastWeek: 'glob:*/api/v1/time_range/?q=%27Last%20week%27',
+ };
+
+ fetchMock.removeRoute(urls.noFilter);
+ fetchMock.get(
+ urls.noFilter,
+ { result: { since: '', until: '', timeRange: 'No filter' } },
+ { name: urls.noFilter },
+ );
+
+ fetchMock.removeRoute(urls.lastDay);
+ fetchMock.get(
+ urls.lastDay,
+ {
+ result: {
+ since: '2021-04-13T00:00:00',
+ until: '2021-04-14T00:00:00',
+ timeRange: 'Last day',
},
- { name: getTimeRangeLastDayMockUrl },
- );
-
- fetchMock.removeRoute(getTimeRangeLastWeekMockUrl);
- fetchMock.get(
- getTimeRangeLastWeekMockUrl,
- {
- result: {
- since: '2021-04-07T00:00:00',
- until: '2021-04-14T00:00:00',
- timeRange: 'Last week',
- },
+ },
+ { name: urls.lastDay },
+ );
+
+ fetchMock.removeRoute(urls.lastWeek);
+ fetchMock.get(
+ urls.lastWeek,
+ {
+ result: {
+ since: '2021-04-07T00:00:00',
+ until: '2021-04-14T00:00:00',
+ timeRange: 'Last week',
},
- { name: getTimeRangeLastWeekMockUrl },
- );
+ },
+ { name: urls.lastWeek },
+ );
+}
- mockedMakeApi.mockReturnValue(mockApi);
- });
+function renderFilterBar(
+ props: { filtersOpen: boolean; toggleFiltersBar: jest.Mock },
+ state?: object,
+) {
+ return render(
+ <FilterBar
+ orientation={FilterBarOrientation.Vertical}
+ verticalConfig={{
+ width: 280,
+ height: 400,
+ offset: 0,
+ ...props,
+ }}
+ />,
+ {
+ initialState: state,
+ useDnd: true,
+ useRedux: true,
+ useRouter: true,
+ },
+ );
+}
- const renderWrapper = (props = closedBarProps, state?: object) =>
- render(
- <FilterBar
- orientation={FilterBarOrientation.Vertical}
- verticalConfig={{
- width: 280,
- height: 400,
- offset: 0,
- ...props,
- }}
- />,
- {
- initialState: state,
- useDnd: true,
- useRedux: true,
- useRouter: true,
- },
- );
+test('FilterBar renders without crashing', () => {
+ const props = createClosedBarProps();
+ const { container } = renderFilterBar(props);
+ expect(container).toBeInTheDocument();
+});
- test('should render', () => {
- const { container } = renderWrapper();
- expect(container).toBeInTheDocument();
- });
+test('FilterBar renders "Filters and controls" heading', () => {
+ const props = createClosedBarProps();
+ renderFilterBar(props);
+ expect(screen.getByText('Filters and controls')).toBeInTheDocument();
+});
- test('should render the "Filters and controls" heading', () => {
- renderWrapper();
- expect(screen.getByText('Filters and controls')).toBeInTheDocument();
- });
+test('FilterBar renders "Clear all" button', () => {
+ const props = createClosedBarProps();
+ renderFilterBar(props);
+ expect(screen.getByText('Clear all')).toBeInTheDocument();
+});
- test('should render the "Clear all" option', () => {
- renderWrapper();
- expect(screen.getByText('Clear all')).toBeInTheDocument();
- });
+test('FilterBar renders "Apply filters" button', () => {
+ const props = createClosedBarProps();
+ renderFilterBar(props);
+ expect(screen.getByText('Apply filters')).toBeInTheDocument();
+});
- test('should render the "Apply filters" option', () => {
- renderWrapper();
- expect(screen.getByText('Apply filters')).toBeInTheDocument();
- });
+test('FilterBar renders collapse icon', () => {
+ const props = createClosedBarProps();
+ renderFilterBar(props);
+ expect(
+ screen.getByRole('img', { name: 'vertical-align' }),
+ ).toBeInTheDocument();
+});
- test('should render the collapse icon', () => {
- renderWrapper();
- expect(
- screen.getByRole('img', { name: 'vertical-align' }),
- ).toBeInTheDocument();
- });
+test('FilterBar renders filter icon', () => {
+ const props = createClosedBarProps();
+ renderFilterBar(props);
+ expect(screen.getByRole('img', { name: 'filter' })).toBeInTheDocument();
+});
- test('should render the filter icon', () => {
- renderWrapper();
- expect(screen.getByRole('img', { name: 'filter' })).toBeInTheDocument();
- });
+test('FilterBar calls toggleFiltersBar when collapse icon is clicked', () => {
+ const toggleFiltersBar = jest.fn();
+ const props = createClosedBarProps(toggleFiltersBar);
+ renderFilterBar(props);
- test('should toggle', () => {
- renderWrapper();
- const collapse = screen.getByRole('img', {
- name: 'vertical-align',
- });
- expect(toggleFiltersBar).not.toHaveBeenCalled();
- userEvent.click(collapse);
- expect(toggleFiltersBar).toHaveBeenCalled();
- });
+ const collapse = screen.getByRole('img', { name: 'vertical-align' });
+ expect(toggleFiltersBar).not.toHaveBeenCalled();
- test('open filter bar', () => {
- renderWrapper();
- expect(screen.getByTestId(getTestId('filter-icon'))).toBeInTheDocument();
- expect(screen.getByTestId(getTestId('expand-button'))).toBeInTheDocument();
+ userEvent.click(collapse);
+ expect(toggleFiltersBar).toHaveBeenCalled();
+});
- userEvent.click(screen.getByTestId(getTestId('collapsable')));
- expect(toggleFiltersBar).toHaveBeenCalledWith(true);
- });
+test('FilterBar opens when expand button is clicked', () => {
+ const toggleFiltersBar = jest.fn();
+ const props = createClosedBarProps(toggleFiltersBar);
+ renderFilterBar(props);
- test('no edit filter button by disabled permissions', () => {
- renderWrapper(openedBarProps, {
- ...stateWithoutNativeFilters,
- dashboardInfo: { metadata: {} },
- });
+ expect(screen.getByTestId(getTestId('filter-icon'))).toBeInTheDocument();
+ expect(screen.getByTestId(getTestId('expand-button'))).toBeInTheDocument();
- expect(
- screen.queryByTestId(getTestId('create-filter')),
- ).not.toBeInTheDocument();
- });
+ userEvent.click(screen.getByTestId(getTestId('collapsable')));
+ expect(toggleFiltersBar).toHaveBeenCalledWith(true);
+});
- test('close filter bar', () => {
- renderWrapper(openedBarProps);
- const collapseButton = screen.getByTestId(getTestId('collapse-button'));
+test('FilterBar hides edit filter button when user lacks permissions', () => {
+ const props = createOpenedBarProps();
+ const stateWithoutPermissions = {
+ ...stateWithoutNativeFilters,
+ dashboardInfo: { metadata: {} },
+ };
- expect(collapseButton).toBeInTheDocument();
- userEvent.click(collapseButton);
+ renderFilterBar(props, stateWithoutPermissions);
- expect(toggleFiltersBar).toHaveBeenCalledWith(false);
- });
+ expect(
+ screen.queryByTestId(getTestId('create-filter')),
+ ).not.toBeInTheDocument();
+});
- test('no filters', () => {
- renderWrapper(openedBarProps, stateWithoutNativeFilters);
+test('FilterBar closes when collapse button is clicked', () => {
+ const toggleFiltersBar = jest.fn();
+ const props = createOpenedBarProps(toggleFiltersBar);
+ renderFilterBar(props);
- expect(screen.getByTestId(getTestId('clear-button'))).toBeDisabled();
- expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
- });
+ const collapseButton = screen.getByTestId(getTestId('collapse-button'));
+ expect(collapseButton).toBeInTheDocument();
- test('renders dividers', async () => {
- const divider = {
- id: 'NATIVE_FILTER_DIVIDER-1',
- type: 'DIVIDER',
- scope: {
- rootPath: ['ROOT_ID'],
- excluded: [],
- },
- title: 'Select time range',
- description: 'Select year/month etc..',
- chartsInScope: [],
- tabsInScope: [],
- };
- const stateWithDivider = {
- ...stateWithoutNativeFilters,
- dashboardInfo: {
- ...stateWithoutNativeFilters.dashboardInfo,
- metadata: {
- ...stateWithoutNativeFilters.dashboardInfo.metadata,
- native_filter_configuration: [divider],
- },
- },
- nativeFilters: {
- filters: {
- 'NATIVE_FILTER_DIVIDER-1': divider,
- },
- },
- };
+ userEvent.click(collapseButton);
+ expect(toggleFiltersBar).toHaveBeenCalledWith(false);
+});
- renderWrapper(openedBarProps, stateWithDivider);
+test('FilterBar disables buttons when there are no filters', () => {
+ const props = createOpenedBarProps();
+ renderFilterBar(props, stateWithoutNativeFilters);
- await act(async () => {
- jest.advanceTimersByTime(1000); // 1s
- });
+ expect(screen.getByTestId(getTestId('clear-button'))).toBeDisabled();
+ expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
+});
- const title = await screen.findByText('Select time range');
- const description = await screen.findByText('Select year/month etc..');
+test('FilterBar renders dividers with title and description', async () => {
+ const props = createOpenedBarProps();
+ const divider = createDivider();
+ const stateWithDivider = {
+ ...stateWithoutNativeFilters,
+ dashboardInfo: {
+ ...stateWithoutNativeFilters.dashboardInfo,
+ metadata: {
+ ...stateWithoutNativeFilters.dashboardInfo.metadata,
+ native_filter_configuration: [divider],
+ },
+ },
+ nativeFilters: {
+ filters: { [divider.id]: divider },
+ },
+ };
- expect(title.tagName).toBe('H3');
- expect(description.tagName).toBe('P');
- // Do not enable buttons if there are not filters
- expect(screen.getByTestId(getTestId('clear-button'))).toBeDisabled();
- expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
+ renderFilterBar(props, stateWithDivider);
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
});
- test('create filter and apply it flow', async () => {
- renderWrapper(openedBarProps, stateWithoutNativeFilters);
- expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
+ const title = await screen.findByText('Select time range');
+ const description = await screen.findByText('Select year/month etc..');
+
+ expect(title.tagName).toBe('H3');
+ expect(description.tagName).toBe('P');
+ expect(screen.getByTestId(getTestId('clear-button'))).toBeDisabled();
+ expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
+});
+
+test('FilterBar apply button is disabled after creating a filter', async () =>
{
+ setupTimeRangeMocks();
+ mockedMakeApi.mockReturnValue(createMockApi());
- await addFilterFlow();
+ const props = createOpenedBarProps();
+ renderFilterBar(props, stateWithoutNativeFilters);
- expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
+ expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
+
+ // Simulate add filter flow
+ userEvent.click(screen.getByTestId(getTestId('collapsable')));
+ userEvent.click(screen.getByLabelText('setting'));
+ userEvent.click(screen.getByText('Add or edit filters and controls'));
+ userEvent.click(screen.getByText('Value'));
+ userEvent.click(screen.getByText('Time range'));
+ userEvent.type(
+ screen.getByTestId(getModalTestId('name-input')),
+ 'Time filter 1',
+ );
+ userEvent.click(screen.getByText('Save'));
+
+ expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
+});
+
+test('FilterBar renders without errors when filter has required
controlValues', () => {
+ const props = createOpenedBarProps();
+ const filter = createFilter({
+ id: 'test-filter',
+ controlValues: { enableEmptyFilter: true },
});
+ const dataMask = createDataMask('test-filter', undefined, {});
+ const state = createStateWithFilter(filter, dataMask);
- test('should render without errors with proper state setup', () => {
- const stateWithFilter = {
- ...stateWithoutNativeFilters,
- dashboardInfo: {
- id: 1,
- },
- dataMask: {
- 'test-filter': {
- id: 'test-filter',
- filterState: { value: undefined },
- extraFormData: {},
- },
- },
- nativeFilters: {
- filters: {
- 'test-filter': {
- id: 'test-filter',
- name: 'Test Filter',
- filterType: 'filter_select',
- targets: [{ datasetId: 1, column: { name: 'test_column' } }],
- defaultDataMask: {
- filterState: { value: undefined },
- extraFormData: {},
- },
- controlValues: {
- enableEmptyFilter: true,
- },
- cascadeParentIds: [],
- scope: {
- rootPath: ['ROOT_ID'],
- excluded: [],
- },
- type: 'NATIVE_FILTER',
- description: '',
- chartsInScope: [],
- tabsInScope: [],
- },
- },
- filtersState: {},
- },
- };
+ const { container } = renderFilterBar(props, state);
+ expect(container).toBeInTheDocument();
+});
- const { container } = renderWrapper(openedBarProps, stateWithFilter);
- expect(container).toBeInTheDocument();
+test('FilterBar auto-applies filter when extraFormData is empty in applied
state', async () => {
+ const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
+
+ const filterId = 'test-filter-auto-apply';
+ const props = createOpenedBarProps();
+ const filter = createFilter({
+ id: filterId,
+ controlValues: { enableEmptyFilter: true },
+ defaultDataMask: {
+ filterState: { value: ['value1', 'value2'] },
+ extraFormData: {},
+ },
});
+ const dataMask = createDataMask(filterId, ['value1', 'value2'], {});
+ const state = createStateWithFilter(filter, dataMask);
- test('auto-applies filter when extraFormData is empty in applied state',
async () => {
- const filterId = 'test-filter-auto-apply';
- const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
+ renderFilterBar(props, state);
- const stateWithIncompleteFilter = {
- ...stateWithoutNativeFilters,
- dashboardInfo: {
- id: 1,
- dash_edit_perm: true,
- },
- dataMask: {
- [filterId]: {
- id: filterId,
- filterState: { value: ['value1', 'value2'] },
- extraFormData: {},
- },
- },
- nativeFilters: {
- filters: {
- [filterId]: {
- id: filterId,
- name: 'Test Filter',
- filterType: 'filter_select',
- targets: [{ datasetId: 1, column: { name: 'test_column' } }],
- defaultDataMask: {
- filterState: { value: ['value1', 'value2'] },
- extraFormData: {},
- },
- controlValues: {
- enableEmptyFilter: true,
- },
- cascadeParentIds: [],
- scope: {
- rootPath: ['ROOT_ID'],
- excluded: [],
- },
- type: 'NATIVE_FILTER',
- description: '',
- chartsInScope: [],
- tabsInScope: [],
- },
- },
- filtersState: {},
+ await act(async () => {
+ jest.advanceTimersByTime(200);
+ });
+
+ expect(screen.getByTestId(getTestId('filter-icon'))).toBeInTheDocument();
+
Review Comment:
**Suggestion:** The test that is supposed to verify that a filter is
auto-applied when `extraFormData` is empty only creates a spy on
`updateDataMask` but never asserts on it, so the test will still pass even if
the auto-apply logic stops calling `updateDataMask`, silently losing coverage
of this behavior. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Auto-apply behavior can regress without tests failing.
- ⚠️ Dashboard filter auto-apply not robustly covered by tests.
```
</details>
```suggestion
expect(updateDataMaskSpy).toHaveBeenCalled();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx`
and locate the test `FilterBar auto-applies filter when extraFormData is
empty in applied
state` at lines 404-431.
2. Observe that this test creates a spy with `jest.spyOn(dataMaskActions,
'updateDataMask')` but the only assertion is
`expect(screen.getByTestId(getTestId('filter-icon'))).toBeInTheDocument();`,
which merely
checks that the FilterBar icon is rendered.
3. Note that `updateDataMaskSpy` is never used in an assertion (e.g.,
`toHaveBeenCalled`),
and is only restored at the end; therefore, the test would still pass if the
FilterBar
auto-apply logic stopped calling `dataMaskActions.updateDataMask`.
4. To verify, temporarily modify the auto-apply logic in
`superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx`
to skip
calling `updateDataMask` when `extraFormData` is empty, then run this single
test file
with Jest; the test will continue to pass, demonstrating that it does not
actually
validate the auto-apply behavior it is named for.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx
**Line:** 427:428
**Comment:**
*Logic Error: The test that is supposed to verify that a filter is
auto-applied when `extraFormData` is empty only creates a spy on
`updateDataMask` but never asserts on it, so the test will still pass even if
the auto-apply logic stops calling `updateDataMask`, silently losing coverage
of this behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37681&comment_hash=971bac9dbdf58b447263f4ca288929c587c4fdff6f4a2c81fc864427ca61eb6b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37681&comment_hash=971bac9dbdf58b447263f4ca288929c587c4fdff6f4a2c81fc864427ca61eb6b&reaction=dislike'>👎</a>
--
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]
