From 004ee0abd9e42762a1975f70edf0812fc4ebb3bf Mon Sep 17 00:00:00 2001
From: Alexander Polyankin <alexander.polyankin@metabase.com>
Date: Mon, 17 Jul 2023 11:48:40 +0300
Subject: [PATCH] Do not show edit model menu options without write permissions
(#32261)
---
.../metabase-enterprise/moderation/index.js | 40 ++++----
frontend/src/metabase/plugins/index.ts | 2 +-
.../components/QuestionActions.tsx | 18 ++--
.../components/QuestionActions.unit.spec.tsx | 99 ++++++++++++++-----
4 files changed, 108 insertions(+), 51 deletions(-)
diff --git a/enterprise/frontend/src/metabase-enterprise/moderation/index.js b/enterprise/frontend/src/metabase-enterprise/moderation/index.js
index c060b33c8f7..0194ab535b1 100644
--- a/enterprise/frontend/src/metabase-enterprise/moderation/index.js
+++ b/enterprise/frontend/src/metabase-enterprise/moderation/index.js
@@ -41,26 +41,30 @@ if (hasPremiumFeature("content_management")) {
const isVerified = isItemVerified(latestModerationReview);
if (isModerator) {
- return {
- title: isVerified
- ? t`Remove verification`
- : isDataset
- ? t`Verify this model`
- : t`Verify this question`,
- icon: isVerified ? "close" : verifiedIconName,
- action: () => {
- if (isVerified) {
- removeReview({ itemId: id, itemType: "card" });
- } else {
- verifyItem({ itemId: id, itemType: "card" });
- }
- reload();
+ return [
+ {
+ title: isVerified
+ ? t`Remove verification`
+ : isDataset
+ ? t`Verify this model`
+ : t`Verify this question`,
+ icon: isVerified ? "close" : verifiedIconName,
+ action: () => {
+ if (isVerified) {
+ removeReview({ itemId: id, itemType: "card" });
+ } else {
+ verifyItem({ itemId: id, itemType: "card" });
+ }
+ reload();
+ },
+ testId: isVerified
+ ? "moderation-remove-verification-action"
+ : "moderation-verify-action",
},
- testId: isVerified
- ? "moderation-remove-verification-action"
- : "moderation-verify-action",
- };
+ ];
}
+
+ return [];
},
});
}
diff --git a/frontend/src/metabase/plugins/index.ts b/frontend/src/metabase/plugins/index.ts
index 2f47681eb44..d994e5a886b 100644
--- a/frontend/src/metabase/plugins/index.ts
+++ b/frontend/src/metabase/plugins/index.ts
@@ -198,7 +198,7 @@ export const PLUGIN_MODERATION = {
question?: Question,
isModerator?: boolean,
reload?: () => void,
- ) => ({}),
+ ) => [],
};
export const PLUGIN_CACHING = {
diff --git a/frontend/src/metabase/query_builder/components/QuestionActions.tsx b/frontend/src/metabase/query_builder/components/QuestionActions.tsx
index f94c8eab538..5e7e6f6269b 100644
--- a/frontend/src/metabase/query_builder/components/QuestionActions.tsx
+++ b/frontend/src/metabase/query_builder/components/QuestionActions.tsx
@@ -128,14 +128,14 @@ const QuestionActions = ({
}
extraButtons.push(
- PLUGIN_MODERATION.getMenuItems(
+ ...PLUGIN_MODERATION.getMenuItems(
question,
isModerator,
dispatchSoftReloadCard,
),
);
- if (isDataset) {
+ if (canWrite && isDataset) {
extraButtons.push(
{
title: t`Edit query definition`,
@@ -237,12 +237,14 @@ const QuestionActions = ({
/>
</ViewHeaderIconButtonContainer>
</Tooltip>
- <EntityMenu
- triggerAriaLabel={t`Move, archive, and more...`}
- items={extraButtons}
- triggerIcon="ellipsis"
- tooltip={t`Move, archive, and more...`}
- />
+ {extraButtons.length > 0 && (
+ <EntityMenu
+ triggerAriaLabel={t`Move, archive, and more...`}
+ items={extraButtons}
+ triggerIcon="ellipsis"
+ tooltip={t`Move, archive, and more...`}
+ />
+ )}
</>
);
};
diff --git a/frontend/src/metabase/query_builder/components/QuestionActions.unit.spec.tsx b/frontend/src/metabase/query_builder/components/QuestionActions.unit.spec.tsx
index 9ffd482f8c9..4e20802efcd 100644
--- a/frontend/src/metabase/query_builder/components/QuestionActions.unit.spec.tsx
+++ b/frontend/src/metabase/query_builder/components/QuestionActions.unit.spec.tsx
@@ -1,17 +1,25 @@
import userEvent from "@testing-library/user-event";
-import { renderWithProviders, screen } from "__support__/ui";
+import { getMetadata } from "metabase/selectors/metadata";
+import { Card, Database } from "metabase-types/api";
+import { createMockCard, createMockNativeCard } from "metabase-types/api/mocks";
+import { createSampleDatabase } from "metabase-types/api/mocks/presets";
import { createMockState } from "metabase-types/store/mocks";
import { createMockEntitiesState } from "__support__/store";
-import QuestionActions from "metabase/query_builder/components/QuestionActions";
-import { createMockCard, createMockNativeCard } from "metabase-types/api/mocks";
-import { getMetadata } from "metabase/selectors/metadata";
-import { Card } from "metabase-types/api";
+import {
+ getIcon,
+ queryIcon,
+ renderWithProviders,
+ screen,
+} from "__support__/ui";
import Question from "metabase-lib/Question";
+import QuestionActions from "./QuestionActions";
-const TEST_STRUCTURED_CARD = createMockCard();
-const TEST_NATIVE_CARD = createMockNativeCard();
+const ICON_CASES_CARDS = [
+ createMockCard({ name: "GUI" }),
+ createMockNativeCard({ name: "SQL" }),
+];
-const iconList = [
+const ICON_CASES_LABELS = [
{ label: "bookmark icon", tooltipText: "Bookmark" },
{ label: "info icon", tooltipText: "More info" },
{
@@ -20,9 +28,19 @@ const iconList = [
},
];
-function setup({ card }: { card: Card }) {
+const ICON_CASES = ICON_CASES_CARDS.flatMap(card =>
+ ICON_CASES_LABELS.map(labels => ({ ...labels, card })),
+);
+
+interface SetupOpts {
+ card: Card;
+ databases?: Database[];
+}
+
+function setup({ card, databases = [createSampleDatabase()] }: SetupOpts) {
const state = createMockState({
entities: createMockEntitiesState({
+ databases,
questions: [card],
}),
});
@@ -46,22 +64,55 @@ function setup({ card }: { card: Card }) {
);
}
-describe("Question Actions | Icons", () => {
- ["structured", "native"].forEach(queryType => {
- iconList.forEach(({ label, tooltipText }) => {
- it(`should display the "${label}" icon with the "${tooltipText}" tooltip for ${queryType} questions`, async () => {
- setup({
- card:
- queryType === "structured"
- ? TEST_STRUCTURED_CARD
- : TEST_NATIVE_CARD,
- });
+describe("QuestionActions", () => {
+ it.each(ICON_CASES)(
+ `should display the "$label" icon with the "$tooltipText" tooltip for $card.name questions`,
+ async ({ label, tooltipText, card }) => {
+ setup({ card });
+
+ await userEvent.hover(screen.getByRole("button", { name: label }));
+ const tooltip = screen.getByRole("tooltip", { name: tooltipText });
+ expect(tooltip).toHaveAttribute("data-placement", "top");
+ expect(tooltip).toHaveTextContent(tooltipText);
+ },
+ );
- await userEvent.hover(screen.getByRole("button", { name: label }));
- const tooltip = screen.getByRole("tooltip", { name: tooltipText });
- expect(tooltip).toHaveAttribute("data-placement", "top");
- expect(tooltip).toHaveTextContent(tooltipText);
- });
+ it("should allow to edit the model only with write permissions", () => {
+ setup({
+ card: createMockCard({
+ dataset: true,
+ can_write: true,
+ }),
});
+
+ userEvent.click(getIcon("ellipsis"));
+ expect(screen.getByText("Edit query definition")).toBeInTheDocument();
+ expect(screen.getByText("Edit metadata")).toBeInTheDocument();
+ });
+
+ it("should not allow to edit the model without write permissions", () => {
+ setup({
+ card: createMockCard({
+ dataset: true,
+ can_write: false,
+ }),
+ });
+
+ userEvent.click(getIcon("ellipsis"));
+ expect(screen.queryByText("Edit query definition")).not.toBeInTheDocument();
+ expect(screen.queryByText("Edit metadata")).not.toBeInTheDocument();
+ });
+
+ it("should not render the menu when there are no menu items", () => {
+ setup({
+ card: createMockCard({
+ dataset: true,
+ can_write: false,
+ }),
+ databases: [],
+ });
+
+ expect(getIcon("info")).toBeInTheDocument();
+ expect(queryIcon("ellipsis")).not.toBeInTheDocument();
});
});
--
GitLab