Alert schedule in email template (#18084)

src/metabase/email/messages.clj

@@ -30,7 +30,9 @@
             [stencil.core :as stencil]
             [stencil.loader :as stencil-loader]
             [toucan.db :as db])
-  (:import [java.io File IOException OutputStream]))
+  (:import [java.io File IOException OutputStream]
+           java.time.format.DateTimeFormatter
+           java.time.LocalTime))
 
 (defn- app-name-trs
   "Return the user configured application name, or Metabase translated
@@ -513,25 +515,60 @@
             (when alert-condition-map
               {:alertCondition (get alert-condition-map (pulse->alert-condition-kwd alert))})))))
 
+(defn- schedule-hour-text
+  [{hour :schedule_hour}]
+  (.format (LocalTime/of hour 0)
+           (DateTimeFormatter/ofPattern "h a")))
+
+(defn- schedule-day-text
+  [{day :schedule_day}]
+  (get {"sun" "Sunday"
+        "mon" "Monday"
+        "tue" "Tuesday"
+        "wed" "Wednesday"
+        "thu" "Thursday"
+        "fri" "Friday"
+        "sat" "Saturday"}
+       day))
+
+(defn- alert-schedule-text
+  "Returns a string that describes the run schedule of an alert (i.e. how often results are checked),
+  for inclusion in the email template. Not translated, since emails in general are not currently translated."
+  [channel]
+  (case (:schedule_type channel)
+    :hourly
+    "Run hourly"
+
+    :daily
+    (format "Run daily at %s %s"
+            (schedule-hour-text channel)
+            (driver/report-timezone))
+
+    :weekly
+    (format "Run weekly on %s at %s %s"
+            (schedule-day-text channel)
+            (schedule-hour-text channel)
+            (driver/report-timezone))))
+
 (defn- alert-context
   "Context that is applicable only to the actual alert template (not alert management templates)"
-  [alert]
+  [alert channel]
   (let [{card-id :id, card-name :name} (first-card alert)]
-    {:title    card-name
-     :titleUrl (url/card-url card-id)
-     :creator  (-> alert :creator :common_name)}))
+    {:title         card-name
+     :titleUrl      (url/card-url card-id)
+     :alertSchedule (alert-schedule-text channel)
+     :creator       (-> alert :creator :common_name)}))
 
 (defn- alert-results-condition-text [goal-value]
-  {:meets (format "reached its goal of %s" goal-value)
-   :below (format "gone below its goal of %s" goal-value)
-   :rows  "results for you to see"})
+  {:meets (format "This question has reached its goal of %s." goal-value)
+   :below (format "This question has gone below its goal of %s." goal-value)})
 
 (defn render-alert-email
   "Take a pulse object and list of results, returns an array of attachment objects for an email"
-  [timezone {:keys [alert_first_only] :as alert} results goal-value]
+  [timezone {:keys [alert_first_only] :as alert} channel results goal-value]
   (let [message-ctx  (merge
                       (common-alert-context alert (alert-results-condition-text goal-value))
-                      (alert-context alert))]
+                      (alert-context alert channel))]
     (render-message-body alert
                          :alert
                          (assoc message-ctx :firstRunOnly? alert_first_only)

src/metabase/email/pulse.mustache

@@ -102,6 +102,16 @@
                   </td>
                 </tr>
               {{/dashboardDescription}}
+              {{#alertSchedule}}
+                <tr>
+                  <td>&#8202;</td>
+                  <td>
+                    <div style="color: {{colorTextDark}}; font-size: 14px">
+                      {{alertSchedule}}
+                    </div>
+                  </td>
+                </tr>
+              {{/alertSchedule}}
               <tr>
                 <td>&#8202;</td>
                 <td>
@@ -121,6 +131,11 @@
             </table>
             <hr style="border-width: 0; background: #F0F0F0; height: 1px; margin-top: 20px; margin-bottom: 20px;">
             {{{pulse}}}
+            {{#alertCondition}}
+              <p style="color: {{colorTextMedium}}; font-size: 12px; font-weight: 400">
+              {{alertCondition}}
+              </p>
+            {{/alertCondition}}
             {{#firstRunOnly?}}
               <p style="color: {{colorTextMedium}}; font-size: 12px; font-weight: 400">
               We’ll stop sending you alerts about this question now.

src/metabase/pulse.clj

@@ -326,19 +326,19 @@
                                     (when dashboard (slack-dashboard-footer pulse dashboard))]))}))
 
 (defmethod notification [:alert :email]
-  [{:keys [id] :as pulse} results {:keys [recipients]}]
+  [{:keys [id] :as pulse} results channel]
   (log/debug (trs "Sending Alert ({0}: {1}) via email" id name))
   (let [condition-kwd    (messages/pulse->alert-condition-kwd pulse)
-        email-subject    (trs "Metabase alert: {0} has {1}"
+        email-subject    (trs "Alert: {0} has {1}"
                               (first-question-name pulse)
                               (alert-condition-type->description condition-kwd))
-        email-recipients (filterv u/email? (map :email recipients))
+        email-recipients (filterv u/email? (map :email (:recipients channel)))
         first-result     (first results)
         timezone         (-> first-result :card defaulted-timezone)]
     {:subject      email-subject
      :recipients   email-recipients
      :message-type :attachments
-     :message      (messages/render-alert-email timezone pulse results (ui/find-goal-value first-result))}))
+     :message      (messages/render-alert-email timezone pulse channel results (ui/find-goal-value first-result))}))
 
 (defmethod notification [:alert :slack]
   [pulse results {{channel-id :channel} :details}]

test/metabase/email/messages_test.clj

@@ -56,3 +56,22 @@
         (re-pattern (format "Unable to create temp file in `%s`" (System/getProperty "java.io.tmpdir")))
         (with-create-temp-failure
           (#'messages/create-temp-file-or-throw "txt")))))
+
+(deftest alert-schedule-text-test
+  (testing "Alert schedules can be described as English strings, with the timezone included"
+    (tu/with-temporary-setting-values [report-timezone "America/Pacific"]
+      (is (= "Run hourly"
+             (@#'messages/alert-schedule-text {:schedule_type :hourly})))
+      (is (= "Run daily at 12 AM America/Pacific"
+             (@#'messages/alert-schedule-text {:schedule_type :daily
+                                               :schedule_hour 0})))
+      (is (= "Run daily at 5 AM America/Pacific"
+             (@#'messages/alert-schedule-text {:schedule_type :daily
+                                               :schedule_hour 5})))
+      (is (= "Run daily at 6 PM America/Pacific"
+             (@#'messages/alert-schedule-text {:schedule_type :daily
+                                               :schedule_hour 18})))
+      (is (= "Run weekly on Monday at 8 AM America/Pacific"
+             (@#'messages/alert-schedule-text {:schedule_type :weekly
+                                               :schedule_day  "mon"
+                                               :schedule_hour 8}))))))

test/metabase/pulse_test.clj

@@ -422,7 +422,7 @@
        {:email
         (fn [_ _]
           (is (= (rasta-alert-email
-                  "Metabase alert: Test card has results"
+                  "Alert: Test card has results"
                   [(assoc test-card-result "More results have been included" false)
                    png-attachment png-attachment])
                  (mt/summarize-multipart-email test-card-regex #"More results have been included"))))
@@ -458,7 +458,7 @@
        :assert
        {:email
         (fn [_ _]
-          (is (= (rasta-alert-email "Metabase alert: Test card has results"
+          (is (= (rasta-alert-email "Alert: Test card has results"
                                     [(merge test-card-result
                                             {"More results have been included" true
                                              "ID</th>"                         true})
@@ -475,7 +475,7 @@
        :assert
        {:email
         (fn [_ _]
-          (is (= (rasta-alert-email "Metabase alert: Test card has results"
+          (is (= (rasta-alert-email "Alert: Test card has results"
                                     [test-card-result png-attachment png-attachment csv-attachment xls-attachment])
                  (mt/summarize-multipart-email test-card-regex))))}})))
 
@@ -488,7 +488,7 @@
      :assert
      {:email
       (fn [{:keys [pulse-id]} _]
-        (is (= (rasta-alert-email "Metabase alert: Test card has results"
+        (is (= (rasta-alert-email "Alert: Test card has results"
                                   [;(assoc test-card-result "stop sending you alerts" true)
                                    test-card-result
                                    png-attachment
@@ -527,7 +527,7 @@
        :assert
        {:email
         (fn [_ _]
-          (is (= (rasta-alert-email "Metabase alert: Test card has reached its goal"
+          (is (= (rasta-alert-email "Alert: Test card has reached its goal"
                                     [test-card-result png-attachment png-attachment])
                  (mt/summarize-multipart-email test-card-regex))))}}
 
@@ -553,7 +553,7 @@
        :assert
        {:email
         (fn [_ _]
-          (is (= (rasta-alert-email "Metabase alert: Test card has reached its goal"
+          (is (= (rasta-alert-email "Alert: Test card has reached its goal"
                                     [test-card-result png-attachment])
                  (mt/summarize-multipart-email test-card-regex))))}})))
 
@@ -572,7 +572,7 @@
        :assert
        {:email
         (fn [_ _]
-          (is (= (rasta-alert-email "Metabase alert: Test card has gone below its goal"
+          (is (= (rasta-alert-email "Alert: Test card has gone below its goal"
                                     [test-card-result png-attachment png-attachment])
                  (mt/summarize-multipart-email test-card-regex))))}}
 
@@ -596,7 +596,7 @@
        :assert
        {:email
         (fn [_ _]
-          (is (= (rasta-alert-email "Metabase alert: Test card has gone below its goal"
+          (is (= (rasta-alert-email "Alert: Test card has gone below its goal"
                                     [test-card-result png-attachment])
                  (mt/summarize-multipart-email test-card-regex))))}})))
 
@@ -618,7 +618,7 @@
                                                                    :alert_above_goal true}}]
         (email-test-setup
          (pulse/send-pulse! (models.pulse/retrieve-notification pulse-id))
-         (is (= (rasta-alert-email "Metabase alert: Test card has reached its goal"
+         (is (= (rasta-alert-email "Alert: Test card has reached its goal"
                                    [test-card-result png-attachment png-attachment])
                 (mt/summarize-multipart-email test-card-regex))))))))
 

test/metabase/task/send_pulses_test.clj

@@ -33,7 +33,7 @@
           (#'send-pulses/send-pulses! 0 "fri" :first :first on-error))
         (testing "emails"
           (is (= (et/email-to :rasta
-                              {:subject "Metabase alert: My Question Name has results",
+                              {:subject "Alert: My Question Name has results",
                                :body    {"My Question Name" true}})
                  (et/regex-email-bodies #"My Question Name"))))
         (testing "exceptions"