Expose model persistence refresh icon when model is still in creating state (#48961)

Co-authored-by: Ryan Laurie <iethree@gmail.com>

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}
     />
   );
 }

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

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) {

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,

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;

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,
 });
 

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,
   };
 }
 

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 () => {

frontend/src/metabase/query_builder/components/view/sidebars/ModelCacheManagementSection/ModelCacheManagementSection.styled.tsx

-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);
-`;

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>
       )}
     </>
   );

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();
+  });
 });

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."

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

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

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)