[MM-45802] Clear CRT notification on deleted reply (#23568)

* reduce the counter on post deletion

* add test

* change translations

* fix collecting mentions for DMs

* add translation texts

* extract logic for getting mentions

* send WS event

* add e2e tests

* tidy mod

* WIP

* Deleting notification async

* Fixed a unit test

* Added more tests

* Updated i18n

* CI

* mattermost-server -> mattermost

* mattermost-server -> mattermost

---------

Co-authored-by: Konstantinos Pittas <konstantinos.pittas@mattermost.com>
Co-authored-by: Mattermost Build <build@mattermost.com>

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);
+            });
         });
     });
 });

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

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

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)
+	})
+}

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

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

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