bito-code-review[bot] commented on code in PR #37681:
URL: https://github.com/apache/superset/pull/37681#discussion_r2800666170
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx:
##########
@@ -41,14 +41,47 @@ interface ActionButtonsProps {
chartCustomizationItems?: (ChartCustomization | ChartCustomizationDivider)[];
isApplyDisabled: boolean;
filterBarOrientation?: FilterBarOrientation;
+ hasOutOfScopeRequiredFilters?: boolean;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test coverage for new feature</b></div>
<div id="fix">
The new hasOutOfScopeRequiredFilters prop conditionally renders a tooltip
with an info icon, but the existing tests don't cover this behavior. Per BITO
rule [6262], tests should verify actual logic and behavior rather than just
component rendering.
</div>
</div>
<small><i>Code Review Run #96fdd6</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -132,540 +109,663 @@ describe('FilterBar', () => {
],
};
});
+}
- 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 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,
+ };
+}
+
+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,
+ },
+ dashboardState: {
+ ...stateWithoutNativeFilters.dashboardState,
+ activeTabs: ['ROOT_ID'],
+ },
+ 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);
+});
+
+test('FilterBar disables buttons when there are no filters', () => {
+ const props = createOpenedBarProps();
+ renderFilterBar(props, stateWithoutNativeFilters);
+
+ expect(screen.getByTestId(getTestId('clear-button'))).toBeDisabled();
+ expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
+});
+
+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 },
+ },
+ };
- renderWrapper(openedBarProps, stateWithDivider);
+ renderFilterBar(props, stateWithDivider);
- await act(async () => {
- jest.advanceTimersByTime(1000); // 1s
- });
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ 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());
+
+ const props = createOpenedBarProps();
+ renderFilterBar(props, stateWithoutNativeFilters);
- const title = await screen.findByText('Select time range');
- const description = await screen.findByText('Select year/month etc..');
+ expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
- 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();
+ // 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('create filter and apply it flow', async () => {
- renderWrapper(openedBarProps, stateWithoutNativeFilters);
- expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
+ const { container } = renderFilterBar(props, state);
+ expect(container).toBeInTheDocument();
+});
- await addFilterFlow();
+test('FilterBar renders correctly when filter has value but empty
extraFormData (auto-apply scenario)', () => {
+ const filterId = 'test-filter-auto-apply';
+ const props = createOpenedBarProps();
- expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
+ const filter = createFilter({
+ id: filterId,
+ requiredFirst: true,
+ controlValues: { enableEmptyFilter: true },
+ defaultDataMask: {
+ filterState: { value: ['value1'] },
+ extraFormData: {},
+ },
});
- 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 dataMask = createDataMask(filterId, ['value1'], {});
+ const state = createStateWithFilter(filter, dataMask);
+
+ renderFilterBar(props, state);
+
+ expect(screen.getByTestId(getTestId('filter-icon'))).toBeInTheDocument();
+ expect(screen.getByText('Filters and controls')).toBeInTheDocument();
+});
+
+test('FilterBar renders correctly when filter has complete extraFormData',
async () => {
+ const filterId = 'test-filter-complete';
+ const props = createOpenedBarProps();
+ const filter = createFilter({
+ id: filterId,
+ controlValues: { enableEmptyFilter: true },
+ defaultDataMask: {
+ filterState: { value: ['value1'] },
+ extraFormData: {
+ filters: [{ col: 'test_column', op: 'IN', val: ['value1'] }],
},
- };
+ },
+ });
+ const dataMask = createDataMask(filterId, ['value1'], {
+ filters: [{ col: 'test_column', op: 'IN', val: ['value1'] }],
+ });
+ const state = createStateWithFilter(filter, dataMask);
- const { container } = renderWrapper(openedBarProps, stateWithFilter);
- expect(container).toBeInTheDocument();
+ renderFilterBar(props, state);
+
+ await act(async () => {
+ jest.advanceTimersByTime(100);
});
- test('auto-applies filter when extraFormData is empty in applied state',
async () => {
- const filterId = 'test-filter-auto-apply';
- const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
+ expect(screen.getByTestId(getTestId('filter-icon'))).toBeInTheDocument();
+});
- 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: {},
+test('handleClearAll dispatches updateDataMask with value undefined for
filter_select', async () => {
+ const filterId = 'NATIVE_FILTER-clear-select';
+ const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
+ const selectFilter = createFilter({
+ id: filterId,
+ name: 'Region',
+ filterType: 'filter_select',
+ targets: [{ datasetId: 7, column: { name: 'region' } }],
+ defaultDataMask: { filterState: { value: null }, extraFormData: {} },
+ chartsInScope: [18],
+ });
+ const stateWithSelect = {
+ ...stateWithoutNativeFilters,
+ dashboardInfo: {
+ id: 1,
+ dash_edit_perm: true,
+ filterBarOrientation: FilterBarOrientation.Vertical,
+ metadata: {
+ native_filter_configuration: [selectFilter],
+ chart_configuration: {},
},
- };
-
- renderWrapper(openedBarProps, stateWithIncompleteFilter);
+ },
+ dashboardState: {
+ ...stateWithoutNativeFilters.dashboardState,
+ activeTabs: ['ROOT_ID'],
+ },
+ dataMask: {
+ [filterId]: createDataMask(filterId, ['East']),
+ },
+ nativeFilters: {
+ filters: { [filterId]: selectFilter },
+ filtersState: {},
+ },
+ };
- await act(async () => {
- jest.advanceTimersByTime(200);
- });
+ const props = createOpenedBarProps();
+ renderFilterBar(props, stateWithSelect);
+ await act(async () => {
+ jest.advanceTimersByTime(300);
+ });
- expect(screen.getByTestId(getTestId('filter-icon'))).toBeInTheDocument();
+ const clearBtn = screen.getByTestId(getTestId('clear-button'));
+ expect(clearBtn).not.toBeDisabled();
+ await act(async () => {
+ userEvent.click(clearBtn);
+ });
- updateDataMaskSpy.mockRestore();
+ expect(updateDataMaskSpy).toHaveBeenCalledWith(filterId, {
+ filterState: { value: undefined },
+ extraFormData: {},
});
+ updateDataMaskSpy.mockRestore();
+});
- test('renders correctly when filter has complete extraFormData', async () =>
{
- const filterId = 'test-filter-complete';
- const stateWithCompleteFilter = {
- ...stateWithoutNativeFilters,
- dashboardInfo: {
- id: 1,
- dash_edit_perm: true,
- },
- dataMask: {
- [filterId]: {
- id: filterId,
- filterState: { value: ['value1'] },
- extraFormData: {
- filters: [{ col: 'test_column', op: 'IN', val: ['value1'] }],
- },
- },
- },
- nativeFilters: {
- filters: {
- [filterId]: {
- id: filterId,
- name: 'Test Filter',
- filterType: 'filter_select',
- targets: [{ datasetId: 1, column: { name: 'test_column' } }],
- defaultDataMask: {
- filterState: { value: ['value1'] },
- extraFormData: {
- filters: [{ col: 'test_column', op: 'IN', val: ['value1'] }],
- },
- },
- controlValues: {
- enableEmptyFilter: true,
- },
- cascadeParentIds: [],
- scope: {
- rootPath: ['ROOT_ID'],
- excluded: [],
- },
- type: 'NATIVE_FILTER',
- description: '',
- chartsInScope: [],
- tabsInScope: [],
- },
- },
- filtersState: {},
+test('handleClearAll dispatches updateDataMask with [null, null] for
filter_range', async () => {
+ fetchMock.post('glob:*/api/v1/chart/data', {
+ result: [{ data: [{ min: 0, max: 100 }] }],
+ });
+ const filterId = 'NATIVE_FILTER-clear-range';
+ const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
+ const rangeFilter = createFilter({
+ id: filterId,
+ name: 'Age',
+ filterType: 'filter_range',
+ targets: [{ datasetId: 7, column: { name: 'age' } }],
+ defaultDataMask: { filterState: { value: null }, extraFormData: {} },
+ chartsInScope: [18],
+ });
+ const stateWithRange = {
+ ...stateWithoutNativeFilters,
+ dashboardInfo: {
+ id: 1,
+ dash_edit_perm: true,
+ filterBarOrientation: FilterBarOrientation.Vertical,
+ metadata: {
+ native_filter_configuration: [rangeFilter],
+ chart_configuration: {},
},
- };
+ },
+ dashboardState: {
+ ...stateWithoutNativeFilters.dashboardState,
+ activeTabs: ['ROOT_ID'],
+ },
+ dataMask: {
+ [filterId]: createDataMask(filterId, [10, 50]),
+ },
+ nativeFilters: {
+ filters: { [filterId]: rangeFilter },
+ filtersState: {},
+ },
+ };
- renderWrapper(openedBarProps, stateWithCompleteFilter);
+ const props = createOpenedBarProps();
+ renderFilterBar(props, stateWithRange);
+ await act(async () => {
+ jest.advanceTimersByTime(300);
+ });
- await act(async () => {
- jest.advanceTimersByTime(100);
- });
+ const clearBtn = screen.getByTestId(getTestId('clear-button'));
+ expect(clearBtn).not.toBeDisabled();
+ await act(async () => {
+ userEvent.click(clearBtn);
+ });
- expect(screen.getByTestId(getTestId('filter-icon'))).toBeInTheDocument();
+ expect(updateDataMaskSpy).toHaveBeenCalledWith(filterId, {
+ filterState: { value: [null, null] },
+ extraFormData: {},
});
+ updateDataMaskSpy.mockRestore();
+});
- test('handleClearAll dispatches updateDataMask with value null for
filter_select', async () => {
- const filterId = 'NATIVE_FILTER-clear-select';
- const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
- const selectFilterConfig = {
- id: filterId,
- name: 'Region',
- filterType: 'filter_select',
- targets: [{ datasetId: 7, column: { name: 'region' } }],
- defaultDataMask: { filterState: { value: null }, extraFormData: {} },
- cascadeParentIds: [],
- scope: { rootPath: ['ROOT_ID'], excluded: [] },
- type: 'NATIVE_FILTER',
- description: '',
- chartsInScope: [18],
- tabsInScope: [],
- };
- const stateWithSelect = {
- ...stateWithoutNativeFilters,
- dashboardInfo: {
- id: 1,
- dash_edit_perm: true,
- filterBarOrientation: FilterBarOrientation.Vertical,
- metadata: {
- native_filter_configuration: [selectFilterConfig],
- chart_configuration: {},
- },
+test('handleClearAll only dispatches for filters present in dataMask', async
() => {
+ const idInMask = 'NATIVE_FILTER-has-value';
+ const idNotInMask = 'NATIVE_FILTER-no-value';
+ const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
+ const filterInMask = createFilter({
+ id: idInMask,
+ name: 'A',
+ filterType: 'filter_select',
+ targets: [{ datasetId: 7, column: { name: 'x' } }],
+ chartsInScope: [18],
+ });
+ const filterNotInMask = createFilter({
+ id: idNotInMask,
+ name: 'B',
+ filterType: 'filter_select',
+ targets: [{ datasetId: 7, column: { name: 'x' } }],
+ chartsInScope: [18],
+ });
+ const stateWithTwoFilters = {
+ ...stateWithoutNativeFilters,
+ dashboardInfo: {
+ id: 1,
+ dash_edit_perm: true,
+ filterBarOrientation: FilterBarOrientation.Vertical,
+ metadata: {
+ native_filter_configuration: [filterInMask, filterNotInMask],
+ chart_configuration: {},
},
- dataMask: {
- [filterId]: {
- id: filterId,
- filterState: { value: ['East'] },
- extraFormData: {},
- },
+ },
+ dashboardState: {
+ ...stateWithoutNativeFilters.dashboardState,
+ activeTabs: ['ROOT_ID'],
+ },
+ dataMask: {
+ [idInMask]: createDataMask(idInMask, ['v']),
+ },
+ nativeFilters: {
+ filters: {
+ [idInMask]: filterInMask,
+ [idNotInMask]: filterNotInMask,
},
- };
+ filtersState: {},
+ },
+ };
- renderWrapper(openedBarProps, stateWithSelect);
- await act(async () => {
- jest.advanceTimersByTime(300);
- });
+ const props = createOpenedBarProps();
+ renderFilterBar(props, stateWithTwoFilters);
+ await act(async () => {
+ jest.advanceTimersByTime(300);
+ });
- const clearBtn = screen.getByTestId(getTestId('clear-button'));
- expect(clearBtn).not.toBeDisabled();
- await act(async () => {
- userEvent.click(clearBtn);
- });
+ const clearBtn = screen.getByTestId(getTestId('clear-button'));
+ await act(async () => {
+ userEvent.click(clearBtn);
+ });
- expect(updateDataMaskSpy).toHaveBeenCalledWith(filterId, {
- filterState: { value: undefined },
- extraFormData: {},
- });
- updateDataMaskSpy.mockRestore();
+ expect(updateDataMaskSpy).toHaveBeenCalledTimes(1);
+ expect(updateDataMaskSpy).toHaveBeenCalledWith(idInMask, {
+ filterState: { value: undefined },
+ extraFormData: {},
});
+ updateDataMaskSpy.mockRestore();
+});
- test('handleClearAll dispatches updateDataMask with value [null, null] for
filter_range', async () => {
- fetchMock.post('glob:*/api/v1/chart/data', {
- result: [{ data: [{ min: 0, max: 100 }] }],
- });
- const filterId = 'NATIVE_FILTER-clear-range';
- const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
- const rangeFilterConfig = {
- id: filterId,
- name: 'Age',
- filterType: 'filter_range',
- targets: [{ datasetId: 7, column: { name: 'age' } }],
- defaultDataMask: { filterState: { value: null }, extraFormData: {} },
- cascadeParentIds: [],
- scope: { rootPath: ['ROOT_ID'], excluded: [] },
- type: 'NATIVE_FILTER',
- description: '',
- chartsInScope: [18],
- tabsInScope: [],
- };
- const stateWithRange = {
- ...stateWithoutNativeFilters,
- dashboardInfo: {
- id: 1,
- dash_edit_perm: true,
- filterBarOrientation: FilterBarOrientation.Vertical,
- metadata: {
- native_filter_configuration: [rangeFilterConfig],
- chart_configuration: {},
- },
- },
- dataMask: {
- [filterId]: {
- id: filterId,
- filterState: { value: [10, 50] },
- extraFormData: {},
- },
- },
- };
+test('FilterBar Clear All does not clear out-of-scope filters', async () => {
+ const inScopeFilterId = 'in-scope-filter';
+ const outOfScopeRequiredFilterId = 'out-of-scope-required-filter';
+ const outOfScopeNonRequiredFilterId = 'out-of-scope-non-required-filter';
+
+ const dashboardLayoutWithTabs = {
+ ROOT_ID: { id: 'ROOT_ID', type: 'ROOT', children: ['TABS-1'] },
+ 'TABS-1': {
+ id: 'TABS-1',
+ type: 'TABS',
+ children: ['TAB-active', 'TAB-inactive'],
+ },
+ 'TAB-active': {
+ id: 'TAB-active',
+ type: 'TAB',
+ children: ['CHART_ROW-1'],
+ meta: { text: 'Active Tab' },
+ parents: ['ROOT_ID', 'TABS-1'],
+ },
+ 'TAB-inactive': {
+ id: 'TAB-inactive',
+ type: 'TAB',
+ children: ['CHART_ROW-2'],
+ meta: { text: 'Inactive Tab' },
+ parents: ['ROOT_ID', 'TABS-1'],
+ },
+ 'CHART_ROW-1': {
+ id: 'CHART_ROW-1',
+ type: 'CHART',
+ meta: { chartId: 1 },
+ parents: ['ROOT_ID', 'TABS-1', 'TAB-active'],
+ },
+ 'CHART_ROW-2': {
+ id: 'CHART_ROW-2',
+ type: 'CHART',
+ meta: { chartId: 2 },
+ parents: ['ROOT_ID', 'TABS-1', 'TAB-inactive'],
+ },
+ };
- renderWrapper(openedBarProps, stateWithRange);
- await act(async () => {
- jest.advanceTimersByTime(300);
- });
+ const inScopeFilter = createFilter({
+ id: inScopeFilterId,
+ name: 'In Scope Filter',
+ targets: [{ datasetId: 1, column: { name: 'column1' } }],
+ controlValues: { enableEmptyFilter: false },
+ chartsInScope: [1],
+ tabsInScope: ['TAB-active'],
+ });
- const clearBtn = screen.getByTestId(getTestId('clear-button'));
- expect(clearBtn).not.toBeDisabled();
- await act(async () => {
- userEvent.click(clearBtn);
- });
+ const outOfScopeRequiredFilter = createFilter({
+ id: outOfScopeRequiredFilterId,
+ name: 'Out of Scope Required Filter',
+ targets: [{ datasetId: 1, column: { name: 'column2' } }],
+ controlValues: { enableEmptyFilter: true },
+ chartsInScope: [2],
+ tabsInScope: ['TAB-inactive'],
+ });
- expect(updateDataMaskSpy).toHaveBeenCalledWith(filterId, {
- filterState: { value: [null, null] },
- extraFormData: {},
- });
- updateDataMaskSpy.mockRestore();
+ const outOfScopeNonRequiredFilter = createFilter({
+ id: outOfScopeNonRequiredFilterId,
+ name: 'Out of Scope Non-Required Filter',
+ targets: [{ datasetId: 1, column: { name: 'column3' } }],
+ controlValues: { enableEmptyFilter: false },
+ chartsInScope: [2],
+ tabsInScope: ['TAB-inactive'],
});
- test('handleClearAll only dispatches for filters present in dataMask', async
() => {
- const idInMask = 'NATIVE_FILTER-has-value';
- const idNotInMask = 'NATIVE_FILTER-no-value';
- const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
- const baseFilter = {
- targets: [{ datasetId: 7, column: { name: 'x' } }],
- defaultDataMask: { filterState: { value: null }, extraFormData: {} },
- cascadeParentIds: [],
- scope: { rootPath: ['ROOT_ID'], excluded: [] },
- type: 'NATIVE_FILTER',
- description: '',
- chartsInScope: [18],
- tabsInScope: [],
- };
- const stateWithTwoFiltersOneInMask = {
- ...stateWithoutNativeFilters,
- dashboardInfo: {
- id: 1,
- dash_edit_perm: true,
- filterBarOrientation: FilterBarOrientation.Vertical,
- metadata: {
- native_filter_configuration: [
- {
- ...baseFilter,
- id: idInMask,
- name: 'A',
- filterType: 'filter_select',
- },
- {
- ...baseFilter,
- id: idNotInMask,
- name: 'B',
- filterType: 'filter_select',
- },
- ],
- chart_configuration: {},
- },
+ const stateWithTabsAndFilters = {
+ ...stateWithoutNativeFilters,
+ dashboardLayout: {
+ present: dashboardLayoutWithTabs,
+ past: [],
+ future: [],
+ },
+ dashboardState: {
+ ...stateWithoutNativeFilters.dashboardState,
+ activeTabs: ['TAB-active'],
+ },
+ dashboardInfo: {
+ id: 1,
+ dash_edit_perm: true,
+ metadata: {
+ native_filter_configuration: [
+ inScopeFilter,
+ outOfScopeRequiredFilter,
+ outOfScopeNonRequiredFilter,
+ ],
},
- dataMask: {
- [idInMask]: {
- id: idInMask,
- filterState: { value: ['v'] },
- extraFormData: {},
- },
+ },
+ dataMask: {
+ [inScopeFilterId]: createDataMask(inScopeFilterId, ['value1'], {
+ filters: [{ col: 'column1', op: 'IN', val: ['value1'] }],
+ }),
+ [outOfScopeRequiredFilterId]: createDataMask(
+ outOfScopeRequiredFilterId,
+ ['value2'],
+ { filters: [{ col: 'column2', op: 'IN', val: ['value2'] }] },
+ ),
+ [outOfScopeNonRequiredFilterId]: createDataMask(
+ outOfScopeNonRequiredFilterId,
+ ['value3'],
+ { filters: [{ col: 'column3', op: 'IN', val: ['value3'] }] },
+ ),
+ },
+ nativeFilters: {
+ filters: {
+ [inScopeFilterId]: inScopeFilter,
+ [outOfScopeRequiredFilterId]: outOfScopeRequiredFilter,
+ [outOfScopeNonRequiredFilterId]: outOfScopeNonRequiredFilter,
},
- };
+ filtersState: {},
+ },
+ };
- renderWrapper(openedBarProps, stateWithTwoFiltersOneInMask);
- await act(async () => {
- jest.advanceTimersByTime(300);
- });
+ const props = createOpenedBarProps();
+ renderFilterBar(props, stateWithTabsAndFilters);
- const clearBtn = screen.getByTestId(getTestId('clear-button'));
- await act(async () => {
- userEvent.click(clearBtn);
- });
+ await act(async () => {
+ jest.advanceTimersByTime(100);
+ });
- expect(updateDataMaskSpy).toHaveBeenCalledTimes(1);
- expect(updateDataMaskSpy).toHaveBeenCalledWith(idInMask, {
- filterState: { value: undefined },
- extraFormData: {},
- });
- updateDataMaskSpy.mockRestore();
+ const clearButton = screen.getByTestId(getTestId('clear-button'));
+ expect(clearButton).toBeInTheDocument();
+
+ await userEvent.click(clearButton);
+
+ await act(async () => {
+ jest.advanceTimersByTime(100);
});
+
+ // Verify Clear All works without crashing
+ expect(clearButton).toBeInTheDocument();
});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test asserts rendering, not behavior</b></div>
<div id="fix">
The test claims to verify that 'Clear All does not clear out-of-scope
filters' but only checks that the clear button remains visible after clicking,
which merely confirms no crash. Per BITO rule [6262], tests must assert actual
behavior logic, not just rendering. The FilterBar's handleClearAll explicitly
skips out-of-scope filters, so the test should validate this by checking
dataMask state or dispatch calls.
</div>
</div>
<small><i>Code Review Run #96fdd6</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
