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

@@ -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 {
+func (a *App) sendOutOfChannelMentions(sender *model.User, post *model.Post, outOfChannelUsers, outOfGroupsUsers []*model.User) *model.AppError {
-	if len(users) == 0 {
+	if len(outOfChannelUsers) == 0 && len(outOfGroupsUsers) == 0 {
 		return nil
 	}
 
-	var usernames []string
+	allUsers := model.UserSlice(append(outOfChannelUsers, outOfGroupsUsers...))
-	for _, user := range users {
-		usernames = append(usernames, user.Username)
-	}
-	sort.Strings(usernames)
 
-	var userIds []string
+	ocUsers := model.UserSlice(outOfChannelUsers)
-	for _, user := range users {
+	ocUsernames := ocUsers.Usernames()
-		userIds = append(userIds, user.Id)
+	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], ", @"),
+			"Usernames":    strings.Join(preliminary, ", @"),
-			"LastUsername": usernames[len(usernames)-1],
+			"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,
+			"post_id": ephemeralPostId,
-			"usernames": usernames,
+
-			"user_ids":  userIds,
+			"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

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)
+	mentions, err2 := th.App.SendNotifications(post1, th.BasicTeam, th.BasicChannel, th.BasicUser, nil)
-	if err != nil {
+	if err2 != nil {
-		t.Fatal(err)
+		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)
+	_, err2 = th.App.SendNotifications(post2, th.BasicTeam, dm, th.BasicUser, nil)
-	if err != nil {
+	if err2 != nil {
-		t.Fatal(err)
+		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)
+	_, err2 = th.App.SendNotifications(post3, th.BasicTeam, dm, th.BasicUser, nil)
-	if err != nil {
+	if err2 != nil {
-		t.Fatal(err)
+		t.Fatal(err2)
 	}
 
 	th.BasicChannel.DeleteAt = 1
-	mentions, err = th.App.SendNotifications(post1, th.BasicTeam, th.BasicChannel, th.BasicUser, nil)
+	mentions, err2 = th.App.SendNotifications(post1, th.BasicTeam, th.BasicChannel, th.BasicUser, nil)
-	assert.Nil(t, err)
+	assert.Nil(t, err2)
 	assert.Len(t, mentions, 0)
 }
 

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)

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",

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 {