fix(dashboard): Cross-filters not working properly for new dashboards (#23194)

superset-frontend/src/dashboard/actions/dashboardState.js

@@ -36,7 +36,10 @@ import {
   SAVE_TYPE_OVERWRITE,
   SAVE_TYPE_OVERWRITE_CONFIRMED,
 } from 'src/dashboard/util/constants';
-import { isCrossFiltersEnabled } from 'src/dashboard/util/crossFilters';
+import {
+  getCrossFiltersConfiguration,
+  isCrossFiltersEnabled,
+} from 'src/dashboard/util/crossFilters';
 import {
   addSuccessToast,
   addWarningToast,
@@ -277,25 +280,17 @@ export function saveDashboardRequest(data, id, saveType) {
 
     const handleChartConfiguration = () => {
       const {
+        dashboardLayout,
+        charts,
         dashboardInfo: {
           metadata: { chart_configuration = {} },
         },
       } = getState();
-      const chartConfiguration = Object.values(chart_configuration).reduce(
-        (prev, next) => {
-          // If chart removed from dashboard - remove it from metadata
-          if (
-            Object.values(layout).find(
-              layoutItem => layoutItem?.meta?.chartId === next.id,
-            )
-          ) {
-            return { ...prev, [next.id]: next };
-          }
-          return prev;
-        },
-        {},
+      return getCrossFiltersConfiguration(
+        dashboardLayout.present,
+        chart_configuration,
+        charts,
       );
-      return chartConfiguration;
     };
 
     const onCopySuccess = response => {

superset-frontend/src/dashboard/actions/hydrate.js

@@ -16,9 +16,6 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-/* eslint-disable camelcase */
-import { Behavior, getChartMetadataRegistry } from '@superset-ui/core';
-
 import { chart } from 'src/components/Chart/chartReducer';
 import { initSliceEntities } from 'src/dashboard/reducers/sliceEntities';
 import { getInitialState as getInitialNativeFilterState } from 'src/dashboard/reducers/nativeFilters';
@@ -26,7 +23,10 @@ import { applyDefaultFormData } from 'src/explore/store';
 import { buildActiveFilters } from 'src/dashboard/util/activeDashboardFilters';
 import { findPermission } from 'src/utils/findPermission';
 import { canUserEditDashboard } from 'src/dashboard/util/permissionUtils';
-import { isCrossFiltersEnabled } from 'src/dashboard/util/crossFilters';
+import {
+  getCrossFiltersConfiguration,
+  isCrossFiltersEnabled,
+} from 'src/dashboard/util/crossFilters';
 import {
   DASHBOARD_FILTER_SCOPE_GLOBAL,
   dashboardFilter,
@@ -54,7 +54,6 @@ import { ResourceStatus } from 'src/hooks/apiResources/apiResources';
 import { FeatureFlag, isFeatureEnabled } from '../../featureFlags';
 import extractUrlParams from '../util/extractUrlParams';
 import { updateColorSchema } from './dashboardInfo';
-import { getChartIdsInFilterScope } from '../util/getChartIdsInFilterScope';
 import updateComponentParentsList from '../util/updateComponentParentsList';
 import { FilterBarOrientation } from '../types';
 
@@ -124,7 +123,7 @@ export const hydrateDashboard =
 
     charts.forEach(slice => {
       const key = slice.slice_id;
-      const form_data = {
+      const formData = {
         ...slice.form_data,
         url_params: {
           ...slice.form_data.url_params,
@@ -134,7 +133,7 @@ export const hydrateDashboard =
       chartQueries[key] = {
         ...chart,
         id: key,
-        form_data: applyDefaultFormData(form_data),
+        form_data: applyDefaultFormData(formData),
       };
 
       slices[key] = {
@@ -311,54 +310,11 @@ export const hydrateDashboard =
     );
 
     if (isFeatureEnabled(FeatureFlag.DASHBOARD_CROSS_FILTERS)) {
-      // If user just added cross filter to dashboard it's not saving it scope on server,
-      // so we tweak it until user will update scope and will save it in server
-      Object.values(dashboardLayout.present).forEach(layoutItem => {
-        const chartId = layoutItem.meta?.chartId;
-        const behaviors =
-          (
-            getChartMetadataRegistry().get(
-              chartQueries[chartId]?.form_data?.viz_type,
-            ) ?? {}
-          )?.behaviors ?? [];
-
-        if (!metadata.chart_configuration) {
-          metadata.chart_configuration = {};
-        }
-        if (behaviors.includes(Behavior.INTERACTIVE_CHART)) {
-          if (!metadata.chart_configuration[chartId]) {
-            metadata.chart_configuration[chartId] = {
-              id: chartId,
-              crossFilters: {
-                scope: {
-                  rootPath: [DASHBOARD_ROOT_ID],
-                  excluded: [chartId], // By default it doesn't affects itself
-                },
-              },
-            };
-          }
-          metadata.chart_configuration[chartId].crossFilters.chartsInScope =
-            getChartIdsInFilterScope(
-              metadata.chart_configuration[chartId].crossFilters.scope,
-              chartQueries,
-              dashboardLayout.present,
-            );
-        }
-        if (
-          behaviors.includes(Behavior.INTERACTIVE_CHART) &&
-          !metadata.chart_configuration[chartId]
-        ) {
-          metadata.chart_configuration[chartId] = {
-            id: chartId,
-            crossFilters: {
-              scope: {
-                rootPath: [DASHBOARD_ROOT_ID],
-                excluded: [chartId], // By default it doesn't affects itself
-              },
-            },
-          };
-        }
-      });
+      metadata.chart_configuration = getCrossFiltersConfiguration(
+        dashboardLayout.present,
+        metadata.chart_configuration,
+        chartQueries,
+      );
     }
 
     const { roles } = user;

superset-frontend/src/dashboard/util/crossFilters.test.ts

@@ -0,0 +1,207 @@
+/**
+ * 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 sinon from 'sinon';
+import { Behavior, FeatureFlag } from '@superset-ui/core';
+import * as core from '@superset-ui/core';
+import { getCrossFiltersConfiguration } from './crossFilters';
+
+const DASHBOARD_LAYOUT = {
+  'CHART-1': {
+    children: [],
+    id: 'CHART-1',
+    meta: {
+      chartId: 1,
+      sliceName: 'Test chart 1',
+      height: 1,
+      width: 1,
+      uuid: '1',
+    },
+    parents: ['ROOT_ID', 'GRID_ID', 'ROW-6XUMf1rV76'],
+    type: 'CHART',
+  },
+  'CHART-2': {
+    children: [],
+    id: 'CHART-2',
+    meta: {
+      chartId: 2,
+      sliceName: 'Test chart 2',
+      height: 1,
+      width: 1,
+      uuid: '2',
+    },
+    parents: ['ROOT_ID', 'GRID_ID', 'ROW-6XUMf1rV76'],
+    type: 'CHART',
+  },
+};
+
+const CHARTS = {
+  '1': {
+    id: 1,
+    form_data: {
+      datasource: '2__table',
+      viz_type: 'echarts_timeseries_line',
+      slice_id: 1,
+    },
+    chartAlert: null,
+    chartStatus: 'rendered' as const,
+    chartUpdateEndTime: 0,
+    chartUpdateStartTime: 0,
+    lastRendered: 0,
+    latestQueryFormData: {},
+    sliceFormData: {
+      datasource: '2__table',
+      viz_type: 'echarts_timeseries_line',
+    },
+    queryController: null,
+    queriesResponse: [{}],
+    triggerQuery: false,
+  },
+  '2': {
+    id: 2,
+    form_data: {
+      datasource: '2__table',
+      viz_type: 'echarts_timeseries_line',
+      slice_id: 2,
+    },
+    chartAlert: null,
+    chartStatus: 'rendered' as const,
+    chartUpdateEndTime: 0,
+    chartUpdateStartTime: 0,
+    lastRendered: 0,
+    latestQueryFormData: {},
+    sliceFormData: {
+      datasource: '2__table',
+      viz_type: 'echarts_timeseries_line',
+    },
+    queryController: null,
+    queriesResponse: [{}],
+    triggerQuery: false,
+  },
+};
+
+const INITIAL_CHART_CONFIG = {
+  '1': {
+    id: 1,
+    crossFilters: {
+      scope: {
+        rootPath: ['ROOT_ID'],
+        excluded: [1],
+      },
+      chartsInScope: [2],
+    },
+  },
+};
+
+test('Generate correct cross filters configuration without initial configuration', () => {
+  // @ts-ignore
+  global.featureFlags = {
+    [FeatureFlag.DASHBOARD_CROSS_FILTERS]: true,
+  };
+  const metadataRegistryStub = sinon
+    .stub(core, 'getChartMetadataRegistry')
+    .callsFake(() => ({
+      // @ts-ignore
+      get: () => ({
+        behaviors: [Behavior.INTERACTIVE_CHART],
+      }),
+    }));
+  expect(
+    getCrossFiltersConfiguration(DASHBOARD_LAYOUT, undefined, CHARTS),
+  ).toEqual({
+    '1': {
+      id: 1,
+      crossFilters: {
+        scope: {
+          rootPath: ['ROOT_ID'],
+          excluded: [1],
+        },
+        chartsInScope: [2],
+      },
+    },
+    '2': {
+      id: 2,
+      crossFilters: {
+        scope: {
+          rootPath: ['ROOT_ID'],
+          excluded: [2],
+        },
+        chartsInScope: [1],
+      },
+    },
+  });
+  metadataRegistryStub.restore();
+});
+
+test('Generate correct cross filters configuration with initial configuration', () => {
+  // @ts-ignore
+  global.featureFlags = {
+    [FeatureFlag.DASHBOARD_CROSS_FILTERS]: true,
+  };
+  const metadataRegistryStub = sinon
+    .stub(core, 'getChartMetadataRegistry')
+    .callsFake(() => ({
+      // @ts-ignore
+      get: () => ({
+        behaviors: [Behavior.INTERACTIVE_CHART],
+      }),
+    }));
+  expect(
+    getCrossFiltersConfiguration(
+      DASHBOARD_LAYOUT,
+      INITIAL_CHART_CONFIG,
+      CHARTS,
+    ),
+  ).toEqual({
+    '1': {
+      id: 1,
+      crossFilters: {
+        scope: {
+          rootPath: ['ROOT_ID'],
+          excluded: [1],
+        },
+        chartsInScope: [2],
+      },
+    },
+    '2': {
+      id: 2,
+      crossFilters: {
+        scope: {
+          rootPath: ['ROOT_ID'],
+          excluded: [2],
+        },
+        chartsInScope: [1],
+      },
+    },
+  });
+  metadataRegistryStub.restore();
+});
+
+test('Return undefined if DASHBOARD_CROSS_FILTERS feature flag is disabled', () => {
+  // @ts-ignore
+  global.featureFlags = {
+    [FeatureFlag.DASHBOARD_CROSS_FILTERS]: false,
+  };
+  expect(
+    getCrossFiltersConfiguration(
+      DASHBOARD_LAYOUT,
+      INITIAL_CHART_CONFIG,
+      CHARTS,
+    ),
+  ).toEqual(undefined);
+});

superset-frontend/src/dashboard/util/crossFilters.ts

@@ -17,10 +17,69 @@
  * under the License.
  */
 
-import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
+import {
+  Behavior,
+  FeatureFlag,
+  getChartMetadataRegistry,
+  isDefined,
+  isFeatureEnabled,
+} from '@superset-ui/core';
+import { DASHBOARD_ROOT_ID } from './constants';
+import { getChartIdsInFilterScope } from './getChartIdsInFilterScope';
+import { ChartsState, DashboardInfo, DashboardLayout } from '../types';
 
 export const isCrossFiltersEnabled = (
   metadataCrossFiltersEnabled: boolean | undefined,
 ): boolean =>
   isFeatureEnabled(FeatureFlag.DASHBOARD_CROSS_FILTERS) &&
   (metadataCrossFiltersEnabled === undefined || metadataCrossFiltersEnabled);
+
+export const getCrossFiltersConfiguration = (
+  dashboardLayout: DashboardLayout,
+  initialConfig: DashboardInfo['metadata']['chart_configuration'] = {},
+  charts: ChartsState,
+) => {
+  if (!isFeatureEnabled(FeatureFlag.DASHBOARD_CROSS_FILTERS)) {
+    return undefined;
+  }
+  // If user just added cross filter to dashboard it's not saving it scope on server,
+  // so we tweak it until user will update scope and will save it in server
+  const chartConfiguration = {};
+  Object.values(dashboardLayout).forEach(layoutItem => {
+    const chartId = layoutItem.meta?.chartId;
+
+    if (!isDefined(chartId)) {
+      return;
+    }
+
+    const behaviors =
+      (
+        getChartMetadataRegistry().get(charts[chartId]?.form_data?.viz_type) ??
+        {}
+      )?.behaviors ?? [];
+
+    if (behaviors.includes(Behavior.INTERACTIVE_CHART)) {
+      if (initialConfig[chartId]) {
+        chartConfiguration[chartId] = initialConfig[chartId];
+      }
+      if (!chartConfiguration[chartId]) {
+        chartConfiguration[chartId] = {
+          id: chartId,
+          crossFilters: {
+            scope: {
+              rootPath: [DASHBOARD_ROOT_ID],
+              excluded: [chartId], // By default it doesn't affects itself
+            },
+          },
+        };
+      }
+      chartConfiguration[chartId].crossFilters.chartsInScope =
+        getChartIdsInFilterScope(
+          chartConfiguration[chartId].crossFilters.scope,
+          charts,
+          dashboardLayout,
+        );
+    }
+  });
+  return chartConfiguration;
+};