Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metabase channel #43924

Merged
merged 29 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
538d488
adding the channel
qnkhuat Jun 10, 2024
3305b8e
pulse with new slack channel
qnkhuat Jun 10, 2024
9b9ce20
pulse with email channels
qnkhuat Jun 10, 2024
d1cd4c2
pulse now use all channels api
qnkhuat Jun 10, 2024
ba0614b
tidy up and support retry
qnkhuat Jun 11, 2024
de5689e
fixing tests + retry sending
qnkhuat Jun 11, 2024
02ab272
pulse tests passed
qnkhuat Jun 11, 2024
17f8b34
fix test failed to compile
qnkhuat Jun 11, 2024
36f750f
test fixing...
qnkhuat Jun 11, 2024
8ed0d00
handle channel correctly
qnkhuat Jun 11, 2024
1c805de
remove debug code
qnkhuat Jun 11, 2024
2c82e37
still fixing tests
qnkhuat Jun 11, 2024
03de60a
test fixes :crossed-finger
qnkhuat Jun 12, 2024
579a475
test fixes :crossed-finger 2
qnkhuat Jun 12, 2024
578171e
ci should be green now
qnkhuat Jun 12, 2024
14df2a5
Tidy
qnkhuat Jun 12, 2024
09598d1
remove pulse test that no longer needed
qnkhuat Jun 12, 2024
607b81e
remove pulse test that no longer needed
qnkhuat Jun 12, 2024
7138fc6
add attachments test for dashboard sub
qnkhuat Jun 12, 2024
be29d5d
no more doall with for
qnkhuat Jun 12, 2024
56998cb
Merge branch 'master' into channel
qnkhuat Jun 12, 2024
fcba1b8
Update test/metabase/pulse_test.clj
qnkhuat Jun 13, 2024
6f67e2e
Merge branch 'master' into channel
qnkhuat Jun 18, 2024
4f3b26f
Add an helper and make sure we don't send to archived channels
qnkhuat Jun 13, 2024
354b6c1
final tidy
qnkhuat Jun 19, 2024
2342e92
no inline def for tests
qnkhuat Jun 19, 2024
caf79fa
helper docstring
qnkhuat Jun 19, 2024
25b55cf
test fixes
qnkhuat Jun 19, 2024
a2e03a9
retrieve the disabled pulses too
qnkhuat Jun 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .clj-kondo/config.edn
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
23 changes: 13 additions & 10 deletions dev/src/dev/render_png.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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
Expand Down Expand Up @@ -81,16 +80,20 @@
nil
query-results)))

(defn open-hiccup-as-html
(defn open-html
"Take a hiccup data structure, render it as html, then open it in the browser."
qnkhuat marked this conversation as resolved.
Show resolved Hide resolved
[hiccup]
(let [html-str (hiccup/html hiccup)
tmp-file (File/createTempFile "card-html" ".html")]
[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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,35 +8,37 @@

(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"]}}
{:recipients [{:email "test@metabase.com"}]
: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"]
(let [{:keys [response recipients]} (send! 200)]
(def inbox @mt/inbox)
qnkhuat marked this conversation as resolved.
Show resolved Hide resolved
(is (= {:ok true}
response))
(is (contains? recipients "test@metabase.com"))))
Expand Down
108 changes: 40 additions & 68 deletions enterprise/backend/test/metabase_enterprise/sandbox/pulse_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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]]
Expand All @@ -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)
Expand All @@ -36,71 +34,41 @@
(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)
: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 [_ _ _ [{: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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to metabase.channel.email_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
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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"
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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"
Expand Down Expand Up @@ -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"}}
Expand All @@ -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
Expand All @@ -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))))))))
Loading
Loading