EmailReports handle recipients

.dir-locals.el

@@ -27,5 +27,6 @@
                               (match-$ 1)
                               (macrolet 1)
                               (org-perms-case 1)
+                              (symbol-macrolet 1)
                               (upd 2)
                               (with-credentials 1)))))))

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;
 

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]

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."

src/metabase/models/emailreport.clj

 (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-]})))))

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."

test/metabase/api/emailreport_test.clj

 (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)]))))

test/metabase/models/emailreport_test.clj

+(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)]))))

test/metabase/test/util.clj

 (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#)))

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)