Skip to content
Snippets Groups Projects
Unverified Commit c10232a2 authored by metabase-bot[bot]'s avatar metabase-bot[bot] Committed by GitHub
Browse files

:robot: backported "Creator sentiment emails iterate" (#40925) 


* Cleanup creator sentiment and follow up email code (#39016)

* creator sentiment cleanup

* change setting to be survey-enabled

* fix test

* change send-creator-sentiment-emails

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

---------

Co-authored-by: default avatarJerry Huang <34140255+qwef@users.noreply.github.com>
Co-authored-by: default avatardpsutton <dan@dpsutton.com>
parent da628b52
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 138 additions and 106 deletions
.clj-kondo/hooks/metabase/models/setting.clj
+ 0
− 1
View file @ c10232a2
......@@ -92,7 +92,6 @@
metabot-prompt-generator-token-limit
multi-setting-read-only
notification-link-base-url
no-surveys
num-metabot-choices
openai-api-key
openai-available-models
......
src/metabase/email.clj
+ 8
− 0
View file @ c10232a2
......@@ -102,6 +102,14 @@
:getter #(boolean (email-smtp-host))
:doc false)
(setting/defsetting surveys-enabled
(deferred-tru "Enable or disable surveys")
:type :boolean
:default true
:export? false
:visibility :internal
:audit :getter)
(defn- add-ssl-settings [m ssl-setting]
(merge
m
......
src/metabase/email/messages.clj
+ 11
− 18
View file @ c10232a2
......@@ -291,31 +291,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 the system admin following up on the installation."
[{: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 (if (public-settings/anon-tracking-enabled)
(str "https://metabase.com/feedback/creator?context=" blob)
"https://metabase.com/feedback/creator")}
:link (cond-> "https://metabase.com/feedback/creator"
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"
......
src/metabase/task/creator_sentiment_emails.clj
+ 43
− 31
View file @ c10232a2
......@@ -10,23 +10,17 @@
[metabase.driver.sql.query-processor :as sql.qp]
[metabase.email :as email]
[metabase.email.messages :as messages]
[metabase.models.setting :as setting]
[metabase.public-settings :as public-settings]
[metabase.public-settings.premium-features :as premium-features]
[metabase.task :as task]
[metabase.util.i18n :refer [deferred-tru]]
[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)
(setting/defsetting ^:private no-surveys
(deferred-tru "Enable or disable creator sentiment emails")
:type :boolean
:default false
:visibility :internal
:audit :getter)
(defn- fetch-creators
"Fetch the creators who are eligible for a creator sentiment email. Which are users who, in the past 2 months:
- Created at least 10 questions total
......@@ -59,40 +53,58 @@
"Figure out what plan this Metabase instance is on."
[]
(cond
(and config/ee-available? (premium-features/is-hosted?) (premium-features/has-any-features?)) "pro-cloud/enterprise-cloud"
(and config/ee-available? (premium-features/is-hosted?) (not (premium-features/has-any-features?))) "starter"
(and config/ee-available? (not (premium-features/is-hosted?))) "pro-self-hosted/enterprise-self-hosted"
(not config/ee-available?) "oss"
:else "unknown"))
(and config/ee-available? (premium-features/is-hosted?))
(if (premium-features/has-any-features?)
"pro-cloud/enterprise-cloud"
"starter")
config/ee-available? "pro-self-hosted/enterprise-self-hosted"
:else "unknown"))
(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?)
(not (no-surveys)))
(let [instance-data (when (public-settings/anon-tracking-enabled) (fetch-instance-data))
creators (fetch-creators (premium-features/enable-whitelabeling?))
month (- (.getValue (t/month)) 1)]
(doseq [creator creators]
;; 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 "Problem sending creator sentiment email:" e))))))))
(email/surveys-enabled))
(let [instance-data (fetch-instance-data)
all-creators (fetch-creators (premium-features/enable-whitelabeling?))
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]
(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")
......@@ -105,6 +117,6 @@
(triggers/with-identity (triggers/key creator-sentiment-emails-trigger-key))
(triggers/start-now)
(triggers/with-schedule
;; Fire at 9:15am on the 1st day of every month
(cron/cron-schedule "0 15 9 1 * ?")))]
;; Fire at 2am every saturday
(cron/cron-schedule "0 0 2 ? * 7")))]
(task/schedule-task! job trigger)))
src/metabase/task/follow_up_emails.clj
+ 3
− 2
View file @ c10232a2
......@@ -32,9 +32,10 @@
(defn- send-follow-up-email!
"Send an email to the instance admin following up on their experience with Metabase thus far."
[]
;; we need access to email AND the instance must be opted into anonymous tracking. Make sure email hasn't been sent yet
;; we need access to email AND the instance must be opted into anonymous tracking AND have surveys enabled. Make sure email hasn't been sent yet
(when (and (email/email-configured?)
(public-settings/anon-tracking-enabled)
(email/surveys-enabled)
(not (follow-up-email-sent)))
;; 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)`?
......@@ -42,7 +43,7 @@
(try
(messages/send-follow-up-email! (:email admin))
(catch Throwable e
(log/error "Problem sending follow-up email:" e))
(log/error e "Problem sending follow-up email:"))
(finally
(follow-up-email-sent! true))))))
......
test/metabase/task/creator_sentiment_emails_test.clj
+ 61
− 52
View file @ c10232a2
(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 no surveys is false."
(with-redefs [creator-sentiment-emails/no-surveys (constantly true)]
(#'creator-sentiment-emails/send-creator-sentiment-emails!)
(is (= 0
(-> @inbox vals first count)))))
(testing "Make sure we only send emails when surveys-enabled is true"
(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?))))))
(testing "Make sure that send-creator-sentiment-emails! only sends emails to creators with the correct month hash."
(with-redefs [creator-sentiment-emails/no-surveys (constantly false)
creator-sentiment-emails/fetch-creators (fn [_] [{:email "a@metabase.com"} ;; mods to 1
{: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/with-temporary-setting-values [surveys-enabled true]
(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 26
]
(#'creator-sentiment-emails/send-creator-sentiment-emails! 45)
(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/no-surveys (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 #"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/no-surveys (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="))))))
(mt/reset-inbox!)
(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/no-surveys (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/no-surveys (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 #"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
......
test/metabase/task/follow_up_emails_test.clj
+ 12
− 2
View file @ c10232a2
......@@ -4,17 +4,27 @@
[metabase.email-test :refer [inbox with-fake-inbox]]
[metabase.task.follow-up-emails :as follow-up-emails]
[metabase.test.fixtures :as fixtures]
[metabase.test.util :as tu]))
[metabase.test.util :as mt]))
(use-fixtures :once (fixtures/initialize :test-users))
(deftest send-follow-up-email-test
(testing (str "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"))
(tu/with-temporary-setting-values [anon-tracking-enabled true
(mt/with-temporary-setting-values [anon-tracking-enabled true
follow-up-email-sent false]
(with-fake-inbox
(#'follow-up-emails/send-follow-up-email!)
(#'follow-up-emails/send-follow-up-email!)
(is (= 1
(-> @inbox vals first count))))))
(deftest send-follow-up-email-survey-not-enabled-test
(testing "Make sure we don't send an email when surveys-enabled is false."
(mt/with-temporary-setting-values [anon-tracking-enabled true
follow-up-email-sent false
surveys-enabled false]
(with-fake-inbox
(#'follow-up-emails/send-follow-up-email!)
(is (= 0
(-> @inbox vals first count)))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment