Skip to content

Commit

Permalink
support file attachments for chat notifications (#700)
Browse files Browse the repository at this point in the history 
* always sync reactions (when chat/channel sync enabled)

* always sync file attachments (when chat/channel sync enabled)

* consolidate to experimentalSyncChats

* make selective sync experimental

* clarify syncNotifications

* s/DisableSyncMsg/UseSharedChannels/ and experimental

* make certificates experimental

* remove untested enabledTeams setting

* plugin.json: whitespace

* make disableCheckCredentials internal

* avoid directly mutating p.configuration in tests

* support file attachments for chat notifications

Fixes: https://mattermost.atlassian.net/browse/MM-58485

* restore missing `user_user_icon`

* skip test timing out

* MM-58851: fix merge issue re: disable vs dismiss

* restore message lost in merge as well
  • Loading branch information
@lieut-data
lieut-data authored Jun 19, 2024
1 parent 2be1d02 commit 0924146
Show file tree
Hide file tree
Showing 9 changed files with 114 additions and 293 deletions.
17 changes: 9 additions & 8 deletions server/attachments.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func (ah *ActivityHandler) ProcessAndUploadFileToMM(attachmentData []byte, attac
return fileInfo.Id, false
}

func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg *clientmodels.Message, chat *clientmodels.Chat, handleFileAttachments bool, existingFileIDs []string) (string, model.StringArray, string, bool, bool) {
func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg *clientmodels.Message, chat *clientmodels.Chat, existingFileIDs []string) (string, model.StringArray, string, int, bool) {
attachments := []string{}
newText := text
parentID := ""
Expand All @@ -115,7 +115,7 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg
errorFound := false
if client == nil {
ah.plugin.GetAPI().LogWarn("Unable to get the client")
return "", nil, "", false, errorFound
return "", nil, "", 0, errorFound
}

isDirectOrGroupMessage := false
Expand All @@ -133,7 +133,7 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg
}
}

skippedFileAttachments := false
skippedFileAttachments := 0
for _, a := range msg.Attachments {
// remove the attachment tags from the text
newText = attachRE.ReplaceAllString(newText, "")
Expand All @@ -152,11 +152,6 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg
continue
}

if !handleFileAttachments {
skippedFileAttachments = true
continue
}

fileInfoID := fileNames[a.Name]
if fileInfoID != "" {
attachments = append(attachments, fileInfoID)
Expand All @@ -173,13 +168,15 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg
if err != nil {
ah.plugin.GetAPI().LogWarn("failed to download the file", "filename", a.Name, "error", err.Error())
ah.plugin.GetMetrics().ObserveFile(metrics.ActionCreated, metrics.ActionSourceMSTeams, metrics.DiscardedReasonUnableToGetTeamsData, isDirectOrGroupMessage)
skippedFileAttachments++
continue
}
} else {
fileSize, downloadURL, err = client.GetFileSizeAndDownloadURL(a.ContentURL)
if err != nil {
ah.plugin.GetAPI().LogWarn("failed to get file size and download URL", "error", err.Error())
ah.plugin.GetMetrics().ObserveFile(metrics.ActionCreated, metrics.ActionSourceMSTeams, metrics.DiscardedReasonUnableToGetTeamsData, isDirectOrGroupMessage)
skippedFileAttachments++
continue
}

Expand All @@ -188,6 +185,7 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg
ah.plugin.GetAPI().LogWarn("skipping file download from MS Teams because the file size is greater than the allowed size")
errorFound = true
ah.plugin.GetMetrics().ObserveFile(metrics.ActionCreated, metrics.ActionSourceMSTeams, metrics.DiscardedReasonMaxFileSizeExceeded, isDirectOrGroupMessage)
skippedFileAttachments++
continue
}

Expand All @@ -197,6 +195,7 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg
if err != nil {
ah.plugin.GetAPI().LogWarn("failed to get file content", "error", err.Error())
ah.plugin.GetMetrics().ObserveFile(metrics.ActionCreated, metrics.ActionSourceMSTeams, metrics.DiscardedReasonUnableToGetTeamsData, isDirectOrGroupMessage)
skippedFileAttachments++
continue
}
}
Expand All @@ -217,6 +216,7 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg

if fileInfoID == "" {
ah.plugin.GetMetrics().ObserveFile(metrics.ActionCreated, metrics.ActionSourceMSTeams, metrics.DiscardedReasonEmptyFileID, isDirectOrGroupMessage)
skippedFileAttachments++
continue
}
attachments = append(attachments, fileInfoID)
Expand All @@ -226,6 +226,7 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg
// Calculate the count of file attachments discarded by subtracting handled file attachments and other attachments from total message attachments.
fileAttachmentsDiscarded := len(msg.Attachments) - countNonFileAttachments - countFileAttachments
ah.plugin.GetMetrics().ObserveFiles(metrics.ActionCreated, metrics.ActionSourceMSTeams, metrics.DiscardedReasonFileLimitReached, isDirectOrGroupMessage, int64(fileAttachmentsDiscarded))
skippedFileAttachments += fileAttachmentsDiscarded
break
}
}
Expand Down
49 changes: 10 additions & 39 deletions server/attachments_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,36 +221,13 @@ func TestHandleAttachments(t *testing.T) {
setupMetrics func(*mocksMetrics.Metrics)
setupStore func(store *storemocks.Store)
attachments []clientmodels.Attachment
handleFileAttachments bool
expectedText string
expectedAttachmentIDsCount int
expectedParentID string
expectedSkippedFileAttachments bool
expectedSkippedFileAttachments int
expectedError bool
fileIDs []string
}{
{
description: "File attachments disabled by configuration",
setupAPI: func(mockAPI *plugintest.API) {
mockAPI.On("UploadFile", []byte{}, testutils.GetChannelID(), "mock-name").Return(&model.FileInfo{
Id: testutils.GetID(),
}, nil)
},
setupClient: func(client *clientmocks.Client) {
},
setupMetrics: func(mockmetrics *mocksMetrics.Metrics) {
},
setupStore: func(store *storemocks.Store) {},
attachments: []clientmodels.Attachment{
{
Name: "mock-name",
},
},
handleFileAttachments: false,
expectedText: "mock-text",
expectedSkippedFileAttachments: true,
expectedAttachmentIDsCount: 0,
},
{
description: "Successfully handled attachments",
setupAPI: func(mockAPI *plugintest.API) {
Expand All @@ -271,7 +248,6 @@ func TestHandleAttachments(t *testing.T) {
Name: "mock-name",
},
},
handleFileAttachments: true,
expectedText: "mock-text",
expectedAttachmentIDsCount: 1,
},
Expand All @@ -293,8 +269,8 @@ func TestHandleAttachments(t *testing.T) {
Name: "mock-name",
},
},
handleFileAttachments: true,
expectedText: "mock-text",
expectedText: "mock-text",
expectedSkippedFileAttachments: 1,
},
{
description: "Number of attachments are greater than 10",
Expand All @@ -313,9 +289,9 @@ func TestHandleAttachments(t *testing.T) {
attachments: []clientmodels.Attachment{
{}, {}, {}, {}, {}, {}, {}, {}, {}, {}, {}, {},
},
handleFileAttachments: true,
expectedText: "mock-text",
expectedAttachmentIDsCount: 10,
expectedText: "mock-text",
expectedAttachmentIDsCount: 10,
expectedSkippedFileAttachments: 2,
},
{
description: "Attachment with existing fileID",
Expand All @@ -331,7 +307,6 @@ func TestHandleAttachments(t *testing.T) {
},
},
expectedAttachmentIDsCount: 1,
handleFileAttachments: true,
expectedText: "mock-text",
fileIDs: []string{"testFileId"},
},
Expand All @@ -345,7 +320,6 @@ func TestHandleAttachments(t *testing.T) {
setupStore: func(store *storemocks.Store) {},
attachments: []clientmodels.Attachment{},
expectedAttachmentIDsCount: 0,
handleFileAttachments: true,
expectedText: "mock-text",
fileIDs: []string{"testFileId"},
},
Expand Down Expand Up @@ -373,7 +347,6 @@ func TestHandleAttachments(t *testing.T) {
Name: "mockFile.Name.txt",
},
},
handleFileAttachments: true,
expectedText: "mock-text",
expectedAttachmentIDsCount: 2,
fileIDs: []string{"testFileId"},
Expand All @@ -395,8 +368,7 @@ func TestHandleAttachments(t *testing.T) {
Content: `{"language": "go", "codeSnippetUrl": "https://example.com/version/chats/mock-chat-id/messages/mock-message-id/hostedContents/mock-content-id/$value"}`,
},
},
handleFileAttachments: true,
expectedText: "mock-text\n```go\nsnippet content\n```\n",
expectedText: "mock-text\n```go\nsnippet content\n```\n",
},
{
description: "Attachment type message reference",
Expand All @@ -418,9 +390,8 @@ func TestHandleAttachments(t *testing.T) {
ContentType: "messageReference",
Content: `{"messageId":"mock-ID"}`,
}},
handleFileAttachments: true,
expectedText: "mock-text",
expectedParentID: testutils.GetID(),
expectedText: "mock-text",
expectedParentID: testutils.GetID(),
},
} {
t.Run(testCase.description, func(t *testing.T) {
Expand All @@ -441,7 +412,7 @@ func TestHandleAttachments(t *testing.T) {
ChannelID: testutils.GetChannelID(),
}

newText, attachmentIDs, parentID, skippedFileAttachments, errorsFound := ah.handleAttachments(testutils.GetChannelID(), testutils.GetUserID(), "mock-text", attachments, nil, testCase.handleFileAttachments, testCase.fileIDs)
newText, attachmentIDs, parentID, skippedFileAttachments, errorsFound := ah.handleAttachments(testutils.GetChannelID(), testutils.GetUserID(), "mock-text", attachments, nil, testCase.fileIDs)
assert.Equal(t, testCase.expectedParentID, parentID)
assert.Equal(t, testCase.expectedAttachmentIDsCount, len(attachmentIDs))
assert.Equal(t, testCase.expectedText, newText)
Expand Down
55 changes: 29 additions & 26 deletions server/bot_messages.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,62 +137,65 @@ func (p *Plugin) makeWelcomeMessageWithNotificationActionPost() *model.Post {
}

// formatNotificationMessage formats the message about a notification of a chat received on Teams.
func formatNotificationMessage(actorDisplayName string, chatTopic string, chatSize int, chatLink string, message string, attachmentCount int) string {
func formatNotificationMessage(actorDisplayName string, chatTopic string, chatSize int, chatLink string, message string, attachmentCount int, skippedFileAttachments int) string {
message = strings.TrimSpace(message)
if message == "" && attachmentCount == 0 {
if message == "" && attachmentCount == 0 && skippedFileAttachments == 0 {
return ""
}

var preamble string
var messageComponents []string

var chatTopicDesc string
if chatTopic != "" {
chatTopicDesc = ": " + chatTopic
}

// Handle the preamble
if chatSize <= 1 {
return ""
} else if chatSize == 2 {
preamble = fmt.Sprintf("**%s** messaged you in an [MS Teams chat%s](%s):", actorDisplayName, chatTopicDesc, chatLink)
messageComponents = append(messageComponents,
fmt.Sprintf("**%s** messaged you in an [MS Teams chat%s](%s):", actorDisplayName, chatTopicDesc, chatLink),
)
} else if chatSize == 3 {
preamble = fmt.Sprintf("**%s** messaged you and 1 other user in an [MS Teams group chat%s](%s):", actorDisplayName, chatTopicDesc, chatLink)
messageComponents = append(messageComponents,
fmt.Sprintf("**%s** messaged you and 1 other user in an [MS Teams group chat%s](%s):", actorDisplayName, chatTopicDesc, chatLink),
)
} else {
preamble = fmt.Sprintf("**%s** messaged you and %d other users in an [MS Teams group chat%s](%s):", actorDisplayName, chatSize-2, chatTopicDesc, chatLink)
messageComponents = append(messageComponents,
fmt.Sprintf("**%s** messaged you and %d other users in an [MS Teams group chat%s](%s):", actorDisplayName, chatSize-2, chatTopicDesc, chatLink),
)
}
preamble += "\n"

if message != "" {
message = "> " + strings.ReplaceAll(message, "\n", "\n> ")
// Handle the message itself
if len(message) > 0 {
messageComponents = append(messageComponents,
fmt.Sprintf("> %s", strings.ReplaceAll(message, "\n", "\n> ")),
)
}

attachmentsNotice := ""
if attachmentCount > 0 {
if len(message) > 0 {
attachmentsNotice += "\n"
}
attachmentsNotice += "\n*"
if attachmentCount == 1 {
attachmentsNotice += "This message was originally sent with one attachment."
} else {
attachmentsNotice += fmt.Sprintf("This message was originally sent with %d attachments.", attachmentCount)
}
attachmentsNotice += "*"
if skippedFileAttachments > 0 {
messageComponents = append(messageComponents,
"\n*Some file attachments from this message could not be delivered.*",
)
}

formattedMessage := fmt.Sprintf(`%s%s%s`, preamble, message, attachmentsNotice)
formattedMessage := strings.Join(messageComponents, "\n")

return formattedMessage
}

// notifyMessage sends the given receipient a notification of a chat received on Teams.
func (p *Plugin) notifyChat(recipientUserID string, actorDisplayName string, chatTopic string, chatSize int, chatLink string, message string, attachmentCount int) {
formattedMessage := formatNotificationMessage(actorDisplayName, chatTopic, chatSize, chatLink, message, attachmentCount)

func (p *Plugin) notifyChat(recipientUserID string, actorDisplayName string, chatTopic string, chatSize int, chatLink string, message string, fileIds model.StringArray, skippedFileAttachments int) {
formattedMessage := formatNotificationMessage(actorDisplayName, chatTopic, chatSize, chatLink, message, len(fileIds), skippedFileAttachments)
if formattedMessage == "" {
return
}

if err := p.botSendDirectMessage(recipientUserID, formattedMessage); err != nil {
if err := p.botSendDirectPost(recipientUserID, &model.Post{
Message: formattedMessage,
FileIds: fileIds,
}); err != nil {
p.GetAPI().LogWarn("Failed to send notification message", "user_id", recipientUserID, "error", err)
}
}
2 changes: 2 additions & 0 deletions server/ce2e/plugin_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ func TestMessageHasBeenPostedNewMessageE2E(t *testing.T) {
}

func TestMessageHasBeenPostedNewMessageWithBotAccountE2E(t *testing.T) {
t.Skip("https://mattermost.atlassian.net/browse/MM-58801")

mattermost, store, mockClient, tearDown := containere2e.NewE2ETestPlugin(t)
defer tearDown()

Expand Down
4 changes: 2 additions & 2 deletions server/converters.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (

const hostedContentsStr = "hostedContents"

func (ah *ActivityHandler) msgToPost(channelID, senderID string, msg *clientmodels.Message, chat *clientmodels.Chat, handleFileAttachments bool, existingFileIDs []string) (*model.Post, bool, bool) {
func (ah *ActivityHandler) msgToPost(channelID, senderID string, msg *clientmodels.Message, chat *clientmodels.Chat, existingFileIDs []string) (*model.Post, int, bool) {
text := ah.handleMentions(msg)
text = ah.handleEmojis(text)
var embeddedImages []clientmodels.Attachment
Expand All @@ -30,7 +30,7 @@ func (ah *ActivityHandler) msgToPost(channelID, senderID string, msg *clientmode
}
}

newText, attachments, parentID, skippedFileAttachments, errorFound := ah.handleAttachments(channelID, senderID, text, msg, chat, handleFileAttachments, existingFileIDs)
newText, attachments, parentID, skippedFileAttachments, errorFound := ah.handleAttachments(channelID, senderID, text, msg, chat, existingFileIDs)
text = newText

if parentID != "" {
Expand Down
2 changes: 1 addition & 1 deletion server/converters_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func TestMsgToPost(t *testing.T) {

ah.plugin = p

post, _, _ := ah.msgToPost(testCase.channelID, testCase.senderID, testCase.message, nil, true, []string{})
post, _, _ := ah.msgToPost(testCase.channelID, testCase.senderID, testCase.message, nil, []string{})
assert.Equal(t, testCase.post, post)
})
}
Expand Down
33 changes: 7 additions & 26 deletions server/handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ func (ah *ActivityHandler) handleCreatedActivity(msg *clientmodels.Message, subs
return metrics.DiscardedReasonOther
}

post, skippedFileAttachments, errorFound := ah.msgToPost(channelID, senderID, msg, chat, true, []string{})
post, skippedFileAttachments, errorFound := ah.msgToPost(channelID, senderID, msg, chat, []string{})

// Last second check to avoid possible duplication
if postInfo, _ = ah.plugin.GetStore().GetPostInfoByMSTeamsID(msg.ChatID+msg.ChannelID, msg.ID); postInfo != nil {
Expand All @@ -345,17 +345,8 @@ func (ah *ActivityHandler) handleCreatedActivity(msg *clientmodels.Message, subs
return metrics.DiscardedReasonInternalError
}

if skippedFileAttachments {
_, appErr := ah.plugin.GetAPI().CreatePost(&model.Post{
ChannelId: newPost.ChannelId,
UserId: ah.plugin.GetBotUserID(),
Message: "Attachments sent from Microsoft Teams aren't delivered to Mattermost.",
// Anchor the post immediately after (never before) the post that was created.
CreateAt: newPost.CreateAt + 1,
})
if appErr != nil {
ah.plugin.GetAPI().LogWarn("Failed to notify channel of skipped attachment", "channel_id", post.ChannelId, "post_id", newPost.Id, "error", appErr)
}
if skippedFileAttachments > 0 {
ah.plugin.GetAPI().LogWarn("Skipped file attachments on post update", "channel_id", post.ChannelId, "post_id", post.Id, "skipped_file_attachments", skippedFileAttachments)
}

ah.plugin.GetMetrics().ObserveMessage(metrics.ActionCreated, metrics.ActionSourceMSTeams, isDirectOrGroupMessage)
Expand Down Expand Up @@ -481,22 +472,12 @@ func (ah *ActivityHandler) handleUpdatedActivity(msg *clientmodels.Message, subs
return metrics.DiscardedReasonInactiveUser
}

post, _, _ := ah.msgToPost(channelID, senderID, msg, chat, true, fileIDs)
post, skippedFileAttachments, _ := ah.msgToPost(channelID, senderID, msg, chat, fileIDs)
post.Id = postInfo.MattermostID

// For now, don't display this message on update
// if skippedFileAttachments {
// _, appErr := ah.plugin.GetAPI().CreatePost(&model.Post{
// ChannelId: post.ChannelId,
// UserId: ah.plugin.GetBotUserID(),
// Message: "Attachments added to an existing post in Microsoft Teams aren't delivered to Mattermost.",
// // Anchor the post immediately after (never before) the post that was edited.
// CreateAt: post.CreateAt + 1,
// })
// if appErr != nil {
// ah.plugin.GetAPI().LogWarn("Failed to notify channel of skipped attachment", "channel_id", post.ChannelId, "post_id", post.Id, "error", appErr)
// }
// }
if skippedFileAttachments > 0 {
ah.plugin.GetAPI().LogWarn("Skipped file attachments on post update", "channel_id", post.ChannelId, "post_id", post.Id, "skipped_file_attachments", skippedFileAttachments)
}

ah.IgnorePluginHooksMap.Store(fmt.Sprintf("post_%s", post.Id), true)
if _, appErr := ah.plugin.GetAPI().UpdatePost(post); appErr != nil {
Expand Down
Loading

0 comments on commit 0924146

Please sign in to comment.