From eba8794540fadedf673277b4345e6fc44b2f08e0 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Fri, 9 Aug 2019 09:27:28 -0400 Subject: [PATCH] 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 | 115 ++++++++++++++++---------- app/notification_test.go | 172 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 245 insertions(+), 42 deletions(-) diff --git a/app/notification.go b/app/notification.go index f9c9dff1c1..c42cc39e12 100644 --- a/app/notification.go +++ b/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() { - if users, err := a.Srv.Store.User().GetProfilesByUsernames(m.OtherPotentialMentions, &model.ViewUsersRestrictions{Teams: []string{team.Id}}); err == nil { - channelMentions := model.UserSlice(users).FilterByActive(true) - - 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) - }) - } + go func() { + _, err := a.sendOutOfChannelMentions(sender, post, channel, m.OtherPotentialMentions) + 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)) } - } + }() // 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 { - if len(outOfChannelUsers) == 0 && len(outOfGroupsUsers) == 0 { - return nil +// sendOutOfChannelMentions sends an ephemeral post to the sender of a post if any of the given potential mentions +// are outside of the post's channel. Returns whether or not an ephemeral post was sent. +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( - post.UserId, - &model.Post{ - Id: ephemeralPostId, - RootId: post.RootId, - ChannelId: post.ChannelId, - Message: message, - CreateAt: post.CreateAt + 1, - Props: props, - }, - ) - - return nil + return &model.Post{ + Id: ephemeralPostId, + RootId: post.RootId, + ChannelId: post.ChannelId, + Message: message, + CreateAt: post.CreateAt + 1, + Props: props, + } } func splitAtFinal(items []string) (preliminary []string, final string) { diff --git a/app/notification_test.go b/app/notification_test.go index a86ce1fbac..c07778896e 100644 --- a/app/notification_test.go +++ b/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()