diff --git a/src/metabase/api/pulse.clj b/src/metabase/api/pulse.clj
index a98340490eb007fa95de290169f8f94eb333f69a..e2bd363d97c7ef8c432af692515bb08fe5c9e6fb 100644
--- a/src/metabase/api/pulse.clj
+++ b/src/metabase/api/pulse.clj
@@ -46,7 +46,7 @@
"Create a new `Pulse`."
[:as {{:keys [name cards channels skip_if_empty collection_id collection_position]} :body}]
{name su/NonBlankString
- cards (su/non-empty [pulse/CardRef])
+ cards (su/non-empty [pulse/CoercibleToCardRef])
channels (su/non-empty [su/Map])
skip_if_empty (s/maybe s/Bool)
collection_id (s/maybe su/IntGreaterThanZero)
@@ -79,7 +79,7 @@
"Update a Pulse with `id`."
[id :as {{:keys [name cards channels skip_if_empty collection_id], :as pulse-updates} :body}]
{name (s/maybe su/NonBlankString)
- cards (s/maybe (su/non-empty [pulse/CardRef]))
+ cards (s/maybe (su/non-empty [pulse/CoercibleToCardRef]))
channels (s/maybe (su/non-empty [su/Map]))
skip_if_empty (s/maybe s/Bool)
collection_id (s/maybe su/IntGreaterThanZero)}
diff --git a/src/metabase/models/pulse.clj b/src/metabase/models/pulse.clj
index 771f55de4d9a0411c3ae95f93fd95fc04ea96a21..b3cefa148d77d14c7560b22e18e2716ff9888a34 100644
--- a/src/metabase/models/pulse.clj
+++ b/src/metabase/models/pulse.clj
@@ -14,7 +14,8 @@
functions for fetching a specific Pulse). At some point in the future, we can clean this namespace up and bring the
code in line with the rest of the codebase, but for the time being, it probably makes sense to follow the existing
patterns in this namespace rather than further confuse things."
- (:require [clojure.tools.logging :as log]
+ (:require [clojure.string :as str]
+ [clojure.tools.logging :as log]
[medley.core :as m]
[metabase
[events :as events]
@@ -27,6 +28,7 @@
[pulse-channel :as pulse-channel :refer [PulseChannel]]
[pulse-channel-recipient :refer [PulseChannelRecipient]]]
[metabase.util.schema :as su]
+ [puppetlabs.i18n.core :refer [tru]]
[schema.core :as s]
[toucan
[db :as db]
@@ -73,6 +75,42 @@
:can-write? (partial i/current-user-has-full-permissions? :write)
:perms-objects-set perms-objects-set})
+;;; ---------------------------------------------------- Schemas -----------------------------------------------------
+
+(def AlertConditions
+ "Schema for valid values of `:alert_condition` for Alerts."
+ (s/enum "rows" "goal"))
+
+(def CardRef
+ "Schema for the map we use to internally represent the fact that a Card is in a Notification and the details about its
+ presence there."
+ (su/with-api-error-message {:id su/IntGreaterThanZero
+ :include_csv s/Bool
+ :include_xls s/Bool}
+ (tru "value must be a map with the keys `{0}`, `{1}`, and `{2}`." "id" "include_csv" "include_xls")))
+
+(def HybridPulseCard
+ "This schema represents the cards that are included in a pulse. This is the data from the `PulseCard` and some
+ additional information used by the UI to display it from `Card`. This is a superset of `CardRef` and is coercible to
+ a `CardRef`"
+ (su/with-api-error-message
+ (merge (:schema CardRef)
+ {:name (s/maybe s/Str)
+ :description (s/maybe s/Str)
+ :display (s/maybe su/KeywordOrString)
+ :collection_id (s/maybe su/IntGreaterThanZero)})
+ (tru "value must be a map with the following keys `({0})`"
+ (str/join ", " ["collection_id" "description" "display" "id" "include_csv" "include_xls" "name"]))))
+
+(def CoercibleToCardRef
+ "Schema for functions accepting either a `HybridPulseCard` or `CardRef`."
+ (s/conditional
+ (fn check-hybrid-pulse-card [maybe-map]
+ (and (map? maybe-map)
+ (some #(contains? maybe-map %) [:name :description :display :collection_id])))
+ HybridPulseCard
+ :else
+ CardRef))
;;; --------------------------------------------------- Hydration ----------------------------------------------------
@@ -81,8 +119,7 @@
[notification-or-id]
(db/select PulseChannel, :pulse_id (u/get-id notification-or-id)))
-
-(defn ^:hydrate cards
+(s/defn ^:hydrate cards :- [HybridPulseCard]
"Return the Cards associated with this `notification`."
[notification-or-id]
(map (partial models/do-post-select Card)
@@ -96,22 +133,6 @@
[:= :c.archived false]]
:order-by [[:pc.position :asc]]})))
-
-;;; ---------------------------------------------------- Schemas -----------------------------------------------------
-
-(def AlertConditions
- "Schema for valid values of `:alert_condition` for Alerts."
- (s/enum "rows" "goal"))
-
-(def CardRef
- "Schema for the map we use to internally represent the fact that a Card is in a Notification and the details about its
- presence there."
- (su/with-api-error-message {:id su/IntGreaterThanZero
- :include_csv s/Bool
- :include_xls s/Bool}
- "value must be a map with the keys `id`, `include_csv`, and `include_xls`."))
-
-
;;; ---------------------------------------- Notification Fetching Helper Fns ----------------------------------------
(s/defn ^:private hydrate-notification :- PulseInstance
@@ -342,7 +363,7 @@
(s/optional-key :skip_if_empty) s/Bool
(s/optional-key :collection_id) (s/maybe su/IntGreaterThanZero)
(s/optional-key :collection_position) (s/maybe su/IntGreaterThanZero)
- (s/optional-key :cards) [CardRef]
+ (s/optional-key :cards) [CoercibleToCardRef]
(s/optional-key :channels) [su/Map]}]
(db/update! Pulse (u/get-id notification)
(u/select-keys-when notification
diff --git a/src/metabase/util/schema.clj b/src/metabase/util/schema.clj
index 75346c8b325c49751db8ca0be49e2a830dd22bab..195f16fe49953549ae310f8d6f2f1bb796a4ca56 100644
--- a/src/metabase/util/schema.clj
+++ b/src/metabase/util/schema.clj
@@ -50,6 +50,13 @@
(instance? java.util.regex.Pattern existing-schema) (tru "value must be a string that matches the regex `{0}`."
existing-schema)))
+(declare api-error-message)
+
+(defn- create-cond-schema-message [child-schemas]
+ (str (tru "value must satisfy one of the following requirements: ")
+ (str/join " " (for [[i child-schema] (m/indexed child-schemas)]
+ (format "%d) %s" (inc i) (api-error-message child-schema))))))
+
(defn api-error-message
"Extract the API error messages attached to a schema, if any.
This functionality is fairly sophisticated:
@@ -74,13 +81,16 @@
;; 1) value must be a boolean.
;; 2) value must be a valid boolean string ('true' or 'false').
(when (instance? schema.core.CondPre schema)
- (str (tru "value must satisfy one of the following requirements: ")
- (str/join " " (for [[i child-schema] (m/indexed (:schemas schema))]
- (format "%d) %s" (inc i) (api-error-message child-schema))))))
+ (create-cond-schema-message (:schemas schema)))
+
+ ;; For conditional schemas we'll generate a string similar to `cond-pre` above
+ (when (instance? schema.core.ConditionalSchema schema)
+ (create-cond-schema-message (map second (:preds-and-schemas schema))))
+
;; do the same for sequences of a schema
(when (vector? schema)
(str (tru "value must be an array.") (when (= (count schema) 1)
- (when-let [message (:api-error-message (first schema))]
+ (when-let [message (api-error-message (first schema))]
(str " " (tru "Each {0}" message))))))))
diff --git a/test/metabase/api/pulse_test.clj b/test/metabase/api/pulse_test.clj
index a2e40472b434c29a7f5439427c90efecea854747..87475c899b2f84d9c337389e489bd40ef441a7c8 100644
--- a/test/metabase/api/pulse_test.clj
+++ b/test/metabase/api/pulse_test.clj
@@ -103,24 +103,28 @@
;;; | POST /api/pulse |
;;; +----------------------------------------------------------------------------------------------------------------+
+(def ^:private default-post-card-ref-validation-error
+ {:errors
+ {:cards (str "value must be an array. Each value must satisfy one of the following requirements: "
+ "1) value must be a map with the following keys "
+ "`(collection_id, description, display, id, include_csv, include_xls, name)` "
+ "2) value must be a map with the keys `id`, `include_csv`, and `include_xls`. The array cannot be empty.")}})
+
(expect
{:errors {:name "value must be a non-blank string."}}
((user->client :rasta) :post 400 "pulse" {}))
(expect
- {:errors {:cards (str "value must be an array. Each value must be a map with the keys `id`, `include_csv`, and "
- "`include_xls`. The array cannot be empty.")}}
+ default-post-card-ref-validation-error
((user->client :rasta) :post 400 "pulse" {:name "abc"}))
(expect
- {:errors {:cards (str "value must be an array. Each value must be a map with the keys `id`, `include_csv`, and "
- "`include_xls`. The array cannot be empty.")}}
+ default-post-card-ref-validation-error
((user->client :rasta) :post 400 "pulse" {:name "abc"
:cards "foobar"}))
(expect
- {:errors {:cards (str "value must be an array. Each value must be a map with the keys `id`, `include_csv`, and "
- "`include_xls`. The array cannot be empty.")}}
+ default-post-card-ref-validation-error
((user->client :rasta) :post 400 "pulse" {:name "abc"
:cards ["abc"]}))
@@ -197,6 +201,42 @@
pulse-response
(update :channels remove-extra-channels-fields))))))
+;; Create a pulse with a HybridPulseCard and a CardRef, PUT accepts this format, we should make sure POST does as well
+(tt/expect-with-temp [Card [card-1]
+ Card [card-2 {:name "The card"
+ :description "Info"
+ :display :table}]]
+ (merge
+ pulse-defaults
+ {:name "A Pulse"
+ :creator_id (user->id :rasta)
+ :creator (user-details (fetch-user :rasta))
+ :cards (for [card [card-1 card-2]]
+ (assoc (pulse-card-details card)
+ :collection_id true))
+ :channels [(merge pulse-channel-defaults
+ {:channel_type "email"
+ :schedule_type "daily"
+ :schedule_hour 12
+ :recipients []})]
+ :collection_id true})
+ (card-api-test/with-cards-in-readable-collection [card-1 card-2]
+ (tt/with-temp Collection [collection]
+ (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
+ (tu/with-model-cleanup [Pulse]
+ (-> ((user->client :rasta) :post 200 "pulse" {:name "A Pulse"
+ :collection_id (u/get-id collection)
+ :cards [{:id (u/get-id card-1)
+ :include_csv false
+ :include_xls false}
+ (-> card-2
+ (select-keys [:id :name :description :display :collection_id])
+ (assoc :include_csv false, :include_xls false))]
+ :channels [daily-email-channel]
+ :skip_if_empty false})
+ pulse-response
+ (update :channels remove-extra-channels-fields))))))
+
;; Create a pulse with a csv and xls
(tt/expect-with-temp [Card [card-1]
Card [card-2]]
@@ -273,23 +313,28 @@
;;; | PUT /api/pulse/:id |
;;; +----------------------------------------------------------------------------------------------------------------+
+(def ^:private default-put-card-ref-validation-error
+ {:errors
+ {:cards (str "value may be nil, or if non-nil, value must be an array. "
+ "Each value must satisfy one of the following requirements: "
+ "1) value must be a map with the following keys "
+ "`(collection_id, description, display, id, include_csv, include_xls, name)` "
+ "2) value must be a map with the keys `id`, `include_csv`, and `include_xls`. The array cannot be empty.")}})
+
(expect
{:errors {:name "value may be nil, or if non-nil, value must be a non-blank string."}}
((user->client :rasta) :put 400 "pulse/1" {:name 123}))
(expect
- {:errors {:cards (str "value may be nil, or if non-nil, value must be an array. Each value must be a map with the "
- "keys `id`, `include_csv`, and `include_xls`. The array cannot be empty.")}}
+ default-put-card-ref-validation-error
((user->client :rasta) :put 400 "pulse/1" {:cards 123}))
(expect
- {:errors {:cards (str "value may be nil, or if non-nil, value must be an array. Each value must be a map with the "
- "keys `id`, `include_csv`, and `include_xls`. The array cannot be empty.")}}
+ default-put-card-ref-validation-error
((user->client :rasta) :put 400 "pulse/1" {:cards "foobar"}))
(expect
- {:errors {:cards (str "value may be nil, or if non-nil, value must be an array. Each value must be a map with the "
- "keys `id`, `include_csv`, and `include_xls`. The array cannot be empty.")}}
+ default-put-card-ref-validation-error
((user->client :rasta) :put 400 "pulse/1" {:cards ["abc"]}))
(expect
@@ -342,6 +387,35 @@
pulse-response
(update :channels remove-extra-channels-fields)))))
+;; Can we add a card to an existing pulse that has a card? Specifically this will include a HybridPulseCard (the
+;; original card associated with the pulse) and a CardRef (the new card)
+(tt/expect-with-temp [Pulse [pulse {:name "Original Pulse Name"}]
+ Card [card-1 {:name "Test"
+ :description "Just Testing"}]
+ PulseCard [_ {:card_id (u/get-id card-1)
+ :pulse_id (u/get-id pulse)}]
+ Card [card-2 {:name "Test2"
+ :description "Just Testing2"}]]
+ (merge
+ pulse-defaults
+ {:name "Original Pulse Name"
+ :creator_id (user->id :rasta)
+ :creator (user-details (fetch-user :rasta))
+ :cards (mapv (comp #(assoc % :collection_id true) pulse-card-details) [card-1 card-2])
+ :channels []
+ :collection_id true})
+ (with-pulses-in-writeable-collection [pulse]
+ (card-api-test/with-cards-in-readable-collection [card-1 card-2]
+ ;; The FE will include the original HybridPulseCard, similar to how the API returns the card via GET
+ (let [pulse-cards (:cards ((user->client :rasta) :get 200 (format "pulse/%d" (u/get-id pulse))))]
+ (-> ((user->client :rasta) :put 200 (format "pulse/%d" (u/get-id pulse))
+ {:cards (concat pulse-cards
+ [{:id (u/get-id card-2)
+ :include_csv false
+ :include_xls false}])})
+ pulse-response
+ (update :channels remove-extra-channels-fields))))))
+
;; Can we update *just* the Collection ID of a Pulse?
(expect
(tt/with-temp* [Pulse [pulse]