diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index 1445a12d026d93ee68fd3bfb2100b66bc5f6f02f..724609db447229bad60e8e835ac7334b34efd75f 100644
--- a/.clj-kondo/config.edn
+++ b/.clj-kondo/config.edn
@@ -162,6 +162,7 @@
metabase.audit #{metabase.audit}
metabase.bootstrap #{metabase.bootstrap}
metabase.cmd #{} ; there are no namespaces here since you shouldn't be using this in any other module.
+ metabase.channel #{metabase.channel.core}
metabase.config #{metabase.config}
metabase.core #{metabase.core.initialization-status} ; TODO -- only namespace used outside of EE, this probably belongs in `metabase.server` anyway since that's the only place it's used.
metabase.db #{metabase.db
@@ -265,6 +266,7 @@
metabase.bootstrap #{}
metabase.cmd :any
metabase.compatibility :any
+ metabase.channel :any
metabase.config #{metabase.plugins}
metabase.core :any
metabase.db :any
@@ -412,6 +414,7 @@
metabase.api.user api.user
metabase.api.util api.util
metabase.async.streaming-response.thread-pool thread-pool
+ metabase.channel.core channel
metabase.cmd.copy.h2 copy.h2
metabase.config config
metabase.config.file config.file
diff --git a/dev/src/dev.clj b/dev/src/dev.clj
index 494a8e20b07562905b89d92e6228798b90180961..d9a0209ffdb939d92cc612a639deadfe10168a2c 100644
--- a/dev/src/dev.clj
+++ b/dev/src/dev.clj
@@ -69,7 +69,9 @@
[metabase.driver :as driver]
[metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
[metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
+ [metabase.email :as email]
[metabase.models.database :refer [Database]]
+ [metabase.models.setting :as setting]
[metabase.query-processor.compile :as qp.compile]
[metabase.query-processor.timezone :as qp.timezone]
[metabase.server :as server]
@@ -386,3 +388,25 @@
(when (failed?)
(throw (ex-info (format "Test failed after running: `%s`" test)
{:test test})))))))
+
+
+(defn setup-email!
+ "Set up email settings for sending emails from Metabase. This is useful for testing email sending in the REPL."
+ [& settings]
+ (let [settings (merge {:host "localhost"
+ :port 1025
+ :user "metabase"
+ :pass "metabase@secret"
+ :security :none}
+ settings)]
+ (when (::email/error (email/test-smtp-connection settings))
+ (throw (ex-info "Failed to connect to SMTP server" {:settings settings})))
+ (setting/set-many! (update-keys settings
+ {:host :email-smtp-host,
+ :user :email-smtp-username,
+ :pass :email-smtp-password,
+ :port :email-smtp-port,
+ :security :email-smtp-security,
+ :sender-name :email-from-name,
+ :sender :email-from-address,
+ :reply-to :email-reply-to}))))
diff --git a/dev/src/dev/render_png.clj b/dev/src/dev/render_png.clj
index 688b3f72d1952cb80f6b84e8d4273d2be7dcee4a..847df73b3ca9e4dd829f210d4d95680b2cf67c4c 100644
--- a/dev/src/dev/render_png.clj
+++ b/dev/src/dev/render_png.clj
@@ -9,7 +9,6 @@
[metabase.email.messages :as messages]
[metabase.models :refer [Card]]
[metabase.models.card :as card]
- [metabase.models.user :as user]
[metabase.pulse :as pulse]
[metabase.pulse.markdown :as markdown]
[metabase.pulse.render :as render]
@@ -30,11 +29,11 @@
(condp
#(<= 0 (.indexOf ^String %2 ^String %1))
(.toLowerCase (System/getProperty "os.name"))
- "win" :win
- "mac" :mac
- "nix" :unix
- "nux" :unix
- nil))
+ "win" :win
+ "mac" :mac
+ "nix" :unix
+ "nux" :unix
+ nil))
;; taken from https://github.com/aysylu/loom/blob/master/src/loom/io.clj
(defn- open
@@ -81,16 +80,20 @@
nil
query-results)))
-(defn open-hiccup-as-html
- "Take a hiccup data structure, render it as html, then open it in the browser."
- [hiccup]
- (let [html-str (hiccup/html hiccup)
- tmp-file (File/createTempFile "card-html" ".html")]
+(defn open-html
+ "Take a html string, then open it in the browser."
+ [html-str]
+ (let [tmp-file (File/createTempFile "card-html" ".html")]
(with-open [w (io/writer tmp-file)]
(.write w ^String html-str))
(.deleteOnExit tmp-file)
(open tmp-file)))
+(defn open-hiccup-as-html
+ "Take a hiccup data structure, render it as html, then open it in the browser."
+ [hiccup]
+ (open-html (hiccup/html hiccup)))
+
(def ^:private execute-dashboard #'pulse/execute-dashboard)
(defn render-dashboard-to-pngs
diff --git a/enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj b/enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj
index 850bf23a3f8d2a5e10d1bebcbf853700703df41b..f4aa4523d496c3e2595142e70e801a81dd02164b 100644
--- a/enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/advanced_config/api/pulse_test.clj
@@ -8,7 +8,8 @@
(deftest test-pulse-endpoint-should-respect-email-domain-allow-list-test
(testing "POST /api/pulse/test"
- (t2.with-temp/with-temp [Card card {:dataset_query (mt/mbql-query venues)}]
+ (t2.with-temp/with-temp [Card card {:name "Test card"
+ :dataset_query (mt/mbql-query venues)}]
;; make sure we validate raw emails whether they're part of `:details` or part of `:recipients` -- we
;; technically allow either right now
(doseq [channel [{:details {:emails ["test@metabase.com"]}}
@@ -16,23 +17,23 @@
:details {}}]]
(testing (format "\nChannel = %s\n" (u/pprint-to-str channel))
(letfn [(send! [expected-status-code]
- (let [pulse-name (mt/random-name)]
- (mt/with-fake-inbox
- {:response (mt/user-http-request
- :rasta :post expected-status-code "pulse/test"
- {:name pulse-name
- :cards [{:id (u/the-id card)
- :include_csv false
- :include_xls false
- :dashboard_card_id nil}]
- :channels [(merge {:enabled true
- :channel_type "email"
- :schedule_type "daily"
- :schedule_hour 12
- :schedule_day nil}
- channel)]
- :skip_if_empty false})
- :recipients (set (keys (mt/regex-email-bodies (re-pattern pulse-name))))})))]
+ (mt/with-fake-inbox
+ {:response (mt/user-http-request
+ :rasta :post expected-status-code "pulse/test"
+ {:name (mt/random-name)
+ :cards [{:id (u/the-id card)
+ :include_csv false
+ :include_xls false
+ :dashboard_card_id nil}]
+ :channels [(merge {:enabled true
+ :channel_type "email"
+ :schedule_type "daily"
+ :schedule_hour 12
+ :schedule_day nil}
+ channel)]
+ :alert_condition "rows"
+ :skip_if_empty false})
+ :recipients (set (keys (mt/regex-email-bodies (re-pattern "Test card"))))}))]
(testing "allowed email -- should pass"
(mt/with-premium-features #{:email-allow-list}
(mt/with-temporary-setting-values [subscription-allowed-domains "metabase.com"]
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/pulse_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/pulse_test.clj
index 834f35ddf219e461c68b3934f91ddc44c6ce2701..c3c17b3117e3ca22c2af0fcdba6441ca87f71773 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/pulse_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/pulse_test.clj
@@ -3,10 +3,8 @@
[clojure.data.csv :as csv]
[clojure.java.io :as io]
[clojure.test :refer :all]
- [medley.core :as m]
[metabase-enterprise.test :as met]
[metabase.api.alert :as api.alert]
- [metabase.email :as email]
[metabase.email.messages :as messages]
[metabase.models
:refer [Card Pulse PulseCard PulseChannel PulseChannelRecipient]]
@@ -21,7 +19,7 @@
(set! *warn-on-reflection* true)
-(deftest sandboxed-pulse-test
+(deftest sandboxed-alert-test
(testing "Pulses should get sent with the row-level restrictions of the User that created them."
(letfn [(send-pulse-created-by-user! [user-kw]
(met/with-gtaps! {:gtaps {:venues {:query (mt/mbql-query venues)
@@ -36,11 +34,12 @@
(is (= [[10]]
(send-pulse-created-by-user! :rasta))))))
-(defn- pulse-results
- "Results for creating and running a Pulse."
+(defn- alert-results
+ "Results for creating and running an Alert"
[query]
- (mt/with-temp [Card pulse-card {:dataset_query query}
- Pulse pulse {:name "Test Pulse"}
+ (mt/with-temp [Card pulse-card {:name "Test card"
+ :dataset_query query}
+ Pulse pulse {:alert_condition "rows"}
PulseCard _ {:pulse_id (:id pulse), :card_id (:id pulse-card)}
PulseChannel pc {:channel_type :email
:pulse_id (:id pulse)
@@ -48,59 +47,28 @@
PulseChannelRecipient _ {:pulse_channel_id (:id pc)
:user_id (mt/user->id :rasta)}]
(mt/with-temporary-setting-values [email-from-address "metamailman@metabase.com"]
- (mt/with-fake-inbox
- (with-redefs [messages/render-pulse-email (fn [_ _ _ [{:keys [result]}] _]
- [{:result result}])]
- (mt/with-test-user nil
- (metabase.pulse/send-pulse! pulse)))
- (let [results @mt/inbox]
- (is (= {"rasta@metabase.com" [{:from "metamailman@metabase.com"
- :bcc ["rasta@metabase.com"]
- :subject "Pulse: Test Pulse"}]}
- (m/dissoc-in results ["rasta@metabase.com" 0 :body])))
- (get-in results ["rasta@metabase.com" 0 :body 0 :result]))))))
+ (-> (#'metabase.pulse/execute-pulse (pulse/retrieve-pulse pulse) nil) first :result))))
-(deftest bcc-enabled-pulse-test
- (testing "When bcc is not enabled, return an email that uses to:"
- (mt/with-temp [Card pulse-card {}
- Pulse pulse {:name "Test Pulse"}
- PulseCard _ {:pulse_id (:id pulse), :card_id (:id pulse-card)}
- PulseChannel pc {:channel_type :email
- :pulse_id (:id pulse)
- :enabled true}
- PulseChannelRecipient _ {:pulse_channel_id (:id pc)
- :user_id (mt/user->id :rasta)}]
- (mt/with-temporary-setting-values [email-from-address "metamailman@metabase.com"]
- (mt/with-fake-inbox
- (with-redefs [messages/render-pulse-email (fn [_timezone _pulse _dashboard [{:keys [result]}, :as _parts] _non-user-email]
- [{:result result}])
- email/bcc-enabled? (constantly false)]
- (mt/with-test-user nil
- (metabase.pulse/send-pulse! pulse)))
- (let [results @mt/inbox]
- (is (= {"rasta@metabase.com" [{:from "metamailman@metabase.com"
- :to ["rasta@metabase.com"]
- :subject "Pulse: Test Pulse"}]}
- (m/dissoc-in results ["rasta@metabase.com" 0 :body])))
- (get-in results ["rasta@metabase.com" 0 :body 0 :result])))))))
-
-(deftest pulse-send-event-test
+(deftest dashboard-subscription-send-event-test
(testing "When we send a pulse, we also log the event:"
(mt/with-premium-features #{:audit-app}
- (t2.with-temp/with-temp [Card pulse-card {}
- Pulse pulse {:creator_id (mt/user->id :crowberto)
- :name "Test Pulse"}
- PulseCard _ {:pulse_id (:id pulse)
- :card_id (:id pulse-card)}
- PulseChannel pc {:channel_type :email
- :pulse_id (:id pulse)
- :enabled true}
- PulseChannelRecipient _ {:pulse_channel_id (:id pc)
- :user_id (mt/user->id :rasta)}]
+ (t2.with-temp/with-temp
+ [:model/Card pulse-card {:dataset_query (mt/mbql-query venues {:limit 1})}
+ :model/Dashboard dashboard {:name "Test Dashboard"}
+ :model/Pulse pulse {:creator_id (mt/user->id :crowberto)
+ :name "Test Pulse"
+ :alert_condition "rows"
+ :dashboard_id (:id dashboard)}
+ :model/PulseCard _ {:pulse_id (:id pulse)
+ :card_id (:id pulse-card)}
+ :model/PulseChannel pc {:channel_type :email
+ :pulse_id (:id pulse)
+ :enabled true}
+ :model/PulseChannelRecipient _ {:pulse_channel_id (:id pc)
+ :user_id (mt/user->id :rasta)}]
(mt/with-temporary-setting-values [email-from-address "metamailman@metabase.com"]
(mt/with-fake-inbox
- (with-redefs [messages/render-pulse-email (fn [_ _ _ [{:keys [result]}] _]
- [{:result result}])]
+ (with-redefs [metabase.pulse/send-retrying! (fn [_ _] :noop)]
(mt/with-test-user :lucky
(metabase.pulse/send-pulse! pulse)))
(is (= {:topic :subscription-send
@@ -112,7 +80,7 @@
(mt/latest-audit-log-entry :subscription-send (:id pulse))))))))))
(deftest alert-send-event-test
- (testing "When we send a pulse, we also log the event:"
+ (testing "When we send an alert, we also log the event:"
(mt/with-premium-features #{:audit-app}
(t2.with-temp/with-temp [Card pulse-card {:dataset_query (mt/mbql-query venues)}
Pulse pulse {:creator_id (mt/user->id :crowberto)
@@ -154,7 +122,7 @@
(testing "GTAPs should apply to Pulses — they should get the same results as if running that query normally"
(is (= [[3 13]]
(mt/rows
- (pulse-results query)))))))))
+ (alert-results query)))))))))
(defn- html->row-count [html]
(or (some->> html (re-find #"of <strong.+>(\d+)</strong> rows") second Integer/parseUnsignedInt)
@@ -183,7 +151,7 @@
(testing "Pulse should be sandboxed"
(is (= 22
- (count (mt/rows (pulse-results query))))))))))))
+ (count (mt/rows (alert-results query))))))))))))
(deftest pulse-preview-test
(testing "Pulse preview endpoints should be sandboxed"
@@ -198,13 +166,15 @@
(testing "POST /api/pulse/test"
(mt/with-fake-inbox
(mt/user-http-request :rasta :post 200 "pulse/test" {:name "venues"
+ :alert_condition "rows"
:cards [{:id (u/the-id card)
:include_csv true
:include_xls false}]
- :channels [{:channel_type :email
- :enabled :true
- :recipients [{:id (mt/user->id :rasta)
- :email "rasta@metabase.com"}]}]})
+ :channels [{:channel_type :email
+ :schedule_type "hourly"
+ :enabled :true
+ :recipients [{:id (mt/user->id :rasta)
+ :email "rasta@metabase.com"}]}]})
(let [[{html :content} {_icon :content} {attachment :content}] (get-in @mt/inbox ["rasta@metabase.com" 0 :body])]
(testing "email"
(is (= 22
@@ -221,8 +191,9 @@
(let [query (mt/mbql-query venues)]
(mt/with-test-user :rasta
(mt/with-temp [Card {card-id :id} {:dataset_query query}
- Pulse {pulse-id :id} {:name "Pulse Name"
- :skip_if_empty false}
+ Pulse {pulse-id :id} {:name "Pulse Name"
+ :skip_if_empty false
+ :alert_condition "rows"}
PulseCard _ {:pulse_id pulse-id
:card_id card-id
:position 0
@@ -232,7 +203,7 @@
:pulse_channel_id pc-id}]
(mt/with-fake-inbox
(mt/with-test-user nil
- (metabase.pulse/send-pulse! (pulse/retrieve-pulse pulse-id)))
+ (metabase.pulse/send-pulse! (pulse/retrieve-alert pulse-id)))
(let [email-results @mt/inbox
[{html :content} {_icon :attachment} {attachment :content}] (get-in email-results ["rasta@metabase.com" 0 :body])]
(testing "email"
@@ -277,7 +248,8 @@
(deftest sandboxed-users-cant-delete-pulse-recipients
(testing "When sandboxed users update a pulse, Metabase users in the recipients list are not deleted, even if they
are not included in the request."
- (mt/with-temp [Pulse {pulse-id :id} {:name "my pulse"}
+ (mt/with-temp [Pulse {pulse-id :id} {:name "my pulse"
+ :alert_condition "rows"}
PulseChannel {pc-id :id :as pc} {:pulse_id pulse-id
:channel_type :email
:details {:emails "asdf@metabase.com"}}
@@ -292,7 +264,7 @@
;; Check that both Rasta and Crowberto are still recipients
(is (= (sort [(mt/user->id :rasta) (mt/user->id :crowberto)])
- (->> (api.alert/email-channel (pulse/retrieve-pulse pulse-id)) :recipients (map :id) sort)))
+ (->> (api.alert/email-channel (pulse/retrieve-alert pulse-id)) :recipients (map :id) sort)))
(with-redefs [premium-features/sandboxed-or-impersonated-user? (constantly false)]
;; Rasta, a non-sandboxed user, updates the pulse, but does not include Crowberto in the recipients list
@@ -302,4 +274,4 @@
;; Crowberto should now be removed as a recipient
(is (= [(mt/user->id :rasta)]
- (->> (api.alert/email-channel (pulse/retrieve-pulse pulse-id)) :recipients (map :id) sort))))))))
+ (->> (api.alert/email-channel (pulse/retrieve-alert pulse-id)) :recipients (map :id) sort))))))))
diff --git a/src/metabase/channel/core.clj b/src/metabase/channel/core.clj
new file mode 100644
index 0000000000000000000000000000000000000000..a3b9945dbc392e795fdf8fd6850cf28d1c0870c9
--- /dev/null
+++ b/src/metabase/channel/core.clj
@@ -0,0 +1,46 @@
+(ns ^{:added "0.51.0"} metabase.channel.core
+ "The Metabase channel system.
+
+ The API is still in development and subject to change."
+ (:require
+ [metabase.plugins.classloader :as classloader]
+ [metabase.util :as u]
+ [metabase.util.log :as log]))
+
+(set! *warn-on-reflection* true)
+
+;; ------------------------------------------------------------------------------------------------;;
+;; Channels methods ;;
+;; ------------------------------------------------------------------------------------------------;;
+
+(defmulti render-notification
+ "Given a notification content, return a sequence of channel-specific messages.
+
+ The message format is channel-specific, one requirement is that it should be the same format that
+ the [[send!]] multimethod expects."
+ {:added "0.51.0"
+ :arglists '([channel-type notification-content recipients])}
+ (fn [channel-type notification-content _recipients]
+ [channel-type (:payload-type notification-content)]))
+
+(defmulti send!
+ "Send a message to a channel."
+ {:added "0.51.0"
+ :arglists '([channel-type message])}
+ (fn [channel-type _message]
+ channel-type))
+
+;; ------------------------------------------------------------------------------------------------;;
+;; Utils ;;
+;; ------------------------------------------------------------------------------------------------;;
+
+(defn- find-and-load-metabase-channels!
+ "Load namespaces that start with `metabase.channel."
+ []
+ (doseq [ns-symb u/metabase-namespace-symbols
+ :when (.startsWith (name ns-symb) "metabase.channel.")]
+ (log/info "Loading channel namespace:" (u/format-color :blue ns-symb))
+ (classloader/require ns-symb)))
+
+(when-not *compile-files*
+ (find-and-load-metabase-channels!))
diff --git a/src/metabase/channel/email.clj b/src/metabase/channel/email.clj
new file mode 100644
index 0000000000000000000000000000000000000000..7789633f82dfd31b31df58664fdb936989ebdb97
--- /dev/null
+++ b/src/metabase/channel/email.clj
@@ -0,0 +1,104 @@
+(ns metabase.channel.email
+ (:require
+ [metabase.channel.core :as channel]
+ [metabase.channel.shared :as channel.shared]
+ [metabase.email :as email]
+ [metabase.email.messages :as messages]
+ [metabase.util :as u]
+ [metabase.util.i18n :refer [trs]]
+ [metabase.util.malli :as mu]
+ [metabase.util.malli.schema :as ms]))
+
+(def ^:private EmailMessage
+ [:map
+ [:subject :string]
+ [:recipients [:sequential ms/Email]]
+ [:message-type [:enum :attachments :html :text]]
+ [:message :any]])
+
+(defn- construct-pulse-email [subject recipients message]
+ {:subject subject
+ :recipients recipients
+ :message-type :attachments
+ :message message})
+
+(defn- recipients->emails
+ [recipients]
+ (update-vals
+ {:user-emails (mapv (comp :email :user) (filter #(= :user (:kind %)) recipients))
+ :non-user-emails (mapv :email (filter #(= :external-email (:kind %)) recipients))}
+ #(filter u/email? %)))
+
+;; ------------------------------------------------------------------------------------------------;;
+;; Alerts ;;
+;; ------------------------------------------------------------------------------------------------;;
+
+(defn- find-goal-value
+ "The goal value can come from a progress goal or a graph goal_value depending on it's type"
+ [card]
+ (case (:display card)
+
+ (:area :bar :line)
+ (get-in card [:visualization_settings :graph.goal_value])
+
+ :progress
+ (get-in card [:visualization_settings :progress.goal])
+
+ nil))
+
+(mu/defmethod channel/send! :channel/email
+ [_channel-type {:keys [subject recipients message-type message]} :- EmailMessage]
+ (email/send-message-or-throw! {:subject subject
+ :recipients recipients
+ :message-type message-type
+ :message message
+ :bcc? (email/bcc-enabled?)}))
+
+(mu/defmethod channel/render-notification [:channel/email :notification/alert] :- [:sequential EmailMessage]
+ [_channel-type {:keys [card pulse payload channel]} recipients]
+ (let [condition-kwd (messages/pulse->alert-condition-kwd pulse)
+ email-subject (case condition-kwd
+ :meets (trs "Alert: {0} has reached its goal" (:name card))
+ :below (trs "Alert: {0} has gone below its goal" (:name card))
+ :rows (trs "Alert: {0} has results" (:name card)))
+ {:keys [user-emails
+ non-user-emails]} (recipients->emails recipients)
+ timezone (channel.shared/defaulted-timezone card)
+ goal (find-goal-value card)
+ email-to-users (when (> (count user-emails) 0)
+ (construct-pulse-email
+ email-subject user-emails
+ (messages/render-alert-email timezone pulse channel
+ [payload]
+ goal
+ nil)))
+ email-to-nonusers (for [non-user-email non-user-emails]
+ (construct-pulse-email
+ email-subject [non-user-email]
+ (messages/render-alert-email timezone pulse channel
+ [payload]
+ goal
+ non-user-email)))]
+ (filter some? (conj email-to-nonusers email-to-users))))
+
+;; ------------------------------------------------------------------------------------------------;;
+;; Dashboard Subscriptions ;;
+;; ------------------------------------------------------------------------------------------------;;
+
+(mu/defmethod channel/render-notification [:channel/email :notification/dashboard-subscription] :- [:sequential EmailMessage]
+ [_channel-details {:keys [dashboard payload pulse]} recipients]
+ (let [{:keys [user-emails
+ non-user-emails]} (recipients->emails recipients)
+ timezone (some->> payload (some :card) channel.shared/defaulted-timezone)
+ email-subject (:name dashboard)
+ email-to-users (when (seq user-emails)
+ (construct-pulse-email
+ email-subject
+ user-emails
+ (messages/render-pulse-email timezone pulse dashboard payload nil)))
+ email-to-nonusers (for [non-user-email non-user-emails]
+ (construct-pulse-email
+ email-subject
+ [non-user-email]
+ (messages/render-pulse-email timezone pulse dashboard payload non-user-email)))]
+ (filter some? (conj email-to-nonusers email-to-users))))
diff --git a/src/metabase/channel/shared.clj b/src/metabase/channel/shared.clj
new file mode 100644
index 0000000000000000000000000000000000000000..1b5700c33d6d0ea59332fc49ac8bc9228ec00356
--- /dev/null
+++ b/src/metabase/channel/shared.clj
@@ -0,0 +1,11 @@
+(ns metabase.channel.shared
+ (:require
+ [metabase.query-processor.timezone :as qp.timezone]
+ [metabase.util.malli :as mu]
+ [toucan2.core :as t2]))
+
+(mu/defn defaulted-timezone :- :string
+ "Returns the timezone ID for the given `card`. Either the report timezone (if applicable) or the JVM timezone."
+ [card]
+ (or (some->> card :database_id (t2/select-one :model/Database :id) qp.timezone/results-timezone-id)
+ (qp.timezone/system-timezone-id)))
diff --git a/src/metabase/channel/slack.clj b/src/metabase/channel/slack.clj
new file mode 100644
index 0000000000000000000000000000000000000000..9fd599ec5a32f47272ab020f590869763782f0b3
--- /dev/null
+++ b/src/metabase/channel/slack.clj
@@ -0,0 +1,167 @@
+(ns metabase.channel.slack
+ (:require
+ [clojure.string :as str]
+ [metabase.channel.core :as channel]
+ [metabase.channel.shared :as channel.shared]
+ ;; TODO: integrations.slack should be migrated to channel.slack
+ [metabase.integrations.slack :as slack]
+ [metabase.public-settings :as public-settings]
+ [metabase.pulse.markdown :as markdown]
+ [metabase.pulse.parameters :as pulse-params]
+ [metabase.pulse.render :as render]
+ [metabase.util.malli :as mu]
+ [metabase.util.urls :as urls]))
+
+(defn- truncate-mrkdwn
+ "If a mrkdwn string is greater than Slack's length limit, truncates it to fit the limit and
+ adds an ellipsis character to the end."
+ [mrkdwn limit]
+ (if (> (count mrkdwn) limit)
+ (-> mrkdwn
+ (subs 0 (dec limit))
+ (str "…"))
+ mrkdwn))
+
+(def ^:private block-text-length-limit 3000)
+(def ^:private attachment-text-length-limit 2000)
+
+(defn- text->markdown-block
+ [text]
+ (let [mrkdwn (markdown/process-markdown text :slack)]
+ (when (not (str/blank? mrkdwn))
+ {:blocks [{:type "section"
+ :text {:type "mrkdwn"
+ :text (truncate-mrkdwn mrkdwn block-text-length-limit)}}]})))
+
+(defn- payload->attachment-data
+ [payload channel-id]
+ (case (:type payload)
+ :card
+ (let [{:keys [card dashcard result]} payload
+ {card-id :id card-name :name :as card} card]
+ {:title (or (-> dashcard :visualization_settings :card.title)
+ card-name)
+ :rendered-info (render/render-pulse-card :inline (channel.shared/defaulted-timezone card) card dashcard result)
+ :title_link (urls/card-url card-id)
+ :attachment-name "image.png"
+ :channel-id channel-id
+ :fallback card-name})
+
+ :text
+ (text->markdown-block (:text payload))
+
+ :tab-title
+ (text->markdown-block (format "# %s" (:text payload)))))
+
+(def slack-width
+ "Maximum width of the rendered PNG of HTML to be sent to Slack. Content that exceeds this width (e.g. a table with
+ many columns) is truncated."
+ 1200)
+
+(defn create-and-upload-slack-attachments!
+ "Create an attachment in Slack for a given Card by rendering its content into an image and uploading
+ it. Slack-attachment-uploader is a function which takes image-bytes and an attachment name, uploads the file, and
+ returns an image url, defaulting to slack/upload-file!.
+
+ Nested `blocks` lists containing text cards are passed through unmodified."
+ [attachments]
+ (letfn [(f [a] (select-keys a [:title :title_link :fallback]))]
+ (reduce (fn [processed {:keys [rendered-info attachment-name channel-id] :as attachment-data}]
+ (conj processed (if (:blocks attachment-data)
+ attachment-data
+ (if (:render/text rendered-info)
+ (-> (f attachment-data)
+ (assoc :text (:render/text rendered-info)))
+ (let [image-bytes (render/png-from-render-info rendered-info slack-width)
+ image-url (slack/upload-file! image-bytes attachment-name channel-id)]
+ (-> (f attachment-data)
+ (assoc :image_url image-url)))))))
+ []
+ attachments)))
+
+(def ^:private SlackMessage
+ [:map {:closed true}
+ [:channel-id :string]
+ ;; TODO: tighten this attachments schema
+ [:attachments :any]
+ [:message {:optional true} [:maybe :string]]])
+
+(mu/defmethod channel/send! :channel/slack
+ [_channel-type message :- SlackMessage]
+ (let [{:keys [channel-id attachments]} message]
+ (slack/post-chat-message! channel-id nil (create-and-upload-slack-attachments! attachments))))
+
+;; ------------------------------------------------------------------------------------------------;;
+;; Alerts ;;
+;; ------------------------------------------------------------------------------------------------;;
+
+(mu/defmethod channel/render-notification [:channel/slack :notification/alert] :- [:sequential SlackMessage]
+ [_channel-details {:keys [payload card]} channel-ids]
+ (let [attachments [{:blocks [{:type "header"
+ :text {:type "plain_text"
+ :text (str "🔔 " (:name card))
+ :emoji true}}]}
+ (payload->attachment-data payload (slack/files-channel))]]
+ (for [channel-id channel-ids]
+ {:channel-id channel-id
+ :attachments attachments})))
+
+;; ------------------------------------------------------------------------------------------------;;
+;; Dashboard Subscriptions ;;
+;; ------------------------------------------------------------------------------------------------;;
+
+(defn- filter-text
+ [filter]
+ (truncate-mrkdwn
+ (format "*%s*\n%s" (:name filter) (pulse-params/value-string filter))
+ attachment-text-length-limit))
+
+(defn- slack-dashboard-header
+ "Returns a block element that includes a dashboard's name, creator, and filters, for inclusion in a
+ Slack dashboard subscription"
+ [pulse dashboard]
+ (let [header-section {:type "header"
+ :text {:type "plain_text"
+ :text (:name dashboard)
+ :emoji true}}
+ creator-section {:type "section"
+ :fields [{:type "mrkdwn"
+ :text (str "Sent by " (-> pulse :creator :common_name))}]}
+ filters (pulse-params/parameters pulse dashboard)
+ filter-fields (for [filter filters]
+ {:type "mrkdwn"
+ :text (filter-text filter)})
+ filter-section (when (seq filter-fields)
+ {:type "section"
+ :fields filter-fields})]
+ (if filter-section
+ {:blocks [header-section filter-section creator-section]}
+ {:blocks [header-section creator-section]})))
+
+(defn- slack-dashboard-footer
+ "Returns a block element with the footer text and link which should be at the end of a Slack dashboard subscription."
+ [pulse dashboard]
+ {:blocks
+ [{:type "divider"}
+ {:type "context"
+ :elements [{:type "mrkdwn"
+ :text (str "<" (pulse-params/dashboard-url (:id dashboard) (pulse-params/parameters pulse dashboard)) "|"
+ "*Sent from " (public-settings/site-name) "*>")}]}]})
+
+(defn- create-slack-attachment-data
+ "Returns a seq of slack attachment data structures, used in `create-and-upload-slack-attachments!`"
+ [parts]
+ (let [channel-id (slack/files-channel)]
+ (for [part parts
+ :let [attachment (payload->attachment-data part channel-id)]
+ :when attachment]
+ attachment)))
+
+(mu/defmethod channel/render-notification [:channel/slack :notification/dashboard-subscription] :- [:sequential SlackMessage]
+ [_channel-type {:keys [payload dashboard pulse]} channel-ids]
+ (for [channel-id channel-ids]
+ {:channel-id channel-id
+ :attachments (remove nil?
+ (flatten [(slack-dashboard-header pulse dashboard)
+ (create-slack-attachment-data payload)
+ (slack-dashboard-footer pulse dashboard)]))}))
diff --git a/src/metabase/email/messages.clj b/src/metabase/email/messages.clj
index ecc2804ca2a909b05b23ce2e69fdb8c34cbf0fbe..5c583c2c9a48fd8eec1087f9d7c593a17d4aac3c 100644
--- a/src/metabase/email/messages.clj
+++ b/src/metabase/email/messages.clj
@@ -338,9 +338,9 @@
(let [dashboard-id (:id dashboard)]
(merge (common-context)
{:emailType "pulse"
- :title (:name pulse)
+ :title (:name dashboard)
:titleUrl (pulse-params/dashboard-url dashboard-id (pulse-params/parameters pulse dashboard))
- :dashboardDescription (:description dashboard)
+ :dashboardDescription (markdown/process-markdown (:description dashboard) :html)
;; There are legacy pulses that exist without being tied to a dashboard
:dashboardHasTabs (when dashboard-id
(boolean (seq (t2/hydrate dashboard :tabs))))
@@ -578,7 +578,7 @@
"Returns a string that describes the run schedule of an alert (i.e. how often results are checked),
for inclusion in the email template. Not translated, since emails in general are not currently translated."
[channel]
- (case (:schedule_type channel)
+ (case (keyword (:schedule_type channel))
:hourly
"Run hourly"
@@ -596,10 +596,13 @@
(defn- alert-context
"Context that is applicable only to the actual alert template (not alert management templates)"
[alert channel non-user-email]
- (let [{card-id :id, card-name :name} (first-card alert)]
+ (let [{card-id :id card-name :name} (first-card alert)]
{:title card-name
:titleUrl (urls/card-url card-id)
:alertSchedule (alert-schedule-text channel)
+ :notificationText (if (nil? non-user-email)
+ "Manage your subscriptions"
+ "Unsubscribe")
:notificationManagementUrl (if (nil? non-user-email)
(urls/notification-management-url)
(str (urls/unsubscribe-url)
diff --git a/src/metabase/models/dashboard.clj b/src/metabase/models/dashboard.clj
index 60e349d9c0d2bb01b9bbe4e43bb6397e2d4506a3..bcb15d0a88095292d69bc0a248a2289b7eff277a 100644
--- a/src/metabase/models/dashboard.clj
+++ b/src/metabase/models/dashboard.clj
@@ -144,6 +144,9 @@
(t2/with-transaction [_conn]
(binding [pulse/*allow-moving-dashboard-subscriptions* true]
(t2/update! :model/Pulse {:dashboard_id dashboard-id}
+ ;; TODO we probably don't need this anymore
+ ;; pulse.name is no longer used for generating title.
+ ;; pulse.collection_id is a thing for the old "Pulse" feature, but it was removed
{:name (:name dashboard)
:collection_id (:collection_id dashboard)})
(pulse-card/bulk-create! new-pulse-cards)))))))
diff --git a/src/metabase/models/pulse.clj b/src/metabase/models/pulse.clj
index 8718a17b5384f4068c75312eda9ac5a4e16a7de9..accbfe84d1a1c6a63febeae58dd57d3ff9b72c43 100644
--- a/src/metabase/models/pulse.clj
+++ b/src/metabase/models/pulse.clj
@@ -293,7 +293,7 @@
"Fetch a single *Pulse*, and hydrate it with a set of 'standard' hydrations; remove Alert columns, since this is a
*Pulse* and they will all be unset."
[pulse-or-id]
- (some-> (t2/select-one Pulse :id (u/the-id pulse-or-id), :alert_condition nil)
+ (some-> (t2/select-one Pulse :id (u/the-id pulse-or-id))
hydrate-notification
notification->pulse))
diff --git a/src/metabase/pulse.clj b/src/metabase/pulse.clj
index 691d99efdb58ecd981d724b1b684d4fe32918f46..ffd680923cda313b6fda2374560c5b868a5b284b 100644
--- a/src/metabase/pulse.clj
+++ b/src/metabase/pulse.clj
@@ -1,10 +1,8 @@
(ns metabase.pulse
"Public API for sending Pulses."
(:require
- [clojure.string :as str]
[metabase.api.common :as api]
- [metabase.email :as email]
- [metabase.email.messages :as messages]
+ [metabase.channel.core :as channel]
[metabase.events :as events]
[metabase.integrations.slack :as slack]
[metabase.models.dashboard :as dashboard :refer [Dashboard]]
@@ -13,8 +11,6 @@
[metabase.models.interface :as mi]
[metabase.models.pulse :as pulse :refer [Pulse]]
[metabase.models.serialization :as serdes]
- [metabase.public-settings :as public-settings]
- [metabase.pulse.markdown :as markdown]
[metabase.pulse.parameters :as pulse-params]
[metabase.pulse.render :as render]
[metabase.pulse.util :as pu]
@@ -22,16 +18,14 @@
[metabase.server.middleware.session :as mw.session]
[metabase.shared.parameters.parameters :as shared.params]
[metabase.util :as u]
- [metabase.util.i18n :refer [trs tru]]
+ [metabase.util.i18n :refer [tru]]
[metabase.util.log :as log]
[metabase.util.malli :as mu]
[metabase.util.malli.schema :as ms]
[metabase.util.retry :as retry]
[metabase.util.ui-logic :as ui-logic]
[metabase.util.urls :as urls]
- [toucan2.core :as t2])
- (:import
- (clojure.lang ExceptionInfo)))
+ [toucan2.core :as t2]))
(set! *warn-on-reflection* true)
@@ -186,110 +180,6 @@
(or (some->> card database-id (t2/select-one Database :id) qp.timezone/results-timezone-id)
(qp.timezone/system-timezone-id)))
-(defn- first-question-name [pulse]
- (-> pulse :cards first :name))
-
-(defn- alert-condition-type->description [condition-type]
- (case (keyword condition-type)
- :meets (trs "reached its goal")
- :below (trs "gone below its goal")
- :rows (trs "results")))
-
-(def ^:private block-text-length-limit 3000)
-(def ^:private attachment-text-length-limit 2000)
-
-(defn- truncate-mrkdwn
- "If a mrkdwn string is greater than Slack's length limit, truncates it to fit the limit and
- adds an ellipsis character to the end."
- [mrkdwn limit]
- (if (> (count mrkdwn) limit)
- (-> mrkdwn
- (subs 0 (dec limit))
- (str "…"))
- mrkdwn))
-
-(defn- text->markdown-block
- [text]
- (let [mrkdwn (markdown/process-markdown text :slack)]
- (when (not (str/blank? mrkdwn))
- {:blocks [{:type "section"
- :text {:type "mrkdwn"
- :text (truncate-mrkdwn mrkdwn block-text-length-limit)}}]})))
-
-(defn- part->attachment-data
- [part channel-id]
- (case (:type part)
- :card
- (let [{:keys [card dashcard result]} part
- {card-id :id card-name :name :as card} card]
- {:title (or (-> dashcard :visualization_settings :card.title)
- card-name)
- :rendered-info (render/render-pulse-card :inline (defaulted-timezone card) card dashcard result)
- :title_link (urls/card-url card-id)
- :attachment-name "image.png"
- :channel-id channel-id
- :fallback card-name})
-
- :text
- (text->markdown-block (:text part))
-
- :tab-title
- (text->markdown-block (format "# %s" (:text part)))))
-
-(defn- create-slack-attachment-data
- "Returns a seq of slack attachment data structures, used in `create-and-upload-slack-attachments!`"
- [parts]
- (let [channel-id (slack/files-channel)]
- (for [part parts
- :let [attachment (part->attachment-data part channel-id)]
- :when attachment]
- attachment)))
-
-(defn- subject
- [{:keys [name cards dashboard_id]}]
- (if (or dashboard_id
- (some :dashboard_id cards))
- name
- (trs "Pulse: {0}" name)))
-
-(defn- filter-text
- [filter]
- (truncate-mrkdwn
- (format "*%s*\n%s" (:name filter) (pulse-params/value-string filter))
- attachment-text-length-limit))
-
-(defn- slack-dashboard-header
- "Returns a block element that includes a dashboard's name, creator, and filters, for inclusion in a
- Slack dashboard subscription"
- [pulse dashboard]
- (let [header-section {:type "header"
- :text {:type "plain_text"
- :text (subject pulse)
- :emoji true}}
- creator-section {:type "section"
- :fields [{:type "mrkdwn"
- :text (str "Sent by " (-> pulse :creator :common_name))}]}
- filters (pulse-params/parameters pulse dashboard)
- filter-fields (for [filter filters]
- {:type "mrkdwn"
- :text (filter-text filter)})
- filter-section (when (seq filter-fields)
- {:type "section"
- :fields filter-fields})]
- (if filter-section
- {:blocks [header-section filter-section creator-section]}
- {:blocks [header-section creator-section]})))
-
-(defn- slack-dashboard-footer
- "Returns a block element with the footer text and link which should be at the end of a Slack dashboard subscription."
- [pulse dashboard]
- {:blocks
- [{:type "divider"}
- {:type "context"
- :elements [{:type "mrkdwn"
- :text (str "<" (pulse-params/dashboard-url (u/the-id dashboard) (pulse-params/parameters pulse dashboard)) "|"
- "*Sent from " (public-settings/site-name) "*>")}]}]})
-
(def slack-width
"Maximum width of the rendered PNG of HTML to be sent to Slack. Content that exceeds this width (e.g. a table with
many columns) is truncated."
@@ -343,9 +233,9 @@
;;; +----------------------------------------------------------------------------------------------------------------+
(defn- alert-or-pulse [pulse]
- (if (:alert_condition pulse)
- :alert
- :pulse))
+ (if (:dashboard_id pulse)
+ :pulse
+ :alert))
(defmulti ^:private should-send-notification?
"Returns true if given the pulse type and resultset a new notification (pulse or alert) should be sent"
@@ -370,168 +260,95 @@
(not (are-all-parts-empty? parts))
true))
-(defn- parts->cards-count
- [parts]
- (count (filter #(some? (#{:text :card} (:type %))) parts)))
-
-;; 'notification' used below means a map that has information needed to send a Pulse/Alert, including results of
-;; running the underlying query
-(defmulti ^:private notification
- "Polymorphic function for creating notifications. This logic is different for pulse type (i.e. alert vs. pulse) and
- channel_type (i.e. email vs. slack)"
- {:arglists '([alert-or-pulse parts channel])}
- (fn [pulse _ {:keys [channel_type]}]
- [(alert-or-pulse pulse) (keyword channel_type)]))
-
-(defn- construct-pulse-email [subject recipients message]
- {:subject subject
- :recipients recipients
- :message-type :attachments
- :message message})
-
-(defmethod notification [:pulse :email]
- [{pulse-id :id, pulse-name :name, dashboard-id :dashboard_id, :as pulse} parts {:keys [recipients]}]
- (log/debug (u/format-color :cyan "Sending Pulse (%s: %s) with %s Cards via email"
- pulse-id (pr-str pulse-name) (parts->cards-count parts)))
- (let [user-recipients (filter (fn [recipient] (and (u/email? (:email recipient))
- (some? (:id recipient)))) recipients)
- non-user-recipients (filter (fn [recipient] (and (u/email? (:email recipient))
- (nil? (:id recipient)))) recipients)
- timezone (some->> parts (some :card) defaulted-timezone)
- dashboard (update (t2/select-one Dashboard :id dashboard-id) :description markdown/process-markdown :html)
- email-to-users (when (> (count user-recipients) 0)
- (construct-pulse-email (subject pulse) (mapv :email user-recipients) (messages/render-pulse-email timezone pulse dashboard parts nil)))
- email-to-nonusers (for [non-user (map :email non-user-recipients)]
- (construct-pulse-email (subject pulse) [non-user] (messages/render-pulse-email timezone pulse dashboard parts non-user)))]
- (if email-to-users
- (conj email-to-nonusers email-to-users)
- email-to-nonusers)))
-
-(defmethod notification [:pulse :slack]
- [{pulse-id :id, pulse-name :name, dashboard-id :dashboard_id, :as pulse}
- parts
- {{channel-id :channel} :details}]
- (log/debug (u/format-color :cyan "Sending Pulse (%s: %s) with %s Cards via Slack"
- pulse-id (pr-str pulse-name) (parts->cards-count parts)))
- (let [dashboard (t2/select-one Dashboard :id dashboard-id)]
- {:channel-id channel-id
- :attachments (remove nil?
- (flatten [(slack-dashboard-header pulse dashboard)
- (create-slack-attachment-data parts)
- (when dashboard (slack-dashboard-footer pulse dashboard))]))}))
-
-(defmethod notification [:alert :email]
- [{:keys [id] :as pulse} parts channel]
- (log/debugf "Sending Alert (%s: %s) via email" id name)
- (let [condition-kwd (messages/pulse->alert-condition-kwd pulse)
- email-subject (trs "Alert: {0} has {1}"
- (first-question-name pulse)
- (alert-condition-type->description condition-kwd))
- user-recipients (filter (fn [recipient] (and (u/email? (:email recipient))
- (some? (:id recipient)))) (:recipients channel))
- non-user-recipients (filter (fn [recipient] (and (u/email? (:email recipient))
- (nil? (:id recipient)))) (:recipients channel))
- first-part (some :card parts)
- timezone (defaulted-timezone first-part)
- email-to-users (when (> (count user-recipients) 0)
- (construct-pulse-email email-subject (mapv :email user-recipients) (messages/render-alert-email timezone pulse channel parts (ui-logic/find-goal-value first-part) nil)))
- email-to-nonusers (for [non-user (map :email non-user-recipients)]
- (construct-pulse-email email-subject [non-user] (messages/render-alert-email timezone pulse channel parts (ui-logic/find-goal-value first-part) non-user)))]
- (if email-to-users
- (conj email-to-nonusers email-to-users)
- email-to-nonusers)))
-
-(defmethod notification [:alert :slack]
- [pulse parts {{channel-id :channel} :details}]
- (log/debug (u/format-color :cyan "Sending Alert (%s: %s) via Slack" (:id pulse) (:name pulse)))
- {:channel-id channel-id
- :attachments (cons {:blocks [{:type "header"
- :text {:type "plain_text"
- :text (str "🔔 " (first-question-name pulse))
- :emoji true}}]}
- (create-slack-attachment-data parts))})
-
-(defmethod notification :default
- [_alert-or-pulse _parts {:keys [channel_type], :as _channel}]
- (throw (UnsupportedOperationException. (tru "Unrecognized channel type {0}" (pr-str channel_type)))))
-
-(defn- parts->notifications [{:keys [channels channel-ids] pulse-id :id :as pulse} parts]
- (let [channel-ids (or channel-ids (mapv :id channels))]
- (when (should-send-notification? pulse parts)
+(defn- get-notification-info
+ [pulse parts channel]
+ (let [alert? (nil? (:dashboard_id pulse))]
+ (merge {:payload-type (if alert?
+ :notification/alert
+ :notification/dashboard-subscription)
+ :payload (if alert? (first parts) parts)
+ :pulse pulse
+ :channel channel}
+ (if alert?
+ {:card (t2/select-one :model/Card (-> parts first :card :id))}
+ {:dashboard (t2/select-one :model/Dashboard (:dashboard_id pulse))}))))
+
+(defn- channels-to-channel-recipients
+ [channel]
+ (if (= :slack (keyword (:channel_type channel)))
+ [(get-in channel [:details :channel])]
+ (for [recipient (:recipients channel)]
+ (if-not (:id recipient)
+ {:kind :external-email
+ :email (:email recipient)}
+ {:kind :user
+ :user recipient}))))
+
+(defn- channel-send!
+ [& args]
+ (try
+ (apply channel/send! args)
+ (catch Exception e
+ ;; Token errors have already been logged and we should not retry.
+ (when-not (and (= :channel/slack (first args))
+ (contains? (:errors (ex-data e)) :slack-token))
+ (throw e)))))
+
+(defn- send-retrying!
+ [& args]
+ (try
+ (apply (retry/decorate channel-send!) args)
+ (catch Throwable e
+ (log/error e "Error sending notification!"))))
+
+(defn- execute-pulse
+ [{:keys [cards] pulse-id :id :as pulse} dashboard]
+ (if dashboard
+ ;; send the dashboard
+ (execute-dashboard pulse dashboard)
+ ;; send the cards instead
+ (for [card cards
+ ;; Pulse ID may be `nil` if the Pulse isn't saved yet
+ :let [part (pu/execute-card pulse (u/the-id card) :pulse-id pulse-id)]
+ ;; some cards may return empty part, e.g. if the card has been archived
+ :when part]
+ part)))
+
+(defn- send-pulse!*
+ [{:keys [channels channel-ids] pulse-id :id :as pulse} dashboard]
+ (let [parts (execute-pulse pulse dashboard)
+ ;; `channel-ids` is the set of channels to send to now, so only send to those. Note the whole set of channels
+ channels (if (seq channel-ids)
+ (filter #((set channel-ids) (:id %)) channels)
+ channels)]
+ (if (should-send-notification? pulse parts)
(let [event-type (if (= :pulse (alert-or-pulse pulse))
:event/subscription-send
:event/alert-send)]
(events/publish-event! event-type {:id (:id pulse)
:user-id (:creator_id pulse)
:object {:recipients (map :recipients (:channels pulse))
- :filters (:parameters pulse)}}))
-
- (when (:alert_first_only pulse)
- (t2/delete! Pulse :id pulse-id))
- ;; `channel-ids` is the set of channels to send to now, so only send to those. Note the whole set of channels
- (for [channel channels
- :when (contains? (set channel-ids) (:id channel))]
- (notification pulse parts channel)))))
-
-(defn- pulse->notifications
- "Execute the underlying queries for a sequence of Pulses and return the parts as 'notification' maps."
- [{:keys [cards] pulse-id :id :as pulse} dashboard]
- (parts->notifications
- pulse
- (if dashboard
- ;; send the dashboard
- (execute-dashboard pulse dashboard)
- ;; send the cards instead
- (for [card cards
- ;; Pulse ID may be `nil` if the Pulse isn't saved yet
- :let [part (assoc (pu/execute-card pulse (u/the-id card) :pulse-id pulse-id) :type :card)]
- ;; some cards may return empty part, e.g. if the card has been archived
- :when part]
- part))))
+ :filters (:parameters pulse)}})
+ (u/prog1 (doseq [channel channels]
+ (try
+ (let [channel-type (if (= :email (keyword (:channel_type channel)))
+ :channel/email
+ :channel/slack)
+ messages (channel/render-notification channel-type
+ (get-notification-info pulse parts channel)
+ (channels-to-channel-recipients channel))]
+ (doseq [message messages]
+ (send-retrying! channel-type message)))
+ (catch Exception e
+ (log/errorf e "Error sending %s %d to channel %s" (alert-or-pulse pulse) (:id pulse) (:channel_type channel)))))
+ (when (:alert_first_only pulse)
+ (t2/delete! Pulse :id pulse-id))))
+ (log/infof "Skipping sending %s %d" (alert-or-pulse pulse) (:id pulse)))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Sending Notifications |
;;; +----------------------------------------------------------------------------------------------------------------+
-(defmulti ^:private send-notification!
- "Invokes the side-effecty function for sending emails/slacks depending on the notification type"
- {:arglists '([pulse-or-alert])}
- (fn [{:keys [channel-id]}]
- (if channel-id :slack :email)))
-
-(defmethod send-notification! :slack
- [{:keys [channel-id message attachments]}]
- (let [attachments (create-and-upload-slack-attachments! attachments)]
- (try
- (slack/post-chat-message! channel-id message attachments)
- (catch ExceptionInfo e
- ;; Token errors have already been logged and we should not retry.
- (when-not (contains? (:errors (ex-data e)) :slack-token)
- (throw e))))))
-
-(defmethod send-notification! :email
- [emails]
- (doseq [{:keys [subject recipients message-type message]} emails]
- (email/send-message-or-throw! {:subject subject
- :recipients recipients
- :message-type message-type
- :message message
- :bcc? (email/bcc-enabled?)})))
-
-(defn- send-notification-retrying!
- "Like [[send-notification!]] but retries sending on errors according to the retry settings."
- [& args]
- (apply (retry/decorate send-notification!) args))
-
-(defn- send-notifications! [notifications]
- (doseq [notification notifications]
- ;; do a try-catch around each notification so if one fails, we'll still send the other ones for example, an Alert
- ;; set up to send over both Slack & email: if Slack fails, we still want to send the email (#7409)
- (try
- (send-notification-retrying! notification)
- (catch Throwable e
- (log/error e "Error sending notification!")))))
-
(defn send-pulse!
"Execute and Send a `Pulse`, optionally specifying the specific `PulseChannels`. This includes running each
`PulseCard`, formatting the content, and sending the content to any specified destination.
@@ -552,4 +369,4 @@
pulse/hydrate-notification
(merge (when channel-ids {:channel-ids channel-ids})))]
(when (not (:archived dashboard))
- (send-notifications! (pulse->notifications pulse dashboard)))))
+ (send-pulse!* pulse dashboard))))
diff --git a/src/metabase/pulse/util.clj b/src/metabase/pulse/util.clj
index e409f8b6326c939b886970d2bc2c5ef5cb7c3467..68f268e904aff9c6388ade55b19fc99df425e348 100644
--- a/src/metabase/pulse/util.clj
+++ b/src/metabase/pulse/util.clj
@@ -42,7 +42,8 @@
(process-query))
(process-query))]
{:card card
- :result result}))
+ :result result
+ :type :card}))
(catch Throwable e
(log/warnf e "Error running query for Card %s" card-id)))))
diff --git a/test/metabase/api/pulse_test.clj b/test/metabase/api/pulse_test.clj
index 917cc830d1473c2e20a07a85851914b137855e14..575da5598330aabfb85efe0ebbb389fd23cc66f2 100644
--- a/test/metabase/api/pulse_test.clj
+++ b/test/metabase/api/pulse_test.clj
@@ -21,6 +21,7 @@
[metabase.models.pulse-channel :as pulse-channel]
[metabase.models.pulse-test :as pulse-test]
[metabase.pulse.render.style :as style]
+ [metabase.pulse.test-util :as pulse.test-util]
[metabase.server.request.util :as req.util]
[metabase.test :as mt]
[metabase.test.mock.util :refer [pulse-channel-defaults]]
@@ -874,13 +875,7 @@
PulseChannelRecipient _ {:pulse_channel_id (u/the-id pc)
:user_id (mt/user->id :rasta)}]
(with-pulses-in-nonreadable-collection [pulse]
- (mt/user-http-request :rasta :get 200 (str "pulse/" (u/the-id pulse))))))
-
- (testing "should 404 for an Alert"
- (t2.with-temp/with-temp [Pulse {pulse-id :id} {:alert_condition "rows"}]
- (is (= "Not found."
- (with-pulses-in-readable-collection [pulse-id]
- (mt/user-http-request :rasta :get 404 (str "pulse/" pulse-id)))))))))
+ (mt/user-http-request :rasta :get 200 (str "pulse/" (u/the-id pulse))))))))
(deftest send-test-pulse-test
;; see [[metabase-enterprise.advanced-config.api.pulse-test/test-pulse-endpoint-should-respect-email-domain-allow-list-test]]
@@ -890,27 +885,37 @@
(mt/with-fake-inbox
(mt/dataset sad-toucan-incidents
(mt/with-temp [Collection collection {}
+ Dashboard {dashboard-id :id} {:name "Daily Sad Toucans"
+ :parameters [{:name "X"
+ :slug "x"
+ :id "__X__"
+ :type "category"
+ :default 3}]}
Card card {:dataset_query (mt/mbql-query incidents {:aggregation [[:count]]})}]
(perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
(api.card-test/with-cards-in-readable-collection [card]
- (is (= {:ok true}
- (mt/user-http-request :rasta :post 200 "pulse/test" {:name "Daily Sad Toucans"
- :collection_id (u/the-id collection)
- :cards [{:id (u/the-id card)
- :include_csv false
- :include_xls false
- :dashboard_card_id nil}]
- :channels [{:enabled true
- :channel_type "email"
- :schedule_type "daily"
- :schedule_hour 12
- :schedule_day nil
- :recipients [(mt/fetch-user :rasta)]}]
- :skip_if_empty false})))
- (is (= (mt/email-to :rasta {:subject "Pulse: Daily Sad Toucans"
- :body {"Daily Sad Toucans" true}
- :bcc? true})
- (mt/regex-email-bodies #"Daily Sad Toucans"))))))))))
+ (let [channel-messages (pulse.test-util/with-captured-channel-send-messages!
+ (is (= {:ok true}
+ (mt/user-http-request :rasta :post 200 "pulse/test"
+ {:name (mt/random-name)
+ :dashboard_id dashboard-id
+ :cards [{:id (:id card)
+ :include_csv false
+ :include_xls false
+ :dashboard_card_id nil}]
+ :channels [{:enabled true
+ :channel_type "email"
+ :schedule_type "daily"
+ :schedule_hour 12
+ :schedule_day nil
+ :recipients [(mt/fetch-user :rasta)]}]
+ :skip_if_empty false}))))]
+ (is (= {:message [{"Daily Sad Toucans" true}
+ pulse.test-util/png-attachment]
+ :message-type :attachments,
+ :recipients #{"rasta@metabase.com"}
+ :subject "Daily Sad Toucans"}
+ (mt/summarize-multipart-single-email (-> channel-messages :channel/email first) #"Daily Sad Toucans")))))))))))
(deftest send-test-pulse-validate-emails-test
(testing (str "POST /api/pulse/test should call " `pulse-channel/validate-email-domains)
@@ -952,7 +957,8 @@
:display-name "X"
:type :number
:required true}}}}}
- Dashboard {dashboard-id :id} {:parameters [{:name "X"
+ Dashboard {dashboard-id :id} {:name "Daily Sad Toucans"
+ :parameters [{:name "X"
:slug "x"
:id "__X__"
:type "category"
@@ -963,90 +969,28 @@
:card_id card-id
:target [:variable [:template-tag "x"]]}]}]
(mt/with-fake-inbox
- (is (= {:ok true}
- (mt/user-http-request :rasta :post 200 "pulse/test" {:name "Daily Sad Toucans"
- :dashboard_id dashboard-id
- :cards [{:id card-id
- :include_csv false
- :include_xls false
- :dashboard_card_id nil}]
- :channels [{:enabled true
- :channel_type "email"
- :schedule_type "daily"
- :schedule_hour 12
- :schedule_day nil
- :recipients [(mt/fetch-user :rasta)]}]
- :skip_if_empty false})))
- (is (= (mt/email-to :rasta {:subject "Daily Sad Toucans"
- :body {"Daily Sad Toucans" true}
- :bcc? true})
- (mt/regex-email-bodies #"Daily Sad Toucans")))))))
-
-(deftest send-placeholder-card-test-pulse-test
- (testing "POST /api/pulse/test should work with placeholder cards"
- (mt/with-temp [Dashboard {dashboard-id :id} {}]
- (mt/with-fake-inbox
- (is (= {:ok true}
- (mt/user-http-request :rasta :post 200 "pulse/test" {:name "Daily Sad Toucans"
- :dashboard_id dashboard-id
- :cards [{:display "placeholder"
- :id nil
- :include_csv false
- :include_xls false
- :dashboard_card_id nil}]
- :channels [{:enabled true
- :channel_type "email"
- :schedule_type "daily"
- :schedule_hour 12
- :schedule_day nil
- :recipients [(mt/fetch-user :rasta)]}]
- :skip_if_empty false})))
- (is (= (mt/email-to :rasta {:subject "Daily Sad Toucans"
- :body {"Daily Sad Toucans" true}
- :bcc? true})
- (mt/regex-email-bodies #"Daily Sad Toucans")))))))
-
-;; This test follows a flow that the user/UI would follow by first creating a pulse, then making a small change to
-;; that pulse and testing it. The primary purpose of this test is to ensure tha the pulse/test endpoint accepts data
-;; of the same format that the pulse GET returns
-(deftest update-flow-test
- (mt/with-temp [Card card-1 {:dataset_query
- {:database (mt/id) :type :query :query {:source-table (mt/id :venues)}}}
- Card card-2 {:dataset_query
- {:database (mt/id) :type :query :query {:source-table (mt/id :venues)}}}]
-
- (api.card-test/with-cards-in-readable-collection [card-1 card-2]
- (mt/with-fake-inbox
- (mt/with-model-cleanup [Pulse]
- (t2.with-temp/with-temp [Collection collection]
- (perms/grant-collection-readwrite-permissions! (perms-group/all-users) collection)
- ;; First create the pulse
- (let [{pulse-id :id} (mt/user-http-request :rasta :post 200 "pulse"
- {:name "A Pulse"
- :collection_id (u/the-id collection)
- :skip_if_empty false
- :cards [{:id (u/the-id card-1)
- :include_csv false
- :include_xls false
- :dashboard_card_id nil}
- {:id (u/the-id card-2)
- :include_csv false
- :include_xls false
- :dashboard_card_id nil}]
-
- :channels [(assoc daily-email-channel :recipients [(mt/fetch-user :rasta)
- (mt/fetch-user :crowberto)])]})
- ;; Retrieve the pulse via GET
- result (mt/user-http-request :rasta :get 200 (str "pulse/" pulse-id))
- ;; Change our fetched copy of the pulse to only have Rasta for the recipients
- email-channel (assoc (-> result :channels first) :recipients [(mt/fetch-user :rasta)])]
- ;; Don't update the pulse, but test the pulse with the updated recipients
- (is (= {:ok true}
- (mt/user-http-request :rasta :post 200 "pulse/test" (assoc result :channels [email-channel]))))
- (is (= (mt/email-to :rasta {:subject "Pulse: A Pulse"
- :body {"A Pulse" true}
- :bcc? true})
- (mt/regex-email-bodies #"A Pulse"))))))))))
+ (let [channel-messages (pulse.test-util/with-captured-channel-send-messages!
+ (is (= {:ok true}
+ (mt/user-http-request :rasta :post 200 "pulse/test"
+ {:name (mt/random-name)
+ :dashboard_id dashboard-id
+ :cards [{:id card-id
+ :include_csv false
+ :include_xls false
+ :dashboard_card_id nil}]
+ :channels [{:enabled true
+ :channel_type "email"
+ :schedule_type "daily"
+ :schedule_hour 12
+ :schedule_day nil
+ :recipients [(mt/fetch-user :rasta)]}]
+ :skip_if_empty false}))))]
+ (is (= {:message [{"Daily Sad Toucans" true}
+ pulse.test-util/png-attachment]
+ :message-type :attachments,
+ :recipients #{"rasta@metabase.com"}
+ :subject "Daily Sad Toucans"}
+ (mt/summarize-multipart-single-email (-> channel-messages :channel/email first) #"Daily Sad Toucans"))))))))
(deftest ^:parallel pulse-card-query-results-test
(testing "viz-settings saved in the DB for a Card should be loaded"
diff --git a/test/metabase/channel/email_test.clj b/test/metabase/channel/email_test.clj
new file mode 100644
index 0000000000000000000000000000000000000000..499572bc7fa720bc268fc4c640e7f66ba955a7ec
--- /dev/null
+++ b/test/metabase/channel/email_test.clj
@@ -0,0 +1,22 @@
+(ns metabase.channel.email-test
+ (:require
+ [clojure.test :refer :all]
+ [metabase.channel.core :as channel]
+ [metabase.email :as email]
+ [metabase.test :as mt]))
+
+(deftest bcc-enabled-test
+ (testing "When bcc is not enabled, return an email that uses to:"
+ (let [sent-message (atom nil)]
+ (with-redefs [email/send-email! (fn [_ message]
+ (reset! sent-message message))]
+ (mt/with-temporary-setting-values [email-from-address "metamailman@metabase.com"
+ email-smtp-host " ake_smtp_host"
+ email-smtp-port 587
+ bcc-enabled? false]
+ (channel/send! :channel/email {:subject "Test"
+ :recipients ["ngoc@metabase.com"]
+ :message-type :html
+ :message "Test message"})
+ (is (=? {:to ["ngoc@metabase.com"]}
+ @sent-message)))))))
diff --git a/test/metabase/dashboard_subscription_test.clj b/test/metabase/dashboard_subscription_test.clj
index 325b953909e95e2fb61d596705ae78975153d271..41a10c296da8c1a82b3f58a7236b5a0b10eab773 100644
--- a/test/metabase/dashboard_subscription_test.clj
+++ b/test/metabase/dashboard_subscription_test.clj
@@ -2,6 +2,7 @@
(:require
[clojure.string :as str]
[clojure.test :refer :all]
+ [metabase.channel.slack :as channel.slack]
[metabase.email.messages :as messages]
[metabase.models
:refer [Card
@@ -17,7 +18,6 @@
User]]
[metabase.models.data-permissions :as data-perms]
[metabase.models.permissions-group :as perms-group]
- [metabase.models.pulse :as pulse]
[metabase.public-settings :as public-settings]
[metabase.pulse]
[metabase.pulse.render.body :as body]
@@ -70,7 +70,7 @@
[[pulse-binding properties] & body]
`(do-with-dashboard-sub-for-card ~properties (fn [~pulse-binding] ~@body)))
-(defn- do-test
+(defn- do-test!
"Run a single Pulse test with a standard set of boilerplate. Creates Card, Pulse, and other related objects using
`card`, `dashboard`, `pulse`, and `pulse-card` properties, then sends the Pulse; finally, test assertions in
`assert` are invoked. `assert` can contain `:email` and/or `:slack` assertions, which are used to test an email and
@@ -109,7 +109,12 @@
(f {:dashboard-id dashboard-id,
:card-id card-id,
:pulse-id pulse-id}
- (metabase.pulse/send-pulse! (pulse/retrieve-notification pulse-id))))
+ ((if (= :email channel-type)
+ :channel/email
+ :channel/slack)
+ (pulse.test-util/with-captured-channel-send-messages!
+ (mt/with-temporary-setting-values [site-url "https://metabase.com/testmb"]
+ (metabase.pulse/send-pulse! (t2/select-one :model/Pulse pulse-id)))))))
(thunk []
(if fixture
(fixture {:dashboard-id dashboard-id,
@@ -117,10 +122,10 @@
:pulse-id pulse-id} thunk*)
(thunk*)))]
(case channel-type
- :email (pulse.test-util/email-test-setup (thunk))
- :slack (pulse.test-util/slack-test-setup (thunk)))))))))
+ :email (thunk)
+ :slack (pulse.test-util/slack-test-setup! (thunk)))))))))
-(defn- tests
+(defn- tests!
"Convenience for writing multiple tests using [[do-test]]. `common` is a map of shared properties as passed
to [[do-test]] that is deeply merged with the individual maps for each test. Other args are alternating `testing`
context messages and properties as passed to [[do-test]]:
@@ -140,14 +145,16 @@
[common & {:as message->m}]
(doseq [[message m] message->m]
(testing message
- (do-test (merge-with merge common m)))))
+ (do-test! (merge-with merge common m)))))
-(defn- rasta-pulse-email [& [email]]
- (mt/email-to :rasta (merge {:subject "Aviary KPIs"
- :body [{"Aviary KPIs" true}
- pulse.test-util/png-attachment]
- :bcc? true}
- email)))
+(defn- rasta-dashsub-message
+ [& [data]]
+ (merge {:subject "Aviary KPIs"
+ :recipients #{"rasta@metabase.com"}
+ :message-type :attachments,
+ :message [{"Aviary KPIs" true}
+ pulse.test-util/png-attachment]}
+ data))
(defn do-with-dashboard-fixture-for-dashboard
"Impl for [[with-link-card-fixture-for-dashboard]]."
@@ -283,7 +290,7 @@
(@#'metabase.pulse/execute-dashboard {:creator_id user-id} dashboard))))))
(deftest basic-table-test
- (tests {:pulse {:skip_if_empty false} :display :table}
+ (tests! {:pulse {:skip_if_empty false} :display :table}
"9 results, so no attachment aside from dashboard icon"
{:card (pulse.test-util/checkins-query-card {:aggregation nil, :limit 9})
@@ -295,27 +302,27 @@
:assert
{:email
- (fn [_ _]
- (is (= (rasta-pulse-email
- {:body [{;; No "Pulse:" prefix
- "Aviary KPIs" true
- ;; Includes dashboard description
- "How are the birds doing today?" true
- ;; Includes name of subscription creator
- "Sent by Rasta Toucan" true
- ;; Includes everything
- "More results have been included" false
- ;; Inline table
- "ID</th>" true
- ;; Links to source dashboard
- "<a class=\\\"title\\\" href=\\\"https://metabase.com/testmb/dashboard/\\d+\\\"" true
- ;; Links to Metabase instance
- "Sent from <a href=\\\"https://metabase.com/testmb\\\"" true
- ;; Links to subscription management page in account settings
- "\\\"https://metabase.com/testmb/account/notifications\\\"" true
- "Manage your subscriptions" true}
- pulse.test-util/png-attachment]})
- (mt/summarize-multipart-email
+ (fn [_ [email]]
+ (is (= (rasta-dashsub-message
+ {:message [{;; No "Pulse:" prefix
+ "Aviary KPIs" true
+ ;; Includes dashboard description
+ "How are the birds doing today?" true
+ ;; Includes name of subscription creator
+ "Sent by Rasta Toucan" true
+ ;; Includes everything
+ "More results have been included" false
+ ;; Inline table
+ "ID</th>" true
+ ;; Links to source dashboard
+ "<a class=\\\"title\\\" href=\\\"https://metabase.com/testmb/dashboard/\\d+\\\"" true
+ ;; Links to Metabase instance
+ "Sent from <a href=\\\"https://metabase.com/testmb\\\"" true
+ ;; Links to subscription management page in account settings
+ "\\\"https://metabase.com/testmb/account/notifications\\\"" true
+ "Manage your subscriptions" true}
+ pulse.test-util/png-attachment]})
+ (mt/summarize-multipart-single-email email
#"Aviary KPIs"
#"How are the birds doing today?"
#"Sent by Rasta Toucan"
@@ -327,8 +334,8 @@
#"Manage your subscriptions"))))
:slack
(fn [{:keys [card-id dashboard-id]} [pulse-results]]
- ;; If we don't force the thunk, the rendering code will never execute and attached-results-text won't be
- ;; called
+ ;; If we don't force the thunk, the rendering code will never execute and attached-results-text won't be
+ ;; called
(testing "\"more results in attachment\" text should not be present for Slack Pulses"
(testing "Pulse results"
(is (= {:channel-id "#general"
@@ -357,7 +364,7 @@
(pulse.test-util/output @#'body/attached-results-text))))))}}))
(deftest virtual-card-test
- (tests {:pulse {:skip_if_empty false}, :dashcard {:row 0, :col 0}}
+ (tests! {:pulse {:skip_if_empty false}, :dashcard {:row 0, :col 0}}
"Dashboard subscription that includes a virtual (markdown) card"
{:card (pulse.test-util/checkins-query-card {})
@@ -372,13 +379,13 @@
:assert
{:email
- (fn [_ _]
+ (fn [_ [email]]
(testing "Markdown cards are included in email subscriptions"
- (is (= (rasta-pulse-email {:body [{"Aviary KPIs" true
- "header" true}
- pulse.test-util/png-attachment]})
- (mt/summarize-multipart-email #"Aviary KPIs"
- #"header")))))
+ (is (= (rasta-dashsub-message {:message [{"Aviary KPIs" true
+ "header" true}
+ pulse.test-util/png-attachment]})
+ (mt/summarize-multipart-single-email email #"Aviary KPIs"
+ #"header")))))
:slack
(fn [{:keys [card-id dashboard-id]} [pulse-results]]
@@ -404,7 +411,7 @@
(pulse.test-util/thunk->boolean pulse-results)))))}}))
(deftest virtual-card-heading-test
- (tests {:pulse {:skip_if_empty false}, :dashcard {:row 0, :col 0}}
+ (tests! {:pulse {:skip_if_empty false}, :dashcard {:row 0, :col 0}}
"Dashboard subscription that includes a virtual card. For heading cards we escape markdown, add a heading markdown, and don't subsitute tags."
{:card (pulse.test-util/checkins-query-card {})
@@ -419,13 +426,13 @@
:assert
{:email
- (fn [_ _]
+ (fn [_ [email]]
(testing "Markdown cards are included in email subscriptions"
- (is (= (rasta-pulse-email {:body [{"Aviary KPIs" true
- "header, quote isn't escaped" true}
- pulse.test-util/png-attachment]})
- (mt/summarize-multipart-email #"Aviary KPIs"
- #"header, quote isn't escaped")))))
+ (is (= (rasta-dashsub-message {:message [{"Aviary KPIs" true
+ "header, quote isn't escaped" true}
+ pulse.test-util/png-attachment]})
+ (mt/summarize-multipart-single-email email #"Aviary KPIs"
+ #"header, quote isn't escaped")))))
:slack
(fn [{:keys [card-id dashboard-id]} [pulse-results]]
@@ -451,9 +458,9 @@
(pulse.test-util/thunk->boolean pulse-results)))))}}))
(deftest dashboard-filter-test
- (with-redefs [metabase.pulse/attachment-text-length-limit 15]
- (tests {:pulse {:skip_if_empty false}
- :dashboard pulse.test-util/test-dashboard}
+ (with-redefs [channel.slack/attachment-text-length-limit 15]
+ (tests! {:pulse {:skip_if_empty false}
+ :dashboard pulse.test-util/test-dashboard}
"Dashboard subscription that includes a dashboard filters"
{:card (pulse.test-util/checkins-query-card {})
@@ -464,12 +471,12 @@
:assert
{:email
- (fn [_ _]
+ (fn [_ [email]]
(testing "Markdown cards are included in email subscriptions"
- (is (= (rasta-pulse-email {:body [{"Aviary KPIs" true
- "<a class=\\\"title\\\" href=\\\"https://metabase.com/testmb/dashboard/\\d+\\?state=CA&state=NY&state=NJ&quarter_and_year=Q1-2021\\\"" true}
- pulse.test-util/png-attachment]})
- (mt/summarize-multipart-email #"Aviary KPIs"
+ (is (= (rasta-dashsub-message {:message [{"Aviary KPIs" true
+ "<a class=\\\"title\\\" href=\\\"https://metabase.com/testmb/dashboard/\\d+\\?state=CA&state=NY&state=NJ&quarter_and_year=Q1-2021\\\"" true}
+ pulse.test-util/png-attachment]})
+ (mt/summarize-multipart-single-email email #"Aviary KPIs"
#"<a class=\"title\" href=\"https://metabase.com/testmb/dashboard/\d+\?state=CA&state=NY&state=NJ&quarter_and_year=Q1-2021\"")))))
:slack
@@ -498,8 +505,8 @@
(pulse.test-util/thunk->boolean pulse-results)))))}})))
(deftest dashboard-with-link-card-test
- (tests {:pulse {:skip_if_empty false}
- :dashboard pulse.test-util/test-dashboard}
+ (tests! {:pulse {:skip_if_empty false}
+ :dashboard pulse.test-util/test-dashboard}
"Dashboard that has link cards should render correctly"
{:card (pulse.test-util/checkins-query-card {})
@@ -510,10 +517,10 @@
(thunk))))
:assert
{:email
- (fn [_ _]
+ (fn [_ [email]]
(is (every?
true?
- (-> (mt/summarize-multipart-email
+ (-> (mt/summarize-multipart-single-email email
#"https://metabase\.com/testmb/collection/\d+"
#"Linked collection name"
#"Linked collection desc"
@@ -598,8 +605,8 @@
(pulse.test-util/thunk->boolean pulse-results))))}}))
(deftest mrkdwn-length-limit-test
- (with-redefs [metabase.pulse/block-text-length-limit 10]
- (tests {:pulse {:skip_if_empty false}, :dashcard {:row 0, :col 0}}
+ (with-redefs [channel.slack/block-text-length-limit 10]
+ (tests! {:pulse {:skip_if_empty false}, :dashcard {:row 0, :col 0}}
"Dashboard subscription that includes a Markdown card that exceeds Slack's length limit when converted to mrkdwn"
{:card (pulse.test-util/checkins-query-card {})
@@ -618,7 +625,7 @@
(nth (:attachments (pulse.test-util/thunk->boolean pulse-results)) 2))))}})))
(deftest archived-dashboard-test
- (tests {:dashboard {:archived true}}
+ (tests! {:dashboard {:archived true}}
"Dashboard subscriptions are not sent if dashboard is archived"
{:card (pulse.test-util/checkins-query-card {})
@@ -628,8 +635,8 @@
(is (= {:attachments []} (pulse.test-util/thunk->boolean pulse-results))))
:email
- (fn [_ _]
- (is (= {} (mt/summarize-multipart-email))))}}))
+ (fn [_ emails]
+ (is (zero? (count emails))))}}))
(deftest use-default-values-test
(testing "Dashboard Subscriptions SHOULD use default values for Dashboard parameters when running (#20516)"
@@ -806,8 +813,8 @@
(@#'metabase.pulse/execute-dashboard {:creator_id (mt/user->id :rasta)} dashboard))))))
(deftest render-dashboard-with-tabs-test
- (tests {:pulse {:skip_if_empty false}
- :dashboard pulse.test-util/test-dashboard}
+ (tests! {:pulse {:skip_if_empty false}
+ :dashboard pulse.test-util/test-dashboard}
"Dashboard that has link cards should render correctly"
{:card (pulse.test-util/checkins-query-card {})
@@ -842,10 +849,10 @@
(thunk))))
:assert
{:email
- (fn [_ _]
+ (fn [_ [email]]
(is (every?
true?
- (-> (mt/summarize-multipart-email
+ (-> (mt/summarize-multipart-single-email email
#"The first tab"
#"Card 1 tab-1"
#"Card 2 tab-1"
@@ -945,3 +952,67 @@
(let [tmp (#'messages/create-temp-file ".tmp")
{:keys [file-name]} (#'messages/create-result-attachment-map :csv "テストSQL質å•" tmp)]
(is (= "テストSQL質å•" (first (str/split file-name #"_")))))))
+
+(deftest dashboard-description-markdown-test
+ (testing "Dashboard description renders markdown"
+ (mt/with-temp [Card {card-id :id} {:name "Test card"
+ :dataset_query {:database (mt/id)
+ :type :native
+ :native {:query "select * from checkins"}}
+ :display :table}
+ Dashboard {dashboard-id :id} {:description "# dashboard description"}
+ DashboardCard {dashboard-card-id :id} {:dashboard_id dashboard-id
+ :card_id card-id}
+ Pulse {pulse-id :id} {:name "Pulse Name"
+ :dashboard_id dashboard-id}
+ PulseCard _ {:pulse_id pulse-id
+ :card_id card-id
+ :dashboard_card_id dashboard-card-id}
+ PulseChannel {pc-id :id} {:pulse_id pulse-id}
+ PulseChannelRecipient _ {:user_id (pulse.test-util/rasta-id)
+ :pulse_channel_id pc-id}]
+ (is (= "<h1>dashboard description</h1>"
+ (->> (pulse.test-util/with-captured-channel-send-messages!
+ (metabase.pulse/send-pulse! (t2/select-one :model/Pulse pulse-id)))
+ :channel/email first :message first :content
+ (re-find #"<h1>dashboard description</h1>")))))))
+
+
+(deftest attachments-test
+ (tests!
+ {:card (pulse.test-util/checkins-query-card {})}
+ "csv"
+ {:pulse-card {:include_csv true}
+ :assert
+ {:email
+ (fn [_ [email]]
+ (is (= (rasta-dashsub-message {:message [{"Aviary KPIs" true}
+ pulse.test-util/png-attachment
+ pulse.test-util/csv-attachment]})
+ (-> (mt/summarize-multipart-single-email email
+ #"Aviary KPIs")))))}}
+
+ "xlsx"
+ {:pulse-card {:include_xls true}
+ :assert
+ {:email
+ (fn [_ [email]]
+ (is (= (rasta-dashsub-message {:message [{"Aviary KPIs" true}
+ pulse.test-util/png-attachment
+ pulse.test-util/xls-attachment]})
+ (-> (mt/summarize-multipart-single-email email
+ #"Aviary KPIs")))))}}
+
+ "no result should not include csv"
+ {:card {:dataset_query (mt/mbql-query venues {:filter [:= $id -1]})}
+ :pulse-card {:include_csv true}
+ :assert
+ {:email
+ (fn [_ [email]]
+ (is (= (rasta-dashsub-message {:message [{"Aviary KPIs" true}
+ ;; no result
+ pulse.test-util/png-attachment
+ ;; icon
+ pulse.test-util/png-attachment]})
+ (-> (mt/summarize-multipart-single-email email
+ #"Aviary KPIs")))))}}))
diff --git a/test/metabase/email_test.clj b/test/metabase/email_test.clj
index 062e76dd3690549fa1a3a5b95bc8ab368f3e5537..b4ba1917014fb5f82f2bef426cf5ac291940dca2 100644
--- a/test/metabase/email_test.clj
+++ b/test/metabase/email_test.clj
@@ -190,23 +190,30 @@
(update :content-id boolean)
(u/update-if-exists :file-name strip-timestamp)))
+(defn summarize-multipart-single-email
+ [email & regexes]
+ (let [email-body->regex-boolean (create-email-body->regex-fn regexes)
+ body-or-content (fn [email-body-seq]
+ (doall
+ (for [{email-type :type :as email-part} email-body-seq]
+ (if (string? email-type)
+ (email-body->regex-boolean email-part)
+ (summarize-attachment email-part)))))]
+ (cond-> email
+ (:recipients email) (update :recipients set)
+ (:to email) (update :to set)
+ (:bcc email) (update :bcc set)
+ (:message email) (update :message body-or-content)
+ (:body email) (update :body body-or-content))))
+
(defn summarize-multipart-email
"For text/html portions of an email, this is similar to `regex-email-bodies`, but for images in the attachments will
summarize the contents for comparison in expects"
[& regexes]
- (let [email-body->regex-boolean (create-email-body->regex-fn regexes)]
- (m/map-vals (fn [emails-for-recipient]
- (for [email emails-for-recipient]
- (cond-> email
- (:to email) (update :to set)
- (:bcc email) (update :bcc set)
- true (update :body (fn [email-body-seq]
- (doall
- (for [{email-type :type :as email-part} email-body-seq]
- (if (string? email-type)
- (email-body->regex-boolean email-part)
- (summarize-attachment email-part)))))))))
- @inbox)))
+ (m/map-vals (fn [emails-for-recipient]
+ (for [email emails-for-recipient]
+ (apply summarize-multipart-single-email email regexes)))
+ @inbox))
(defn email-to
"Creates a default email map for `user-or-user-kwd`, as would be returned by `with-fake-inbox`."
@@ -271,11 +278,11 @@
(is (= 1 (count (filter #{:metabase-email/messages} @calls))))
(is (= 0 (count (filter #{:metabase-email/message-errors} @calls))))))
(testing "error metrics collection"
- (let [calls (atom nil)]
- (let [retry-config (assoc (#'retry/retry-configuration)
- :max-attempts 1
- :initial-interval-millis 1)
- test-retry (retry/random-exponential-backoff-retry "test-retry" retry-config)]
+ (let [calls (atom nil)
+ retry-config (assoc (#'retry/retry-configuration)
+ :max-attempts 1
+ :initial-interval-millis 1)
+ test-retry (retry/random-exponential-backoff-retry "test-retry" retry-config)]
(with-redefs [prometheus/inc #(swap! calls conj %)
retry/decorate (rt/test-retry-decorate-fn test-retry)
email/send-email! (fn [_ _] (throw (Exception. "test-exception")))]
@@ -285,7 +292,7 @@
:message-type :html
:message "101. Metabase will make you a better person"))
(is (= 1 (count (filter #{:metabase-email/messages} @calls))))
- (is (= 1 (count (filter #{:metabase-email/message-errors} @calls)))))))
+ (is (= 1 (count (filter #{:metabase-email/message-errors} @calls))))))
(testing "basic sending without email-from-name"
(tu/with-temporary-setting-values [email-from-name nil]
(is (=
diff --git a/test/metabase/pulse/pulse_integration_test.clj b/test/metabase/pulse/pulse_integration_test.clj
index 3795dd797358b47b5f30b197b37abf0686c79a40..fdd5746a4a6ccba84e9a3f8fc4a1016a8d3c27af 100644
--- a/test/metabase/pulse/pulse_integration_test.clj
+++ b/test/metabase/pulse/pulse_integration_test.clj
@@ -12,6 +12,7 @@
[metabase.email :as email]
[metabase.models :refer [Card Collection Dashboard DashboardCard Pulse PulseCard PulseChannel PulseChannelRecipient]]
[metabase.pulse]
+ [metabase.pulse.test-util :as pulse.test-util]
[metabase.test :as mt]
[metabase.util :as u]))
@@ -55,29 +56,29 @@
:query {:source-table (format "card__%s" ~model-card-id)}}}]
~@body)))
-(defn- run-pulse-and-return-last-data-columns
+(defn- run-pulse-and-return-last-data-columns!
"Simulate sending the pulse email, get the html body of the response, then return the last columns of each pulse body
element. In our test cases that's the Tax Rate column."
[pulse]
- (mt/with-fake-inbox
- (with-redefs [email/bcc-enabled? (constantly false)]
- (mt/with-test-user nil
- (metabase.pulse/send-pulse! pulse)))
- (let [html-body (get-in @mt/inbox ["rasta@metabase.com" 0 :body 0 :content])
- doc (-> html-body hik/parse hik/as-hickory)
- data-tables (hik.s/select
- (hik.s/class "pulse-body")
- doc)]
- (map
- (fn [data-table]
- (->> (hik.s/select
- (hik.s/child
- (hik.s/tag :tbody)
- (hik.s/tag :tr)
- hik.s/last-child)
- data-table)
- (map (comp first :content))))
- data-tables))))
+ (let [channel-messages (pulse.test-util/with-captured-channel-send-messages!
+ (with-redefs [email/bcc-enabled? (constantly false)]
+ (mt/with-test-user nil
+ (metabase.pulse/send-pulse! pulse))))
+ html-body (-> channel-messages :channel/email first :message first :content)
+ doc (-> html-body hik/parse hik/as-hickory)
+ data-tables (hik.s/select
+ (hik.s/class "pulse-body")
+ doc)]
+ (map
+ (fn [data-table]
+ (->> (hik.s/select
+ (hik.s/child
+ (hik.s/tag :tbody)
+ (hik.s/tag :tr)
+ hik.s/last-child)
+ data-table)
+ (map (comp first :content))))
+ data-tables)))
(def ^:private all-pct-2d?
"Is every element in the sequence percent formatted with 2 significant digits?"
@@ -117,7 +118,7 @@
:user_id (mt/user->id :rasta)}]
(let [[base-data-row
model-data-row
- question-data-row] (run-pulse-and-return-last-data-columns pulse)]
+ question-data-row] (run-pulse-and-return-last-data-columns! pulse)]
(testing "The data from the first question is just numbers."
(is (all-float? base-data-row)))
(testing "The data from the second question (a model) is percent formatted"
@@ -134,7 +135,8 @@
(with-metadata-data-cards [base-card-id model-card-id question-card-id]
(testing "The data from the first question is just numbers."
(mt/with-temp [Pulse {pulse-id :id
- :as pulse} {:name "Test Pulse"}
+ :as pulse} {:name "Test Pulse"
+ :alert_condition "rows"}
PulseCard _ {:pulse_id pulse-id
:card_id base-card-id}
PulseChannel {pulse-channel-id :id} {:channel_type :email
@@ -142,10 +144,11 @@
:enabled true}
PulseChannelRecipient _ {:pulse_channel_id pulse-channel-id
:user_id (mt/user->id :rasta)}]
- (is (all-float? (first (run-pulse-and-return-last-data-columns pulse))))))
+ (is (all-float? (first (run-pulse-and-return-last-data-columns! pulse))))))
(testing "The data from the second question (a model) is percent formatted"
(mt/with-temp [Pulse {pulse-id :id
- :as pulse} {:name "Test Pulse"}
+ :as pulse} {:name "Test Pulse"
+ :alert_condition "rows"}
PulseCard _ {:pulse_id pulse-id
:card_id model-card-id}
PulseChannel {pulse-channel-id :id} {:channel_type :email
@@ -153,10 +156,11 @@
:enabled true}
PulseChannelRecipient _ {:pulse_channel_id pulse-channel-id
:user_id (mt/user->id :rasta)}]
- (is (all-pct-2d? (first (run-pulse-and-return-last-data-columns pulse))))))
+ (is (all-pct-2d? (first (run-pulse-and-return-last-data-columns! pulse))))))
(testing "The data from the last question (based on a a model) is percent formatted"
(mt/with-temp [Pulse {pulse-id :id
- :as pulse} {:name "Test Pulse"}
+ :as pulse} {:name "Test Pulse"
+ :alert_condition "rows"}
PulseCard _ {:pulse_id pulse-id
:card_id question-card-id}
PulseChannel {pulse-channel-id :id} {:channel_type :email
@@ -164,7 +168,7 @@
:enabled true}
PulseChannelRecipient _ {:pulse_channel_id pulse-channel-id
:user_id (mt/user->id :rasta)}]
- (is (all-pct-2d? (first (run-pulse-and-return-last-data-columns pulse)))))))))
+ (is (all-pct-2d? (first (run-pulse-and-return-last-data-columns! pulse)))))))))
(defn- strip-timestamp
"Remove the timestamp portion of attachment filenames.
@@ -177,25 +181,26 @@
name-parts (butlast (str/split fname #"_"))]
(format "%s.%s" (str/join "_" name-parts) ext)))
-(defn- run-pulse-and-return-attached-csv-data
+(defn- run-pulse-and-return-attached-csv-data!
"Simulate sending the pulse email, get the attached text/csv content, and parse into a map of
attachment name -> column name -> column data"
[pulse]
- (mt/with-fake-inbox
- (with-redefs [email/bcc-enabled? (constantly false)]
- (mt/with-test-user nil
- (metabase.pulse/send-pulse! pulse)))
- (->>
- (get-in @mt/inbox ["rasta@metabase.com" 0 :body])
- (keep
- (fn [{:keys [type content-type file-name content]}]
- (when (and
- (= :attachment type)
- (= "text/csv" content-type))
- [(strip-timestamp file-name)
- (let [[h & r] (csv/read-csv (slurp content))]
- (zipmap h (apply mapv vector r)))])))
- (into {}))))
+ (with-redefs [email/bcc-enabled? (constantly false)]
+ (->> (mt/with-test-user nil
+ (pulse.test-util/with-captured-channel-send-messages!
+ (metabase.pulse/send-pulse! pulse)))
+ :channel/email
+ first
+ :message
+ (keep
+ (fn [{:keys [type content-type file-name content]}]
+ (when (and
+ (= :attachment type)
+ (= "text/csv" content-type))
+ [(strip-timestamp file-name)
+ (let [[h & r] (csv/read-csv (slurp content))]
+ (zipmap h (apply mapv vector r)))])))
+ (into {}))))
(deftest apply-formatting-in-csv-dashboard-test
(testing "An exported dashboard should preserve the formatting specified in the column metadata (#36320)"
@@ -224,7 +229,7 @@
:enabled true}
PulseChannelRecipient _ {:pulse_channel_id pulse-channel-id
:user_id (mt/user->id :rasta)}]
- (let [parsed-data (run-pulse-and-return-attached-csv-data pulse)]
+ (let [parsed-data (run-pulse-and-return-attached-csv-data! pulse)]
(testing "The base model has no special formatting"
(is (all-float? (get-in parsed-data ["Base question - no special metadata.csv" "Tax Rate"]))))
(testing "The model with metadata formats the Tax Rate column with the user-defined semantic type"
@@ -237,7 +242,8 @@
(with-metadata-data-cards [base-card-id model-card-id question-card-id]
(testing "The attached data from the first question is just numbers."
(mt/with-temp [Pulse {pulse-id :id
- :as pulse} {:name "Test Pulse"}
+ :as pulse} {:name "Test Pulse"
+ :alert_condition "rows"}
PulseCard _ {:pulse_id pulse-id
:card_id base-card-id}
PulseChannel {pulse-channel-id :id} {:channel_type :email
@@ -245,11 +251,12 @@
:enabled true}
PulseChannelRecipient _ {:pulse_channel_id pulse-channel-id
:user_id (mt/user->id :rasta)}]
- (let [parsed-data (run-pulse-and-return-attached-csv-data pulse)]
+ (let [parsed-data (run-pulse-and-return-attached-csv-data! pulse)]
(is (all-float? (get-in parsed-data ["Base question - no special metadata.csv" "Tax Rate"]))))))
(testing "The attached data from the second question (a model) is percent formatted"
(mt/with-temp [Pulse {pulse-id :id
- :as pulse} {:name "Test Pulse"}
+ :as pulse} {:name "Test Pulse"
+ :alert_condition "rows"}
PulseCard _ {:pulse_id pulse-id
:card_id model-card-id}
PulseChannel {pulse-channel-id :id} {:channel_type :email
@@ -257,11 +264,12 @@
:enabled true}
PulseChannelRecipient _ {:pulse_channel_id pulse-channel-id
:user_id (mt/user->id :rasta)}]
- (let [parsed-data (run-pulse-and-return-attached-csv-data pulse)]
+ (let [parsed-data (run-pulse-and-return-attached-csv-data! pulse)]
(is (all-pct-2d? (get-in parsed-data ["Model with percent semantic type.csv" "Tax Rate"]))))))
(testing "The attached data from the last question (based on a a model) is percent formatted"
(mt/with-temp [Pulse {pulse-id :id
- :as pulse} {:name "Test Pulse"}
+ :as pulse} {:name "Test Pulse"
+ :alert_condition "rows"}
PulseCard _ {:pulse_id pulse-id
:card_id question-card-id}
PulseChannel {pulse-channel-id :id} {:channel_type :email
@@ -269,7 +277,7 @@
:enabled true}
PulseChannelRecipient _ {:pulse_channel_id pulse-channel-id
:user_id (mt/user->id :rasta)}]
- (let [parsed-data (run-pulse-and-return-attached-csv-data pulse)]
+ (let [parsed-data (run-pulse-and-return-attached-csv-data! pulse)]
(is (all-pct-2d? (get-in parsed-data ["Query based on model.csv" "Tax Rate"])))))))))
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; Consistent Date Formatting ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
@@ -367,7 +375,8 @@
DashboardCard {metamodel-dash-card-id :id} {:dashboard_id dash-id
:card_id meta-model-card-id}
Pulse {pulse-id :id
- :as pulse} {:name "Consistent Time Formatting Pulse"}
+ :as pulse} {:name "Consistent Time Formatting Pulse"
+ :dashboard_id dash-id}
PulseCard _ {:pulse_id pulse-id
:card_id native-card-id
:dashboard_card_id base-dash-card-id
@@ -385,7 +394,7 @@
:enabled true}
PulseChannelRecipient _ {:pulse_channel_id pulse-channel-id
:user_id (mt/user->id :rasta)}]
- (let [attached-data (run-pulse-and-return-attached-csv-data pulse)
+ (let [attached-data (run-pulse-and-return-attached-csv-data! pulse)
get-res #(-> (get attached-data %)
(update-vals first)
(dissoc "X"))
@@ -536,7 +545,8 @@
DashboardCard {question-dash-card-id :id} {:dashboard_id dash-id
:card_id question-card-id}
Pulse {pulse-id :id
- :as pulse} {:name "Consistent Column Names"}
+ :as pulse} {:name "Consistent Column Names"
+ :dashboard_id dash-id}
PulseCard _ {:pulse_id pulse-id
:card_id base-card-id
:dashboard_card_id base-dash-card-id
diff --git a/test/metabase/pulse/test_util.clj b/test/metabase/pulse/test_util.clj
index cc470826bc4953142dffb1b5054179a2d8798b16..a75bc38862b217f9b068d2d6d4e2547ea7d141a4 100644
--- a/test/metabase/pulse/test_util.clj
+++ b/test/metabase/pulse/test_util.clj
@@ -2,9 +2,8 @@
(:require
[clojure.walk :as walk]
[medley.core :as m]
+ [metabase.channel.core :as channel]
[metabase.integrations.slack :as slack]
- [metabase.models.pulse :refer [Pulse]]
- [metabase.models.pulse-card :refer [PulseCard]]
[metabase.pulse :as pulse]
[metabase.query-processor.test-util :as qp.test-util]
[metabase.test :as mt]
@@ -17,13 +16,18 @@
(defn send-pulse-created-by-user!
"Create a Pulse with `:creator_id` of `user-kw`, and simulate sending it, executing it and returning the results."
[user-kw card]
- (t2.with-temp/with-temp [Pulse pulse {:creator_id (test.users/user->id user-kw)}
- PulseCard _ {:pulse_id (:id pulse), :card_id (u/the-id card)}]
- (with-redefs [pulse/send-notifications! identity
- pulse/parts->notifications (fn [_ results]
- (vec results))]
- (let [[{:keys [result]}] (pulse/send-pulse! pulse)]
- (qp.test-util/rows result)))))
+ (t2.with-temp/with-temp [:model/Pulse pulse {:creator_id (test.users/user->id user-kw)
+ :alert_condition "rows"}
+ :model/PulseCard _ {:pulse_id (:id pulse), :card_id (u/the-id card)}]
+ (let [pulse-result (atom nil)
+ orig-execute-pulse @#'pulse/execute-pulse]
+ (with-redefs [channel/send! (fn [& _args]
+ :noop)
+ pulse/execute-pulse (fn [& args]
+ (u/prog1 (apply orig-execute-pulse args)
+ (reset! pulse-result <>)))]
+ (pulse/send-pulse! pulse)
+ (qp.test-util/rows (:result (first @pulse-result)))))))
(def card-name "Test card")
@@ -68,13 +72,32 @@
`(mt/with-fake-inbox
(do-with-site-url (fn [] ~@body))))
-(defmacro slack-test-setup
+(defmacro slack-test-setup!
"Macro that ensures test-data is present and disables sending of all notifications"
[& body]
- `(with-redefs [pulse/send-notifications! realize-lazy-seqs
- slack/files-channel (constantly "FOO")]
+ `(with-redefs [channel/send! (fn [& _args#]
+ :noop)
+ slack/files-channel (constantly "FOO")]
(do-with-site-url (fn [] ~@body))))
+(def ^:dynamic *channel-messages* nil)
+
+(defn do-with-captured-channel-send-messages!
+ [thunk]
+ (let [channel-messages (atom nil)]
+ (with-redefs [channel/send! (fn [channel-type message]
+ (swap! channel-messages update channel-type conj message))]
+ (thunk)
+ @channel-messages)))
+
+(defmacro with-captured-channel-send-messages!
+ "Macro that captures all messages sent to channels in the body of the macro.
+ Returns a map of channel-type -> messages sent to that channel."
+ [& body]
+ `(do-with-captured-channel-send-messages!
+ (fn []
+ ~@body)))
+
(def png-attachment
{:type :inline
:content-id true
diff --git a/test/metabase/pulse_test.clj b/test/metabase/pulse_test.clj
index 3401a42acdf394b175425a0b2a6d0ecf6c3b1f92..fd5dffafee2557cb46c09cb34fc78357a8227162 100644
--- a/test/metabase/pulse_test.clj
+++ b/test/metabase/pulse_test.clj
@@ -1,15 +1,14 @@
(ns metabase.pulse-test
+ "These are mostly Alerts test, dashboard subscriptions could be found in
+ [[metabase.dashboard-subscription-test]]."
(:require
[clojure.java.io :as io]
[clojure.string :as str]
[clojure.test :refer :all]
- [medley.core :as m]
[metabase.email :as email]
[metabase.integrations.slack :as slack]
[metabase.models
:refer [Card Collection Pulse PulseCard PulseChannel PulseChannelRecipient]]
- [metabase.models.dashboard :refer [Dashboard]]
- [metabase.models.dashboard-card :refer [DashboardCard]]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as perms-group]
[metabase.models.pulse :as pulse]
@@ -32,19 +31,17 @@
;;; | Util Fns & Macros |
;;; +----------------------------------------------------------------------------------------------------------------+
-(defn- rasta-pulse-email [& [email]]
- (mt/email-to :rasta (merge {:subject "Pulse: Pulse Name"
- :body [{"Pulse Name" true}
- pulse.test-util/png-attachment
- pulse.test-util/png-attachment]
- :bcc? true}
- email)))
-
-(defn- rasta-alert-email
- [subject email-body]
- (mt/email-to :rasta {:subject subject
- :body email-body
- :bcc? true}))
+(defn- rasta-alert-message
+ [& [data]]
+ (merge {:subject "Alert: Test card has results"
+ :recipients #{"rasta@metabase.com"}
+ :message-type :attachments,
+ :message [{pulse.test-util/card-name true}
+ ;; card static-viz
+ pulse.test-util/png-attachment
+ ;; icon
+ pulse.test-util/png-attachment]}
+ data))
(defn do-with-pulse-for-card
"Creates a Pulse and other relevant rows for a `card` (using `pulse` and `pulse-card` properties if specified), then
@@ -54,12 +51,13 @@
[{:keys [pulse pulse-card channel card]
:or {channel :email}}
f]
- (mt/with-temp [Pulse {pulse-id :id, :as pulse}
- (-> pulse
- (merge {:name "Pulse Name"}))
- PulseCard _ (merge {:pulse_id pulse-id
- :card_id (u/the-id card)
- :position 0}
+ (mt/with-temp [Pulse {pulse-id :id, :as pulse} (->> pulse
+ (merge {:name "Pulse Name"
+ :alert_condition "rows"}))
+ PulseCard _ (merge {:pulse_id pulse-id
+ :card_id (u/the-id card)
+ :position 0}
+
pulse-card)
PulseChannel {pc-id :id} (case channel
:email
@@ -83,7 +81,7 @@
[[pulse-binding properties] & body]
`(do-with-pulse-for-card ~properties (fn [~pulse-binding] ~@body)))
-(defn- do-test
+(defn- do-test!
"Run a single Pulse test with a standard set of boilerplate. Creates Card, Pulse, and other related objects using
`card`, `pulse`, `pulse-card` properties, then sends the Pulse; finally, test assertions in `assert` are invoked.
`assert` can contain `:email` and/or `:slack` assertions, which are used to test an email and Slack version of that
@@ -105,27 +103,31 @@
:when f]
(assert (fn? f))
(testing (format "sent to %s channel" channel-type)
- (testing (when (= :email channel-type) @mt/inbox)
- (mt/with-temp [Card {card-id :id} (merge {:name pulse.test-util/card-name
- :display (or display :line)}
- card)]
- (with-pulse-for-card [{pulse-id :id}
- {:card card-id
- :pulse pulse
- :pulse-card pulse-card
- :channel channel-type}]
- (letfn [(thunk* []
- (f {:card-id card-id, :pulse-id pulse-id}
- (metabase.pulse/send-pulse! (pulse/retrieve-notification pulse-id))))
- (thunk []
- (if fixture
- (fixture {:card-id card-id, :pulse-id pulse-id} thunk*)
- (thunk*)))]
- (case channel-type
- :email (pulse.test-util/email-test-setup (thunk))
- :slack (pulse.test-util/slack-test-setup (thunk))))))))))
-
-(defn- tests
+ (mt/with-temp [Card {card-id :id} (merge {:name pulse.test-util/card-name
+ :display (or display :line)}
+ card)]
+ (with-pulse-for-card [{pulse-id :id}
+ {:card card-id
+ :pulse pulse
+ :pulse-card pulse-card
+ :channel channel-type}]
+ (letfn [(thunk* []
+ (f {:card-id card-id, :pulse-id pulse-id}
+ ((if (= :email channel-type)
+ :channel/email
+ :channel/slack)
+ (pulse.test-util/with-captured-channel-send-messages!
+ (mt/with-temporary-setting-values [site-url "https://metabase.com/testmb"]
+ (metabase.pulse/send-pulse! (t2/select-one :model/Pulse pulse-id)))))))
+ (thunk []
+ (if fixture
+ (fixture {:card-id card-id, :pulse-id pulse-id} thunk*)
+ (thunk*)))]
+ (case channel-type
+ :email (thunk)
+ :slack (pulse.test-util/slack-test-setup! (thunk)))))))))
+
+(defn- tests!
"Convenience for writing multiple tests using `do-test`. `common` is a map of shared properties as passed to `do-test`
that is deeply merged with the individual maps for each test. Other args are alternating `testing` context messages
and properties as passed to `do-test`:
@@ -145,10 +147,10 @@
[common & {:as message->m}]
(doseq [[message m] message->m]
(testing message
- (do-test (merge-with merge common m)))))
+ (do-test! (merge-with merge common m)))))
(def ^:private test-card-result {pulse.test-util/card-name true})
-(def ^:private test-card-regex (re-pattern pulse.test-util/card-name))
+(def ^:private test-card-regex (re-pattern pulse.test-util/card-name))
(defn- produces-bytes? [{:keys [rendered-info]}]
@@ -156,58 +158,40 @@
(pos? (alength (or (render/png-from-render-info rendered-info 500)
(byte-array 0))))))
-(defn- email-body? [{message-type :type, ^String content :content}]
- (and (= "text/html; charset=utf-8" message-type)
- (string? content)
- (.startsWith content "<!doctype html>")))
-
-(defn- attachment? [{message-type :type, content-type :content-type, content :content}]
- (and (= :inline message-type)
- (= "image/png" content-type)
- (instance? java.net.URL content)))
-
-(defn- add-rasta-attachment
- "Append `attachment` to the first email found for Rasta"
- [email attachment]
- (update-in email ["rasta@metabase.com" 0] #(update % :body conj attachment)))
-
-
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Tests |
;;; +----------------------------------------------------------------------------------------------------------------+
(deftest basic-timeseries-test
- (do-test
+ (do-test!
{:card
(merge
- (pulse.test-util/checkins-query-card {:breakout [!day.date]})
+ (pulse.test-util/checkins-query-card {:breakout [!day.date]
+ :limit 1})
{:visualization_settings {:graph.dimensions ["DATE"]
:graph.metrics ["count"]}})
- :pulse {:skip_if_empty false}
-
:assert
{:email
- (fn [_ _]
- (is (= (rasta-pulse-email)
- (mt/summarize-multipart-email #"Pulse Name"))))
+ (fn [_ [email]]
+ (is (= (rasta-alert-message)
+ (mt/summarize-multipart-single-email email test-card-regex))))
:slack
(fn [{:keys [card-id]} [pulse-results]]
- (is (= {:channel-id "#general"
- :attachments
- [{:blocks [{:type "header", :text {:type "plain_text", :text "Pulse: Pulse Name", :emoji true}}
- {:type "section", :fields [{:type "mrkdwn", :text "Sent by Rasta Toucan"}]}]}
- {:title pulse.test-util/card-name
- :rendered-info {:attachments false
- :content true}
- :title_link (str "https://metabase.com/testmb/question/" card-id)
- :attachment-name "image.png"
- :channel-id "FOO"
- :fallback pulse.test-util/card-name}]}
- (pulse.test-util/thunk->boolean pulse-results))))}}))
+ (is (= {:channel-id "#general"
+ :attachments
+ [{:blocks [{:type "header", :text {:type "plain_text", :text "🔔 Test card", :emoji true}}]}
+ {:title pulse.test-util/card-name
+ :rendered-info {:attachments false
+ :content true}
+ :title_link (str "https://metabase.com/testmb/question/" card-id)
+ :attachment-name "image.png"
+ :channel-id "FOO"
+ :fallback pulse.test-util/card-name}]}
+ (pulse.test-util/thunk->boolean pulse-results))))}}))
(deftest basic-table-test
- (tests {:pulse {:skip_if_empty false} :display :table}
+ (tests! {:display :table}
"9 results, so no attachment"
{:card (pulse.test-util/checkins-query-card {:aggregation nil, :limit 9})
@@ -218,14 +202,15 @@
:assert
{:email
- (fn [_ _]
- (is (= (rasta-pulse-email {:body [{"Pulse Name" true
- "More results have been included" false
- "ID</th>" true
- "<a href=\\\"https://metabase.com/testmb/dashboard/" false}
- pulse.test-util/png-attachment]})
- (mt/summarize-multipart-email
- #"Pulse Name"
+ (fn [_ [email]]
+ (is (= (rasta-alert-message {:message [{pulse.test-util/card-name true
+ "More results have been included" false
+ "ID</th>" true
+ "<a href=\\\"https://metabase.com/testmb/dashboard/" false}
+ pulse.test-util/png-attachment]})
+ (mt/summarize-multipart-single-email
+ email
+ test-card-regex
#"More results have been included"
#"ID</th>"
#"<a href=\"https://metabase.com/testmb/dashboard/"))))
@@ -237,8 +222,7 @@
(is (= {:channel-id "#general"
:attachments
[{:blocks
- [{:type "header", :text {:type "plain_text", :text "Pulse: Pulse Name", :emoji true}}
- {:type "section", :fields [{:type "mrkdwn", :text "Sent by Rasta Toucan"}]}]}
+ [{:type "header", :text {:type "plain_text", :text "🔔 Test card", :emoji true}}]}
{:title pulse.test-util/card-name
:rendered-info {:attachments false
:content true}
@@ -259,65 +243,64 @@
:assert
{:email
- (fn [_ _]
- (is (= (rasta-pulse-email {:body [{"Pulse Name" true
- "More results have been included" false
- "ID</th>" true}
- pulse.test-util/png-attachment]})
- (mt/summarize-multipart-email
- #"Pulse Name"
+ (fn [_ [email]]
+ (is (= (rasta-alert-message {:message [{pulse.test-util/card-name true
+ "More results have been included" false
+ "ID</th>" true}
+ pulse.test-util/png-attachment]})
+ (mt/summarize-multipart-single-email
+ email
+ test-card-regex
#"More results have been included" #"ID</th>"))))}}))
(deftest csv-test
- (tests {:pulse {:skip_if_empty false}
- :card (merge
- (pulse.test-util/checkins-query-card {:breakout [!day.date]})
- {:visualization_settings {:graph.dimensions ["DATE"]
- :graph.metrics ["count"]}})}
+ (tests! {:pulse {}
+ :card (merge
+ (pulse.test-util/checkins-query-card {:breakout [!day.date]})
+ {:visualization_settings {:graph.dimensions ["DATE"]
+ :graph.metrics ["count"]}})}
"alert with a CSV"
{:pulse-card {:include_csv true}
:assert
{:email
- (fn [_ _]
- (is (= (rasta-alert-email "Pulse: Pulse Name"
- [test-card-result
- pulse.test-util/png-attachment
- pulse.test-util/png-attachment
- pulse.test-util/csv-attachment])
- (mt/summarize-multipart-email test-card-regex))))}}
+ (fn [_ [email]]
+ (is (= (rasta-alert-message {:message [test-card-result
+ pulse.test-util/png-attachment
+ pulse.test-util/png-attachment
+ pulse.test-util/csv-attachment]})
+ (mt/summarize-multipart-single-email email test-card-regex))))}}
"With a \"rows\" type of pulse (table visualization) we should not include the CSV by default, per issue #36441"
{:card {:display :table :dataset_query (mt/mbql-query checkins)}
:assert
{:email
- (fn [_ _]
- (is (= (-> (rasta-pulse-email)
- ;; There's no PNG with a table visualization, so only assert on one png (the dashboard icon)
- (assoc-in ["rasta@metabase.com" 0 :body] [{"Pulse Name" true} pulse.test-util/png-attachment]))
- (mt/summarize-multipart-email #"Pulse Name"))))}}))
+ (fn [_ [email]]
+ ;; There's no PNG with a table visualization, so only assert on one png (the dashboard icon)
+ (is (= (rasta-alert-message {:message [{pulse.test-util/card-name true} pulse.test-util/png-attachment]})
+ (mt/summarize-multipart-single-email email test-card-regex))))}}))
(deftest xls-test
(testing "If the pulse is already configured to send an XLS, no need to include a CSV"
- (do-test
+ (do-test!
{:card {:dataset_query (mt/mbql-query checkins)}
:pulse-card {:include_xls true}
:display :table
:assert
{:email
- (fn [_ _]
- (is (= (-> (rasta-pulse-email)
- ;; There's no PNG with a table visualization, so only assert on one png (the dashboard icon)
- (assoc-in ["rasta@metabase.com" 0 :body] [{"Pulse Name" true} pulse.test-util/png-attachment])
- (add-rasta-attachment pulse.test-util/xls-attachment))
- (mt/summarize-multipart-email #"Pulse Name"))))}})))
+ (fn [_ [email]]
+ (is (= ;; There's no PNG with a table visualization, so only assert on one png (the dashboard icon)
+ (rasta-alert-message {:message [{pulse.test-util/card-name true}
+ pulse.test-util/png-attachment
+ pulse.test-util/xls-attachment]})
+ (mt/summarize-multipart-single-email email test-card-regex))))}})))
;; Not really sure how this is significantly different from `xls-test`
(deftest xls-test-2
(testing "Basic test, 1 card, 1 recipient, with XLS attachment"
- (do-test
+ (do-test!
{:card
(merge
(pulse.test-util/checkins-query-card {:breakout [!day.date]})
@@ -326,27 +309,16 @@
:pulse-card {:include_xls true}
:assert
{:email
- (fn [_ _]
- (is (= (add-rasta-attachment (rasta-pulse-email) pulse.test-util/xls-attachment)
- (mt/summarize-multipart-email #"Pulse Name"))))}})))
-
-(deftest csv-xls-no-data-test
- (testing "card with CSV and XLS attachments, but no data. Should not include an attachment"
- (do-test
- {:card (pulse.test-util/checkins-query-card {:filter [:> $date "2017-10-24"]
- :breakout [!day.date]})
- :pulse {:skip_if_empty false}
- :pulse-card {:include_csv true
- :include_xls true}
- :assert
- {:email
- (fn [_ _]
- (is (= (rasta-pulse-email)
- (mt/summarize-multipart-email #"Pulse Name"))))}})))
+ (fn [_ [email]]
+ (is (= (rasta-alert-message {:message [{pulse.test-util/card-name true}
+ pulse.test-util/png-attachment
+ pulse.test-util/png-attachment
+ pulse.test-util/xls-attachment]})
+ (mt/summarize-multipart-single-email email test-card-regex))))}})))
(deftest ensure-constraints-test
(testing "Validate pulse queries are limited by `default-query-constraints`"
- (do-test
+ (do-test!
{:card
(pulse.test-util/checkins-query-card {:aggregation nil})
:display :table
@@ -359,27 +331,25 @@
:pulse-card {:include_csv true}
:assert
{:email
- (fn [_ _]
- (let [first-message (-> @mt/inbox vals ffirst)]
- (is (= true
- (some? first-message))
- "Should have a message in the inbox")
- (when first-message
- (let [filename (-> first-message :body last :content)
- exists? (some-> filename io/file .exists)]
- (testing "File should exist"
- (is (= true
- exists?)))
- (testing (str "tmp file = %s" filename)
- (testing "Slurp in the generated CSV and count the lines found in the file"
- (when exists?
- (testing "Should return 30 results (the redef'd limit) plus the header row"
- (is (= 31
- (-> (slurp filename) str/split-lines count)))))))))))}})))
+ (fn [_ [email]]
+ (is (= true (some? email))
+ "Should have a message in the inbox")
+ (when email
+ (let [filename (-> email :message last :content)
+ exists? (some-> filename io/file .exists)]
+ (testing "File should exist"
+ (is (= true
+ exists?)))
+ (testing (str "tmp file = %s" filename)
+ (testing "Slurp in the generated CSV and count the lines found in the file"
+ (when exists?
+ (testing "Should return 30 results (the redef'd limit) plus the header row"
+ (is (= 31
+ (-> (slurp filename) str/split-lines count))))))))))}})))
(deftest multiple-recipients-test
(testing "Pulse should be sent to two recipients"
- (do-test
+ (do-test!
{:card
(merge
(pulse.test-util/checkins-query-card {:breakout [!day.date]})
@@ -394,68 +364,31 @@
:assert
{:email
- (fn [_ _]
- (is (= (into {} (map (fn [user-kwd]
- (mt/email-to user-kwd {:subject "Pulse: Pulse Name"
- :bcc #{"rasta@metabase.com" "crowberto@metabase.com"}
- :body [{"Pulse Name" true}
- pulse.test-util/png-attachment
- pulse.test-util/png-attachment]
- :bcc? true}))
- [:rasta :crowberto]))
- (mt/summarize-multipart-email #"Pulse Name"))))}})))
-
-(deftest two-cards-in-one-pulse-test
- (testing "1 pulse that has 2 cards, should contain two query image attachments (as well as an icon attachment)"
- (do-test
- {:card
- (assoc (pulse.test-util/checkins-query-card {:breakout [!day.date]})
- :visualization_settings {:graph.dimensions ["DATE"]
- :graph.metrics ["count"]}
- :name "card 1")
-
- :fixture
- (fn [{:keys [pulse-id]} thunk]
- (mt/with-temp [Card {card-id-2 :id} (assoc (pulse.test-util/checkins-query-card {:breakout [!month.date]})
- :visualization_settings {:graph.dimensions ["DATE"]
- :graph.metrics ["count"]}
- :name "card 2"
- :display :line)
- PulseCard _ {:pulse_id pulse-id
- :card_id card-id-2
- :position 1}]
- (thunk)))
-
- :assert
- {:email
- (fn [_ _]
- (is (= (rasta-pulse-email {:body [{"Pulse Name" true}
- pulse.test-util/png-attachment
- pulse.test-util/png-attachment
- pulse.test-util/png-attachment]})
- (mt/summarize-multipart-email #"Pulse Name"))))}})))
-
-(deftest empty-results-test
- (testing "Pulse where the card has no results"
- (tests {:card (assoc (pulse.test-util/checkins-query-card {:filter [:> $date "2017-10-24"]
- :breakout [!day.date]})
- :visualization_settings {:graph.dimensions ["DATE"]
- :graph.metrics ["count"]})}
- "skip if empty = false"
- {:pulse {:skip_if_empty false}
- :assert {:email (fn [_ _]
- (is (= (rasta-pulse-email)
- (mt/summarize-multipart-email #"Pulse Name"))))}}
-
- "skip if empty = true"
- {:pulse {:skip_if_empty true}
- :assert {:email (fn [_ _]
- (is (= {}
- (mt/summarize-multipart-email #"Pulse Name"))))}})))
+ (fn [_ [email]]
+ (is (= (rasta-alert-message {:recipients #{"rasta@metabase.com" "crowberto@metabase.com"}})
+ (mt/summarize-multipart-single-email email test-card-regex))))}})))
+
+;; this should be in dashboard subscriptions
+#_(deftest empty-results-test
+ (testing "Pulse where the card has no results"
+ (tests! {:card (assoc (pulse.test-util/checkins-query-card {:filter [:> $date "2017-10-24"]
+ :breakout [!day.date]})
+ :visualization_settings {:graph.dimensions ["DATE"]
+ :graph.metrics ["count"]})}
+ "skip if empty = false"
+ {:pulse {:skip_if_empty false}
+ :assert {:email (fn [_ [email]]
+ (is (= (rasta-alert-email-2)
+ (mt/summarize-multipart-single-email email test-card-regex))))}}
+
+ "skip if empty = true"
+ {:pulse {:skip_if_empty true}
+ :assert {:email (fn [_ emails]
+ (is (empty? emails)))}})))
(deftest rows-alert-test
(testing "Rows alert"
- (tests {:pulse {:alert_condition "rows", :alert_first_only false}}
+ (tests! {:pulse {:alert_condition "rows", :alert_first_only false}}
"with data"
{:card
(merge
@@ -465,12 +398,12 @@
:assert
{:email
- (fn [_ _]
- (is (= (rasta-alert-email
- "Alert: Test card has results"
- [(assoc test-card-result "More results have been included" false)
- pulse.test-util/png-attachment pulse.test-util/png-attachment])
- (mt/summarize-multipart-email test-card-regex #"More results have been included"))))
+ (fn [_ [email]]
+ (is (= (rasta-alert-message {:message [{pulse.test-util/card-name true
+ "More results have been included" false}
+ pulse.test-util/png-attachment
+ pulse.test-util/png-attachment]})
+ (mt/summarize-multipart-single-email email test-card-regex #"More results have been included"))))
:slack
(fn [{:keys [card-id]} [result]]
@@ -492,9 +425,8 @@
:breakout [!day.date]})
:assert
{:email
- (fn [_ _]
- (is (= {}
- @mt/inbox)))}}
+ (fn [_ emails]
+ (is (empty? emails)))}}
"too much data"
{:card
@@ -503,16 +435,14 @@
:assert
{:email
- (fn [_ _]
- (is (= (rasta-alert-email "Alert: Test card has results"
- [(merge test-card-result
- {"More results have been included" false
- "ID</th>" true})
- pulse.test-util/png-attachment])
- (mt/summarize-multipart-email test-card-regex
- #"More results have been included"
- #"ID</th>"))))}}
-
+ (fn [_ [email]]
+ (is (= (rasta-alert-message {:message [{pulse.test-util/card-name true
+ "More results have been included" false
+ "ID</th>" true}
+ pulse.test-util/png-attachment]})
+ (mt/summarize-multipart-single-email email test-card-regex
+ #"More results have been included"
+ #"ID</th>"))))}}
"with data and a CSV + XLS attachment"
{:card
@@ -525,17 +455,16 @@
:assert
{:email
- (fn [_ _]
- (is (= (rasta-alert-email "Alert: Test card has results"
- [test-card-result
- pulse.test-util/png-attachment
- pulse.test-util/png-attachment
- pulse.test-util/csv-attachment
- pulse.test-util/xls-attachment])
- (mt/summarize-multipart-email test-card-regex))))}})))
+ (fn [_ [email]]
+ (is (= (rasta-alert-message {:message [test-card-result
+ pulse.test-util/png-attachment
+ pulse.test-util/png-attachment
+ pulse.test-util/csv-attachment
+ pulse.test-util/xls-attachment]})
+ (mt/summarize-multipart-single-email email test-card-regex))))}})))
(deftest alert-first-run-only-test
- (tests {:pulse {:alert_condition "rows", :alert_first_only true}}
+ (tests! {:pulse {:alert_condition "rows", :alert_first_only true}}
"first run only with data"
{:card
(merge
@@ -545,13 +474,9 @@
:assert
{:email
- (fn [{:keys [pulse-id]} _]
- (is (= (rasta-alert-email "Alert: Test card has results"
- [;(assoc test-card-result "stop sending you alerts" true)
- test-card-result
- pulse.test-util/png-attachment
- pulse.test-util/png-attachment])
- (mt/summarize-multipart-email test-card-regex))) ;#"stop sending you alerts")))
+ (fn [{pulse-id :pulse-id} [email]]
+ (is (= (rasta-alert-message)
+ (mt/summarize-multipart-single-email email test-card-regex))) ;#"stop sending you alerts")))
(testing "Pulse should be deleted"
(is (= false
(t2/exists? Pulse :id pulse-id)))))}}
@@ -563,18 +488,17 @@
:assert
{:email
- (fn [{:keys [pulse-id]} _]
- (is (= {}
- @mt/inbox))
+ (fn [{:keys [pulse-id]} emails]
+ (is (empty? emails))
(testing "Pulse should still exist"
(is (= true
(t2/exists? Pulse :id pulse-id)))))}}))
(deftest above-goal-alert-test
(testing "above goal alert"
- (tests {:pulse {:alert_condition "goal"
- :alert_first_only false
- :alert_above_goal true}}
+ (tests! {:pulse {:alert_condition "goal"
+ :alert_first_only false
+ :alert_above_goal true}}
"with data"
{:card
(merge (pulse.test-util/checkins-query-card {:filter [:between $date "2014-04-01" "2014-06-01"]
@@ -587,10 +511,9 @@
:assert
{:email
- (fn [_ _]
- (is (= (rasta-alert-email "Alert: Test card has reached its goal"
- [test-card-result pulse.test-util/png-attachment pulse.test-util/png-attachment])
- (mt/summarize-multipart-email test-card-regex))))}}
+ (fn [_ [email]]
+ (is (= (rasta-alert-message {:subject "Alert: Test card has reached its goal"})
+ (mt/summarize-multipart-single-email email test-card-regex))))}}
"no data"
{:card
@@ -604,9 +527,8 @@
:assert
{:email
- (fn [_ _]
- (is (= {}
- @mt/inbox)))}}
+ (fn [_ emails]
+ (is (empty? emails)))}}
"with progress bar"
{:card
@@ -618,16 +540,15 @@
:assert
{:email
- (fn [_ _]
- (is (= (rasta-alert-email "Alert: Test card has reached its goal"
- [test-card-result pulse.test-util/png-attachment pulse.test-util/png-attachment])
- (mt/summarize-multipart-email test-card-regex))))}})))
+ (fn [_ [email]]
+ (is (= (rasta-alert-message {:subject "Alert: Test card has reached its goal"})
+ (mt/summarize-multipart-single-email email test-card-regex))))}})))
(deftest below-goal-alert-test
(testing "Below goal alert"
- (tests {:pulse {:alert_condition "goal"
- :alert_first_only false
- :alert_above_goal false}}
+ (tests! {:pulse {:alert_condition "goal"
+ :alert_first_only false
+ :alert_above_goal false}}
"with data"
{:card
(merge (pulse.test-util/checkins-query-card {:filter [:between $date "2014-02-12" "2014-02-17"]
@@ -640,10 +561,9 @@
:assert
{:email
- (fn [_ _]
- (is (= (rasta-alert-email "Alert: Test card has gone below its goal"
- [test-card-result pulse.test-util/png-attachment pulse.test-util/png-attachment])
- (mt/summarize-multipart-email test-card-regex))))}}
+ (fn [_ [email]]
+ (is (= (rasta-alert-message {:subject "Alert: Test card has gone below its goal"})
+ (mt/summarize-multipart-single-email email test-card-regex))))}}
"with no satisfying data"
{:card
@@ -657,9 +577,8 @@
:assert
{:email
- (fn [_ _]
- (is (= {}
- @mt/inbox)))}}
+ (fn [_ emails]
+ (is (empty? emails)))}}
"with progress bar"
{:card
@@ -672,12 +591,11 @@
:assert
{:email
- (fn [_ _]
- (is (= (rasta-alert-email "Alert: Test card has gone below its goal"
- [test-card-result pulse.test-util/png-attachment pulse.test-util/png-attachment])
- (mt/summarize-multipart-email test-card-regex))))}})))
+ (fn [_ [email]]
+ (is (= (rasta-alert-message {:subject "Alert: Test card has gone below its goal"})
+ (mt/summarize-multipart-single-email email test-card-regex))))}})))
-(deftest goal-met-test
+(deftest ^:parallel goal-met-test
(let [alert-above-pulse {:alert_above_goal true}
alert-below-pulse {:alert_above_goal false}
progress-result (fn [val] [{:card {:display :progress
@@ -728,36 +646,36 @@
(with-pulse-for-card [{pulse-id :id} {:card card-id, :pulse {:alert_condition "goal"
:alert_first_only false
:alert_above_goal true}}]
- (pulse.test-util/email-test-setup
- (metabase.pulse/send-pulse! (pulse/retrieve-notification pulse-id))
- (is (= (rasta-alert-email "Alert: Test card has reached its goal"
- [test-card-result pulse.test-util/png-attachment pulse.test-util/png-attachment])
- (mt/summarize-multipart-email test-card-regex))))))))
-
-(deftest dashboard-description-markdown-test
- (testing "Dashboard description renders markdown"
- (mt/with-temp [Card {card-id :id} {:name "Test card"
- :dataset_query {:database (mt/id)
- :type :native
- :native {:query "select * from checkins"}}
- :display :table}
- Dashboard {dashboard-id :id} {:description "# dashboard description"}
- DashboardCard {dashboard-card-id :id} {:dashboard_id dashboard-id
- :card_id card-id}
- Pulse {pulse-id :id} {:name "Pulse Name"
- :dashboard_id dashboard-id}
- PulseCard _ {:pulse_id pulse-id
- :card_id card-id
- :dashboard_card_id dashboard-card-id}
- PulseChannel {pc-id :id} {:pulse_id pulse-id}
- PulseChannelRecipient _ {:user_id (pulse.test-util/rasta-id)
- :pulse_channel_id pc-id}]
- (pulse.test-util/email-test-setup
- (metabase.pulse/send-pulse! (pulse/retrieve-notification pulse-id))
- (is (= (mt/email-to :rasta {:subject "Pulse Name"
- :body {"<h1>dashboard description</h1>" true}
- :bcc? true})
- (mt/regex-email-bodies #"<h1>dashboard description</h1>")))))))
+ (let [channel-messsages (pulse.test-util/with-captured-channel-send-messages!
+ (metabase.pulse/send-pulse! (pulse/retrieve-notification pulse-id)))]
+ (is (= (rasta-alert-message {:subject "Alert: Test card has reached its goal"})
+ (mt/summarize-multipart-single-email (-> channel-messsages :channel/email first) test-card-regex))))))))
+
+;; TODO should be in dashboard subscription test
+#_(deftest dashboard-description-markdown-test
+ (testing "Dashboard description renders markdown"
+ (mt/with-temp [Card {card-id :id} {:name "Test card"
+ :dataset_query {:database (mt/id)
+ :type :native
+ :native {:query "select * from checkins"}}
+ :display :table}
+ Dashboard {dashboard-id :id} {:description "# dashboard description"}
+ DashboardCard {dashboard-card-id :id} {:dashboard_id dashboard-id
+ :card_id card-id}
+ Pulse {pulse-id :id} {:name "Pulse Name"
+ :dashboard_id dashboard-id}
+ PulseCard _ {:pulse_id pulse-id
+ :card_id card-id
+ :dashboard_card_id dashboard-card-id}
+ PulseChannel {pc-id :id} {:pulse_id pulse-id}
+ PulseChannelRecipient _ {:user_id (pulse.test-util/rasta-id)
+ :pulse_channel_id pc-id}]
+ (pulse.test-util/email-test-setup
+ (metabase.pulse/send-pulse! (pulse/retrieve-notification pulse-id))
+ (is (= (mt/email-to :rasta {:subject "Pulse Name"
+ :body {"<h1>dashboard description</h1>" true}
+ :bcc? true})
+ (mt/regex-email-bodies #"<h1>dashboard description</h1>")))))))
(deftest nonuser-email-test
(testing "Both users and Nonusers get an email, with unsubscribe text for nonusers"
@@ -766,7 +684,8 @@
:type :native
:native {:query "select * from checkins"}}
:display :table}
- Pulse {pulse-id :id} {:name "Pulse Name"}
+ Pulse {pulse-id :id} {:name "Pulse Name"
+ :alert_condition "rows"}
PulseCard _ {:pulse_id pulse-id
:card_id card-id}
PulseChannel {pc-id :id} {:pulse_id pulse-id
@@ -778,48 +697,6 @@
(is (mt/received-email-body? :rasta #"Manage your subscriptions"))
(is (mt/received-email-body? "nonuser@metabase.com" #"Unsubscribe"))))))
-(deftest basic-slack-test-2
- (testing "Basic slack test, 2 cards, 1 recipient channel"
- (mt/with-temp [Card {card-id-1 :id} (pulse.test-util/checkins-query-card {:breakout [!day.date]})
- Card {card-id-2 :id} (-> {:breakout [:field (mt/id :checkins :date) {:temporal-unit :month}]}
- pulse.test-util/checkins-query-card
- (assoc :name "Test card 2"))
- Pulse {pulse-id :id} {:name "Pulse Name"
- :skip_if_empty false}
- PulseCard _ {:pulse_id pulse-id
- :card_id card-id-1
- :position 0}
- PulseCard _ {:pulse_id pulse-id
- :card_id card-id-2
- :position 1}
- PulseChannel _ {:pulse_id pulse-id
- :channel_type "slack"
- :details {:channel "#general"}}]
- (pulse.test-util/slack-test-setup
- (let [[slack-data] (metabase.pulse/send-pulse! (pulse/retrieve-pulse pulse-id))]
- (is (= {:channel-id "#general",
- :attachments
- [{:blocks
- [{:type "header", :text {:type "plain_text", :text "Pulse: Pulse Name", :emoji true}}
- {:type "section", :fields [{:type "mrkdwn", :text "Sent by Rasta Toucan"}]}]}
- {:title pulse.test-util/card-name,
- :rendered-info {:attachments false
- :content true}
- :title_link (str "https://metabase.com/testmb/question/" card-id-1),
- :attachment-name "image.png",
- :channel-id "FOO",
- :fallback pulse.test-util/card-name}
- {:title "Test card 2",
- :rendered-info {:attachments false
- :content true}
- :title_link (str "https://metabase.com/testmb/question/" card-id-2),
- :attachment-name "image.png",
- :channel-id "FOO",
- :fallback "Test card 2"}]}
- (pulse.test-util/thunk->boolean slack-data)))
- (testing "attachments"
- (is (true? (every? produces-bytes? (rest (:attachments slack-data)))))))))))
-
(deftest create-and-upload-slack-attachments!-test
(let [slack-uploader (fn [storage]
(fn [_bytes attachment-name _channel-id]
@@ -861,40 +738,6 @@
processed))
(is (= @titles ["a.png"]))))))
-(deftest multi-channel-test
- (testing "Test with a slack channel and an email"
- (t2.with-temp/with-temp [Card {card-id :id} (pulse.test-util/checkins-query-card {:breakout [!day.date]})]
- ;; create a Pulse with an email channel
- (with-pulse-for-card [{pulse-id :id} {:card card-id, :pulse {:skip_if_empty false}}]
- ;; add additional Slack channel
- (t2.with-temp/with-temp [PulseChannel _ {:pulse_id pulse-id
- :channel_type "slack"
- :details {:channel "#general"}}]
- (pulse.test-util/slack-test-setup
- (let [pulse-data (metabase.pulse/send-pulse! (pulse/retrieve-pulse pulse-id))
- slack-data (m/find-first map? pulse-data)
- email-data (first (m/find-first seq? pulse-data))]
- (is (= {:channel-id "#general"
- :attachments [{:blocks
- [{:type "header", :text {:type "plain_text", :text "Pulse: Pulse Name", :emoji true}}
- {:type "section", :fields [{:type "mrkdwn", :text "Sent by Rasta Toucan"}]}]}
- {:title pulse.test-util/card-name
- :title_link (str "https://metabase.com/testmb/question/" card-id)
- :rendered-info {:attachments false
- :content true}
- :attachment-name "image.png"
- :channel-id "FOO"
- :fallback pulse.test-util/card-name}]}
- (pulse.test-util/thunk->boolean slack-data)))
- (is (= [true]
- (map (comp some? :content :rendered-info) (rest (:attachments slack-data)))))
- (is (= {:subject "Pulse: Pulse Name", :recipients ["rasta@metabase.com"], :message-type :attachments}
- (select-keys email-data [:subject :recipients :message-type])))
- (is (= 2
- (count (:message email-data))))
- (is (email-body? (first (:message email-data))))
- (is (attachment? (second (:message email-data)))))))))))
-
(deftest pulse-permissions-test
(testing "Pulses should be sent with the Permissions of the user that created them."
(letfn [(send-pulse-created-by-user!* [user-kw]
@@ -926,105 +769,102 @@
:numberOfSuccessfulCallsWithoutRetryAttempt])))
(def ^:private fake-email-notification
- [{:subject "test-message"
- :recipients ["whoever@example.com"]
- :message-type :text
- :message "test message body"}])
+ {:subject "test-message"
+ :recipients ["whoever@example.com"]
+ :message-type :text
+ :message "test message body"})
+
+(defn ^:private test-retry-configuration
+ []
+ (assoc (#'retry/retry-configuration)
+ :initial-interval-millis 1))
(deftest email-notification-retry-test
(testing "send email succeeds w/o retry"
- (let [test-retry (retry/random-exponential-backoff-retry "test-retry" (#'retry/retry-configuration))]
+ (let [test-retry (retry/random-exponential-backoff-retry "test-retry" (test-retry-configuration))]
(with-redefs [email/send-email! mt/fake-inbox-email-fn
retry/decorate (rt/test-retry-decorate-fn test-retry)]
(mt/with-temporary-setting-values [email-smtp-host "fake_smtp_host"
email-smtp-port 587]
(mt/reset-inbox!)
- (#'metabase.pulse/send-notifications! [fake-email-notification])
+ (#'metabase.pulse/send-retrying! :channel/email fake-email-notification)
(is (= {:numberOfSuccessfulCallsWithoutRetryAttempt 1}
(get-positive-retry-metrics test-retry)))
(is (= 1 (count @mt/inbox)))))))
(testing "send email succeeds hiding SMTP host not set error"
- (let [test-retry (retry/random-exponential-backoff-retry "test-retry" (#'retry/retry-configuration))]
+ (let [test-retry (retry/random-exponential-backoff-retry "test-retry" (test-retry-configuration))]
(with-redefs [email/send-email! (fn [& _] (throw (ex-info "Bumm!" {:cause :smtp-host-not-set})))
retry/decorate (rt/test-retry-decorate-fn test-retry)]
(mt/with-temporary-setting-values [email-smtp-host "fake_smtp_host"
email-smtp-port 587]
(mt/reset-inbox!)
- (#'metabase.pulse/send-notifications! [fake-email-notification])
+ (#'metabase.pulse/send-retrying! :channel/email fake-email-notification)
(is (= {:numberOfSuccessfulCallsWithoutRetryAttempt 1}
(get-positive-retry-metrics test-retry)))
(is (= 0 (count @mt/inbox)))))))
(testing "send email fails b/c retry limit"
- (let [retry-config (assoc (#'retry/retry-configuration)
- :max-attempts 1
- :initial-interval-millis 1)
+ (let [retry-config (assoc (test-retry-configuration) :max-attempts 1)
test-retry (retry/random-exponential-backoff-retry "test-retry" retry-config)]
(with-redefs [email/send-email! (tu/works-after 1 mt/fake-inbox-email-fn)
retry/decorate (rt/test-retry-decorate-fn test-retry)]
(mt/with-temporary-setting-values [email-smtp-host "fake_smtp_host"
email-smtp-port 587]
(mt/reset-inbox!)
- (#'metabase.pulse/send-notifications! [fake-email-notification])
+ (#'metabase.pulse/send-retrying! :channel/email fake-email-notification)
(is (= {:numberOfFailedCallsWithRetryAttempt 1}
(get-positive-retry-metrics test-retry)))
(is (= 0 (count @mt/inbox)))))))
(testing "send email succeeds w/ retry"
- (let [retry-config (assoc (#'retry/retry-configuration)
- :max-attempts 2
- :initial-interval-millis 1)
+ (let [retry-config (assoc (test-retry-configuration) :max-attempts 2)
test-retry (retry/random-exponential-backoff-retry "test-retry" retry-config)]
(with-redefs [email/send-email! (tu/works-after 1 mt/fake-inbox-email-fn)
retry/decorate (rt/test-retry-decorate-fn test-retry)]
(mt/with-temporary-setting-values [email-smtp-host "fake_smtp_host"
email-smtp-port 587]
(mt/reset-inbox!)
- (#'metabase.pulse/send-notifications! [fake-email-notification])
+ (#'metabase.pulse/send-retrying! :channel/email fake-email-notification)
(is (= {:numberOfSuccessfulCallsWithRetryAttempt 1}
(get-positive-retry-metrics test-retry)))
(is (= 1 (count @mt/inbox))))))))
(def ^:private fake-slack-notification
- {:channel-id "test-channel"
+ {:channel-id "#test-channel"
:message "test message body"
:attachments []})
(deftest slack-notification-retry-test
(testing "post slack message succeeds w/o retry"
- (let [test-retry (retry/random-exponential-backoff-retry "test-retry" (#'retry/retry-configuration))]
+ (let [test-retry (retry/random-exponential-backoff-retry "test-retry" (test-retry-configuration))]
(with-redefs [slack/post-chat-message! (constantly nil)
retry/decorate (rt/test-retry-decorate-fn test-retry)]
- (#'metabase.pulse/send-notifications! [fake-slack-notification])
+ (#'metabase.pulse/send-retrying! :channel/slack fake-slack-notification)
(is (= {:numberOfSuccessfulCallsWithoutRetryAttempt 1}
(get-positive-retry-metrics test-retry))))))
(testing "post slack message succeeds hiding token error"
- (let [test-retry (retry/random-exponential-backoff-retry "test-retry" (#'retry/retry-configuration))]
+ (let [test-retry (retry/random-exponential-backoff-retry "test-retry" (test-retry-configuration))]
(with-redefs [slack/post-chat-message! (fn [& _]
(throw (ex-info "Invalid token"
{:errors {:slack-token "Invalid token"}})))
retry/decorate (rt/test-retry-decorate-fn test-retry)]
- (#'metabase.pulse/send-notifications! [fake-slack-notification])
+ (#'metabase.pulse/send-retrying! :channel/slack fake-slack-notification)
(is (= {:numberOfSuccessfulCallsWithoutRetryAttempt 1}
(get-positive-retry-metrics test-retry))))))
(testing "post slack message fails b/c retry limit"
- (let [retry-config (assoc (#'retry/retry-configuration)
- :max-attempts 1
- :initial-interval-millis 1)
- test-retry (retry/random-exponential-backoff-retry "test-retry" retry-config)]
- (with-redefs [slack/post-chat-message! (tu/works-after 1 (constantly nil))
- retry/decorate (rt/test-retry-decorate-fn test-retry)]
- (#'metabase.pulse/send-notifications! [fake-slack-notification])
- (is (= {:numberOfFailedCallsWithRetryAttempt 1}
- (get-positive-retry-metrics test-retry))))))
+ (let [retry-config (assoc (test-retry-configuration) :max-attempts 1)
+ test-retry (retry/random-exponential-backoff-retry "test-retry" retry-config)]
+ (with-redefs [slack/post-chat-message! (tu/works-after 1 (constantly nil))
+ retry/decorate (rt/test-retry-decorate-fn test-retry)]
+ (#'metabase.pulse/send-retrying! :channel/slack fake-slack-notification)
+ (is (= {:numberOfFailedCallsWithRetryAttempt 1}
+ (get-positive-retry-metrics test-retry))))))
(testing "post slack message succeeds with retry"
- (let [retry-config (assoc (#'retry/retry-configuration)
- :max-attempts 2
- :initial-interval-millis 1)
- test-retry (retry/random-exponential-backoff-retry "test-retry" retry-config)]
- (with-redefs [slack/post-chat-message! (tu/works-after 1 (constantly nil))
- retry/decorate (rt/test-retry-decorate-fn test-retry)]
- (#'metabase.pulse/send-notifications! [fake-slack-notification])
- (is (= {:numberOfSuccessfulCallsWithRetryAttempt 1}
- (get-positive-retry-metrics test-retry)))))))
+ (let [retry-config (assoc (test-retry-configuration) :max-attempts 2)
+ test-retry (retry/random-exponential-backoff-retry "test-retry" retry-config)]
+ (with-redefs [slack/post-chat-message! (tu/works-after 1 (constantly nil))
+ retry/decorate (rt/test-retry-decorate-fn test-retry)]
+ (#'metabase.pulse/send-retrying! :channel/slack fake-slack-notification)
+ (is (= {:numberOfSuccessfulCallsWithRetryAttempt 1}
+ (get-positive-retry-metrics test-retry)))))))
(deftest alerts-do-not-remove-user-metadata
(testing "Alerts that exist on a Model shouldn't remove metadata (#35091)."
@@ -1046,7 +886,7 @@
:dataset_query q
:type :model
:result_metadata result-metadata}
- Pulse {pulse-id :id :as p} {:name "Test Pulse"}
+ Pulse {pulse-id :id :as p} {:name "Test Pulse" :alert_condition "rows"}
PulseCard _ {:pulse_id pulse-id
:card_id card-id}
PulseChannel _ {:channel_type :email
diff --git a/test/metabase/test.clj b/test/metabase/test.clj
index c6c04a4b38b6ae34ee1cf583fa22d052efdb6e09..e5b04bfa473ab418f6737a4a77a95c8f76cabc40 100644
--- a/test/metabase/test.clj
+++ b/test/metabase/test.clj
@@ -139,6 +139,7 @@
regex-email-bodies
reset-inbox!
summarize-multipart-email
+ summarize-multipart-single-email
with-expected-messages
with-fake-inbox]