From 145ecc6cbb98ed63f6e3a84f65d44ffeede2845d Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Fri, 29 Oct 2021 14:33:58 -0400
Subject: [PATCH] Skip sending dashboard subscription if dashboard is archived
(#18712)
---
src/metabase/models/pulse.clj | 11 +++++--
src/metabase/pulse.clj | 31 ++++++++++---------
test/metabase/api/pulse_test.clj | 7 ++++-
test/metabase/dashboard_subscription_test.clj | 26 ++++++++++++----
4 files changed, 50 insertions(+), 25 deletions(-)
diff --git a/src/metabase/models/pulse.clj b/src/metabase/models/pulse.clj
index 1381ca9d3f8..bd424fe7303 100644
--- a/src/metabase/models/pulse.clj
+++ b/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
diff --git a/src/metabase/pulse.clj b/src/metabase/pulse.clj
index bf8c095f1bc..852779630ef 100644
--- a/src/metabase/pulse.clj
+++ b/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)))))
diff --git a/test/metabase/api/pulse_test.clj b/test/metabase/api/pulse_test.clj
index 8c29130c785..ce595f4bab8 100644
--- a/test/metabase/api/pulse_test.clj
+++ b/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")))))))
;;; +----------------------------------------------------------------------------------------------------------------+
diff --git a/test/metabase/dashboard_subscription_test.clj b/test/metabase/dashboard_subscription_test.clj
index e3e475a0036..d63d0e2c039 100644
--- a/test/metabase/dashboard_subscription_test.clj
+++ b/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))))}}))
--
GitLab