From d83b13a398cc5b86a83b295e476d381a1c0aa3dc Mon Sep 17 00:00:00 2001
From: Alexander Lesnenko <alxnddr@users.noreply.github.com>
Date: Sat, 7 May 2022 02:05:32 +0400
Subject: [PATCH] Fix permission bugs (#22470)
* fix save button is enabled when users do not have a permission to save a question
* do not show new button
* hide reload query button when users do not have permissions
* update specs
* types
* review
---
.../components/NewCollectionItemMenu.jsx | 7 ++-
.../containers/CollectionContent.jsx | 2 +-
.../containers/CollectionHeader.tsx | 18 ++++++
.../metabase/components/Tooltip/Tooltip.tsx | 2 +-
.../metabase/core/components/Link/Link.tsx | 18 +++++-
frontend/src/metabase/lib/errors.ts | 3 +
.../components/view/ViewHeader.jsx | 14 ++++-
...llogical-UI-elements-for-nodata.cy.spec.js | 59 ++++++++++---------
8 files changed, 86 insertions(+), 37 deletions(-)
create mode 100644 frontend/src/metabase/collections/containers/CollectionHeader.tsx
create mode 100644 frontend/src/metabase/lib/errors.ts
diff --git a/frontend/src/metabase/collections/components/NewCollectionItemMenu.jsx b/frontend/src/metabase/collections/components/NewCollectionItemMenu.jsx
index 1fe9df69969..4e4af5cb587 100644
--- a/frontend/src/metabase/collections/components/NewCollectionItemMenu.jsx
+++ b/frontend/src/metabase/collections/components/NewCollectionItemMenu.jsx
@@ -10,11 +10,12 @@ import { ANALYTICS_CONTEXT } from "metabase/collections/constants";
const propTypes = {
collection: PropTypes.object,
list: PropTypes.arrayOf(PropTypes.object),
+ canCreateQuestions: PropTypes.bool,
};
-function NewCollectionItemMenu({ collection }) {
+function NewCollectionItemMenu({ collection, canCreateQuestions }) {
const items = [
- {
+ canCreateQuestions && {
icon: "insight",
title: t`Question`,
link: newQuestion({ mode: "notebook", collectionId: collection.id }),
@@ -32,7 +33,7 @@ function NewCollectionItemMenu({ collection }) {
link: newCollection(collection.id),
event: `${ANALYTICS_CONTEXT};New Item Menu;Collection Click`,
},
- ];
+ ].filter(Boolean);
return <EntityMenu items={items} triggerIcon="add" tooltip={t`New…`} />;
}
diff --git a/frontend/src/metabase/collections/containers/CollectionContent.jsx b/frontend/src/metabase/collections/containers/CollectionContent.jsx
index 6e5d4a22bbc..8a52ea0825b 100644
--- a/frontend/src/metabase/collections/containers/CollectionContent.jsx
+++ b/frontend/src/metabase/collections/containers/CollectionContent.jsx
@@ -14,7 +14,7 @@ import { getIsNavbarOpen, openNavbar } from "metabase/redux/app";
import BulkActions from "metabase/collections/components/BulkActions";
import CollectionEmptyState from "metabase/components/CollectionEmptyState";
-import Header from "metabase/collections/components/CollectionHeader/CollectionHeader";
+import Header from "metabase/collections/containers/CollectionHeader";
import ItemsTable from "metabase/collections/components/ItemsTable";
import PinnedItemOverview from "metabase/collections/components/PinnedItemOverview";
import { isPersonalCollectionChild } from "metabase/collections/utils";
diff --git a/frontend/src/metabase/collections/containers/CollectionHeader.tsx b/frontend/src/metabase/collections/containers/CollectionHeader.tsx
new file mode 100644
index 00000000000..1d2260ee970
--- /dev/null
+++ b/frontend/src/metabase/collections/containers/CollectionHeader.tsx
@@ -0,0 +1,18 @@
+import _ from "underscore";
+import { connect } from "react-redux";
+
+import { State } from "metabase-types/store";
+import Databases from "metabase/entities/databases";
+import { getHasDataAccess } from "metabase/new_query/selectors";
+import CollectionHeader from "../components/CollectionHeader/CollectionHeader";
+
+const mapStateToProps = (state: State) => ({
+ canCreateQuestions: getHasDataAccess(state),
+});
+
+export default _.compose(
+ Databases.loadList({
+ loadingAndErrorWrapper: false,
+ }),
+ connect(mapStateToProps),
+)(CollectionHeader);
diff --git a/frontend/src/metabase/components/Tooltip/Tooltip.tsx b/frontend/src/metabase/components/Tooltip/Tooltip.tsx
index 7edb0849763..401f4e22cab 100644
--- a/frontend/src/metabase/components/Tooltip/Tooltip.tsx
+++ b/frontend/src/metabase/components/Tooltip/Tooltip.tsx
@@ -19,7 +19,7 @@ Tooltip.propTypes = {
maxWidth: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),
};
-interface TooltipProps
+export interface TooltipProps
extends Partial<
Pick<
Tippy.TippyProps,
diff --git a/frontend/src/metabase/core/components/Link/Link.tsx b/frontend/src/metabase/core/components/Link/Link.tsx
index ed62decfac3..1b4eb34b059 100644
--- a/frontend/src/metabase/core/components/Link/Link.tsx
+++ b/frontend/src/metabase/core/components/Link/Link.tsx
@@ -1,13 +1,14 @@
import React, { CSSProperties, HTMLAttributes, ReactNode } from "react";
import Tooltip from "metabase/components/Tooltip";
import { LinkRoot } from "./Link.styled";
+import { TooltipProps } from "metabase/components/Tooltip/Tooltip";
export interface LinkProps extends HTMLAttributes<HTMLAnchorElement> {
to: string;
disabled?: boolean;
className?: string;
children?: ReactNode;
- tooltip?: string;
+ tooltip?: string | TooltipProps;
activeClassName?: string;
activeStyle?: CSSProperties;
onlyActiveOnIndex?: boolean;
@@ -32,7 +33,20 @@ const Link = ({
</LinkRoot>
);
- return tooltip ? <Tooltip tooltip={tooltip}>{link}</Tooltip> : link;
+ const tooltipProps =
+ typeof tooltip === "string"
+ ? {
+ tooltip,
+ }
+ : tooltip;
+
+ return tooltip ? (
+ <Tooltip {...tooltipProps}>
+ <span>{link}</span>
+ </Tooltip>
+ ) : (
+ link
+ );
};
export default Object.assign(Link, {
diff --git a/frontend/src/metabase/lib/errors.ts b/frontend/src/metabase/lib/errors.ts
new file mode 100644
index 00000000000..37d725514a6
--- /dev/null
+++ b/frontend/src/metabase/lib/errors.ts
@@ -0,0 +1,3 @@
+export const SERVER_ERROR_TYPES = {
+ missingPermissions: "missing-required-permissions",
+};
diff --git a/frontend/src/metabase/query_builder/components/view/ViewHeader.jsx b/frontend/src/metabase/query_builder/components/view/ViewHeader.jsx
index a3be9928c4f..1e761eaaafc 100644
--- a/frontend/src/metabase/query_builder/components/view/ViewHeader.jsx
+++ b/frontend/src/metabase/query_builder/components/view/ViewHeader.jsx
@@ -4,6 +4,7 @@ import { t } from "ttag";
import cx from "classnames";
import * as Urls from "metabase/lib/urls";
+import { SERVER_ERROR_TYPES } from "metabase/lib/errors";
import MetabaseSettings from "metabase/lib/settings";
import ButtonBar from "metabase/components/ButtonBar";
@@ -408,6 +409,10 @@ function ViewTitleHeaderRightSide(props) {
const isNewQuery = !query.hasData();
const hasSaveButton = !isDataset && !!isDirty && (isNewQuery || canEditQuery);
+ const isMissingPermissions =
+ result?.error_type === SERVER_ERROR_TYPES.missingPermissions;
+ const hasRunButton =
+ isRunnable && !isNativeEditorOpen && !isMissingPermissions;
return (
<div
@@ -416,7 +421,12 @@ function ViewTitleHeaderRightSide(props) {
>
{hasSaveButton && (
<SaveButton
- disabled={!question.canRun()}
+ disabled={!question.canRun() || !canEditQuery}
+ tooltip={{
+ tooltip: t`You don't have permission to save this question.`,
+ isEnabled: !canEditQuery,
+ placement: "left",
+ }}
data-metabase-event={
isShowingNotebook
? `Notebook Mode; Click Save`
@@ -478,7 +488,7 @@ function ViewTitleHeaderRightSide(props) {
/>
)}
{hasExploreResultsLink && <ExploreResultsLink question={question} />}
- {isRunnable && !isNativeEditorOpen && (
+ {hasRunButton && (
<RunButtonWithTooltip
className={cx("text-brand-hover text-dark hide", {
"sm-show": !isShowingNotebook || isNative,
diff --git a/frontend/test/metabase/scenarios/permissions/reproductions/22447-illogical-UI-elements-for-nodata.cy.spec.js b/frontend/test/metabase/scenarios/permissions/reproductions/22447-illogical-UI-elements-for-nodata.cy.spec.js
index 00054670ec9..a3814dd4221 100644
--- a/frontend/test/metabase/scenarios/permissions/reproductions/22447-illogical-UI-elements-for-nodata.cy.spec.js
+++ b/frontend/test/metabase/scenarios/permissions/reproductions/22447-illogical-UI-elements-for-nodata.cy.spec.js
@@ -1,9 +1,9 @@
-import { restore, visitQuestion, isEE } from "__support__/e2e/cypress";
+import { restore, visitQuestion, isEE, popover } from "__support__/e2e/cypress";
import { USER_GROUPS, SAMPLE_DB_ID } from "__support__/e2e/cypress_data";
const { ALL_USERS_GROUP, COLLECTION_GROUP } = USER_GROUPS;
-describe.skip("UI elements that make no sense for users without data permissions (metabase#22447, metabase##22449, metabase#22450)", () => {
+describe("UI elements that make no sense for users without data permissions (metabase#22447, metabase##22449, metabase#22450)", () => {
beforeEach(() => {
restore();
});
@@ -20,23 +20,28 @@ describe.skip("UI elements that make no sense for users without data permissions
cy.icon("line").click();
cy.findByTextEnsureVisible("Line options");
- cy.findByText("Save").should("not.exist");
+ cy.findByText("Save")
+ .as("saveButton")
+ .invoke("css", "pointer-events")
+ .should("equal", "none");
- // TODO: Please uncoment this part when metabase#22449 gets fixed
- // cy.icon("refresh").should("not.exist");
+ cy.get("@saveButton").realHover();
+ cy.findByText("You don't have permission to save this question.");
- // TODO: Please uncoment this part when metabase#22450 gets fixed
- // cy.visit("/collection/root");
+ cy.findByTestId("qb-header-action-panel").within(() => {
+ cy.icon("refresh").should("not.exist");
+ });
+
+ cy.visit("/collection/root");
- // cy.get("main")
- // .find(".Icon-add")
- // .click();
+ cy.get("main")
+ .find(".Icon-add")
+ .click();
- // Do not forget to import popover
- // popover()
- // .should("contain", "Dashboard")
- // .and("contain", "Collection")
- // .and("not.contain", "Question");
+ popover()
+ .should("contain", "Dashboard")
+ .and("contain", "Collection")
+ .and("not.contain", "Question");
});
it("should not show visualization or question settings to users with block data permissions", () => {
@@ -61,20 +66,18 @@ describe.skip("UI elements that make no sense for users without data permissions
cy.findByText("Settings").should("not.exist");
cy.findByText("Visualization").should("not.exist");
- // TODO: Please uncoment this part when metabase#22449 gets fixed
- // cy.icon("refresh").should("not.exist");
-
- // TODO: Please uncoment this part when metabase#22450 gets fixed
- // cy.visit("/collection/root");
+ cy.findByTestId("qb-header-action-panel").within(() => {
+ cy.icon("refresh").should("not.exist");
+ });
+ cy.visit("/collection/root");
- // cy.get("main")
- // .find(".Icon-add")
- // .click();
+ cy.get("main")
+ .find(".Icon-add")
+ .click();
- // Do not forget to import popover
- // popover()
- // .should("contain", "Dashboard")
- // .and("contain", "Collection")
- // .and("not.contain", "Question");
+ popover()
+ .should("contain", "Dashboard")
+ .and("contain", "Collection")
+ .and("not.contain", "Question");
});
});
--
GitLab