Skip to content
Snippets Groups Projects
Unverified Commit ce0e6760 authored by Tim Macdonald's avatar Tim Macdonald Committed by GitHub
Browse files

Add link to dashboard in emailed dash subs (#14500)


* Add link to dashboard in emailed dash subs

[Fixes #14211]

* styling for dash subscription title link

* Add end-to-end dashboard sub testing

Test for dashboard link inclusion in email

Co-authored-by: default avatarMaz Ameli <maz@metabase.com>
parent df269409
Branches
Tags
No related merge requests found
<html>
<head>
<style>
.pulse .container .title-link {
color: #616D75;
text-decoration: none;
}
.pulse .container .title-link:hover {
color: #509EE3;
}
@media screen and (min-width:600px) {
.pulse .container {
padding: 2em 4em !important;
......
......@@ -250,6 +250,12 @@
:content-type "image/png"
:content url})
(defn- pulse-link-context
[{:keys [cards dashboard_id]}]
(when-let [dashboard-id (or dashboard_id
(some :dashboard_id cards))]
{:pulseLink (url/dashboard-url dashboard-id)}))
(defn- pulse-context [pulse]
(merge (common-context)
{:emailType "pulse"
......@@ -257,6 +263,7 @@
:sectionStyle (render.style/style (render.style/section-style))
:colorGrey4 render.style/color-gray-4
:logoFooter true}
(pulse-link-context pulse)
(random-quote-context)))
(defn- create-temp-file
......@@ -357,7 +364,6 @@
(defn- render-message-body [message-template message-context timezone results]
(let [rendered-cards (binding [render/*include-title* true]
;; doall to ensure we haven't exited the binding before the valures are created
(mapv #(render/render-pulse-section timezone %) results))
message-body (assoc message-context :pulse (html (vec (cons :div (map :content rendered-cards)))))
attachments (apply merge (map :attachments rendered-cards))]
......
{{> metabase/email/_header}}
{{#pulseName}}
<h1 style="{{sectionStyle}} margin: 16px; color: {{color-brand}};">
{{pulseName}}
</h1>
{{#pulseLink}}
<a href="{{pulseLink}}" class="title-link">
<h1 style="{{sectionStyle}} margin: 24px 0 24px 0;">
{{pulseName}}
</h1>
</a>
{{/pulseLink}}
{{^pulseLink}}
<h1 style="{{sectionStyle}} margin: 16px; color: {{color-brand}};">
{{pulseName}}
</h1>
{{/pulseLink}}
{{/pulseName}}
{{{pulse}}}
{{> metabase/email/_footer_pulse}}
......@@ -168,8 +168,9 @@
:pulse))
(defn- subject
[{:keys [name cards]}]
(if (some :dashboard_id cards)
[{:keys [name cards dashboard_id]}]
(if (or dashboard_id
(some :dashboard_id cards))
name
(trs "Pulse: {0}" name)))
......@@ -271,7 +272,7 @@
;; send the cards instead
(for [card cards
;; Pulse ID may be `nil` if the Pulse isn't saved yet
:let [result (execute-card pulse (u/get-id card), :pulse-id pulse-id)]
:let [result (execute-card pulse (u/the-id card), :pulse-id pulse-id)]
;; some cards may return empty results, e.g. if the card has been archived
:when result]
result))))
......
(ns metabase.dashboard-subscription-test
(:require [clojure.test :refer :all]
[clojure.walk :as walk]
[metabase.integrations.slack :as slack]
[metabase.models :refer [Card Dashboard DashboardCard Pulse PulseCard PulseChannel PulseChannelRecipient User]]
[metabase.models.pulse :as models.pulse]
[metabase.pulse :as pulse]
[metabase.pulse.render.body :as render.body]
[metabase.pulse.test-util :refer :all]
[metabase.test :as mt]
[metabase.util :as u]
[schema.core :as s]
[toucan.db :as db]))
(defn- do-with-dashboard-sub-for-card
"Creates a Pulse, Dashboard, and other relevant rows for a `card` (using `pulse` and `pulse-card` properties if
specified), then invokes
(f pulse)"
[{:keys [dashboard pulse pulse-card channel card]
:or {channel :email}}
f]
(mt/with-temp* [Pulse [{pulse-id :id, :as pulse}
(-> pulse
(merge {:name "Aviary KPIs"
:dashboard_id (u/the-id dashboard)}))]
PulseCard [_ (merge {:pulse_id pulse-id
:card_id (u/the-id card)
:position 0}
pulse-card)]
DashboardCard [{dashcard-id :id} {:dashboard_id (u/the-id dashboard)
:card_id (u/the-id card)}]
PulseChannel [{pc-id :id} (case channel
:email
{:pulse_id pulse-id}
:slack
{:pulse_id pulse-id
:channel_type "slack"
:details {:channel "#general"}})]]
(if (= channel :email)
(mt/with-temp PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]
(f pulse))
(f pulse))))
(defmacro ^:private with-dashboard-sub-for-card
"e.g.
(with-dashboard-sub-for-card [pulse {:card my-card, :pulse pulse-properties, ...}]
...)"
[[pulse-binding properties] & body]
`(do-with-dashboard-sub-for-card ~properties (fn [~pulse-binding] ~@body)))
(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
Slack version of that Pulse respectively. `:assert` functions have the signature
(f object-ids send-pulse!-response)
Example:
(do-test
{:card {:dataset_query (mt/mbql-query checkins)}
:assert {:slack (fn [{:keys [pulse-id]} response]
(is (= {:sent pulse-id}
response)))}})"
[{:keys [card pulse pulse-card fixture], assertions :assert}]
{:pre [(map? assertions) ((some-fn :email :slack) assertions)]}
(doseq [channel-type [:email :slack]
:let [f (get assertions channel-type)]
:when f]
(assert (fn? f))
(testing (format "sent to %s channel" channel-type)
(mt/with-temp* [Dashboard [{dashboard-id :id} {:name "Aviary KPIs"}]
Card [{card-id :id} (merge {:name card-name} card)]]
(with-dashboard-sub-for-card [{pulse-id :id}
{:card card-id
:dashboard dashboard-id
:pulse pulse
:pulse-card pulse-card
:channel channel-type}]
(letfn [(thunk* []
(f {:card-id card-id, :pulse-id pulse-id}
(pulse/send-pulse! (models.pulse/retrieve-notification pulse-id))))
(thunk []
(if fixture
(fixture {:card-id card-id, :pulse-id pulse-id} thunk*)
(thunk*)))]
(case channel-type
:email (email-test-setup (thunk))
:slack (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`:
(tests
;; shared properties used for both tests
{:card {:dataset_query (mt/mbql-query)}}
\"Test 1\"
{:assert {:email (fn [_ _] (is ...))}}
\"Test 2\"
;; override just the :display property of the Card
{:card {:display \"table\"}
:assert {:email (fn [_ _] (is ...))}})"
{:style/indent 1}
[common & {:as message->m}]
(doseq [[message m] message->m]
(testing message
(do-test (merge-with merge common m)))))
(defn- rasta-pulse-email [& [email]]
(mt/email-to :rasta (merge {:subject "Aviary KPIs",
:body [{"Aviary KPIs" true}
png-attachment]}
email)))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Tests |
;;; +----------------------------------------------------------------------------------------------------------------+
(deftest execute-dashboard-test
(testing "it runs for each card"
(mt/with-temp* [Card [{card-id-1 :id}]
Card [{card-id-2 :id}]
Dashboard [{dashboard-id :id} {:name "Birdfeed Usage"}]
DashboardCard [dashcard-1 {:dashboard_id dashboard-id :card_id card-id-1}]
DashboardCard [dashcard-2 {:dashboard_id dashboard-id :card_id card-id-2}]
User [{user-id :id}]]
(let [result (pulse/execute-dashboard {:creator_id user-id} dashboard-id)]
(is (= (count result) 2))
(is (schema= [{:card (s/pred map?)
:result (s/pred map?)}]
result))))))
(deftest basic-table-test
(tests {:pulse {:skip_if_empty false}}
"19 results, so no attachment"
{:card (checkins-query-card {:aggregation nil, :limit 19})
:fixture
(fn [_ thunk]
(with-redefs [render.body/attached-results-text (wrap-function @#'render.body/attached-results-text)]
(thunk)))
:assert
{:email
(fn [_ _]
(is (= (rasta-pulse-email {:body [{;; No "Pulse:" prefix
"Aviary KPIs" true
;; Includes everything
"More results have been included" false
;; Inline table
"ID</th>" true
;; Links to source dashboard
"<a href=\\\"https://metabase.com/testmb/dashboard/\\d+\\\" class=\\\"title-link\\\">" true}]})
(mt/summarize-multipart-email
#"Aviary KPIs"
#"More results have been included"
#"ID</th>"
#"<a href=\"https://metabase.com/testmb/dashboard/\d+\" class=\"title-link\">"))))
:slack
(fn [{:keys [card-id]} [pulse-results]]
;; If we don't force the thunk, the rendering code will never execute and attached-results-text won't be
;; called
(force-bytes-thunk pulse-results)
(testing "\"more results in attachment\" text should not be present for Slack Pulses"
(testing "Pulse results"
(is (= {:channel-id "#general"
:message "Aviary KPIs"
:attachments
[{:title card-name
:attachment-bytes-thunk true
:title_link (str "https://metabase.com/testmb/question/" card-id)
:attachment-name "image.png"
:channel-id "FOO"
:fallback card-name}]}
(thunk->boolean pulse-results))))
(testing "attached-results-text should be invoked exactly once"
(is (= 1
(count (input @#'render.body/attached-results-text)))))
(testing "attached-results-text should return nil since it's a slack message"
(is (= [nil]
(output @#'render.body/attached-results-text))))))}}))
(ns metabase.pulse.test-util
(:require [metabase.models.pulse :as models.pulse :refer [Pulse]]
(:require [clojure.walk :as walk]
[metabase.models.pulse :as models.pulse :refer [Pulse]]
[metabase.models.pulse-card :refer [PulseCard]]
[metabase.pulse :as pulse]
[metabase.query-processor-test :as qp.test]
[metabase.test :as mt]
[metabase.test.data.users :as users]
[metabase.util :as u]
[toucan.util.test :as tt]))
......@@ -17,3 +19,126 @@
(vec results))]
(let [[{:keys [result]}] (pulse/send-pulse! pulse)]
(qp.test/rows result)))))
(def card-name "Test card")
(defn checkins-query-card*
"Basic query that will return results for an alert"
[query-map]
{:name card-name
:dataset_query {:database (mt/id)
:type :query
:query (merge {:source-table (mt/id :checkins)
:aggregation [["count"]]}
query-map)}})
(defmacro checkins-query-card [query]
`(checkins-query-card* (mt/$ids ~'checkins ~query)))
(defn venues-query-card [aggregation-op]
{:name card-name
:dataset_query {:database (mt/id)
:type :query
:query {:source-table (mt/id :venues)
:aggregation [[aggregation-op (mt/id :venues :price)]]}}})
(defn rasta-id []
(mt/user->id :rasta))
(defn realize-lazy-seqs
"It's possible when data structures contain lazy sequences that the database will be torn down before the lazy seq
is realized, causing the data returned to be nil. This function walks the datastructure, realizing all the lazy
sequences it finds"
[data]
(walk/postwalk identity data))
(defn do-with-site-url
[f]
(mt/with-temporary-setting-values [site-url "https://metabase.com/testmb"]
(f)))
(defmacro email-test-setup
"Macro that ensures test-data is present and will use a fake inbox for emails"
[& body]
`(mt/with-fake-inbox
(do-with-site-url (fn [] ~@body))))
(defmacro slack-test-setup
"Macro that ensures test-data is present and disables sending of all notifications"
[& body]
`(with-redefs [metabase.pulse/send-notifications! realize-lazy-seqs
slack/files-channel (constantly {:name "metabase_files"
:id "FOO"})]
(do-with-site-url (fn [] ~@body))))
(def png-attachment
{:type :inline
:content-id true
:content-type "image/png"
:content java.net.URL})
(def csv-attachment
{:type :attachment
:content-type "text/csv"
:file-name "Test card.csv",
:content java.net.URL
:description "More results for 'Test card'"
:content-id false})
(def xls-attachment
{:type :attachment
:file-name "Test card.xlsx"
:content-type "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"
:content java.net.URL
:description "More results for 'Test card'"
:content-id false})
(defprotocol WrappedFunction
(input [_])
(output [_]))
(defn- invoke-with-wrapping
"Apply `args` to `func`, capturing the arguments of the invocation and the result of the invocation. Store the
arguments in `input-atom` and the result in `output-atom`."
[input-atom output-atom func args]
(swap! input-atom conj args)
(let [result (apply func args)]
(swap! output-atom conj result)
result))
(defn wrap-function
"Return a function that wraps `func`, not interfering with it but recording it's input and output, which is
available via the `input` function and `output`function that can be used directly on this object"
[func]
(let [input (atom nil)
output (atom nil)]
(reify WrappedFunction
(input [_] @input)
(output [_] @output)
clojure.lang.IFn
(invoke [_ x1]
(invoke-with-wrapping input output func [x1]))
(invoke [_ x1 x2]
(invoke-with-wrapping input output func [x1 x2]))
(invoke [_ x1 x2 x3]
(invoke-with-wrapping input output func [x1 x2 x3]))
(invoke [_ x1 x2 x3 x4]
(invoke-with-wrapping input output func [x1 x2 x3 x4]))
(invoke [_ x1 x2 x3 x4 x5]
(invoke-with-wrapping input output func [x1 x2 x3 x4 x5]))
(invoke [_ x1 x2 x3 x4 x5 x6]
(invoke-with-wrapping input output func [x1 x2 x3 x4 x5 x6])))))
(defn force-bytes-thunk
"Grabs the thunk that produces the image byte array and invokes it"
[results]
((-> results
:attachments
first
:attachment-bytes-thunk)))
(defn thunk->boolean [{:keys [attachments] :as result}]
(assoc result :attachments (for [attachment-info attachments]
(update attachment-info :attachment-bytes-thunk fn?))))
......@@ -5,13 +5,13 @@
[clojure.walk :as walk]
[medley.core :as m]
[metabase.integrations.slack :as slack]
[metabase.models :refer [Card Collection Dashboard DashboardCard Pulse PulseCard PulseChannel PulseChannelRecipient User]]
[metabase.models :refer [Card Collection Dashboard Pulse PulseCard PulseChannel PulseChannelRecipient User]]
[metabase.models.permissions :as perms]
[metabase.models.permissions-group :as group]
[metabase.models.pulse :as models.pulse]
[metabase.pulse :as pulse]
[metabase.pulse.render.body :as render.body]
[metabase.pulse.test-util :as pulse.tu]
[metabase.pulse.test-util :refer :all]
[metabase.query-processor.middleware.constraints :as constraints]
[metabase.test :as mt]
[metabase.util :as u]
......@@ -22,59 +22,19 @@
;;; | Util Fns & Macros |
;;; +----------------------------------------------------------------------------------------------------------------+
(def ^:private card-name "Test card")
(defn checkins-query-card*
"Basic query that will return results for an alert"
[query-map]
{:name card-name
:dataset_query {:database (mt/id)
:type :query
:query (merge {:source-table (mt/id :checkins)
:aggregation [["count"]]}
query-map)}})
(defmacro checkins-query-card [query]
`(checkins-query-card* (mt/$ids ~'checkins ~query)))
(defn- venues-query-card [aggregation-op]
{:name card-name
:dataset_query {:database (mt/id)
:type :query
:query {:source-table (mt/id :venues)
:aggregation [[aggregation-op (mt/id :venues :price)]]}}})
(defn- rasta-id []
(mt/user->id :rasta))
(defn- realize-lazy-seqs
"It's possible when data structures contain lazy sequences that the database will be torn down before the lazy seq
is realized, causing the data returned to be nil. This function walks the datastructure, realizing all the lazy
sequences it finds"
[data]
(walk/postwalk identity data))
(defn- do-with-site-url
[f]
(mt/with-temporary-setting-values [site-url "https://metabase.com/testmb"]
(f)))
(defmacro ^:private slack-test-setup
"Macro that ensures test-data is present and disables sending of all notifications"
[& body]
`(with-redefs [metabase.pulse/send-notifications! realize-lazy-seqs
slack/files-channel (constantly {:name "metabase_files"
:id "FOO"})]
(do-with-site-url (fn [] ~@body))))
(defmacro ^:private email-test-setup
"Macro that ensures test-data is present and will use a fake inbox for emails"
[& body]
`(mt/with-fake-inbox
(do-with-site-url (fn [] ~@body))))
(defn- rasta-pulse-email [& [email]]
(mt/email-to :rasta (merge {:subject "Pulse: Pulse Name",
:body [{"Pulse Name" true}
png-attachment]}
email)))
(defn- rasta-alert-email
[subject email-body]
(mt/email-to :rasta {:subject subject
:body email-body}))
(defn- do-with-pulse-for-card
"Creates a Pulse and other relevant rows for a `card` (using `pulse` and `pulse-card` properties if specfied), then
"Creates a Pulse and other relevant rows for a `card` (using `pulse` and `pulse-card` properties if specified), then
invokes
(f pulse)"
......@@ -85,7 +45,7 @@
(-> pulse
(merge {:name "Pulse Name"}))]
PulseCard [_ (merge {:pulse_id pulse-id
:card_id (u/get-id card)
:card_id (u/the-id card)
:position 0}
pulse-card)]
PulseChannel [{pc-id :id} (case channel
......@@ -171,89 +131,9 @@
(testing message
(do-test (merge-with merge common m)))))
(defn- force-bytes-thunk
"Grabs the thunk that produces the image byte array and invokes it"
[results]
((-> results
:attachments
first
:attachment-bytes-thunk)))
(def ^:private png-attachment
{:type :inline
:content-id true
:content-type "image/png"
:content java.net.URL})
(defn- rasta-pulse-email [& [email]]
(mt/email-to :rasta (merge {:subject "Pulse: Pulse Name",
:body [{"Pulse Name" true}
png-attachment]}
email)))
(def ^:private csv-attachment
{:type :attachment
:content-type "text/csv"
:file-name "Test card.csv",
:content java.net.URL
:description "More results for 'Test card'"
:content-id false})
(def ^:private xls-attachment
{:type :attachment
:file-name "Test card.xlsx"
:content-type "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"
:content java.net.URL
:description "More results for 'Test card'"
:content-id false})
(defn- rasta-alert-email
[subject email-body]
(mt/email-to :rasta {:subject subject
:body email-body}))
(def ^:private test-card-result {card-name true})
(def ^:private test-card-regex (re-pattern card-name))
(defn- thunk->boolean [{:keys [attachments] :as result}]
(assoc result :attachments (for [attachment-info attachments]
(update attachment-info :attachment-bytes-thunk fn?))))
(defprotocol ^:private WrappedFunction
(^:private input [_])
(^:private output [_]))
(defn- invoke-with-wrapping
"Apply `args` to `func`, capturing the arguments of the invocation and the result of the invocation. Store the
arguments in `input-atom` and the result in `output-atom`."
[input-atom output-atom func args]
(swap! input-atom conj args)
(let [result (apply func args)]
(swap! output-atom conj result)
result))
(defn- wrap-function
"Return a function that wraps `func`, not interfering with it but recording it's input and output, which is
available via the `input` function and `output`function that can be used directly on this object"
[func]
(let [input (atom nil)
output (atom nil)]
(reify WrappedFunction
(input [_] @input)
(output [_] @output)
clojure.lang.IFn
(invoke [_ x1]
(invoke-with-wrapping input output func [x1]))
(invoke [_ x1 x2]
(invoke-with-wrapping input output func [x1 x2]))
(invoke [_ x1 x2 x3]
(invoke-with-wrapping input output func [x1 x2 x3]))
(invoke [_ x1 x2 x3 x4]
(invoke-with-wrapping input output func [x1 x2 x3 x4]))
(invoke [_ x1 x2 x3 x4 x5]
(invoke-with-wrapping input output func [x1 x2 x3 x4 x5]))
(invoke [_ x1 x2 x3 x4 x5 x6]
(invoke-with-wrapping input output func [x1 x2 x3 x4 x5 x6])))))
(defn- produces-bytes? [{:keys [attachment-bytes-thunk]}]
(pos? (alength ^bytes (attachment-bytes-thunk))))
......@@ -315,12 +195,15 @@
:assert
{:email
(fn [_ _]
(is (= (rasta-pulse-email {:body [{"Pulse Name" true
"More results have been included" false
"ID</th>" true}]})
(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}]})
(mt/summarize-multipart-email
#"Pulse Name"
#"More results have been included" #"ID</th>"))))
#"More results have been included"
#"ID</th>"
#"<a href=\"https://metabase.com/testmb/dashboard/"))))
:slack
(fn [{:keys [card-id]} [pulse-results]]
......@@ -807,35 +690,21 @@
:result (s/pred map?)}
(pulse/execute-card {:creator_id (mt/user->id :rasta)} card))))))
(deftest execute-dashboard-test
(testing "it runs for each card"
(mt/with-temp* [Card [{card-id-1 :id}]
Card [{card-id-2 :id}]
Dashboard [{dashboard-id :id} {:name "Birdfeed Usage"}]
DashboardCard [dashcard-1 {:dashboard_id dashboard-id :card_id card-id-1}]
DashboardCard [dashcard-2 {:dashboard_id dashboard-id :card_id card-id-2}]
User [{user-id :id}]]
(let [result (pulse/execute-dashboard {:creator_id user-id} dashboard-id)]
(is (= (count result) 2))
(is (schema= [{:card (s/pred map?)
:result (s/pred map?)}]
result))))))
(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]
(letfn [(send-pulse-created-by-user!* [user-kw]
(mt/with-temp* [Collection [coll]
Card [card {:dataset_query (mt/mbql-query checkins
{:order-by [[:asc $id]]
:limit 1})
:collection_id (:id coll)}]]
(perms/revoke-collection-permissions! (group/all-users) coll)
(pulse.tu/send-pulse-created-by-user! user-kw card)))]
(send-pulse-created-by-user! user-kw card)))]
(is (= [[1 "2014-04-07T00:00:00Z" 5 12]]
(send-pulse-created-by-user! :crowberto)))
(send-pulse-created-by-user!* :crowberto)))
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
#"^You do not have permissions to view Card [\d,]+."
(mt/suppress-output
(send-pulse-created-by-user! :rasta)))
(send-pulse-created-by-user!* :rasta)))
"If the current user doesn't have permissions to execute the Card for a Pulse, an Exception should be thrown."))))
......@@ -7,7 +7,7 @@
[metabase.models.pulse-card :refer [PulseCard]]
[metabase.models.pulse-channel :refer [PulseChannel]]
[metabase.models.pulse-channel-recipient :refer [PulseChannelRecipient]]
[metabase.pulse-test :refer [checkins-query-card]]
[metabase.pulse.test-util :refer [checkins-query-card]]
[metabase.task.send-pulses :refer :all]
[metabase.test.data :as data]
[metabase.test.data.users :as users]
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment