From 84a59ddb39283441459cc51d44cc45b11e0ba76c Mon Sep 17 00:00:00 2001 From: Martin Kraft Date: Wed, 17 Apr 2019 10:44:45 -0400 Subject: [PATCH] MM-14417: Messaging for mentions of users who are not in associated channel groups. (#10594) * MM-14417: Adds support for out-of-channel notifications of users who are not in associated groups of group-constrained channels. * MM-14417: Fix for mobile backwards compatibility. --- app/notification.go | 90 ++++++++++++++++++++++++++++++---------- app/notification_test.go | 22 +++++----- app/post.go | 2 +- i18n/en.json | 12 +++++- model/user.go | 48 +++++++++++++++++++++ 5 files changed, 138 insertions(+), 36 deletions(-) diff --git a/app/notification.go b/app/notification.go index d7d8d94108..fce84f168d 100644 --- a/app/notification.go +++ b/app/notification.go @@ -22,7 +22,7 @@ const ( THREAD_ROOT = "root" ) -func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *model.Channel, sender *model.User, parentPostList *model.PostList) ([]string, *model.AppError) { +func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *model.Channel, sender *model.User, parentPostList *model.PostList) ([]string, error) { // Do not send notifications in archived channels if channel.DeleteAt > 0 { return []string{}, nil @@ -126,10 +126,19 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod if len(m.OtherPotentialMentions) > 0 && !post.IsSystemMessage() { if result := <-a.Srv.Store.User().GetProfilesByUsernames(m.OtherPotentialMentions, team.Id); result.Err == nil { - outOfChannelMentions := result.Data.([]*model.User) + channelMentions := model.UserSlice(result.Data.([]*model.User)) + + nonMemberIDs, err := a.FilterNonGroupChannelMembers(channelMentions.IDs(), channel) + if err != nil { + return nil, err + } + + outOfChannelMentions := channelMentions.FilterWithoutID(nonMemberIDs) + outOfGroupsMentions := channelMentions.FilterByID(nonMemberIDs) + if channel.Type != model.CHANNEL_GROUP { a.Srv.Go(func() { - a.sendOutOfChannelMentions(sender, post, outOfChannelMentions) + a.sendOutOfChannelMentions(sender, post, outOfChannelMentions, outOfGroupsMentions) }) } } @@ -355,42 +364,70 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod return mentionedUsersList, nil } -func (a *App) sendOutOfChannelMentions(sender *model.User, post *model.Post, users []*model.User) *model.AppError { - if len(users) == 0 { +func (a *App) sendOutOfChannelMentions(sender *model.User, post *model.Post, outOfChannelUsers, outOfGroupsUsers []*model.User) *model.AppError { + if len(outOfChannelUsers) == 0 && len(outOfGroupsUsers) == 0 { return nil } - var usernames []string - for _, user := range users { - usernames = append(usernames, user.Username) - } - sort.Strings(usernames) + allUsers := model.UserSlice(append(outOfChannelUsers, outOfGroupsUsers...)) - var userIds []string - for _, user := range users { - userIds = append(userIds, user.Id) - } + ocUsers := model.UserSlice(outOfChannelUsers) + ocUsernames := ocUsers.Usernames() + ocUserIDs := ocUsers.IDs() + + ogUsers := model.UserSlice(outOfGroupsUsers) + ogUsernames := ogUsers.Usernames() T := utils.GetUserTranslations(sender.Locale) ephemeralPostId := model.NewId() var message string - if len(users) == 1 { + if len(outOfChannelUsers) == 1 { message = T("api.post.check_for_out_of_channel_mentions.message.one", map[string]interface{}{ - "Username": usernames[0], + "Username": ocUsernames[0], }) - } else { + } else if len(outOfChannelUsers) > 1 { + preliminary, final := splitAtFinal(ocUsernames) + message = T("api.post.check_for_out_of_channel_mentions.message.multiple", map[string]interface{}{ - "Usernames": strings.Join(usernames[:len(usernames)-1], ", @"), - "LastUsername": usernames[len(usernames)-1], + "Usernames": strings.Join(preliminary, ", @"), + "LastUsername": final, + }) + } + + if len(outOfGroupsUsers) == 1 { + if len(message) > 0 { + message += "\n" + } + + message += T("api.post.check_for_out_of_channel_groups_mentions.message.one", map[string]interface{}{ + "Username": ogUsernames[0], + }) + } else if len(outOfGroupsUsers) > 1 { + preliminary, final := splitAtFinal(ogUsernames) + + if len(message) > 0 { + message += "\n" + } + + message += T("api.post.check_for_out_of_channel_groups_mentions.message.multiple", map[string]interface{}{ + "Usernames": strings.Join(preliminary, ", @"), + "LastUsername": final, }) } props := model.StringInterface{ model.PROPS_ADD_CHANNEL_MEMBER: model.StringInterface{ - "post_id": ephemeralPostId, - "usernames": usernames, - "user_ids": userIds, + "post_id": ephemeralPostId, + + "usernames": allUsers.Usernames(), // Kept for backwards compatibility of mobile app. + "not_in_channel_usernames": ocUsernames, + + "user_ids": allUsers.IDs(), // Kept for backwards compatibility of mobile app. + "not_in_channel_user_ids": ocUserIDs, + + "not_in_groups_usernames": ogUsernames, + "not_in_groups_user_ids": ogUsers.IDs(), }, } @@ -409,6 +446,15 @@ func (a *App) sendOutOfChannelMentions(sender *model.User, post *model.Post, use return nil } +func splitAtFinal(items []string) (preliminary []string, final string) { + if len(items) == 0 { + return + } + preliminary = items[:len(items)-1] + final = items[len(items)-1] + return +} + type ExplicitMentions struct { // MentionedUserIds contains a key for each user mentioned by keyword. MentionedUserIds map[string]bool diff --git a/app/notification_test.go b/app/notification_test.go index 2438112ad0..ac94734262 100644 --- a/app/notification_test.go +++ b/app/notification_test.go @@ -30,9 +30,9 @@ func TestSendNotifications(t *testing.T) { t.Fatal(err) } - mentions, err := th.App.SendNotifications(post1, th.BasicTeam, th.BasicChannel, th.BasicUser, nil) - if err != nil { - t.Fatal(err) + mentions, err2 := th.App.SendNotifications(post1, th.BasicTeam, th.BasicChannel, th.BasicUser, nil) + if err2 != nil { + t.Fatal(err2) } else if mentions == nil { t.Log(mentions) t.Fatal("user should have been mentioned") @@ -56,9 +56,9 @@ func TestSendNotifications(t *testing.T) { t.Fatal(err) } - _, err = th.App.SendNotifications(post2, th.BasicTeam, dm, th.BasicUser, nil) - if err != nil { - t.Fatal(err) + _, err2 = th.App.SendNotifications(post2, th.BasicTeam, dm, th.BasicUser, nil) + if err2 != nil { + t.Fatal(err2) } th.App.UpdateActive(th.BasicUser2, false) @@ -74,14 +74,14 @@ func TestSendNotifications(t *testing.T) { t.Fatal(err) } - _, err = th.App.SendNotifications(post3, th.BasicTeam, dm, th.BasicUser, nil) - if err != nil { - t.Fatal(err) + _, err2 = th.App.SendNotifications(post3, th.BasicTeam, dm, th.BasicUser, nil) + if err2 != nil { + t.Fatal(err2) } th.BasicChannel.DeleteAt = 1 - mentions, err = th.App.SendNotifications(post1, th.BasicTeam, th.BasicChannel, th.BasicUser, nil) - assert.Nil(t, err) + mentions, err2 = th.App.SendNotifications(post1, th.BasicTeam, th.BasicChannel, th.BasicUser, nil) + assert.Nil(t, err2) assert.Len(t, mentions, 0) } diff --git a/app/post.go b/app/post.go index 57ec3ee5fc..ac8cee5356 100644 --- a/app/post.go +++ b/app/post.go @@ -362,7 +362,7 @@ func (a *App) FillInPostProps(post *model.Post, channel *model.Channel) *model.A return nil } -func (a *App) handlePostEvents(post *model.Post, user *model.User, channel *model.Channel, triggerWebhooks bool, parentPostList *model.PostList) *model.AppError { +func (a *App) handlePostEvents(post *model.Post, user *model.User, channel *model.Channel, triggerWebhooks bool, parentPostList *model.PostList) error { var team *model.Team if len(channel.TeamId) > 0 { result := <-a.Srv.Store.Team().Get(channel.TeamId) diff --git a/i18n/en.json b/i18n/en.json index 79a66c174b..a09dd3ad04 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1440,13 +1440,21 @@ "id": "api.plugin.upload.no_file.app_error", "translation": "Missing file in multipart/form request" }, + { + "id": "api.post.check_for_out_of_channel_groups_mentions.message.multiple", + "translation": "@{{.Usernames}} and @{{.LastUsername}} did not get notified by this mention because they are not in the channel. They are also not a member of the groups linked to this channel." + }, + { + "id": "api.post.check_for_out_of_channel_groups_mentions.message.one", + "translation": "@{{.Username}} did not get notified by this mention because they are not in the channel. They are also not a member of the groups linked to this channel." + }, { "id": "api.post.check_for_out_of_channel_mentions.message.multiple", - "translation": "@{{.Usernames}} and @{{.LastUsername}} were mentioned, but they did not receive notifications because they do not belong to this channel." + "translation": "@{{.Usernames}} and @{{.LastUsername}} did not get notified by this mention because they are not in the channel." }, { "id": "api.post.check_for_out_of_channel_mentions.message.one", - "translation": "@{{.Username}} was mentioned, but they did not receive notifications because they do not belong to this channel." + "translation": "@{{.Username}} did not get notified by this mention because they are not in the channel." }, { "id": "api.post.create_post.can_not_post_to_deleted.error", diff --git a/model/user.go b/model/user.go index d8948eadbd..7c1cd146ee 100644 --- a/model/user.go +++ b/model/user.go @@ -9,6 +9,7 @@ import ( "io" "net/http" "regexp" + "sort" "strings" "unicode/utf8" @@ -117,6 +118,53 @@ type UserForIndexing struct { ChannelsIds []string `json:"channel_id"` } +type UserSlice []*User + +func (u UserSlice) Usernames() []string { + usernames := []string{} + for _, user := range u { + usernames = append(usernames, user.Username) + } + sort.Strings(usernames) + return usernames +} + +func (u UserSlice) IDs() []string { + ids := []string{} + for _, user := range u { + ids = append(ids, user.Id) + } + return ids +} + +func (u UserSlice) FilterByID(ids []string) UserSlice { + var matches []*User + for _, user := range u { + for _, id := range ids { + if id == user.Id { + matches = append(matches, user) + } + } + } + return UserSlice(matches) +} + +func (u UserSlice) FilterWithoutID(ids []string) UserSlice { + var keep []*User + for _, user := range u { + present := false + for _, id := range ids { + if id == user.Id { + present = true + } + } + if !present { + keep = append(keep, user) + } + } + return UserSlice(keep) +} + func (u *User) DeepCopy() *User { copyUser := *u if u.AuthData != nil {