Merge pull request #8634 from metabase/archived-pulse-schema-failure

Ensure we don't try to hydrate a nil pulse

src/metabase/models/pulse.clj

@@ -163,8 +163,8 @@
 (s/defn retrieve-notification :- (s/maybe PulseInstance)
   "Fetch an Alert or Pulse, and do the 'standard' hydrations."
   [notification-or-id & additional-condtions]
-  (-> (apply Pulse :id (u/get-id notification-or-id), additional-condtions)
-      hydrate-notification))
+  (some-> (apply Pulse :id (u/get-id notification-or-id), additional-condtions)
+          hydrate-notification))
 
 (s/defn ^:private notification->alert :- PulseInstance
   "Take a generic `Notification` and put it in the standard `Alert` format the frontend expects. This really just

src/metabase/task/send_pulses.clj

@@ -78,22 +78,29 @@
 
 ;;; ------------------------------------------------- PULSE SENDING --------------------------------------------------
 
+(defn- log-pulse-exception [pulse-id exception]
+  (log/error "Error sending pulse:" pulse-id exception))
+
 (defn- send-pulses!
-  "Send any `Pulses` which are scheduled to run in the current day/hour. We use the current time and determine the hour
-  of the day and day of the week according to the defined reporting timezone, or UTC. We then find all `Pulses` that
-  are scheduled to run and send them."
-  [hour weekday monthday monthweek]
-  {:pre [(integer? hour)
-         (and (<= 0 hour) (>= 23 hour))
-         (pulse-channel/day-of-week? weekday)
-         (contains? #{:first :last :mid :other} monthday)
-         (contains? #{:first :last :other} monthweek)]}
-  (let [channels-by-pulse (group-by :pulse_id (pulse-channel/retrieve-scheduled-channels hour weekday monthday monthweek))]
-    (doseq [pulse-id (keys channels-by-pulse)]
-      (try
-        (log/debug (format "Starting Pulse Execution: %d" pulse-id))
-        (when-let [pulse (pulse/retrieve-notification pulse-id :archived false)]
-          (p/send-pulse! pulse :channel-ids (mapv :id (get channels-by-pulse pulse-id))))
-        (log/debug (format "Finished Pulse Execution: %d" pulse-id))
-        (catch Throwable e
-          (log/error "Error sending pulse:" pulse-id e))))))
+  "Send any `Pulses` which are scheduled to run in the current day/hour. We use the current time and determine the
+  hour of the day and day of the week according to the defined reporting timezone, or UTC. We then find all `Pulses`
+  that are scheduled to run and send them. The `on-error` function is called if an exception is thrown when sending
+  the pulse. Since this is a background process, the exception is only logged and not surfaced to the user. The
+  `on-error` function makes it easier to test for when an error doesn't occur"
+  ([hour weekday monthday monthweek]
+   (send-pulses! hour weekday monthday monthweek log-pulse-exception))
+  ([hour weekday monthday monthweek on-error]
+   {:pre [(integer? hour)
+          (and (<= 0 hour) (>= 23 hour))
+          (pulse-channel/day-of-week? weekday)
+          (contains? #{:first :last :mid :other} monthday)
+          (contains? #{:first :last :other} monthweek)]}
+   (let [channels-by-pulse (group-by :pulse_id (pulse-channel/retrieve-scheduled-channels hour weekday monthday monthweek))]
+     (doseq [pulse-id (keys channels-by-pulse)]
+       (try
+         (log/debug (format "Starting Pulse Execution: %d" pulse-id))
+         (when-let [pulse (pulse/retrieve-notification pulse-id :archived false)]
+           (p/send-pulse! pulse :channel-ids (mapv :id (get channels-by-pulse pulse-id))))
+         (log/debug (format "Finished Pulse Execution: %d" pulse-id))
+         (catch Throwable e
+           (on-error pulse-id e)))))))

test/metabase/task/send_pulses_test.clj

 (ns metabase.task.send-pulses-test
-  (:require [metabase
+  (:require [expectations :refer :all]
+            [metabase
+             [email :as email]
              [email-test :as et]
              [pulse-test :refer [checkins-query]]]
             [metabase.models
@@ -28,11 +30,56 @@
                                                             :channel_type  :email}]
                       PulseChannelRecipient [_             {:user_id          (users/user->id :rasta)
                                                             :pulse_channel_id pc-id}]]
-  (et/email-to :rasta
-               {:subject "Metabase alert: My Question Name has results",
-                :body    {"My Question Name" true}})
+  {:emails     (et/email-to :rasta
+                            {:subject "Metabase alert: My Question Name has results",
+                             :body    {"My Question Name" true}})
+   :exceptions []}
   (et/with-fake-inbox
     (data/with-db (data/get-or-create-database! defs/test-data)
-      (et/with-expected-messages 1
-        (#'metabase.task.send-pulses/send-pulses! 0 "fri" :first :first))
-      (et/regex-email-bodies #"My Question Name"))))
+      (let [exceptions (atom [])
+            on-error   (fn [_ exception]
+                         (swap! exceptions conj exception))]
+        (et/with-expected-messages 1
+          (#'metabase.task.send-pulses/send-pulses! 0 "fri" :first :first on-error))
+        {:emails     (et/regex-email-bodies #"My Question Name")
+         :exceptions @exceptions}))))
+
+;; Test that when we attempt to send a pulse that is archived, it just skips the pulse and sends nothing. Previously
+;; this failed schema validation (see issue #8581)
+(expect
+  {:emails     (et/email-to :rasta
+                            {:subject "Test"
+                             :body    {"Test Message" true}})
+   :exceptions []}
+
+  (tt/with-temp* [Card                 [{card-id :id}    (assoc (checkins-query {:breakout [["datetime-field" (data/id :checkins :date) "hour"]]})
+                                                           :name "My Question Name")]
+                  Pulse                [{pulse-id :id}   {:name "Test", :archived true}]
+                  PulseCard             [_               {:pulse_id pulse-id
+                                                          :card_id  card-id
+                                                          :position 0}]
+                  PulseChannel          [{pc-id :id}     {:pulse_id      pulse-id
+                                                          :schedule_hour nil
+                                                          :schedule_type "hourly"
+                                                          :channel_type  :email}]
+                  PulseChannelRecipient [_               {:user_id          (users/user->id :rasta)
+                                                          :pulse_channel_id pc-id}]]
+    (et/with-fake-inbox
+      (data/with-db (data/get-or-create-database! defs/test-data)
+        (let [exceptions (atom [])
+              on-error   (fn [_ exception]
+                           (swap! exceptions conj exception))]
+          (et/with-expected-messages 1
+            ;; Send the pulse, though it's not going to send anything. Typically we'd block waiting for the message to
+            ;; arrive, but there should be no message
+            (#'metabase.task.send-pulses/send-pulses! 0 "fri" :first :first on-error)
+            ;; This sends a test message. If we see our test message that means we didn't send a pulse message (which
+            ;; is what we want)
+            (email/send-message!
+              :subject      "Test"
+              :recipients   ["rasta@metabase.com"]
+              :message-type :html
+              :message      "Test Message"))
+          {:emails     (et/regex-email-bodies #"Test Message")
+           ;; There shouldn't be any failures, just skipping over the archived pulse
+           :exceptions @exceptions})))))