From 95a958ba11874424a09417e890add84b5edbb240 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Fri, 3 Feb 2023 09:15:12 -0500 Subject: [PATCH 1/2] fix: Prevents last temporal filter removal --- .../components/ControlPanelsContainer.tsx | 47 +++++++++---------- .../DndFilterSelect.tsx | 33 +++++-------- .../AdhocFilterControl/index.jsx | 26 +++------- 3 files changed, 41 insertions(+), 65 deletions(-) diff --git a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx index 497ed2733685b..c873f99a9f7cb 100644 --- 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, - values: Record[], - ) => { - const isTemporalRange = (filter: Record) => - filter.operator === Operators.TEMPORAL_RANGE; - if (isTemporalRange(valueToBeDeleted)) { - const count = values.filter(isTemporalRange).length; - if (count < 2) { - return true; - } + restProps.canDelete = ( + valueToBeDeleted: Record, + values: Record[], + ) => { + const isTemporalRange = (filter: Record) => + filter.operator === Operators.TEMPORAL_RANGE; + if (isTemporalRange(valueToBeDeleted)) { + const count = values.filter(isTemporalRange).length; + if (count < 2) { + 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 index 90cf156c118f9..df1fa3e87a8f6 100644 --- 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,14 +78,10 @@ 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) => { @@ -93,7 +89,7 @@ const DndFilterSelect = (props: DndFilterSelectProps) => { datasource, onChange = () => {}, name: controlName, - confirmDeletion, + canDelete, } = props; const propsValues = Array.from(props.value ?? []); @@ -217,23 +213,16 @@ 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); - }, - }); + if (canDelete) { + 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 index 44f4c469e6b23..79e0326cc2fe6 100644 --- 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,20 +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); - }, - }); + if (canDelete) { + const result = canDelete(values[index], values); + if (typeof result === 'string') { + warning({ title: t('Warning'), content: result }); return; } } From 7ac2ac04cc7ba6dc013b68418352eea428f36231 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Fri, 3 Feb 2023 09:56:46 -0500 Subject: [PATCH 2/2] Addresses comments --- .../src/explore/components/ControlPanelsContainer.tsx | 2 +- .../DndColumnSelectControl/DndFilterSelect.tsx | 10 ++++------ .../FilterControl/AdhocFilterControl/index.jsx | 10 ++++------ 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx index c873f99a9f7cb..60aac1948bf4e 100644 --- a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx +++ b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx @@ -496,7 +496,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => { filter.operator === Operators.TEMPORAL_RANGE; if (isTemporalRange(valueToBeDeleted)) { const count = values.filter(isTemporalRange).length; - if (count < 2) { + if (count === 1) { return t( `You cannot delete the last temporal filter as it's used for time range filters in dashboards.`, ); diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx index df1fa3e87a8f6..134678aa6fad9 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx @@ -213,12 +213,10 @@ const DndFilterSelect = (props: DndFilterSelectProps) => { const onClickClose = useCallback( (index: number) => { - if (canDelete) { - const result = canDelete(values[index], values); - if (typeof result === 'string') { - warning({ title: t('Warning'), content: result }); - return; - } + const result = canDelete?.(values[index], values); + if (typeof result === 'string') { + warning({ title: t('Warning'), content: result }); + return; } removeValue(index); }, diff --git a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.jsx b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.jsx index 79e0326cc2fe6..c0750c7805d00 100644 --- a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.jsx +++ b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.jsx @@ -194,12 +194,10 @@ class AdhocFilterControl extends React.Component { onRemoveFilter(index) { const { canDelete } = this.props; const { values } = this.state; - if (canDelete) { - const result = canDelete(values[index], values); - if (typeof result === 'string') { - warning({ title: t('Warning'), content: result }); - return; - } + const result = canDelete?.(values[index], values); + if (typeof result === 'string') { + warning({ title: t('Warning'), content: result }); + return; } this.removeFilter(index); }