From 8714760ec954089721ac5d782a059c979edbafac Mon Sep 17 00:00:00 2001 From: Scott Bishel Date: Mon, 5 Feb 2024 10:10:46 -0700 Subject: [PATCH] don't add message sender if also in group (#26094) --- server/channels/app/notification.go | 14 ++++--- server/channels/app/notification_test.go | 42 +++++++++++++++---- .../app/post_persistent_notification.go | 2 +- 3 files changed, 42 insertions(+), 16 deletions(-) diff --git a/server/channels/app/notification.go b/server/channels/app/notification.go index 8e85059c36..5e86cb25b5 100644 --- a/server/channels/app/notification.go +++ b/server/channels/app/notification.go @@ -126,7 +126,7 @@ func (a *App) SendNotifications(c request.CTX, post *model.Post, team *model.Tea // Iterate through all groups that were mentioned and insert group members into the list of mentions or potential mentions for groupID := range mentions.GroupMentions { group := groups[groupID] - anyUsersMentionedByGroup, err := a.insertGroupMentions(group, channel, profileMap, mentions) + anyUsersMentionedByGroup, err := a.insertGroupMentions(sender.Id, group, channel, profileMap, mentions) if err != nil { return nil, err } @@ -1200,7 +1200,7 @@ func (a *App) getMentionKeywordsInChannel(profiles map[string]*model.User, allow // insertGroupMentions adds group members in the channel to Mentions, adds group members not in the channel to OtherPotentialMentions // returns false if no group members present in the team that the channel belongs to -func (a *App) insertGroupMentions(group *model.Group, channel *model.Channel, profileMap map[string]*model.User, mentions *MentionResults) (bool, *model.AppError) { +func (a *App) insertGroupMentions(senderID string, group *model.Group, channel *model.Channel, profileMap map[string]*model.User, mentions *MentionResults) (bool, *model.AppError) { var err error var groupMembers []*model.User outOfChannelGroupMembers := []*model.User{} @@ -1221,10 +1221,12 @@ func (a *App) insertGroupMentions(group *model.Group, channel *model.Channel, pr } for _, member := range groupMembers { - if _, ok := profileMap[member.Id]; ok { - mentions.Mentions[member.Id] = GroupMention - } else { - outOfChannelGroupMembers = append(outOfChannelGroupMembers, member) + if member.Id != senderID { + if _, ok := profileMap[member.Id]; ok { + mentions.Mentions[member.Id] = GroupMention + } else { + outOfChannelGroupMembers = append(outOfChannelGroupMembers, member) + } } } diff --git a/server/channels/app/notification_test.go b/server/channels/app/notification_test.go index b7249c2c82..0dfa677013 100644 --- a/server/channels/app/notification_test.go +++ b/server/channels/app/notification_test.go @@ -2492,6 +2492,12 @@ func TestInsertGroupMentions(t *testing.T) { _, err = th.App.UpsertGroupMember(group.Id, groupChannelMember.Id) require.Nil(t, err) + senderGroupChannelMember := th.CreateUser() + th.LinkUserToTeam(senderGroupChannelMember, team) + th.App.AddUserToChannel(th.Context, senderGroupChannelMember, channel, false) + _, err = th.App.UpsertGroupMember(group.Id, senderGroupChannelMember.Id) + require.Nil(t, err) + nonGroupChannelMember := th.CreateUser() th.LinkUserToTeam(nonGroupChannelMember, team) th.App.AddUserToChannel(th.Context, nonGroupChannelMember, channel, false) @@ -2507,11 +2513,11 @@ func TestInsertGroupMentions(t *testing.T) { groupWithNoMembers, err = th.App.UpdateGroup(groupWithNoMembers) require.Nil(t, err) - profileMap := map[string]*model.User{groupChannelMember.Id: groupChannelMember, nonGroupChannelMember.Id: nonGroupChannelMember} + profileMap := map[string]*model.User{groupChannelMember.Id: groupChannelMember, senderGroupChannelMember.Id: senderGroupChannelMember, nonGroupChannelMember.Id: nonGroupChannelMember} - t.Run("should add expected mentions for users part of the mentioned group", func(t *testing.T) { + t.Run("should add expected mentions for users part of the mentioned group sender in group", func(t *testing.T) { mentions := &MentionResults{} - usersMentioned, err := th.App.insertGroupMentions(group, channel, profileMap, mentions) + usersMentioned, err := th.App.insertGroupMentions(senderGroupChannelMember.Id, group, channel, profileMap, mentions) require.Nil(t, err) require.Equal(t, usersMentioned, true) @@ -2525,9 +2531,27 @@ func TestInsertGroupMentions(t *testing.T) { require.Equal(t, mentions.OtherPotentialMentions[0], nonChannelGroupMember.Username) }) + t.Run("should add expected mentions for users part of the mentioned group sender not in group", func(t *testing.T) { + mentions := &MentionResults{} + usersMentioned, err := th.App.insertGroupMentions(nonGroupChannelMember.Id, group, channel, profileMap, mentions) + require.Nil(t, err) + require.Equal(t, usersMentioned, true) + + // Ensure group member that is also a channel member is added to the mentions list. + require.Equal(t, 2, len(mentions.Mentions)) + _, found := mentions.Mentions[groupChannelMember.Id] + require.Equal(t, found, true) + _, found = mentions.Mentions[senderGroupChannelMember.Id] + require.Equal(t, found, true) + + // Ensure group member that is not a channel member is added to the other potential mentions list. + require.Equal(t, len(mentions.OtherPotentialMentions), 1) + require.Equal(t, mentions.OtherPotentialMentions[0], nonChannelGroupMember.Username) + }) + t.Run("should add no expected or potential mentions if the group has no users ", func(t *testing.T) { mentions := &MentionResults{} - usersMentioned, err := th.App.insertGroupMentions(groupWithNoMembers, channel, profileMap, mentions) + usersMentioned, err := th.App.insertGroupMentions(senderGroupChannelMember.Id, groupWithNoMembers, channel, profileMap, mentions) require.Nil(t, err) require.Equal(t, usersMentioned, false) @@ -2538,8 +2562,8 @@ func TestInsertGroupMentions(t *testing.T) { t.Run("should keep existing mentions", func(t *testing.T) { mentions := &MentionResults{} - th.App.insertGroupMentions(group, channel, profileMap, mentions) - th.App.insertGroupMentions(groupWithNoMembers, channel, profileMap, mentions) + th.App.insertGroupMentions(senderGroupChannelMember.Id, group, channel, profileMap, mentions) + th.App.insertGroupMentions(senderGroupChannelMember.Id, groupWithNoMembers, channel, profileMap, mentions) // Ensure mentions from group are kept after running with groupWithNoMembers require.Equal(t, len(mentions.Mentions), 1) @@ -2551,13 +2575,13 @@ func TestInsertGroupMentions(t *testing.T) { emptyProfileMap := make(map[string]*model.User) groupChannel := &model.Channel{Type: model.ChannelTypeGroup} - usersMentioned, _ := th.App.insertGroupMentions(group, groupChannel, emptyProfileMap, mentions) + usersMentioned, _ := th.App.insertGroupMentions(senderGroupChannelMember.Id, group, groupChannel, emptyProfileMap, mentions) // Ensure group channel with no group members mentioned always returns true require.Equal(t, usersMentioned, true) require.Equal(t, len(mentions.Mentions), 0) directChannel := &model.Channel{Type: model.ChannelTypeDirect} - usersMentioned, _ = th.App.insertGroupMentions(group, directChannel, emptyProfileMap, mentions) + usersMentioned, _ = th.App.insertGroupMentions(senderGroupChannelMember.Id, group, directChannel, emptyProfileMap, mentions) // Ensure direct channel with no group members mentioned always returns true require.Equal(t, usersMentioned, true) require.Equal(t, len(mentions.Mentions), 0) @@ -2568,7 +2592,7 @@ func TestInsertGroupMentions(t *testing.T) { require.Nil(t, err) mentions := &MentionResults{} - th.App.insertGroupMentions(group, groupChannel, profileMap, mentions) + th.App.insertGroupMentions(senderGroupChannelMember.Id, group, groupChannel, profileMap, mentions) require.Equal(t, len(mentions.Mentions), 1) _, found := mentions.Mentions[groupChannelMember.Id] diff --git a/server/channels/app/post_persistent_notification.go b/server/channels/app/post_persistent_notification.go index 7325f86931..237ed3bbc7 100644 --- a/server/channels/app/post_persistent_notification.go +++ b/server/channels/app/post_persistent_notification.go @@ -185,7 +185,7 @@ func (a *App) forEachPersistentNotificationPost(posts []*model.Post, fn func(pos mentions = getExplicitMentions(post, keywords) for groupID := range mentions.GroupMentions { group := channelGroupMap[channel.Id][groupID] - _, err := a.insertGroupMentions(group, channel, profileMap, mentions) + _, err := a.insertGroupMentions(post.UserId, group, channel, profileMap, mentions) if err != nil { return errors.Wrapf(err, "failed to include mentions from group - %s for channel - %s", group.Id, channel.Id) }