diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx index a5f1dd03fb8b8..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%, @@ -79,6 +80,17 @@ export const FilterTabTitle = styled.span` animation-name: tabTitleRemovalAnimation; animation-duration: ${REMOVAL_DELAY_SECS}s; } + + &.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)` @@ -169,6 +181,7 @@ type FilterTabsProps = { currentFilterId: string; onEdit: (filterId: string, action: 'add' | 'remove') => void; filterIds: string[]; + erroredFilters: string[]; removedFilters: Record; restoreFilter: Function; children: Function; @@ -180,6 +193,7 @@ const FilterTabs: FC = ({ onChange, currentFilterId, filterIds = [], + erroredFilters = [], removedFilters = [], restoreFilter, children, @@ -217,34 +231,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); + return ( + + + {removedFilters[id] ? t('(Removed)') : filterName} + + {!removedFilters[id] && showErroredFilter && } + {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..18f10c8cd962f 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, ) => { @@ -852,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) { + // requires managing the error as the DefaultValue + // component does not use an Antdesign compatible input + const formValidationFields = form.getFieldsError(); + setErroredFilters(prevErroredFilters => { + if ( + prevErroredFilters.length && + !formValidationFields.find( + f => f.errors.length > 0, + ) + ) { + return []; + } + return prevErroredFilters; + }); + return Promise.resolve(); } - setNativeFilterFieldValues(form, filterId, { - defaultDataMask: dataMask, + 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)} @@ -939,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 5dbba2fc79a81..0709d8d9f3d4a 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'; @@ -126,6 +126,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) @@ -174,12 +175,13 @@ export function FiltersConfigModal({ setSaveAlertVisible(false); setFormValues({ filters: {} }); form.setFieldsValue({ changed: false }); + setErroredFilters([]); }; const getFilterTitle = (id: string) => - formValues.filters[id]?.name ?? - filterConfigMap[id]?.name ?? - t('New filter'); + formValues.filters[id]?.name || + filterConfigMap[id]?.name || + t('[untitled]'); const getParentFilters = (id: string) => filterIds @@ -217,6 +219,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, @@ -227,6 +255,8 @@ export function FiltersConfigModal({ setCurrentFilterId, ); + handleErroredFilters(); + if (values) { cleanDeletedParents(values); createHandleSave( @@ -259,18 +289,20 @@ export function FiltersConfigModal({ const onValuesChange = useMemo( () => debounce((changes: any, values: NativeFiltersForm) => { - 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) { + if ( + Object.values(changes.filters).some( + (filter: any) => filter.name != null, + ) + ) { + // we only need to set this if a name changed + setFormValues(values); + } + handleErroredFilters(); } setSaveAlertVisible(false); }, SLOW_DEBOUNCE), - [], + [handleErroredFilters], ); return ( @@ -303,6 +335,7 @@ export function FiltersConfigModal({ layout="vertical" > )} diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts index a6698d2b84ebe..0e52e19f1eaed 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'; @@ -66,7 +67,7 @@ export const validateForm = async ( addValidationError( filterId, 'parentFilter', - 'Cannot create cyclic hierarchy', + t('Cannot create cyclic hierarchy'), ); return false; } @@ -103,7 +104,8 @@ export const validateForm = async ( field => field.name[0] === 'filters', ); if (filterError) { - setCurrentFilterId(filterError.name[1]); + const filterId = filterError.name[1]; + setCurrentFilterId(filterId); } } return null;