Add reply-to and from-name email settings (#23381)

* Add reply-to setting for emails

* Add reply-to to SMTPSettings

* Change setting UI to match spec

* Remove WIP comment

* Add email-from-name setting

* Support comma delimited list of emails for reply-to setting

* Remove extra require

* Remove console.logging

* Improve the setting descriptions

* update email settings

* For now, limit reply-to field to one email

* Change back SMTPSettings to closed map

* Change test to only one reply-to email

* Fix SMTPSettings reply-to type

* Fix email-api-test

* Fix e2e email setup

* Add more checks in e2e tests

* Fix smoketest

Co-authored-by: Aleksandr Lesnenko <alxnddr@gmail.com>

enterprise/backend/test/metabase_enterprise/advanced_permissions/api/setting_test.clj

@@ -73,8 +73,9 @@
                                                                     :email-smtp-security :tls
                                                                     :email-smtp-username "munchkin"
                                                                     :email-smtp-password "gobble gobble"
-                                                                    :email-from-address  "eating@hungry.com"}))))
-
+                                                                    :email-from-address  "eating@hungry.com"
+                                                                    :email-from-name     "Eating"
+                                                                    :email-reply-to      ["reply-to@hungry.com"]}))))
               (delete-email-setting [user status]
                 (testing (format "delete email setting with %s user" (mt/user-descriptor user))
                   (mt/user-http-request user :delete status "email")))

frontend/src/metabase/admin/settings/components/SettingsBatchForm.jsx

@@ -19,6 +19,10 @@ const VALIDATIONS = {
     validate: value => MetabaseUtils.isEmail(value),
     message: t`That's not a valid email address`,
   },
+  email_list: {
+    validate: value => value.every(MetabaseUtils.isEmail),
+    message: t`That's not a valid list of email addresses`,
+  },
   integer: {
     validate: value => !isNaN(parseInt(value)),
     message: t`That's not a valid integer`,

frontend/src/metabase/admin/settings/components/SettingsEmailForm.jsx

@@ -74,12 +74,14 @@ class SettingsEmailForm extends Component {
 
   render() {
     const { sendingEmail } = this.state;
-
+    const { elements } = this.props;
+    const visibleElements = elements.filter(setting => !setting.getHidden?.());
     return (
       <EmailFormRoot>
         <SettingsBatchForm
           ref={form => (this._form = form && form.getWrappedInstance())}
           {...this.props}
+          elements={visibleElements}
           updateSettings={this.props.updateEmailSettings}
           disable={sendingEmail !== "default"}
           renderExtraButtons={({ disabled, valid, pristine, submitting }) => (

frontend/src/metabase/admin/settings/components/widgets/SettingCommaDelimitedInput.jsx

+/* eslint-disable react/prop-types */
+import React from "react";
+import InputBlurChange from "metabase/components/InputBlurChange";
+import cx from "classnames";
+
+const SettingCommaDelimitedInput = ({
+  setting,
+  onChange,
+  disabled,
+  autoFocus,
+  errorMessage,
+  fireOnChange,
+  id,
+  type = "text",
+}) => {
+  return (
+    <InputBlurChange
+      className={cx("Form-input", {
+        SettingsInput: true,
+        "border-error bg-error-input": errorMessage,
+      })}
+      id={id}
+      type={type}
+      // TOOD: change this to support multiple email addresses
+      // https://github.com/metabase/metabase/issues/22540
+      value={setting.value ? setting.value[0] : ""}
+      placeholder={setting.placeholder}
+      onChange={fireOnChange ? e => onChange([e.target.value]) : null}
+      onBlurChange={!fireOnChange ? e => onChange([e.target.value]) : null}
+      autoFocus={autoFocus}
+    />
+  );
+};
+
+export default SettingCommaDelimitedInput;

frontend/src/metabase/admin/settings/selectors.js

@@ -4,6 +4,7 @@ import _ from "underscore";
 import { createSelector } from "reselect";
 import { t, jt } from "ttag";
 import ExternalLink from "metabase/core/components/ExternalLink";
+import SettingCommaDelimitedInput from "./components/widgets/SettingCommaDelimitedInput";
 import MetabaseSettings from "metabase/lib/settings";
 import CustomGeoJSONWidget from "./components/widgets/CustomGeoJSONWidget";
 import SettingsLicense from "./components/SettingsLicense";
@@ -155,6 +156,7 @@ const SECTIONS = updateSectionsWithPlugins({
         type: "string",
         required: true,
         autoFocus: true,
+        getHidden: () => MetabaseSettings.isHosted(),
       },
       {
         key: "email-smtp-port",
@@ -163,6 +165,7 @@ const SECTIONS = updateSectionsWithPlugins({
         type: "number",
         required: true,
         validations: [["integer", t`That's not a valid port number`]],
+        getHidden: () => MetabaseSettings.isHosted(),
       },
       {
         key: "email-smtp-security",
@@ -171,6 +174,7 @@ const SECTIONS = updateSectionsWithPlugins({
         type: "radio",
         options: { none: "None", ssl: "SSL", tls: "TLS", starttls: "STARTTLS" },
         defaultValue: "none",
+        getHidden: () => MetabaseSettings.isHosted(),
       },
       {
         key: "email-smtp-username",
@@ -178,6 +182,7 @@ const SECTIONS = updateSectionsWithPlugins({
         description: null,
         placeholder: "nicetoseeyou",
         type: "string",
+        getHidden: () => MetabaseSettings.isHosted(),
       },
       {
         key: "email-smtp-password",
@@ -185,6 +190,14 @@ const SECTIONS = updateSectionsWithPlugins({
         description: null,
         placeholder: "Shhh...",
         type: "password",
+        getHidden: () => MetabaseSettings.isHosted(),
+      },
+      {
+        key: "email-from-name",
+        display_name: t`From Name`,
+        placeholder: "Metabase",
+        type: "string",
+        required: false,
       },
       {
         key: "email-from-address",
@@ -194,6 +207,15 @@ const SECTIONS = updateSectionsWithPlugins({
         required: true,
         validations: [["email", t`That's not a valid email address`]],
       },
+      {
+        key: "email-reply-to",
+        display_name: t`Reply-To Address`,
+        placeholder: "metabase-replies@yourcompany.com",
+        type: "string",
+        required: false,
+        widget: SettingCommaDelimitedInput,
+        validations: [["email_list", t`That's not a valid email addresses`]],
+      },
     ],
   },
   slack: {

frontend/src/metabase/plugins/builtin/settings/hosted.js

@@ -6,9 +6,7 @@ import { PLUGIN_ADMIN_SETTINGS_UPDATES } from "metabase/plugins";
 import { SettingsCloudStoreLink } from "../../components/SettingsCloudStoreLink";
 
 if (MetabaseSettings.isHosted()) {
-  PLUGIN_ADMIN_SETTINGS_UPDATES.push(sections =>
-    _.omit(sections, ["email", "updates"]),
-  );
+  PLUGIN_ADMIN_SETTINGS_UPDATES.push(sections => _.omit(sections, ["updates"]));
 
   PLUGIN_ADMIN_SETTINGS_UPDATES.push(sections =>
     updateIn(sections, ["general", "settings"], settings =>

frontend/test/__support__/e2e/helpers/e2e-email-helpers.js

@@ -20,6 +20,8 @@ export const setupSMTP = () => {
     "email-smtp-password": "admin",
     "email-smtp-security": "none",
     "email-from-address": "mailer@metabase.test",
+    "email-from-name": "Metabase",
+    "email-reply-to": ["reply-to@metabase.test"],
   });
 
   // We must always clear Webmail's inbox before each test

frontend/test/metabase/scenarios/admin/settings/email.cy.spec.js

@@ -23,6 +23,12 @@ describe("scenarios > admin > settings > email settings", () => {
     cy.findByLabelText("From Address")
       .type("mailer@metabase.test")
       .blur();
+    cy.findByLabelText("From Name")
+      .type("Sender Name")
+      .blur();
+    cy.findByLabelText("Reply-To Address")
+      .type("reply-to@metabase.test")
+      .blur();
     cy.findByText("Save changes").click();
 
     cy.findByText("Changes saved!", { timeout: 10000 });
@@ -32,12 +38,16 @@ describe("scenarios > admin > settings > email settings", () => {
     cy.findByDisplayValue("25");
     cy.findAllByDisplayValue("admin");
     cy.findByDisplayValue("mailer@metabase.test");
+    cy.findByDisplayValue("Sender Name");
+    cy.findByDisplayValue("reply-to@metabase.test");
   });
 
   it("should show an error if test email fails", () => {
     // Reuse Email setup without relying on the previous test
     cy.request("PUT", "/api/setting", {
       "email-from-address": "admin@metabase.test",
+      "email-from-name": "Metabase Admin",
+      "email-reply-to": "reply-to@metabase.test",
       "email-smtp-host": "localhost",
       "email-smtp-password": null,
       "email-smtp-port": "1234",
@@ -66,7 +76,9 @@ describe("scenarios > admin > settings > email settings", () => {
     cy.findByText("Clear").click();
     cy.findByLabelText("SMTP Host").should("have.value", "");
     cy.findByLabelText("SMTP Port").should("have.value", "");
+    cy.findByLabelText("From Name").should("have.value", "");
     cy.findByLabelText("From Address").should("have.value", "");
+    cy.findByLabelText("Reply-To Address").should("have.value", "");
   });
 
   it("should not offer to save email changes when there aren't any (metabase#14749)", () => {

src/metabase/api/email.clj

@@ -19,7 +19,9 @@
    :email-smtp-password :pass
    :email-smtp-port     :port
    :email-smtp-security :security
-   :email-from-address  :sender})
+   :email-from-name     :sender-name
+   :email-from-address  :sender
+   :email-reply-to      :reply-to})
 
 (defn- humanize-error-messages
   "Convert raw error message responses from our email functions into our normal api error response structure."

src/metabase/email.clj

@@ -16,9 +16,16 @@
 ;;; CONFIG
 
 (defsetting email-from-address
-  (deferred-tru "Email address you want to use as the sender of Metabase.")
+  (deferred-tru "The email address you want to use for the sender of emails.")
   :default "notifications@metabase.com")
 
+(defsetting email-from-name
+  (deferred-tru "The name you want to use for the sender of emails."))
+
+(defsetting email-reply-to
+  (deferred-tru "The email address you want the replies to go to, if different from the from address. You can have multiple reply-to email addresses, just separate them with a comma.")
+  :type :json)
+
 (defsetting email-smtp-host
   (deferred-tru "The address of the SMTP server that handles your emails."))
 
@@ -95,14 +102,19 @@
     (throw (ex-info (tru "SMTP host is not set.") {:cause :smtp-host-not-set})))
   ;; Now send the email
   (send-email! (smtp-settings)
-    {:from    (email-from-address)
-     :to      recipients
-     :subject subject
-     :body    (case message-type
-                :attachments message
-                :text        message
-                :html        [{:type    "text/html; charset=utf-8"
-                               :content message}])}))
+               (merge
+                {:from    (if-let [from-name (email-from-name)]
+                            (str from-name " <" (email-from-address) ">")
+                            (email-from-address))
+                 :to      recipients
+                 :subject subject
+                 :body    (case message-type
+                            :attachments message
+                            :text        message
+                            :html        [{:type    "text/html; charset=utf-8"
+                                           :content message}])}
+                (when-let [reply-to (email-reply-to)]
+                  {:reply-to reply-to}))))
 
 (def ^:private SMTPStatus
   "Schema for the response returned by various functions in [[metabase.email]]. Response will be a map with the key
@@ -130,13 +142,15 @@
       {::error e})))
 
 (def ^:private SMTPSettings
-  {:host                      su/NonBlankString
-   :port                      su/IntGreaterThanZero
-   ;; TODO -- not sure which of these other ones are actually required or not, and which are optional.
-   (s/optional-key :user)     (s/maybe s/Str)
-   (s/optional-key :security) (s/maybe (s/enum :tls :ssl :none :starttls))
-   (s/optional-key :pass)     (s/maybe s/Str)
-   (s/optional-key :sender)   (s/maybe s/Str)})
+  {:host                         su/NonBlankString
+   :port                         su/IntGreaterThanZero
+     ;; TODO -- not sure which of these other ones are actually required or not, and which are optional.
+   (s/optional-key :user)        (s/maybe s/Str)
+   (s/optional-key :security)    (s/maybe (s/enum :tls :ssl :none :starttls))
+   (s/optional-key :pass)        (s/maybe s/Str)
+   (s/optional-key :sender)      (s/maybe s/Str)
+   (s/optional-key :sender-name) (s/maybe s/Str)
+   (s/optional-key :reply-to)    (s/maybe [s/Str])})
 
 (s/defn ^:private test-smtp-settings :- SMTPStatus
   "Tests an SMTP configuration by attempting to connect and authenticate if an authenticated method is passed

test/metabase/api/email_test.clj

@@ -22,7 +22,9 @@
    :email-smtp-security (setting/get :email-smtp-security)
    :email-smtp-username (setting/get :email-smtp-username)
    :email-smtp-password (setting/get :email-smtp-password)
-   :email-from-address  (setting/get :email-from-address)})
+   :email-from-address  (setting/get :email-from-address)
+   :email-from-name     (setting/get :email-from-name)
+   :email-reply-to      (setting/get :email-reply-to)})
 
 (def ^:private default-email-settings
   {:email-smtp-host     "foobar"
@@ -30,11 +32,15 @@
    :email-smtp-security :tls
    :email-smtp-username "munchkin"
    :email-smtp-password "gobble gobble"
-   :email-from-address  "eating@hungry.com"})
+   :email-from-address  "eating@hungry.com"
+   :email-from-name     "Eating"
+   :email-reply-to      ["reply-to@hungry.com"]})
 
 (deftest test-email-settings-test
   (testing "POST /api/email/test -- send a test email"
-    (mt/with-temporary-setting-values [email-from-address "notifications@metabase.com"]
+    (mt/with-temporary-setting-values [email-from-address "notifications@metabase.com"
+                                       email-from-name    "Sender Name"
+                                       email-reply-to     ["reply-to@metabase.com"]]
       (mt/with-fake-inbox
         (testing "Non-admin -- request should fail"
           (is (= "You don't have permissions to do that."
@@ -44,10 +50,11 @@
         (is (= {:ok true}
                (mt/user-http-request :crowberto :post 200 "email/test")))
         (is (= {"crowberto@metabase.com"
-                [{:from    "notifications@metabase.com",
-                  :to      ["crowberto@metabase.com"],
-                  :subject "Metabase Test Email",
-                  :body    "Your Metabase emails are working — hooray!"}]}
+                [{:from     "Sender Name <notifications@metabase.com>",
+                  :to       ["crowberto@metabase.com"],
+                  :reply-to ["reply-to@metabase.com"]
+                  :subject  "Metabase Test Email",
+                  :body     "Your Metabase emails are working — hooray!"}]}
                @mt/inbox))))))
 
 (deftest update-email-settings-test
@@ -66,7 +73,7 @@
                                   (with-redefs [email/retry-delay-ms 0]
                                     (thunk)))}]
       (tu/discard-setting-changes [email-smtp-host email-smtp-port email-smtp-security email-smtp-username
-                                   email-smtp-password email-from-address]
+                                   email-smtp-password email-from-address email-from-name email-reply-to]
         (testing (format "SMTP connection is valid? %b\n" success?)
           (f (fn []
                (testing "API request"
@@ -88,7 +95,7 @@
 (deftest clear-email-settings-test
   (testing "DELETE /api/email"
     (tu/discard-setting-changes [email-smtp-host email-smtp-port email-smtp-security email-smtp-username
-                                 email-smtp-password email-from-address]
+                                 email-smtp-password email-from-address email-from-name email-reply-to]
       (with-redefs [email/test-smtp-settings (constantly {::email/error nil})]
         (is (= (-> default-email-settings
                    (assoc :with-corrections {})
@@ -97,11 +104,13 @@
         (let [new-email-settings (email-settings)]
           (is (nil? (mt/user-http-request :crowberto :delete 204 "email")))
           (is (= default-email-settings
-                 new-email-settings))
+                new-email-settings))
           (is (= {:email-smtp-host     nil
                   :email-smtp-port     nil
                   :email-smtp-security :none
                   :email-smtp-username nil
                   :email-smtp-password nil
-                  :email-from-address  "notifications@metabase.com"}
+                  :email-from-address  "notifications@metabase.com"
+                  :email-from-name     nil
+                  :email-reply-to      nil}
                  (email-settings))))))))

test/metabase/email_test.clj

@@ -209,25 +209,44 @@
 
 (deftest send-message!-test
   (tu/with-temporary-setting-values [email-from-address "lucky@metabase.com"
+                                     email-from-name    "Lucky"
                                      email-smtp-host    "smtp.metabase.com"
                                      email-smtp-username "lucky"
                                      email-smtp-password "d1nner3scapee!"
                                      email-smtp-port     1025
+                                     email-reply-to      ["reply-to-me@metabase.com" "reply-to-me-too@metabase.com"]
                                      email-smtp-security :none]
     (testing "basic sending"
       (is (=
-           [{:from    (email/email-from-address)
-             :to      ["test@test.com"]
-             :subject "101 Reasons to use Metabase"
-             :body    [{:type    "text/html; charset=utf-8"
-                        :content "101. Metabase will make you a better person"}]}]
+           [{:from     (str (email/email-from-name) " <" (email/email-from-address) ">")
+             :to       ["test@test.com"]
+             :subject  "101 Reasons to use Metabase"
+             :reply-to (email/email-reply-to)
+             :body     [{:type    "text/html; charset=utf-8"
+                         :content "101. Metabase will make you a better person"}]}]
            (with-fake-inbox
              (email/send-message!
-               :subject      "101 Reasons to use Metabase"
-               :recipients   ["test@test.com"]
-               :message-type :html
-               :message      "101. Metabase will make you a better person")
+              :subject      "101 Reasons to use Metabase"
+              :recipients   ["test@test.com"]
+              :message-type :html
+              :message      "101. Metabase will make you a better person")
              (@inbox "test@test.com")))))
+    (testing "basic sending without email-from-name"
+      (tu/with-temporary-setting-values [email-from-name nil]
+        (is (=
+             [{:from     (email/email-from-address)
+               :to       ["test@test.com"]
+               :subject  "101 Reasons to use Metabase"
+               :reply-to (email/email-reply-to)
+               :body     [{:type    "text/html; charset=utf-8"
+                           :content "101. Metabase will make you a better person"}]}]
+             (with-fake-inbox
+               (email/send-message!
+                :subject      "101 Reasons to use Metabase"
+                :recipients   ["test@test.com"]
+                :message-type :html
+                :message      "101. Metabase will make you a better person")
+               (@inbox "test@test.com"))))))
     (testing "with an attachment"
       (let [recipient    "csv_user@example.com"
             csv-contents "hugs_with_metabase,hugs_without_metabase\n1,0"
@@ -235,25 +254,26 @@
             params       {:subject      "101 Reasons to use Metabase"
                           :recipients   [recipient]
                           :message-type :attachments
-                          :message      [{:type "text/html; charset=utf-8"
+                          :message      [{:type    "text/html; charset=utf-8"
                                           :content "100. Metabase will hug you when you're sad"}
-                                         {:type :attachment
+                                         {:type         :attachment
                                           :content-type "text/csv"
-                                          :file-name "metabase-reasons.csv"
-                                          :content csv-file
-                                          :description "very scientific data"}]}]
+                                          :file-name    "metabase-reasons.csv"
+                                          :content      csv-file
+                                          :description  "very scientific data"}]}]
         (testing "it sends successfully"
           (is (=
-               [{:from    (email/email-from-address)
-                 :to      [recipient]
-                 :subject "101 Reasons to use Metabase"
-                 :body    [{:type    "text/html; charset=utf-8"
-                            :content "100. Metabase will hug you when you're sad"}
-                           {:type         :attachment
-                            :content-type "text/csv"
-                            :file-name    "metabase-reasons.csv"
-                            :content      csv-file
-                            :description  "very scientific data"}]}]
+               [{:from     (str (email/email-from-name) " <" (email/email-from-address) ">")
+                 :to       [recipient]
+                 :subject  "101 Reasons to use Metabase"
+                 :reply-to (email/email-reply-to)
+                 :body     [{:type    "text/html; charset=utf-8"
+                             :content "100. Metabase will hug you when you're sad"}
+                            {:type         :attachment
+                             :content-type "text/csv"
+                             :file-name    "metabase-reasons.csv"
+                             :content      csv-file
+                             :description  "very scientific data"}]}]
                (with-fake-inbox
                  (m/mapply email/send-message! params)
                  (@inbox recipient)))))