From 90e7c5a8529bea5ab405c5ea33a9982826033365 Mon Sep 17 00:00:00 2001 From: Eli Yukelzon Date: Tue, 9 Mar 2021 16:10:47 +0200 Subject: [PATCH] MM-33359 corrected unread replies update (#17068) Co-authored-by: Mattermod --- app/channel.go | 2 +- app/notification.go | 15 +++----- app/post.go | 6 ---- app/user.go | 2 +- store/opentracinglayer/opentracinglayer.go | 36 +++++++++---------- store/retrylayer/retrylayer.go | 40 +++++++++++----------- store/sqlstore/thread_store.go | 14 ++++++-- store/store.go | 2 +- store/storetest/mocks/ThreadStore.go | 28 +++++++-------- store/storetest/mocks/dbSelecter.go | 38 ++++++++++++++++++++ store/storetest/thread_store.go | 22 +++++++++--- store/timerlayer/timerlayer.go | 32 ++++++++--------- 12 files changed, 142 insertions(+), 95 deletions(-) create mode 100644 store/storetest/mocks/dbSelecter.go diff --git a/app/channel.go b/app/channel.go index 2d6d138d3f..54655f981d 100644 --- a/app/channel.go +++ b/app/channel.go @@ -2378,7 +2378,7 @@ func (a *App) MarkChannelAsUnreadFromPost(postID string, userID string) (*model. } threadMembership, _ := a.Srv().Store.Thread().GetMembershipForUser(user.Id, threadId) - if threadMembership != nil { + if 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 b7b86fa460..db311ef923 100644 --- a/app/notification.go +++ b/app/notification.go @@ -183,16 +183,10 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod mac := make(chan *model.AppError, 1) go func(userID string) { defer close(mac) - incrementMentions := false - for mid := range mentions.Mentions { - if userID == mid { - incrementMentions = true - break - } - } - nErr := a.Srv().Store.Thread().CreateMembershipIfNeeded(userID, post.RootId, true, incrementMentions, *a.Config().ServiceSettings.ThreadAutoFollow) - if nErr != nil { - mac <- model.NewAppError("SendNotifications", "app.channel.autofollow.app_error", nil, nErr.Error(), http.StatusInternalServerError) + _, incrementMentions := mentions.Mentions[userID] + err := a.Srv().Store.Thread().MaintainMembership(userID, post.RootId, true, incrementMentions, *a.Config().ServiceSettings.ThreadAutoFollow, userID == post.UserId) + if err != nil { + mac <- model.NewAppError("SendNotifications", "app.channel.autofollow.app_error", nil, err.Error(), http.StatusInternalServerError) return } @@ -439,7 +433,6 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod sendEvent = preference.Value == "on" } if sendEvent { - message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_THREAD_UPDATED, team.Id, "", uid, nil) userThread, _ := a.Srv().Store.Thread().GetThreadForUser(uid, channel.TeamId, thread.PostId, true) a.sanitizeProfiles(userThread.Participants, false) diff --git a/app/post.go b/app/post.go index fc6f94d8c8..20b87512c9 100644 --- a/app/post.go +++ b/app/post.go @@ -458,12 +458,6 @@ func (a *App) handlePostEvents(post *model.Post, user *model.User, channel *mode return err } - if *a.Config().ServiceSettings.ThreadAutoFollow && post.RootId != "" { - if err := a.Srv().Store.Thread().CreateMembershipIfNeeded(post.UserId, post.RootId, true, false, true); err != nil { - return err - } - } - if post.Type != model.POST_AUTO_RESPONDER { // don't respond to an auto-responder a.Srv().Go(func() { _, err := a.SendAutoResponseIfNecessary(channel, user, post) diff --git a/app/user.go b/app/user.go index 2f0f1dc13e..52cc519c28 100644 --- a/app/user.go +++ b/app/user.go @@ -2403,7 +2403,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().CreateMembershipIfNeeded(userID, threadID, state, false, true) + err := a.Srv().Store.Thread().MaintainMembership(userID, threadID, state, false, true, 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 d8b1604319..4b74fa3e05 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -7738,24 +7738,6 @@ func (s *OpenTracingLayerThreadStore) CollectThreadsWithNewerReplies(userId stri return result, err } -func (s *OpenTracingLayerThreadStore) CreateMembershipIfNeeded(userId string, postID string, following bool, incrementMentions bool, updateFollowing bool) error { - origCtx := s.Root.Store.Context() - span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ThreadStore.CreateMembershipIfNeeded") - s.Root.Store.SetContext(newCtx) - defer func() { - s.Root.Store.SetContext(origCtx) - }() - - defer span.Finish() - err := s.ThreadStore.CreateMembershipIfNeeded(userId, postID, following, incrementMentions, updateFollowing) - if err != nil { - span.LogFields(spanlog.Error(err)) - ext.Error.Set(span, true) - } - - return err -} - func (s *OpenTracingLayerThreadStore) Delete(postId string) error { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ThreadStore.Delete") @@ -7918,6 +7900,24 @@ 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) error { + origCtx := s.Root.Store.Context() + span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ThreadStore.MaintainMembership") + s.Root.Store.SetContext(newCtx) + defer func() { + s.Root.Store.SetContext(origCtx) + }() + + defer span.Finish() + err := s.ThreadStore.MaintainMembership(userId, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp) + if err != nil { + span.LogFields(spanlog.Error(err)) + ext.Error.Set(span, true) + } + + return err +} + func (s *OpenTracingLayerThreadStore) MarkAllAsRead(userId string, teamID string) error { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ThreadStore.MarkAllAsRead") diff --git a/store/retrylayer/retrylayer.go b/store/retrylayer/retrylayer.go index db90c20ec7..405f07b39f 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -8398,26 +8398,6 @@ func (s *RetryLayerThreadStore) CollectThreadsWithNewerReplies(userId string, ch } -func (s *RetryLayerThreadStore) CreateMembershipIfNeeded(userId string, postID string, following bool, incrementMentions bool, updateFollowing bool) error { - - tries := 0 - for { - err := s.ThreadStore.CreateMembershipIfNeeded(userId, postID, following, incrementMentions, updateFollowing) - if err == nil { - return nil - } - if !isRepeatableError(err) { - return err - } - tries++ - if tries >= 3 { - err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") - return err - } - } - -} - func (s *RetryLayerThreadStore) Delete(postId string) error { tries := 0 @@ -8598,6 +8578,26 @@ func (s *RetryLayerThreadStore) GetThreadsForUser(userId string, teamId string, } +func (s *RetryLayerThreadStore) MaintainMembership(userId string, postID string, following bool, incrementMentions bool, updateFollowing bool, updateViewedTimestamp bool) error { + + tries := 0 + for { + err := s.ThreadStore.MaintainMembership(userId, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp) + if err == nil { + return nil + } + if !isRepeatableError(err) { + return err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return err + } + } + +} + func (s *RetryLayerThreadStore) MarkAllAsRead(userId string, teamID string) error { tries := 0 diff --git a/store/sqlstore/thread_store.go b/store/sqlstore/thread_store.go index 7cf96a0c33..2850329c73 100644 --- a/store/sqlstore/thread_store.go +++ b/store/sqlstore/thread_store.go @@ -501,14 +501,22 @@ func (s *SqlThreadStore) DeleteMembershipForUser(userId string, postId string) e return nil } -func (s *SqlThreadStore) CreateMembershipIfNeeded(userId, postId string, following, incrementMentions, updateFollowing bool) error { +func (s *SqlThreadStore) MaintainMembership(userId, postId string, following, incrementMentions, updateFollowing, updateViewedTimestamp bool) error { membership, err := s.GetMembershipForUser(userId, postId) now := utils.MillisFromTime(time.Now()) + // if memebership exists, update it if: + // a. user started/stopped following a thread + // b. mention count changed + // c. user viewed a thread if err == nil { - if (updateFollowing && !membership.Following || membership.Following != following) || incrementMentions { - if updateFollowing { + followingNeedsUpdate := (updateFollowing && !membership.Following || membership.Following != following) + if followingNeedsUpdate || incrementMentions || updateViewedTimestamp { + if followingNeedsUpdate { membership.Following = following } + if updateViewedTimestamp { + membership.LastViewed = now + } membership.LastUpdated = now if incrementMentions { membership.UnreadMentions += 1 diff --git a/store/store.go b/store/store.go index ae46951503..7df0890659 100644 --- a/store/store.go +++ b/store/store.go @@ -265,7 +265,7 @@ type ThreadStore interface { GetMembershipsForUser(userId, teamID string) ([]*model.ThreadMembership, error) GetMembershipForUser(userId, postID string) (*model.ThreadMembership, error) DeleteMembershipForUser(userId, postID string) error - CreateMembershipIfNeeded(userId, postID string, following, incrementMentions, updateFollowing bool) error + MaintainMembership(userId, postID string, following, incrementMentions, updateFollowing, updateViewedTimestamp bool) 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 f747e8ad28..5099684ab4 100644 --- a/store/storetest/mocks/ThreadStore.go +++ b/store/storetest/mocks/ThreadStore.go @@ -37,20 +37,6 @@ func (_m *ThreadStore) CollectThreadsWithNewerReplies(userId string, channelIds return r0, r1 } -// CreateMembershipIfNeeded provides a mock function with given fields: userId, postID, following, incrementMentions, updateFollowing -func (_m *ThreadStore) CreateMembershipIfNeeded(userId string, postID string, following bool, incrementMentions bool, updateFollowing bool) error { - ret := _m.Called(userId, postID, following, incrementMentions, updateFollowing) - - var r0 error - if rf, ok := ret.Get(0).(func(string, string, bool, bool, bool) error); ok { - r0 = rf(userId, postID, following, incrementMentions, updateFollowing) - } else { - r0 = ret.Error(0) - } - - return r0 -} - // Delete provides a mock function with given fields: postId func (_m *ThreadStore) Delete(postId string) error { ret := _m.Called(postId) @@ -240,6 +226,20 @@ 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) error { + ret := _m.Called(userId, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp) + + var r0 error + if rf, ok := ret.Get(0).(func(string, string, bool, bool, bool, bool) error); ok { + r0 = rf(userId, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // MarkAllAsRead provides a mock function with given fields: userId, teamID func (_m *ThreadStore) MarkAllAsRead(userId string, teamID string) error { ret := _m.Called(userId, teamID) diff --git a/store/storetest/mocks/dbSelecter.go b/store/storetest/mocks/dbSelecter.go new file mode 100644 index 0000000000..8343daf52a --- /dev/null +++ b/store/storetest/mocks/dbSelecter.go @@ -0,0 +1,38 @@ +// Code generated by mockery v1.0.0. DO NOT EDIT. + +// Regenerate this file using `make store-mocks`. + +package mocks + +import mock "github.com/stretchr/testify/mock" + +// dbSelecter is an autogenerated mock type for the dbSelecter type +type dbSelecter struct { + mock.Mock +} + +// Select provides a mock function with given fields: i, query, args +func (_m *dbSelecter) Select(i interface{}, query string, args ...interface{}) ([]interface{}, error) { + var _ca []interface{} + _ca = append(_ca, i, query) + _ca = append(_ca, args...) + ret := _m.Called(_ca...) + + var r0 []interface{} + if rf, ok := ret.Get(0).(func(interface{}, string, ...interface{}) []interface{}); ok { + r0 = rf(i, query, args...) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]interface{}) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(interface{}, string, ...interface{}) error); ok { + r1 = rf(i, query, args...) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} diff --git a/store/storetest/thread_store.go b/store/storetest/thread_store.go index 11774bfb40..948d05ecd0 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() - require.NoError(t, ss.Thread().CreateMembershipIfNeeded(newPosts[0].UserId, newPosts[0].Id, true, false, true)) + require.NoError(t, ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false)) m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id) require.NoError(t, err1) m.LastUpdated -= 1000 @@ -255,7 +255,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() - require.NoError(t, ss.Thread().CreateMembershipIfNeeded(newPosts[0].UserId, newPosts[0].Id, true, false, true)) + require.NoError(t, ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false)) m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id) require.NoError(t, err1) m.LastUpdated -= 1000 @@ -275,7 +275,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() - require.NoError(t, ss.Thread().CreateMembershipIfNeeded(newPosts[0].UserId, newPosts[0].Id, true, false, true)) + require.NoError(t, ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false)) m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id) require.NoError(t, err1) m.LastUpdated -= 1000 @@ -292,10 +292,24 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) { }, time.Second, 10*time.Millisecond) }) + t.Run("Thread membership 'viewed' timestamp is updated properly", func(t *testing.T) { + newPosts := makeSomePosts() + + require.NoError(t, ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false)) + m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id) + require.NoError(t, err1) + require.Equal(t, int64(0), m.LastViewed) + + require.NoError(t, ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, true)) + m2, err2 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id) + require.NoError(t, err2) + require.Greater(t, m2.LastViewed, int64(0)) + }) + t.Run("Thread last updated is changed when channel is updated after UpdateLastViewedAtPost for mark unread", func(t *testing.T) { newPosts := makeSomePosts() - require.NoError(t, ss.Thread().CreateMembershipIfNeeded(newPosts[0].UserId, newPosts[0].Id, true, false, true)) + require.NoError(t, ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false)) m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id) require.NoError(t, err1) m.LastUpdated += 1000 diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index a0ad073ca3..a9140fd332 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -6984,22 +6984,6 @@ func (s *TimerLayerThreadStore) CollectThreadsWithNewerReplies(userId string, ch return result, err } -func (s *TimerLayerThreadStore) CreateMembershipIfNeeded(userId string, postID string, following bool, incrementMentions bool, updateFollowing bool) error { - start := timemodule.Now() - - err := s.ThreadStore.CreateMembershipIfNeeded(userId, postID, following, incrementMentions, updateFollowing) - - elapsed := float64(timemodule.Since(start)) / float64(timemodule.Second) - if s.Root.Metrics != nil { - success := "false" - if err == nil { - success = "true" - } - s.Root.Metrics.ObserveStoreMethodDuration("ThreadStore.CreateMembershipIfNeeded", success, elapsed) - } - return err -} - func (s *TimerLayerThreadStore) Delete(postId string) error { start := timemodule.Now() @@ -7144,6 +7128,22 @@ 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) error { + start := timemodule.Now() + + err := s.ThreadStore.MaintainMembership(userId, postID, following, incrementMentions, updateFollowing, updateViewedTimestamp) + + elapsed := float64(timemodule.Since(start)) / float64(timemodule.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("ThreadStore.MaintainMembership", success, elapsed) + } + return err +} + func (s *TimerLayerThreadStore) MarkAllAsRead(userId string, teamID string) error { start := timemodule.Now()