From 56198a99c6dff314565f1a50d69d2d27ed3e570b Mon Sep 17 00:00:00 2001 From: Eli Yukelzon Date: Wed, 21 Apr 2021 09:48:30 +0300 Subject: [PATCH] MM-34871 CRT: Participants of thread include non-replying followers (and past followers) (#17447) Co-authored-by: Mattermod --- app/channel.go | 5 +- app/notification.go | 2 +- app/post_test.go | 56 +++++++++++++++++++++- app/user.go | 2 +- store/opentracinglayer/opentracinglayer.go | 4 +- store/retrylayer/retrylayer.go | 4 +- store/sqlstore/thread_store.go | 18 +++---- store/store.go | 2 +- store/storetest/mocks/ThreadStore.go | 14 +++--- store/storetest/thread_store.go | 12 ++--- store/timerlayer/timerlayer.go | 4 +- 11 files changed, 89 insertions(+), 34 deletions(-) diff --git a/app/channel.go b/app/channel.go index 020e1d2070..221299eb12 100644 --- a/app/channel.go +++ b/app/channel.go @@ -2388,9 +2388,10 @@ func (a *App) MarkChannelAsUnreadFromPost(postID string, userID string) (*model. threadMembership, _ := a.Srv().Store.Thread().GetMembershipForUser(user.Id, threadId) if threadMembership == nil { - threadMembership, _ = a.Srv().Store.Thread().MaintainMembership(user.Id, threadId, true, true, true, true) + threadMembership, _ = a.Srv().Store.Thread().MaintainMembership(user.Id, threadId, true, true, true, true, false) } - if threadMembership != nil && threadMembership.Following { + threadData, _ := a.Srv().Store.Thread().Get(threadId) + if threadData != nil && threadMembership != nil && threadMembership.Following { channel, nErr := a.Srv().Store.Channel().Get(post.ChannelId, true) if nErr != nil { return nil, model.NewAppError("MarkChannelAsUnreadFromPost", "app.channel.update_last_viewed_at_post.app_error", nil, nErr.Error(), http.StatusInternalServerError) diff --git a/app/notification.go b/app/notification.go index 6450a2708a..911614c6c7 100644 --- a/app/notification.go +++ b/app/notification.go @@ -185,7 +185,7 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod go func(userID string) { defer close(mac) _, incrementMentions := mentions.Mentions[userID] - _, err := a.Srv().Store.Thread().MaintainMembership(userID, post.RootId, true, incrementMentions, *a.Config().ServiceSettings.ThreadAutoFollow, userID == post.UserId) + _, err := a.Srv().Store.Thread().MaintainMembership(userID, post.RootId, true, incrementMentions, *a.Config().ServiceSettings.ThreadAutoFollow, userID == post.UserId, userID == post.UserId) if err != nil { mac <- model.NewAppError("SendNotifications", "app.channel.autofollow.app_error", nil, err.Error(), http.StatusInternalServerError) return diff --git a/app/post_test.go b/app/post_test.go index f28a49b4f6..f838d1deb6 100644 --- a/app/post_test.go +++ b/app/post_test.go @@ -1924,6 +1924,58 @@ func TestThreadMembership(t *testing.T) { }) } +func TestFollowThreadSkipsParticipants(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + os.Setenv("MM_FEATUREFLAGS_COLLAPSEDTHREADS", "true") + defer os.Unsetenv("MM_FEATUREFLAGS_COLLAPSEDTHREADS") + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.ThreadAutoFollow = true + *cfg.ServiceSettings.CollapsedThreads = model.COLLAPSED_THREADS_DEFAULT_ON + }) + + channel := th.BasicChannel + user := th.BasicUser + user2 := th.BasicUser2 + sysadmin := th.SystemAdminUser + + appErr := th.App.JoinChannel(channel, user.Id) + require.Nil(t, appErr) + appErr = th.App.JoinChannel(channel, user2.Id) + require.Nil(t, appErr) + _, appErr = th.App.JoinUserToTeam(th.BasicTeam, sysadmin, sysadmin.Id) + require.Nil(t, appErr) + appErr = th.App.JoinChannel(channel, sysadmin.Id) + require.Nil(t, appErr) + + p1, err := th.App.CreatePost(&model.Post{UserId: user.Id, ChannelId: channel.Id, Message: "Hi @" + sysadmin.Username}, channel, false, false) + require.Nil(t, err) + _, err = th.App.CreatePost(&model.Post{RootId: p1.Id, UserId: user.Id, ChannelId: channel.Id, Message: "Hola"}, channel, false, false) + require.Nil(t, err) + + thread, err := th.App.GetThreadForUser(user.Id, th.BasicTeam.Id, p1.Id, false) + require.Nil(t, err) + require.Len(t, thread.Participants, 1) // length should be 1, the original poster, since sysadmin was just mentioned but didn't post + + _, err = th.App.CreatePost(&model.Post{RootId: p1.Id, UserId: sysadmin.Id, ChannelId: channel.Id, Message: "sysadmin reply"}, channel, false, false) + require.Nil(t, err) + + thread, err = th.App.GetThreadForUser(user.Id, th.BasicTeam.Id, p1.Id, false) + require.Nil(t, err) + require.Len(t, thread.Participants, 2) // length should be 2, the original poster and sysadmin, since sysadmin participated now + + // another user follows the thread + th.App.UpdateThreadFollowForUser(user2.Id, th.BasicTeam.Id, p1.Id, true) + + thread, err = th.App.GetThreadForUser(user2.Id, th.BasicTeam.Id, p1.Id, false) + require.Nil(t, err) + require.Len(t, thread.Participants, 2) // length should be 2, since follow shouldn't update participant list, only user1 and sysadmin are participants + for _, p := range thread.Participants { + require.True(t, p.Id == sysadmin.Id || p.Id == user.Id) + } +} + func TestAutofollowBasedOnRootPost(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() @@ -1986,13 +2038,13 @@ func TestCollapsedThreadFetch(t *testing.T) { require.Nil(t, err) thread, nErr := th.App.Srv().Store.Thread().Get(postRoot.Id) require.NoError(t, nErr) - require.Len(t, thread.Participants, 2) + require.Len(t, thread.Participants, 1) th.App.MarkChannelAsUnreadFromPost(postRoot.Id, user1.Id) l, err := th.App.GetPostsForChannelAroundLastUnread(channel.Id, user1.Id, 10, 10, true, true, false) require.Nil(t, err) require.Len(t, l.Order, 1) require.EqualValues(t, 1, l.Posts[postRoot.Id].ReplyCount) - require.EqualValues(t, []string{user1.Id, user2.Id}, []string{l.Posts[postRoot.Id].Participants[0].Id, l.Posts[postRoot.Id].Participants[1].Id}) + require.EqualValues(t, []string{user1.Id}, []string{l.Posts[postRoot.Id].Participants[0].Id}) require.Empty(t, l.Posts[postRoot.Id].Participants[0].Email) require.NotZero(t, l.Posts[postRoot.Id].LastReplyAt) require.True(t, l.Posts[postRoot.Id].IsFollowing) diff --git a/app/user.go b/app/user.go index 2bd2779207..e2b44d6343 100644 --- a/app/user.go +++ b/app/user.go @@ -2428,7 +2428,7 @@ func (a *App) UpdateThreadsReadForUser(userID, teamID string) *model.AppError { } func (a *App) UpdateThreadFollowForUser(userID, teamID, threadID string, state bool) *model.AppError { - _, err := a.Srv().Store.Thread().MaintainMembership(userID, threadID, state, false, true, false) + _, err := a.Srv().Store.Thread().MaintainMembership(userID, threadID, state, false, true, false, false) if err != nil { return model.NewAppError("UpdateThreadFollowForUser", "app.user.update_thread_follow_for_user.app_error", nil, err.Error(), http.StatusInternalServerError) } diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index dc39ba0349..545ed01bd0 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -8812,7 +8812,7 @@ func (s *OpenTracingLayerThreadStore) GetThreadsForUser(userId string, teamID st return result, err } -func (s *OpenTracingLayerThreadStore) MaintainMembership(userID string, postID string, following bool, incrementMentions bool, updateFollowing bool, updateViewedTimestamp bool) (*model.ThreadMembership, error) { +func (s *OpenTracingLayerThreadStore) MaintainMembership(userID string, postID string, following bool, incrementMentions bool, updateFollowing bool, updateViewedTimestamp bool, updateParticipants bool) (*model.ThreadMembership, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ThreadStore.MaintainMembership") s.Root.Store.SetContext(newCtx) @@ -8821,7 +8821,7 @@ func (s *OpenTracingLayerThreadStore) MaintainMembership(userID string, postID s }() defer span.Finish() - result, err := s.ThreadStore.MaintainMembership(userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp) + result, err := s.ThreadStore.MaintainMembership(userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp, updateParticipants) if err != nil { span.LogFields(spanlog.Error(err)) ext.Error.Set(span, true) diff --git a/store/retrylayer/retrylayer.go b/store/retrylayer/retrylayer.go index aad669102d..aea4df3b78 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -9588,11 +9588,11 @@ func (s *RetryLayerThreadStore) GetThreadsForUser(userId string, teamID string, } -func (s *RetryLayerThreadStore) MaintainMembership(userID string, postID string, following bool, incrementMentions bool, updateFollowing bool, updateViewedTimestamp bool) (*model.ThreadMembership, error) { +func (s *RetryLayerThreadStore) MaintainMembership(userID string, postID string, following bool, incrementMentions bool, updateFollowing bool, updateViewedTimestamp bool, updateParticipants bool) (*model.ThreadMembership, error) { tries := 0 for { - result, err := s.ThreadStore.MaintainMembership(userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp) + result, err := s.ThreadStore.MaintainMembership(userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp, updateParticipants) if err == nil { return result, nil } diff --git a/store/sqlstore/thread_store.go b/store/sqlstore/thread_store.go index b318a12205..56acbd128a 100644 --- a/store/sqlstore/thread_store.go +++ b/store/sqlstore/thread_store.go @@ -472,7 +472,7 @@ func (s *SqlThreadStore) DeleteMembershipForUser(userId string, postId string) e return nil } -func (s *SqlThreadStore) MaintainMembership(userId, postId string, following, incrementMentions, updateFollowing, updateViewedTimestamp bool) (*model.ThreadMembership, error) { +func (s *SqlThreadStore) MaintainMembership(userId, postId string, following, incrementMentions, updateFollowing, updateViewedTimestamp, updateParticipants bool) (*model.ThreadMembership, error) { membership, err := s.GetMembershipForUser(userId, postId) now := utils.MillisFromTime(time.Now()) // if memebership exists, update it if: @@ -518,13 +518,15 @@ func (s *SqlThreadStore) MaintainMembership(userId, postId string, following, in return nil, err } - thread, err := s.Get(postId) - if err != nil { - return nil, err - } - if !thread.Participants.Contains(userId) { - thread.Participants = append(thread.Participants, userId) - _, err = s.Update(thread) + if updateParticipants { + thread, err2 := s.Get(postId) + if err2 != nil { + return nil, err2 + } + if !thread.Participants.Contains(userId) { + thread.Participants = append(thread.Participants, userId) + _, err = s.Update(thread) + } } return membership, err } diff --git a/store/store.go b/store/store.go index 514e5fdde5..14ee7fa9e9 100644 --- a/store/store.go +++ b/store/store.go @@ -289,7 +289,7 @@ type ThreadStore interface { GetMembershipsForUser(userId, teamID string) ([]*model.ThreadMembership, error) GetMembershipForUser(userId, postID string) (*model.ThreadMembership, error) DeleteMembershipForUser(userId, postID string) error - MaintainMembership(userID, postID string, following, incrementMentions, updateFollowing, updateViewedTimestamp bool) (*model.ThreadMembership, error) + MaintainMembership(userID, postID string, following, incrementMentions, updateFollowing, updateViewedTimestamp, updateParticipants bool) (*model.ThreadMembership, error) CollectThreadsWithNewerReplies(userId string, channelIds []string, timestamp int64) ([]string, error) UpdateUnreadsByChannel(userId string, changedThreads []string, timestamp int64, updateViewedTimestamp bool) error } diff --git a/store/storetest/mocks/ThreadStore.go b/store/storetest/mocks/ThreadStore.go index c6e03c3a23..e32b7a08aa 100644 --- a/store/storetest/mocks/ThreadStore.go +++ b/store/storetest/mocks/ThreadStore.go @@ -203,13 +203,13 @@ func (_m *ThreadStore) GetThreadsForUser(userId string, teamID string, opts mode return r0, r1 } -// MaintainMembership provides a mock function with given fields: userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp -func (_m *ThreadStore) MaintainMembership(userID string, postID string, following bool, incrementMentions bool, updateFollowing bool, updateViewedTimestamp bool) (*model.ThreadMembership, error) { - ret := _m.Called(userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp) +// MaintainMembership provides a mock function with given fields: userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp, updateParticipants +func (_m *ThreadStore) MaintainMembership(userID string, postID string, following bool, incrementMentions bool, updateFollowing bool, updateViewedTimestamp bool, updateParticipants bool) (*model.ThreadMembership, error) { + ret := _m.Called(userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp, updateParticipants) var r0 *model.ThreadMembership - if rf, ok := ret.Get(0).(func(string, string, bool, bool, bool, bool) *model.ThreadMembership); ok { - r0 = rf(userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp) + if rf, ok := ret.Get(0).(func(string, string, bool, bool, bool, bool, bool) *model.ThreadMembership); ok { + r0 = rf(userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp, updateParticipants) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*model.ThreadMembership) @@ -217,8 +217,8 @@ func (_m *ThreadStore) MaintainMembership(userID string, postID string, followin } var r1 error - if rf, ok := ret.Get(1).(func(string, string, bool, bool, bool, bool) error); ok { - r1 = rf(userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp) + if rf, ok := ret.Get(1).(func(string, string, bool, bool, bool, bool, bool) error); ok { + r1 = rf(userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp, updateParticipants) } else { r1 = ret.Error(1) } diff --git a/store/storetest/thread_store.go b/store/storetest/thread_store.go index 1f63c21af0..073b102a34 100644 --- a/store/storetest/thread_store.go +++ b/store/storetest/thread_store.go @@ -235,7 +235,7 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) { t.Run("Thread last updated is changed when channel is updated after UpdateLastViewedAtPost", func(t *testing.T) { newPosts := makeSomePosts() - _, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false) + _, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false, false) require.NoError(t, e) m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id) require.NoError(t, err1) @@ -256,7 +256,7 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) { t.Run("Thread last updated is changed when channel is updated after IncrementMentionCount", func(t *testing.T) { newPosts := makeSomePosts() - _, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false) + _, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false, false) require.NoError(t, e) m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id) require.NoError(t, err1) @@ -276,7 +276,7 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) { t.Run("Thread last updated is changed when channel is updated after UpdateLastViewedAt", func(t *testing.T) { newPosts := makeSomePosts() - _, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false) + _, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false, false) require.NoError(t, e) m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id) require.NoError(t, err1) @@ -297,13 +297,13 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) { t.Run("Thread membership 'viewed' timestamp is updated properly", func(t *testing.T) { newPosts := makeSomePosts() - _, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false) + _, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false, false) require.NoError(t, e) m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id) require.NoError(t, err1) require.Equal(t, int64(0), m.LastViewed) - _, e = ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, true) + _, e = ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, true, false) require.NoError(t, e) m2, err2 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id) require.NoError(t, err2) @@ -312,7 +312,7 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) { t.Run("Thread last updated is changed when channel is updated after UpdateLastViewedAtPost for mark unread", func(t *testing.T) { newPosts := makeSomePosts() - _, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false) + _, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false, false) require.NoError(t, e) m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id) require.NoError(t, err1) diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index 7b9c39515b..e362268d3a 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -7942,10 +7942,10 @@ func (s *TimerLayerThreadStore) GetThreadsForUser(userId string, teamID string, return result, err } -func (s *TimerLayerThreadStore) MaintainMembership(userID string, postID string, following bool, incrementMentions bool, updateFollowing bool, updateViewedTimestamp bool) (*model.ThreadMembership, error) { +func (s *TimerLayerThreadStore) MaintainMembership(userID string, postID string, following bool, incrementMentions bool, updateFollowing bool, updateViewedTimestamp bool, updateParticipants bool) (*model.ThreadMembership, error) { start := timemodule.Now() - result, err := s.ThreadStore.MaintainMembership(userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp) + result, err := s.ThreadStore.MaintainMembership(userID, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp, updateParticipants) elapsed := float64(timemodule.Since(start)) / float64(timemodule.Second) if s.Root.Metrics != nil {