feat(dashboard): Add edit button to dashboard native filters filter cards (#22364)

Co-authored-by: Herbert Gainor <herbert.gainor@preset.io>

superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink/index.tsx

@@ -27,6 +27,8 @@ import { getFilterBarTestId } from '../utils';
 export interface FCBProps {
   createNewOnOpen?: boolean;
   dashboardId?: number;
+  initialFilterId?: string;
+  onClick?: () => void;
   children?: React.ReactNode;
 }
 
@@ -37,6 +39,8 @@ const HeaderButton = styled(Button)`
 export const FilterConfigurationLink: React.FC<FCBProps> = ({
   createNewOnOpen,
   dashboardId,
+  initialFilterId,
+  onClick,
   children,
 }) => {
   const dispatch = useDispatch();
@@ -56,7 +60,10 @@ export const FilterConfigurationLink: React.FC<FCBProps> = ({
 
   const handleClick = useCallback(() => {
     setOpen(true);
-  }, [setOpen]);
+    if (onClick) {
+      onClick();
+    }
+  }, [setOpen, onClick]);
 
   return (
     <>
@@ -73,6 +80,7 @@ export const FilterConfigurationLink: React.FC<FCBProps> = ({
         isOpen={isOpen}
         onSave={submit}
         onCancel={close}
+        initialFilterId={initialFilterId}
         createNewOnOpen={createNewOnOpen}
         key={`filters-for-${dashboardId}`}
       />

superset-frontend/src/dashboard/components/nativeFilters/FilterCard/DependenciesRow.tsx

@@ -106,6 +106,7 @@ export const DependenciesRow = React.memo(({ filter }: FilterCardRowProps) => {
         <RowValue ref={dependenciesRef}>
           {dependencies.map((dependency, index) => (
             <DependencyValue
+              key={dependency.id}
               dependency={dependency}
               hasSeparator={index !== 0}
             />

superset-frontend/src/dashboard/components/nativeFilters/FilterCard/FilterCard.test.tsx

@@ -27,6 +27,7 @@ import { SET_FOCUSED_NATIVE_FILTER } from 'src/dashboard/actions/nativeFilters';
 import { FilterCardContent } from './FilterCardContent';
 
 const baseInitialState = {
+  dashboardInfo: {},
   nativeFilters: {
     filters: {
       'NATIVE_FILTER-1': {
@@ -204,6 +205,13 @@ jest.mock('@superset-ui/core', () => ({
   }),
 }));
 
+jest.mock(
+  'src/components/Icons/Icon',
+  () =>
+    ({ fileName }: { fileName: string }) =>
+      <span role="img" aria-label={fileName.replace('_', '-')} />,
+);
+
 // extract text from embedded html tags
 // source: https://polvara.me/posts/five-things-you-didnt-know-about-testing-library
 const getTextInHTMLTags =
@@ -217,90 +225,115 @@ const getTextInHTMLTags =
     return nodeHasText && childrenDontHaveText;
   };
 
+const hidePopover = jest.fn();
+
 const renderContent = (filter = baseFilter, initialState = baseInitialState) =>
-  render(<FilterCardContent filter={filter} />, {
+  render(<FilterCardContent filter={filter} hidePopover={hidePopover} />, {
     useRedux: true,
     initialState,
   });
 
-describe('Filter Card', () => {
-  it('Basic', () => {
-    renderContent();
-    expect(screen.getByText('Native filter 1')).toBeVisible();
-    expect(screen.getByLabelText('filter-small')).toBeVisible();
+test('filter card title, type, scope, dependencies', () => {
+  renderContent();
+  expect(screen.getByText('Native filter 1')).toBeVisible();
+  expect(screen.getByLabelText('filter-small')).toBeVisible();
 
-    expect(screen.getByText('Filter type')).toBeVisible();
-    expect(screen.getByText('Select filter')).toBeVisible();
+  expect(screen.getByText('Filter type')).toBeVisible();
+  expect(screen.getByText('Select filter')).toBeVisible();
 
-    expect(screen.getByText('Scope')).toBeVisible();
-    expect(screen.getByText('All charts')).toBeVisible();
+  expect(screen.getByText('Scope')).toBeVisible();
+  expect(screen.getByText('All charts')).toBeVisible();
 
-    expect(screen.queryByText('Dependencies')).not.toBeInTheDocument();
-  });
+  expect(screen.queryByText('Dependencies')).not.toBeInTheDocument();
+});
 
-  describe('Scope', () => {
-    it('Scope with excluded', () => {
-      const filter = {
-        ...baseFilter,
-        scope: { rootPath: [DASHBOARD_ROOT_ID], excluded: [1, 4] },
-      };
-      renderContent(filter);
-      expect(screen.getByText('Scope')).toBeVisible();
-      expect(
-        screen.getByText(getTextInHTMLTags('Test chart 2, Test chart 3')),
-      ).toBeVisible();
-    });
+test('filter card scope with excluded', () => {
+  const filter = {
+    ...baseFilter,
+    scope: { rootPath: [DASHBOARD_ROOT_ID], excluded: [1, 4] },
+  };
+  renderContent(filter);
+  expect(screen.getByText('Scope')).toBeVisible();
+  expect(
+    screen.getByText(getTextInHTMLTags('Test chart 2, Test chart 3')),
+  ).toBeVisible();
+});
 
-    it('Scope with top level tab as root', () => {
-      const filter = {
-        ...baseFilter,
-        scope: { rootPath: ['TAB-1', 'TAB-2'], excluded: [1, 2] },
-      };
-      renderContent(filter);
-      expect(screen.getByText('Scope')).toBeVisible();
-      expect(
-        screen.getByText(getTextInHTMLTags('Tab 2, Test chart 3')),
-      ).toBeVisible();
-    });
+test('filter card scope with top level tab as root', () => {
+  const filter = {
+    ...baseFilter,
+    scope: { rootPath: ['TAB-1', 'TAB-2'], excluded: [1, 2] },
+  };
+  renderContent(filter);
+  expect(screen.getByText('Scope')).toBeVisible();
+  expect(
+    screen.getByText(getTextInHTMLTags('Tab 2, Test chart 3')),
+  ).toBeVisible();
+});
 
-    it('Empty scope', () => {
-      const filter = {
-        ...baseFilter,
-        scope: { rootPath: [], excluded: [1, 2, 3, 4] },
-      };
-      renderContent(filter);
-      expect(screen.getByText('Scope')).toBeVisible();
-      expect(screen.getByText('None')).toBeVisible();
-    });
-  });
+test('filter card empty scope', () => {
+  const filter = {
+    ...baseFilter,
+    scope: { rootPath: [], excluded: [1, 2, 3, 4] },
+  };
+  renderContent(filter);
+  expect(screen.getByText('Scope')).toBeVisible();
+  expect(screen.getByText('None')).toBeVisible();
+});
 
-  describe('Dependencies', () => {
-    it('Has dependency', () => {
-      const filter = {
-        ...baseFilter,
-        cascadeParentIds: ['NATIVE_FILTER-2'],
-      };
-      renderContent(filter);
-      expect(screen.getByText('Dependent on')).toBeVisible();
-      expect(screen.getByText('Native filter 2')).toBeVisible();
-    });
+test('filter card with dependency', () => {
+  const filter = {
+    ...baseFilter,
+    cascadeParentIds: ['NATIVE_FILTER-2'],
+  };
+  renderContent(filter);
+  expect(screen.getByText('Dependent on')).toBeVisible();
+  expect(screen.getByText('Native filter 2')).toBeVisible();
+});
 
-    it('Focus filter on dependency click', () => {
-      const useDispatchMock = jest.spyOn(reactRedux, 'useDispatch');
-      const dummyDispatch = jest.fn();
-      useDispatchMock.mockReturnValue(dummyDispatch);
+test('focus filter on filter card dependency click', () => {
+  const useDispatchMock = jest.spyOn(reactRedux, 'useDispatch');
+  const dummyDispatch = jest.fn();
+  useDispatchMock.mockReturnValue(dummyDispatch);
 
-      const filter = {
-        ...baseFilter,
-        cascadeParentIds: ['NATIVE_FILTER-2'],
-      };
-      renderContent(filter);
+  const filter = {
+    ...baseFilter,
+    cascadeParentIds: ['NATIVE_FILTER-2'],
+  };
+  renderContent(filter);
 
-      userEvent.click(screen.getByText('Native filter 2'));
-      expect(dummyDispatch).toHaveBeenCalledWith({
-        type: SET_FOCUSED_NATIVE_FILTER,
-        id: 'NATIVE_FILTER-2',
-      });
-    });
+  userEvent.click(screen.getByText('Native filter 2'));
+  expect(dummyDispatch).toHaveBeenCalledWith({
+    type: SET_FOCUSED_NATIVE_FILTER,
+    id: 'NATIVE_FILTER-2',
   });
 });
+
+test('edit filter button for dashboard viewer', () => {
+  renderContent();
+  expect(
+    screen.queryByRole('button', { name: /edit/i }),
+  ).not.toBeInTheDocument();
+});
+
+test('edit filter button for dashboard editor', () => {
+  renderContent(baseFilter, {
+    ...baseInitialState,
+    dashboardInfo: { dash_edit_perm: true },
+  });
+
+  expect(screen.getByRole('button', { name: /edit/i })).toBeVisible();
+});
+
+test('open modal on edit filter button click', async () => {
+  renderContent(baseFilter, {
+    ...baseInitialState,
+    dashboardInfo: { dash_edit_perm: true },
+  });
+
+  const editButton = screen.getByRole('button', { name: /edit/i });
+  userEvent.click(editButton);
+  expect(
+    await screen.findByRole('dialog', { name: /add and edit filters/i }),
+  ).toBeVisible();
+});

superset-frontend/src/dashboard/components/nativeFilters/FilterCard/FilterCardContent.tsx

@@ -24,9 +24,17 @@ import { DependenciesRow } from './DependenciesRow';
 import { NameRow } from './NameRow';
 import { TypeRow } from './TypeRow';
 
-export const FilterCardContent = ({ filter }: { filter: Filter }) => (
+interface FilterCardContentProps {
+  filter: Filter;
+  hidePopover: () => void;
+}
+
+export const FilterCardContent = ({
+  filter,
+  hidePopover,
+}: FilterCardContentProps) => (
   <div>
-    <NameRow filter={filter} />
+    <NameRow filter={filter} hidePopover={hidePopover} />
     <TypeRow filter={filter} />
     <ScopeRow filter={filter} />
     <DependenciesRow filter={filter} />

superset-frontend/src/dashboard/components/nativeFilters/FilterCard/NameRow.tsx

@@ -17,34 +17,61 @@
  * under the License.
  */
 import React, { useRef } from 'react';
-import { css, SupersetTheme } from '@superset-ui/core';
+import { useSelector } from 'react-redux';
+import { css, SupersetTheme, useTheme } from '@superset-ui/core';
 import Icons from 'src/components/Icons';
 import { useTruncation } from 'src/hooks/useTruncation';
-import { Row, FilterName } from './Styles';
+import { RootState } from 'src/dashboard/types';
+import { Row, FilterName, InternalRow } from './Styles';
 import { FilterCardRowProps } from './types';
+import { FilterConfigurationLink } from '../FilterBar/FilterConfigurationLink';
 import { TooltipWithTruncation } from './TooltipWithTruncation';
 
-export const NameRow = ({ filter }: FilterCardRowProps) => {
+export const NameRow = ({
+  filter,
+  hidePopover,
+}: FilterCardRowProps & { hidePopover: () => void }) => {
+  const theme = useTheme();
   const filterNameRef = useRef<HTMLElement>(null);
   const [elementsTruncated] = useTruncation(filterNameRef);
+  const dashboardId = useSelector<RootState, number>(
+    ({ dashboardInfo }) => dashboardInfo.id,
+  );
+
+  const canEdit = useSelector<RootState, boolean>(
+    ({ dashboardInfo }) => dashboardInfo.dash_edit_perm,
+  );
+
   return (
     <Row
       css={(theme: SupersetTheme) =>
         css`
           margin-bottom: ${theme.gridUnit * 3}px;
+          justify-content: space-between;
         `
       }
     >
-      <Icons.FilterSmall
-        css={(theme: SupersetTheme) =>
-          css`
-            margin-right: ${theme.gridUnit}px;
-          `
-        }
-      />
-      <TooltipWithTruncation title={elementsTruncated ? filter.name : null}>
-        <FilterName ref={filterNameRef}>{filter.name}</FilterName>
-      </TooltipWithTruncation>
+      <InternalRow>
+        <Icons.FilterSmall
+          css={(theme: SupersetTheme) =>
+            css`
+              margin-right: ${theme.gridUnit}px;
+            `
+          }
+        />
+        <TooltipWithTruncation title={elementsTruncated ? filter.name : null}>
+          <FilterName ref={filterNameRef}>{filter.name}</FilterName>
+        </TooltipWithTruncation>
+      </InternalRow>
+      {canEdit && (
+        <FilterConfigurationLink
+          dashboardId={dashboardId}
+          onClick={hidePopover}
+          initialFilterId={filter.id}
+        >
+          <Icons.Edit iconSize="l" iconColor={theme.colors.grayscale.light1} />
+        </FilterConfigurationLink>
+      )}
     </Row>
   );
 };

superset-frontend/src/dashboard/components/nativeFilters/FilterCard/Styles.ts

@@ -92,3 +92,8 @@ export const TooltipTrigger = styled.div`
   display: inline-flex;
   white-space: nowrap;
 `;
+
+export const InternalRow = styled.div`
+  display: flex;
+  align-items: center;
+`;

superset-frontend/src/dashboard/components/nativeFilters/FilterCard/index.tsx

@@ -31,6 +31,10 @@ export const FilterCard = ({
 }: FilterCardProps) => {
   const [internalIsVisible, setInternalIsVisible] = useState(false);
 
+  const hidePopover = () => {
+    setInternalIsVisible(false);
+  };
+
   useEffect(() => {
     if (!externalIsVisible) {
       setInternalIsVisible(false);
@@ -46,9 +50,8 @@ export const FilterCard = ({
         setInternalIsVisible(externalIsVisible && visible);
       }}
       visible={externalIsVisible && internalIsVisible}
-      content={<FilterCardContent filter={filter} />}
+      content={<FilterCardContent filter={filter} hidePopover={hidePopover} />}
       getPopupContainer={getPopupContainer ?? (() => document.body)}
-      destroyTooltipOnHide
     >
       {children}
     </Popover>