Merge pull request #7919 from metabase/fix-failure-updating-existing-pulse

Change the Pulse API to accept a CardRef or HybridPulseCard

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

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

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

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]