From 517a678cd7a8531ceb2f0fdf64d275ef8c928879 Mon Sep 17 00:00:00 2001
From: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
Date: Fri, 13 Aug 2021 10:02:28 +0200
Subject: [PATCH] fix(dashboard): cross filter chart highlight when filters
 badge icon clicked (#16233)

* fix(dashboard): cross filter chart highlight when filters badge icon pressed

* Fix tests

* Fix tests

* break out label logic

Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
---
 .../DetailsPanel/DetailsPanel.test.tsx        |  5 ++
 .../FiltersBadge/DetailsPanel/index.tsx       | 10 +++
 .../components/FiltersBadge/selectors.ts      | 65 +++++++++++--------
 3 files changed, 54 insertions(+), 26 deletions(-)

diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx
index 239df0f54d700..12efe2c3e5d67 100644
--- a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx
+++ b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/DetailsPanel.test.tsx
@@ -96,6 +96,7 @@ test('Should render "appliedCrossFilterIndicators"', () => {
     <DetailsPanel {...props}>
       <div data-test="details-panel-content">Content</div>
     </DetailsPanel>,
+    { useRedux: true },
   );
 
   userEvent.click(screen.getByTestId('details-panel-content'));
@@ -129,6 +130,7 @@ test('Should render "appliedIndicators"', () => {
     <DetailsPanel {...props}>
       <div data-test="details-panel-content">Content</div>
     </DetailsPanel>,
+    { useRedux: true },
   );
 
   userEvent.click(screen.getByTestId('details-panel-content'));
@@ -160,6 +162,7 @@ test('Should render "incompatibleIndicators"', () => {
     <DetailsPanel {...props}>
       <div data-test="details-panel-content">Content</div>
     </DetailsPanel>,
+    { useRedux: true },
   );
 
   userEvent.click(screen.getByTestId('details-panel-content'));
@@ -193,6 +196,7 @@ test('Should render "unsetIndicators"', () => {
     <DetailsPanel {...props}>
       <div data-test="details-panel-content">Content</div>
     </DetailsPanel>,
+    { useRedux: true },
   );
 
   userEvent.click(screen.getByTestId('details-panel-content'));
@@ -227,6 +231,7 @@ test('Should render empty', () => {
     <DetailsPanel {...props}>
       <div data-test="details-panel-content">Content</div>
     </DetailsPanel>,
+    { useRedux: true },
   );
 
   expect(screen.getByTestId('details-panel-content')).toBeInTheDocument();
diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx
index 6a311c3bcb3b8..54058236c1d2d 100644
--- a/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx
+++ b/superset-frontend/src/dashboard/components/FiltersBadge/DetailsPanel/index.tsx
@@ -17,6 +17,7 @@
  * under the License.
  */
 import React, { useEffect, useState } from 'react';
+import { useSelector } from 'react-redux';
 import { Global, css } from '@emotion/react';
 import { t, useTheme } from '@superset-ui/core';
 import {
@@ -35,6 +36,7 @@ import {
 } from 'src/dashboard/components/FiltersBadge/Styles';
 import { Indicator } from 'src/dashboard/components/FiltersBadge/selectors';
 import FilterIndicator from 'src/dashboard/components/FiltersBadge/FilterIndicator';
+import { RootState } from 'src/dashboard/types';
 
 export interface DetailsPanelProps {
   appliedCrossFilterIndicators: Indicator[];
@@ -55,6 +57,9 @@ const DetailsPanelPopover = ({
 }: DetailsPanelProps) => {
   const [visible, setVisible] = useState(false);
   const theme = useTheme();
+  const activeTabs = useSelector<RootState>(
+    state => state.dashboardState?.activeTabs,
+  );
 
   // we don't need to clean up useEffect, setting { once: true } removes the event listener after handle function is called
   useEffect(() => {
@@ -65,6 +70,11 @@ const DetailsPanelPopover = ({
     }
   }, [visible]);
 
+  // if tabs change, popover doesn't close automatically
+  useEffect(() => {
+    setVisible(false);
+  }, [activeTabs]);
+
   const getDefaultActivePanel = () => {
     const result = [];
     if (appliedCrossFilterIndicators.length) {
diff --git a/superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts b/superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts
index 48d706fc835e9..34bdfce6d9754 100644
--- a/superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts
+++ b/superset-frontend/src/dashboard/components/FiltersBadge/selectors.ts
@@ -20,7 +20,12 @@ import { NO_TIME_RANGE, TIME_FILTER_MAP } from 'src/explore/constants';
 import { getChartIdsInFilterScope } from 'src/dashboard/util/activeDashboardFilters';
 import { ChartConfiguration, Filters } from 'src/dashboard/reducers/types';
 import { DataMaskStateWithId, DataMaskType } from 'src/dataMask/types';
-import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
+import {
+  ensureIsArray,
+  FeatureFlag,
+  FilterState,
+  isFeatureEnabled,
+} from '@superset-ui/core';
 import { Layout } from '../../types';
 import { getTreeCheckedItems } from '../nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils';
 
@@ -50,6 +55,16 @@ type Filter = {
   datasourceId: string;
 };
 
+const extractLabel = (filter?: FilterState): string | null => {
+  if (filter?.label) {
+    return filter.label;
+  }
+  if (filter?.value) {
+    return ensureIsArray(filter?.value).join(', ');
+  }
+  return null;
+};
+
 const selectIndicatorValue = (
   columnKey: string,
   filter: Filter,
@@ -182,16 +197,16 @@ export const selectNativeIndicatorsForChart = (
   const rejectedColumns = getRejectedColumns(chart);
 
   const getStatus = ({
-    value,
+    label,
     column,
     type = DataMaskType.NativeFilters,
   }: {
-    value: any;
+    label: string | null;
     column?: string;
     type?: DataMaskType;
   }): IndicatorStatus => {
     // a filter is only considered unset if it's value is null
-    const hasValue = value !== null;
+    const hasValue = label !== null;
     if (type === DataMaskType.CrossFilters && hasValue) {
       return IndicatorStatus.CrossFilterApplied;
     }
@@ -220,19 +235,14 @@ export const selectNativeIndicatorsForChart = (
         )
         .map(nativeFilter => {
           const column = nativeFilter.targets[0]?.column?.name;
-          let value =
-            dataMask[nativeFilter.id]?.filterState?.label ??
-            dataMask[nativeFilter.id]?.filterState?.value ??
-            null;
-          if (!Array.isArray(value) && value !== null) {
-            value = [value];
-          }
+          const filterState = dataMask[nativeFilter.id]?.filterState;
+          const label = extractLabel(filterState);
           return {
             column,
             name: nativeFilter.name,
             path: [nativeFilter.id],
-            status: getStatus({ value, column }),
-            value,
+            status: getStatus({ label, column }),
+            value: label,
           };
         });
   }
@@ -249,23 +259,26 @@ export const selectNativeIndicatorsForChart = (
         ),
       )
       .map(chartConfig => {
-        let value =
-          dataMask[chartConfig.id]?.filterState?.label ??
-          dataMask[chartConfig.id]?.filterState?.value ??
-          null;
-        if (!Array.isArray(value) && value !== null) {
-          value = [value];
-        }
+        const filterState = dataMask[chartConfig.id]?.filterState;
+        const label = extractLabel(filterState);
+        const filtersState = filterState?.filters;
+        const column = filtersState && Object.keys(filtersState)[0];
+
+        const dashboardLayoutItem = Object.values(dashboardLayout).find(
+          layoutItem => layoutItem?.meta?.chartId === chartConfig.id,
+        );
         return {
-          name: Object.values(dashboardLayout).find(
-            layoutItem => layoutItem?.meta?.chartId === chartConfig.id,
-          )?.meta?.sliceName as string,
-          path: [`${chartConfig.id}`],
+          column,
+          name: dashboardLayoutItem?.meta?.sliceName as string,
+          path: [
+            ...(dashboardLayoutItem?.parents ?? []),
+            dashboardLayoutItem?.id,
+          ],
           status: getStatus({
-            value,
+            label,
             type: DataMaskType.CrossFilters,
           }),
-          value,
+          value: label,
         };
       })
       .filter(filter => filter.status === IndicatorStatus.CrossFilterApplied);