fix: Prevents last temporal filter removal (apache#22982)

preset-io · Feb 3, 2023 · b63ea63 · b63ea63
1 parent 32d4977
commit b63ea63
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 69 deletions.
diff --git a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx
@@ -46,6 +46,7 @@ import {
   CustomControlItem,
   Dataset,
   ExpandedControlItem,
+  isTemporalColumn,
   sections,
 } from '@superset-ui/chart-controls';
 import { useSelector } from 'react-redux';
@@ -293,13 +294,17 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
   const previousXAxis = usePrevious(x_axis);
 
   useEffect(() => {
-    if (x_axis && x_axis !== previousXAxis) {
+    if (
+      x_axis &&
+      x_axis !== previousXAxis &&
+      isTemporalColumn(x_axis, props.exploreState.datasource)
+    ) {
       const noFilter =
         !adhoc_filters ||
         !adhoc_filters.find(
           filter =>
             filter.expressionType === 'SIMPLE' &&
-            filter.operator === 'TEMPORAL_RANGE' &&
+            filter.operator === Operators.TEMPORAL_RANGE &&
             filter.subject === x_axis,
         );
       if (noFilter) {
@@ -314,7 +319,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
               {
                 clause: 'WHERE',
                 subject: x_axis,
-                operator: 'TEMPORAL_RANGE',
+                operator: Operators.TEMPORAL_RANGE,
                 comparator: defaultTimeFilter || NO_TIME_RANGE,
                 expressionType: 'SIMPLE',
               },
@@ -329,6 +334,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
     setControlValue,
     defaultTimeFilter,
     previousXAxis,
+    props.exploreState.datasource,
   ]);
 
   useEffect(() => {
@@ -482,28 +488,21 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
         : baseDescription;
 
     if (name === 'adhoc_filters') {
-      restProps.confirmDeletion = {
-        triggerCondition: (
-          valueToBeDeleted: Record<string, any>,
-          values: Record<string, any>[],
-        ) => {
-          const isTemporalRange = (filter: Record<string, any>) =>
-            filter.operator === Operators.TEMPORAL_RANGE;
-          if (isTemporalRange(valueToBeDeleted)) {
-            const count = values.filter(isTemporalRange).length;
-            if (count < 2) {
-              return true;
-            }
+      restProps.canDelete = (
+        valueToBeDeleted: Record<string, any>,
+        values: Record<string, any>[],
+      ) => {
+        const isTemporalRange = (filter: Record<string, any>) =>
+          filter.operator === Operators.TEMPORAL_RANGE;
+        if (isTemporalRange(valueToBeDeleted)) {
+          const count = values.filter(isTemporalRange).length;
+          if (count === 1) {
+            return t(
+              `You cannot delete the last temporal filter as it's used for time range filters in dashboards.`,
+            );
           }
-          return false;
-        },
-        confirmationTitle: t(
-          'Are you sure you want to remove the last temporal filter?',
-        ),
-        confirmationText: t(
-          `This filter is the last temporal filter. If you proceed,
-          this chart won't be affected by time range filters in dashboards.`,
-        ),
+        }
+        return true;
       };
     }
 

diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx
@@ -59,7 +59,7 @@ import AdhocFilterControl from '../FilterControl/AdhocFilterControl';
 import DndAdhocFilterOption from './DndAdhocFilterOption';
 import { useDefaultTimeFilter } from '../DateFilterControl/utils';
 
-const { confirm } = Modal;
+const { warning } = Modal;
 
 const EMPTY_OBJECT = {};
 const DND_ACCEPTED_TYPES = [
@@ -78,22 +78,18 @@ export interface DndFilterSelectProps
   savedMetrics: Metric[];
   selectedMetrics: QueryFormMetric[];
   datasource: Datasource;
-  confirmDeletion?: {
-    triggerCondition: (
-      valueToBeDeleted: OptionValueType,
-      values: OptionValueType[],
-    ) => boolean;
-    confirmationTitle: string;
-    confirmationText: string;
-  };
+  canDelete?: (
+    valueToBeDeleted: OptionValueType,
+    values: OptionValueType[],
+  ) => true | string;
 }
 
 const DndFilterSelect = (props: DndFilterSelectProps) => {
   const {
     datasource,
     onChange = () => {},
     name: controlName,
-    confirmDeletion,
+    canDelete,
   } = props;
 
   const propsValues = Array.from(props.value ?? []);
@@ -217,23 +213,14 @@ const DndFilterSelect = (props: DndFilterSelectProps) => {
 
   const onClickClose = useCallback(
     (index: number) => {
-      if (confirmDeletion) {
-        const { confirmationText, confirmationTitle, triggerCondition } =
-          confirmDeletion;
-        if (triggerCondition(values[index], values)) {
-          confirm({
-            title: confirmationTitle,
-            content: confirmationText,
-            onOk() {
-              removeValue(index);
-            },
-          });
-          return;
-        }
+      const result = canDelete?.(values[index], values);
+      if (typeof result === 'string') {
+        warning({ title: t('Warning'), content: result });
+        return;
       }
       removeValue(index);
     },
-    [confirmDeletion, removeValue, values],
+    [canDelete, removeValue, values],
   );
 
   const onShiftOptions = useCallback(

diff --git a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.jsx b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.jsx
@@ -52,7 +52,7 @@ import AdhocFilter, {
 import adhocFilterType from 'src/explore/components/controls/FilterControl/adhocFilterType';
 import columnType from 'src/explore/components/controls/FilterControl/columnType';
 
-const { confirm } = Modal;
+const { warning } = Modal;
 
 const selectedMetricType = PropTypes.oneOfType([
   PropTypes.string,
@@ -74,11 +74,7 @@ const propTypes = {
     PropTypes.arrayOf(selectedMetricType),
   ]),
   isLoading: PropTypes.bool,
-  confirmDeletion: PropTypes.shape({
-    triggerCondition: PropTypes.func,
-    confirmationTitle: PropTypes.string,
-    confirmationText: PropTypes.string,
-  }),
+  canDelete: PropTypes.func,
 };
 
 const defaultProps = {
@@ -196,22 +192,12 @@ class AdhocFilterControl extends React.Component {
   }
 
   onRemoveFilter(index) {
-    const { confirmDeletion } = this.props;
+    const { canDelete } = this.props;
     const { values } = this.state;
-    const { removeFilter } = this;
-    if (confirmDeletion) {
-      const { confirmationText, confirmationTitle, triggerCondition } =
-        confirmDeletion;
-      if (triggerCondition(values[index], values)) {
-        confirm({
-          title: confirmationTitle,
-          content: confirmationText,
-          onOk() {
-            removeFilter(index);
-          },
-        });
-        return;
-      }
+    const result = canDelete?.(values[index], values);
+    if (typeof result === 'string') {
+      warning({ title: t('Warning'), content: result });
+      return;
     }
     this.removeFilter(index);
   }