Simplifiying mail service API (#13036)

app/email.go

@@ -7,7 +7,6 @@ import (
 	"bytes"
 	"fmt"
 	"io"
-	"net/mail"
 	"net/url"
 	"path"
 	"strings"
@@ -525,8 +524,6 @@ func (a *App) SendMail(to, subject, htmlBody string) *model.AppError {
 func (a *App) SendMailWithEmbeddedFiles(to, subject, htmlBody string, embeddedFiles map[string]io.Reader) *model.AppError {
 	license := a.License()
 	config := a.Config()
-	fromMail := mail.Address{Name: *config.EmailSettings.FeedbackName, Address: *config.EmailSettings.FeedbackEmail}
-	replyTo := mail.Address{Name: *config.EmailSettings.FeedbackName, Address: *config.EmailSettings.ReplyToAddress}
 
-	return mailservice.SendMailUsingConfigAdvanced(to, to, fromMail, replyTo, subject, htmlBody, nil, embeddedFiles, nil, config, license != nil && *license.Features.Compliance)
+	return mailservice.SendMailWithEmbeddedFilesUsingConfig(to, subject, htmlBody, embeddedFiles, config, license != nil && *license.Features.Compliance)
 }

services/mailservice/mail.go

@@ -24,6 +24,18 @@ import (
 	"github.com/mattermost/mattermost-server/utils"
 )
 
+type mailData struct {
+	mimeTo        string
+	smtpTo        string
+	from          mail.Address
+	replyTo       mail.Address
+	subject       string
+	htmlBody      string
+	attachments   []*model.FileInfo
+	embeddedFiles map[string]io.Reader
+	mimeHeaders   map[string]string
+}
+
 // smtpClient is implemented by an smtp.Client. See https://golang.org/pkg/net/smtp/#Client.
 //
 type smtpClient interface {
@@ -204,15 +216,29 @@ func TestConnection(config *model.Config) {
 	defer c.Close()
 }
 
-func SendMailUsingConfig(to, subject, htmlBody string, config *model.Config, enableComplianceFeatures bool) *model.AppError {
+func SendMailWithEmbeddedFilesUsingConfig(to, subject, htmlBody string, embeddedFiles map[string]io.Reader, config *model.Config, enableComplianceFeatures bool) *model.AppError {
 	fromMail := mail.Address{Name: *config.EmailSettings.FeedbackName, Address: *config.EmailSettings.FeedbackEmail}
 	replyTo := mail.Address{Name: *config.EmailSettings.FeedbackName, Address: *config.EmailSettings.ReplyToAddress}
 
-	return SendMailUsingConfigAdvanced(to, to, fromMail, replyTo, subject, htmlBody, nil, nil, nil, config, enableComplianceFeatures)
+	mail := mailData{
+		mimeTo:        to,
+		smtpTo:        to,
+		from:          fromMail,
+		replyTo:       replyTo,
+		subject:       subject,
+		htmlBody:      htmlBody,
+		embeddedFiles: embeddedFiles,
+	}
+
+	return sendMailUsingConfigAdvanced(mail, config, enableComplianceFeatures)
+}
+
+func SendMailUsingConfig(to, subject, htmlBody string, config *model.Config, enableComplianceFeatures bool) *model.AppError {
+	return SendMailWithEmbeddedFilesUsingConfig(to, subject, htmlBody, nil, config, enableComplianceFeatures)
 }
 
 // allows for sending an email with attachments and differing MIME/SMTP recipients
-func SendMailUsingConfigAdvanced(mimeTo, smtpTo string, from, replyTo mail.Address, subject, htmlBody string, attachments []*model.FileInfo, embeddedFiles map[string]io.Reader, mimeHeaders map[string]string, config *model.Config, enableComplianceFeatures bool) *model.AppError {
+func sendMailUsingConfigAdvanced(mail mailData, config *model.Config, enableComplianceFeatures bool) *model.AppError {
 	if len(*config.EmailSettings.SMTPServer) == 0 {
 		return nil
 	}
@@ -235,34 +261,34 @@ func SendMailUsingConfigAdvanced(mimeTo, smtpTo string, from, replyTo mail.Addre
 		return err
 	}
 
-	return SendMail(c, mimeTo, smtpTo, from, replyTo, subject, htmlBody, attachments, embeddedFiles, mimeHeaders, fileBackend, time.Now())
+	return SendMail(c, mail, fileBackend, time.Now())
 }
 
-func SendMail(c smtpClient, mimeTo, smtpTo string, from, replyTo mail.Address, subject, htmlBody string, attachments []*model.FileInfo, embeddedFiles map[string]io.Reader, mimeHeaders map[string]string, fileBackend filesstore.FileBackend, date time.Time) *model.AppError {
+func SendMail(c smtpClient, mail mailData, fileBackend filesstore.FileBackend, date time.Time) *model.AppError {
-	mlog.Debug("sending mail", mlog.String("to", smtpTo), mlog.String("subject", subject))
+	mlog.Debug("sending mail", mlog.String("to", mail.smtpTo), mlog.String("subject", mail.subject))
 
-	htmlMessage := "\r\n<html><body>" + htmlBody + "</body></html>"
+	htmlMessage := "\r\n<html><body>" + mail.htmlBody + "</body></html>"
 
-	txtBody, err := html2text.FromString(htmlBody)
+	txtBody, err := html2text.FromString(mail.htmlBody)
 	if err != nil {
 		mlog.Warn("Unable to convert html body to text", mlog.Err(err))
 		txtBody = ""
 	}
 
 	headers := map[string][]string{
-		"From":                      {from.String()},
+		"From":                      {mail.from.String()},
-		"To":                        {mimeTo},
+		"To":                        {mail.mimeTo},
-		"Subject":                   {encodeRFC2047Word(subject)},
+		"Subject":                   {encodeRFC2047Word(mail.subject)},
 		"Content-Transfer-Encoding": {"8bit"},
 		"Auto-Submitted":            {"auto-generated"},
 		"Precedence":                {"bulk"},
 	}
 
-	if len(replyTo.Address) > 0 {
+	if len(mail.replyTo.Address) > 0 {
-		headers["Reply-To"] = []string{replyTo.String()}
+		headers["Reply-To"] = []string{mail.replyTo.String()}
 	}
 
-	for k, v := range mimeHeaders {
+	for k, v := range mail.mimeHeaders {
 		headers[k] = []string{encodeRFC2047Word(v)}
 	}
 
@@ -272,11 +298,11 @@ func SendMail(c smtpClient, mimeTo, smtpTo string, from, replyTo mail.Address, s
 	m.SetBody("text/plain", txtBody)
 	m.AddAlternative("text/html", htmlMessage)
 
-	for name, reader := range embeddedFiles {
+	for name, reader := range mail.embeddedFiles {
 		m.EmbedReader(name, reader)
 	}
 
-	for _, fileInfo := range attachments {
+	for _, fileInfo := range mail.attachments {
 		bytes, err := fileBackend.ReadFile(fileInfo.Path)
 		if err != nil {
 			return err
@@ -290,11 +316,11 @@ func SendMail(c smtpClient, mimeTo, smtpTo string, from, replyTo mail.Address, s
 		}))
 	}
 
-	if err = c.Mail(from.Address); err != nil {
+	if err = c.Mail(mail.from.Address); err != nil {
 		return model.NewAppError("SendMail", "utils.mail.send_mail.from_address.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 
-	if err = c.Rcpt(smtpTo); err != nil {
+	if err = c.Rcpt(mail.smtpTo); err != nil {
 		return model.NewAppError("SendMail", "utils.mail.send_mail.to_address.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 

services/mailservice/mail_test.go

@@ -128,6 +128,50 @@ func TestSendMailUsingConfig(t *testing.T) {
 	}
 }
 
+func TestSendMailWithEmbeddedFilesUsingConfig(t *testing.T) {
+	utils.T = utils.GetUserTranslations("en")
+
+	fs, err := config.NewFileStore("config.json", false)
+	require.Nil(t, err)
+
+	cfg := fs.Get()
+
+	var emailTo = "test@example.com"
+	var emailSubject = "Testing this email"
+	var emailBody = "This is a test from autobot"
+
+	//Delete all the messages before check the sample email
+	DeleteMailBox(emailTo)
+
+	embeddedFiles := map[string]io.Reader{
+		"test1.png": bytes.NewReader([]byte("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")),
+		"test2.png": bytes.NewReader([]byte("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")),
+	}
+	err2 := SendMailWithEmbeddedFilesUsingConfig(emailTo, emailSubject, emailBody, embeddedFiles, cfg, true)
+	require.Nil(t, err2, "Should connect to the SMTP Server")
+
+	//Check if the email was send to the right email address
+	var resultsMailbox JSONMessageHeaderInbucket
+	err3 := RetryInbucket(5, func() error {
+		var err error
+		resultsMailbox, err = GetMailBox(emailTo)
+		return err
+	})
+	if err3 != nil {
+		t.Log(err3)
+		t.Log("No email was received, maybe due load on the server. Skipping this verification")
+	} else {
+		if len(resultsMailbox) > 0 {
+			require.Contains(t, resultsMailbox[0].To[0], emailTo, "Wrong To: recipient")
+			resultsEmail, err := GetMessageFromMailbox(emailTo, resultsMailbox[0].ID)
+			require.Nil(t, err, "Could not get message from mailbox")
+			require.Contains(t, emailBody, resultsEmail.Body.Text, "Wrong received message %s", resultsEmail.Body.Text)
+			// Usign the message size because the inbucket API doesn't return embedded attachments through the API
+			require.Greater(t, resultsEmail.Size, 1500, "the file size should be more because the embedded attachemtns")
+		}
+	}
+}
+
 func TestSendMailUsingConfigAdvanced(t *testing.T) {
 	utils.T = utils.GetUserTranslations("en")
 
@@ -136,15 +180,8 @@ func TestSendMailUsingConfigAdvanced(t *testing.T) {
 
 	cfg := fs.Get()
 
-	var mimeTo = "test@example.com"
-	var smtpTo = "test2@example.com"
-	var from = mail.Address{Name: "Nobody", Address: "nobody@mattermost.com"}
-	var replyTo = mail.Address{Name: "ReplyTo", Address: "reply_to@mattermost.com"}
-	var emailSubject = "Testing this email"
-	var emailBody = "This is a test from autobot"
-
 	//Delete all the messages before check the sample email
-	DeleteMailBox(smtpTo)
+	DeleteMailBox("test2@example.com")
 
 	fileBackend, err := filesstore.NewFileBackend(&cfg.FileSettings, true)
 	assert.Nil(t, err)
@@ -178,31 +215,43 @@ func TestSendMailUsingConfigAdvanced(t *testing.T) {
 	headers := make(map[string]string)
 	headers["TestHeader"] = "TestValue"
 
-	err = SendMailUsingConfigAdvanced(mimeTo, smtpTo, from, replyTo, emailSubject, emailBody, attachments, embeddedFiles, headers, cfg, true)
+	mail := mailData{
+		mimeTo:        "test@example.com",
+		smtpTo:        "test2@example.com",
+		from:          mail.Address{Name: "Nobody", Address: "nobody@mattermost.com"},
+		replyTo:       mail.Address{Name: "ReplyTo", Address: "reply_to@mattermost.com"},
+		subject:       "Testing this email",
+		htmlBody:      "This is a test from autobot",
+		attachments:   attachments,
+		embeddedFiles: embeddedFiles,
+		mimeHeaders:   headers,
+	}
+
+	err = sendMailUsingConfigAdvanced(mail, cfg, true)
 	require.Nil(t, err, "Should connect to the STMP Server: %v", err)
 
 	//Check if the email was send to the right email address
 	var resultsMailbox JSONMessageHeaderInbucket
 	err = RetryInbucket(5, func() error {
 		var mailErr error
-		resultsMailbox, mailErr = GetMailBox(smtpTo)
+		resultsMailbox, mailErr = GetMailBox(mail.smtpTo)
 		return mailErr
 	})
-	require.Nil(t, err, "No emails found for address %s. error: %v", smtpTo, err)
+	require.Nil(t, err, "No emails found for address %s. error: %v", mail.smtpTo, err)
 	require.NotEqual(t, len(resultsMailbox), 0)
 
-	require.Contains(t, resultsMailbox[0].To[0], mimeTo, "Wrong To recipient")
+	require.Contains(t, resultsMailbox[0].To[0], mail.mimeTo, "Wrong To recipient")
 
-	resultsEmail, err := GetMessageFromMailbox(smtpTo, resultsMailbox[0].ID)
+	resultsEmail, err := GetMessageFromMailbox(mail.smtpTo, resultsMailbox[0].ID)
 	require.Nil(t, err)
 
-	require.Contains(t, emailBody, resultsEmail.Body.Text, "Wrong received message")
+	require.Contains(t, mail.htmlBody, resultsEmail.Body.Text, "Wrong received message")
 
 	// verify that the To header of the email message is set to the MIME recipient, even though we got it out of the SMTP recipient's email inbox
-	assert.Equal(t, mimeTo, resultsEmail.Header["To"][0])
+	assert.Equal(t, mail.mimeTo, resultsEmail.Header["To"][0])
 
 	// verify that the MIME from address is correct - unfortunately, we can't verify the SMTP from address
-	assert.Equal(t, from.String(), resultsEmail.Header["From"][0])
+	assert.Equal(t, mail.from.String(), resultsEmail.Header["From"][0])
 
 	// check that the custom mime headers came through - header case seems to get mutated
 	assert.Equal(t, "TestValue", resultsEmail.Header["Testheader"][0])
@@ -330,7 +379,8 @@ func TestSendMail(t *testing.T) {
 
 	for testName, tc := range testCases {
 		t.Run(testName, func(t *testing.T) {
-			appErr = SendMail(mocm, "", "", mail.Address{}, tc.replyTo, "", "", nil, nil, nil, mockBackend, time.Now())
+			mail := mailData{"", "", mail.Address{}, tc.replyTo, "", "", nil, nil, nil}
+			appErr = SendMail(mocm, mail, mockBackend, time.Now())
 			require.Nil(t, appErr)
 			if len(tc.contains) > 0 {
 				require.Contains(t, string(mocm.data), tc.contains)