Skip to content
Snippets Groups Projects
Unverified Commit 0f06c5b6 authored by Jerry Huang's avatar Jerry Huang Committed by GitHub
Browse files

Switch pulse email sending to use bcc instead of sending a seperate email (#33604) 


* inital commit

* fix tests

* update test

* fix tests

* fix tests

* fix tests

* fix tests

* update test

* update tests

* update tests

* address changes

* fix tests

* fix test

* fix tess

---------

Co-authored-by: default avataradam-james <21064735+adam-james-v@users.noreply.github.com>
parent 8cb63dfa
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 100 additions and 74 deletions
enterprise/backend/test/metabase_enterprise/sandbox/pulse_test.clj
+ 1
− 1
View file @ 0f06c5b6
......@@ -54,7 +54,7 @@
(metabase.pulse/send-pulse! pulse)))
(let [results @mt/inbox]
(is (= {"rasta@metabase.com" [{:from "metamailman@metabase.com"
:to ["rasta@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]))))))
......
src/metabase/email.clj
+ 21
− 19
View file @ 0f06c5b6
......@@ -107,10 +107,11 @@
(def ^:private EmailMessage
(s/constrained
{:subject s/Str
:recipients [(s/pred u/email?)]
:message-type (s/enum :text :html :attachments)
:message (s/cond-pre s/Str [su/Map])} ; TODO - what should this be a sequence of?
{:subject s/Str
:recipients [(s/pred u/email?)]
:message-type (s/enum :text :html :attachments)
:message (s/cond-pre s/Str [su/Map]) ; TODO - what should this be a sequence of?
(s/optional-key :bcc?) (s/maybe s/Bool)}
(fn [{:keys [message-type message]}]
(if (= message-type :attachments)
(and (sequential? message) (every? map? message))
......@@ -122,25 +123,26 @@
"Send an email to one or more `recipients`. Upon success, this returns the `message` that was just sent. This function
does not catch and swallow thrown exceptions, it will bubble up."
{:style/indent 0}
[{:keys [subject recipients message-type message]} :- EmailMessage]
[{:keys [subject recipients message-type message], :as email} :- EmailMessage]
(try
(when-not (email-smtp-host)
(throw (ex-info (tru "SMTP host is not set.") {:cause :smtp-host-not-set})))
;; Now send the email
(send-email! (smtp-settings)
(merge
{:from (if-let [from-name (email-from-name)]
(str from-name " <" (email-from-address) ">")
(email-from-address))
:to recipients
:subject subject
:body (case message-type
:attachments message
:text message
:html [{:type "text/html; charset=utf-8"
:content message}])}
(when-let [reply-to (email-reply-to)]
{:reply-to reply-to})))
(let [to-type (if (:bcc? email) :bcc :to)]
(send-email! (smtp-settings)
(merge
{:from (if-let [from-name (email-from-name)]
(str from-name " <" (email-from-address) ">")
(email-from-address))
to-type recipients
:subject subject
:body (case message-type
:attachments message
:text message
:html [{:type "text/html; charset=utf-8"
:content message}])}
(when-let [reply-to (email-reply-to)]
{:reply-to reply-to}))))
(catch Throwable e
(prometheus/inc :metabase-email/message-errors)
(throw e))
......
src/metabase/email/messages.clj
+ 13
− 7
View file @ 0f06c5b6
......@@ -594,12 +594,18 @@
(defn- alert-context
"Context that is applicable only to the actual alert template (not alert management templates)"
[alert channel]
[alert channel non-user-email]
(let [{card-id :id, card-name :name} (first-card alert)]
{:title card-name
:titleUrl (urls/card-url card-id)
:alertSchedule (alert-schedule-text channel)
:creator (-> alert :creator :common_name)}))
{:title card-name
:titleUrl (urls/card-url card-id)
:alertSchedule (alert-schedule-text channel)
:notificationManagementUrl (if (nil? non-user-email)
(urls/notification-management-url)
(str (urls/unsubscribe-url)
"?hash=" (generate-pulse-unsubscribe-hash (:id alert) non-user-email)
"&email=" non-user-email
"&pulse-id=" (:id alert)))
:creator (-> alert :creator :common_name)}))
(defn- alert-results-condition-text [goal-value]
{:meets (format "This question has reached its goal of %s." goal-value)
......@@ -607,10 +613,10 @@
(defn render-alert-email
"Take a pulse object and list of results, returns an array of attachment objects for an email"
[timezone {:keys [alert_first_only] :as alert} channel results goal-value]
[timezone {:keys [alert_first_only] :as alert} channel results goal-value non-user-email]
(let [message-ctx (merge
(common-alert-context alert (alert-results-condition-text goal-value))
(alert-context alert channel))]
(alert-context alert channel non-user-email))]
(render-message-body alert
:alert
(assoc message-ctx :firstRunOnly? alert_first_only)
......
src/metabase/pulse.clj
+ 16
− 11
View file @ 0f06c5b6
......@@ -411,9 +411,9 @@
(fn [pulse _ {:keys [channel_type]}]
[(alert-or-pulse pulse) (keyword channel_type)]))
(defn- construct-pulse-email [subject recipient message]
(defn- construct-pulse-email [subject recipients message]
{:subject subject
:recipients [recipient]
:recipients recipients
:message-type :attachments
:message message})
......@@ -427,11 +427,13 @@
(nil? (:id recipient)))) recipients)
timezone (->> parts (some :card) defaulted-timezone)
dashboard (update (t2/select-one Dashboard :id dashboard-id) :description markdown/process-markdown :html)
email-to-users (for [user (map :email user-recipients)]
(construct-pulse-email (subject pulse) user (messages/render-pulse-email timezone pulse dashboard parts nil)))
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)))]
(concat email-to-users email-to-nonusers)))
(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}
......@@ -459,11 +461,13 @@
(nil? (:id recipient)))) (:recipients channel))
first-part (some :card parts)
timezone (defaulted-timezone first-part)
email-to-users (for [user (map :email user-recipients)]
(construct-pulse-email email-subject user (messages/render-alert-email timezone pulse channel parts (ui-logic/find-goal-value 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))))]
(concat email-to-users email-to-nonusers)))
(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}]
......@@ -531,7 +535,8 @@
(email/send-message-or-throw! {:subject subject
:recipients recipients
:message-type message-type
:message message})
:message message
:bcc? true})
(catch ExceptionInfo e
(when (not= :smtp-host-not-set (:cause (ex-data e)))
(throw e))))))
......
test/metabase/api/pulse_test.clj
+ 6
− 3
View file @ 0f06c5b6
......@@ -910,7 +910,8 @@
:recipients [(mt/fetch-user :rasta)]}]
:skip_if_empty false})))
(is (= (mt/email-to :rasta {:subject "Pulse: Daily Sad Toucans"
:body {"Daily Sad Toucans" true}})
:body {"Daily Sad Toucans" true}
:bcc? true})
(mt/regex-email-bodies #"Daily Sad Toucans"))))))))))
(deftest send-test-pulse-validate-emails-test
......@@ -979,7 +980,8 @@
:recipients [(mt/fetch-user :rasta)]}]
:skip_if_empty false})))
(is (= (mt/email-to :rasta {:subject "Daily Sad Toucans"
:body {"Daily Sad Toucans" true}})
: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
......@@ -1020,7 +1022,8 @@
(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}})
:body {"A Pulse" true}
:bcc? true})
(mt/regex-email-bodies #"A Pulse"))))))))))
(deftest pulse-card-query-results-test
......
test/metabase/dashboard_subscription_test.clj
+ 5
− 4
View file @ 0f06c5b6
......@@ -140,10 +140,11 @@
(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]}
email)))
(mt/email-to :rasta (merge {:subject "Aviary KPIs"
:body [{"Aviary KPIs" true}
pulse.test-util/png-attachment]
:bcc? true}
email)))
(defn do-with-dashboard-fixture-for-dashboard
"Impl for [[with-link-card-fixture-for-dashboard]]."
......
test/metabase/email_test.clj
+ 23
− 19
View file @ 0f06c5b6
......@@ -30,7 +30,7 @@
"A function that can be used in place of `send-email!`.
Put all messages into `inbox` instead of actually sending them."
[_ email]
(doseq [recipient (:to email)]
(doseq [recipient (concat (:to email) (:bcc email))]
(swap! inbox assoc recipient (-> (get @inbox recipient [])
(conj email)))))
......@@ -106,9 +106,10 @@
(for [{:keys [body] :as email} emails-for-recipient
:let [matches (-> body first email-body->regex-boolean)]
:when (some true? (vals matches))]
(-> email
(update :to set)
(assoc :body matches)))))
(cond-> email
(:to email) (update :to set)
(:bcc email) (update :bcc set)
true (assoc :body matches)))))
(m/filter-vals seq))))
(defn regex-email-bodies
......@@ -181,25 +182,28 @@
(let [email-body->regex-boolean (create-email-body->regex-fn regexes)]
(m/map-vals (fn [emails-for-recipient]
(for [email emails-for-recipient]
(-> email
(update :to set)
(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)))))))))
(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)))
(defn email-to
"Creates a default email map for `user-kwd` via `test.users/fetch-user`, as would be returned by `with-fake-inbox`"
[user-kwd & [email-map]]
(let [{:keys [email]} (test.users/fetch-user user-kwd)]
{email [(merge {:from (if-let [from-name (email/email-from-name)]
(str from-name " <" (email/email-from-address) ">")
(email/email-from-address))
:to #{email}}
email-map)]}))
([user-kwd & [email-map]]
(let [{:keys [email]} (test.users/fetch-user user-kwd)
to-type (if (:bcc? email-map) :bcc :to)
email-map (dissoc email-map :bcc?)]
{email [(merge {:from (if-let [from-name (email/email-from-name)]
(str from-name " <" (email/email-from-address) ">")
(email/email-from-address))
to-type #{email}}
email-map)]})))
(defn temp-csv
[file-basename content]
......
test/metabase/pulse_test.clj
+ 13
− 9
View file @ 0f06c5b6
......@@ -33,16 +33,18 @@
;;; +----------------------------------------------------------------------------------------------------------------+
(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]}
(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}))
:body email-body
:bcc? true}))
(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
......@@ -381,11 +383,12 @@
{:email
(fn [_ _]
(is (= (into {} (map (fn [user-kwd]
(mt/email-to user-kwd {:subject "Pulse: Pulse Name",
:to #{(:email (mt/fetch-user 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]}))
pulse.test-util/png-attachment]
:bcc? true}))
[:rasta :crowberto]))
(mt/summarize-multipart-email #"Pulse Name"))))}})))
......@@ -700,7 +703,8 @@
(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}})
:body {"<h1>dashboard description</h1>" true}
:bcc? true})
(mt/regex-email-bodies #"<h1>dashboard description</h1>")))))))
(deftest nonuser-email-test
......
test/metabase/task/send_pulses_test.clj
+ 2
− 1
View file @ 0f06c5b6
......@@ -36,7 +36,8 @@
(testing "emails"
(is (= (et/email-to :rasta
{:subject "Alert: My Question Name has results",
:body {"My Question Name" true}})
:body {"My Question Name" true}
:bcc? true})
(et/regex-email-bodies #"My Question Name"))))
(testing "exceptions"
(is (= []
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment