bito-code-review[bot] commented on code in PR #44527:
URL: https://github.com/apache/superset/pull/44527#discussion_r4073232077
##########
superset/reports/models.py:
##########
@@ -259,6 +259,7 @@ def _generate_native_filter(
# Filter types that require at least one value
requires_values = (
"filter_time",
+ "filter_date_range",
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Null time_range on cleared filter</b></div>
<div id="fix">
`requires_values` guard checks `not values`, but a cleared RangePicker
yields `filterValues: [null]` (truthy), so `values[0]` (None) lands in
`extraFormData.time_range`/`filterState.value`. The frontend
`validNativeFilters` keeps `[null]` (length 1). Consider also skipping when
`values[0]` is None for `filter_time`/`filter_date_range`.
</div>
</div>
<small><i>Code Review Run #c6366a</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/features/alerts/AlertReportModal.tsx:
##########
@@ -1699,6 +1706,28 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
/>
);
}
+ if (filterType === 'filter_date_range') {
+ return (
+ <RangePicker
+ value={parseTimeRange(filterValues?.[0])}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Magic format string</b></div>
<div id="fix">
The literal `YYYY-MM-DD` duplicates the exported `DATE_FORMAT` constant in
the same module already imported for `parseTimeRange`/`formatTimeRange`. Using
the constant keeps the picker format and the stored range format in sync if the
format ever changes.
</div>
</div>
<small><i>Code Review Run #c6366a</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/DateRange/controlPanel.ts:
##########
@@ -0,0 +1,46 @@
+/**
+ * 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 { ControlPanelConfig } from '@superset-ui/chart-controls';
+import { t } from '@apache-superset/core/translation';
+
+const config: ControlPanelConfig = {
+ controlPanelSections: [
+ {
+ label: t('UI Configuration'),
+ expanded: true,
+ controlSetRows: [
+ [
+ {
+ name: 'enableEmptyFilter',
+ config: {
+ type: 'CheckboxControl',
+ label: () => t('Filter value is required'),
+ default: false,
+ renderTrigger: true,
+ description: () =>
+ t('User must select a value before applying the filter'),
+ },
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated control block</b></div>
<div id="fix">
The `enableEmptyFilter` control block (lines 30-38) is copied verbatim from
`Range/controlPanel.ts`, `Time/controlPanel.ts`, `TimeColumn`, `TimeGrain` and
`Select` control panels. This duplicated UI-config logic will drift (e.g.
label/description wording). Consider extracting a shared control definition.
</div>
</div>
<small><i>Code Review Run #c6366a</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/DateRange/DateRangeFilterPlugin.tsx:
##########
@@ -0,0 +1,88 @@
+/**
+ * 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 { useCallback, useMemo } from 'react';
+import { styled } from '@apache-superset/core/theme';
+import { RangePicker } from '@superset-ui/core/components';
+import { PluginFilterDateRangeProps } from './types';
+import { DATE_FORMAT, formatTimeRange, parseTimeRange } from './utils';
+import { FilterPluginStyle } from '../common';
+
+const ControlContainer = styled.div`
+ width: 100%;
+`;
+
+export default function DateRangeFilterPlugin(
+ props: PluginFilterDateRangeProps,
+) {
+ const {
+ setDataMask,
+ setHoveredFilter,
+ unsetHoveredFilter,
+ setFocusedFilter,
+ unsetFocusedFilter,
+ setFilterActive,
+ width,
+ height,
+ filterState,
+ inputRef,
+ } = props;
+
+ const value = useMemo(
+ () => parseTimeRange(filterState.value),
+ [filterState.value],
+ );
+
+ const handleChange = useCallback(
+ (dates: ReturnType<typeof parseTimeRange>) => {
+ const timeRange = formatTimeRange(dates);
+ setDataMask({
+ extraFormData: timeRange ? { time_range: timeRange } : {},
+ filterState: { value: timeRange },
+ });
+ },
+ [setDataMask],
+ );
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unused filter option</b></div>
<div id="fix">
`controlPanel.ts` exposes an `enableEmptyFilter` option ("Filter value is
required"), but this plugin never reads `props.formData.enableEmptyFilter`.
With `allowClear`, clearing the picker always emits `filterState: { value:
undefined }` and `extraFormData: {}`, so the option has no effect. Honor the
flag like `RangeFilterPlugin`/`SelectFilterPlugin` do.
</div>
</div>
<small><i>Code Review Run #c6366a</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/features/alerts/AlertReportModal.tsx:
##########
@@ -1699,6 +1706,28 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
/>
);
}
+ if (filterType === 'filter_date_range') {
+ return (
+ <RangePicker
+ value={parseTimeRange(filterValues?.[0])}
+ format="YYYY-MM-DD"
+ allowClear
+ onChange={dates => {
+ const timeRange = formatTimeRange(dates);
+ setNativeFilterData(
+ nativeFilterData.map((f: any) =>
+ filter.nativeFilterId === f.nativeFilterId
+ ? {
+ ...f,
+ filterValues: [timeRange],
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Cleared range persists null</b></div>
<div id="fix">
With `allowClear`, clearing the picker makes `dates` null, so
`formatTimeRange` returns `undefined` and `filterValues: [timeRange]` stores
`[undefined]`. Because `[undefined].length === 1`, the save check
`hasFilterValues` (line 946) and validation (line 1831) both treat it as
populated, persisting `[null]` for a cleared filter. Store `[]` when
`timeRange` is undefined.
</div>
</div>
<small><i>Code Review Run #c6366a</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/DateRange/types.ts:
##########
@@ -0,0 +1,47 @@
+/**
+ * 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 { RefObject } from 'react';
+import {
+ Behavior,
+ DataRecord,
+ FilterState,
+ QueryFormData,
+} from '@superset-ui/core';
+import { PluginFilterHooks, PluginFilterStylesProps } from '../types';
+
+interface PluginFilterDateRangeCustomizeProps {
+ defaultValue?: string | null;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead contract field</b></div>
<div id="fix">
`defaultValue` is declared in `PluginFilterDateRangeCustomizeProps` and
`DEFAULT_FORM_DATA`, but `DateRangeFilterPlugin` only reads `filterState.value`
and `controlPanel.ts` exposes no `defaultValue` control. A configured default
value is silently ignored. Either wire it up (use as initial value when
`filterState.value` is empty, like `RangeFilterPlugin`) or drop the field from
the contract.
</div>
</div>
<small><i>Code Review Run #c6366a</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]
