From 3e5678baa5ce0fc0ed46f9c3913fbadf508bc322 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Thu, 12 Jan 2023 16:48:44 -0500 Subject: [PATCH 1/4] feat: Warns users when deleting temporal filters --- .../FilterConfigurePane.tsx | 2 +- .../FiltersConfigForm/FiltersConfigForm.tsx | 20 +++++++++ .../components/ControlPanelsContainer.tsx | 29 +++++++++++++ .../DndFilterSelect.tsx | 42 ++++++++++++++++++- .../AdhocFilterControl/index.jsx | 31 +++++++++++++- .../AdhocFilterEditPopover/index.jsx | 5 ++- .../AdhocFilterPopoverTrigger/index.tsx | 2 + .../controls/OptionControls/index.tsx | 5 ++- 8 files changed, 130 insertions(+), 6 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx index d02a6ba2c3165..b582b240977d4 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx @@ -46,7 +46,7 @@ const ContentHolder = styled.div` `; const TitlesContainer = styled.div` - width: 270px; + min-width: 270px; border-right: 1px solid ${({ theme }) => theme.colors.grayscale.light2}; `; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx index d70679128e4cb..0ea30a99b8b4d 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx @@ -258,6 +258,19 @@ const StyledAsterisk = styled.span` } `; +const TimeRangeInfo = styled.div` + ${({ theme }) => ` + width: 49%; + font-size: ${theme.typography.sizes.s}px; + color: ${theme.colors.grayscale.light1}; + margin: + ${-theme.gridUnit * 2}px + 0px + ${theme.gridUnit * 4}px + ${theme.gridUnit * 4}px; + `} +`; + const FilterTabs = { configuration: { key: 'configuration', @@ -795,6 +808,13 @@ const FiltersConfigForm = ( /> + {filterToEdit?.filterType === 'filter_time' && ( + + {t(`Dashboard time range filters apply to temporal columns defined in + the filter section of each chart. Add temporal columns to the chart + filters to have this dashboard filter impact those charts.`)} + + )} {hasDataset && ( {showDataset ? ( diff --git a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx index 7b0ffeb3b67d0..91f0daf36329e 100644 --- a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx +++ b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx @@ -66,6 +66,7 @@ import ControlRow from './ControlRow'; import Control from './Control'; import { ExploreAlert } from './ExploreAlert'; import { RunQueryButton } from './RunQueryButton'; +import { Operators } from '../constants'; export type ControlPanelsContainerProps = { exploreState: ExplorePageState['explore']; @@ -431,6 +432,34 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => { ? baseDescription(exploreState, controls[name], chart) : baseDescription; + if (name === 'adhoc_filters') { + restProps.confirmDeletion = { + triggerCondition: ( + valueToBeDeleted: Record, + values: Record[], + ) => { + const isTemporalRange = + valueToBeDeleted.operator === Operators.TEMPORAL_RANGE; + if (isTemporalRange) { + const count = values.filter( + value => value.operator === Operators.TEMPORAL_RANGE, + ).length; + if (count < 2) { + return true; + } + } + 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, + your charts won't be affected by time range filters in dashboards.`, + ), + }; + } + return ( boolean; + confirmationTitle: string; + confirmationText: string; + }; } const DndFilterSelect = (props: DndFilterSelectProps) => { - const { datasource, onChange = () => {}, name: controlName } = props; + const { + datasource, + onChange = () => {}, + name: controlName, + confirmDeletion, + } = props; const propsValues = Array.from(props.value ?? []); const [values, setValues] = useState( @@ -189,7 +205,7 @@ const DndFilterSelect = (props: DndFilterSelectProps) => { ); }, [props.value]); - const onClickClose = useCallback( + const removeValue = useCallback( (index: number) => { const valuesCopy = [...values]; valuesCopy.splice(index, 1); @@ -199,6 +215,27 @@ const DndFilterSelect = (props: DndFilterSelectProps) => { [onChange, values], ); + 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; + } + } + removeValue(index); + }, + [confirmDeletion, removeValue, values], + ); + const onShiftOptions = useCallback( (dragIndex: number, hoverIndex: number) => { const newValues = [...values]; @@ -404,6 +441,7 @@ const DndFilterSelect = (props: DndFilterSelectProps) => { visible={newFilterPopoverVisible} togglePopover={togglePopover} closePopover={closePopover} + requireSave={!!droppedItem} /> ); 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 813600a4c16d6..2114a9bd45d5c 100644 --- a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.jsx +++ b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.jsx @@ -42,6 +42,7 @@ import { LabelsContainer, } from 'src/explore/components/controls/OptionControls'; import Icons from 'src/components/Icons'; +import Modal from 'src/components/Modal'; import AdhocFilterPopoverTrigger from 'src/explore/components/controls/FilterControl/AdhocFilterPopoverTrigger'; import AdhocFilterOption from 'src/explore/components/controls/FilterControl/AdhocFilterOption'; import AdhocFilter, { @@ -51,6 +52,8 @@ import AdhocFilter, { import adhocFilterType from 'src/explore/components/controls/FilterControl/adhocFilterType'; import columnType from 'src/explore/components/controls/FilterControl/columnType'; +const { confirm } = Modal; + const selectedMetricType = PropTypes.oneOfType([ PropTypes.string, adhocMetricType, @@ -71,6 +74,11 @@ const propTypes = { PropTypes.arrayOf(selectedMetricType), ]), isLoading: PropTypes.bool, + confirmDeletion: PropTypes.shape({ + triggerCondition: PropTypes.func, + confirmationTitle: PropTypes.string, + confirmationText: PropTypes.string, + }), }; const defaultProps = { @@ -96,6 +104,7 @@ class AdhocFilterControl extends React.Component { this.onChange = this.onChange.bind(this); this.mapOption = this.mapOption.bind(this); this.getMetricExpression = this.getMetricExpression.bind(this); + this.removeFilter = this.removeFilter.bind(this); const filters = (this.props.value || []).map(filter => isDictionaryForAdhocFilter(filter) ? new AdhocFilter(filter) : filter, @@ -173,7 +182,7 @@ class AdhocFilterControl extends React.Component { } } - onRemoveFilter(index) { + removeFilter(index) { const valuesCopy = [...this.state.values]; valuesCopy.splice(index, 1); this.setState(prevState => ({ @@ -183,6 +192,26 @@ class AdhocFilterControl extends React.Component { this.props.onChange(valuesCopy); } + onRemoveFilter(index) { + const { confirmDeletion } = this.props; + const { values } = this.state; + if (confirmDeletion) { + const { confirmationText, confirmationTitle, triggerCondition } = + confirmDeletion; + if (triggerCondition(values[index], values)) { + confirm({ + title: confirmationTitle, + content: confirmationText, + onOk() { + this.removeFilter(index); + }, + }); + return; + } + } + this.removeFilter(index); + } + onNewFilter(newFilter) { const mappedOption = this.mapOption(newFilter); if (mappedOption) { diff --git a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx index fc3d5a3a0183e..98c6da8f1c004 100644 --- a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx +++ b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.jsx @@ -52,6 +52,7 @@ const propTypes = { theme: PropTypes.object, sections: PropTypes.arrayOf(PropTypes.string), operators: PropTypes.arrayOf(PropTypes.string), + requireSave: PropTypes.bool, }; const ResizeIcon = styled.i` @@ -181,12 +182,14 @@ export default class AdhocFilterEditPopover extends React.Component { partitionColumn, theme, operators, + requireSave, ...popoverProps } = this.props; const { adhocFilter } = this.state; const stateIsValid = adhocFilter.isValid(); - const hasUnsavedChanges = !adhocFilter.equals(propsAdhocFilter); + const hasUnsavedChanges = + requireSave || !adhocFilter.equals(propsAdhocFilter); return ( void; closePopover?: () => void; + requireSave?: boolean; } interface AdhocFilterPopoverTriggerState { @@ -96,6 +97,7 @@ class AdhocFilterPopoverTrigger extends React.PureComponent< sections={this.props.sections} operators={this.props.operators} onChange={this.props.onFilterEdit} + requireSave={this.props.requireSave} /> ); diff --git a/superset-frontend/src/explore/components/controls/OptionControls/index.tsx b/superset-frontend/src/explore/components/controls/OptionControls/index.tsx index 90ae73c6370aa..495fab71e5287 100644 --- a/superset-frontend/src/explore/components/controls/OptionControls/index.tsx +++ b/superset-frontend/src/explore/components/controls/OptionControls/index.tsx @@ -308,7 +308,10 @@ export const OptionControlLabel = ({ { + e.stopPropagation(); + onRemove(); + }} > From f6aeed46a16b3aec231c0512644b65d0e281ec6d Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Sun, 15 Jan 2023 13:33:09 -0500 Subject: [PATCH 2/4] Adds helper for X-axis changes --- .../components/ControlPanelsContainer.tsx | 70 +++++++++++++++---- 1 file changed, 58 insertions(+), 12 deletions(-) diff --git a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx index 91f0daf36329e..985c87480c15a 100644 --- a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx +++ b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx @@ -38,6 +38,7 @@ import { useTheme, isDefined, JsonValue, + NO_TIME_RANGE, } from '@superset-ui/core'; import { ControlPanelSectionConfig, @@ -55,6 +56,7 @@ import Collapse from 'src/components/Collapse'; import Tabs from 'src/components/Tabs'; import { PluginContext } from 'src/components/DynamicPlugins'; import Loading from 'src/components/Loading'; +import Modal from 'src/components/Modal'; import { usePrevious } from 'src/hooks/usePrevious'; import { getSectionsToRender } from 'src/explore/controlUtils'; @@ -68,6 +70,8 @@ import { ExploreAlert } from './ExploreAlert'; import { RunQueryButton } from './RunQueryButton'; import { Operators } from '../constants'; +const { confirm } = Modal; + export type ControlPanelsContainerProps = { exploreState: ExplorePageState['explore']; actions: ExploreActions; @@ -278,6 +282,55 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => { string[] | undefined >(state => state.explore.controlsTransferred); + const defaultTimeFilter = useSelector( + state => state.common?.conf?.DEFAULT_TIME_FILTER, + ); + + const { form_data, actions } = props; + const { setControlValue } = actions; + const { x_axis, adhoc_filters } = form_data; + + const previousXAxis = usePrevious(x_axis); + + useEffect(() => { + if (x_axis && x_axis !== previousXAxis) { + const noFilter = + !adhoc_filters || + !adhoc_filters.find( + filter => + filter.expressionType === 'SIMPLE' && + filter.operator === 'TEMPORAL_RANGE' && + filter.subject === x_axis, + ); + if (noFilter) { + confirm({ + title: t('The X-axis is not on the filters list'), + content: + t(`The X-axis is not on the filters list which will prevent it from being used in + time range filters in dashboards. Would you like to add it to the filters list?`), + onOk: () => { + setControlValue('adhoc_filters', [ + ...(adhoc_filters || []), + { + clause: 'WHERE', + subject: x_axis, + operator: 'TEMPORAL_RANGE', + comparator: defaultTimeFilter || NO_TIME_RANGE, + expressionType: 'SIMPLE', + }, + ]); + }, + }); + } + } + }, [ + x_axis, + adhoc_filters, + setControlValue, + defaultTimeFilter, + previousXAxis, + ]); + useEffect(() => { let shouldUpdateControls = false; const removeDatasourceWarningFromControl = ( @@ -347,15 +400,11 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => { } = useMemo( () => getState( - props.form_data.viz_type, + form_data.viz_type, props.exploreState.datasource, props.datasource_type, ), - [ - props.exploreState.datasource, - props.form_data.viz_type, - props.datasource_type, - ], + [props.exploreState.datasource, form_data.viz_type, props.datasource_type], ); const resetTransferredControls = useCallback(() => { @@ -476,7 +525,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => { const sectionHasHadNoErrors = useResetOnChangeRef( () => ({}), - props.form_data.viz_type, + form_data.viz_type, ); const renderControlPanelSection = ( @@ -644,7 +693,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => { const dataTabHasHadNoErrors = useResetOnChangeRef( () => false, - props.form_data.viz_type, + form_data.viz_type, ); const dataTabTitle = useMemo(() => { @@ -690,10 +739,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => { ]); const controlPanelRegistry = getChartControlPanelRegistry(); - if ( - !controlPanelRegistry.has(props.form_data.viz_type) && - pluginContext.loading - ) { + if (!controlPanelRegistry.has(form_data.viz_type) && pluginContext.loading) { return ; } From 45258ccad9d784261c453c70f183f15780fdca18 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Tue, 17 Jan 2023 15:23:45 -0500 Subject: [PATCH 3/4] Addresses comments --- .../FiltersConfigForm/FiltersConfigForm.tsx | 2 +- .../explore/components/ControlPanelsContainer.tsx | 12 +++++------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx index 0ea30a99b8b4d..11718cb148391 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx @@ -808,7 +808,7 @@ const FiltersConfigForm = ( /> - {filterToEdit?.filterType === 'filter_time' && ( + {formFilter?.filterType === 'filter_time' && ( {t(`Dashboard time range filters apply to temporal columns defined in the filter section of each chart. Add temporal columns to the chart diff --git a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx index 985c87480c15a..497ed2733685b 100644 --- a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx +++ b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx @@ -487,12 +487,10 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => { valueToBeDeleted: Record, values: Record[], ) => { - const isTemporalRange = - valueToBeDeleted.operator === Operators.TEMPORAL_RANGE; - if (isTemporalRange) { - const count = values.filter( - value => value.operator === Operators.TEMPORAL_RANGE, - ).length; + const isTemporalRange = (filter: Record) => + filter.operator === Operators.TEMPORAL_RANGE; + if (isTemporalRange(valueToBeDeleted)) { + const count = values.filter(isTemporalRange).length; if (count < 2) { return true; } @@ -504,7 +502,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => { ), confirmationText: t( `This filter is the last temporal filter. If you proceed, - your charts won't be affected by time range filters in dashboards.`, + this chart won't be affected by time range filters in dashboards.`, ), }; } From 7033fed93187cc8c61ef5404e918000d52e73a89 Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" Date: Wed, 18 Jan 2023 07:39:48 -0500 Subject: [PATCH 4/4] Uses a generic name for the filter type message --- .../FiltersConfigForm/FiltersConfigForm.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx index 11718cb148391..fcfde1f33a067 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx @@ -258,7 +258,7 @@ const StyledAsterisk = styled.span` } `; -const TimeRangeInfo = styled.div` +const FilterTypeInfo = styled.div` ${({ theme }) => ` width: 49%; font-size: ${theme.typography.sizes.s}px; @@ -809,11 +809,11 @@ const FiltersConfigForm = ( {formFilter?.filterType === 'filter_time' && ( - + {t(`Dashboard time range filters apply to temporal columns defined in the filter section of each chart. Add temporal columns to the chart filters to have this dashboard filter impact those charts.`)} - + )} {hasDataset && (