From 94081aeadd00fa188e6836d907356edf2fce0357 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Wed, 23 Oct 2024 15:15:24 -0400
Subject: [PATCH] Expose model persistence refresh icon when model is still in
creating state (#48961)
Co-authored-by: Ryan Laurie <iethree@gmail.com>
---
.../ModelCacheControl/ModelCacheControl.tsx | 18 +++--
frontend/src/metabase-lib/v1/Question.ts | 4 ++
.../metabase-lib/v1/metadata/utils/models.ts | 2 +-
.../v1/metadata/utils/models.unit.spec.js | 2 +-
frontend/src/metabase-types/api/card.ts | 1 +
frontend/src/metabase-types/api/mocks/card.ts | 1 +
.../QueryActionContextProvider.tsx | 1 +
.../ModelCacheRefreshJobs.unit.spec.tsx | 2 +-
.../ModelCacheManagementSection.styled.tsx | 43 ------------
.../ModelCacheManagementSection.tsx | 68 +++++++++++--------
.../ModelCacheManagementSection.unit.spec.tsx | 10 ++-
src/metabase/api/card.clj | 7 +-
src/metabase/api/persist.clj | 2 +-
src/metabase/models/card.clj | 15 ++++
test/metabase/api/card_test.clj | 10 ++-
15 files changed, 101 insertions(+), 85 deletions(-)
delete mode 100644 frontend/src/metabase/query_builder/components/view/sidebars/ModelCacheManagementSection/ModelCacheManagementSection.styled.tsx
diff --git a/enterprise/frontend/src/metabase-enterprise/model_persistence/components/ModelCacheControl/ModelCacheControl.tsx b/enterprise/frontend/src/metabase-enterprise/model_persistence/components/ModelCacheControl/ModelCacheControl.tsx
index fc56ba74b16..3df346c03b8 100644
--- a/enterprise/frontend/src/metabase-enterprise/model_persistence/components/ModelCacheControl/ModelCacheControl.tsx
+++ b/enterprise/frontend/src/metabase-enterprise/model_persistence/components/ModelCacheControl/ModelCacheControl.tsx
@@ -36,18 +36,22 @@ export function ModelCacheToggle({
const isPersisted = persistedModel && persistedModel.state !== "off";
const modelId = model.id();
- const canPersist = database?.settings?.["persist-models-enabled"];
+ const userCanPersist = model.canManageDB();
+ const canPersistDatabase = database?.settings?.["persist-models-enabled"];
+
+ if (!canPersistDatabase || !userCanPersist) {
+ const tooltipLabel = !canPersistDatabase
+ ? t`Model persistence is disabled for this database`
+ : t`You don't have permission to modify model persistence`;
- if (!canPersist) {
return (
- <Tooltip label={t`Model persistence is disabled for this database`}>
+ <Tooltip label={tooltipLabel}>
+ {/* need this div so that disabled input doesn't swallow pointer events */}
<div>
- {" "}
- {/* need this element so that disabled input doesn't swallow pointer events */}
<Switch
label={t`Persist model data`}
size="sm"
- checked={false}
+ checked={isPersisted}
disabled
/>
</div>
@@ -65,7 +69,7 @@ export function ModelCacheToggle({
size="sm"
checked={isPersisted}
onChange={toggleModelPersistence}
- disabled={!canPersist}
+ disabled={false}
/>
);
}
diff --git a/frontend/src/metabase-lib/v1/Question.ts b/frontend/src/metabase-lib/v1/Question.ts
index 3799c6365bf..a71d30eec3a 100644
--- a/frontend/src/metabase-lib/v1/Question.ts
+++ b/frontend/src/metabase-lib/v1/Question.ts
@@ -821,6 +821,10 @@ class Question {
return getIn(this, ["_card", "created_at"]) || "";
}
+ canManageDB(): boolean {
+ return this.card().can_manage_db || false;
+ }
+
/**
* TODO Atte Keinänen 6/13/17: Discussed with Tom that we could use the default Question constructor instead,
* but it would require changing the constructor signature so that `card` is an optional parameter and has a default value
diff --git a/frontend/src/metabase-lib/v1/metadata/utils/models.ts b/frontend/src/metabase-lib/v1/metadata/utils/models.ts
index 2ff77730a32..8599558fbc8 100644
--- a/frontend/src/metabase-lib/v1/metadata/utils/models.ts
+++ b/frontend/src/metabase-lib/v1/metadata/utils/models.ts
@@ -130,7 +130,7 @@ export function checkCanRefreshModelCache(
return false;
}
- return refreshInfo.state === "persisted" || refreshInfo.state === "error";
+ return ["creating", "persisted", "error"].includes(refreshInfo.state);
}
export function getModelCacheSchemaName(databaseId: number, siteUUID: string) {
diff --git a/frontend/src/metabase-lib/v1/metadata/utils/models.unit.spec.js b/frontend/src/metabase-lib/v1/metadata/utils/models.unit.spec.js
index f107d3747ea..6ad6740f2c2 100644
--- a/frontend/src/metabase-lib/v1/metadata/utils/models.unit.spec.js
+++ b/frontend/src/metabase-lib/v1/metadata/utils/models.unit.spec.js
@@ -275,7 +275,7 @@ describe("data model utils", () => {
describe("checkCanRefreshModelCache", () => {
const testCases = {
- creating: false,
+ creating: true,
refreshing: false,
persisted: true,
error: true,
diff --git a/frontend/src/metabase-types/api/card.ts b/frontend/src/metabase-types/api/card.ts
index babfb5c17ef..1bd6e2148a1 100644
--- a/frontend/src/metabase-types/api/card.ts
+++ b/frontend/src/metabase-types/api/card.ts
@@ -39,6 +39,7 @@ export interface Card<Q extends DatasetQuery = DatasetQuery>
can_write: boolean;
can_restore: boolean;
can_delete: boolean;
+ can_manage_db: boolean;
initially_published_at: string | null;
database_id?: DatabaseId;
diff --git a/frontend/src/metabase-types/api/mocks/card.ts b/frontend/src/metabase-types/api/mocks/card.ts
index 88c0a7671eb..56c5c801ca1 100644
--- a/frontend/src/metabase-types/api/mocks/card.ts
+++ b/frontend/src/metabase-types/api/mocks/card.ts
@@ -45,6 +45,7 @@ export const createMockCard = (opts?: Partial<Card>): Card => ({
enable_embedding: false,
embedding_params: null,
initially_published_at: null,
+ can_manage_db: true,
...opts,
});
diff --git a/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/QueryActionContextProvider/QueryActionContextProvider.tsx b/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/QueryActionContextProvider/QueryActionContextProvider.tsx
index d124492333a..adba64c0d85 100644
--- a/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/QueryActionContextProvider/QueryActionContextProvider.tsx
+++ b/frontend/src/metabase/actions/containers/ActionCreator/ActionContext/QueryActionContextProvider/QueryActionContextProvider.tsx
@@ -80,6 +80,7 @@ function convertActionToQuestionCard(
enable_embedding: false,
embedding_params: null,
initially_published_at: null,
+ can_manage_db: true,
};
}
diff --git a/frontend/src/metabase/admin/tasks/containers/ModelCacheRefreshJobs/ModelCacheRefreshJobs.unit.spec.tsx b/frontend/src/metabase/admin/tasks/containers/ModelCacheRefreshJobs/ModelCacheRefreshJobs.unit.spec.tsx
index b8e20e5e22e..b589b33c720 100644
--- a/frontend/src/metabase/admin/tasks/containers/ModelCacheRefreshJobs/ModelCacheRefreshJobs.unit.spec.tsx
+++ b/frontend/src/metabase/admin/tasks/containers/ModelCacheRefreshJobs/ModelCacheRefreshJobs.unit.spec.tsx
@@ -102,7 +102,7 @@ describe("ModelCacheRefreshJobs", () => {
it("displays 'creating' state correctly", async () => {
await setup({ logs: [getMockModelCacheInfo({ state: "creating" })] });
expect(screen.getByText("Queued")).toBeInTheDocument();
- expect(screen.queryByLabelText("refresh icon")).not.toBeInTheDocument();
+ expect(screen.getByLabelText("refresh icon")).toBeInTheDocument();
});
it("displays 'refreshing' state correctly", async () => {
diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/ModelCacheManagementSection/ModelCacheManagementSection.styled.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/ModelCacheManagementSection/ModelCacheManagementSection.styled.tsx
deleted file mode 100644
index 45aa39af02e..00000000000
--- a/frontend/src/metabase/query_builder/components/view/sidebars/ModelCacheManagementSection/ModelCacheManagementSection.styled.tsx
+++ /dev/null
@@ -1,43 +0,0 @@
-import styled from "@emotion/styled";
-
-import { Icon } from "metabase/ui";
-
-export const Row = styled.div`
- display: flex;
- justify-content: space-between;
- align-items: center;
-`;
-
-export const StatusContainer = styled.div`
- display: flex;
- align-items: center;
-`;
-
-export const StatusLabel = styled.span`
- font-size: 0.875rem;
- font-weight: bold;
- color: var(--mb-color-text-dark);
-`;
-
-export const LastRefreshTimeLabel = styled.span`
- display: block;
- font-size: 0.875rem;
- font-weight: 400;
- color: var(--mb-color-text-medium);
- margin-top: 4px;
-`;
-
-export const IconButton = styled.button`
- display: flex;
- cursor: pointer;
-`;
-
-export const ErrorIcon = styled(Icon)`
- color: var(--mb-color-error);
- margin-top: 1px;
- margin-left: 4px;
-`;
-
-export const RefreshIcon = styled(Icon)`
- color: var(--mb-color-text-dark);
-`;
diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/ModelCacheManagementSection/ModelCacheManagementSection.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/ModelCacheManagementSection/ModelCacheManagementSection.tsx
index f15c70341fd..715ae45b2d5 100644
--- a/frontend/src/metabase/query_builder/components/view/sidebars/ModelCacheManagementSection/ModelCacheManagementSection.tsx
+++ b/frontend/src/metabase/query_builder/components/view/sidebars/ModelCacheManagementSection/ModelCacheManagementSection.tsx
@@ -7,20 +7,11 @@ import {
} from "metabase/api";
import { DelayedLoadingAndErrorWrapper } from "metabase/components/LoadingAndErrorWrapper/DelayedLoadingAndErrorWrapper";
import { PLUGIN_MODEL_PERSISTENCE } from "metabase/plugins";
+import { Box, Button, Flex, Icon } from "metabase/ui";
import type Question from "metabase-lib/v1/Question";
import { checkCanRefreshModelCache } from "metabase-lib/v1/metadata/utils/models";
import type { ModelCacheRefreshStatus } from "metabase-types/api";
-import {
- ErrorIcon,
- IconButton,
- LastRefreshTimeLabel,
- RefreshIcon,
- Row,
- StatusContainer,
- StatusLabel,
-} from "./ModelCacheManagementSection.styled";
-
type Props = {
model: Question;
};
@@ -59,6 +50,21 @@ export function ModelCacheManagementSection({ model }: Props) {
const isError = persistedModel?.state === "error";
const lastRefreshTime = moment(persistedModel?.refresh_end).fromNow();
+ const canRefreshCache =
+ persistedModel && checkCanRefreshModelCache(persistedModel);
+
+ const refreshButtonLabel =
+ persistedModel?.state === "creating" ? (
+ t`Create now`
+ ) : (
+ <Icon name="refresh" tooltip={t`Refresh now`} />
+ );
+
+ const canManageDB = model.canManageDB();
+
+ const statusMessage = persistedModel ? getStatusMessage(persistedModel) : "";
+ const lastRefreshLabel = t`Last attempt ${lastRefreshTime}`;
+
return (
<>
{
@@ -69,24 +75,32 @@ export function ModelCacheManagementSection({ model }: Props) {
}
{shouldShowRefreshStatus && (
- <Row data-testid="model-cache-section">
- <div>
- <StatusContainer>
- <StatusLabel>{getStatusMessage(persistedModel)}</StatusLabel>
- {isError && <ErrorIcon name="warning" />}
- </StatusContainer>
- {isError && (
- <LastRefreshTimeLabel>
- {t`Last attempt ${lastRefreshTime}`}
- </LastRefreshTimeLabel>
- )}
- </div>
- {checkCanRefreshModelCache(persistedModel) && (
- <IconButton onClick={() => onRefresh(model.id())}>
- <RefreshIcon name="refresh" tooltip={t`Refresh now`} />
- </IconButton>
+ <Flex
+ justify="space-between"
+ align="center"
+ data-testid="model-cache-section"
+ c={canManageDB ? "text-dark" : "text-light"}
+ fz="md"
+ >
+ <Box>
+ <Flex align="center" fw="bold" gap="sm">
+ {statusMessage}
+ {isError && <Icon name="warning" c="error" ml="sm" />}
+ </Flex>
+ {isError && <Box pt="sm">{lastRefreshLabel}</Box>}
+ </Box>
+ {canRefreshCache && canManageDB && (
+ <Button
+ variant="subtle"
+ p="xs"
+ c="text-dark"
+ size="xs"
+ onClick={() => onRefresh(model.id())}
+ >
+ {refreshButtonLabel}
+ </Button>
)}
- </Row>
+ </Flex>
)}
</>
);
diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/ModelCacheManagementSection/ModelCacheManagementSection.unit.spec.tsx b/frontend/src/metabase/query_builder/components/view/sidebars/ModelCacheManagementSection/ModelCacheManagementSection.unit.spec.tsx
index d4880bc11c3..8332bb9fa4c 100644
--- a/frontend/src/metabase/query_builder/components/view/sidebars/ModelCacheManagementSection/ModelCacheManagementSection.unit.spec.tsx
+++ b/frontend/src/metabase/query_builder/components/view/sidebars/ModelCacheManagementSection/ModelCacheManagementSection.unit.spec.tsx
@@ -28,10 +28,12 @@ const ordersTable = checkNotNull(metadata.table(ORDERS_ID));
type SetupOpts = Partial<ModelCacheRefreshStatus> & {
waitForSectionAppearance?: boolean;
+ canManageDB?: boolean;
};
async function setup({
waitForSectionAppearance = true,
+ canManageDB = true,
...cacheInfo
}: SetupOpts = {}) {
const question = ordersTable.question();
@@ -40,6 +42,7 @@ async function setup({
id: 1,
name: "Order model",
type: "model",
+ can_manage_db: canManageDB,
});
const modelCacheInfo = getMockModelCacheInfo({
@@ -88,7 +91,7 @@ describe("ModelCacheManagementSection", () => {
expect(
await screen.findByText("Waiting to create the first model cache"),
).toBeInTheDocument();
- expect(screen.queryByLabelText("refresh icon")).not.toBeInTheDocument();
+ expect(await screen.findByText("Create now")).toBeInTheDocument();
});
it("displays 'refreshing' state correctly", async () => {
@@ -138,4 +141,9 @@ describe("ModelCacheManagementSection", () => {
// get, post, get
await waitFor(() => expect(fetchMock.calls().length).toBe(3));
});
+
+ it("disables refresh when DB management is not available to the user", async () => {
+ await setup({ state: "persisted", canManageDB: false });
+ expect(screen.queryByLabelText("refresh icon")).not.toBeInTheDocument();
+ });
});
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index bc7e0210d48..00333ce294e 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -191,10 +191,13 @@
:parameter_usage_count
:can_restore
:can_delete
+ :can_manage_db
[:collection :is_personal]
[:moderation_reviews :moderator_details])
- (cond-> ; card
- (card/model? card) (t2/hydrate :persisted)))))
+ (cond->
+ (card/model? card) (t2/hydrate :persisted
+ ;; can_manage_db determines whether we should enable model persistence settings
+ :can_manage_db)))))
(defn get-card
"Get `Card` with ID."
diff --git a/src/metabase/api/persist.clj b/src/metabase/api/persist.clj
index a614ac69c56..b3c8a68b4d4 100644
--- a/src/metabase/api/persist.clj
+++ b/src/metabase/api/persist.clj
@@ -96,7 +96,7 @@
[card-id]
{card-id [:maybe ms/PositiveInt]}
(api/let-404 [persisted-info (first (fetch-persisted-info {:card-id card-id} nil nil))]
- (api/write-check (t2/select-one Database :id (:database_id persisted-info)))
+ (api/read-check (t2/select-one Database :id (:database_id persisted-info)))
persisted-info))
(def ^:private CronSchedule
diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj
index 68680521a15..732fdd814f1 100644
--- a/src/metabase/models/card.clj
+++ b/src/metabase/models/card.clj
@@ -21,6 +21,7 @@
[metabase.models.audit-log :as audit-log]
[metabase.models.card.metadata :as card.metadata]
[metabase.models.collection :as collection]
+ [metabase.models.data-permissions :as data-perms]
[metabase.models.field-values :as field-values]
[metabase.models.interface :as mi]
[metabase.models.moderation-review :as moderation-review]
@@ -193,6 +194,20 @@
[cards]
(with-can-run-adhoc-query cards))
+;; note: perms lookup here in the course of fetching a card/model should hit a cache pre-warmed by
+;; the `:can_run_adhoc_query` above
+(mi/define-batched-hydration-method add-can-manage-db
+ :can_manage_db
+ "Hydrate can_manage_db onto cards. Indicates whether the current user has access to the database admin page for the
+ database powering this card."
+ [cards]
+ (map
+ (fn [card]
+ (assoc card
+ :can_manage_db
+ (data-perms/user-has-permission-for-database? api/*current-user-id* :perms/manage-database :yes (:database_id card))))
+ cards))
+
(methodical/defmethod t2/batched-hydrate [:model/Card :parameter_usage_count]
[_model k cards]
(mi/instances-with-hydrated-data
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index f670cabe807..102a60a85c7 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -2657,7 +2657,7 @@
(update-card card {:description "a new description"})
(is (empty? (reviews card)))))
(testing "Does not add nil moderation reviews when there are reviews but not verified"
- ;; testing that we aren't just adding a nil moderation each time we update a card
+ ;; testing that we aren't just adding a nil moderation each time we update a card
(with-card :verified
(is (verified? card))
(moderation-review/create-review! {:moderated_item_id (u/the-id card)
@@ -3642,6 +3642,14 @@
(is (=? {:can_run_adhoc_query false}
(mt/user-http-request :crowberto :get 200 (str "card/" (:id no-query))))))))
+(deftest can-manage-db-test
+ (mt/with-temp [:model/Card card {:type :model}]
+ (mt/with-no-data-perms-for-all-users!
+ (is (=? {:can_manage_db true}
+ (mt/user-http-request :crowberto :get 200 (str "card/" (:id card)))))
+ (is (=? {:can_manage_db false}
+ (mt/user-http-request :rasta :get 200 (str "card/" (:id card))))))))
+
(deftest data-and-collection-query-permissions-test
(mt/with-temp [:model/Collection collection {}
:model/Card card {:dataset_query {:database (mt/id)
--
GitLab