From 12ea8d42b515fb8128ec6c053ed9a07581f9f7d1 Mon Sep 17 00:00:00 2001
From: dpsutton <dan@dpsutton.com>
Date: Tue, 2 Apr 2024 12:34:56 -0700
Subject: [PATCH] Creator sentiment emails iterate (#40922)
* Send creator emails every week
mod the email by 52 instead of by 12 and use the current week as an
anchor
```clojure
user=> (require '[java-time.api :as t])
nil
user=> (let [every-day (take 1000 (t/iterate t/plus (t/local-date 2024 1 1) (t/days 1)))
f (fn [d]
(let [wf (java.time.temporal.WeekFields/of (java.util.Locale/getDefault))]
(.get d (.weekOfWeekBasedYear wf))))]
(sort (keys (frequencies (map f every-day)))))
(1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21
22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39
40 41 42 43 44 45 46 47 48 49 50 51 52)
```
```clojure
user=> (metabase.util.cron/cron-string->schedule-map "0 0 2 ? * 7")
{:schedule_minute 0,
:schedule_day "sat",
:schedule_frame nil,
:schedule_hour 2,
:schedule_type "weekly"}
```
* fix typo in creator sentiment email, clean up a bit
typo: had `:verison` instead of `version`.
And while I'm in there, make the payload all in one file and have the
email namespace json->bytes->b64-encode so the data is all in one place
and the setting that allows it to be present or not is co-located.
---
src/metabase/email/messages.clj | 27 ++---
.../task/creator_sentiment_emails.clj | 48 +++++---
.../task/creator_sentiment_emails_test.clj | 107 ++++++++++--------
3 files changed, 99 insertions(+), 83 deletions(-)
diff --git a/src/metabase/email/messages.clj b/src/metabase/email/messages.clj
index 74a9f8fb999..070c231588a 100644
--- a/src/metabase/email/messages.clj
+++ b/src/metabase/email/messages.clj
@@ -289,31 +289,24 @@
:message (stencil/render-file "metabase/email/follow_up_email" context)}]
(email/send-message! email)))
-(defn- creator-sentiment-blob
- "Create a blob of instance/user data to be sent to the creator sentiment survey."
- [instance-data created_at num_dashboards num_questions num_models]
- (-> {:instance instance-data
- :user {:created_at created_at
- :num_dashboards num_dashboards
- :num_questions num_questions
- :num_models num_models}}
- json/generate-string
- .getBytes
- codecs/bytes->b64-str))
+
(defn send-creator-sentiment-email!
- "Format and send an email to a creator with a link to a survey. Can include info about the instance and the creator
- if [[public-settings/anon-tracking-enabled]] is true."
- [{:keys [email created_at first_name num_dashboards num_questions num_models]} instance-data]
+ "Format and send an email to a creator with a link to a survey. If a [[blob]] is included, it will be turned into json
+ and then base64 encoded."
+ [{:keys [email first_name]} blob]
{:pre [(u/email? email)]}
- (let [blob (when (public-settings/anon-tracking-enabled)
- (creator-sentiment-blob instance-data created_at num_dashboards num_questions num_models))
+ (let [encoded-info (when blob
+ (-> blob
+ json/generate-string
+ .getBytes
+ codecs/bytes->b64-str))
context (merge (common-context)
{:emailType "notification"
:logoHeader true
:first-name first_name
:link (cond-> "https://metabase.com/feedback/creator"
- blob (str "?context=" blob))}
+ encoded-info (str "?context=" encoded-info))}
(when-not (premium-features/is-hosted?)
{:self-hosted (public-settings/site-url)}))
message {:subject "Metabase would love your take on something"
diff --git a/src/metabase/task/creator_sentiment_emails.clj b/src/metabase/task/creator_sentiment_emails.clj
index a45921bb793..7279557c318 100644
--- a/src/metabase/task/creator_sentiment_emails.clj
+++ b/src/metabase/task/creator_sentiment_emails.clj
@@ -14,7 +14,10 @@
[metabase.public-settings.premium-features :as premium-features]
[metabase.task :as task]
[metabase.util.log :as log]
- [toucan2.core :as t2]))
+ [toucan2.core :as t2])
+ (:import
+ (java.util Locale)
+ (java.time.temporal WeekFields)))
(set! *warn-on-reflection* true)
@@ -61,36 +64,47 @@
(defn- fetch-instance-data []
{:created_at (snowplow/instance-creation)
:plan (fetch-plan-info)
- :verison (config/mb-version-info :tag)
+ :version (config/mb-version-info :tag)
:num_users (t2/count :model/User :is_active true, :type "personal")
:num_dashboards (t2/count :model/Dashboard :archived false)
+ :num_databases (t2/count :model/Database :is_audit false)
:num_questions (t2/count :model/Card :archived false :type "question")
:num_models (t2/count :model/Card :archived false :type "model")})
+(defn- user-instance-info
+ "Create a blob of instance/user data to be sent to the creator sentiment survey."
+ [instance-data {:keys [created_at num_dashboards num_questions num_models]}]
+ {:instance instance-data
+ :user {:created_at created_at
+ :num_dashboards num_dashboards
+ :num_questions num_questions
+ :num_models num_models}})
+
(defn- send-creator-sentiment-emails!
"Send an email to the instance admin following up on their experience with Metabase thus far."
- []
+ [current-week]
;; we need access to email AND the instance must have surveys enabled.
(when (and (email/email-configured?)
(email/surveys-enabled))
- (let [instance-data (when (public-settings/anon-tracking-enabled)
- (fetch-instance-data))
+ (let [instance-data (fetch-instance-data)
all-creators (fetch-creators (premium-features/enable-whitelabeling?))
- month (- (.getValue (t/month)) 1)
- this-month (fn [c] (= month (-> c :email hash (mod 12))))
- recipients (filter this-month all-creators)]
+ this-week? (fn [c] (= current-week (-> c :email hash (mod 52))))
+ recipients (filter this-week? all-creators)
+ blob (if (public-settings/anon-tracking-enabled)
+ (fn [creator]
+ (user-instance-info instance-data creator))
+ (constantly nil))]
(log/infof "Sending surveys to %d creators of a total %d"
(count all-creators) (count recipients))
(doseq [creator recipients]
- ;; Send the email if the creator's email hash matches the current month
- (when (= (-> creator :email hash (mod 12)) month)
- (try
- (messages/send-creator-sentiment-email! creator instance-data)
- (catch Throwable e
- (log/error e "Problem sending creator sentiment email:"))))))))
+ (try
+ (messages/send-creator-sentiment-email! creator (blob creator))
+ (catch Throwable e
+ (log/error e "Problem sending creator sentiment email:")))))))
(jobs/defjob ^{:doc "Sends out a monthly survey to a portion of the creators."} CreatorSentimentEmail [_]
- (send-creator-sentiment-emails!))
+ (let [current-week (.get (t/local-date) (.weekOfWeekBasedYear (WeekFields/of (Locale/getDefault))))]
+ (send-creator-sentiment-emails! current-week)))
(def ^:private creator-sentiment-emails-job-key "metabase.task.creator-sentiment-emails.job")
(def ^:private creator-sentiment-emails-trigger-key "metabase.task.creator-sentiment-emails.trigger")
@@ -103,6 +117,6 @@
(triggers/with-identity (triggers/key creator-sentiment-emails-trigger-key))
(triggers/start-now)
(triggers/with-schedule
- ;; Fire at 2am on the second saturday of the month
- (cron/cron-schedule "0 0 2 ? * 7#2")))]
+ ;; Fire at 2am every saturday
+ (cron/cron-schedule "0 0 2 ? * 7")))]
(task/schedule-task! job trigger)))
diff --git a/test/metabase/task/creator_sentiment_emails_test.clj b/test/metabase/task/creator_sentiment_emails_test.clj
index d47dbe2e62a..488e2891cc1 100644
--- a/test/metabase/task/creator_sentiment_emails_test.clj
+++ b/test/metabase/task/creator_sentiment_emails_test.clj
@@ -1,71 +1,80 @@
(ns ^:mb/once metabase.task.creator-sentiment-emails-test
(:require
+ [buddy.core.codecs :as codecs]
+ [cheshire.core :as json]
[clojure.test :refer :all]
[java-time.api :as t]
[metabase.email-test :as et :refer [inbox]]
- [metabase.public-settings :as public-settings]
[metabase.public-settings.premium-features :as premium-features]
[metabase.task.creator-sentiment-emails :as creator-sentiment-emails]
[metabase.test :as mt]
+ [metabase.util.malli.schema :as ms]
[toucan2.tools.with-temp :as t2.with-temp]))
(deftest send-creator-sentiment-emails!-test
(mt/with-fake-inbox
(testing "Make sure we only send emails when surveys-enabled is true"
- (mt/with-temporary-setting-values [surveys-enabled false]
- (with-redefs [creator-sentiment-emails/fetch-creators (fn [_] [{:email "a@metabase.com"} ;; mods to 1, this email would be sent if surveys-enabled was true
- {:email "b@metabase.com"} ;; mods to 4
- {:email "c@metabase.com"}]) ;; mods to 2
- t/month (constantly (t/month 2))]
- (#'creator-sentiment-emails/send-creator-sentiment-emails!)
- (is (= 0
- (-> @inbox vals first count))))))
+ (doseq [enabled? [true false]]
+ (mt/reset-inbox!)
+ (mt/with-temporary-setting-values [surveys-enabled enabled?]
+ (with-redefs [creator-sentiment-emails/fetch-creators (fn [_] [{:email "a@metabase.com"} ;; mods to 45, this email would be sent if surveys-enabled was true
+ {:email "b@metabase.com"} ;; mods to 4
+ {:email "c@metabase.com"}]) ;; mods to 26
+ ]
+ (#'creator-sentiment-emails/send-creator-sentiment-emails! 45)
+ (is (= (if enabled? 1 0)
+ (-> @inbox vals first count))
+ (str "error when enabled? is " enabled?))))))
(mt/with-temporary-setting-values [surveys-enabled true]
- (testing "Make sure that send-creator-sentiment-emails! only sends emails to creators with the correct month hash."
- (with-redefs [creator-sentiment-emails/fetch-creators (fn [_] [{:email "a@metabase.com"} ;; mods to 1
+ (testing "Make sure that send-creator-sentiment-emails! only sends emails to creators with the correct week hash."
+ (with-redefs [creator-sentiment-emails/fetch-creators (fn [_] [{:email "a@metabase.com"} ;; mods to 45
{:email "b@metabase.com"} ;; mods to 4
- {:email "c@metabase.com"}]) ;; mods to 2
- t/month (constantly (t/month 2))]
- (#'creator-sentiment-emails/send-creator-sentiment-emails!)
- (is (= 1
- (-> @inbox vals first count)))))
-
- (mt/reset-inbox!)
- (testing "Make sure context is included when anon tracking is enabled"
- (with-redefs [public-settings/anon-tracking-enabled (constantly true)
- creator-sentiment-emails/fetch-creators (fn [_] [{:email "a@metabase.com"}])
- t/month (constantly (t/month 2))]
- (#'creator-sentiment-emails/send-creator-sentiment-emails!)
+ {:email "c@metabase.com"}]) ;; mods to 26
+ ]
+ (#'creator-sentiment-emails/send-creator-sentiment-emails! 45)
(is (= 1
- (count (et/regex-email-bodies #"creator\?context=")))))))
-
- (mt/reset-inbox!)
- (testing "Make sure context isn't included when anon tracking is enabled"
- (with-redefs [public-settings/anon-tracking-enabled (constantly false)
- creator-sentiment-emails/fetch-creators (fn [_] [{:email "a@metabase.com"}])
- t/month (constantly (t/month 2))]
- (#'creator-sentiment-emails/send-creator-sentiment-emails!)
- (is (= 0
- (count (et/regex-email-bodies #"creator\?context="))))))
+ (-> @inbox vals first count))))))
- (mt/reset-inbox!)
+ (testing "Make sure context is included when anon tracking is enabled"
+ (doseq [tracking-enabled? [true false]]
+ (mt/reset-inbox!)
+ (mt/with-temporary-setting-values [anon-tracking-enabled tracking-enabled?]
+ (with-redefs [creator-sentiment-emails/fetch-creators (fn [_] [{:email "a@metabase.com"
+ :created_at (t/local-date-time)
+ :num_dashboards 4
+ :num_questions 7
+ :num_models 2}])]
+ (#'creator-sentiment-emails/send-creator-sentiment-emails! 45)
+ (is (= (if tracking-enabled? 1 0)
+ (count (et/regex-email-bodies #"creator\?context="))))
+ (when tracking-enabled?
+ (let [email-body (-> @inbox vals first first :body first :content)]
+ (is (string? email-body))
+ (let [[_ query-params] (re-find #"creator\?context=(.*)\"" email-body)
+ decoded (some-> query-params codecs/b64->str json/parse-string )]
+ (is (=? {"instance" {"created_at" (mt/malli=? ms/TemporalString)
+ "plan" (mt/malli=? :string)
+ "version" (mt/malli=? :string)
+ "num_users" (mt/malli=? :int)
+ "num_dashboards" (mt/malli=? :int)
+ "num_databases" (mt/malli=? :int)
+ "num_questions" (mt/malli=? :int)
+ "num_models" (mt/malli=? :int)}
+ "user" {"created_at" (mt/malli=? ms/TemporalString)
+ "num_dashboards" 4
+ "num_questions" 7
+ "num_models" 2}}
+ decoded)))))))))
(testing "Make sure external services message is included when is self hosted"
- (with-redefs [premium-features/is-hosted? (constantly false)
- creator-sentiment-emails/fetch-creators (fn [_] [{:email "a@metabase.com"}])
- t/month (constantly (t/month 2))]
- (#'creator-sentiment-emails/send-creator-sentiment-emails!)
- (is (= 1
- (count (et/regex-email-bodies #"external services"))))))))
-
- (mt/reset-inbox!)
- (testing "Make sure external services isn't included when not self hosted"
- (with-redefs [premium-features/is-hosted? (constantly true)
- creator-sentiment-emails/fetch-creators (fn [_] [{:email "a@metabase.com"}])
- t/month (constantly (t/month 2))]
- (#'creator-sentiment-emails/send-creator-sentiment-emails!)
- (is (= 0
- (count (et/regex-email-bodies #"external services"))))))
+ (doseq [hosted? [true false]]
+ (mt/reset-inbox!)
+ (with-redefs [creator-sentiment-emails/fetch-creators (fn [_] [{:email "a@metabase.com"}])
+ ;; can't use mt/with-temporary-setting-values because of a custom :getter
+ premium-features/is-hosted? (constantly hosted?)]
+ (#'creator-sentiment-emails/send-creator-sentiment-emails! 45)
+ (is (= (if hosted? 0 1)
+ (count (et/regex-email-bodies #"external services")))))))))
(deftest fetch-creators-test
(let [creator-id 33
--
GitLab