From 0c64252c9f7c35b0545ff012dc3838d129e3aea3 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Mon, 5 Dec 2022 16:47:28 +0530 Subject: [PATCH] [MM-48521] Fix for batch notification email not rendering properly (#21736) * Add MessageAttachment field to postdata while rendering batched email notifications * Add test for SlackAttachments body generator * Add context to changes * Add license text to new test file * Use app/email/notification_email.go as single source for attachment generation --- app/email/email_batching.go | 2 + app/email/notification_email.go | 90 +++++++++++++++++++++++++++ app/email/notification_email_test.go | 72 ++++++++++++++++++++++ app/notification_email.go | 92 +--------------------------- 4 files changed, 167 insertions(+), 89 deletions(-) create mode 100644 app/email/notification_email_test.go diff --git a/app/email/email_batching.go b/app/email/email_batching.go index ff016c0314..c4b3b01b60 100644 --- a/app/email/email_batching.go +++ b/app/email/email_batching.go @@ -33,6 +33,7 @@ type postData struct { Time string ShowChannelIcon bool OtherChannelMembersCount int + MessageAttachments []*EmailMessageAttachment } func (es *Service) InitEmailBatching() { @@ -314,6 +315,7 @@ func (es *Service) sendBatchedEmailNotification(userID string, notifications []* MessageURL: MessageURL, ShowChannelIcon: showChannelIcon, OtherChannelMembersCount: otherChannelMembersCount, + MessageAttachments: ProcessMessageAttachments(notification.post), }) } } diff --git a/app/email/notification_email.go b/app/email/notification_email.go index 6f83b1191f..06c0364b38 100644 --- a/app/email/notification_email.go +++ b/app/email/notification_email.go @@ -4,6 +4,8 @@ package email import ( + "html" + "html/template" "net/url" "path/filepath" "strings" @@ -11,8 +13,21 @@ import ( "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/shared/i18n" "github.com/mattermost/mattermost-server/v6/shared/mlog" + "github.com/mattermost/mattermost-server/v6/utils" ) +type FieldRow struct { + Cells []*model.SlackAttachmentField +} + +type EmailMessageAttachment struct { + model.SlackAttachment + + Pretext template.HTML + Text template.HTML + FieldRows []FieldRow +} + func (es *Service) GetMessageForNotification(post *model.Post, translateFunc i18n.TranslateFunc) string { if strings.TrimSpace(post.Message) != "" || len(post.FileIds) == 0 { return post.Message @@ -44,3 +59,78 @@ func (es *Service) GetMessageForNotification(post *model.Post, translateFunc i18 } return translateFunc("api.post.get_message_for_notification.files_sent", len(filenames), props) } + +func ProcessMessageAttachments(post *model.Post) []*EmailMessageAttachment { + emailMessageAttachments := []*EmailMessageAttachment{} + + for _, messageAttachment := range post.Attachments() { + emailMessageAttachment := &EmailMessageAttachment{ + SlackAttachment: *messageAttachment, + Pretext: prepareTextForEmail(messageAttachment.Pretext), + Text: prepareTextForEmail(messageAttachment.Text), + } + + stripedTitle, err := utils.StripMarkdown(emailMessageAttachment.Title) + if err != nil { + mlog.Warn("Failed parse to markdown from messageatatchment title", mlog.String("post_id", post.Id), mlog.Err(err)) + stripedTitle = "" + } + + emailMessageAttachment.Title = stripedTitle + + shortFieldRow := FieldRow{} + + for i := range messageAttachment.Fields { + // Create a new instance to avoid altering the original pointer reference + // We update field value to parse markdown. + // If we do that on the original pointer, the rendered text in mattermost + // becomes invalid as its no longer a markdown string, but rather an HTML string. + field := &model.SlackAttachmentField{ + Title: messageAttachment.Fields[i].Title, + Value: messageAttachment.Fields[i].Value, + Short: messageAttachment.Fields[i].Short, + } + + if stringValue, ok := field.Value.(string); ok { + field.Value = prepareTextForEmail(stringValue) + } + + if !field.Short { + if len(shortFieldRow.Cells) > 0 { + emailMessageAttachment.FieldRows = append(emailMessageAttachment.FieldRows, shortFieldRow) + shortFieldRow = FieldRow{} + } + + emailMessageAttachment.FieldRows = append(emailMessageAttachment.FieldRows, FieldRow{[]*model.SlackAttachmentField{field}}) + } else { + shortFieldRow.Cells = append(shortFieldRow.Cells, field) + + if len(shortFieldRow.Cells) == 2 { + emailMessageAttachment.FieldRows = append(emailMessageAttachment.FieldRows, shortFieldRow) + shortFieldRow = FieldRow{} + } + } + } + + // collect any leftover short fields + if len(shortFieldRow.Cells) > 0 { + emailMessageAttachment.FieldRows = append(emailMessageAttachment.FieldRows, shortFieldRow) + shortFieldRow = FieldRow{} + } + + emailMessageAttachments = append(emailMessageAttachments, emailMessageAttachment) + } + + return emailMessageAttachments +} + +func prepareTextForEmail(text string) template.HTML { + escapedText := html.EscapeString(text) + markdownText, err := utils.MarkdownToHTML(escapedText) + if err != nil { + mlog.Warn("Encountered error while converting markdown to HTML", mlog.Err(err)) + return template.HTML(text) + } + + return template.HTML(markdownText) +} diff --git a/app/email/notification_email_test.go b/app/email/notification_email_test.go new file mode 100644 index 0000000000..4f8e176c5b --- /dev/null +++ b/app/email/notification_email_test.go @@ -0,0 +1,72 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package email + +import ( + "testing" + + "github.com/mattermost/mattermost-server/v6/model" + "github.com/stretchr/testify/require" +) + +func TestProcessMessageAttachments(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + post := &model.Post{ + Message: "This is the message", + } + + messageAttachments := []*model.SlackAttachment{ + { + Color: "#FF0000", + Pretext: "message attachment 1 pretext", + AuthorName: "author name", + AuthorLink: "https://example.com/slack_attachment_1/author_link", + AuthorIcon: "https://example.com/slack_attachment_1/author_icon", + Title: "message attachment 1 title", + TitleLink: "https://example.com/slack_attachment_1/title_link", + Text: "message attachment 1 text", + ImageURL: "https://example.com/slack_attachment_1/image", + ThumbURL: "https://example.com/slack_attachment_1/thumb", + Fields: []*model.SlackAttachmentField{ + { + Short: true, + Title: "message attachment 1 field 1 title", + Value: "message attachment 1 field 1 value", + }, + { + Short: false, + Title: "message attachment 1 field 2 title", + Value: "message attachment 1 field 2 value", + }, + { + Short: true, + Title: "message attachment 1 field 3 title", + Value: "message attachment 1 field 3 value", + }, + { + Short: true, + Title: "message attachment 1 field 4 title", + Value: "message attachment 1 field 4 value", + }, + }, + }, + { + Color: "#FF0000", + Pretext: "message attachment 2 pretext", + AuthorName: "author name 2", + Text: "message attachment 2 text", + }, + } + + model.ParseSlackAttachment(post, messageAttachments) + + processedAttachcmentsPost := ProcessMessageAttachments(post) + require.NotNil(t, processedAttachcmentsPost) + require.Len(t, processedAttachcmentsPost, 2) + require.Equal(t, processedAttachcmentsPost[0].Color, "#FF0000") + require.Equal(t, processedAttachcmentsPost[0].FieldRows[0].Cells[0].Title, "message attachment 1 field 1 title") + require.Equal(t, processedAttachcmentsPost[1].Color, "#FF0000") +} diff --git a/app/notification_email.go b/app/notification_email.go index 965dd9a292..a56180ea2f 100644 --- a/app/notification_email.go +++ b/app/notification_email.go @@ -12,6 +12,7 @@ import ( "strings" "time" + email "github.com/mattermost/mattermost-server/v6/app/email" "github.com/mattermost/mattermost-server/v6/app/request" "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/shared/i18n" @@ -201,18 +202,6 @@ func truncateUserNames(name string, i int) string { return name } -type FieldRow struct { - Cells []*model.SlackAttachmentField -} - -type EmailMessageAttachment struct { - model.SlackAttachment - - Pretext template.HTML - Text template.HTML - FieldRows []FieldRow -} - type postData struct { SenderName string ChannelName string @@ -223,7 +212,7 @@ type postData struct { Time string ShowChannelIcon bool OtherChannelMembersCount int - MessageAttachments []*EmailMessageAttachment + MessageAttachments []*email.EmailMessageAttachment } /** @@ -258,7 +247,7 @@ func (a *App) getNotificationEmailBody(c request.CTX, recipient *model.User, pos } pData.Message = template.HTML(normalizedPostMessage) pData.Time = translateFunc("app.notification.body.dm.time", messageTime) - pData.MessageAttachments = a.processMessageAttachments(post) + pData.MessageAttachments = email.ProcessMessageAttachments(post) } data := a.Srv().EmailService.NewEmailTemplateData(recipient.Locale) @@ -320,81 +309,6 @@ func (a *App) getNotificationEmailBody(c request.CTX, recipient *model.User, pos return a.Srv().TemplatesContainer().RenderToString("messages_notification", data) } -func (a *App) processMessageAttachments(post *model.Post) []*EmailMessageAttachment { - emailMessageAttachments := []*EmailMessageAttachment{} - - for _, messageAttachment := range post.Attachments() { - emailMessageAttachment := &EmailMessageAttachment{ - SlackAttachment: *messageAttachment, - Pretext: a.prepareTextForEmail(messageAttachment.Pretext), - Text: a.prepareTextForEmail(messageAttachment.Text), - } - - stripedTitle, err := utils.StripMarkdown(emailMessageAttachment.Title) - if err != nil { - mlog.Warn("Failed parse to markdown from messageatatchment title", mlog.String("post_id", post.Id), mlog.Err(err)) - stripedTitle = "" - } - - emailMessageAttachment.Title = stripedTitle - - shortFieldRow := FieldRow{} - - for i := range messageAttachment.Fields { - // Create a new instance to avoid altering the original pointer reference - // We update field value to parse markdown. - // If we do that on the original pointer, the rendered text in mattermost - // becomes invalid as its no longer a markdown string, but rather an HTML string. - field := &model.SlackAttachmentField{ - Title: messageAttachment.Fields[i].Title, - Value: messageAttachment.Fields[i].Value, - Short: messageAttachment.Fields[i].Short, - } - - if stringValue, ok := field.Value.(string); ok { - field.Value = a.prepareTextForEmail(stringValue) - } - - if !field.Short { - if len(shortFieldRow.Cells) > 0 { - emailMessageAttachment.FieldRows = append(emailMessageAttachment.FieldRows, shortFieldRow) - shortFieldRow = FieldRow{} - } - - emailMessageAttachment.FieldRows = append(emailMessageAttachment.FieldRows, FieldRow{[]*model.SlackAttachmentField{field}}) - } else { - shortFieldRow.Cells = append(shortFieldRow.Cells, field) - - if len(shortFieldRow.Cells) == 2 { - emailMessageAttachment.FieldRows = append(emailMessageAttachment.FieldRows, shortFieldRow) - shortFieldRow = FieldRow{} - } - } - } - - // collect any leftover short fields - if len(shortFieldRow.Cells) > 0 { - emailMessageAttachment.FieldRows = append(emailMessageAttachment.FieldRows, shortFieldRow) - shortFieldRow = FieldRow{} - } - - emailMessageAttachments = append(emailMessageAttachments, emailMessageAttachment) - } - - return emailMessageAttachments -} - -func (a *App) prepareTextForEmail(text string) template.HTML { - escapedText := html.EscapeString(text) - markdownText, err := utils.MarkdownToHTML(escapedText) - if err != nil { - mlog.Warn("Encountered error while converting markdown to HTML", mlog.Err(err)) - return template.HTML(text) - } - - return template.HTML(markdownText) -} - type formattedPostTime struct { Time time.Time Year string