From e6aded3d0be9524a8d352a7afaafbbc6a957cb3f Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Mon, 25 Jul 2022 12:22:18 -0600
Subject: [PATCH] [Actions] Add emitter and action tests (#24199)
* wip
* Update kondo hook for with-temp*
`with-temp*` allows for unpaired bindings if you just want the default
model. This would break the `let` that the hook was binding if you had
an unpaired temp model followed by others.
* Adding tests for emitters and actions
Test hydration of :emitters
Test hydration of :action
Test http emitter execution
Test emitter crud
Test failure cases around emitter execution
Small consistency changes made to non-test code that shouldn't affect happy path FE code, largely 404 checking and keyword/string consistency.
* Fix unused requires
* Fix macro resolution
---
.clj-kondo/config.edn | 4 +
.clj-kondo/hooks/toucan/util/test.clj | 4 +-
.lsp/config.edn | 2 +-
src/metabase/actions/http_action.clj | 64 +++++++++-------
src/metabase/api/action.clj | 6 +-
src/metabase/api/emitter.clj | 48 +-----------
src/metabase/api/testing.clj | 8 ++
src/metabase/models/action.clj | 38 +++++++---
src/metabase/models/card.clj | 3 +-
test/metabase/actions/test_util.clj | 102 +++++++++++++++++++-------
test/metabase/api/emitter_test.clj | 94 +++++++++++++++++++++---
test/metabase/models/action_test.clj | 34 +++++++++
test/metabase/models/emitter_test.clj | 28 +++++++
13 files changed, 306 insertions(+), 129 deletions(-)
create mode 100644 test/metabase/models/action_test.clj
create mode 100644 test/metabase/models/emitter_test.clj
diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index 6ba853341a9..0f85f88d20c 100644
--- a/.clj-kondo/config.edn
+++ b/.clj-kondo/config.edn
@@ -350,6 +350,10 @@
:lint-as
{metabase-enterprise.serialization.test-util/with-random-dump-dir clojure.core/let
+ metabase.actions.test-util/with-query-action clojure.core/fn
+ metabase.actions.test-util/with-http-action clojure.core/fn
+ metabase.actions.test-util/with-card-emitter clojure.core/let
+ metabase.actions.test-util/with-dashboard-emitter clojure.core/let
metabase.api.common/let-404 clojure.core/let
metabase.db.data-migrations/defmigration clojure.core/def
metabase.db.liquibase/with-liquibase clojure.core/let
diff --git a/.clj-kondo/hooks/toucan/util/test.clj b/.clj-kondo/hooks/toucan/util/test.clj
index 2c4aca4ee92..abe0f89c78a 100644
--- a/.clj-kondo/hooks/toucan/util/test.clj
+++ b/.clj-kondo/hooks/toucan/util/test.clj
@@ -5,7 +5,9 @@
(let [pairs (partition 2 bindings)
db-refs (map first pairs)
let-stream (for [[_ binding+opts] pairs
- part (:children binding+opts)]
+ ;; if children is size 1 then ensure nil is on the right side of the let
+ :let [[binding-part opts] (:children binding+opts)]
+ part [binding-part opts]]
part)]
(api/vector-node [(api/vector-node db-refs)
(api/list-node (list* (api/token-node `let)
diff --git a/.lsp/config.edn b/.lsp/config.edn
index 06fda82f6da..52ba8320eec 100644
--- a/.lsp/config.edn
+++ b/.lsp/config.edn
@@ -2,7 +2,7 @@
:show-docs-arity-on-same-line? true
:project-specs [{:project-path "deps.edn"
:classpath-cmd ["clojure" "-A:dev:ee:ee-dev:drivers:drivers-dev" "-Spath"]}]
-
+ :clean {:ns-inner-blocks-indentation :keep}
:linters {:clojure-lsp/unused-public-var {:level :warning
:exclude-when-defined-by
#{metabase.api.common/defendpoint}}}}
diff --git a/src/metabase/actions/http_action.clj b/src/metabase/actions/http_action.clj
index c9e98b4cce3..365e394e55f 100644
--- a/src/metabase/actions/http_action.clj
+++ b/src/metabase/actions/http_action.clj
@@ -113,32 +113,40 @@
(defn execute-http-action!
"Calls an http endpoint based on action and params"
[action params->value]
- (let [{:keys [method url body headers]} (:template action)
- request {:method (keyword method)
- :url (parse-and-substitute url params->value)
- :accept :json
- :content-type :json
- :throw-exceptions false
- :headers (merge
- ;; TODO maybe we want to default Agent here? Maybe Origin/Referer?
- {"X-Metabase-Action" (:name action)}
- (-> headers
- (parse-and-substitute params->value)
- (json/decode)))
- :body (parse-and-substitute body params->value)}
- response (update (select-keys (http/request request) [:body :headers :status]) :body json/decode)
- ;; TODO this is pretty ineficient. We parse with `:as :json`, then reencode within a response
- ;; I couldn't find a way to get JSONNode out of cheshire, so we fall back to jackson.
- ;; Should jackson be added explicitly to deps.edn?
- response-node (.readTree ^ObjectMapper @object-mapper (json/generate-string response))
- error (json/parse-string (apply-json-query response-node (or (:error_handle action) ".status >= 400")))]
- (if error
- {:status 400
- :headers {"Content-Type" "application/json"}
- :body (when-not (boolean? error) error)}
- (if-some [response (some->> action :response_handle (apply-json-query response-node))]
- {:status 200
+ (try
+ (let [{:keys [method url body headers]} (:template action)
+ request {:method (keyword method)
+ :url (parse-and-substitute url params->value)
+ :accept :json
+ :content-type :json
+ :throw-exceptions false
+ :headers (merge
+ ;; TODO maybe we want to default Agent here? Maybe Origin/Referer?
+ {"X-Metabase-Action" (:name action)}
+ (-> headers
+ (parse-and-substitute params->value)
+ (json/decode)))
+ :body (parse-and-substitute body params->value)}
+ response (update (select-keys (http/request request) [:body :headers :status]) :body json/decode)
+ ;; TODO this is pretty ineficient. We parse with `:as :json`, then reencode within a response
+ ;; I couldn't find a way to get JSONNode out of cheshire, so we fall back to jackson.
+ ;; Should jackson be added explicitly to deps.edn?
+ response-node (.readTree ^ObjectMapper @object-mapper (json/generate-string response))
+ error (json/parse-string (apply-json-query response-node (or (:error_handle action) ".status >= 400")))]
+ (log/trace "Response before handle:" response)
+ (if error
+ {:status 400
:headers {"Content-Type" "application/json"}
- :body response}
- {:status 204
- :body nil}))))
+ :body (if (boolean? error)
+ {:remote-status (:status response)}
+ error)}
+ (if-some [response (some->> action :response_handle (apply-json-query response-node))]
+ {:status 200
+ :headers {"Content-Type" "application/json"}
+ :body response}
+ {:status 204
+ :body nil})))
+ (catch Exception e
+ (throw (ex-info (str "Problem building request: " (ex-message e))
+ {:template (:template action)}
+ e)))))
diff --git a/src/metabase/api/action.clj b/src/metabase/api/action.clj
index f9b55724194..c183eb72607 100644
--- a/src/metabase/api/action.clj
+++ b/src/metabase/api/action.clj
@@ -59,15 +59,13 @@
(db/delete! HTTPAction :action_id action-id)
api/generic-204-no-content)
-;; TODO -- 99% sure these are busted. See https://github.com/metabase/metabase/issues/23935
-
(api/defendpoint POST "/"
"Create a new HTTP action."
[:as {action :body}]
(when (not= "http" (:type action))
(throw (ex-info (trs "Action type is not supported") {:status-code 400 :action action})))
- (let [http-action (db/insert! HTTPAction action)]
- (if-let [action-id (:action_id http-action)]
+ (let [action-id (action/insert! action)]
+ (if action-id
(first (action/select-actions :id action-id))
;; db/insert! does not return a value when used with h2
;; so we return the most recently updated http action.
diff --git a/src/metabase/api/emitter.clj b/src/metabase/api/emitter.clj
index e1e7b6f99df..1bbc5e752ed 100644
--- a/src/metabase/api/emitter.clj
+++ b/src/metabase/api/emitter.clj
@@ -60,12 +60,14 @@
(api/defendpoint PUT "/:emitter-id"
"Endpoint to update an emitter."
[emitter-id :as {emitter :body}]
+ (api/check-404 (Emitter emitter-id))
(db/update! Emitter emitter-id emitter)
api/generic-204-no-content)
(api/defendpoint DELETE "/:emitter-id"
"Endpoint to delete an emitter."
[emitter-id]
+ (api/check-404 (Emitter emitter-id))
(db/delete! Emitter :id emitter-id)
api/generic-204-no-content)
@@ -75,52 +77,6 @@
s/Keyword s/Any}}
(su/with-api-error-message "map of parameter name or ID -> map of parameter `:value` and `:type` of the value")))
-(comment
- ;; Assuming things look like this
-;; GET native query action
-:parameters
-;;[
-;; {
-;; "id": "2fddf797-838e-81eb-4828-53f947932486",
-;; "type": "number/=",
-;; "target": [
-;; "variable",
-;; [
-;; "template-tag",
-;; "product_id"
-;; ]
-;; ],
-;; "name": "Product",
-;; "slug": "product_id"
-;; }
-;;]
-
-;; PUT dashboards/:id
-:emitters
-:parameter_mappings
-;;{
-;; "2fddf797-838e-81eb-4828-53f947932486": [
-;; "variable",
-;; [
-;; "template-tag",
-;; "product_id"
-;; ]
-;; ]
-;;}
-
-
-;; on execution
-:parameters
-;;{
-;; "2fddf797-838e-81eb-4828-53f947932486": {
-;; "value": 1,
-;; "type": "number/="
-;; }
-;;}
-
-
- )
-
(defn- execute-http-emitter!
[emitter parameters]
;; TODO check the types match
diff --git a/src/metabase/api/testing.clj b/src/metabase/api/testing.clj
index 77fde0f26c3..36b6a2bbb4a 100644
--- a/src/metabase/api/testing.clj
+++ b/src/metabase/api/testing.clj
@@ -42,4 +42,12 @@
(restore-snapshot! name)
nil)
+(api/defendpoint POST "/echo"
+ [fail :as {:keys [body]}]
+ (if fail
+ {:status 400
+ :body {:error-code "oops"}}
+ {:status 200
+ :body body}))
+
(api/define-routes)
diff --git a/src/metabase/models/action.clj b/src/metabase/models/action.clj
index 0bbf3a1f8d7..5546f0af655 100644
--- a/src/metabase/models/action.clj
+++ b/src/metabase/models/action.clj
@@ -1,5 +1,6 @@
(ns metabase.models.action
- (:require [medley.core :as m]
+ (:require [cheshire.core :as json]
+ [medley.core :as m]
[metabase.models.interface :as mi]
[metabase.util :as u]
[metabase.util.encryption :as encryption]
@@ -10,19 +11,20 @@
(models/defmodel HTTPAction :http_action)
(models/defmodel Action :action)
+(models/add-type! ::json-with-nested-parameters
+ :in (comp mi/json-in
+ (fn [template]
+ (u/update-if-exists template :parameters mi/normalize-parameters-list)))
+ :out (comp (fn [template]
+ (u/update-if-exists template :parameters (mi/catch-normalization-exceptions mi/normalize-parameters-list)))
+ mi/json-out-with-keywordization))
+
(u/strict-extend (class Action)
models/IModel
(merge models/IModelDefaults
{:types (constantly {:type :keyword})
:properties (constantly {:timestamped? true})}))
-(defn- pre-insert
- [action]
- (let [base-action (db/insert! Action (select-keys action [:type]))]
- (-> action
- (dissoc :type)
- (assoc :action_id (u/the-id base-action)))))
-
(defn- pre-update
[action]
;; All possible sub-type columns
@@ -39,8 +41,7 @@
(merge models/IModelDefaults
{:primary-key (constantly :action_id) ; This is ok as long as we're 1:1
:pre-delete pre-delete
- :pre-update pre-update
- :pre-insert pre-insert}))
+ :pre-update pre-update}))
(u/strict-extend (class QueryAction)
models/IModel
@@ -49,7 +50,22 @@
(u/strict-extend (class HTTPAction)
models/IModel
(merge Action-subtype-IModel-impl
- {:types (constantly {:template :json})}))
+ {:types (constantly {:template ::json-with-nested-parameters})}))
+
+(defn insert!
+ "Inserts an Action and related HTTPAction or QueryAction. Returns the action id."
+ [action-data]
+ (db/transaction
+ (let [action (db/insert! Action {:type (:type action-data)})
+ model (case (keyword (:type action))
+ :http HTTPAction
+ :query QueryAction)]
+ (db/execute! {:insert-into model
+ :values [(-> action-data
+ (dissoc :type)
+ (u/update-if-exists :template json/encode)
+ (assoc :action_id (:id action)))]})
+ (:id action))))
(def ^:private encrypted-json-out (comp mi/json-out-with-keywordization encryption/maybe-decrypt))
diff --git a/src/metabase/models/card.clj b/src/metabase/models/card.clj
index 93bd11910a4..574361065bd 100644
--- a/src/metabase/models/card.clj
+++ b/src/metabase/models/card.clj
@@ -217,8 +217,7 @@
(defn- create-actions-when-is-writable! [{is-write? :is_write card-id :id}]
(when is-write?
(when-not (db/select-one action/QueryAction :card_id card-id)
- (db/insert! action/QueryAction {:card_id card-id
- :type :query}))))
+ (action/insert! {:card_id card-id :type :query}))))
(defn- delete-actions-when-not-writable! [{is-write? :is_write card-id :id}]
(when (not is-write?)
diff --git a/test/metabase/actions/test_util.clj b/test/metabase/actions/test_util.clj
index f4d1fbe4218..f29320190f8 100644
--- a/test/metabase/actions/test_util.clj
+++ b/test/metabase/actions/test_util.clj
@@ -3,11 +3,14 @@
[clojure.java.jdbc :as jdbc]
[clojure.test :refer :all]
[metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
- [metabase.models
- :refer [Card CardEmitter Database Emitter EmitterAction QueryAction]]
+ [metabase.http-client :as client]
+ [metabase.models :refer [Action Card CardEmitter Dashboard DashboardEmitter
+ Database Emitter EmitterAction QueryAction]]
+ [metabase.models.action :as action]
[metabase.test :as mt]
[metabase.test.data.dataset-definitions :as defs]
[metabase.test.data.interface :as tx]
+ [metabase.test.initialize :as initialize]
[toucan.db :as db]))
(def ^:dynamic ^:private *actions-test-data-tables*
@@ -120,6 +123,8 @@
:display-name "ID"
:type :number
:required true}}}}
+ :name "Query Example"
+ :parameters [{:id "id" :type "number"}]
:is_write true}]]
(let [action-id (db/select-one-field :action_id QueryAction :card_id card-id)]
(f {:query-action-card-id card-id
@@ -135,37 +140,84 @@
[[bindings] & body]
`(do-with-query-action (fn [~bindings] ~@body)))
-(defn do-with-card-emitter
- "Impl for [[with-card-emitter]]."
- [{:keys [action-id], :as context} f]
- (mt/with-temp* [Card [{emitter-card-id :id}]
- Emitter [{emitter-id :id} {:parameter_mappings {"my_id" [:variable [:template-tag "id"]]}}]]
- (testing "Sanity check: emitter-id should be non-nil"
- (is (integer? emitter-id)))
- (testing "Sanity check: make sure parameter mappings were defined the way we'd expect"
- (is (= {:my_id [:variable [:template-tag "id"]]}
- (db/select-one-field :parameter_mappings Emitter :id emitter-id))))
- ;; these are tied to the Card and Emitter above and will get cascade deleted. We can't use `with-temp*` for them
- ;; because it doesn't seem to work with tables with compound PKs
- (db/insert! EmitterAction {:emitter_id emitter-id
- :action_id action-id})
- (db/insert! CardEmitter {:card_id emitter-card-id
- :action_id action-id})
- (f (assoc context
- :emitter-id emitter-id
- :emitter-card-id emitter-card-id))))
+(defn do-with-http-action
+ "Impl for [[with-http-action]]."
+ [f]
+ (initialize/initialize-if-needed! :web-server)
+ (mt/with-model-cleanup [Action]
+ (let [action-id (action/insert! {:type :http
+ :name "Echo Example"
+ :template {:url (client/build-url "testing/echo[[?fail={{fail}}]]" {})
+ :method :post
+ :body "{\"the_parameter\": {{id}}}"
+ :headers "{\"x-test\": \"{{id}}\"}"
+ :parameters [{:id "id" :type "number"}
+ {:id "fail" :type "text"}]}
+ :response_handle ".body"})]
+ (f {:action-id action-id}))))
+
+(defmacro with-http-action
+ "Execute `body` with a newly created QueryAction. `bindings` is a map with key `:action-id`.
+
+ (with-http-action [{:keys [action-id], :as context}]
+ (do-something))"
+ {:style/indent 1}
+ [[bindings] & body]
+ `(do-with-http-action (fn [~bindings] ~@body)))
+
+(defn do-with-emitter
+ "Impl for [[with-emitter]]."
+ [card-or-dashboard-model {:keys [action-id], :as context} f]
+ (let [parent-model (db/resolve-model card-or-dashboard-model)]
+ (mt/with-temp* [parent-model [{emitter-parent-id :id}]
+ Emitter [{emitter-id :id} {:parameter_mappings {"my_id" [:variable [:template-tag "id"]]
+ "my_fail" [:variable [:template-tag "fail"]]}}]]
+ (testing "Sanity check: emitter-id should be non-nil"
+ (is (integer? emitter-id)))
+ (testing "Sanity check: make sure parameter mappings were defined the way we'd expect"
+ (is (= {:my_id [:variable [:template-tag "id"]]
+ :my_fail [:variable [:template-tag "fail"]]}
+ (db/select-one-field :parameter_mappings Emitter :id emitter-id))))
+ ;; these are tied to the Card or Dashboad and Emitter above and will get cascade deleted. We can't use `with-temp*` for them
+ ;; because it doesn't seem to work with tables with compound PKs
+ (db/insert! EmitterAction {:emitter_id emitter-id
+ :action_id action-id})
+ (condp = (type parent-model)
+ (type Card)
+ (db/insert! CardEmitter {:card_id emitter-parent-id
+ :emitter_id emitter-id})
+ (type Dashboard)
+ (db/insert! DashboardEmitter {:dashboard_id emitter-parent-id
+ :emitter_id emitter-id}))
+ (f (assoc context
+ :emitter-id emitter-id
+ :emitter-parent-id emitter-parent-id)))))
+
+(defmacro with-dashboard-emitter
+ "Execute `body` with a newly created DashboardEmitter created for an Action with `:action-id`. Intended for use with the
+ `context` returned by with [[with-query-action]]. `bindings` is bound to a map with the keys `:emitter-id` and
+ `:emitter-parent-id` pointing to the dashboard-id.
+
+ (with-query-action [{:keys [action-id query-action-card-id], :as context}]
+ (with-dashboard-emitter [{:keys [emitter-id emitter-parent-id]} context]
+ (do-something)))"
+ {:style/indent 1
+ :arglists '([bindings {:keys [action-id], :as _action}] & body)}
+ [[bindings action] & body]
+ `(do-with-emitter Dashboard ~action (fn [~bindings] ~@body)))
(defmacro with-card-emitter
"Execute `body` with a newly created CardEmitter created for an Action with `:action-id`. Intended for use with the
`context` returned by with [[with-query-action]]. `bindings` is bound to a map with the keys `:emitter-id` and
- `:emitter-card-id`.
+ `:emitter-parent-id` pointing to the card-id.
(with-query-action [{:keys [action-id query-action-card-id], :as context}]
- (with-card-emitter [{:keys [emitter-id emitter-card-id]} context]
+ (with-card-emitter [{:keys [emitter-id emitter-parent-id]} context]
(do-something)))"
- {:style/indent 1, :arglists '([bindings {:keys [action-id], :as _action}] & body)}
+ {:style/indent 1
+ :arglists '([bindings {:keys [action-id], :as _action}] & body)}
[[bindings action] & body]
- `(do-with-card-emitter ~action (fn [~bindings] ~@body)))
+ `(do-with-emitter Card ~action (fn [~bindings] ~@body)))
(defn do-with-actions-enabled
"Impl for [[with-actions-enabled]]."
diff --git a/test/metabase/api/emitter_test.clj b/test/metabase/api/emitter_test.clj
index 12c15032770..fb37bf8601f 100644
--- a/test/metabase/api/emitter_test.clj
+++ b/test/metabase/api/emitter_test.clj
@@ -1,8 +1,9 @@
(ns metabase.api.emitter-test
(:require
+ [clojure.string :as str]
[clojure.test :refer :all]
[metabase.actions.test-util :as actions.test-util]
- [metabase.models :refer [Card Dashboard]]
+ [metabase.models :refer [Card Dashboard Emitter]]
[metabase.test :as mt]
[metabase.util.schema :as su]
[schema.core :as s]))
@@ -31,16 +32,87 @@
(mt/user-http-request :crowberto :post 200 "emitter" {:dashboard_id dashboard-id
:action_id action-id})))))))))))
-(deftest execute-custom-action-test
+(deftest update-emitter-test
+ (testing "PUT /api/emitter/:id"
+ (mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
+ (actions.test-util/with-actions-test-data-and-actions-enabled
+ (actions.test-util/with-query-action [action]
+ (actions.test-util/with-card-emitter [{:keys [emitter-id]} action]
+ (testing "Should be able to update an emitter"
+ (mt/user-http-request :crowberto :put 204 (format "emitter/%d" emitter-id)
+ {:options {:a 1}})
+ (is (partial= {:options {:a 1}} (Emitter emitter-id))))
+ (testing "Should 404 if bad emitter-id"
+ (is (= "Not found."
+ (mt/user-http-request :crowberto :put 404 (format "emitter/%d" Integer/MAX_VALUE)
+ {}))))))))))
+
+(deftest delete-emitter-test
+ (testing "DELETE /api/emitter/:id"
+ (mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
+ (actions.test-util/with-actions-test-data-and-actions-enabled
+ (actions.test-util/with-query-action [action]
+ (actions.test-util/with-card-emitter [{:keys [emitter-id]} action]
+ (testing "Should be able to delete an emitter"
+ (is (nil? (mt/user-http-request :crowberto :delete 204 (format "emitter/%d" emitter-id)))))
+ (testing "Should 404 if bad emitter-id"
+ (is (= "Not found."
+ (mt/user-http-request :crowberto :delete 404 (format "emitter/%d" Integer/MAX_VALUE)
+ {}))))))))))
+
+(deftest execute-query-action-test
+ (mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
+ (actions.test-util/with-actions-test-data-and-actions-enabled
+ (actions.test-util/with-query-action [action]
+ (actions.test-util/with-card-emitter [{:keys [emitter-id]} action]
+ (let [emitter-path (format "emitter/%d/execute" emitter-id)]
+ (testing "Should be able to execute an emitter"
+ (is (= {:rows-affected 1}
+ (mt/user-http-request :crowberto :post 200 emitter-path
+ {:parameters {"my_id" {:type :number/=
+ :value 1}}}))))
+ (is (= [1 "Bird Shop"]
+ (mt/first-row
+ (mt/run-mbql-query categories {:filter [:= $id 1]}))))
+ (testing "Should affect 0 rows if id is out of range"
+ (is (= {:rows-affected 0}
+ (mt/user-http-request :crowberto :post 200 emitter-path
+ {:parameters {"my_id" {:type :number/=
+ :value Integer/MAX_VALUE}}}))))
+ (testing "Should 404 if bad emitter-id"
+ (is (= "Not found."
+ (mt/user-http-request :crowberto :post 404 (format "emitter/%d/execute" Integer/MAX_VALUE)
+ {}))))
+ (testing "Missing parameter should fail gracefully"
+ (is (partial= {:message "Error executing QueryEmitter: Error building query parameter map: Error determining value for parameter \"id\": You'll need to pick a value for 'ID' before this query can run."}
+ (mt/user-http-request :crowberto :post 500 emitter-path
+ {:parameters {}}))))
+ (testing "Sending an invalid number should fail gracefully"
+
+ (is (partial= {:message "Error executing QueryEmitter: Error building query parameter map: Error determining value for parameter \"id\": Unparseable number: \"BAD\""}
+ (mt/user-http-request :crowberto :post 500 emitter-path
+ {:parameters {"my_id" {:type :number/= :value "BAD"}}}))))))))))
+
+(deftest execute-http-action-test
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
(actions.test-util/with-actions-test-data-and-actions-enabled
- (actions.test-util/with-query-action [context]
+ (actions.test-util/with-http-action [context]
(actions.test-util/with-card-emitter [{:keys [emitter-id]} context]
- (testing "Should be able to execute an emitter"
- (is (= {:rows-affected 1}
- (mt/user-http-request :crowberto :post 200 (format "emitter/%d/execute" emitter-id)
- {:parameters {"my_id" {:type :number/=
- :value 1}}}))))
- (is (= [1 "Bird Shop"]
- (mt/first-row
- (mt/run-mbql-query categories {:filter [:= $id 1]})))))))))
+ (let [emitter-path (format "emitter/%d/execute" emitter-id)]
+ (testing "Should be able to execute an emitter"
+ (is (= {:the_parameter 1}
+ (mt/user-http-request :crowberto :post 200 emitter-path
+ {:parameters {"my_id" {:type :number/= :value 1}}}))))
+ (testing "Should handle errors"
+ (is (= {:remote-status 400}
+ (mt/user-http-request :crowberto :post 400 emitter-path
+ {:parameters {"my_id" {:type :number/= :value 1}
+ "my_fail" {:type :text :value "true"}}}))))
+ (testing "Missing parameter should fail gracefully"
+ (is (partial= {:message "Problem building request: Cannot call the service: missing required parameters: #{\"id\"}"}
+ (mt/user-http-request :crowberto :post 500 emitter-path
+ {:parameters {}}))))
+ (testing "Sending an invalid number should fail gracefully"
+ (is (str/starts-with? (:message (mt/user-http-request :crowberto :post 500 emitter-path
+ {:parameters {"my_id" {:type :number/= :value "BAD"}}}))
+ "Problem building request:")))))))))
diff --git a/test/metabase/models/action_test.clj b/test/metabase/models/action_test.clj
new file mode 100644
index 00000000000..23654a40e50
--- /dev/null
+++ b/test/metabase/models/action_test.clj
@@ -0,0 +1,34 @@
+(ns metabase.models.action-test
+ (:require [clojure.test :refer :all]
+ [metabase.actions.test-util :as actions.test-util]
+ [metabase.models :refer [Emitter]]
+ [metabase.test :as mt]
+ [toucan.hydrate :refer [hydrate]]))
+
+(deftest hydrate-query-action-test
+ (mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
+ (actions.test-util/with-actions-test-data-and-actions-enabled
+ (actions.test-util/with-query-action [{:keys [query-action-card-id action-id] :as context}]
+ (actions.test-util/with-card-emitter [{:keys [emitter-id]} context]
+ (let [emitter (Emitter emitter-id)
+ hydrated-emitter (hydrate emitter :action)]
+ (is (partial=
+ {:id action-id
+ :name "Query Example"
+ :card {:id query-action-card-id}
+ :parameters [{:id "id" :type :number}]}
+ (:action hydrated-emitter)))))))))
+
+(deftest hydrate-http-action-test
+ (mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
+ (actions.test-util/with-actions-test-data-and-actions-enabled
+ (actions.test-util/with-http-action [{:keys [action-id] :as context}]
+ (actions.test-util/with-card-emitter [{:keys [emitter-id]} context]
+ (let [emitter (Emitter emitter-id)
+ hydrated-emitter (hydrate emitter :action)]
+ (is (partial=
+ {:id action-id
+ :name "Echo Example"
+ :parameters [{:id "id" :type :number}
+ {:id "fail" :type :text}]}
+ (:action hydrated-emitter)))))))))
diff --git a/test/metabase/models/emitter_test.clj b/test/metabase/models/emitter_test.clj
new file mode 100644
index 00000000000..1c1ac327f14
--- /dev/null
+++ b/test/metabase/models/emitter_test.clj
@@ -0,0 +1,28 @@
+(ns metabase.models.emitter-test
+ (:require [clojure.test :refer :all]
+ [metabase.actions.test-util :as actions.test-util]
+ [metabase.models :refer [Card Dashboard]]
+ [metabase.test :as mt]
+ [toucan.hydrate :refer [hydrate]]))
+
+(deftest test-hydration
+ (mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
+ (actions.test-util/with-actions-test-data-and-actions-enabled
+ (actions.test-util/with-query-action [context]
+ (actions.test-util/with-card-emitter [{:keys [emitter-id emitter-parent-id]} context]
+ (let [card (Card emitter-parent-id)
+ hydrated-card (hydrate card :emitters)]
+ (is (partial=
+ [{:id emitter-id}]
+ (:emitters hydrated-card)))))))))
+
+(deftest dashboard-emitter-hydration-test
+ (mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
+ (actions.test-util/with-actions-test-data-and-actions-enabled
+ (actions.test-util/with-query-action [context]
+ (actions.test-util/with-dashboard-emitter [{:keys [emitter-id emitter-parent-id]} context]
+ (let [dashboard (Dashboard emitter-parent-id)
+ hydrated-card (hydrate dashboard [:emitters :action])]
+ (is (partial=
+ [{:id emitter-id}]
+ (:emitters hydrated-card)))))))))
--
GitLab