Copilot commented on code in PR #38568: URL: https://github.com/apache/superset/pull/38568#discussion_r2915395985
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -149,15 +150,15 @@ const controlsOrder: ControlKey[] = [
'inverseSelection',
];
-export const StyledFormItem = styled(FormItem)<{ expanded: boolean }>`
+export const StyledFormItem = styled(FormItem) <{ expanded: boolean }>`
width: ${({ expanded }) => (expanded ? '49%' : `${FORM_ITEM_WIDTH}px`)};
`;
-export const StyledRowFormItem = styled(FormItem)<{ expanded: boolean }>`
+export const StyledRowFormItem = styled(FormItem) <{ expanded: boolean }>`
min-width: ${({ expanded }) => (expanded ? '50%' : `${FORM_ITEM_WIDTH}px`)};
`;
Review Comment:
`styled(FormItem) <{ expanded: boolean }>` has an extra space before the
generic type argument, which will cause a TS parse error. Remove the space so
the styled-component generic is applied correctly.
##########
superset-frontend/src/filters/components/TimeGrain/TimeGrainFilterPlugin.test.tsx:
##########
@@ -0,0 +1,136 @@
+/**
+ * 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', () => {
+ render(<PluginFilterTimegrain {...defaultProps} />);
+
+ // Verify the select component is rendered
+ const select = screen.getByRole('combobox');
+ expect(select).toBeInTheDocument();
Review Comment:
The test name says it “renders all options when time_grains is not set”, but
it only asserts that the combobox exists. This will still pass even if the
option filtering is broken (e.g., options list is empty). Open the dropdown and
assert the expected option count/labels to validate the behavior.
```suggestion
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 = screen.getAllByRole('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');
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -149,15 +150,15 @@ const controlsOrder: ControlKey[] = [
'inverseSelection',
];
-export const StyledFormItem = styled(FormItem)<{ expanded: boolean }>`
+export const StyledFormItem = styled(FormItem) <{ expanded: boolean }>`
width: ${({ expanded }) => (expanded ? '49%' : `${FORM_ITEM_WIDTH}px`)};
`;
Review Comment:
`styled(FormItem) <{ expanded: boolean }>` introduces a space before the
generic type argument, which parses as a JSX/TS comparison and will break
TypeScript compilation. Remove the space so the generic is applied to
`styled(FormItem)`.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/transformers/timeGrains.test.ts:
##########
@@ -0,0 +1,103 @@
+/**
+ * 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';
+
+describe('Time Grains Filter Transformer', () => {
+ const mockScope: NativeFilterScope = {
+ rootPath: ['ROOT_ID'],
+ excluded: [],
+ };
+
+ const mockDataMask = getInitialDataMask();
+
+ test('time_grains field is persisted when subset is selected', () => {
+ // This tests the contract that time_grains are properly saved
+ // when a subset of grains are selected
+ const timeGrains = ['P1D', 'P1W'];
+ const expectedOutput = {
+ time_grains: timeGrains,
+ };
+
+ // Verify the structure matches what the transformer should produce
+ expect(expectedOutput.time_grains).toEqual(timeGrains);
+ expect(expectedOutput.time_grains?.length).toBe(2);
+ });
Review Comment:
This test file doesn’t exercise any production transformer logic (it only
asserts on locally-constructed objects), so it won’t fail if the actual
filter-save transformer stops persisting/omitting `time_grains`. Refactor these
tests to call the real transformer used on save (e.g., `transformFilterForSave`
/ `transformFormInput` path) and assert on its output for subset vs
all-selected vs undefined cases.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1036,425 +1041,494 @@ 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(`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.`)}
Review Comment:
The tooltip text for the Time Grain allowlist reuses the pre-filter
(adhoc/time_range) tooltip about adding filter clauses to the source
query/autocomplete. For `time_grains`, this is a UI allowlist and doesn’t add
query clauses, so the tooltip is misleading and should be updated to describe
the allowlist behavior.
```suggestion
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.',
)}
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -149,15 +150,15 @@ const controlsOrder: ControlKey[] = [
'inverseSelection',
];
-export const StyledFormItem = styled(FormItem)<{ expanded: boolean }>`
+export const StyledFormItem = styled(FormItem) <{ expanded: boolean }>`
width: ${({ expanded }) => (expanded ? '49%' : `${FORM_ITEM_WIDTH}px`)};
`;
-export const StyledRowFormItem = styled(FormItem)<{ expanded: boolean }>`
+export const StyledRowFormItem = styled(FormItem) <{ expanded: boolean }>`
min-width: ${({ expanded }) => (expanded ? '50%' : `${FORM_ITEM_WIDTH}px`)};
`;
-export const StyledRowSubFormItem = styled(FormItem)<{ expanded: boolean }>`
+export const StyledRowSubFormItem = styled(FormItem) <{ expanded: boolean }>`
min-width: ${({ expanded }) => (expanded ? '50%' : `${FORM_ITEM_WIDTH}px`)};
`;
Review Comment:
`styled(FormItem) <{ expanded: boolean }>` includes a space before the
generic, which will break parsing/compilation. Remove the space so the generic
parameter is attached to `styled(FormItem)`.
##########
superset-frontend/playwright/tests/experimental/dashboard/nativeFilters.timeGrain.test.ts:
##########
@@ -0,0 +1,24 @@
+/**
+ * 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 { test } from '@playwright/test';
+
+test('time grain native filter allowlist persists and reloads correctly',
async () => {
+ test.skip(true, 'Planning scaffold: implement once feature code exists.');
Review Comment:
This Playwright test is permanently skipped (`test.skip(true, ...)`) and
doesn’t provide coverage. Either remove it until it’s ready, or implement it
and gate execution with a feature flag/environment condition instead of a
hardcoded skip.
```suggestion
const isTimeGrainNativeFilterEnabled =
process.env.NATIVE_FILTER_TIME_GRAIN_FEATURE === '1';
test.skip(
!isTimeGrainNativeFilterEnabled,
'Planning scaffold: implement once feature code exists.',
);
// TODO: Implement this test once the time grain native filter feature
code exists.
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/transformers/timeGrains.test.ts:
##########
@@ -0,0 +1,103 @@
+/**
+ * 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';
+
+describe('Time Grains Filter Transformer', () => {
+ const mockScope: NativeFilterScope = {
+ rootPath: ['ROOT_ID'],
Review Comment:
Repository testing guidance prefers using `test()` over `describe()` blocks
to avoid nesting and keep tests flatter. Consider removing the outer
`describe()` and leaving these as top-level `test()` cases.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1036,425 +1041,494 @@ 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(`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 => {
- 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
+ : undefined,
Review Comment:
The save logic only omits `time_grains` when the selection is empty; if a
user selects *all* available grains, it will still persist the full array. That
breaks the stated backward-compat contract (“undefined means all allowed”) and
can unintentionally hide newly-added dataset time grains later. Consider
normalizing by comparing the selected values against
`datasetDetails.time_grain_sqla` and storing `undefined` when they match.
```suggestion
const availableTimeGrains =
datasetDetails.time_grain_sqla
??
[];
let timeGrainsValue:
| string[]
| undefined;
// Normalize: if no selection or
selection
// matches all available grains,
store
// `undefined` to mean "all
allowed".
if (
values.length === 0 ||
(availableTimeGrains.length >
0 &&
values.length ===
availableTimeGrains.length
&&
isEqual(
[...values].sort(),
[...availableTimeGrains].sort(),
))
) {
timeGrainsValue = undefined;
} else {
timeGrainsValue = values;
}
setNativeFilterFieldValues(
form,
filterId,
{
time_grains: timeGrainsValue,
```
--
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]
