From 122fb292de8116a0c0ec9c83519c5994463874a7 Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Tue, 30 Aug 2022 09:34:56 -0600
Subject: [PATCH] [Apps] Hydrate action_id on is_write Card (#25075)
* [Apps] Hydrate action_id on is_write Card
It'll be helpful for the frontend if we send back the created action_id
when saving an `is_write` Card.
Also update the `api/card` tests to ensure that a QueryAction (and by
fk constraints the Action) is created/deleted as expected when
changing `is_write`.
* Add extra arg to more result-fns
---
src/metabase/api/card.clj | 6 +++--
src/metabase/models/action.clj | 13 ++++++++++
test/metabase/api/card_test.clj | 43 +++++++++++++++++++++------------
3 files changed, 45 insertions(+), 17 deletions(-)
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index f7a60afab6c..43f27409dd3 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -163,7 +163,7 @@
(cond-> ;; card
api/*is-superuser?* (hydrate [:emitters [:action :card]])
(:dataset raw-card) (hydrate :persisted)
- (:is_write raw-card) (hydrate :card/emitter-usages))
+ (:is_write raw-card) (hydrate :card/emitter-usages :card/action-id))
api/read-check
(last-edit/with-last-edit-info :card))]
(u/prog1 card
@@ -361,6 +361,8 @@ saved later when it is ready."
:average_query_time
:last_query_start
:collection [:moderation_reviews :moderator_details])
+ (cond-> ;; card
+ (:is_write card) (hydrate :card/action-id))
(assoc :last-edit-info (last-edit/edit-information-for-user @api/*current-user*)))
(when timed-out?
(schedule-metadata-saving result-metadata-chan <>)))))
@@ -596,7 +598,7 @@ saved later when it is ready."
:collection [:moderation_reviews :moderator_details])
(cond-> ;; card
(:dataset card) (hydrate :persisted)
- (:is_write card) (hydrate :card/emitter-usages))
+ (:is_write card) (hydrate :card/emitter-usages :card/action-id))
(assoc :last-edit-info (last-edit/edit-information-for-user @api/*current-user*)))))
(api/defendpoint PUT "/:id"
diff --git a/src/metabase/models/action.clj b/src/metabase/models/action.clj
index 96e0e3a2005..84a59413932 100644
--- a/src/metabase/models/action.clj
+++ b/src/metabase/models/action.clj
@@ -138,3 +138,16 @@
(for [{emitter-id :id, :as emitter} emitters]
(some-> emitter (assoc :action (get actions-by-id (get action-id-by-emitter-id emitter-id))))))
emitters))
+
+(defn cards-by-action-id
+ "Hydrates action_id from Card for is_write cards"
+ {:batched-hydrate :card/action-id}
+ [cards]
+ (if-let [card-id->action-id (not-empty (db/select-field->field
+ :card_id :action_id
+ 'QueryAction
+ :card_id [:in (map :id cards)]))]
+
+ (for [card cards]
+ (m/assoc-some card :action_id (get card-id->action-id (:id card))))
+ cards))
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 696fe70a6f1..6b2bf7f02fd 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -2260,7 +2260,7 @@
(defmacro ^:private with-actions-enabled {:style/indent 0} [& body]
`(do-with-actions-enabled (fn [] ~@body)))
-(defn- test-update-is-write-card [{:keys [user query status-code before-fn result-fn]
+(defn- test-update-is-write-card [{:keys [user query status-code before-fn result-fn get-fn]
:or {user :crowberto
query (mt/native-query {:query "UPDATE whatever SET whatever = {{whatever}};"})}}]
(testing "PUT /api/card/:id"
@@ -2274,7 +2274,7 @@
(when before-fn
(before-fn (db/select-one Card :id card-id)))
(let [result (mt/user-http-request user :put status-code (str "card/" card-id) {:is_write new-value})]
- (result-fn result))
+ (result-fn result new-value))
(let [fail? (>= status-code 400)
expected-value (if fail?
initial-value
@@ -2283,8 +2283,11 @@
(is (= expected-value
(db/select-one-field :is_write Card :id card-id))))
(testing "GET /api/card/:id value"
- (is (partial= {:is_write expected-value}
- (mt/user-http-request :crowberto :get 200 (str "card/" card-id)))))
+ (let [get-result (mt/user-http-request :crowberto :get 200 (str "card/" card-id))]
+ (is (partial= {:is_write expected-value}
+ get-result))
+ (when get-fn
+ (get-fn get-result expected-value))))
(when fail?
(testing "\nNo-op update should be allowed."
(is (some? (mt/user-http-request user :put 200 (str "card/" card-id) {:is_write initial-value})))))))))))
@@ -2297,7 +2300,7 @@
(let [result (mt/user-http-request user :post status-code "card" (merge (mt/with-temp-defaults Card)
{:is_write true
:dataset_query query}))]
- (result-fn result)
+ (result-fn result true)
(when (map? result)
(when-let [card-id (:id result)]
(let [fail? (>= status-code 400)]
@@ -2313,7 +2316,7 @@
(doseq [f [test-update-is-write-card
test-create-is-write-card]]
(f {:status-code 400
- :result-fn (fn [result]
+ :result-fn (fn [result _]
(is (= {:errors {:is_write "Cannot mark Saved Question as 'is_write': Actions are not enabled."}}
result)))})))))
@@ -2326,7 +2329,7 @@
(doseq [f [test-update-is-write-card
test-create-is-write-card]]
(f {:status-code 400
- :result-fn (fn [result]
+ :result-fn (fn [result _]
(is (schema= {:errors {:is_write #"Cannot mark Saved Question as 'is_write': Actions are not enabled for Database [\d,]+\."}}
result)))})))))
@@ -2342,7 +2345,7 @@
(doseq [f [test-update-is-write-card
test-create-is-write-card]]
(f {:status-code 400
- :result-fn (fn [result]
+ :result-fn (fn [result _]
(is (schema= {:errors {:is_write #"Cannot mark Saved Question as 'is_write': Actions are not enabled for Database [\d,]+\."}}
result)))})))))
@@ -2352,7 +2355,7 @@
{:before-fn (fn [{card-id :id}]
(db/update! Card card-id :dataset true))
:status-code 400
- :result-fn (fn [result]
+ :result-fn (fn [result _]
(is (= {:errors {:is_write "Cannot mark Saved Question as 'is_write': Saved Question is a Dataset."}}
result)))})))
@@ -2362,7 +2365,7 @@
test-create-is-write-card]]
(f {:status-code 403
:user :rasta
- :result-fn (fn [result]
+ :result-fn (fn [result _]
(is (= "You don't have permissions to do that."
result)))}))))
@@ -2372,7 +2375,7 @@
test-create-is-write-card]]
(f {:status-code 400
:query (mt/mbql-query venues)
- :result-fn (fn [result]
+ :result-fn (fn [result _]
(is (schema= {:errors {:is_write #"Cannot mark Saved Question as 'is_write': Query must be a native query."}}
result)))}))))
@@ -2380,11 +2383,21 @@
(with-actions-enabled
(doseq [f [test-update-is-write-card
test-create-is-write-card]]
- ;; TODO -- Setting `is_write` also needs to create the `Action` and `QueryAction`. Unsetting should delete those
- ;; rows. Add tests for these once that code is in place.
(f {:status-code 200
- :result-fn (fn [result]
- (is (map? result)))}))))
+ :get-fn (fn [result is-write]
+ (if is-write
+ (is (contains? result :action_id))
+ (is (not (contains? result :action_id)))))
+ :result-fn (fn [result is-write]
+ (if is-write
+ (do
+ (is (contains? result :action_id))
+ (is (some? (db/select-one 'QueryAction :card_id (:id result)))))
+ (do
+ (is (not (contains? result :action_id)))
+ (is (nil? (db/select-one 'QueryAction :card_id (:id result))))))
+ (is (map? result)))}))))
+
(defn- do-with-persistence-setup [f]
;; mt/with-temp-scheduler actually just reuses the current scheduler. The scheduler factory caches by name set in
;; the resources/quartz.properties file and we reuse that scheduler
--
GitLab