From 77a64cbb2da94850c568bebe5da60cd472f529dd Mon Sep 17 00:00:00 2001
From: Cam Saul <cam@geotip.com>
Date: Tue, 31 Mar 2015 17:24:35 -0700
Subject: [PATCH] EmailReports handle recipients
---
.dir-locals.el | 1 +
.../emailreport/emailreport.controllers.js | 6 +--
src/metabase/api/emailreport.clj | 48 ++++++++++-------
src/metabase/middleware/log_api_call.clj | 2 +-
src/metabase/models/emailreport.clj | 45 +++++++++++++---
src/metabase/models/user.clj | 7 ++-
test/metabase/api/emailreport_test.clj | 53 +++++++++++++++++--
test/metabase/models/emailreport_test.clj | 37 +++++++++++++
test/metabase/test/util.clj | 35 +++++++++++-
test/metabase/test_data.clj | 8 +++
10 files changed, 203 insertions(+), 39 deletions(-)
create mode 100644 test/metabase/models/emailreport_test.clj
diff --git a/.dir-locals.el b/.dir-locals.el
index 0bb401a0c28..778036b06af 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -27,5 +27,6 @@
(match-$ 1)
(macrolet 1)
(org-perms-case 1)
+ (symbol-macrolet 1)
(upd 2)
(with-credentials 1)))))))
diff --git a/resources/frontend_client/app/admin/emailreport/emailreport.controllers.js b/resources/frontend_client/app/admin/emailreport/emailreport.controllers.js
index de7f93d4f15..43fb0eef095 100644
--- a/resources/frontend_client/app/admin/emailreport/emailreport.controllers.js
+++ b/resources/frontend_client/app/admin/emailreport/emailreport.controllers.js
@@ -106,11 +106,7 @@ EmailReportControllers.controller('EmailReportDetail', ['$scope', '$routeParams'
// we need to ensure our recipients list is properly set on the report
var recipients = [];
$scope.form_input.users.forEach(function(user) {
- if (user.incl) {
- recipients.push({
- 'id': user.id
- });
- }
+ if (user.incl) recipients.push(user.id);
});
reportDetail.recipients = recipients;
diff --git a/src/metabase/api/emailreport.clj b/src/metabase/api/emailreport.clj
index 38cd85bb31c..7fb7b461496 100644
--- a/src/metabase/api/emailreport.clj
+++ b/src/metabase/api/emailreport.clj
@@ -54,38 +54,48 @@
(defendpoint POST "/"
"Create a new `EmailReport`."
- [:as {{:keys [dataset_query description email_addresses mode name organization public_perms schedule] :as body} :body}]
+ [:as {{:keys [dataset_query description email_addresses mode name organization public_perms schedule recipients] :as body} :body}]
{dataset_query Required
name Required
organization Required
schedule Required
mode EmailReportMode
- public_perms PublicPerms}
+ public_perms PublicPerms
+ recipients ArrayOfIntegers}
(read-check Org organization)
- (check-500 (ins EmailReport
- :creator_id *current-user-id*
- :dataset_query dataset_query
- :description description
- :email_addresses email_addresses
- :mode mode
- :name name
- :organization_id organization
- :public_perms public_perms
- :schedule schedule))) ; TODO - deal with recipients
-
-
-(defendpoint GET "/:id" [id]
+ (let-500 [report (ins EmailReport
+ :creator_id *current-user-id*
+ :dataset_query dataset_query
+ :description description
+ :email_addresses email_addresses
+ :mode mode
+ :name name
+ :organization_id organization
+ :public_perms public_perms
+ :schedule schedule)]
+ (model/update-recipients report recipients)
+ (hydrate report :recipients)))
+
+
+(defendpoint GET "/:id"
+ "Fetch `EmailReport` with ID."
+ [id]
(->404 (sel :one EmailReport :id id)
read-check
- (hydrate :creator :organization :can_read :can_write)))
+ (hydrate :creator :organization :can_read :can_write :recipients)))
-(defendpoint PUT "/:id" [id :as {{:keys [dataset_query description email_addresses mode name public_perms schedule] :as body} :body}]
+(defendpoint PUT "/:id"
+ "Update an `EmailReport`."
+ [id :as {{:keys [dataset_query description email_addresses mode name public_perms schedule recipients] :as body} :body}]
{name NonEmptyString
mode EmailReportMode
- public_perms PublicPerms}
+ public_perms PublicPerms
+ recipients ArrayOfIntegers}
+ (clojure.pprint/pprint recipients)
(let-404 [report (sel :one EmailReport :id id)]
(write-check report)
+ (model/update-recipients report recipients)
(check-500 (upd-non-nil-keys EmailReport id
:dataset_query dataset_query
:description description
@@ -102,7 +112,7 @@
(defendpoint DELETE "/:id" [id]
(write-check EmailReport id)
- (del EmailReport :id id))
+ (cascade-delete EmailReport :id id))
(defendpoint POST "/:id" [id]
diff --git a/src/metabase/middleware/log_api_call.clj b/src/metabase/middleware/log_api_call.clj
index 816e6a026e6..59d8f948337 100644
--- a/src/metabase/middleware/log_api_call.clj
+++ b/src/metabase/middleware/log_api_call.clj
@@ -27,7 +27,7 @@
(def ^:private only-display-output-on-error
"Set this to `false` to see all API responses."
- true)
+ false)
(defn log-api-call
"Middleware to log `:request` and/or `:response` by passing corresponding OPTIONS."
diff --git a/src/metabase/models/emailreport.clj b/src/metabase/models/emailreport.clj
index 98124f838da..b04c1a2c78c 100644
--- a/src/metabase/models/emailreport.clj
+++ b/src/metabase/models/emailreport.clj
@@ -1,5 +1,6 @@
(ns metabase.models.emailreport
(:require [clojure.data.json :as json]
+ [clojure.set :as set]
[korma.core :refer :all]
[metabase.api.common :refer [check]]
[metabase.db :refer :all]
@@ -91,14 +92,11 @@
(assoc :dataset_query (json/write-str dataset_query)
:schedule (json/write-str schedule)))))
-(defmethod pre-update EmailReport [_ {:keys [version dataset_query schedule id] :as report}]
- (let [version (or version
- (sel :one :field [EmailReport :version] :id id))]
- (assoc report
- :updated_at (util/new-sql-timestamp)
- :dataset_query (json/write-str dataset_query)
- :schedule (json/write-str schedule)
- :version (inc version))))
+(defmethod pre-update EmailReport [_ {:keys [dataset_query schedule id] :as report}]
+ (assoc report ; don't increment `version` here.
+ :updated_at (util/new-sql-timestamp) ; we do that in the API endpoint
+ :dataset_query (json/write-str dataset_query)
+ :schedule (json/write-str schedule)))
(defmethod post-select EmailReport [_ {:keys [id creator_id organization_id] :as report}]
@@ -112,3 +110,34 @@
:recipients (delay (sel :many User
(where {:id [in (subselect EmailReportRecipients (fields :user_id) (where {:emailreport_id id}))]}))))
assoc-permissions-sets))
+
+(defmethod pre-cascade-delete EmailReport [_ {:keys [id]}]
+ (cascade-delete EmailReportRecipients :emailreport_id id))
+
+
+;; ## Related Functions
+
+(defn update-recipients
+ "Update the `EmailReportRecipients` for EMAIL-REPORT.
+ USER-IDS should be a definitive collection of *all* IDs of users who should receive the report.
+
+ * If an ID in USER-IDS has no corresponding existing `EmailReportRecipients` object, one will be created.
+ * If an existing `EmailReportRecipients` has no corresponding ID in USER-IDs, it will be deleted."
+ {:arglists '([email-report user-ids])}
+ [{:keys [id]} user-ids]
+ {:pre [(integer? id)
+ (coll? user-ids)
+ (every? integer? user-ids)]}
+ (let [recipients-old (set (sel :many :field [EmailReportRecipients :user_id] :emailreport_id id))
+ recipients-new (set user-ids)
+ recipients+ (set/difference recipients-new recipients-old)
+ recipients- (set/difference recipients-old recipients-new)]
+ (when (seq recipients+)
+ (let [vs (map #(assoc {:emailreport_id id} :user_id %)
+ recipients+)]
+ (insert EmailReportRecipients
+ (values vs))))
+ (when (seq recipients-)
+ (delete EmailReportRecipients
+ (where {:emailreport_id id
+ :user_id [in recipients-]})))))
diff --git a/src/metabase/models/user.clj b/src/metabase/models/user.clj
index 9dd92436868..3b3546fbfb9 100644
--- a/src/metabase/models/user.clj
+++ b/src/metabase/models/user.clj
@@ -6,6 +6,8 @@
(metabase.models [org-perm :refer [OrgPerm]])
[metabase.util :as util]))
+;; ## Enity + DB Multimethods
+
(defentity User
(table :core_user)
(has-many OrgPerm {:fk :user_id})
@@ -68,8 +70,11 @@
(defmethod pre-cascade-delete User [_ {:keys [id]}]
(cascade-delete 'metabase.models.org-perm/OrgPerm :user_id id)
- (cascade-delete 'metabase.models.session/Session :user_id id))
+ (cascade-delete 'metabase.models.session/Session :user_id id)
+ (cascade-delete 'metabase.models.emailreport-recipients/EmailReportRecipients :user_id id))
+
+;; ## Related Functions
(defn create-user
"Convenience function for creating a new `User` and sending out the welcome email."
diff --git a/test/metabase/api/emailreport_test.clj b/test/metabase/api/emailreport_test.clj
index 842d60e5406..508cc1b5285 100644
--- a/test/metabase/api/emailreport_test.clj
+++ b/test/metabase/api/emailreport_test.clj
@@ -1,13 +1,15 @@
(ns metabase.api.emailreport-test
"Tests for /api/emailreport endpoints."
- (:require [expectations :refer :all]
+ (:require [clojure.tools.macro :refer [symbol-macrolet]]
+ [expectations :refer :all]
[korma.core :refer :all]
[metabase.db :refer :all]
(metabase.models [common :as common]
[database :refer [Database]]
[emailreport :refer [EmailReport] :as emailreport])
- [metabase.test.util :refer [match-$ expect-eval-actual-first random-name]]
- [metabase.test-data :refer :all]))
+ [metabase.test.util :refer [match-$ expect-eval-actual-first random-name with-temp]]
+ [metabase.test-data :refer :all]
+ metabase.test-setup))
;; ## Helper Fns
@@ -17,7 +19,7 @@
:mode (emailreport/mode->id :active)
:public_perms common/perms-readwrite
:email_addresses ""
- :recipients [{:id (user->id :lucky)}]
+ :recipients [(user->id :lucky)]
:dataset_query {:type "query"
:query {:source_table (table->id :venues)
:filter [nil nil]
@@ -92,6 +94,15 @@
:sun true}
:timezone ""
:time_of_day "morning"}
+ :recipients [(match-$ (fetch-user :lucky)
+ {:email "lucky@metabase.com"
+ :first_name "Lucky"
+ :last_login $
+ :is_superuser false
+ :id $
+ :last_name "Pigeon"
+ :date_joined $
+ :common_name "Lucky Pigeon"})]
:organization_id @org-id
:name "My Cool Email Report"
:mode (emailreport/mode->id :active)
@@ -134,6 +145,15 @@
:last_login $
:first_name "Rasta"
:email "rasta@metabase.com"})
+ :recipients [(match-$ (fetch-user :lucky)
+ {:email "lucky@metabase.com"
+ :first_name "Lucky"
+ :last_login $
+ :is_superuser false
+ :id $
+ :last_name "Pigeon"
+ :date_joined $
+ :common_name "Lucky Pigeon"})]
:can_write true
:organization_id @org-id
:name "My Cool Email Report"
@@ -173,3 +193,28 @@
(let [{id :id} (sel :one EmailReport :name er-name)]
((user->client :rasta) :delete 204 (format "emailreport/%d" id))
(er-exists?))]))
+
+
+;; ## RECPIENTS-RELATED TESTS
+;; * Check that recipients are returned by GET /api/emailreport/:id
+;; * Check that we can set them via PUT /api/emailreport/:id
+(expect
+ [#{}
+ #{:rasta}
+ #{:crowberto :lucky}
+ #{}]
+ (with-temp EmailReport [{:keys [id]} {:creator_id (user->id :rasta)
+ :name (random-name)
+ :organization_id @org-id}]
+ (symbol-macrolet [get-recipients (->> ((user->client :rasta) :get 200 (format "emailreport/%d" id))
+ :recipients
+ (map :id)
+ (map id->user)
+ set)]
+ (let [put-recipients (fn [& user-kws]
+ ((user->client :rasta) :put 200 (format "emailreport/%d" id) {:recipients (map user->id user-kws)})
+ get-recipients)]
+ [get-recipients
+ (put-recipients :rasta)
+ (put-recipients :lucky :crowberto)
+ (put-recipients)]))))
diff --git a/test/metabase/models/emailreport_test.clj b/test/metabase/models/emailreport_test.clj
new file mode 100644
index 00000000000..aec6fd63914
--- /dev/null
+++ b/test/metabase/models/emailreport_test.clj
@@ -0,0 +1,37 @@
+(ns metabase.models.emailreport-test
+ (:require [clojure.tools.macro :refer [symbol-macrolet]]
+ [expectations :refer :all]
+ [medley.core :as m]
+ [metabase.db :refer :all]
+ (metabase.models [emailreport :refer :all]
+ [emailreport-recipients :refer :all])
+ [metabase.test-data :refer :all]
+ metabase.test-setup
+ [metabase.test.util :as tu]))
+
+;; ## UPDATE-RECIPIENTS
+;; Check that update-recipients inserts/deletes EmailReportRecipeints as we expect
+(expect
+ [#{}
+ #{:rasta :lucky}
+ #{:rasta :lucky :trashbird}
+ #{:trashbird}
+ #{:crowberto :lucky}
+ #{}
+ #{:rasta}]
+ (tu/with-temp EmailReport [report {:creator_id (user->id :rasta)
+ :name (tu/random-name)
+ :organization_id @org-id}]
+ (symbol-macrolet [recipients (->> (sel :many :field [EmailReportRecipients :user_id] :emailreport_id (:id report))
+ (map id->user)
+ set)]
+ (let [upd-recipients (fn [& recipient-ids]
+ (update-recipients report (map user->id recipient-ids))
+ recipients)]
+ [recipients
+ (upd-recipients :rasta :lucky)
+ (upd-recipients :rasta :lucky :trashbird)
+ (upd-recipients :trashbird)
+ (upd-recipients :crowberto :lucky)
+ (upd-recipients)
+ (upd-recipients :rasta)]))))
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index a7b6fac9318..46abed99bc3 100644
--- a/test/metabase/test/util.clj
+++ b/test/metabase/test/util.clj
@@ -1,7 +1,9 @@
(ns metabase.test.util
"Helper functions and macros for writing unit tests."
(:require [expectations :refer :all]
- [metabase.util :as u]))
+ [medley.core :as m]
+ (metabase [db :refer :all]
+ [util :as u])))
(declare $->prop)
@@ -64,9 +66,40 @@
`(def ~(vary-meta fn-name assoc :expectation true)
(fn [] (-doexpect ~expected ~actual)))))
+
;; ## random-name
(defn random-name
"Generate a random string of 20 uppercase letters."
[]
(->> (repeatedly 20 #(-> (rand-int 26) (+ (int \A)) char))
(apply str)))
+
+
+;; ## with-temp
+;;
+(defmacro with-temp
+ "Create a temporary instance of ENTITY bound to BINDING-FORM, execute BODY,
+ then delete it via `cascade-delete`.
+
+ Our unit tests rely a heavily on the test data and make some assumptions about the
+ DB staying in the same *clean* state. This allows us to write very conscise tests.
+ Generally this means tests should \"clean up after themselves\" and leave things the
+ way they found them.
+
+ `with-temp` should be preferrable going forward over creating random objects *without*
+ deleting them afterward.
+
+ (with-temp EmailReport [report {:creator_id (user->id :rasta)
+ :name (random-name)
+ :organization_id @org-id}]
+ ...)"
+ [entity [binding-form & [options-map]] & body]
+ `(let [object# (m/mapply ins ~entity ~options-map)
+ ~binding-form object#
+ delete-fn# (fn [] (cascade-delete ~entity :id (:id object#)))]
+ (let [result# (try (do ~@body)
+ (catch Throwable e#
+ (delete-fn#)
+ (throw e#)))]
+ (delete-fn#)
+ result#)))
diff --git a/test/metabase/test_data.clj b/test/metabase/test_data.clj
index 6d825813c6d..6572481cca5 100644
--- a/test/metabase/test_data.clj
+++ b/test/metabase/test_data.clj
@@ -123,6 +123,14 @@
{:pre [(contains? usernames username)]}
(:id (fetch-user username)))))
+(def id->user
+ "Reverse of `user->id`.
+
+ (id->user 4) -> :rasta"
+ (let [m (delay (zipmap (map user->id usernames) usernames))]
+ (fn [id]
+ (@m id))))
+
(let [tokens (atom {})
user->token (fn [user]
(or (@tokens user)
--
GitLab