From aca9dfcaac40df912bf05d95d5bb3e9dd2347c80 Mon Sep 17 00:00:00 2001 From: geido Date: Fri, 1 Oct 2021 20:19:51 +0000 Subject: [PATCH 1/5] Implement errored filters --- .../FiltersConfigModal/FilterTabs.tsx | 76 ++++++++++++------- .../FilterScope/FilterScope.test.tsx | 1 + .../FiltersConfigForm/FiltersConfigForm.tsx | 7 +- .../FiltersConfigModal/FiltersConfigModal.tsx | 33 +++++++- .../nativeFilters/FiltersConfigModal/utils.ts | 23 +++++- 5 files changed, 108 insertions(+), 32 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx index a5f1dd03fb8b8..8a1db12d9acb5 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx @@ -79,6 +79,11 @@ export const FilterTabTitle = styled.span` animation-name: tabTitleRemovalAnimation; animation-duration: ${REMOVAL_DELAY_SECS}s; } + + &.errored > span, + &.errored > span > span { + color: ${({ theme }) => theme.colors.error.base}; + } `; const FilterTabsContainer = styled(LineEditableTabs)` @@ -169,6 +174,7 @@ type FilterTabsProps = { currentFilterId: string; onEdit: (filterId: string, action: 'add' | 'remove') => void; filterIds: string[]; + erroredFilters: string[]; removedFilters: Record; restoreFilter: Function; children: Function; @@ -180,6 +186,7 @@ const FilterTabs: FC = ({ onChange, currentFilterId, filterIds = [], + erroredFilters = [], removedFilters = [], restoreFilter, children, @@ -217,34 +224,47 @@ const FilterTabs: FC = ({ ), }} > - {filterIds.map(id => ( - - - {removedFilters[id] ? t('(Removed)') : getFilterTitle(id)} - - {removedFilters[id] && ( - restoreFilter(id)} - > - {t('Undo?')} - - )} - - } - key={id} - closeIcon={removedFilters[id] ? <> : } - > - { - // @ts-ignore - children(id) - } - - ))} + {filterIds.map(id => { + const showErroredFilter = erroredFilters.includes(id); + const filterName = getFilterTitle(id) || t('[untitled]'); + return ( + + + {showErroredFilter && } + {removedFilters[id] ? t('(Removed)') : filterName} + + {removedFilters[id] && ( + restoreFilter(id)} + > + {t('Undo?')} + + )} + + } + key={id} + closeIcon={removedFilters[id] ? <> : } + > + { + // @ts-ignore + children(id) + } + + ); + })} ); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.test.tsx index cf0c87400dd97..7f1456110934b 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.test.tsx @@ -35,6 +35,7 @@ describe('FilterScope', () => { const mockedProps = { filterId: 'DefaultFilterId', restoreFilter: jest.fn(), + setErroredFilters: jest.fn(), parentFilters: [], save, removedFilters: {}, 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 a4caf6bf89faf..5bc84ebbae7b6 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx @@ -268,6 +268,7 @@ export interface FiltersConfigFormProps { restoreFilter: (filterId: string) => void; form: FormInstance; parentFilters: { id: string; title: string }[]; + setErroredFilters: (f: (filters: string[]) => string[]) => void; } const FILTERS_WITH_ADHOC_FILTERS = ['filter_select', 'filter_range']; @@ -304,6 +305,7 @@ const FiltersConfigForm = ( restoreFilter, form, parentFilters, + setErroredFilters, }: FiltersConfigFormProps, ref: React.RefObject, ) => { @@ -515,7 +517,10 @@ const FiltersConfigForm = ( value: true, }, ]); - }, [form]); + setErroredFilters((prevErroredFilters: string[]) => [ + ...prevErroredFilters.filter((f: string) => f !== filterId), + ]); + }, [filterId, form, setErroredFilters]); const updateFormValues = useCallback( (values: any) => { diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx index 5dbba2fc79a81..3a19f10e04d28 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx @@ -16,7 +16,13 @@ * specific language governing permissions and limitations * under the License. */ -import React, { useCallback, useMemo, useState, useRef } from 'react'; +import React, { + useCallback, + useEffect, + useMemo, + useState, + useRef, +} from 'react'; import { uniq, debounce } from 'lodash'; import { t, styled } from '@superset-ui/core'; import { SLOW_DEBOUNCE } from 'src/constants'; @@ -126,6 +132,7 @@ export function FiltersConfigModal({ const [currentFilterId, setCurrentFilterId] = useState( initialCurrentFilterId, ); + const [erroredFilters, setErroredFilters] = useState([]); // the form values are managed by the antd form, but we copy them to here // so that we can display them (e.g. filter titles in the tab headers) @@ -225,6 +232,7 @@ export function FiltersConfigModal({ filterIds, removedFilters, setCurrentFilterId, + setErroredFilters, ); if (values) { @@ -259,6 +267,17 @@ export function FiltersConfigModal({ const onValuesChange = useMemo( () => debounce((changes: any, values: NativeFiltersForm) => { + const changedFilters: string[] = []; + Object.keys(changes.filters).forEach(filterId => { + changedFilters.push(filterId); + }); + if (changedFilters.length) { + setErroredFilters(prevErroredFilters => [ + ...prevErroredFilters.filter( + filterId => !changedFilters.find(f => f === filterId), + ), + ]); + } if ( changes.filters && Object.values(changes.filters).some( @@ -273,6 +292,16 @@ export function FiltersConfigModal({ [], ); + useEffect(() => { + setErroredFilters(prevErroredFilters => [ + ...prevErroredFilters.filter(filterId => !removedFilters[filterId]), + ]); + }, [removedFilters]); + + useEffect(() => { + console.log('erroredFilters', erroredFilters); + }, [erroredFilters]); + return ( )} diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts index a6698d2b84ebe..4491f47eacd93 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts @@ -19,6 +19,7 @@ import { FormInstance } from 'antd/lib/form'; import shortid from 'shortid'; import { getInitialDataMask } from 'src/dataMask/reducer'; +import { t } from '@superset-ui/core'; import { FilterRemoval, NativeFiltersForm } from './types'; import { Filter, FilterConfiguration, Target } from '../types'; @@ -31,6 +32,7 @@ export const validateForm = async ( filterIds: string[], removedFilters: Record, setCurrentFilterId: Function, + setErroredFilters: Function, ) => { const addValidationError = ( filterId: string, @@ -43,6 +45,12 @@ export const validateForm = async ( }; form.setFields([fieldError]); setCurrentFilterId(filterId); + setErroredFilters((prevErroredFilters: string[]) => { + if (!prevErroredFilters.includes(filterId)) { + return [...prevErroredFilters, filterId]; + } + return prevErroredFilters; + }); }; try { @@ -66,7 +74,7 @@ export const validateForm = async ( addValidationError( filterId, 'parentFilter', - 'Cannot create cyclic hierarchy', + t('Cannot create cyclic hierarchy'), ); return false; } @@ -103,9 +111,20 @@ export const validateForm = async ( field => field.name[0] === 'filters', ); if (filterError) { - setCurrentFilterId(filterError.name[1]); + const filterId = filterError.name[1]; + setCurrentFilterId(filterId); } } + error.errorFields.forEach((err: { name: string[] }) => { + const filterId = err.name[1]; + setErroredFilters((prevErroredFilters: string[]) => { + if (!prevErroredFilters.includes(filterId)) { + return [...prevErroredFilters, filterId]; + } + return prevErroredFilters; + }); + }); + return null; } }; From 582e508a0c95b44702de9f041c3688a7e7f7dcb8 Mon Sep 17 00:00:00 2001 From: geido Date: Fri, 1 Oct 2021 20:26:26 +0000 Subject: [PATCH 2/5] Clean up --- .../FiltersConfigModal/FilterTabs.tsx | 4 +++- .../FiltersConfigModal/FiltersConfigModal.tsx | 18 +----------------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx index 8a1db12d9acb5..778d59c57351c 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx @@ -240,7 +240,9 @@ const FilterTabs: FC = ({ } > - {showErroredFilter && } + {!removedFilters[id] && showErroredFilter && ( + + )} {removedFilters[id] ? t('(Removed)') : filterName} {removedFilters[id] && ( diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx index 3a19f10e04d28..3e4163c461759 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx @@ -16,13 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import React, { - useCallback, - useEffect, - useMemo, - useState, - useRef, -} from 'react'; +import React, { useCallback, useMemo, useState, useRef } from 'react'; import { uniq, debounce } from 'lodash'; import { t, styled } from '@superset-ui/core'; import { SLOW_DEBOUNCE } from 'src/constants'; @@ -292,16 +286,6 @@ export function FiltersConfigModal({ [], ); - useEffect(() => { - setErroredFilters(prevErroredFilters => [ - ...prevErroredFilters.filter(filterId => !removedFilters[filterId]), - ]); - }, [removedFilters]); - - useEffect(() => { - console.log('erroredFilters', erroredFilters); - }, [erroredFilters]); - return ( Date: Mon, 4 Oct 2021 12:50:28 +0000 Subject: [PATCH 3/5] Handle errors on the fly --- .../FiltersConfigForm/FiltersConfigForm.tsx | 541 +++++++++--------- .../FiltersConfigModal/FiltersConfigModal.tsx | 49 +- .../nativeFilters/FiltersConfigModal/utils.ts | 21 +- 3 files changed, 324 insertions(+), 287 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 5bc84ebbae7b6..6ec9f695e6b12 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx @@ -517,10 +517,7 @@ const FiltersConfigForm = ( value: true, }, ]); - setErroredFilters((prevErroredFilters: string[]) => [ - ...prevErroredFilters.filter((f: string) => f !== filterId), - ]); - }, [filterId, form, setErroredFilters]); + }, [form]); const updateFormValues = useCallback( (values: any) => { @@ -857,82 +854,104 @@ const FiltersConfigForm = ( hidden initialValue={null} /> - { - setHasDefaultValue(value); - formChanged(); - }} - > - {!isRemoved && ( - {t('Default Value')}} - required={hasDefaultValue} - rules={[ - { - validator: () => { - if (formFilter?.defaultDataMask?.filterState?.value) { - return Promise.resolve(); - } - return Promise.reject( - new Error(t('Default value is required')), - ); - }, - }, - ]} - > - {error ? ( - - ) : showDefaultValue ? ( - - { - if ( - !isEqual( - initialDefaultValue?.filterState?.value, - dataMask?.filterState?.value, - ) - ) { - formChanged(); + + { + setHasDefaultValue(value); + formChanged(); + }} + > + {!isRemoved && ( + {t('Default Value')}} + required={hasDefaultValue} + rules={[ + { + validator: () => { + if (formFilter?.defaultDataMask?.filterState?.value) { + setErroredFilters(prevErroredFilters => { + // removes the error if this was the only errored form item + if ( + prevErroredFilters.length === 1 && + prevErroredFilters[0] === filterId + ) { + return []; + } + return prevErroredFilters.filter( + f => f !== filterId, + ); + }); + return Promise.resolve(); } - setNativeFilterFieldValues(form, filterId, { - defaultDataMask: dataMask, + // requires managing the error as the DefaultValue + // component does not use an Antdesign compatible input + setErroredFilters(prevErroredFilters => { + if (prevErroredFilters.includes(filterId)) { + return prevErroredFilters; + } + return [...prevErroredFilters, filterId]; }); - form.validateFields([ - ['filters', filterId, 'defaultDataMask'], - ]); - forceUpdate(); - }} - hasDefaultValue={hasDefaultValue} - filterId={filterId} - hasDataset={hasDataset} - form={form} - formData={newFormData} - enableNoResults={enableNoResults} + return Promise.reject( + new Error(t('Default value is required')), + ); + }, + }, + ]} + > + {error ? ( + - {hasDataset && datasetId && ( - - refreshHandler(true)} /> - - )} - - ) : ( - t('Fill all required fields to enable "Default Value"') - )} - - )} - + ) : showDefaultValue ? ( + + { + if ( + !isEqual( + initialDefaultValue?.filterState?.value, + dataMask?.filterState?.value, + ) + ) { + formChanged(); + } + 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)) .map(key => controlItems[key].element)} @@ -944,213 +963,221 @@ const FiltersConfigForm = ( key={FilterPanels.advanced.key} > {isCascadingFilter && ( - { - formChanged(); - if (checked) { - // execute after render - setTimeout( - () => - form.validateFields([ - ['filters', filterId, 'parentFilter'], - ]), - 0, - ); - } - }} + - {t('Parent filter')}} - initialValue={parentFilter} - normalize={value => (value ? { value } : undefined)} - data-test="parent-filter-input" - required - rules={[ - { - required: true, - message: t('Parent filter is required'), - }, - ]} + { + formChanged(); + if (checked) { + // execute after render + setTimeout( + () => + form.validateFields([ + ['filters', filterId, 'parentFilter'], + ]), + 0, + ); + } + }} > - - - + {t('Parent filter')}} + initialValue={parentFilter} + normalize={value => (value ? { value } : undefined)} + data-test="parent-filter-input" + required + rules={[ + { + required: true, + message: t('Parent filter is required'), + }, + ]} + > + + + + )} {Object.keys(controlItems) .filter(key => !BASIC_CONTROL_ITEMS.includes(key)) .map(key => controlItems[key].element)} {hasDataset && hasAdditionalFilters && ( - { - formChanged(); - if (checked) { - validatePreFilter(); - } - }} - > - - c.filterable, - ) || [] - } - savedMetrics={datasetDetails?.metrics || []} - datasource={datasetDetails} - onChange={(filters: AdhocFilter[]) => { - setNativeFilterFieldValues(form, filterId, { - adhoc_filters: filters, - }); - forceUpdate(); + + { + formChanged(); + if (checked) { validatePreFilter(); - }} - label={ - - {t('Pre-filter')} - {!hasTimeRange && } - } - /> - - {showTimeRangePicker && ( - {t('Time range')}} - initialValue={filterToEdit?.time_range || 'No filter'} - required={!hasAdhoc} + }} + > + - { + c.filterable, + ) || [] + } + savedMetrics={datasetDetails?.metrics || []} + datasource={datasetDetails} + onChange={(filters: AdhocFilter[]) => { setNativeFilterFieldValues(form, filterId, { - time_range: timeRange, + adhoc_filters: filters, }); forceUpdate(); validatePreFilter(); }} + label={ + + {t('Pre-filter')} + {!hasTimeRange && } + + } /> - - )} - {hasTimeRange && ( - - {t('Time column')}  - - - } - initialValue={filterToEdit?.granularity_sqla} - > - !!column.is_dttm} - datasetId={datasetId} - onChange={column => { - // We need reset default value when when column changed - setNativeFilterFieldValues(form, filterId, { - granularity_sqla: column, - }); - forceUpdate(); - }} - /> - - )} - + + {showTimeRangePicker && ( + {t('Time range')}} + initialValue={filterToEdit?.time_range || 'No filter'} + required={!hasAdhoc} + rules={[ + { + validator: preFilterValidator, + }, + ]} + > + { + setNativeFilterFieldValues(form, filterId, { + time_range: timeRange, + }); + forceUpdate(); + validatePreFilter(); + }} + /> + + )} + {hasTimeRange && ( + + {t('Time column')}  + + + } + initialValue={filterToEdit?.granularity_sqla} + > + !!column.is_dttm} + datasetId={datasetId} + onChange={column => { + // We need reset default value when when column changed + setNativeFilterFieldValues(form, filterId, { + granularity_sqla: column, + }); + forceUpdate(); + }} + /> + + )} + + )} {formFilter?.filterType !== 'filter_range' && ( - { - onSortChanged(checked || undefined); - formChanged(); - }} - initialValue={hasSorting} - > - {t('Sort type')}} + + { + onSortChanged(checked || undefined); + formChanged(); + }} > - { - onSortChanged(value.target.value); - }} - > - {t('Sort ascending')} - {t('Sort descending')} - - - {hasMetrics && ( - - {t('Sort Metric')}  - - - } - data-test="field-input" + {t('Sort type')}} > - ({ + value: metric.metric_name, + label: metric.verbose_name ?? metric.metric_name, + }))} + onChange={value => { + if (value !== undefined) { + setNativeFilterFieldValues(form, filterId, { + sortMetric: value, + }); + forceUpdate(); + } + }} + /> + + )} + + )} )} diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx index 3e4163c461759..96fb3bc93a87b 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx @@ -17,7 +17,7 @@ * under the License. */ import React, { useCallback, useMemo, useState, useRef } from 'react'; -import { uniq, debounce } from 'lodash'; +import { uniq, debounce, isEqual, sortBy } from 'lodash'; import { t, styled } from '@superset-ui/core'; import { SLOW_DEBOUNCE } from 'src/constants'; import { Form } from 'src/common/components'; @@ -261,29 +261,36 @@ export function FiltersConfigModal({ const onValuesChange = useMemo( () => debounce((changes: any, values: NativeFiltersForm) => { - const changedFilters: string[] = []; - Object.keys(changes.filters).forEach(filterId => { - changedFilters.push(filterId); - }); - if (changedFilters.length) { - setErroredFilters(prevErroredFilters => [ - ...prevErroredFilters.filter( - filterId => !changedFilters.find(f => f === filterId), - ), - ]); - } - if ( - changes.filters && - Object.values(changes.filters).some( - (filter: any) => filter.name != null, - ) - ) { - // we only need to set this if a name changed - setFormValues(values); + if (changes.filters) { + const formValidationFields = form.getFieldsError(); + const newErroredFilters: string[] = []; + formValidationFields.forEach(field => { + if (field.errors.length > 0) { + const fieldName = field.name[1] as string; + newErroredFilters.push(fieldName); + } + }); + // adds or reset errored filters + if ( + (erroredFilters.length && !newErroredFilters.length) || + (newErroredFilters.length && + !isEqual(sortBy(newErroredFilters), sortBy(erroredFilters))) + ) { + setErroredFilters(newErroredFilters); + } + + if ( + Object.values(changes.filters).some( + (filter: any) => filter.name != null, + ) + ) { + // we only need to set this if a name changed + setFormValues(values); + } } setSaveAlertVisible(false); }, SLOW_DEBOUNCE), - [], + [erroredFilters, form], ); return ( diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts index 4491f47eacd93..bcb1741cd81d9 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts @@ -46,10 +46,10 @@ export const validateForm = async ( form.setFields([fieldError]); setCurrentFilterId(filterId); setErroredFilters((prevErroredFilters: string[]) => { - if (!prevErroredFilters.includes(filterId)) { - return [...prevErroredFilters, filterId]; + if (prevErroredFilters.includes(filterId)) { + return prevErroredFilters; } - return prevErroredFilters; + return [...prevErroredFilters, filterId]; }); }; @@ -115,16 +115,19 @@ export const validateForm = async ( setCurrentFilterId(filterId); } } + + const erroredFilters: string[] = []; error.errorFields.forEach((err: { name: string[] }) => { const filterId = err.name[1]; - setErroredFilters((prevErroredFilters: string[]) => { - if (!prevErroredFilters.includes(filterId)) { - return [...prevErroredFilters, filterId]; - } - return prevErroredFilters; - }); + erroredFilters.push(filterId); }); + if (errorFields.length) { + setErroredFilters((prevErroredFilters: string[]) => [ + ...new Set([...prevErroredFilters, ...erroredFilters]), + ]); + } + return null; } }; From b24e39ecd2b1d20b8d801d82a9395908b9e0ff89 Mon Sep 17 00:00:00 2001 From: geido Date: Mon, 4 Oct 2021 13:38:48 +0000 Subject: [PATCH 4/5] Implement handleErroredFilters --- .../FiltersConfigForm/FiltersConfigForm.tsx | 16 +++--- .../FiltersConfigModal/FiltersConfigModal.tsx | 49 ++++++++++++------- .../nativeFilters/FiltersConfigModal/utils.ts | 20 -------- 3 files changed, 38 insertions(+), 47 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 6ec9f695e6b12..18f10c8cd962f 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx @@ -877,22 +877,22 @@ const FiltersConfigForm = ( { validator: () => { if (formFilter?.defaultDataMask?.filterState?.value) { + // requires managing the error as the DefaultValue + // component does not use an Antdesign compatible input + const formValidationFields = form.getFieldsError(); setErroredFilters(prevErroredFilters => { - // removes the error if this was the only errored form item if ( - prevErroredFilters.length === 1 && - prevErroredFilters[0] === filterId + prevErroredFilters.length && + !formValidationFields.find( + f => f.errors.length > 0, + ) ) { return []; } - return prevErroredFilters.filter( - f => f !== filterId, - ); + return prevErroredFilters; }); return Promise.resolve(); } - // requires managing the error as the DefaultValue - // component does not use an Antdesign compatible input setErroredFilters(prevErroredFilters => { if (prevErroredFilters.includes(filterId)) { return prevErroredFilters; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx index 96fb3bc93a87b..ffd71e48960e2 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx @@ -218,6 +218,32 @@ export function FiltersConfigModal({ } }; + const handleErroredFilters = useCallback(() => { + // managing left pane errored filters indicators + const formValidationFields = form.getFieldsError(); + const erroredFiltersIds: string[] = []; + + formValidationFields.forEach(field => { + const filterId = field.name[1] as string; + if (field.errors.length > 0 && !erroredFiltersIds.includes(filterId)) { + erroredFiltersIds.push(filterId); + } + }); + + // no form validation issues found, resets errored filters + if (!erroredFiltersIds.length && erroredFilters.length > 0) { + setErroredFilters([]); + return; + } + // form validation issues found, sets errored filters + if ( + erroredFiltersIds.length > 0 && + !isEqual(sortBy(erroredFilters), sortBy(erroredFiltersIds)) + ) { + setErroredFilters(erroredFiltersIds); + } + }, [form, erroredFilters]); + const handleSave = async () => { const values: NativeFiltersForm | null = await validateForm( form, @@ -226,9 +252,10 @@ export function FiltersConfigModal({ filterIds, removedFilters, setCurrentFilterId, - setErroredFilters, ); + handleErroredFilters(); + if (values) { cleanDeletedParents(values); createHandleSave( @@ -262,23 +289,6 @@ export function FiltersConfigModal({ () => debounce((changes: any, values: NativeFiltersForm) => { if (changes.filters) { - const formValidationFields = form.getFieldsError(); - const newErroredFilters: string[] = []; - formValidationFields.forEach(field => { - if (field.errors.length > 0) { - const fieldName = field.name[1] as string; - newErroredFilters.push(fieldName); - } - }); - // adds or reset errored filters - if ( - (erroredFilters.length && !newErroredFilters.length) || - (newErroredFilters.length && - !isEqual(sortBy(newErroredFilters), sortBy(erroredFilters))) - ) { - setErroredFilters(newErroredFilters); - } - if ( Object.values(changes.filters).some( (filter: any) => filter.name != null, @@ -287,10 +297,11 @@ export function FiltersConfigModal({ // we only need to set this if a name changed setFormValues(values); } + handleErroredFilters(); } setSaveAlertVisible(false); }, SLOW_DEBOUNCE), - [erroredFilters, form], + [handleErroredFilters], ); return ( diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts index bcb1741cd81d9..0e52e19f1eaed 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts @@ -32,7 +32,6 @@ export const validateForm = async ( filterIds: string[], removedFilters: Record, setCurrentFilterId: Function, - setErroredFilters: Function, ) => { const addValidationError = ( filterId: string, @@ -45,12 +44,6 @@ export const validateForm = async ( }; form.setFields([fieldError]); setCurrentFilterId(filterId); - setErroredFilters((prevErroredFilters: string[]) => { - if (prevErroredFilters.includes(filterId)) { - return prevErroredFilters; - } - return [...prevErroredFilters, filterId]; - }); }; try { @@ -115,19 +108,6 @@ export const validateForm = async ( setCurrentFilterId(filterId); } } - - const erroredFilters: string[] = []; - error.errorFields.forEach((err: { name: string[] }) => { - const filterId = err.name[1]; - erroredFilters.push(filterId); - }); - - if (errorFields.length) { - setErroredFilters((prevErroredFilters: string[]) => [ - ...new Set([...prevErroredFilters, ...erroredFilters]), - ]); - } - return null; } }; From 39f0906d49d49521c850796b0c5ec4840fed282a Mon Sep 17 00:00:00 2001 From: geido Date: Tue, 5 Oct 2021 14:22:45 +0000 Subject: [PATCH 5/5] Reset errors --- .../FiltersConfigModal/FilterTabs.tsx | 19 ++++++++++++------- .../FiltersConfigModal/FiltersConfigModal.tsx | 7 ++++--- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx index 778d59c57351c..925e1dfa579b5 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx @@ -35,7 +35,7 @@ export const StyledSpan = styled.span` `; export const StyledFilterTitle = styled.span` - width: ${FILTER_WIDTH}px; + width: 100%; white-space: normal; color: ${({ theme }) => theme.colors.grayscale.dark1}; `; @@ -61,6 +61,7 @@ export const FilterTabTitle = styled.span` display: flex; flex-direction: row; justify-content: space-between; + align-items: center; @keyframes tabTitleRemovalAnimation { 0%, @@ -80,12 +81,18 @@ export const FilterTabTitle = styled.span` animation-duration: ${REMOVAL_DELAY_SECS}s; } - &.errored > span, - &.errored > span > span { + &.errored > span { color: ${({ theme }) => theme.colors.error.base}; } `; +const StyledWarning = styled(Icons.Warning)` + color: ${({ theme }) => theme.colors.error.base}; + &.anticon { + margin-right: 0; + } +`; + const FilterTabsContainer = styled(LineEditableTabs)` ${({ theme }) => ` height: 100%; @@ -226,7 +233,7 @@ const FilterTabs: FC = ({ > {filterIds.map(id => { const showErroredFilter = erroredFilters.includes(id); - const filterName = getFilterTitle(id) || t('[untitled]'); + const filterName = getFilterTitle(id); return ( = ({ } > - {!removedFilters[id] && showErroredFilter && ( - - )} {removedFilters[id] ? t('(Removed)') : filterName} + {!removedFilters[id] && showErroredFilter && } {removedFilters[id] && ( - formValues.filters[id]?.name ?? - filterConfigMap[id]?.name ?? - t('New filter'); + formValues.filters[id]?.name || + filterConfigMap[id]?.name || + t('[untitled]'); const getParentFilters = (id: string) => filterIds