feat(native-filters): Set default scope by filters' and charts' datasets (#15302)

* feat(native-filters): Set default scope by filter's and charts datasets

* Fix undefined error

* Use JSON.stringify in dependency array

* Fix lint issue

* Lock scope after switching radio buttons

* Fix weird eslint issue

* Change prop names

* Implement useComponentDidUpdate

* Fix lint

* Refactor useComponentDidUpdate

* Remove screen.debug()

superset-frontend/spec/javascripts/dashboard/util/getFormDataWithExtraFilters_spec.ts

@@ -48,6 +48,7 @@ describe('getFormDataWithExtraFilters', () => {
           val: ['United States'],
         },
       ],
+      datasource: '123',
     },
   };
   const mockArgs: GetFormDataWithExtraFiltersArguments = {

superset-frontend/src/common/hooks/useComponentDidUpdate/index.ts

@@ -0,0 +1,20 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+export * from './useComponentDidUpdate';

superset-frontend/src/common/hooks/useComponentDidUpdate/useComponentDidUpdate.test.ts

@@ -0,0 +1,31 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { renderHook } from '@testing-library/react-hooks';
+import { useComponentDidUpdate } from './useComponentDidUpdate';
+
+test('the effect should not be executed on the first render', () => {
+  const effect = jest.fn();
+  const hook = renderHook(props => useComponentDidUpdate(props.effect), {
+    initialProps: { effect },
+  });
+  expect(effect).toBeCalledTimes(0);
+  const changedEffect = jest.fn();
+  hook.rerender({ effect: changedEffect });
+  expect(changedEffect).toBeCalledTimes(1);
+});

superset-frontend/src/common/hooks/useComponentDidUpdate/useComponentDidUpdate.ts

@@ -0,0 +1,31 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { EffectCallback, useEffect, useRef } from 'react';
+
+export const useComponentDidUpdate = (effect: EffectCallback) => {
+  const isMountedRef = useRef(false);
+  useEffect(() => {
+    if (isMountedRef.current) {
+      effect();
+    } else {
+      isMountedRef.current = true;
+    }
+  }, [effect]);
+};

superset-frontend/src/dashboard/components/CrossFilterScopingModal/CrossFilterScopingForm/CrossFilterScopingForm.test.tsx

@@ -43,9 +43,9 @@ test('Should send correct props', () => {
   expect(FilterScope).toHaveBeenCalledWith(
     expect.objectContaining({
       chartId: 123,
-      scope: 'Scope',
+      filterScope: 'Scope',
-      formScope: 'scope',
+      formFilterScope: 'scope',
-      formScoping: 'scoping',
+      formScopingType: 'scoping',
     }),
     {},
   );

superset-frontend/src/dashboard/components/CrossFilterScopingModal/CrossFilterScopingForm/index.tsx

@@ -45,11 +45,11 @@ const CrossFilterScopingForm: FC<CrossFilterScopingFormProps> = ({
           ...values,
         });
       }}
-      scope={scope}
+      filterScope={scope}
       chartId={chartId}
-      formScope={formScope}
+      formFilterScope={formScope}
       forceUpdate={forceUpdate}
-      formScoping={formScoping}
+      formScopingType={formScoping}
     />
   );
 };

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.tsx

@@ -17,23 +17,25 @@
  * under the License.
  */
 
-import React, { FC } from 'react';
+import React, { FC, useCallback, useState } from 'react';
 import { t, styled } from '@superset-ui/core';
 import { Radio } from 'src/components/Radio';
 import { Form, Typography } from 'src/common/components';
+import { useComponentDidUpdate } from 'src/common/hooks/useComponentDidUpdate/useComponentDidUpdate';
 import { Scope } from '../../../types';
-import { Scoping } from './types';
+import { ScopingType } from './types';
 import ScopingTree from './ScopingTree';
 import { getDefaultScopeValue, isScopingAll } from './utils';
 
 type FilterScopeProps = {
   pathToFormValue?: string[];
   updateFormValues: (values: any) => void;
-  formScope?: Scope;
+  formFilterScope?: Scope;
   forceUpdate: Function;
-  scope?: Scope;
+  filterScope?: Scope;
-  formScoping?: Scoping;
+  formScopingType?: ScopingType;
   chartId?: number;
+  initiallyExcludedCharts?: number[];
 };
 
 const Wrapper = styled.div`
@@ -50,59 +52,98 @@ const CleanFormItem = styled(Form.Item)`
 
 const FilterScope: FC<FilterScopeProps> = ({
   pathToFormValue = [],
-  formScoping,
+  formScopingType,
-  formScope,
+  formFilterScope,
   forceUpdate,
-  scope,
+  filterScope,
   updateFormValues,
   chartId,
+  initiallyExcludedCharts,
 }) => {
-  const initialScope = scope || getDefaultScopeValue(chartId);
+  const [initialFilterScope] = useState(
-  const initialScoping = isScopingAll(initialScope, chartId)
+    filterScope || getDefaultScopeValue(chartId, initiallyExcludedCharts),
-    ? Scoping.all
+  );
-    : Scoping.specific;
+  const [initialScopingType] = useState(
+    isScopingAll(initialFilterScope, chartId)
+      ? ScopingType.all
+      : ScopingType.specific,
+  );
+  const [hasScopeBeenModified, setHasScopeBeenModified] = useState(
+    !!filterScope,
+  );
+
+  const onUpdateFormValues = useCallback(
+    (formValues: any) => {
+      updateFormValues(formValues);
+      setHasScopeBeenModified(true);
+    },
+    [updateFormValues],
+  );
+
+  const updateScopes = useCallback(() => {
+    if (filterScope || hasScopeBeenModified) {
+      return;
+    }
+
+    const newScope = getDefaultScopeValue(chartId, initiallyExcludedCharts);
+    updateFormValues({
+      scope: newScope,
+      scoping: isScopingAll(newScope, chartId)
+        ? ScopingType.all
+        : ScopingType.specific,
+    });
+  }, [
+    chartId,
+    filterScope,
+    hasScopeBeenModified,
+    initiallyExcludedCharts,
+    updateFormValues,
+  ]);
+  useComponentDidUpdate(updateScopes);
 
   return (
     <Wrapper>
       <CleanFormItem
         name={[...pathToFormValue, 'scoping']}
-        initialValue={initialScoping}
+        initialValue={initialScopingType}
       >
         <Radio.Group
           onChange={({ target: { value } }) => {
-            if (value === Scoping.all) {
+            if (value === ScopingType.all) {
               const scope = getDefaultScopeValue(chartId);
               updateFormValues({
                 scope,
               });
             }
+            setHasScopeBeenModified(true);
             forceUpdate();
           }}
         >
-          <Radio value={Scoping.all}>{t('Apply to all panels')}</Radio>
+          <Radio value={ScopingType.all}>{t('Apply to all panels')}</Radio>
-          <Radio value={Scoping.specific}>
+          <Radio value={ScopingType.specific}>
             {t('Apply to specific panels')}
           </Radio>
         </Radio.Group>
       </CleanFormItem>
       <Typography.Text type="secondary">
-        {(formScoping ?? initialScoping) === Scoping.specific
+        {(formScopingType ?? initialScopingType) === ScopingType.specific
           ? t('Only selected panels will be affected by this filter')
           : t('All panels with this column will be affected by this filter')}
       </Typography.Text>
-      {(formScoping ?? initialScoping) === Scoping.specific && (
+      {(formScopingType ?? initialScopingType) === ScopingType.specific && (
         <ScopingTree
-          updateFormValues={updateFormValues}
+          updateFormValues={onUpdateFormValues}
-          initialScope={initialScope}
+          initialScope={initialFilterScope}
-          formScope={formScope}
+          formScope={formFilterScope}
           forceUpdate={forceUpdate}
           chartId={chartId}
+          initiallyExcludedCharts={initiallyExcludedCharts}
         />
       )}
       <CleanFormItem
         name={[...pathToFormValue, 'scope']}
         hidden
-        initialValue={initialScope}
+        initialValue={initialFilterScope}
       />
     </Wrapper>
   );

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/ScopingTree.tsx

@@ -20,6 +20,8 @@
 import React, { FC, useMemo, useState } from 'react';
 import { Tree } from 'src/common/components';
 import { DASHBOARD_ROOT_ID } from 'src/dashboard/util/constants';
+import { Tooltip } from 'src/components/Tooltip';
+import Icons from 'src/components/Icons';
 import { useFilterScopeTree } from './state';
 import { findFilterScope, getTreeCheckedItems } from './utils';
 import { Scope } from '../../../types';
@@ -30,6 +32,26 @@ type ScopingTreeProps = {
   formScope?: Scope;
   initialScope: Scope;
   chartId?: number;
+  initiallyExcludedCharts?: number[];
+};
+
+const buildTreeLeafTitle = (
+  label: string,
+  hasTooltip: boolean,
+  tooltipTitle?: string,
+) => {
+  let title = <span>{label}</span>;
+  if (hasTooltip) {
+    title = (
+      <>
+        {title}&nbsp;
+        <Tooltip title={tooltipTitle}>
+          <Icons.Info iconSize="m" />
+        </Tooltip>
+      </>
+    );
+  }
+  return title;
 };
 
 const ScopingTree: FC<ScopingTreeProps> = ({
@@ -38,12 +60,17 @@ const ScopingTree: FC<ScopingTreeProps> = ({
   forceUpdate,
   updateFormValues,
   chartId,
+  initiallyExcludedCharts = [],
 }) => {
   const [expandedKeys, setExpandedKeys] = useState<string[]>([
     DASHBOARD_ROOT_ID,
   ]);
 
-  const { treeData, layout } = useFilterScopeTree(chartId);
+  const { treeData, layout } = useFilterScopeTree(
+    chartId,
+    initiallyExcludedCharts,
+    buildTreeLeafTitle,
+  );
   const [autoExpandParent, setAutoExpandParent] = useState<boolean>(true);
 
   const handleExpand = (expandedKeys: string[]) => {

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/state.ts

@@ -25,12 +25,14 @@ import {
   CHART_TYPE,
   DASHBOARD_ROOT_TYPE,
 } from 'src/dashboard/util/componentTypes';
-import { TreeItem } from './types';
+import { BuildTreeLeafTitle, TreeItem } from './types';
 import { buildTree } from './utils';
 
 // eslint-disable-next-line import/prefer-default-export
 export function useFilterScopeTree(
   currentChartId?: number,
+  initiallyExcludedCharts: number[] = [],
+  buildTreeLeafTitle: BuildTreeLeafTitle = label => label,
 ): {
   treeData: [TreeItem];
   layout: Layout;
@@ -61,8 +63,16 @@ export function useFilterScopeTree(
   );
 
   useMemo(() => {
-    buildTree(layout[DASHBOARD_ROOT_ID], tree, layout, charts, validNodes);
+    buildTree(
-  }, [charts, layout, tree]);
+      layout[DASHBOARD_ROOT_ID],
+      tree,
+      layout,
+      charts,
+      validNodes,
+      initiallyExcludedCharts,
+      buildTreeLeafTitle,
+    );
+  }, [layout, tree, charts, initiallyExcludedCharts, buildTreeLeafTitle]);
 
   return { treeData: [tree], layout };
 }

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/types.ts

@@ -17,7 +17,9 @@
  * under the License.
  */
 
-export enum Scoping {
+import { ReactNode } from 'react';
+
+export enum ScopingType {
   all,
   specific,
 }
@@ -26,5 +28,11 @@ export enum Scoping {
 export type TreeItem = {
   children: TreeItem[];
   key: string;
-  title: string;
+  title: ReactNode;
 };
+
+export type BuildTreeLeafTitle = (
+  label: string,
+  hasTooltip: boolean,
+  tooltipTitle?: string,
+) => ReactNode;

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.ts

@@ -23,7 +23,8 @@ import {
   TAB_TYPE,
 } from 'src/dashboard/util/componentTypes';
 import { DASHBOARD_ROOT_ID } from 'src/dashboard/util/constants';
-import { TreeItem } from './types';
+import { t } from '@superset-ui/core';
+import { BuildTreeLeafTitle, TreeItem } from './types';
 import { Scope } from '../../../types';
 
 export const isShowTypeInTree = ({ type, meta }: LayoutItem, charts?: Charts) =>
@@ -36,6 +37,8 @@ export const buildTree = (
   layout: Layout,
   charts: Charts,
   validNodes: string[],
+  initiallyExcludedCharts: number[],
+  buildTreeLeafTitle: BuildTreeLeafTitle,
 ) => {
   let itemToPass: TreeItem = treeItem;
   if (
@@ -43,20 +46,35 @@ export const buildTree = (
     node.type !== DASHBOARD_ROOT_TYPE &&
     validNodes.includes(node.id)
   ) {
-    const currentTreeItem = {
+    const title = buildTreeLeafTitle(
-      key: node.id,
+      node.meta.sliceNameOverride ||
-      title:
-        node.meta.sliceNameOverride ||
         node.meta.sliceName ||
         node.meta.text ||
         node.id.toString(),
+      initiallyExcludedCharts.includes(node.meta?.chartId),
+      t(
+        "This chart might be incompatible with the filter (datasets don't match)",
+      ),
+    );
+
+    const currentTreeItem = {
+      key: node.id,
+      title,
       children: [],
     };
     treeItem.children.push(currentTreeItem);
     itemToPass = currentTreeItem;
   }
   node.children.forEach(child =>
-    buildTree(layout[child], itemToPass, layout, charts, validNodes),
+    buildTree(
+      layout[child],
+      itemToPass,
+      layout,
+      charts,
+      validNodes,
+      initiallyExcludedCharts,
+      buildTreeLeafTitle,
+    ),
   );
 };
 
@@ -148,9 +166,14 @@ export const findFilterScope = (
   };
 };
 
-export const getDefaultScopeValue = (chartId?: number): Scope => ({
+export const getDefaultScopeValue = (
+  chartId?: number,
+  initiallyExcludedCharts: number[] = [],
+): Scope => ({
   rootPath: [DASHBOARD_ROOT_ID],
-  excluded: chartId ? [chartId] : [],
+  excluded: chartId
+    ? [chartId, ...initiallyExcludedCharts]
+    : initiallyExcludedCharts,
 });
 
 export const isScopingAll = (scope: Scope, chartId?: number) =>

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx

@@ -64,6 +64,12 @@ import Tabs from 'src/components/Tabs';
 import Icons from 'src/components/Icons';
 import { Tooltip } from 'src/components/Tooltip';
 import BasicErrorAlert from 'src/components/ErrorMessage/BasicErrorAlert';
+import {
+  Chart,
+  ChartsState,
+  DatasourcesState,
+  RootState,
+} from 'src/dashboard/types';
 import { ColumnSelect } from './ColumnSelect';
 import { NativeFiltersForm } from '../types';
 import {
@@ -324,9 +330,10 @@ const FiltersConfigForm = (
     )
     .map(([key]) => key);
 
-  const loadedDatasets = useSelector<any, DatasourceMeta>(
+  const loadedDatasets = useSelector<RootState, DatasourcesState>(
     ({ datasources }) => datasources,
   );
+  const charts = useSelector<RootState, ChartsState>(({ charts }) => charts);
 
   const doLoadedDatasetsHaveTemporalColumns = useMemo(
     () =>
@@ -349,9 +356,9 @@ const FiltersConfigForm = (
     ?.datasourceCount;
   const hasColumn =
     hasDataset && !FILTERS_WITHOUT_COLUMN.includes(formFilter?.filterType);
+  const nativeFilterItem = nativeFilterItems[formFilter?.filterType] ?? {};
   // @ts-ignore
-  const enableNoResults = !!nativeFilterItems[formFilter?.filterType]?.value
+  const enableNoResults = !!nativeFilterItem.value?.enableNoResults;
-    ?.enableNoResults;
   const datasetId = formFilter?.dataset?.value;
 
   useEffect(() => {
@@ -517,6 +524,11 @@ const FiltersConfigForm = (
     [],
   );
 
+  const updateFormValues = useCallback(
+    (values: any) => setNativeFilterFieldValues(form, filterId, values),
+    [filterId, form],
+  );
+
   const parentFilterOptions = parentFilters.map(filter => ({
     value: filter.id,
     label: filter.title,
@@ -598,6 +610,28 @@ const FiltersConfigForm = (
     setActiveFilterPanelKey(activeFilterPanelKey);
   }, [hasCheckedAdvancedControl]);
 
+  const initiallyExcludedCharts = useMemo(() => {
+    const excluded: number[] = [];
+    if (formFilter?.dataset?.value === undefined) {
+      return [];
+    }
+
+    Object.values(charts).forEach((chart: Chart) => {
+      const chartDatasetUid = chart.formData?.datasource;
+      if (chartDatasetUid === undefined) {
+        return;
+      }
+      if (loadedDatasets[chartDatasetUid]?.id !== formFilter?.dataset?.value) {
+        excluded.push(chart.id);
+      }
+    });
+    return excluded;
+  }, [
+    JSON.stringify(charts),
+    formFilter?.dataset?.value,
+    JSON.stringify(loadedDatasets),
+  ]);
+
   if (removed) {
     return <RemovedFilter onClick={() => restoreFilter(filterId)} />;
   }
@@ -1053,14 +1087,13 @@ const FiltersConfigForm = (
         forceRender
       >
         <FilterScope
-          updateFormValues={(values: any) =>
+          updateFormValues={updateFormValues}
-            setNativeFilterFieldValues(form, filterId, values)
-          }
           pathToFormValue={['filters', filterId]}
           forceUpdate={forceUpdate}
-          scope={filterToEdit?.scope}
+          filterScope={filterToEdit?.scope}
-          formScope={formFilter?.scope}
+          formFilterScope={formFilter?.scope}
-          formScoping={formFilter?.scoping}
+          formScopingType={formFilter?.scoping}
+          initiallyExcludedCharts={initiallyExcludedCharts}
         />
       </TabPane>
     </StyledTabs>

superset-frontend/src/dashboard/types.ts

@@ -16,7 +16,13 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { ChartProps, ExtraFormData, JsonObject } from '@superset-ui/core';
+import {
+  ChartProps,
+  ExtraFormData,
+  GenericDataType,
+  JsonObject,
+} from '@superset-ui/core';
+import { DatasourceMeta } from '@superset-ui/chart-controls';
 import { chart } from 'src/chart/chartReducer';
 import componentTypes from 'src/dashboard/util/componentTypes';
 import { DataMaskStateWithId } from '../dataMask/types';
@@ -38,6 +44,7 @@ export interface ChartQueryPayload extends Partial<ChartReducerInitialState> {
 export type Chart = ChartState & {
   formData: {
     viz_type: string;
+    datasource: string;
   };
 };
 
@@ -60,10 +67,16 @@ export type DashboardInfo = {
 };
 
 export type ChartsState = { [key: string]: Chart };
+export type DatasourcesState = {
+  [key: string]: DatasourceMeta & {
+    column_types: GenericDataType[];
+    table_name: string;
+  };
+};
 
 /** Root state of redux */
 export type RootState = {
-  datasources: JsonObject;
+  datasources: DatasourcesState;
   sliceEntities: JsonObject;
   charts: ChartsState;
   dashboardLayout: DashboardLayoutState;