From b15cd9d29f05d01d3102981c18f9b05c60eda9cb Mon Sep 17 00:00:00 2001 From: Kyriakos Z <3829551+koox00@users.noreply.github.com> Date: Fri, 15 Apr 2022 19:47:04 +0300 Subject: [PATCH] MM-41565,MM-42473: fixes incorrect unread thread count (#19962) * MM-41565,MM-42473: fixes incorrect unread thread count The infamous "-1 unread threads" bug, was due to incorrectly counting previous unread mentions and replies. When we calculate the previous counts, we didn't take into account if a thread was newly followed one or not. So when a user gets added to a thread via a mention receives a WS event with the previous count values calculated by subtracting "1" from the current count values. In the result the webapp subtracts the previous values for the thread from the total values of all threads for the user. This is wrong since the user just followed the thread those previous values should be 0. Which results into showing a negative value of unread threads to the user. This commit fixes this issue by zeroing the previous count values when the user was not following the thread previously. * Adds test, and addresses comments * Changes test's description * Removes unneeded assertion * Empty commit just to re-build --- api4/user_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++++ app/notification.go | 21 ++++++++++++++------- 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/api4/user_test.go b/api4/user_test.go index 0afab99d12..75aec717e1 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -6116,6 +6116,50 @@ func TestThreadSocketEvents(t *testing.T) { require.Truef(t, caught, "User should have received %s event", model.WebsocketEventThreadUpdated) } }) + + t.Run("Listen for thread updated event after create post when not previously following the thread", func(t *testing.T) { + rpost2 := &model.Post{ChannelId: th.BasicChannel.Id, UserId: th.BasicUser2.Id, Message: "root post"} + + var appErr *model.AppError + rpost2, appErr = th.App.CreatePostAsUser(th.Context, rpost2, th.Context.Session().Id, false) + require.Nil(t, appErr) + + reply1 := &model.Post{ChannelId: th.BasicChannel.Id, UserId: th.BasicUser2.Id, Message: "reply 1", RootId: rpost2.Id} + reply2 := &model.Post{ChannelId: th.BasicChannel.Id, UserId: th.BasicUser2.Id, Message: "reply 2", RootId: rpost2.Id} + reply3 := &model.Post{ChannelId: th.BasicChannel.Id, UserId: th.BasicUser2.Id, Message: "mention @" + th.BasicUser.Username, RootId: rpost2.Id} + + _, appErr = th.App.CreatePostAsUser(th.Context, reply1, th.Context.Session().Id, false) + require.Nil(t, appErr) + _, appErr = th.App.CreatePostAsUser(th.Context, reply2, th.Context.Session().Id, false) + require.Nil(t, appErr) + _, appErr = th.App.CreatePostAsUser(th.Context, reply3, th.Context.Session().Id, false) + require.Nil(t, appErr) + + count := 0 + func() { + for { + select { + case ev := <-userWSClient.EventChannel: + if ev.EventType() == model.WebsocketEventThreadUpdated { + count++ + data := ev.GetData() + var thread model.ThreadResponse + jsonErr := json.Unmarshal([]byte(data["thread"].(string)), &thread) + require.NoError(t, jsonErr) + + require.Equal(t, int64(0), int64(data["previous_unread_replies"].(float64))) + require.Equal(t, int64(0), int64(data["previous_unread_mentions"].(float64))) + require.Equal(t, int64(3), thread.UnreadReplies) + require.Equal(t, int64(1), thread.UnreadMentions) + } + case <-time.After(1 * time.Second): + return + } + } + }() + + require.Equalf(t, 1, count, "User should have received 1 %s event", model.WebsocketEventThreadUpdated) + }) } func TestFollowThreads(t *testing.T) { diff --git a/app/notification.go b/app/notification.go index b1385cbd4c..7e46290e62 100644 --- a/app/notification.go +++ b/app/notification.go @@ -198,6 +198,7 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod mentionedUsersList := make(model.StringArray, 0, len(mentions.Mentions)) mentionAutofollowChans := []chan *model.AppError{} threadParticipants := map[string]bool{post.UserId: true} + newParticipants := map[string]bool{} participantMemberships := map[string]*model.ThreadMembership{} membershipsMutex := &sync.Mutex{} followersMutex := &sync.Mutex{} @@ -246,9 +247,6 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod } if membership != nil && !membership.Following { - membershipsMutex.Lock() - participantMemberships[userID] = membership - membershipsMutex.Unlock() return } } @@ -275,6 +273,7 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod // add new followers to existing followers if threadMembership.Following && !followers.Contains(userID) { followers = append(followers, userID) + newParticipants[userID] = true } followersMutex.Unlock() @@ -595,11 +594,19 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod return nil, errors.Wrapf(err, "cannot get thread %q for user %q", post.RootId, uid) } if userThread != nil { - previousUnreadMentions := userThread.UnreadMentions - previousUnreadReplies := max(userThread.UnreadReplies-1, 0) - if mentions.isUserMentioned(uid) { - previousUnreadMentions = max(userThread.UnreadMentions-1, 0) + previousUnreadMentions := int64(0) + previousUnreadReplies := int64(0) + + // if it's not a newly followed thread, calculate previous unread values. + if !newParticipants[uid] { + previousUnreadMentions = userThread.UnreadMentions + previousUnreadReplies = max(userThread.UnreadReplies-1, 0) + + if mentions.isUserMentioned(uid) { + previousUnreadMentions = max(userThread.UnreadMentions-1, 0) + } } + // set LastViewed to now for commenter if uid == post.UserId { opts := store.ThreadMembershipOpts{