Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Add permission to view and drill on Dashboard context #26798

Merged
merged 15 commits into from
Jan 29, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,12 @@ const ChartContextMenu = (
const canExplore = useSelector((state: RootState) =>
findPermission('can_explore', 'Superset', state.user?.roles),
);
const canViewDrill = useSelector((state: RootState) =>
findPermission('can_view_and_drill', 'Dashboard', state.user?.roles),
);
const canDatasourceSamples = useSelector((state: RootState) =>
findPermission('can_samples', 'Datasource', state.user?.roles),
);
const crossFiltersEnabled = useSelector<RootState, boolean>(
({ dashboardInfo }) => dashboardInfo.crossFiltersEnabled,
);
Expand All @@ -105,15 +111,17 @@ const ChartContextMenu = (
}>({ clientX: 0, clientY: 0 });

const menuItems = [];
const canExploreOrView = canExplore || canViewDrill;

const showDrillToDetail =
isFeatureEnabled(FeatureFlag.DRILL_TO_DETAIL) &&
canExplore &&
canExploreOrView &&
canDatasourceSamples &&
isDisplayed(ContextMenuItem.DrillToDetail);

const showDrillBy =
isFeatureEnabled(FeatureFlag.DRILL_BY) &&
canExplore &&
canExploreOrView &&
isDisplayed(ContextMenuItem.DrillBy);

const showCrossFilters =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,12 @@ const setup = ({
onSelection = noOp,
displayedItems = ContextMenuItem.All,
additionalConfig = {},
roles = undefined,
}: {
onSelection?: () => void;
displayedItems?: ContextMenuItem | ContextMenuItem[];
additionalConfig?: Record<string, any>;
roles?: Record<string, string[][]>;
} = {}) => {
const { result } = renderHook(() =>
useContextMenu(
Expand All @@ -58,7 +60,12 @@ const setup = ({
...mockState,
user: {
...mockState.user,
roles: { Admin: [['can_explore', 'Superset']] },
roles: roles ?? {
Admin: [
['can_explore', 'Superset'],
['can_samples', 'Datasource'],
],
},
},
},
});
Expand All @@ -75,7 +82,7 @@ test('Context menu renders', () => {
expect(screen.getByText('Drill by')).toBeInTheDocument();
});

test('Context menu contains all items only', () => {
test('Context menu contains all displayed items only', () => {
const result = setup({
displayedItems: [ContextMenuItem.DrillToDetail, ContextMenuItem.DrillBy],
});
Expand All @@ -84,3 +91,28 @@ test('Context menu contains all items only', () => {
expect(screen.getByText('Drill to detail')).toBeInTheDocument();
expect(screen.getByText('Drill by')).toBeInTheDocument();
});

test('Context menu shows all items tied to can_view_and_drill permission', () => {
const result = setup({
roles: {
Admin: [
['can_view_and_drill', 'Dashboard'],
['can_samples', 'Datasource'],
],
},
});
result.current.onContextMenu(0, 0, {});
expect(screen.getByText('Drill to detail')).toBeInTheDocument();
expect(screen.getByText('Drill by')).toBeInTheDocument();
expect(screen.getByText('Add cross-filter')).toBeInTheDocument();
});

test('Context menu does not show "Drill to detail" without proper permissions', () => {
const result = setup({
roles: { Admin: [['can_view_and_drill', 'Dashboard']] },
});
result.current.onContextMenu(0, 0, {});
expect(screen.queryByText('Drill to detail')).not.toBeInTheDocument();
expect(screen.getByText('Drill by')).toBeInTheDocument();
expect(screen.getByText('Add cross-filter')).toBeInTheDocument();
});
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,10 @@ const dataset = {
],
};

const renderModal = async (modalProps: Partial<DrillByModalProps> = {}) => {
const renderModal = async (
modalProps: Partial<DrillByModalProps> = {},
overrideState: Record<string, any> = {},
) => {
const DrillByModalWrapper = () => {
const [showModal, setShowModal] = useState(false);

Expand All @@ -93,7 +96,10 @@ const renderModal = async (modalProps: Partial<DrillByModalProps> = {}) => {
useDnd: true,
useRedux: true,
useRouter: true,
initialState: drillByModalState,
initialState: {
...drillByModalState,
...overrideState,
},
});

userEvent.click(screen.getByRole('button', { name: 'Show modal' }));
Expand Down Expand Up @@ -233,3 +239,29 @@ test('render breadcrumbs', async () => {
expect(newBreadcrumbItems).toHaveLength(1);
expect(within(breadcrumbItems[0]).getByText('gender')).toBeInTheDocument();
});

test('should render "Edit chart" as disabled without can_explore permission', async () => {
await renderModal(
{},
{
user: {
...drillByModalState.user,
roles: { Admin: [['test_invalid_role', 'Superset']] },
},
},
);
expect(screen.getByRole('button', { name: 'Edit chart' })).toBeDisabled();
});

test('should render "Edit chart" enabled with can_explore permission', async () => {
await renderModal(
{},
{
user: {
...drillByModalState.user,
roles: { Admin: [['can_explore', 'Superset']] },
},
},
);
expect(screen.getByRole('button', { name: 'Edit chart' })).toBeEnabled();
});
17 changes: 16 additions & 1 deletion superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import {
LOG_ACTIONS_DRILL_BY_MODAL_OPENED,
LOG_ACTIONS_FURTHER_DRILL_BY,
} from 'src/logger/LogUtils';
import { findPermission } from 'src/utils/findPermission';
import { Dataset, DrillByType } from '../types';
import DrillByChart from './DrillByChart';
import { ContextMenuItem } from '../ChartContextMenu/ChartContextMenu';
Expand All @@ -75,6 +76,7 @@ interface ModalFooterProps {
const ModalFooter = ({ formData, closeModal }: ModalFooterProps) => {
const dispatch = useDispatch();
const { addDangerToast } = useToasts();
const theme = useTheme();
const [url, setUrl] = useState('');
const dashboardPageId = useContext(DashboardPageIdContext);
const onEditChartClick = useCallback(() => {
Expand All @@ -84,6 +86,9 @@ const ModalFooter = ({ formData, closeModal }: ModalFooterProps) => {
}),
);
}, [dispatch, formData.slice_id]);
const canExplore = useSelector((state: RootState) =>
findPermission('can_explore', 'Superset', state.user?.roles),
);

const [datasource_id, datasource_type] = formData.datasource.split('__');
useEffect(() => {
Expand All @@ -103,13 +108,20 @@ const ModalFooter = ({ formData, closeModal }: ModalFooterProps) => {
datasource_type,
formData,
]);
const isEditDisabled = !url || !canExplore;

return (
<>
<Button
buttonStyle="secondary"
buttonSize="small"
onClick={onEditChartClick}
disabled={!url}
disabled={isEditDisabled}
tooltip={
isEditDisabled
? t('You do not have sufficient permissions to edit the chart')
: undefined
}
>
<Link
css={css`
Expand All @@ -128,6 +140,9 @@ const ModalFooter = ({ formData, closeModal }: ModalFooterProps) => {
buttonSize="small"
onClick={closeModal}
data-test="close-drill-by-modal"
css={css`
margin-left: ${theme.gridUnit * 2}px;
`}
>
{t('Close')}
</Button>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,16 @@ jest.mock('react-router-dom', () => ({

const { id: chartId, form_data: formData } = chartQueries[sliceId];
const { slice_name: chartName } = formData;
const store = getMockStoreWithNativeFilters();
const drillToDetailModalState = {
...store.getState(),
user: {
...store.getState().user,
roles: { Admin: [['can_explore', 'Superset']] },
},
};

const renderModal = async () => {
const store = getMockStoreWithNativeFilters();
const renderModal = async (overrideState: Record<string, any> = {}) => {
const DrillDetailModalWrapper = () => {
const [showModal, setShowModal] = useState(false);
return (
Expand All @@ -59,7 +66,10 @@ const renderModal = async () => {
render(<DrillDetailModalWrapper />, {
useRouter: true,
useRedux: true,
store,
initialState: {
...drillToDetailModalState,
...overrideState,
},
});

userEvent.click(screen.getByRole('button', { name: 'Show modal' }));
Expand Down Expand Up @@ -93,3 +103,13 @@ test('should forward to Explore', async () => {
`/explore/?dashboard_page_id=&slice_id=${sliceId}`,
);
});

test('should render "Edit chart" as disabled without can_explore permission', async () => {
await renderModal({
user: {
...drillToDetailModalState.user,
roles: { Admin: [['test_invalid_role', 'Superset']] },
},
});
expect(screen.getByRole('button', { name: 'Edit chart' })).toBeDisabled();
});
Original file line number Diff line number Diff line change
Expand Up @@ -31,28 +31,52 @@ import Button from 'src/components/Button';
import { useSelector } from 'react-redux';
import { DashboardPageIdContext } from 'src/dashboard/containers/DashboardPage';
import { Slice } from 'src/types/Chart';
import { RootState } from 'src/dashboard/types';
import { findPermission } from 'src/utils/findPermission';
import DrillDetailPane from './DrillDetailPane';

interface ModalFooterProps {
exploreChart: () => void;
canExplore: boolean;
closeModal?: () => void;
exploreChart: () => void;
}

const ModalFooter = ({ exploreChart, closeModal }: ModalFooterProps) => (
<>
<Button buttonStyle="secondary" buttonSize="small" onClick={exploreChart}>
{t('Edit chart')}
</Button>
<Button
buttonStyle="primary"
buttonSize="small"
onClick={closeModal}
data-test="close-drilltodetail-modal"
>
{t('Close')}
</Button>
</>
);
const ModalFooter = ({
canExplore,
closeModal,
exploreChart,
}: ModalFooterProps) => {
const theme = useTheme();

return (
<>
<Button
buttonStyle="secondary"
buttonSize="small"
onClick={exploreChart}
disabled={!canExplore}
tooltip={
!canExplore
? t('You do not have sufficient permissions to edit the chart')
: undefined
}
>
{t('Edit chart')}
</Button>
<Button
buttonStyle="primary"
buttonSize="small"
onClick={closeModal}
data-test="close-drilltodetail-modal"
css={css`
margin-left: ${theme.gridUnit * 2}px;
`}
>
{t('Close')}
</Button>
</>
);
};

interface DrillDetailModalProps {
chartId: number;
Expand All @@ -76,6 +100,9 @@ export default function DrillDetailModal({
(state: { sliceEntities: { slices: Record<number, Slice> } }) =>
state.sliceEntities.slices[chartId],
);
const canExplore = useSelector((state: RootState) =>
findPermission('can_explore', 'Superset', state.user?.roles),
);

const exploreUrl = useMemo(
() => `/explore/?dashboard_page_id=${dashboardPageId}&slice_id=${chartId}`,
Expand All @@ -97,7 +124,9 @@ export default function DrillDetailModal({
}
`}
title={t('Drill to detail: %s', chartName)}
footer={<ModalFooter exploreChart={exploreChart} />}
footer={
<ModalFooter exploreChart={exploreChart} canExplore={canExplore} />
}
responsive
resizable
resizableConfig={{
Expand Down
Loading
Loading