MM-17071 Extract and add tests for handling of out of channel mentions (#11788)

* MM-17071 Extract and add tests for handling of out of channel mentions

* Rename checkForOutOfChannelMentions back to sendOutOfChannelMentions

* Log errors returned by sendOutOfChannelMentions

app/notification.go

@@ -140,33 +140,12 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod
 			delete(mentionedUserIds, post.UserId)
 		}
 
-		if len(m.OtherPotentialMentions) > 0 && !post.IsSystemMessage() {
+		go func() {
-			if users, err := a.Srv.Store.User().GetProfilesByUsernames(m.OtherPotentialMentions, &model.ViewUsersRestrictions{Teams: []string{team.Id}}); err == nil {
+			_, err := a.sendOutOfChannelMentions(sender, post, channel, m.OtherPotentialMentions)
-				channelMentions := model.UserSlice(users).FilterByActive(true)
+			if err != nil {
-
+				mlog.Error("Failed to send warning for out of channel mentions", mlog.String("user_id", sender.Id), mlog.String("post_id", post.Id), mlog.Err(err))
-				var outOfChannelMentions model.UserSlice
-				var outOfGroupsMentions model.UserSlice
-
-				if channel.IsGroupConstrained() {
-					nonMemberIDs, err := a.FilterNonGroupChannelMembers(channelMentions.IDs(), channel)
-					if err != nil {
-						return nil, err
-					}
-
-					outOfChannelMentions = channelMentions.FilterWithoutID(nonMemberIDs)
-					outOfGroupsMentions = channelMentions.FilterByID(nonMemberIDs)
-				} else {
-					outOfChannelMentions = channelMentions
-				}
-				outOfChannelMentions = outOfChannelMentions.FilterWithoutBots()
-
-				if channel.Type != model.CHANNEL_GROUP {
-					a.Srv.Go(func() {
-						a.sendOutOfChannelMentions(sender, post, outOfChannelMentions, outOfGroupsMentions)
-					})
-				}
 			}
-		}
+		}()
 
 		// find which users in the channel are set up to always receive mobile notifications
 		for _, profile := range profileMap {
@@ -411,11 +390,68 @@ 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, outOfChannelUsers, outOfGroupsUsers []*model.User) *model.AppError {
+// sendOutOfChannelMentions sends an ephemeral post to the sender of a post if any of the given potential mentions
-	if len(outOfChannelUsers) == 0 && len(outOfGroupsUsers) == 0 {
+// are outside of the post's channel. Returns whether or not an ephemeral post was sent.
-		return nil
+func (a *App) sendOutOfChannelMentions(sender *model.User, post *model.Post, channel *model.Channel, potentialMentions []string) (bool, error) {
+	outOfChannelUsers, outOfGroupsUsers, err := a.filterOutOfChannelMentions(sender, post, channel, potentialMentions)
+	if err != nil {
+		return false, err
 	}
 
+	if len(outOfChannelUsers) == 0 && len(outOfGroupsUsers) == 0 {
+		return false, nil
+	}
+
+	a.SendEphemeralPost(post.UserId, makeOutOfChannelMentionPost(sender, post, outOfChannelUsers, outOfGroupsUsers))
+
+	return true, nil
+}
+
+func (a *App) filterOutOfChannelMentions(sender *model.User, post *model.Post, channel *model.Channel, potentialMentions []string) ([]*model.User, []*model.User, error) {
+	if post.IsSystemMessage() {
+		return nil, nil, nil
+	}
+
+	if channel.TeamId == "" || channel.Type == model.CHANNEL_DIRECT || channel.Type == model.CHANNEL_GROUP {
+		return nil, nil, nil
+	}
+
+	if len(potentialMentions) == 0 {
+		return nil, nil, nil
+	}
+
+	users, err := a.Srv.Store.User().GetProfilesByUsernames(potentialMentions, &model.ViewUsersRestrictions{Teams: []string{channel.TeamId}})
+	if err != nil {
+		return nil, nil, err
+	}
+
+	// Filter out inactive users and bots
+	allUsers := model.UserSlice(users).FilterByActive(true)
+	allUsers = allUsers.FilterWithoutBots()
+
+	if len(allUsers) == 0 {
+		return nil, nil, nil
+	}
+
+	// Differentiate between users who can and can't be added to the channel
+	var outOfChannelUsers model.UserSlice
+	var outOfGroupsUsers model.UserSlice
+	if channel.IsGroupConstrained() {
+		nonMemberIDs, err := a.FilterNonGroupChannelMembers(allUsers.IDs(), channel)
+		if err != nil {
+			return nil, nil, err
+		}
+
+		outOfChannelUsers = allUsers.FilterWithoutID(nonMemberIDs)
+		outOfGroupsUsers = allUsers.FilterByID(nonMemberIDs)
+	} else {
+		outOfChannelUsers = users
+	}
+
+	return outOfChannelUsers, outOfGroupsUsers, nil
+}
+
+func makeOutOfChannelMentionPost(sender *model.User, post *model.Post, outOfChannelUsers, outOfGroupsUsers []*model.User) *model.Post {
 	allUsers := model.UserSlice(append(outOfChannelUsers, outOfGroupsUsers...))
 
 	ocUsers := model.UserSlice(outOfChannelUsers)
@@ -478,19 +514,14 @@ func (a *App) sendOutOfChannelMentions(sender *model.User, post *model.Post, out
 		},
 	}
 
-	a.SendEphemeralPost(
+	return &model.Post{
-		post.UserId,
+		Id:        ephemeralPostId,
-		&model.Post{
+		RootId:    post.RootId,
-			Id:        ephemeralPostId,
+		ChannelId: post.ChannelId,
-			RootId:    post.RootId,
+		Message:   message,
-			ChannelId: post.ChannelId,
+		CreateAt:  post.CreateAt + 1,
-			Message:   message,
+		Props:     props,
-			CreateAt:  post.CreateAt + 1,
+	}
-			Props:     props,
-		},
-	)
-
-	return nil
 }
 
 func splitAtFinal(items []string) (preliminary []string, final string) {

app/notification_test.go

@@ -123,6 +123,178 @@ func TestSendNotificationsWithManyUsers(t *testing.T) {
 	})
 }
 
+func TestSendOutOfChannelMentions(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	channel := th.BasicChannel
+
+	user1 := th.BasicUser
+	user2 := th.BasicUser2
+
+	t.Run("should send ephemeral post when there is an out of channel mention", func(t *testing.T) {
+		post := &model.Post{}
+		potentialMentions := []string{user2.Username}
+
+		sent, err := th.App.sendOutOfChannelMentions(user1, post, channel, potentialMentions)
+
+		assert.Nil(t, err)
+		assert.True(t, sent)
+	})
+
+	t.Run("should not send ephemeral post when there are no out of channel mentions", func(t *testing.T) {
+		post := &model.Post{}
+		potentialMentions := []string{"not a user"}
+
+		sent, err := th.App.sendOutOfChannelMentions(user1, post, channel, potentialMentions)
+
+		assert.Nil(t, err)
+		assert.False(t, sent)
+	})
+}
+
+func TestFilterOutOfChannelMentions(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	channel := th.BasicChannel
+
+	user1 := th.BasicUser
+	user2 := th.BasicUser2
+	user3 := th.CreateUser()
+	th.LinkUserToTeam(user3, th.BasicTeam)
+
+	t.Run("should return users not in the channel", func(t *testing.T) {
+		post := &model.Post{}
+		potentialMentions := []string{user2.Username, user3.Username}
+
+		outOfChannelUsers, outOfGroupUsers, err := th.App.filterOutOfChannelMentions(user1, post, channel, potentialMentions)
+
+		assert.Nil(t, err)
+		assert.Len(t, outOfChannelUsers, 2)
+		assert.True(t, (outOfChannelUsers[0].Id == user2.Id || outOfChannelUsers[1].Id == user2.Id))
+		assert.True(t, (outOfChannelUsers[0].Id == user3.Id || outOfChannelUsers[1].Id == user3.Id))
+		assert.Nil(t, outOfGroupUsers)
+	})
+
+	t.Run("should not return results for a system message", func(t *testing.T) {
+		post := &model.Post{
+			Type: model.POST_ADD_REMOVE,
+		}
+		potentialMentions := []string{user2.Username, user3.Username}
+
+		outOfChannelUsers, outOfGroupUsers, err := th.App.filterOutOfChannelMentions(user1, post, channel, potentialMentions)
+
+		assert.Nil(t, err)
+		assert.Nil(t, outOfChannelUsers)
+		assert.Nil(t, outOfGroupUsers)
+	})
+
+	t.Run("should not return results for a direct message", func(t *testing.T) {
+		post := &model.Post{}
+		directChannel := &model.Channel{
+			Type: model.CHANNEL_DIRECT,
+		}
+		potentialMentions := []string{user2.Username, user3.Username}
+
+		outOfChannelUsers, outOfGroupUsers, err := th.App.filterOutOfChannelMentions(user1, post, directChannel, potentialMentions)
+
+		assert.Nil(t, err)
+		assert.Nil(t, outOfChannelUsers)
+		assert.Nil(t, outOfGroupUsers)
+	})
+
+	t.Run("should not return results for a group message", func(t *testing.T) {
+		post := &model.Post{}
+		groupChannel := &model.Channel{
+			Type: model.CHANNEL_GROUP,
+		}
+		potentialMentions := []string{user2.Username, user3.Username}
+
+		outOfChannelUsers, outOfGroupUsers, err := th.App.filterOutOfChannelMentions(user1, post, groupChannel, potentialMentions)
+
+		assert.Nil(t, err)
+		assert.Nil(t, outOfChannelUsers)
+		assert.Nil(t, outOfGroupUsers)
+	})
+
+	t.Run("should not return inactive users", func(t *testing.T) {
+		inactiveUser := th.CreateUser()
+		inactiveUser, appErr := th.App.UpdateActive(inactiveUser, false)
+		require.Nil(t, appErr)
+
+		post := &model.Post{}
+		potentialMentions := []string{inactiveUser.Username}
+
+		outOfChannelUsers, outOfGroupUsers, err := th.App.filterOutOfChannelMentions(user1, post, channel, potentialMentions)
+
+		assert.Nil(t, err)
+		assert.Nil(t, outOfChannelUsers)
+		assert.Nil(t, outOfGroupUsers)
+	})
+
+	t.Run("should not return bot users", func(t *testing.T) {
+		botUser := th.CreateUser()
+		botUser.IsBot = true
+
+		post := &model.Post{}
+		potentialMentions := []string{botUser.Username}
+
+		outOfChannelUsers, outOfGroupUsers, err := th.App.filterOutOfChannelMentions(user1, post, channel, potentialMentions)
+
+		assert.Nil(t, err)
+		assert.Nil(t, outOfChannelUsers)
+		assert.Nil(t, outOfGroupUsers)
+	})
+
+	t.Run("should not return results for non-existant users", func(t *testing.T) {
+		post := &model.Post{}
+		potentialMentions := []string{"foo", "bar"}
+
+		outOfChannelUsers, outOfGroupUsers, err := th.App.filterOutOfChannelMentions(user1, post, channel, potentialMentions)
+
+		assert.Nil(t, err)
+		assert.Nil(t, outOfChannelUsers)
+		assert.Nil(t, outOfGroupUsers)
+	})
+
+	t.Run("should separate users not in the channel from users not in the group", func(t *testing.T) {
+		nonChannelMember := th.CreateUser()
+		th.LinkUserToTeam(nonChannelMember, th.BasicTeam)
+		nonGroupMember := th.CreateUser()
+		th.LinkUserToTeam(nonGroupMember, th.BasicTeam)
+
+		group := th.CreateGroup()
+		_, appErr := th.App.UpsertGroupMember(group.Id, th.BasicUser.Id)
+		require.Nil(t, appErr)
+		_, appErr = th.App.UpsertGroupMember(group.Id, nonChannelMember.Id)
+		require.Nil(t, appErr)
+
+		constrainedChannel := th.CreateChannel(th.BasicTeam)
+		constrainedChannel.GroupConstrained = model.NewBool(true)
+		constrainedChannel, appErr = th.App.UpdateChannel(constrainedChannel)
+		require.Nil(t, appErr)
+
+		_, appErr = th.App.CreateGroupSyncable(&model.GroupSyncable{
+			GroupId:    group.Id,
+			Type:       model.GroupSyncableTypeChannel,
+			SyncableId: constrainedChannel.Id,
+		})
+		require.Nil(t, appErr)
+
+		post := &model.Post{}
+		potentialMentions := []string{nonChannelMember.Username, nonGroupMember.Username}
+
+		outOfChannelUsers, outOfGroupUsers, err := th.App.filterOutOfChannelMentions(user1, post, constrainedChannel, potentialMentions)
+
+		assert.Nil(t, err)
+		assert.Len(t, outOfChannelUsers, 1)
+		assert.Equal(t, nonChannelMember.Id, outOfChannelUsers[0].Id)
+		assert.Len(t, outOfGroupUsers, 1)
+		assert.Equal(t, nonGroupMember.Id, outOfGroupUsers[0].Id)
+	})
+}
+
 func TestGetExplicitMentions(t *testing.T) {
 	id1 := model.NewId()
 	id2 := model.NewId()