From a6d54b681bea0b7632ae643a43ba8c8c1ceaa2f7 Mon Sep 17 00:00:00 2001
From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com>
Date: Thu, 27 May 2021 07:37:36 -0300
Subject: [PATCH] chore: Improves the native filters UI/UX - iteration 4
 (#14854)

---
 .../FiltersConfigForm/FiltersConfigForm.tsx   | 119 ++++++++++++++++--
 .../FiltersConfigModal/FiltersConfigModal.tsx |  39 ++++--
 .../nativeFilters/FiltersConfigModal/utils.ts |  14 +--
 3 files changed, 135 insertions(+), 37 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 f200ee1860..8672d0e09d 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
@@ -32,7 +32,14 @@ import {
   Metric,
 } from '@superset-ui/chart-controls';
 import { FormInstance } from 'antd/lib/form';
-import React, { useCallback, useEffect, useState, useMemo } from 'react';
+import React, {
+  useCallback,
+  useEffect,
+  useState,
+  useMemo,
+  forwardRef,
+  useImperativeHandle,
+} from 'react';
 import { useSelector } from 'react-redux';
 import { Checkbox, Form, Input } from 'src/common/components';
 import { Select } from 'src/components/Select';
@@ -153,6 +160,12 @@ const StyledTabs = styled(Tabs)`
   }
 `;
 
+const StyledAsterisk = styled.span`
+  color: ${({ theme }) => theme.colors.error.base};
+  font-family: SimSun, sans-serif;
+  margin-right: ${({ theme }) => theme.gridUnit - 1}px;
+`;
+
 const FilterTabs = {
   configuration: {
     key: 'configuration',
@@ -199,15 +212,19 @@ const BASIC_CONTROL_ITEMS = ['enableEmptyFilter', 'multiSelect'];
  * The configuration form for a specific filter.
  * Assigns field values to `filters[filterId]` in the form.
  */
-export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
-  filterId,
-  filterToEdit,
-  removed,
-  restoreFilter,
-  form,
-  parentFilters,
-}) => {
+const FiltersConfigForm = (
+  {
+    filterId,
+    filterToEdit,
+    removed,
+    restoreFilter,
+    form,
+    parentFilters,
+  }: FiltersConfigFormProps,
+  ref: React.RefObject<any>,
+) => {
   const [metrics, setMetrics] = useState<Metric[]>([]);
+  const [activeTabKey, setActiveTabKey] = useState<string | undefined>();
   const [hasDefaultValue, setHasDefaultValue] = useState(
     !!filterToEdit?.defaultDataMask?.filterState?.value,
   );
@@ -257,6 +274,12 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
     }
   }, [datasetId, hasColumn]);
 
+  useImperativeHandle(ref, () => ({
+    changeTab(tab: 'configuration' | 'scoping') {
+      setActiveTabKey(tab);
+    },
+  }));
+
   const hasMetrics = hasColumn && !!metrics.length;
 
   const hasFilledDataset =
@@ -374,7 +397,7 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
 
   const controlItems = formFilter
     ? getControlItemsMap({
-        disabled: !showDefaultValue,
+        disabled: false,
         forceUpdate,
         form,
         filterId,
@@ -396,7 +419,12 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
 
   return (
     <>
-      <StyledTabs defaultActiveKey={FilterTabs.configuration.key} centered>
+      <StyledTabs
+        defaultActiveKey={FilterTabs.configuration.key}
+        activeKey={activeTabKey}
+        onChange={activeKey => setActiveTabKey(activeKey)}
+        centered
+      >
         <TabPane
           tab={FilterTabs.configuration.name}
           key={FilterTabs.configuration.key}
@@ -522,6 +550,23 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
                   initialValue={filterToEdit?.defaultDataMask}
                   data-test="default-input"
                   label={<StyledLabel>{t('Default Value')}</StyledLabel>}
+                  required
+                  rules={[
+                    {
+                      required: true,
+                    },
+                    {
+                      validator: (rule, value) => {
+                        const hasValue = !!value.filterState?.value;
+                        if (hasValue) {
+                          return Promise.resolve();
+                        }
+                        return Promise.reject(
+                          new Error(t('Default value is required')),
+                        );
+                      },
+                    },
+                  ]}
                 >
                   {showDefaultValue ? (
                     <DefaultValue
@@ -529,6 +574,9 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
                         setNativeFilterFieldValues(form, filterId, {
                           defaultDataMask: dataMask,
                         });
+                        form.validateFields([
+                          ['filters', filterId, 'defaultDataMask'],
+                        ]);
                         forceUpdate();
                       }}
                       filterId={filterId}
@@ -565,12 +613,31 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
                   <CollapsibleControl
                     title={t('Filter is hierarchical')}
                     checked={!!parentFilter}
+                    onChange={checked => {
+                      if (checked) {
+                        // execute after render
+                        setTimeout(
+                          () =>
+                            form.validateFields([
+                              ['filters', filterId, 'parentFilter'],
+                            ]),
+                          0,
+                        );
+                      }
+                    }}
                   >
                     <StyledRowFormItem
                       name={['filters', filterId, 'parentFilter']}
                       label={<StyledLabel>{t('Parent filter')}</StyledLabel>}
                       initialValue={parentFilter}
                       data-test="parent-filter-input"
+                      required
+                      rules={[
+                        {
+                          required: true,
+                          message: t('Parent filter is required'),
+                        },
+                      ]}
                     >
                       <Select
                         placeholder={t('None')}
@@ -590,10 +657,29 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
                       !!filterToEdit?.adhoc_filters ||
                       !!filterToEdit?.time_range
                     }
+                    onChange={checked => {
+                      if (checked) {
+                        // execute after render
+                        setTimeout(
+                          () =>
+                            form.validateFields([
+                              ['filters', filterId, 'adhoc_filters'],
+                            ]),
+                          0,
+                        );
+                      }
+                    }}
                   >
                     <StyledRowFormItem
                       name={['filters', filterId, 'adhoc_filters']}
                       initialValue={filterToEdit?.adhoc_filters}
+                      required
+                      rules={[
+                        {
+                          required: true,
+                          message: t('Adhoc filters is required'),
+                        },
+                      ]}
                     >
                       <AdhocFilterControl
                         columns={
@@ -609,7 +695,12 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
                           });
                           forceUpdate();
                         }}
-                        label={<StyledLabel>{t('Adhoc filters')}</StyledLabel>}
+                        label={
+                          <span>
+                            <StyledAsterisk>*</StyledAsterisk>
+                            <StyledLabel>{t('Adhoc filters')}</StyledLabel>
+                          </span>
+                        }
                       />
                     </StyledRowFormItem>
                     <StyledRowFormItem
@@ -720,4 +811,6 @@ export const FiltersConfigForm: React.FC<FiltersConfigFormProps> = ({
   );
 };
 
-export default FiltersConfigForm;
+export default forwardRef<typeof FiltersConfigForm, FiltersConfigFormProps>(
+  FiltersConfigForm,
+);
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
index 7516f376de..a7a4acdd22 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React, { useCallback, useMemo, useState } from 'react';
+import React, { useCallback, useMemo, useState, useRef } from 'react';
 import { uniq } from 'lodash';
 import { t, styled } from '@superset-ui/core';
 import { Form } from 'src/common/components';
@@ -27,6 +27,7 @@ import { useFilterConfigMap, useFilterConfiguration } from '../state';
 import { FilterRemoval, NativeFiltersForm } from './types';
 import { FilterConfiguration } from '../types';
 import {
+  validateForm,
   createHandleSave,
   createHandleTabEdit,
   generateFilterId,
@@ -89,6 +90,8 @@ export function FiltersConfigModal({
 }: FiltersConfigModalProps) {
   const [form] = Form.useForm<NativeFiltersForm>();
 
+  const configFormRef = useRef<any>();
+
   // the filter config from redux state, this does not change until modal is closed.
   const filterConfig = useFilterConfiguration();
   const filterConfigMap = useFilterConfigMap();
@@ -187,16 +190,29 @@ export function FiltersConfigModal({
         title: getFilterTitle(id),
       }));
 
-  const handleSave = createHandleSave(
-    form,
-    currentFilterId,
-    filterConfigMap,
-    filterIds,
-    removedFilters,
-    setCurrentFilterId,
-    resetForm,
-    onSave,
-  );
+  const handleSave = async () => {
+    const values: NativeFiltersForm | null = await validateForm(
+      form,
+      currentFilterId,
+      filterConfigMap,
+      filterIds,
+      removedFilters,
+      setCurrentFilterId,
+    );
+
+    if (values) {
+      createHandleSave(
+        filterConfigMap,
+        filterIds,
+        removedFilters,
+        resetForm,
+        onSave,
+        values,
+      )();
+    } else {
+      configFormRef.current.changeTab('configuration');
+    }
+  };
 
   const handleConfirmCancel = () => {
     resetForm();
@@ -264,6 +280,7 @@ export function FiltersConfigModal({
             >
               {(id: string) => (
                 <FiltersConfigForm
+                  ref={configFormRef}
                   form={form}
                   filterId={id}
                   filterToEdit={filterConfigMap[id]}
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts
index 968eb17672..e34d8b1287 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/utils.ts
@@ -114,25 +114,13 @@ export const validateForm = async (
 };
 
 export const createHandleSave = (
-  form: FormInstance<NativeFiltersForm>,
-  currentFilterId: string,
   filterConfigMap: Record<string, Filter>,
   filterIds: string[],
   removedFilters: Record<string, FilterRemoval>,
-  setCurrentFilterId: Function,
   resetForm: Function,
   saveForm: Function,
+  values: NativeFiltersForm,
 ) => async () => {
-  const values: NativeFiltersForm | null = await validateForm(
-    form,
-    currentFilterId,
-    filterConfigMap,
-    filterIds,
-    removedFilters,
-    setCurrentFilterId,
-  );
-  if (values === null) return;
-
   const newFilterConfig: FilterConfiguration = filterIds
     .filter(id => !removedFilters[id])
     .map(id => {