fussing around a bit with a more complete persistence layer implementation and...

fussing around a bit with a more complete persistence layer implementation and really simplifying api endpoints.

src/metabase/api/pulse.clj

@@ -7,9 +7,7 @@
             [metabase.api.common :refer :all]
             [metabase.db :as db]
             [metabase.driver :as driver]
-            (metabase.models [card :refer [Card] :as card]
-                             [common :as common]
-                             [database :refer [Database]]
+            (metabase.models [database :refer [Database]]
                              [hydrate :refer :all]
                              [pulse :refer [Pulse] :as pulse])
             [metabase.util :as util]
@@ -19,8 +17,7 @@
 (defendpoint GET "/"
   "Fetch all `Pulses`"
   []
-  (-> (db/sel :many Pulse (order :name :ASC))
-      (hydrate :creator :cards :channels)))
+  (pulse/retrieve-pulses))
 
 
 (defendpoint POST "/"
@@ -29,16 +26,13 @@
   {name     [Required NonEmptyString]
    cards    [Required ArrayOfMaps]
    channels [Required ArrayOfMaps]}
-  (let-500 [card-ids (filter identity (map :id cards))
-            pulse    (pulse/create-pulse name *current-user-id* card-ids channels)]
-    (hydrate pulse :cards :channels)))
+  (->500 (pulse/create-pulse name *current-user-id* (filter identity (map :id cards)) channels)))
 
 
 (defendpoint GET "/:id"
   "Fetch `Pulse` with ID."
   [id]
-  (->404 (db/sel :one Pulse :id id)
-         (hydrate :creator :cards :channels)))
+  (->404 (pulse/retrieve-pulse id)))
 
 
 (defendpoint PUT "/:id"
@@ -48,12 +42,11 @@
    cards    [Required ArrayOfMaps]
    channels [Required ArrayOfMaps]}
   (check-404 (db/exists? Pulse :id id))
-  (let-500 [result (pulse/update-pulse {:id       id
-                                        :name     name
-                                        :cards    (filter identity (into [] (map :id cards)))
-                                        :channels channels})]
-    (-> (db/sel :one Pulse :id id)
-        (hydrate :creator :cards :channels))))
+  (pulse/update-pulse {:id       id
+                       :name     name
+                       :cards    (filter identity (into [] (map :id cards)))
+                       :channels channels})
+  (pulse/retrieve-pulse id))
 
 
 (defendpoint DELETE "/:id"

src/metabase/models/pulse.clj

@@ -5,6 +5,7 @@
             [metabase.db :as db]
             (metabase.models [card :refer [Card]]
                              [common :refer [perms-readwrite]]
+                             [hydrate :refer :all]
                              [interface :refer :all]
                              [pulse-card :refer [PulseCard]]
                              [pulse-channel :refer [PulseChannel] :as pulse-channel]
@@ -65,7 +66,7 @@
     (k/insert PulseCard (k/values cards))))
 
 (defn- update-pulse-channel
-  "Utility function which determines how to properly update a single pulse channel given "
+  "Utility function which determines how to properly update a single pulse channel."
   [pulse-id new-channel existing-channel]
   ;; NOTE that we force the :id of the channel being updated to the :id we *know* from our
   ;;      existing list of `PulseChannels` pulled from the db to ensure we affect the right record
@@ -98,8 +99,23 @@
     (assert (= 0 (count (get new-channels nil))) "Cannot have channels without a :channel_type attribute")
     (dorun (map handle-channel (vec (keys pulse-channel/channel-types))))))
 
+(defn retrieve-pulse
+  "Fetch a single `Pulse` by its ID value."
+  [id]
+  {:pre [(integer? id)]}
+  (-> (db/sel :one Pulse :id id)
+      (hydrate :creator :cards :channels)))
+
+(defn retrieve-pulses
+  "Fetch all `Pulses`."
+  []
+  (-> (db/sel :many Pulse (k/order :name :ASC))
+      (hydrate :creator :cards :channels)))
+
 (defn update-pulse
-  "Update an existing `Pulse`"
+  "Update an existing `Pulse`, including all associated data such as: `PulseCards`, `PulseChannels`, and `PulseChannelRecipients`.
+
+   Returns the updated `Pulse` or throws an Exception."
   [{:keys [id name cards channels] :as pulse}]
   {:pre [(integer? id)
          (string? name)
@@ -112,11 +128,13 @@
     (db/upd Pulse id :name name)
     (update-pulse-cards pulse cards)
     (update-pulse-channels pulse channels)
-    (db/sel :one Pulse :id id)))
+    (retrieve-pulse id)))
 
 (defn create-pulse
   "Create a new `Pulse` by inserting it into the database along with all associated pieces of data such as:
-  `PulseCards`, `PulseChannels`, and `PulseChannelRecipients`."
+  `PulseCards`, `PulseChannels`, and `PulseChannelRecipients`.
+
+   Returns the newly created `Pulse` or throws an Exception."
   [name creator-id cards channels]
   {:pre [(string? name)
          (integer? creator-id)
@@ -133,4 +151,4 @@
       (update-pulse-cards pulse cards)
       ;; add channels to the Pulse
       (update-pulse-channels pulse channels)
-      (db/sel :one Pulse :id id))))
+      (retrieve-pulse id))))

src/metabase/models/pulse_channel.clj

@@ -105,7 +105,7 @@
 ;; ## Helper Functions
 
 (defn update-recipients
-  "Update the `PulseChannelRecipients` for PULSE.
+  "Update the `PulseChannelRecipients` for PULSE-CHANNEL.
    USER-IDS should be a definitive collection of *all* IDs of users who should receive the pulse.
 
    *  If an ID in USER-IDS has no corresponding existing `PulseChannelRecipients` object, one will be created.
@@ -120,14 +120,10 @@
         recipients+    (set/difference recipients-new recipients-old)
         recipients-    (set/difference recipients-old recipients-new)]
     (when (seq recipients+)
-      (let [vs (map #(assoc {:pulse_channel_id id} :user_id %)
-                    recipients+)]
-        (k/insert PulseChannelRecipient
-                (k/values vs))))
+      (let [vs (map #(assoc {:pulse_channel_id id} :user_id %) recipients+)]
+        (k/insert PulseChannelRecipient (k/values vs))))
     (when (seq recipients-)
-      (k/delete PulseChannelRecipient
-              (k/where {:pulse_channel_id id
-                      :user_id [in recipients-]})))))
+      (k/delete PulseChannelRecipient (k/where {:pulse_channel_id id :user_id [in recipients-]})))))
 
 (defn update-pulse-channel
   "Updates an existing `PulseChannel` along with all related data associated with the channel such as `PulseChannelRecipients`."
@@ -154,7 +150,8 @@
   {:pre [(channel-type? channel_type)
          (integer? pulse_id)
          (schedule-type? schedule_type)]}
-  (let [{:keys [id]} (db/ins PulseChannel
+  (let [
+        {:keys [id]} (db/ins PulseChannel
                        :pulse_id         pulse_id
                        :channel_type     channel_type
                        :details          details