diff --git a/enterprise/backend/test/metabase_enterprise/advanced_permissions/api/setting_test.clj b/enterprise/backend/test/metabase_enterprise/advanced_permissions/api/setting_test.clj
index 2f5cbbd18d5097bd2d2bd5277ec0756c75232bf6..69eb5807bf0b3278e33e52f8c51f554fdff6a46a 100644
--- a/enterprise/backend/test/metabase_enterprise/advanced_permissions/api/setting_test.clj
+++ b/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")))
diff --git a/frontend/src/metabase/admin/settings/components/SettingsBatchForm.jsx b/frontend/src/metabase/admin/settings/components/SettingsBatchForm.jsx
index b96230fbae82cea5a6e5c324868b9ec31b930965..8c41e55c5c629a489b9c4661dc91c4b46d8c95f8 100644
--- a/frontend/src/metabase/admin/settings/components/SettingsBatchForm.jsx
+++ b/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`,
diff --git a/frontend/src/metabase/admin/settings/components/SettingsEmailForm.jsx b/frontend/src/metabase/admin/settings/components/SettingsEmailForm.jsx
index 604dcade6b4904dfb8072850fc72f80e1ff68c8c..64e5394b0ca58d9e31d756cc77d673b48028a331 100644
--- a/frontend/src/metabase/admin/settings/components/SettingsEmailForm.jsx
+++ b/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 }) => (
diff --git a/frontend/src/metabase/admin/settings/components/widgets/SettingCommaDelimitedInput.jsx b/frontend/src/metabase/admin/settings/components/widgets/SettingCommaDelimitedInput.jsx
new file mode 100644
index 0000000000000000000000000000000000000000..a254db13a0b27a68036b1942b0e0c184c75d8d18
--- /dev/null
+++ b/frontend/src/metabase/admin/settings/components/widgets/SettingCommaDelimitedInput.jsx
@@ -0,0 +1,35 @@
+/* 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;
diff --git a/frontend/src/metabase/admin/settings/selectors.js b/frontend/src/metabase/admin/settings/selectors.js
index d4751a1940d7814c6cda63deca4a9b07163a523b..979c379438eb06671ac5eafd39475735305188c9 100644
--- a/frontend/src/metabase/admin/settings/selectors.js
+++ b/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: {
diff --git a/frontend/src/metabase/plugins/builtin/settings/hosted.js b/frontend/src/metabase/plugins/builtin/settings/hosted.js
index 609ab7a6f072116b6ee7f773b723aa7fad507a0a..39395fa310bc6b5becab63b4a9944a16e19df637 100644
--- a/frontend/src/metabase/plugins/builtin/settings/hosted.js
+++ b/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 =>
diff --git a/frontend/test/__support__/e2e/helpers/e2e-email-helpers.js b/frontend/test/__support__/e2e/helpers/e2e-email-helpers.js
index 1318c84d341b21236f431fdab296556e9e8c48a3..38f172328ce4e12ea8a131b07fac4248e81343c7 100644
--- a/frontend/test/__support__/e2e/helpers/e2e-email-helpers.js
+++ b/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
diff --git a/frontend/test/metabase/scenarios/admin/settings/email.cy.spec.js b/frontend/test/metabase/scenarios/admin/settings/email.cy.spec.js
index c06d77ce17067f0653c99aa64e3f99d82ca01f1c..fd68fae9745f2f54fa301ca47a4f4850517e0667 100644
--- a/frontend/test/metabase/scenarios/admin/settings/email.cy.spec.js
+++ b/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)", () => {
diff --git a/src/metabase/api/email.clj b/src/metabase/api/email.clj
index 3678f1fbf04a9054bdc0279ab82b76fac0fab0cc..55745dc22f2c623d0ab8763dcb4882d80111bf99 100644
--- a/src/metabase/api/email.clj
+++ b/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."
diff --git a/src/metabase/email.clj b/src/metabase/email.clj
index 807ccebd7c8fcdff288f992c8b9a2652ea509738..c91b8fd98f7769b15fc231027006b617de9cbaf8 100644
--- a/src/metabase/email.clj
+++ b/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
diff --git a/test/metabase/api/email_test.clj b/test/metabase/api/email_test.clj
index 369eb4dce47b05dcf56e24c302cfe8b5638886f0..d55c7a02a2a9be67bc7441addc3390cad030aa38 100644
--- a/test/metabase/api/email_test.clj
+++ b/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))))))))
diff --git a/test/metabase/email_test.clj b/test/metabase/email_test.clj
index 96f31ce183ef6853f51db45f1a4db175ef27cdf4..c9493e182e9a04df7340652c6035d27fd38f8935 100644
--- a/test/metabase/email_test.clj
+++ b/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)))))