bito-code-review[bot] commented on code in PR #38568: URL: https://github.com/apache/superset/pull/38568#discussion_r2943876706
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1041,425 +1072,492 @@ const FiltersConfigForm = (
items={[
...(itemTypeField !== 'filter_time'
? [
- {
- key:
`${filterId}-${FilterPanels.configuration.key}`,
- forceRender: true,
- label: isChartCustomization
- ? CustomizationPanels.configuration.name
- : FilterPanels.configuration.name,
- children: (
- <>
- {canDependOnOtherFilters &&
- (hasAvailableFilters ||
- dependencies.length > 0) && (
- <StyledRowFormItem
+ {
+ key: `${filterId}-${FilterPanels.configuration.key}`,
+ forceRender: true,
+ label: isChartCustomization
+ ? CustomizationPanels.configuration.name
+ : FilterPanels.configuration.name,
+ children: (
+ <>
+ {canDependOnOtherFilters &&
+ (hasAvailableFilters ||
+ dependencies.length > 0) && (
+ <StyledRowFormItem
+ expanded={expanded}
+ name={[
+ 'filters',
+ filterId,
+ 'dependencies',
+ ]}
+ initialValue={dependencies}
+ >
+ <DependencyList
+ availableFilters={availableFilters}
+ dependencies={dependencies}
+ onDependenciesChange={dependencies => {
+ setNativeFilterFieldValues(
+ form,
+ filterId,
+ {
+ dependencies,
+ },
+ );
+ forceUpdate();
+ validateDependencies();
+ formChanged();
+ }}
+ getDependencySuggestion={() =>
+ getDependencySuggestion(filterId)
+ }
+ >
+ {hasTimeDependency
+ ? timeColumn
+ : undefined}
+ </DependencyList>
+ </StyledRowFormItem>
+ )}
+ {hasDataset && hasAdditionalFilters && (
+ <FormItem
+ name={['filters', filterId, 'preFilter']}
+ >
+ <CollapsibleControl
+ initialValue={hasPreFilter}
+ title={t('Pre-filter available values')}
+ tooltip={t(`Add filter clauses to control
the filter's source query,
+ though only in the context of the autocomplete i.e., these
conditions
+ do not impact how the filter is applied to the dashboard.
This is useful
+ when you want to improve the query's performance by only
scanning a subset
+ of the underlying data or limit the available values
displayed in the filter.`)}
+ onChange={checked => {
+ formChanged();
+ if (checked) {
+ validatePreFilter();
+ }
+ }}
+ >
+ <StyledRowSubFormItem
expanded={expanded}
name={[
'filters',
filterId,
- 'dependencies',
+ 'adhoc_filters',
+ ]}
+ css={{ width: INPUT_WIDTH }}
+ initialValue={
+ filterToEdit?.adhoc_filters
+ }
+ required
+ rules={[
+ {
+ validator: preFilterValidator,
+ },
]}
- initialValue={dependencies}
>
- <DependencyList
- availableFilters={availableFilters}
- dependencies={dependencies}
- onDependenciesChange={dependencies => {
+ <AdhocFilterControl
+ columns={
+ datasetDetails?.columns?.filter(
+ (c: ColumnMeta) => c.filterable,
+ ) || []
+ }
+ savedMetrics={
+ datasetDetails?.metrics || []
+ }
+ datasource={datasetDetails}
+ onChange={(
+ filters: AdhocFilterClass[],
+ ) => {
setNativeFilterFieldValues(
form,
filterId,
{
- dependencies,
+ adhoc_filters: filters,
},
);
forceUpdate();
- validateDependencies();
formChanged();
- }}
- getDependencySuggestion={() =>
- getDependencySuggestion(filterId)
- }
- >
- {hasTimeDependency
- ? timeColumn
- : undefined}
- </DependencyList>
- </StyledRowFormItem>
- )}
- {hasDataset && hasAdditionalFilters && (
- <FormItem
- name={['filters', filterId, 'preFilter']}
- >
- <CollapsibleControl
- initialValue={hasPreFilter}
- title={t('Pre-filter available values')}
- tooltip={t(`Add filter clauses to
control the filter's source query,
- though only in the context of the autocomplete i.e., these
conditions
- do not impact how the filter is applied to the dashboard.
This is useful
- when you want to improve the query's performance by only
scanning a subset
- of the underlying data or limit the available values
displayed in the filter.`)}
- onChange={checked => {
- formChanged();
- if (checked) {
validatePreFilter();
+ }}
+ label={
+ <span>
+ <StyledLabel>
+ {t('Pre-filter')}
+ </StyledLabel>
+ {!hasTimeRange && (
+ <StyledAsterisk />
+ )}
+ </span>
}
- }}
- >
- <StyledRowSubFormItem
+ />
+ </StyledRowSubFormItem>
+ {showTimeRangePicker && (
+ <StyledRowFormItem
expanded={expanded}
name={[
'filters',
filterId,
- 'adhoc_filters',
+ 'time_range',
]}
- css={{ width: INPUT_WIDTH }}
+ label={
+ <StyledLabel>
+ {t('Time range')}
+ </StyledLabel>
+ }
initialValue={
- filterToEdit?.adhoc_filters
+ filterToEdit?.time_range ||
+ t('No filter')
}
- required
+ required={!hasAdhoc}
rules={[
{
validator: preFilterValidator,
},
]}
>
- <AdhocFilterControl
- columns={
- datasetDetails?.columns?.filter(
- (c: ColumnMeta) => c.filterable,
- ) || []
- }
- savedMetrics={
- datasetDetails?.metrics || []
- }
- datasource={datasetDetails}
- onChange={(
- filters: AdhocFilterClass[],
- ) => {
+ <DateFilterComponent
+ name="time_range"
+ onChange={timeRange => {
setNativeFilterFieldValues(
form,
filterId,
{
- adhoc_filters: filters,
+ time_range: timeRange,
},
);
forceUpdate();
formChanged();
validatePreFilter();
}}
- label={
- <span>
- <StyledLabel>
- {t('Pre-filter')}
- </StyledLabel>
- {!hasTimeRange && (
- <StyledAsterisk />
- )}
- </span>
- }
/>
- </StyledRowSubFormItem>
- {showTimeRangePicker && (
- <StyledRowFormItem
- expanded={expanded}
- name={[
- 'filters',
- filterId,
- 'time_range',
- ]}
- label={
- <StyledLabel>
- {t('Time range')}
- </StyledLabel>
- }
- initialValue={
- filterToEdit?.time_range ||
- t('No filter')
- }
- required={!hasAdhoc}
- rules={[
- {
- validator: preFilterValidator,
- },
- ]}
- >
- <DateFilterComponent
- name="time_range"
- onChange={timeRange => {
- setNativeFilterFieldValues(
- form,
- filterId,
- {
- time_range: timeRange,
- },
- );
- forceUpdate();
- formChanged();
- validatePreFilter();
- }}
- />
- </StyledRowFormItem>
- )}
- {hasTimeRange && !hasTimeDependency
- ? timeColumn
- : undefined}
- </CollapsibleControl>
- </FormItem>
- )}
- {itemTypeField !== 'filter_range' ? (
+ </StyledRowFormItem>
+ )}
+ {hasTimeRange && !hasTimeDependency
+ ? timeColumn
+ : undefined}
+ </CollapsibleControl>
+ </FormItem>
+ )}
+ {itemTypeField === 'filter_timegrain' &&
+ hasDataset &&
+ datasetDetails?.time_grain_sqla &&
+ datasetDetails.time_grain_sqla.length > 0 && (
<FormItem
- name={['filters', filterId, 'sortFilter']}
- initialValue={hasSorting}
+ name={[
+ 'filters',
+ filterId,
+ 'preFilterTimegrain',
+ ]}
>
<CollapsibleControl
- initialValue={hasSorting}
- title={
- isChartCustomization
- ? t('Sort display control values')
- : t('Sort filter values')
- }
+ initialValue={hasTimeGrainPreFilter}
+ title={t('Pre-filter available values')}
+ tooltip={t(
+ 'Select which time grains are
available in the filter control. This is a UI allow list only and does not add
extra conditions to the underlying queries.',
+ )}
onChange={checked => {
- onSortChanged(checked || undefined);
+ if (!checked) {
+ setNativeFilterFieldValues(
+ form,
+ filterId,
+ { time_grains: undefined },
+ );
+ forceUpdate();
+ }
formChanged();
}}
>
- <StyledRowFormItem
- expanded={expanded}
+ <FormItem
name={[
'filters',
filterId,
- 'controlValues',
- 'sortAscending',
+ 'time_grains',
]}
- initialValue={sort}
- label={
- <StyledLabel>
- {t('Sort type')}
- </StyledLabel>
+ initialValue={
+ filterToEdit?.time_grains
}
+ {...getFiltersConfigModalTestId(
+ 'time-grain-allowlist',
+ )}
>
- <Radio.GroupWrapper
- options={[
- {
- value: true,
- label: t('Sort ascending'),
- },
- {
- value: false,
- label: t('Sort descending'),
- },
- ]}
- onChange={value => {
- onSortChanged(value.target.value);
+ <Select
+ mode="multiple"
+ ariaLabel={t('Time grain options')}
+ options={getTimeGrainOptions(
+ datasetDetails.time_grain_sqla,
+ )}
+ sortComparator={() => 0}
+ onChange={(values: string[]) => {
+ setNativeFilterFieldValues(
+ form,
+ filterId,
+ {
+ time_grains:
+ values.length > 0 &&
+ values.length <
datasetDetails.time_grain_sqla.length
+ ? values
+ : undefined,
+ },
+ );
+ forceUpdate();
formChanged();
}}
+ css={{ width: INPUT_WIDTH }}
/>
- </StyledRowFormItem>
- {hasMetrics && (
- <StyledRowSubFormItem
- expanded={expanded}
- name={[
- 'filters',
- filterId,
- 'controlValues',
- 'sortMetric',
- ]}
- initialValue={
- filterToEdit?.controlValues
- ?.sortMetric ??
- customizationToEdit?.controlValues
- ?.sortMetric
- }
- label={
- <>
- <StyledLabel>
- {t('Sort Metric')}
- </StyledLabel>
-
- <InfoTooltip
- placement="top"
- tooltip={t(
- 'If a metric is specified,
sorting will be done based on the metric value',
- )}
- />
- </>
- }
- data-test="field-input"
- >
- <Select
- allowClear
- ariaLabel={t('Sort metric')}
- name="sortMetric"
- options={metrics.map(
- (metric: Metric) => ({
- value: metric.metric_name,
- label:
- metric.verbose_name ??
- metric.metric_name,
- }),
- )}
- onChange={value => {
- if (value !== undefined) {
- const previous =
- form.getFieldValue(
- 'filters',
- )?.[filterId].controlValues
||
- {};
- setNativeFilterFieldValues(
- form,
- filterId,
- {
- controlValues: {
- ...previous,
- sortMetric: value,
- },
- },
- );
- forceUpdate();
- }
- formChanged();
- }}
- />
- </StyledRowSubFormItem>
- )}
+ </FormItem>
</CollapsibleControl>
</FormItem>
- ) : (
- <>
- <FormItem
+ )}
+ {itemTypeField !== 'filter_range' ? (
+ <FormItem
+ name={['filters', filterId, 'sortFilter']}
+ initialValue={hasSorting}
+ >
+ <CollapsibleControl
+ initialValue={hasSorting}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Sort toggle state mismatch</b></div>
<div id="fix">
The initial state of the sort toggle (CollapsibleControl) does not reflect
whether sorting is enabled for existing filters that have sortMetric set but no
sortAscending. This could confuse users as the toggle appears off while sorting
is active. Consider updating hasSorting to check for sortMetric as well.
</div>
</div>
<small><i>Code Review Run #0a077c</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/FiltersConfigModal/transformers/timeGrains.test.ts:
##########
@@ -0,0 +1,86 @@
+/**
+ * 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 { NativeFilterType, NativeFilterScope } from '@superset-ui/core';
+import { getInitialDataMask } from 'src/dataMask/reducer';
+
+const mockScope: NativeFilterScope = {
+ rootPath: ['ROOT_ID'],
+ excluded: [],
+};
+
+const mockDataMask = getInitialDataMask();
+
+test('time_grains field is omitted when all grains are selected', () => {
+ // When all time grains are selected/allowed, time_grains should be undefined
+ const filterOutput = {
+ id: 'NATIVE_FILTER-abc123',
+ type: NativeFilterType.NativeFilter,
+ name: 'Time Grain Filter',
+ filterType: 'filter_timegrain',
+ description: '',
+ targets: [],
+ scope: mockScope,
+ controlValues: {},
+ defaultDataMask: mockDataMask,
+ cascadeParentIds: [],
+ adhoc_filters: undefined,
+ time_range: undefined,
+ granularity_sqla: undefined,
+ time_grains: undefined, // omitted when all selected
+ sortMetric: null,
+ };
+
+ expect(filterOutput.time_grains).toBeUndefined();
+});
+
+test('time_grains field is omitted when empty array is reset', () => {
+ // When user clears all selections (empty array), it should be treated as
undefined
+ const emptySelection = [];
+ const shouldOmit = emptySelection.length === 0;
+
+ expect(shouldOmit).toBe(true);
+});
+
+test('time_grains allows partial selection of grains', () => {
+ const allGrains = ['PT1M', 'PT1H', 'P1D', 'P1W', 'P1M', 'P0.25Y', 'P1Y'];
+ const selectedGrains = ['P1D', 'P1W', 'P1M'];
+
+ expect(selectedGrains).toEqual(expect.arrayContaining(['P1D', 'P1W',
'P1M']));
+ expect(selectedGrains.length).toBe(3);
+ expect(selectedGrains.length).toBeLessThan(allGrains.length);
+});
+
+test('backward compatibility: missing time_grains defaults to show all', () =>
{
+ // Existing filter configs without time_grains key should show all options
+ const legacyFilter = {
+ id: 'NATIVE_FILTER-legacy',
+ type: NativeFilterType.NativeFilter,
+ name: 'Legacy Time Grain Filter',
+ filterType: 'filter_timegrain',
+ targets: [],
+ // Note: no time_grains field
+ };
+
+ // When time_grains is undefined, runtime should show all options
+ const runtime_allowlist =
+ legacyFilter['time_grains' as keyof typeof legacyFilter];
+ const shouldShowAll = !runtime_allowlist || runtime_allowlist.length === 0;
+
+ expect(shouldShowAll).toBe(true);
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Tests don't verify actual behavior logic</b></div>
<div id="fix">
These tests assert on hardcoded mock objects and trivial conditions instead
of testing the actual time_grains transformation logic in transformFormInput.
For example, the first test creates a literal object with time_grains:
undefined and checks it's undefined, but doesn't verify the transformer's
behavior with different inputs. This won't catch bugs if the transformer logic
changes. Consider testing the transformFormInput function directly with various
time_grains values to ensure it correctly omits the field when empty or shows
all when undefined.
</div>
</div>
<small><i>Code Review Run #0a077c</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/filters/components/TimeGrain/TimeGrainFilterPlugin.test.tsx:
##########
@@ -0,0 +1,145 @@
+/**
+ * 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 { screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import { render } from 'spec/helpers/testing-library';
+import PluginFilterTimegrain from './TimeGrainFilterPlugin';
+import { PluginFilterTimeGrainProps } from './types';
+
+const mockSetDataMask = jest.fn();
+const mockSetFilterActive = jest.fn();
+const mockSetHoveredFilter = jest.fn();
+const mockUnsetHoveredFilter = jest.fn();
+const mockSetFocusedFilter = jest.fn();
+const mockUnsetFocusedFilter = jest.fn();
+
+const defaultProps: PluginFilterTimeGrainProps = {
+ data: [
+ { duration: 'P1D', name: 'Day' },
+ { duration: 'P1W', name: 'Week' },
+ { duration: 'P1M', name: 'Month' },
+ { duration: 'P1Y', name: 'Year' },
+ ],
+ formData: {
+ datasource: '3__table',
+ viz_type: 'filter_timegrain',
+ groupby: [],
+ adhoc_filters: [],
+ extra_filters: [],
+ extra_form_data: {},
+ granularity_sqla: 'ds',
+ time_range_endpoints: ['inclusive', 'exclusive'],
+ url_params: {},
+ height: 300,
+ width: 300,
+ nativeFilterId: 'filter-1',
+ defaultValue: null,
+ inputRef: { current: null },
+ },
+ filterState: {
+ value: null,
+ validateStatus: undefined,
+ validateMessage: undefined,
+ },
+ height: 300,
+ width: 300,
+ setDataMask: mockSetDataMask,
+ setFilterActive: mockSetFilterActive,
+ setHoveredFilter: mockSetHoveredFilter,
+ unsetHoveredFilter: mockUnsetHoveredFilter,
+ setFocusedFilter: mockSetFocusedFilter,
+ unsetFocusedFilter: mockUnsetFocusedFilter,
+ inputRef: { current: null },
+};
+
+test('renders all options when time_grains is not set', async () => {
+ render(<PluginFilterTimegrain {...defaultProps} />);
+
+ // Verify the select component is rendered
+ const select = screen.getByRole('combobox');
+ expect(select).toBeInTheDocument();
+
+ // Open the dropdown and verify all options are available
+ await userEvent.click(select);
+ const options = await screen.findAllByRole('option');
+ expect(options.length).toBe(4);
+ expect(options[0]).toHaveTextContent('Day');
+ expect(options[1]).toHaveTextContent('Week');
+ expect(options[2]).toHaveTextContent('Month');
+ expect(options[3]).toHaveTextContent('Year');
+});
+
+test('filters options based on time_grains allowlist', async () => {
+ const propsWithAllowlist = {
+ ...defaultProps,
+ formData: {
+ ...defaultProps.formData,
+ time_grains: ['P1D', 'P1W'],
+ },
+ };
+
+ render(<PluginFilterTimegrain {...propsWithAllowlist} />);
+
+ const select = screen.getByRole('combobox');
+ await userEvent.click(select);
+
+ // Only Day and Week should be available
+ const options = await screen.findAllByRole('option');
+ expect(options.length).toBe(2);
+ expect(options[0]).toHaveTextContent('Day');
+ expect(options[1]).toHaveTextContent('Week');
+});
+
+test('shows all options when time_grains is empty array', async () => {
+ const propsWithEmptyAllowlist = {
+ ...defaultProps,
+ formData: {
+ ...defaultProps.formData,
+ time_grains: [],
+ },
+ };
+
+ render(<PluginFilterTimegrain {...propsWithEmptyAllowlist} />);
+
+ const select = screen.getByRole('combobox');
+ await userEvent.click(select);
+
+ // All 4 options should be available
+ const options = await screen.findAllByRole('option');
+ expect(options.length).toBe(4);
+});
+
+test('shows all options when time_grains is undefined', async () => {
+ const propsWithUndefined = {
+ ...defaultProps,
+ formData: {
+ ...defaultProps.formData,
+ time_grains: undefined,
+ },
+ };
+
+ render(<PluginFilterTimegrain {...propsWithUndefined} />);
+
+ const select = screen.getByRole('combobox');
+ await userEvent.click(select);
+
+ // All 4 options should be available
+ const options = await screen.findAllByRole('option');
+ expect(options.length).toBe(4);
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete Test Assertions</b></div>
<div id="fix">
Tests 3 and 4 check only the count of options but do not assert the specific
option texts, which could allow bugs in the filtering logic to pass undetected.
Add assertions like `expect(options[0]).toHaveTextContent('Day')` for each
expected option to ensure the behavior is correctly validated.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
@@ -123,4 +123,8 @@
// All 4 options should be available
const options = await screen.findAllByRole('option');
expect(options.length).toBe(4);
+ expect(options[0]).toHaveTextContent('Day');
+ expect(options[1]).toHaveTextContent('Week');
+ expect(options[2]).toHaveTextContent('Month');
+ expect(options[3]).toHaveTextContent('Year');
});
test('shows all options when time_grains is undefined', async () => {
@@ -141,4 +145,8 @@
// All 4 options should be available
const options = await screen.findAllByRole('option');
expect(options.length).toBe(4);
+ expect(options[0]).toHaveTextContent('Day');
+ expect(options[1]).toHaveTextContent('Week');
+ expect(options[2]).toHaveTextContent('Month');
+ expect(options[3]).toHaveTextContent('Year');
});
```
</div>
</details>
</div>
<small><i>Code Review Run #0a077c</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/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1041,425 +1072,492 @@ const FiltersConfigForm = (
items={[
...(itemTypeField !== 'filter_time'
? [
- {
- key:
`${filterId}-${FilterPanels.configuration.key}`,
- forceRender: true,
- label: isChartCustomization
- ? CustomizationPanels.configuration.name
- : FilterPanels.configuration.name,
- children: (
- <>
- {canDependOnOtherFilters &&
- (hasAvailableFilters ||
- dependencies.length > 0) && (
- <StyledRowFormItem
+ {
+ key: `${filterId}-${FilterPanels.configuration.key}`,
+ forceRender: true,
+ label: isChartCustomization
+ ? CustomizationPanels.configuration.name
+ : FilterPanels.configuration.name,
+ children: (
+ <>
+ {canDependOnOtherFilters &&
+ (hasAvailableFilters ||
+ dependencies.length > 0) && (
+ <StyledRowFormItem
+ expanded={expanded}
+ name={[
+ 'filters',
+ filterId,
+ 'dependencies',
+ ]}
+ initialValue={dependencies}
+ >
+ <DependencyList
+ availableFilters={availableFilters}
+ dependencies={dependencies}
+ onDependenciesChange={dependencies => {
+ setNativeFilterFieldValues(
+ form,
+ filterId,
+ {
+ dependencies,
+ },
+ );
+ forceUpdate();
+ validateDependencies();
+ formChanged();
+ }}
+ getDependencySuggestion={() =>
+ getDependencySuggestion(filterId)
+ }
+ >
+ {hasTimeDependency
+ ? timeColumn
+ : undefined}
+ </DependencyList>
+ </StyledRowFormItem>
+ )}
+ {hasDataset && hasAdditionalFilters && (
+ <FormItem
+ name={['filters', filterId, 'preFilter']}
+ >
+ <CollapsibleControl
+ initialValue={hasPreFilter}
+ title={t('Pre-filter available values')}
+ tooltip={t(`Add filter clauses to control
the filter's source query,
+ though only in the context of the autocomplete i.e., these
conditions
+ do not impact how the filter is applied to the dashboard.
This is useful
+ when you want to improve the query's performance by only
scanning a subset
+ of the underlying data or limit the available values
displayed in the filter.`)}
+ onChange={checked => {
+ formChanged();
+ if (checked) {
+ validatePreFilter();
+ }
+ }}
+ >
+ <StyledRowSubFormItem
expanded={expanded}
name={[
'filters',
filterId,
- 'dependencies',
+ 'adhoc_filters',
+ ]}
+ css={{ width: INPUT_WIDTH }}
+ initialValue={
+ filterToEdit?.adhoc_filters
+ }
+ required
+ rules={[
+ {
+ validator: preFilterValidator,
+ },
]}
- initialValue={dependencies}
>
- <DependencyList
- availableFilters={availableFilters}
- dependencies={dependencies}
- onDependenciesChange={dependencies => {
+ <AdhocFilterControl
+ columns={
+ datasetDetails?.columns?.filter(
+ (c: ColumnMeta) => c.filterable,
+ ) || []
+ }
+ savedMetrics={
+ datasetDetails?.metrics || []
+ }
+ datasource={datasetDetails}
+ onChange={(
+ filters: AdhocFilterClass[],
+ ) => {
setNativeFilterFieldValues(
form,
filterId,
{
- dependencies,
+ adhoc_filters: filters,
},
);
forceUpdate();
- validateDependencies();
formChanged();
- }}
- getDependencySuggestion={() =>
- getDependencySuggestion(filterId)
- }
- >
- {hasTimeDependency
- ? timeColumn
- : undefined}
- </DependencyList>
- </StyledRowFormItem>
- )}
- {hasDataset && hasAdditionalFilters && (
- <FormItem
- name={['filters', filterId, 'preFilter']}
- >
- <CollapsibleControl
- initialValue={hasPreFilter}
- title={t('Pre-filter available values')}
- tooltip={t(`Add filter clauses to
control the filter's source query,
- though only in the context of the autocomplete i.e., these
conditions
- do not impact how the filter is applied to the dashboard.
This is useful
- when you want to improve the query's performance by only
scanning a subset
- of the underlying data or limit the available values
displayed in the filter.`)}
- onChange={checked => {
- formChanged();
- if (checked) {
validatePreFilter();
+ }}
+ label={
+ <span>
+ <StyledLabel>
+ {t('Pre-filter')}
+ </StyledLabel>
+ {!hasTimeRange && (
+ <StyledAsterisk />
+ )}
+ </span>
}
- }}
- >
- <StyledRowSubFormItem
+ />
+ </StyledRowSubFormItem>
+ {showTimeRangePicker && (
+ <StyledRowFormItem
expanded={expanded}
name={[
'filters',
filterId,
- 'adhoc_filters',
+ 'time_range',
]}
- css={{ width: INPUT_WIDTH }}
+ label={
+ <StyledLabel>
+ {t('Time range')}
+ </StyledLabel>
+ }
initialValue={
- filterToEdit?.adhoc_filters
+ filterToEdit?.time_range ||
+ t('No filter')
}
- required
+ required={!hasAdhoc}
rules={[
{
validator: preFilterValidator,
},
]}
>
- <AdhocFilterControl
- columns={
- datasetDetails?.columns?.filter(
- (c: ColumnMeta) => c.filterable,
- ) || []
- }
- savedMetrics={
- datasetDetails?.metrics || []
- }
- datasource={datasetDetails}
- onChange={(
- filters: AdhocFilterClass[],
- ) => {
+ <DateFilterComponent
+ name="time_range"
+ onChange={timeRange => {
setNativeFilterFieldValues(
form,
filterId,
{
- adhoc_filters: filters,
+ time_range: timeRange,
},
);
forceUpdate();
formChanged();
validatePreFilter();
}}
- label={
- <span>
- <StyledLabel>
- {t('Pre-filter')}
- </StyledLabel>
- {!hasTimeRange && (
- <StyledAsterisk />
- )}
- </span>
- }
/>
- </StyledRowSubFormItem>
- {showTimeRangePicker && (
- <StyledRowFormItem
- expanded={expanded}
- name={[
- 'filters',
- filterId,
- 'time_range',
- ]}
- label={
- <StyledLabel>
- {t('Time range')}
- </StyledLabel>
- }
- initialValue={
- filterToEdit?.time_range ||
- t('No filter')
- }
- required={!hasAdhoc}
- rules={[
- {
- validator: preFilterValidator,
- },
- ]}
- >
- <DateFilterComponent
- name="time_range"
- onChange={timeRange => {
- setNativeFilterFieldValues(
- form,
- filterId,
- {
- time_range: timeRange,
- },
- );
- forceUpdate();
- formChanged();
- validatePreFilter();
- }}
- />
- </StyledRowFormItem>
- )}
- {hasTimeRange && !hasTimeDependency
- ? timeColumn
- : undefined}
- </CollapsibleControl>
- </FormItem>
- )}
- {itemTypeField !== 'filter_range' ? (
+ </StyledRowFormItem>
+ )}
+ {hasTimeRange && !hasTimeDependency
+ ? timeColumn
+ : undefined}
+ </CollapsibleControl>
+ </FormItem>
+ )}
+ {itemTypeField === 'filter_timegrain' &&
+ hasDataset &&
+ datasetDetails?.time_grain_sqla &&
+ datasetDetails.time_grain_sqla.length > 0 && (
<FormItem
- name={['filters', filterId, 'sortFilter']}
- initialValue={hasSorting}
+ name={[
+ 'filters',
+ filterId,
+ 'preFilterTimegrain',
+ ]}
>
<CollapsibleControl
- initialValue={hasSorting}
- title={
- isChartCustomization
- ? t('Sort display control values')
- : t('Sort filter values')
- }
+ initialValue={hasTimeGrainPreFilter}
+ title={t('Pre-filter available values')}
+ tooltip={t(
+ 'Select which time grains are
available in the filter control. This is a UI allow list only and does not add
extra conditions to the underlying queries.',
+ )}
onChange={checked => {
- onSortChanged(checked || undefined);
+ if (!checked) {
+ setNativeFilterFieldValues(
+ form,
+ filterId,
+ { time_grains: undefined },
+ );
+ forceUpdate();
+ }
formChanged();
}}
>
- <StyledRowFormItem
- expanded={expanded}
+ <FormItem
name={[
'filters',
filterId,
- 'controlValues',
- 'sortAscending',
+ 'time_grains',
]}
- initialValue={sort}
- label={
- <StyledLabel>
- {t('Sort type')}
- </StyledLabel>
+ initialValue={
+ filterToEdit?.time_grains
}
+ {...getFiltersConfigModalTestId(
+ 'time-grain-allowlist',
+ )}
>
- <Radio.GroupWrapper
- options={[
- {
- value: true,
- label: t('Sort ascending'),
- },
- {
- value: false,
- label: t('Sort descending'),
- },
- ]}
- onChange={value => {
- onSortChanged(value.target.value);
+ <Select
+ mode="multiple"
+ ariaLabel={t('Time grain options')}
+ options={getTimeGrainOptions(
+ datasetDetails.time_grain_sqla,
+ )}
+ sortComparator={() => 0}
+ onChange={(values: string[]) => {
+ setNativeFilterFieldValues(
+ form,
+ filterId,
+ {
+ time_grains:
+ values.length > 0 &&
+ values.length <
datasetDetails.time_grain_sqla.length
+ ? values
+ : undefined,
+ },
+ );
+ forceUpdate();
formChanged();
}}
+ css={{ width: INPUT_WIDTH }}
/>
- </StyledRowFormItem>
- {hasMetrics && (
- <StyledRowSubFormItem
- expanded={expanded}
- name={[
- 'filters',
- filterId,
- 'controlValues',
- 'sortMetric',
- ]}
- initialValue={
- filterToEdit?.controlValues
- ?.sortMetric ??
- customizationToEdit?.controlValues
- ?.sortMetric
- }
- label={
- <>
- <StyledLabel>
- {t('Sort Metric')}
- </StyledLabel>
-
- <InfoTooltip
- placement="top"
- tooltip={t(
- 'If a metric is specified,
sorting will be done based on the metric value',
- )}
- />
- </>
- }
- data-test="field-input"
- >
- <Select
- allowClear
- ariaLabel={t('Sort metric')}
- name="sortMetric"
- options={metrics.map(
- (metric: Metric) => ({
- value: metric.metric_name,
- label:
- metric.verbose_name ??
- metric.metric_name,
- }),
- )}
- onChange={value => {
- if (value !== undefined) {
- const previous =
- form.getFieldValue(
- 'filters',
- )?.[filterId].controlValues
||
- {};
- setNativeFilterFieldValues(
- form,
- filterId,
- {
- controlValues: {
- ...previous,
- sortMetric: value,
- },
- },
- );
- forceUpdate();
- }
- formChanged();
- }}
- />
- </StyledRowSubFormItem>
- )}
+ </FormItem>
</CollapsibleControl>
</FormItem>
- ) : (
- <>
- <FormItem
+ )}
+ {itemTypeField !== 'filter_range' ? (
+ <FormItem
+ name={['filters', filterId, 'sortFilter']}
+ initialValue={hasSorting}
+ >
+ <CollapsibleControl
+ initialValue={hasSorting}
+ title={
+ isChartCustomization
+ ? t('Sort display control values')
+ : t('Sort filter values')
+ }
+ onChange={checked => {
+ onSortChanged(checked || undefined);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete sort disable</b></div>
<div id="fix">
When disabling the sort toggle, only sortAscending is cleared, but
sortMetric remains set, so sorting continues. To properly disable sorting,
sortMetric should also be cleared when the toggle is turned off.
</div>
</div>
<small><i>Code Review Run #0a077c</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/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1041,425 +1072,492 @@ const FiltersConfigForm = (
items={[
...(itemTypeField !== 'filter_time'
? [
- {
- key:
`${filterId}-${FilterPanels.configuration.key}`,
- forceRender: true,
- label: isChartCustomization
- ? CustomizationPanels.configuration.name
- : FilterPanels.configuration.name,
- children: (
- <>
- {canDependOnOtherFilters &&
- (hasAvailableFilters ||
- dependencies.length > 0) && (
- <StyledRowFormItem
+ {
+ key: `${filterId}-${FilterPanels.configuration.key}`,
+ forceRender: true,
+ label: isChartCustomization
+ ? CustomizationPanels.configuration.name
+ : FilterPanels.configuration.name,
+ children: (
+ <>
+ {canDependOnOtherFilters &&
+ (hasAvailableFilters ||
+ dependencies.length > 0) && (
+ <StyledRowFormItem
+ expanded={expanded}
+ name={[
+ 'filters',
+ filterId,
+ 'dependencies',
+ ]}
+ initialValue={dependencies}
+ >
+ <DependencyList
+ availableFilters={availableFilters}
+ dependencies={dependencies}
+ onDependenciesChange={dependencies => {
+ setNativeFilterFieldValues(
+ form,
+ filterId,
+ {
+ dependencies,
+ },
+ );
+ forceUpdate();
+ validateDependencies();
+ formChanged();
+ }}
+ getDependencySuggestion={() =>
+ getDependencySuggestion(filterId)
+ }
+ >
+ {hasTimeDependency
+ ? timeColumn
+ : undefined}
+ </DependencyList>
+ </StyledRowFormItem>
+ )}
+ {hasDataset && hasAdditionalFilters && (
+ <FormItem
+ name={['filters', filterId, 'preFilter']}
+ >
+ <CollapsibleControl
+ initialValue={hasPreFilter}
+ title={t('Pre-filter available values')}
+ tooltip={t(`Add filter clauses to control
the filter's source query,
+ though only in the context of the autocomplete i.e., these
conditions
+ do not impact how the filter is applied to the dashboard.
This is useful
+ when you want to improve the query's performance by only
scanning a subset
+ of the underlying data or limit the available values
displayed in the filter.`)}
+ onChange={checked => {
+ formChanged();
+ if (checked) {
+ validatePreFilter();
+ }
+ }}
+ >
+ <StyledRowSubFormItem
expanded={expanded}
name={[
'filters',
filterId,
- 'dependencies',
+ 'adhoc_filters',
+ ]}
+ css={{ width: INPUT_WIDTH }}
+ initialValue={
+ filterToEdit?.adhoc_filters
+ }
+ required
+ rules={[
+ {
+ validator: preFilterValidator,
+ },
]}
- initialValue={dependencies}
>
- <DependencyList
- availableFilters={availableFilters}
- dependencies={dependencies}
- onDependenciesChange={dependencies => {
+ <AdhocFilterControl
+ columns={
+ datasetDetails?.columns?.filter(
+ (c: ColumnMeta) => c.filterable,
+ ) || []
+ }
+ savedMetrics={
+ datasetDetails?.metrics || []
+ }
+ datasource={datasetDetails}
+ onChange={(
+ filters: AdhocFilterClass[],
+ ) => {
setNativeFilterFieldValues(
form,
filterId,
{
- dependencies,
+ adhoc_filters: filters,
},
);
forceUpdate();
- validateDependencies();
formChanged();
- }}
- getDependencySuggestion={() =>
- getDependencySuggestion(filterId)
- }
- >
- {hasTimeDependency
- ? timeColumn
- : undefined}
- </DependencyList>
- </StyledRowFormItem>
- )}
- {hasDataset && hasAdditionalFilters && (
- <FormItem
- name={['filters', filterId, 'preFilter']}
- >
- <CollapsibleControl
- initialValue={hasPreFilter}
- title={t('Pre-filter available values')}
- tooltip={t(`Add filter clauses to
control the filter's source query,
- though only in the context of the autocomplete i.e., these
conditions
- do not impact how the filter is applied to the dashboard.
This is useful
- when you want to improve the query's performance by only
scanning a subset
- of the underlying data or limit the available values
displayed in the filter.`)}
- onChange={checked => {
- formChanged();
- if (checked) {
validatePreFilter();
+ }}
+ label={
+ <span>
+ <StyledLabel>
+ {t('Pre-filter')}
+ </StyledLabel>
+ {!hasTimeRange && (
+ <StyledAsterisk />
+ )}
+ </span>
}
- }}
- >
- <StyledRowSubFormItem
+ />
+ </StyledRowSubFormItem>
+ {showTimeRangePicker && (
+ <StyledRowFormItem
expanded={expanded}
name={[
'filters',
filterId,
- 'adhoc_filters',
+ 'time_range',
]}
- css={{ width: INPUT_WIDTH }}
+ label={
+ <StyledLabel>
+ {t('Time range')}
+ </StyledLabel>
+ }
initialValue={
- filterToEdit?.adhoc_filters
+ filterToEdit?.time_range ||
+ t('No filter')
}
- required
+ required={!hasAdhoc}
rules={[
{
validator: preFilterValidator,
},
]}
>
- <AdhocFilterControl
- columns={
- datasetDetails?.columns?.filter(
- (c: ColumnMeta) => c.filterable,
- ) || []
- }
- savedMetrics={
- datasetDetails?.metrics || []
- }
- datasource={datasetDetails}
- onChange={(
- filters: AdhocFilterClass[],
- ) => {
+ <DateFilterComponent
+ name="time_range"
+ onChange={timeRange => {
setNativeFilterFieldValues(
form,
filterId,
{
- adhoc_filters: filters,
+ time_range: timeRange,
},
);
forceUpdate();
formChanged();
validatePreFilter();
}}
- label={
- <span>
- <StyledLabel>
- {t('Pre-filter')}
- </StyledLabel>
- {!hasTimeRange && (
- <StyledAsterisk />
- )}
- </span>
- }
/>
- </StyledRowSubFormItem>
- {showTimeRangePicker && (
- <StyledRowFormItem
- expanded={expanded}
- name={[
- 'filters',
- filterId,
- 'time_range',
- ]}
- label={
- <StyledLabel>
- {t('Time range')}
- </StyledLabel>
- }
- initialValue={
- filterToEdit?.time_range ||
- t('No filter')
- }
- required={!hasAdhoc}
- rules={[
- {
- validator: preFilterValidator,
- },
- ]}
- >
- <DateFilterComponent
- name="time_range"
- onChange={timeRange => {
- setNativeFilterFieldValues(
- form,
- filterId,
- {
- time_range: timeRange,
- },
- );
- forceUpdate();
- formChanged();
- validatePreFilter();
- }}
- />
- </StyledRowFormItem>
- )}
- {hasTimeRange && !hasTimeDependency
- ? timeColumn
- : undefined}
- </CollapsibleControl>
- </FormItem>
- )}
- {itemTypeField !== 'filter_range' ? (
+ </StyledRowFormItem>
+ )}
+ {hasTimeRange && !hasTimeDependency
+ ? timeColumn
+ : undefined}
+ </CollapsibleControl>
+ </FormItem>
+ )}
+ {itemTypeField === 'filter_timegrain' &&
+ hasDataset &&
+ datasetDetails?.time_grain_sqla &&
+ datasetDetails.time_grain_sqla.length > 0 && (
<FormItem
- name={['filters', filterId, 'sortFilter']}
- initialValue={hasSorting}
+ name={[
+ 'filters',
+ filterId,
+ 'preFilterTimegrain',
+ ]}
>
<CollapsibleControl
- initialValue={hasSorting}
- title={
- isChartCustomization
- ? t('Sort display control values')
- : t('Sort filter values')
- }
+ initialValue={hasTimeGrainPreFilter}
+ title={t('Pre-filter available values')}
+ tooltip={t(
+ 'Select which time grains are
available in the filter control. This is a UI allow list only and does not add
extra conditions to the underlying queries.',
+ )}
onChange={checked => {
- onSortChanged(checked || undefined);
+ if (!checked) {
+ setNativeFilterFieldValues(
+ form,
+ filterId,
+ { time_grains: undefined },
+ );
+ forceUpdate();
+ }
formChanged();
}}
>
- <StyledRowFormItem
- expanded={expanded}
+ <FormItem
name={[
'filters',
filterId,
- 'controlValues',
- 'sortAscending',
+ 'time_grains',
]}
- initialValue={sort}
- label={
- <StyledLabel>
- {t('Sort type')}
- </StyledLabel>
+ initialValue={
+ filterToEdit?.time_grains
}
+ {...getFiltersConfigModalTestId(
+ 'time-grain-allowlist',
+ )}
>
- <Radio.GroupWrapper
- options={[
- {
- value: true,
- label: t('Sort ascending'),
- },
- {
- value: false,
- label: t('Sort descending'),
- },
- ]}
- onChange={value => {
- onSortChanged(value.target.value);
+ <Select
+ mode="multiple"
+ ariaLabel={t('Time grain options')}
+ options={getTimeGrainOptions(
+ datasetDetails.time_grain_sqla,
+ )}
+ sortComparator={() => 0}
+ onChange={(values: string[]) => {
+ setNativeFilterFieldValues(
+ form,
+ filterId,
+ {
+ time_grains:
+ values.length > 0 &&
+ values.length <
datasetDetails.time_grain_sqla.length
+ ? values
+ : undefined,
+ },
+ );
+ forceUpdate();
formChanged();
}}
+ css={{ width: INPUT_WIDTH }}
/>
- </StyledRowFormItem>
- {hasMetrics && (
- <StyledRowSubFormItem
- expanded={expanded}
- name={[
- 'filters',
- filterId,
- 'controlValues',
- 'sortMetric',
- ]}
- initialValue={
- filterToEdit?.controlValues
- ?.sortMetric ??
- customizationToEdit?.controlValues
- ?.sortMetric
- }
- label={
- <>
- <StyledLabel>
- {t('Sort Metric')}
- </StyledLabel>
-
- <InfoTooltip
- placement="top"
- tooltip={t(
- 'If a metric is specified,
sorting will be done based on the metric value',
- )}
- />
- </>
- }
- data-test="field-input"
- >
- <Select
- allowClear
- ariaLabel={t('Sort metric')}
- name="sortMetric"
- options={metrics.map(
- (metric: Metric) => ({
- value: metric.metric_name,
- label:
- metric.verbose_name ??
- metric.metric_name,
- }),
- )}
- onChange={value => {
- if (value !== undefined) {
- const previous =
- form.getFieldValue(
- 'filters',
- )?.[filterId].controlValues
||
- {};
- setNativeFilterFieldValues(
- form,
- filterId,
- {
- controlValues: {
- ...previous,
- sortMetric: value,
- },
- },
- );
- forceUpdate();
- }
- formChanged();
- }}
- />
- </StyledRowSubFormItem>
- )}
+ </FormItem>
</CollapsibleControl>
</FormItem>
- ) : (
- <>
- <FormItem
+ )}
+ {itemTypeField !== 'filter_range' ? (
+ <FormItem
+ name={['filters', filterId, 'sortFilter']}
+ initialValue={hasSorting}
+ >
+ <CollapsibleControl
+ initialValue={hasSorting}
+ title={
+ isChartCustomization
+ ? t('Sort display control values')
+ : t('Sort filter values')
+ }
+ onChange={checked => {
+ onSortChanged(checked || undefined);
+ formChanged();
+ }}
+ >
+ <StyledRowFormItem
+ expanded={expanded}
name={[
'filters',
filterId,
- 'rangeFilter',
+ 'controlValues',
+ 'sortAscending',
]}
+ initialValue={sort}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing default sort direction</b></div>
<div id="fix">
The sortAscending radio has initialValue={sort}, but if sort is undefined
(for new or migrated filters), no radio option is selected. Since sorting
defaults to ascending, it should default to true when undefined.
</div>
</div>
<small><i>Code Review Run #0a077c</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]
