Don't blast people with follow-up emails

src/metabase/email/messages.clj

@@ -148,7 +148,7 @@
   [email context]
   {:pre [(u/is-email? email) (map? context)]}
   (let [context      (merge (update context :dependencies build-dependencies)
-                            (notification-context)
+                            notification-context
                             (random-quote-context))
         message-body (stencil/render-file "metabase/email/notification" context)]
     (email/send-message!
@@ -164,11 +164,11 @@
   (let [subject      (if (= "abandon" msg-type)
                        "[Metabase] Help make Metabase better."
                        "[Metabase] Tell us how things are going.")
-        context      (merge (notification-context)
+        context      (merge notification-context
                             (random-quote-context)
                             (if (= "abandon" msg-type)
-                              (abandonment-context)
-                              (follow-up-context)))
+                              abandonment-context
+                              follow-up-context))
         message-body (stencil/render-file "metabase/email/follow_up_email" context)]
     (email/send-message!
       :subject      subject

src/metabase/task/follow_up_emails.clj

@@ -51,30 +51,30 @@
 
 ;; this sends out a general 2 week email follow up email
 (jobs/defjob FollowUpEmail
-             [ctx]
-             ;; if we've already sent the follow-up email then we are done
-             (when-not (follow-up-email-sent)
-               ;; figure out when we consider the instance created
-               (when-let [instance-created (instance-creation-timestamp)]
-                 ;; we need to be 2+ weeks (14 days) from creation to send the follow up
-                 (when (< (* 14 24 60 60 1000)
-                          (- (System/currentTimeMillis) (.getTime instance-created)))
-                   (send-follow-up-email!)))))
+  [ctx]
+  ;; if we've already sent the follow-up email then we are done
+  (when-not (follow-up-email-sent)
+    ;; figure out when we consider the instance created
+    (when-let [instance-created (instance-creation-timestamp)]
+      ;; we need to be 2+ weeks (14 days) from creation to send the follow up
+      (when (< (* 14 24 60 60 1000)
+               (- (System/currentTimeMillis) (.getTime instance-created)))
+        (send-follow-up-email!)))))
 
 ;; this sends out an email any time after 30 days if the instance has stopped being used for 14 days
 (jobs/defjob AbandonmentEmail
-             [ctx]
-             ;; if we've already sent the abandonment email then we are done
-             (when-not (abandonment-email-sent)
-               ;; figure out when we consider the instance created
-               (when-let [instance-created (instance-creation-timestamp)]
-                 ;; we need to be 4+ weeks (30 days) from creation to send the follow up
-                 (when (< (* 30 24 60 60 1000)
-                          (- (System/currentTimeMillis) (.getTime instance-created)))
-                   ;; we need access to email AND the instance must be opted into anonymous tracking
-                   (when (and (email/email-configured?)
-                              (public-settings/anon-tracking-enabled))
-                     (send-abandonment-email!))))))
+  [ctx]
+  ;; if we've already sent the abandonment email then we are done
+  (when-not (abandonment-email-sent)
+    ;; figure out when we consider the instance created
+    (when-let [instance-created (instance-creation-timestamp)]
+      ;; we need to be 4+ weeks (30 days) from creation to send the follow up
+      (when (< (* 30 24 60 60 1000)
+               (- (System/currentTimeMillis) (.getTime instance-created)))
+        ;; we need access to email AND the instance must be opted into anonymous tracking
+        (when (and (email/email-configured?)
+                   (public-settings/anon-tracking-enabled))
+          (send-abandonment-email!))))))
 
 (defn task-init
   "Automatically called during startup; start the job for sending follow up emails."
@@ -109,17 +109,22 @@
 (defn- send-follow-up-email!
   "Send an email to the instance admin following up on their experience with Metabase thus far."
   []
-  (try
-    ;; we need access to email AND the instance must be opted into anonymous tracking
-    (when (and (email/email-configured?)
-               (public-settings/anon-tracking-enabled))
-      ;; grab the oldest admins email address, that's who we'll send to
-      (when-let [admin (User :is_superuser true, {:order-by [:date_joined]})]
-        (messages/send-follow-up-email! (:email admin) "follow-up")))
-    (catch Throwable t
-      (log/error "Problem sending follow-up email" t))
-    (finally
-      (follow-up-email-sent true))))
+  ;; we need access to email AND the instance must be opted into anonymous tracking. Make sure email hasn't been sent yet
+  (when (and (email/email-configured?)
+             (public-settings/anon-tracking-enabled)
+             (not (follow-up-email-sent)))
+    (println "here") ; NOCOMMIT
+    ;; grab the oldest admins email address (likely the user who created this MB instance), that's who we'll send to
+    ;; TODO - Does it make to send to this user instead of `(public-settings/admin-email)`?
+    (when-let [admin (User :is_superuser true, :is_active true, {:order-by [:date_joined]})]
+      (println "admin:" admin) ; NOCOMMIT
+      (try
+        (messages/send-follow-up-email! (:email admin) "follow-up")
+        (catch Throwable e
+          (log/error "Problem sending follow-up email:" e))
+        (finally
+          (follow-up-email-sent true))))))
+
 
 (defn- send-abandonment-email!
   "Send an email to the instance admin about why Metabase usage has died down."

test/metabase/email_test.clj

@@ -2,7 +2,8 @@
   "Various helper functions for testing email functionality."
   ;; TODO - Move to something like `metabase.test.util.email`?
   (:require [expectations :refer :all]
-            [metabase.email :as email]))
+            [metabase.email :as email]
+            [metabase.test.util :as tu]))
 
 (def inbox
   "Map of email addresses -> sequence of messages they've received."
@@ -21,20 +22,28 @@
     (swap! inbox assoc recipient (-> (get @inbox recipient [])
                                      (conj email)))))
 
+(defn do-with-fake-inbox
+  "Impl for `with-fake-inbox` macro; prefer using that rather than calling this directly."
+  [f]
+  (binding [email/*send-email-fn* fake-inbox-email-fn]
+    (reset-inbox!)
+    (tu/with-temporary-setting-values [email-smtp-host "fake_smtp_host"
+                                       email-smtp-port "587"]
+      (f))))
+
 (defmacro with-fake-inbox
   "Clear `inbox`, bind `*send-email-fn*` to `fake-inbox-email-fn`, set temporary settings for `email-smtp-username`
-   and `email-smtp-password`, and execute BODY."
+   and `email-smtp-password` (which will cause `metabase.email/email-configured?` to return `true`, and execute BODY.
+
+   Fetch the emails send by dereffing `inbox`.
+
+     (with-fake-inbox
+       (send-some-emails!)
+       @inbox)"
   [& body]
-  `(binding [email/*send-email-fn* fake-inbox-email-fn]
-     (reset-inbox!)
-     ;; Push some fake settings for SMTP username + password, and restore originals when done
-     (let [orig-hostname# (email/email-smtp-host)
-           orig-port#     (email/email-smtp-port)]
-       (email/email-smtp-host "fake_smtp_host")
-       (email/email-smtp-port "587")
-       (try ~@body
-            (finally (email/email-smtp-host orig-hostname#)
-                     (email/email-smtp-port orig-port#))))))
+  {:style/indent 0}
+  `(do-with-fake-inbox (fn [] ~@body)))
+
 
 ;; simple test of email sending capabilities
 (expect

test/metabase/permissions_collection_test.clj

@@ -18,6 +18,7 @@
 (defn- api-call-was-successful? {:style/indent 0} [response]
   (when (and (string? response)
              (not= response "You don't have permissions to do that."))
+    (println "created users:" (db/select-field :email 'User)) ; NOCOMMIT
     (println "RESPONSE:" response)) ; DEBUG
   (not= response "You don't have permissions to do that."))
 

test/metabase/task/follow_up_emails_test.clj

+(ns metabase.task.follow-up-emails-test
+  (:require [expectations :refer :all]
+            [metabase.email-test :refer [with-fake-inbox inbox]]
+            metabase.task.follow-up-emails
+            [metabase.test.data.users :as test-users]
+            [metabase.test.util :as tu]))
+
+(tu/resolve-private-vars metabase.task.follow-up-emails send-follow-up-email!)
+
+;; Make sure that `send-follow-up-email!` only sends a single email instead even when triggered multiple times (#4253)
+;; follow-up emails get sent to the oldest admin
+(expect
+  1
+  (tu/with-temporary-setting-values [anon-tracking-enabled true
+                                     follow-up-email-sent  false]
+    (test-users/create-users-if-needed!)
+    (with-fake-inbox
+      (send-follow-up-email!)
+      (send-follow-up-email!)
+      (-> @inbox vals first count))))

test/metabase/test/data/users.clj

@@ -63,15 +63,24 @@
 
 
 (defn fetch-user
-  "Fetch the User object associated with USERNAME.
+  "Fetch the User object associated with USERNAME. Creates user if needed.
 
     (fetch-user :rasta) -> {:id 100 :first_name \"Rasta\" ...}"
   [username]
   {:pre [(contains? usernames username)]}
   (m/mapply fetch-or-create-user! (user->info username)))
 
+(defn create-users-if-needed!
+  "Force creation of the test users if they don't already exist."
+  ([]
+   (apply create-users-if-needed! usernames))
+  ([& usernames]
+   (doseq [username usernames]
+     ;; fetch-user will force creation of users
+     (fetch-user username))))
+
 (def ^{:arglists '([username])} user->id
-  "Memoized fn that returns the ID of User associated with USERNAME.
+  "Memoized fn that returns the ID of User associated with USERNAME. Creates user if needed.
 
     (user->id :rasta) -> 4"
   (memoize
@@ -103,6 +112,17 @@
         (swap! tokens assoc username <>))
       (throw (Exception. (format "Authentication failed for %s with credentials %s" username (user->credentials username))))))
 
+(defn- client-fn [username & args]
+  (try
+    (apply http/client (username->token username) args)
+    (catch Throwable e
+      (let [{:keys [status-code]} (ex-data e)]
+        (when-not (= status-code 401)
+          (throw e))
+        ;; If we got a 401 unauthenticated clear the tokens cache + recur
+        (reset! tokens {})
+        (apply client-fn username args)))))
+
 ;; TODO - does it make sense just to make this a non-higher-order function? Or a group of functions, e.g.
 ;; (GET :rasta 200 "field/10/values")
 ;; vs.
@@ -113,18 +133,8 @@
 
      ((user->client) :get 200 \"meta/table\")"
   [username]
-  ;; Force lazy creation of User if need be
-  (user->id username)
-  (fn client-fn [& args]
-    (try
-      (apply http/client (username->token username) args)
-      (catch Throwable e
-        (let [{:keys [status-code]} (ex-data e)]
-          (when-not (= status-code 401)
-            (throw e))
-          ;; If we got a 401 unauthenticated clear the tokens cache + recur
-          (reset! tokens {})
-          (apply client-fn args))))))
+  (create-users-if-needed! username)
+  (partial client-fn username))
 
 
 (defn ^:deprecated delete-temp-users!