Skip sending dashboard subscription if dashboard is archived (#18712)

src/metabase/models/pulse.clj

@@ -309,12 +309,17 @@
   (let [query {:select    [:p.* [:%lower.p.name :lower-name]]
                :modifiers [:distinct]
                :from      [[Pulse :p]]
-               :left-join (when user-id
-                            [[PulseChannel :pchan] [:= :p.id :pchan.pulse_id]
-                             [PulseChannelRecipient :pcr] [:= :pchan.id :pcr.pulse_channel_id]])
+               :left-join (concat
+                           [[:report_dashboard :d] [:= :p.dashboard_id :d.id]]
+                           (when user-id
+                             [[PulseChannel :pchan] [:= :p.id :pchan.pulse_id]
+                              [PulseChannelRecipient :pcr] [:= :pchan.id :pcr.pulse_channel_id]]))
                :where     [:and
                            [:= :p.alert_condition nil]
                            [:= :p.archived archived?]
+                           [:or
+                            [:= :p.dashboard_id nil]
+                            [:= :d.archived false]]
                            (when dashboard-id
                              [:= :p.dashboard_id dashboard-id])
                            (when user-id

src/metabase/pulse.clj

@@ -96,11 +96,10 @@
     (compare (:row dashcard-1) (:row dashcard-2))
     (compare (:col dashcard-1) (:col dashcard-2))))
 
-(defn execute-dashboard
+(defn- execute-dashboard
   "Fetch all the dashcards in a dashboard for a Pulse, and execute non-text cards"
-  [{pulse-creator-id :creator_id, :as pulse} dashboard-or-id & {:as options}]
-  (let [dashboard-id      (u/the-id dashboard-or-id)
-        dashboard         (Dashboard :id dashboard-id)
+  [{pulse-creator-id :creator_id, :as pulse} dashboard & {:as options}]
+  (let [dashboard-id      (u/the-id dashboard)
         dashcards         (db/select DashboardCard :dashboard_id dashboard-id)
         ordered-dashcards (sort dashcard-comparator dashcards)]
     (for [dashcard ordered-dashcards]
@@ -367,11 +366,11 @@
 
 (defn- pulse->notifications
   "Execute the underlying queries for a sequence of Pulses and return the results as 'notification' maps."
-  [{:keys [cards dashboard_id], pulse-id :id, :as pulse}]
+  [{:keys [cards], pulse-id :id, :as pulse} dashboard]
   (results->notifications pulse
-                          (if dashboard_id
+                          (if dashboard
                             ;; send the dashboard
-                            (execute-dashboard pulse dashboard_id)
+                            (execute-dashboard pulse dashboard)
                             ;; send the cards instead
                             (for [card  cards
                                   ;; Pulse ID may be `nil` if the Pulse isn't saved yet
@@ -423,12 +422,14 @@
    Example:
        (send-pulse! pulse)                       Send to all Channels
        (send-pulse! pulse :channel-ids [312])    Send only to Channel with :id = 312"
-  [{:keys [cards], :as pulse} & {:keys [channel-ids]}]
+  [{:keys [cards dashboard_id], :as pulse} & {:keys [channel-ids]}]
   {:pre [(map? pulse) (integer? (:creator_id pulse))]}
-  (let [pulse (-> pulse
-                  pulse/map->PulseInstance
-                  ;; This is usually already done by this step, in the `send-pulses` task which uses `retrieve-pulse`
-                  ;; to fetch the Pulse.
-                  pulse/hydrate-notification
-                  (merge (when channel-ids {:channel-ids channel-ids})))]
-    (send-notifications! (pulse->notifications pulse))))
+  (let [dashboard (Dashboard :id dashboard_id)
+        pulse     (-> pulse
+                      pulse/map->PulseInstance
+                      ;; This is usually already done by this step, in the `send-pulses` task which uses `retrieve-pulse`
+                      ;; to fetch the Pulse.
+                      pulse/hydrate-notification
+                      (merge (when channel-ids {:channel-ids channel-ids})))]
+    (when (not (:archived dashboard))
+      (send-notifications! (pulse->notifications pulse dashboard)))))

test/metabase/api/pulse_test.clj

@@ -791,7 +791,12 @@
         (is (= #{"LuckyRecipient" "Other"}
                (set (map :name (mt/user-http-request :rasta :get 200 (str "pulse?user_id=" (mt/user->id :rasta)))))))
         (is (= #{}
-               (set (map :name (mt/user-http-request :rasta :get 200 (str "pulse?user_id=" (mt/user->id :trashbird)))))))))))
+               (set (map :name (mt/user-http-request :rasta :get 200 (str "pulse?user_id=" (mt/user->id :trashbird)))))))))
+
+    (testing "excludes dashboard subscriptions associated with archived dashboards"
+      (mt/with-temp* [Dashboard [{dashboard-id :id} {:archived true}]
+                      Pulse     [_ {:dashboard_id dashboard-id}]]
+        (is (= [] (mt/user-http-request :rasta :get 200 "pulse")))))))
 
 
 ;;; +----------------------------------------------------------------------------------------------------------------+

test/metabase/dashboard_subscription_test.clj

@@ -133,11 +133,11 @@
   (testing "it runs for each non-virtual card"
     (mt/with-temp* [Card          [{card-id-1 :id}]
                     Card          [{card-id-2 :id}]
-                    Dashboard     [{dashboard-id :id} {:name "Birdfeed Usage"}]
+                    Dashboard     [{dashboard-id :id, :as dashboard} {:name "Birdfeed Usage"}]
                     DashboardCard [dashcard-1 {:dashboard_id dashboard-id :card_id card-id-1}]
                     DashboardCard [dashcard-2 {:dashboard_id dashboard-id :card_id card-id-2}]
                     User [{user-id :id}]]
-      (let [result (pulse/execute-dashboard {:creator_id user-id} dashboard-id)]
+      (let [result (@#'pulse/execute-dashboard {:creator_id user-id} dashboard)]
         (is (= (count result) 2))
         (is (schema= [{:card     (s/pred map?)
                        :dashcard (s/pred map?)
@@ -147,22 +147,22 @@
     (mt/with-temp* [Card          [{card-id-1 :id}]
                     Card          [{card-id-2 :id}]
                     Card          [{card-id-3 :id}]
-                    Dashboard     [{dashboard-id :id} {:name "Birdfeed Usage"}]
+                    Dashboard     [{dashboard-id :id, :as dashboard} {:name "Birdfeed Usage"}]
                     DashboardCard [dashcard-1 {:dashboard_id dashboard-id :card_id card-id-1 :row 1 :col 0}]
                     DashboardCard [dashcard-2 {:dashboard_id dashboard-id :card_id card-id-2 :row 0 :col 1}]
                     DashboardCard [dashcard-3 {:dashboard_id dashboard-id :card_id card-id-3 :row 0 :col 0}]
                     User [{user-id :id}]]
-      (let [result (pulse/execute-dashboard {:creator_id user-id} dashboard-id)]
+      (let [result (@#'pulse/execute-dashboard {:creator_id user-id} dashboard)]
         (is (= [card-id-3 card-id-2 card-id-1]
                (map #(-> % :card :id) result))))))
   (testing "virtual (text) cards are returned as a viz settings map"
     (mt/with-temp* [Card          [{card-id-1 :id}]
                     Card          [{card-id-2 :id}]
-                    Dashboard     [{dashboard-id :id} {:name "Birdfeed Usage"}]
+                    Dashboard     [{dashboard-id :id, :as dashboard} {:name "Birdfeed Usage"}]
                     DashboardCard [dashcard-1 {:dashboard_id dashboard-id
                                                :visualization_settings {:virtual_card {}, :text "test"}}]
                     User [{user-id :id}]]
-      (is (= [{:virtual_card {}, :text "test"}] (pulse/execute-dashboard {:creator_id user-id} dashboard-id))))))
+      (is (= [{:virtual_card {}, :text "test"}] (@#'pulse/execute-dashboard {:creator_id user-id} dashboard))))))
 
 (deftest basic-table-test
   (tests {:pulse {:skip_if_empty false}}
@@ -337,3 +337,17 @@
       (fn [{:keys [card-id]} [pulse-results]]
         (is (= {:blocks [{:type "section" :text {:type "mrkdwn" :text "abcdefghi…"}}]}
                (nth (:attachments (thunk->boolean pulse-results)) 2))))}}))
+
+(deftest archived-dashboard-test
+  (tests {:dashboard {:archived true}}
+    "Dashboard subscriptions are not sent if dashboard is archived"
+    {:card (checkins-query-card {})
+
+     :assert
+     {:slack
+      (fn [_ [pulse-results]]
+        (is (= {:attachments []} (thunk->boolean pulse-results))))
+
+      :email
+      (fn [_ _]
+        (is (= {} (mt/summarize-multipart-email))))}}))