diff --git a/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/channel_notifications_spec.js b/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/channel_notifications_spec.js index 231ca2eea18..70edbe4c0e8 100644 --- a/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/channel_notifications_spec.js +++ b/e2e-tests/cypress/tests/integration/channels/collapsed_reply_threads/channel_notifications_spec.js @@ -93,6 +93,9 @@ describe('CRT Desktop notifications', () => { expect(args.body, `Notification body: "${args.body}" should match: "${message}"`).to.equal(`@${sender.username}: ${message}`); return true; }); + + // # Cleanup + cy.apiDeletePost(postId); }); }); @@ -147,6 +150,85 @@ describe('CRT Desktop notifications', () => { expect(args.body, `Notification body: "${args.body}" should match: "${message}"`).to.equal(`@${sender.username}: ${message}`); return true; }); + + // # Cleanup + cy.apiDeletePost(postId); + }); + }); + + it('When a reply is deleted in open channel, the notification should be cleared', () => { + // # Visit channel + cy.visit(testChannelUrl); + + // # Post a root message as other user + cy.postMessageAs({sender, message: 'a thread', channelId: testChannelId, rootId: ''}); + + // # Get post id of message + cy.getLastPostId().then((postId) => { + // # Post a reply to the thread, which will trigger a follow + cy.postMessageAs({sender: receiver, message: 'following the thread', channelId: testChannelId, rootId: postId}); + + // # Post a reply to the thread + cy.postMessageAs({sender, message: 'a reply', channelId: testChannelId, rootId: postId}).as('reply'); + + // # Post a mention reply to the thread + cy.postMessageAs({sender, message: `@${receiver.username} mention reply`, channelId: testChannelId, rootId: postId}). + as('replyMention'); + + // * Verify there is a notification + cy.get('#sidebarItem_threads #unreadMentions').should('exist').and('have.text', '1'); + + // # Delete the replies + cy.wrap(['@reply', '@replyMention']).each((reply) => { + cy.get(reply).then(({id}) => { + cy.apiDeletePost(id); + }); + }); + + // * Verify there is no notification + cy.get('#sidebarItem_threads #unreadMentions').should('not.exist'); + + // # Cleanup + cy.apiDeletePost(postId); + }); + }); + + it('When a reply is deleted in DM channel, the notification should be cleared', () => { + cy.apiCreateDirectChannel([receiver.id, sender.id]).then(({channel: dmChannel}) => { + // # Visit channel + cy.visit(`/${testTeam.name}/messages/@${sender.username}`); + + // # Post a root message as other user + cy.postMessageAs({sender, message: 'a thread', channelId: dmChannel.id, rootId: ''}).as('rootPost'); + + // # Get post id of message + cy.get('@rootPost').then(({id: rootId}) => { + // # Post a reply to the thread, which will trigger a follow + cy.postMessageAs({sender: receiver, message: 'following the thread', channelId: dmChannel.id, rootId}); + + // # Post a reply to the thread + cy.postMessageAs({sender, message: 'a reply', channelId: dmChannel.id, rootId}).as('reply'); + + // # Post a mention reply to the thread + cy.postMessageAs({sender, message: `@${receiver.username} mention reply`, channelId: dmChannel.id, rootId}). + as('replyMention'); + + // * Verify there is a notification + cy.get('#sidebarItem_threads #unreadMentions').should('exist'); + + // # Delete the replies + cy.wrap(['@reply', '@replyMention']).each((reply) => { + cy.get(reply).then(({id}) => { + cy.apiDeletePost(id); + }); + }); + + // * Verify there is no notification + cy.get('#sidebarItem_threads #unreadMentions').should('not.exist'); + + // # Cleanup + cy.apiDeletePost(rootId); + }); }); }); }); diff --git a/server/channels/app/app_iface.go b/server/channels/app/app_iface.go index 9f52c2539e6..e9786483572 100644 --- a/server/channels/app/app_iface.go +++ b/server/channels/app/app_iface.go @@ -987,6 +987,7 @@ type AppIface interface { RemoveFile(path string) *model.AppError RemoveLdapPrivateCertificate() *model.AppError RemoveLdapPublicCertificate() *model.AppError + RemoveNotifications(c request.CTX, post *model.Post, channel *model.Channel) error RemoveRecentCustomStatus(userID string, status *model.CustomStatus) *model.AppError RemoveSamlIdpCertificate() *model.AppError RemoveSamlPrivateCertificate() *model.AppError diff --git a/server/channels/app/notification.go b/server/channels/app/notification.go index 54c5921bf5d..5981e6742a7 100644 --- a/server/channels/app/notification.go +++ b/server/channels/app/notification.go @@ -121,35 +121,10 @@ func (a *App) SendNotifications(c request.CTX, post *model.Post, team *model.Tea groups = result.Data.(map[string]*model.Group) } - mentions := &ExplicitMentions{} - allActivityPushUserIds := []string{} - var allowChannelMentions bool - var keywords map[string][]string - if channel.Type == model.ChannelTypeDirect { - otherUserId := channel.GetOtherUserIdForDM(post.UserId) - - _, ok := profileMap[otherUserId] - if ok { - mentions.addMention(otherUserId, DMMention) - } - - if post.GetProp("from_webhook") == "true" { - mentions.addMention(post.UserId, DMMention) - } - } else { - allowChannelMentions = a.allowChannelMentions(c, post, len(profileMap)) - keywords = a.getMentionKeywordsInChannel(profileMap, allowChannelMentions, channelMemberNotifyPropsMap) - - mentions = getExplicitMentions(post, keywords, groups) - // Add an implicit mention when a user is added to a channel - // even if the user has set 'username mentions' to false in account settings. - if post.Type == model.PostTypeAddToChannel { - addedUserId, ok := post.GetProp(model.PostPropsAddedUserId).(string) - if ok { - mentions.addMention(addedUserId, KeywordMention) - } - } + mentions, keywords := a.getExplicitMentionsAndKeywords(c, post, channel, profileMap, groups, channelMemberNotifyPropsMap, parentPostList) + var allActivityPushUserIds []string + if channel.Type != model.ChannelTypeDirect { // Iterate through all groups that were mentioned and insert group members into the list of mentions or potential mentions for _, group := range mentions.GroupMentions { anyUsersMentionedByGroup, err := a.insertGroupMentions(group, channel, profileMap, mentions) @@ -162,36 +137,6 @@ func (a *App) SendNotifications(c request.CTX, post *model.Post, team *model.Tea } } - // get users that have comment thread mentions enabled - if post.RootId != "" && parentPostList != nil { - for _, threadPost := range parentPostList.Posts { - profile := profileMap[threadPost.UserId] - if profile == nil { - continue - } - // If this is the root post and it was posted by an OAuth bot, don't notify the user - if threadPost.Id == parentPostList.Order[0] && threadPost.IsFromOAuthBot() { - continue - } - if a.IsCRTEnabledForUser(c, profile.Id) { - continue - } - if profile.NotifyProps[model.CommentsNotifyProp] == model.CommentsNotifyAny || (profile.NotifyProps[model.CommentsNotifyProp] == model.CommentsNotifyRoot && threadPost.Id == parentPostList.Order[0]) { - mentionType := ThreadMention - if threadPost.Id == parentPostList.Order[0] { - mentionType = CommentMention - } - - mentions.addMention(threadPost.UserId, mentionType) - } - } - } - - // prevent the user from mentioning themselves - if post.GetProp("from_webhook") != "true" { - mentions.removeMention(post.UserId) - } - go func() { _, err := a.sendOutOfChannelMentions(c, sender, post, channel, mentions.OtherPotentialMentions) if err != nil { @@ -663,6 +608,212 @@ func (a *App) SendNotifications(c request.CTX, post *model.Post, team *model.Tea return mentionedUsersList, nil } +func (a *App) RemoveNotifications(c request.CTX, post *model.Post, channel *model.Channel) error { + isCRTAllowed := *a.Config().ServiceSettings.CollapsedThreads != model.CollapsedThreadsDisabled + + // CRT is the main issue in this case as notifications indicator are not updated when accessing threads from the sidebar. + if isCRTAllowed && post.RootId != "" { + var team *model.Team + if channel.TeamId != "" { + t, err1 := a.Srv().Store().Team().Get(channel.TeamId) + if err1 != nil { + return model.NewAppError("RemoveNotifications", "app.post.delete_post.get_team.app_error", nil, "", http.StatusInternalServerError).Wrap(err1) + } + team = t + } else { + // Blank team for DMs + team = &model.Team{} + } + + pCh := make(chan store.StoreResult, 1) + go func() { + props, err := a.Srv().Store().User().GetAllProfilesInChannel(context.Background(), channel.Id, true) + pCh <- store.StoreResult{Data: props, NErr: err} + close(pCh) + }() + + cmnCh := make(chan store.StoreResult, 1) + go func() { + props, err := a.Srv().Store().Channel().GetAllChannelMembersNotifyPropsForChannel(channel.Id, true) + cmnCh <- store.StoreResult{Data: props, NErr: err} + close(cmnCh) + }() + + var gCh chan store.StoreResult + if a.allowGroupMentions(c, post) { + gCh = make(chan store.StoreResult, 1) + go func() { + groupsMap, err := a.getGroupsAllowedForReferenceInChannel(channel, team) + gCh <- store.StoreResult{Data: groupsMap, NErr: err} + close(gCh) + }() + } + + result := <-pCh + if result.NErr != nil { + return result.NErr + } + profileMap := result.Data.(map[string]*model.User) + + result = <-cmnCh + if result.NErr != nil { + return result.NErr + } + channelMemberNotifyPropsMap := result.Data.(map[string]model.StringMap) + + groups := make(map[string]*model.Group) + if gCh != nil { + result = <-gCh + if result.NErr != nil { + return result.NErr + } + groups = result.Data.(map[string]*model.Group) + } + + mentions, _ := a.getExplicitMentionsAndKeywords(c, post, channel, profileMap, groups, channelMemberNotifyPropsMap, nil) + + userIDs := []string{} + for _, group := range mentions.GroupMentions { + for page := 0; ; page++ { + groupMemberPage, count, appErr := a.GetGroupMemberUsersPage(group.Id, page, 100, &model.ViewUsersRestrictions{Channels: []string{channel.Id}}) + if appErr != nil { + return appErr + } + + for _, user := range groupMemberPage { + userIDs = append(userIDs, user.Id) + } + + // count is the total number of users that match the filter criteria. + // When we've processed `count` number of users, we know there aren't + // any more users left to query and we can break the loop + if len(userIDs) == count { + break + } + } + } + + for userID := range mentions.Mentions { + userIDs = append(userIDs, userID) + } + + for _, userID := range userIDs { + threadMembership, appErr := a.GetThreadMembershipForUser(userID, post.RootId) + if appErr != nil { + return appErr + } + + // If the user has viewed the thread or there are no unread mentions, skip. + if threadMembership.LastViewed > post.CreateAt || threadMembership.UnreadMentions == 0 { + continue + } + + threadMembership.UnreadMentions -= 1 + if _, err := a.Srv().Store().Thread().UpdateMembership(threadMembership); err != nil { + return err + } + + userThread, err := a.Srv().Store().Thread().GetThreadForUser(threadMembership, true, a.IsPostPriorityEnabled()) + if err != nil { + return err + } + + if userThread != nil { + previousUnreadMentions := int64(0) + previousUnreadReplies := int64(0) + + a.sanitizeProfiles(userThread.Participants, false) + userThread.Post.SanitizeProps() + + sanitizedPost, err1 := a.SanitizePostMetadataForUser(c, userThread.Post, userID) + if err1 != nil { + return err1 + } + userThread.Post = sanitizedPost + + payload, jsonErr := json.Marshal(userThread) + if jsonErr != nil { + mlog.Warn("Failed to encode thread to JSON") + } + + message := model.NewWebSocketEvent(model.WebsocketEventThreadUpdated, team.Id, "", userID, nil, "") + message.Add("thread", string(payload)) + message.Add("previous_unread_mentions", previousUnreadMentions) + message.Add("previous_unread_replies", previousUnreadReplies) + + a.Publish(message) + } + } + } + + return nil +} + +func (a *App) getExplicitMentionsAndKeywords(c request.CTX, post *model.Post, channel *model.Channel, profileMap map[string]*model.User, groups map[string]*model.Group, channelMemberNotifyPropsMap map[string]model.StringMap, parentPostList *model.PostList) (*ExplicitMentions, map[string][]string) { + mentions := &ExplicitMentions{} + var allowChannelMentions bool + var keywords map[string][]string + + if channel.Type == model.ChannelTypeDirect { + otherUserId := channel.GetOtherUserIdForDM(post.UserId) + + _, ok := profileMap[otherUserId] + if ok { + mentions.addMention(otherUserId, DMMention) + } + + if post.GetProp("from_webhook") == "true" { + mentions.addMention(post.UserId, DMMention) + } + } else { + allowChannelMentions = a.allowChannelMentions(c, post, len(profileMap)) + keywords = a.getMentionKeywordsInChannel(profileMap, allowChannelMentions, channelMemberNotifyPropsMap) + + mentions = getExplicitMentions(post, keywords, groups) + // Add an implicit mention when a user is added to a channel + // even if the user has set 'username mentions' to false in account settings. + if post.Type == model.PostTypeAddToChannel { + addedUserId, ok := post.GetProp(model.PostPropsAddedUserId).(string) + if ok { + mentions.addMention(addedUserId, KeywordMention) + } + } + + // Get users that have comment thread mentions enabled + if post.RootId != "" && parentPostList != nil { + for _, threadPost := range parentPostList.Posts { + profile := profileMap[threadPost.UserId] + if profile == nil { + continue + } + + // If this is the root post and it was posted by an OAuth bot, don't notify the user + if threadPost.Id == parentPostList.Order[0] && threadPost.IsFromOAuthBot() { + continue + } + if a.IsCRTEnabledForUser(c, profile.Id) { + continue + } + if profile.NotifyProps[model.CommentsNotifyProp] == model.CommentsNotifyAny || (profile.NotifyProps[model.CommentsNotifyProp] == model.CommentsNotifyRoot && threadPost.Id == parentPostList.Order[0]) { + mentionType := ThreadMention + if threadPost.Id == parentPostList.Order[0] { + mentionType = CommentMention + } + + mentions.addMention(threadPost.UserId, mentionType) + } + } + } + + // Prevent the user from mentioning themselves + if post.GetProp("from_webhook") != "true" { + mentions.removeMention(post.UserId) + } + } + + return mentions, keywords +} + func max(a, b int64) int64 { if a < b { return b diff --git a/server/channels/app/notification_test.go b/server/channels/app/notification_test.go index 7a5eafa2f5a..9ac09554791 100644 --- a/server/channels/app/notification_test.go +++ b/server/channels/app/notification_test.go @@ -6,6 +6,7 @@ package app import ( "fmt" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -2883,3 +2884,116 @@ func TestChannelAutoFollowThreads(t *testing.T) { require.NotNil(t, threadMembership) assert.False(t, threadMembership.Following) } + +func TestRemoveNotifications(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + u1 := th.BasicUser + u2 := th.BasicUser2 + c1 := th.BasicChannel + th.AddUserToChannel(u2, c1) + + // Enable CRT + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.ThreadAutoFollow = true + *cfg.ServiceSettings.CollapsedThreads = model.CollapsedThreadsDefaultOn + }) + + t.Run("base case", func(t *testing.T) { + rootPost := &model.Post{ + ChannelId: c1.Id, + Message: "root post by user1", + UserId: u1.Id, + } + rootPost, appErr := th.App.CreatePost(th.Context, rootPost, c1, false, true) + require.Nil(t, appErr) + + replyPost1 := &model.Post{ + ChannelId: c1.Id, + Message: "reply post by user2", + UserId: u2.Id, + RootId: rootPost.Id, + } + _, appErr = th.App.CreatePost(th.Context, replyPost1, c1, false, true) + require.Nil(t, appErr) + + replyPost2 := &model.Post{ + ChannelId: c1.Id, + Message: "@" + u2.Username + " mention by user1", + UserId: u1.Id, + RootId: rootPost.Id, + } + replyPost2, appErr = th.App.CreatePost(th.Context, replyPost2, c1, false, true) + require.Nil(t, appErr) + + _, appErr = th.App.DeletePost(th.Context, replyPost2.Id, u1.Id) + require.Nil(t, appErr) + + // Because we delete notification async, we need to wait + // just for a little while before checking the data + // 2 seconds is a very long time for the task we're performing + // but its okay considering sometimes the CI machines are slow. + time.Sleep(2 * time.Second) + + threadMembership, appErr := th.App.GetThreadMembershipForUser(u2.Id, rootPost.Id) + require.Nil(t, appErr) + thread, appErr := th.App.GetThreadForUser(threadMembership, false) + require.Nil(t, appErr) + require.Equal(t, int64(0), thread.UnreadMentions) + require.Equal(t, int64(0), thread.UnreadReplies) + }) + + t.Run("when mentioned via a user group", func(t *testing.T) { + group, appErr := th.App.CreateGroup(&model.Group{ + Name: model.NewString("test_group"), + DisplayName: "test_group", + Source: model.GroupSourceCustom, + }) + require.Nil(t, appErr) + + _, appErr = th.App.UpsertGroupMember(group.Id, u1.Id) + require.Nil(t, appErr) + + _, appErr = th.App.UpsertGroupMember(group.Id, u2.Id) + require.Nil(t, appErr) + + rootPost := &model.Post{ + ChannelId: c1.Id, + Message: "root post by user1", + UserId: u1.Id, + } + rootPost, appErr = th.App.CreatePost(th.Context, rootPost, c1, false, true) + require.Nil(t, appErr) + + replyPost1 := &model.Post{ + ChannelId: c1.Id, + Message: "reply post by user2", + UserId: u2.Id, + RootId: rootPost.Id, + } + _, appErr = th.App.CreatePost(th.Context, replyPost1, c1, false, true) + require.Nil(t, appErr) + + replyPost2 := &model.Post{ + ChannelId: c1.Id, + Message: "@" + *group.Name + " mention by user1", + UserId: u1.Id, + RootId: rootPost.Id, + } + replyPost2, appErr = th.App.CreatePost(th.Context, replyPost2, c1, false, true) + require.Nil(t, appErr) + + _, appErr = th.App.DeletePost(th.Context, replyPost2.Id, u1.Id) + require.Nil(t, appErr) + + time.Sleep(2 * time.Second) + + threadMembership, appErr := th.App.GetThreadMembershipForUser(u2.Id, rootPost.Id) + require.Nil(t, appErr) + thread, appErr := th.App.GetThreadForUser(threadMembership, false) + require.Nil(t, appErr) + require.Equal(t, int64(0), thread.UnreadMentions) + require.Equal(t, int64(0), thread.UnreadReplies) + }) +} diff --git a/server/channels/app/opentracing/opentracing_layer.go b/server/channels/app/opentracing/opentracing_layer.go index d268cc9b0e2..3bc1db02728 100644 --- a/server/channels/app/opentracing/opentracing_layer.go +++ b/server/channels/app/opentracing/opentracing_layer.go @@ -14123,6 +14123,28 @@ func (a *OpenTracingAppLayer) RemoveLdapPublicCertificate() *model.AppError { return resultVar0 } +func (a *OpenTracingAppLayer) RemoveNotifications(c request.CTX, post *model.Post, channel *model.Channel) error { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.RemoveNotifications") + + a.ctx = newCtx + a.app.Srv().Store().SetContext(newCtx) + defer func() { + a.app.Srv().Store().SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0 := a.app.RemoveNotifications(c, post, channel) + + if resultVar0 != nil { + span.LogFields(spanlog.Error(resultVar0)) + ext.Error.Set(span, true) + } + + return resultVar0 +} + func (a *OpenTracingAppLayer) RemoveRecentCustomStatus(userID string, status *model.CustomStatus) *model.AppError { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.RemoveRecentCustomStatus") diff --git a/server/channels/app/post.go b/server/channels/app/post.go index 8de17053dab..c8f1b6cdd25 100644 --- a/server/channels/app/post.go +++ b/server/channels/app/post.go @@ -1338,6 +1338,12 @@ func (a *App) DeletePost(c request.CTX, postID, deleteByID string) (*model.Post, a.deleteFlaggedPosts(post.Id) }) + a.Srv().Go(func() { + if err = a.RemoveNotifications(c, post, channel); err != nil { + a.Log().Error("DeletePost failed to delete notification", mlog.Err(err)) + } + }) + a.invalidateCacheForChannelPosts(post.ChannelId) return post, nil diff --git a/server/i18n/en.json b/server/i18n/en.json index a64084f4820..bc58a6a106a 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -6191,6 +6191,10 @@ "id": "app.post.delete.app_error", "translation": "Unable to delete the post." }, + { + "id": "app.post.delete_post.get_team.app_error", + "translation": "An error occurred getting the team." + }, { "id": "app.post.get.app_error", "translation": "Unable to get the post."