From 3b45ad8b97ff7a72ac4d57cfbd2289bf38022cfc Mon Sep 17 00:00:00 2001 From: Cody Leff Date: Fri, 9 Dec 2022 07:00:15 -0700 Subject: [PATCH] feat(dashboard): Add edit button to dashboard native filters filter cards (#22364) Co-authored-by: Herbert Gainor --- .../FilterConfigurationLink/index.tsx | 10 +- .../FilterCard/DependenciesRow.tsx | 1 + .../FilterCard/FilterCard.test.tsx | 173 +++++++++++------- .../FilterCard/FilterCardContent.tsx | 12 +- .../nativeFilters/FilterCard/NameRow.tsx | 53 ++++-- .../nativeFilters/FilterCard/Styles.ts | 5 + .../nativeFilters/FilterCard/index.tsx | 7 +- 7 files changed, 173 insertions(+), 88 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink/index.tsx index 8319c3c9fb..c9e4b834d9 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink/index.tsx +++ b/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 = ({ createNewOnOpen, dashboardId, + initialFilterId, + onClick, children, }) => { const dispatch = useDispatch(); @@ -56,7 +60,10 @@ export const FilterConfigurationLink: React.FC = ({ const handleClick = useCallback(() => { setOpen(true); - }, [setOpen]); + if (onClick) { + onClick(); + } + }, [setOpen, onClick]); return ( <> @@ -73,6 +80,7 @@ export const FilterConfigurationLink: React.FC = ({ isOpen={isOpen} onSave={submit} onCancel={close} + initialFilterId={initialFilterId} createNewOnOpen={createNewOnOpen} key={`filters-for-${dashboardId}`} /> diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/DependenciesRow.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/DependenciesRow.tsx index ccffdf4b56..6a846c012f 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/DependenciesRow.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/DependenciesRow.tsx @@ -106,6 +106,7 @@ export const DependenciesRow = React.memo(({ filter }: FilterCardRowProps) => { {dependencies.map((dependency, index) => ( diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/FilterCard.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/FilterCard.test.tsx index 970d494ebd..560fa1faf0 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/FilterCard.test.tsx +++ b/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 }) => + , +); + // 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(, { + render(, { 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(); +}); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/FilterCardContent.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/FilterCardContent.tsx index 41696ed98f..02b0d875a9 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/FilterCardContent.tsx +++ b/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) => (
- + diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/NameRow.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/NameRow.tsx index f6268296ef..6c7e82b15d 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/NameRow.tsx +++ b/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(null); const [elementsTruncated] = useTruncation(filterNameRef); + const dashboardId = useSelector( + ({ dashboardInfo }) => dashboardInfo.id, + ); + + const canEdit = useSelector( + ({ dashboardInfo }) => dashboardInfo.dash_edit_perm, + ); + return ( css` margin-bottom: ${theme.gridUnit * 3}px; + justify-content: space-between; ` } > - - css` - margin-right: ${theme.gridUnit}px; - ` - } - /> - - {filter.name} - + + + css` + margin-right: ${theme.gridUnit}px; + ` + } + /> + + {filter.name} + + + {canEdit && ( + + + + )} ); }; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/Styles.ts b/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/Styles.ts index bda865063d..8090201f1c 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/Styles.ts +++ b/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; +`; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/index.tsx index 8d4b9051eb..bdbfcff8ae 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/index.tsx +++ b/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={} + content={} getPopupContainer={getPopupContainer ?? (() => document.body)} - destroyTooltipOnHide > {children}