From d70ac21054bddd8016e8864f9f28d8e3c8dd9f7f Mon Sep 17 00:00:00 2001 From: simcha90 <56388545+simcha90@users.noreply.github.com> Date: Mon, 12 Jul 2021 14:55:11 +0300 Subject: [PATCH] fix(native-filters): Fix required filters (#15572) * fix:fix get permission function * fix: filters required state * fix: fix CR notes * fix: removre required message * fix: fix validation state --- .../FilterBar/FilterControls/FilterValue.tsx | 2 +- .../FiltersConfigForm/DefaultValue.tsx | 6 +- .../FiltersConfigForm/FiltersConfigForm.tsx | 117 +++++++++--------- .../FiltersConfigForm/state.ts | 41 +++--- .../FiltersConfigModal/FiltersConfigModal.tsx | 12 -- .../nativeFilters/FiltersConfigModal/utils.ts | 2 - .../GroupBy/GroupByFilterPlugin.tsx | 22 ++-- .../components/Range/RangeFilterPlugin.tsx | 44 ++++--- .../components/Select/SelectFilterPlugin.tsx | 22 ++-- .../components/Time/TimeFilterPlugin.tsx | 12 +- .../TimeColumn/TimeColumnFilterPlugin.tsx | 21 ++-- .../TimeGrain/TimeGrainFilterPlugin.tsx | 21 ++-- .../src/filters/components/common.ts | 6 + 13 files changed, 180 insertions(+), 148 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx index f833e9e737..016b4987be 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx @@ -204,7 +204,7 @@ const FilterValue: React.FC = ({ ); const filterState = { ...filter.dataMask?.filterState, - validateMessage: isMissingRequiredValue && t('Value is required'), + validateStatus: isMissingRequiredValue && 'error', }; if (filterState.value === undefined && preselection) { filterState.value = preselection; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx index 85d55807b2..b56f2c221b 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx @@ -30,6 +30,7 @@ import { NativeFiltersForm } from '../types'; import { getFormData } from '../../utils'; type DefaultValueProps = { + hasDefaultValue: boolean; filterId: string; setDataMask: SetDataMaskHook; hasDataset: boolean; @@ -39,6 +40,7 @@ type DefaultValueProps = { }; const DefaultValue: FC = ({ + hasDefaultValue, filterId, hasDataset, form, @@ -59,8 +61,7 @@ const DefaultValue: FC = ({ }, [hasDataset, queriesData]); const value = formFilter.defaultDataMask?.filterState.value; const isMissingRequiredValue = - (value === null || value === undefined) && - formFilter?.controlValues?.enableEmptyFilter; + hasDefaultValue && (value === null || value === undefined); return loading ? ( ) : ( @@ -80,6 +81,7 @@ const DefaultValue: FC = ({ filterState={{ ...formFilter.defaultDataMask?.filterState, validateMessage: isMissingRequiredValue && t('Value is required'), + validateStatus: isMissingRequiredValue && 'error', }} /> ); 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 306f06bb32..61115312dd 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx @@ -756,64 +756,69 @@ const FiltersConfigForm = ( checked={hasDefaultValue} onChange={value => setHasDefaultValue(value)} > - {t('Default Value')}} - required={hasDefaultValue} - rules={[ - { - validator: (rule, value) => { - const hasValue = !!value?.filterState?.value; - if (hasValue) { - return Promise.resolve(); - } - return Promise.reject( - new Error(t('Default value is required')), - ); + {formFilter.filterType && ( + {t('Default Value')}} + required={hasDefaultValue} + rules={[ + { + validator: (rule, value) => { + const hasValue = + value?.filterState?.value !== null && + value?.filterState?.value !== undefined; + if (hasValue) { + return Promise.resolve(); + } + return Promise.reject( + new Error(t('Default value is required')), + ); + }, }, - }, - ]} - > - {error ? ( - - ) : showDefaultValue ? ( - - { - setNativeFilterFieldValues(form, filterId, { - defaultDataMask: dataMask, - }); - form.validateFields([ - ['filters', filterId, 'defaultDataMask'], - ]); - forceUpdate(); - }} - filterId={filterId} - hasDataset={hasDataset} - form={form} - formData={newFormData} - enableNoResults={enableNoResults} + ]} + > + {error ? ( + - {hasDataset && datasetId && ( - - refreshHandler(true)} /> - - )} - - ) : ( - t('Fill all required fields to enable "Default Value"') - )} - + ) : showDefaultValue ? ( + + { + setNativeFilterFieldValues(form, filterId, { + defaultDataMask: dataMask, + }); + form.validateFields([ + ['filters', filterId, 'defaultDataMask'], + ]); + forceUpdate(); + }} + hasDefaultValue={hasDefaultValue} + filterId={filterId} + hasDataset={hasDataset} + form={form} + formData={newFormData} + enableNoResults={enableNoResults} + /> + {hasDataset && datasetId && ( + + refreshHandler(true)} /> + + )} + + ) : ( + t('Fill all required fields to enable "Default Value"') + )} + + )} {Object.keys(controlItems) .filter(key => BASIC_CONTROL_ITEMS.includes(key)) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/state.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/state.ts index fa6058de85..6580ade3f9 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/state.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/state.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { useCallback, useEffect, useState } from 'react'; +import { useEffect, useState } from 'react'; import { FormInstance } from 'antd/lib/form'; import { t } from '@superset-ui/core'; import { NativeFiltersForm, NativeFiltersFormItem } from '../types'; @@ -52,27 +52,19 @@ export const useBackendFormUpdate = ( export const useDefaultValue = ( formFilter?: NativeFiltersFormItem, filterToEdit?: Filter, -) => { - const [hasDefaultValue, setHasPartialDefaultValue] = useState( - !!filterToEdit?.defaultDataMask?.filterState?.value, - ); - const [isRequired, setisRequired] = useState( - formFilter?.controlValues?.enableEmptyFilter, - ); +): [boolean, boolean, string, Function] => { + const enableEmptyFilter = !!formFilter?.controlValues?.enableEmptyFilter; + const defaultToFirstItem = !!formFilter?.controlValues?.defaultToFirstItem; + const [hasDefaultValue, setHasPartialDefaultValue] = useState(false); + const [isRequired, setIsRequired] = useState(enableEmptyFilter); const [defaultValueTooltip, setDefaultValueTooltip] = useState(''); - const defaultToFirstItem = formFilter?.controlValues?.defaultToFirstItem; - - const setHasDefaultValue = useCallback( - (value?) => { - const required = - !!formFilter?.controlValues?.enableEmptyFilter && !defaultToFirstItem; - setisRequired(required); - setHasPartialDefaultValue(required ? true : value); - }, - [formFilter?.controlValues?.enableEmptyFilter, defaultToFirstItem], - ); + const setHasDefaultValue = (value = false) => { + const required = enableEmptyFilter && !defaultToFirstItem; + setIsRequired(required); + setHasPartialDefaultValue(required ? true : value); + }; useEffect(() => { setHasDefaultValue( @@ -80,7 +72,16 @@ export const useDefaultValue = ( ? false : !!formFilter?.defaultDataMask?.filterState?.value, ); - }, [setHasDefaultValue, defaultToFirstItem]); + // TODO: this logic should be unhardcoded + }, [defaultToFirstItem, enableEmptyFilter]); + + useEffect(() => { + setHasDefaultValue( + defaultToFirstItem + ? false + : !!filterToEdit?.defaultDataMask?.filterState?.value, + ); + }, []); useEffect(() => { let tooltip = ''; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx index 64ac790741..bde38f250a 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx @@ -164,16 +164,6 @@ export function FiltersConfigModal({ addFilter, ); - // After this, it should be as if the modal was just opened fresh. - // Called when the modal is closed. - const resetForm = () => { - form.resetFields(); - setNewFilterIds([]); - setCurrentFilterId(initialCurrentFilterId); - setRemovedFilters({}); - setSaveAlertVisible(false); - }; - const getFilterTitle = (id: string) => formValues.filters[id]?.name ?? filterConfigMap[id]?.name ?? @@ -209,7 +199,6 @@ export function FiltersConfigModal({ filterConfigMap, filterIds, removedFilters, - resetForm, onSave, values, )(); @@ -219,7 +208,6 @@ export function FiltersConfigModal({ }; const handleConfirmCancel = () => { - resetForm(); onCancel(); }; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts index 89de8c96f5..c2307e4b45 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts @@ -104,7 +104,6 @@ export const createHandleSave = ( filterConfigMap: Record, filterIds: string[], removedFilters: Record, - resetForm: Function, saveForm: Function, values: NativeFiltersForm, ) => async () => { @@ -145,7 +144,6 @@ export const createHandleSave = ( }); await saveForm(newFilterConfig); - resetForm(); }; export const createHandleTabEdit = ( diff --git a/superset-frontend/src/filters/components/GroupBy/GroupByFilterPlugin.tsx b/superset-frontend/src/filters/components/GroupBy/GroupByFilterPlugin.tsx index 981a9280e4..32fa094891 100644 --- a/superset-frontend/src/filters/components/GroupBy/GroupByFilterPlugin.tsx +++ b/superset-frontend/src/filters/components/GroupBy/GroupByFilterPlugin.tsx @@ -16,18 +16,15 @@ * specific language governing permissions and limitations * under the License. */ -import { ensureIsArray, ExtraFormData, styled, t, tn } from '@superset-ui/core'; +import { ensureIsArray, ExtraFormData, t, tn } from '@superset-ui/core'; import React, { useEffect, useState } from 'react'; import { Select } from 'src/common/components'; -import { Styles, StyledSelect, StyledFormItem } from '../common'; +import { FormItemProps } from 'antd/lib/form'; +import { Styles, StyledSelect, StyledFormItem, StatusMessage } from '../common'; import { PluginFilterGroupByProps } from './types'; const { Option } = Select; -const Error = styled.div` - color: ${({ theme }) => theme.colors.error.base}; -`; - export default function PluginFilterGroupBy(props: PluginFilterGroupByProps) { const { data, @@ -84,11 +81,20 @@ export default function PluginFilterGroupBy(props: PluginFilterGroupByProps) { columns.length === 0 ? t('No columns') : tn('%s option', '%s options', columns.length, columns.length); + + const formItemData: FormItemProps = {}; + if (filterState.validateMessage) { + formItemData.extra = ( + + {filterState.validateMessage} + + ); + } return ( {filterState.validateMessage}} + validateStatus={filterState.validateStatus} + {...formItemData} > theme.colors.error.base}; -`; - -const Wrapper = styled.div<{ validateStatus?: string }>` +const Wrapper = styled.div<{ validateStatus?: 'error' | 'warning' | 'info' }>` border: 1px solid transparent; &:focus { border: 1px solid ${({ theme, validateStatus }) => - theme.colors[validateStatus ? 'error' : 'primary'].base}; + theme.colors[validateStatus || 'primary']?.base}; outline: 0; box-shadow: 0 0 0 3px ${({ theme, validateStatus }) => - rgba(theme.colors[validateStatus ? 'error' : 'primary'].base, 0.2)}; + rgba(theme.colors[validateStatus || 'primary']?.base, 0.2)}; } & .ant-slider { & .ant-slider-track { background-color: ${({ theme, validateStatus }) => - validateStatus && theme.colors.error.light1}; + validateStatus && theme.colors[validateStatus]?.light1}; } & .ant-slider-handle { border: ${({ theme, validateStatus }) => - validateStatus && `2px solid ${theme.colors.error.light1}`}; + validateStatus && `2px solid ${theme.colors[validateStatus]?.light1}`}; &:focus { box-shadow: 0 0 0 3px ${({ theme, validateStatus }) => - rgba(theme.colors[validateStatus ? 'error' : 'primary'].base, 0.2)}; + rgba(theme.colors[validateStatus || 'primary']?.base, 0.2)}; } } &:hover { & .ant-slider-track { background-color: ${({ theme, validateStatus }) => - validateStatus && theme.colors.error.base}; + validateStatus && theme.colors[validateStatus]?.base}; } & .ant-slider-handle { border: ${({ theme, validateStatus }) => - validateStatus && `2px solid ${theme.colors.error.base}`}; + validateStatus && `2px solid ${theme.colors[validateStatus]?.base}`}; } } } @@ -150,22 +147,31 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) { }; useEffect(() => { + // when switch filter type and queriesData still not updated we need ignore this case (in FilterBar) + if (row?.min === undefined && row?.max === undefined) { + return; + } handleAfterChange(filterState.value ?? [min, max]); - }, [JSON.stringify(filterState.value)]); + }, [JSON.stringify(filterState.value), JSON.stringify(data)]); + const formItemData: FormItemProps = {}; + if (filterState.validateMessage) { + formItemData.extra = ( + + {filterState.validateMessage} + + ); + } return ( {Number.isNaN(Number(min)) || Number.isNaN(Number(max)) ? (

{t('Chosen non-numeric column')}

) : ( - {filterState.validateMessage}} - > + theme.colors.error.base}; -`; - type DataMaskAction = | { type: 'ownState'; ownState: JsonObject } | { @@ -152,6 +148,7 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { inverseSelection, ), filterState: { + ...filterState, label: values?.length ? `${(values || []).join(', ')}${suffix}` : undefined, @@ -276,11 +273,20 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { : tn('%s option', '%s options', data.length, data.length); const Icon = inverseSelection ? Icons.StopOutlined : Icons.CheckOutlined; + const formItemData: FormItemProps = {}; + if (filterState.validateMessage) { + formItemData.extra = ( + + {filterState.validateMessage} + + ); + } + return ( {filterState.validateMessage}} + validateStatus={filterState.validateStatus} + {...formItemData} > ` +const ControlContainer = styled.div<{ + validateStatus?: 'error' | 'warning' | 'info'; +}>` padding: 2px; & > span { border: 2px solid transparent; display: inline-block; border: ${({ theme, validateStatus }) => - validateStatus && `2px solid ${theme.colors.error.base}`}; + validateStatus && `2px solid ${theme.colors[validateStatus]?.base}`}; } &:focus { & > span { border: 2px solid ${({ theme, validateStatus }) => - validateStatus ? theme.colors.error.base : theme.colors.primary.base}; + validateStatus + ? theme.colors[validateStatus]?.base + : theme.colors.primary.base}; outline: 0; box-shadow: 0 0 0 2px ${({ validateStatus }) => @@ -85,7 +89,7 @@ export default function TimeFilterPlugin(props: PluginFilterTimeProps) { theme.colors.error.base}; -`; - export default function PluginFilterTimeColumn( props: PluginFilterTimeColumnProps, ) { @@ -86,11 +82,20 @@ export default function PluginFilterTimeColumn( timeColumns.length === 0 ? t('No time columns') : tn('%s option', '%s options', timeColumns.length, timeColumns.length); + + const formItemData: FormItemProps = {}; + if (filterState.validateMessage) { + formItemData.extra = ( + + {filterState.validateMessage} + + ); + } return ( {filterState.validateMessage}} + validateStatus={filterState.validateStatus} + {...formItemData} > theme.colors.error.base}; -`; - export default function PluginFilterTimegrain( props: PluginFilterTimeGrainProps, ) { @@ -96,11 +92,20 @@ export default function PluginFilterTimegrain( (data || []).length === 0 ? t('No data') : tn('%s option', '%s options', data.length, data.length); + + const formItemData: FormItemProps = {}; + if (filterState.validateMessage) { + formItemData.extra = ( + + {filterState.validateMessage} + + ); + } return ( {filterState.validateMessage}} + validateStatus={filterState.validateStatus} + {...formItemData} > ` + color: ${({ theme, status = 'error' }) => theme.colors[status]?.base}; +`;