diff --git a/api4/post_test.go b/api4/post_test.go index 9e6f6141b1..ed3400c3ae 100644 --- a/api4/post_test.go +++ b/api4/post_test.go @@ -2621,7 +2621,9 @@ func TestSetPostUnreadWithoutCollapsedThreads(t *testing.T) { // MentionCountRoot should be zero so that supported clients don't show the channel as unread require.Equal(t, channelUnread.MsgCountRoot, int64(0)) - thread, err := th.App.GetThreadForUser(th.BasicUser.Id, th.BasicTeam.Id, rootPost1.Id, false) + threadMembership, err := th.App.GetThreadMembershipForUser(th.BasicUser.Id, rootPost1.Id) + require.Nil(t, err) + thread, err := th.App.GetThreadForUser(th.BasicTeam.Id, threadMembership, false) require.Nil(t, err) require.Equal(t, int64(2), thread.UnreadMentions) require.Equal(t, int64(3), thread.UnreadReplies) diff --git a/api4/user.go b/api4/user.go index c04f9ddba4..6e78f562a8 100644 --- a/api4/user.go +++ b/api4/user.go @@ -2871,15 +2871,21 @@ func getThreadForUser(c *Context, w http.ResponseWriter, r *http.Request) { return } extendedStr := r.URL.Query().Get("extended") - extended, _ := strconv.ParseBool(extendedStr) - threads, err := c.App.GetThreadForUser(c.Params.UserId, c.Params.TeamId, c.Params.ThreadId, extended) + + threadMembership, err := c.App.GetThreadMembershipForUser(c.Params.UserId, c.Params.ThreadId) if err != nil { c.Err = err return } - w.Write([]byte(threads.ToJson())) + thread, err := c.App.GetThreadForUser(c.Params.TeamId, threadMembership, extended) + if err != nil { + c.Err = err + return + } + + w.Write([]byte(thread.ToJson())) } func getThreadsForUser(c *Context, w http.ResponseWriter, r *http.Request) { diff --git a/app/app_iface.go b/app/app_iface.go index bfe2eb5e09..4d9949ba6b 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -759,7 +759,8 @@ type AppIface interface { GetTeamsForUser(userID string) ([]*model.Team, *model.AppError) GetTeamsUnreadForUser(excludeTeamId string, userID string) ([]*model.TeamUnread, *model.AppError) GetTermsOfService(id string) (*model.TermsOfService, *model.AppError) - GetThreadForUser(userID, teamID, threadId string, extended bool) (*model.ThreadResponse, *model.AppError) + GetThreadForUser(teamID string, threadMembership *model.ThreadMembership, extended bool) (*model.ThreadResponse, *model.AppError) + GetThreadMembershipForUser(userId, threadId string) (*model.ThreadMembership, *model.AppError) GetThreadMembershipsForUser(userID, teamID string) ([]*model.ThreadMembership, error) GetThreadsForUser(userID, teamID string, options model.GetUserThreadsOpts) (*model.Threads, *model.AppError) GetUploadSession(uploadId string) (*model.UploadSession, *model.AppError) diff --git a/app/channel.go b/app/channel.go index 055491cf35..424d26241b 100644 --- a/app/channel.go +++ b/app/channel.go @@ -2500,7 +2500,7 @@ func (a *App) MarkChannelAsUnreadFromPost(postID string, userID string, collapse if nErr != nil { return nil, model.NewAppError("MarkChannelAsUnreadFromPost", "app.channel.update_last_viewed_at_post.app_error", nil, nErr.Error(), http.StatusInternalServerError) } - thread, _ := a.Srv().Store.Thread().GetThreadForUser(userID, channel.TeamId, threadId, true) + thread, _ := a.Srv().Store.Thread().GetThreadForUser(channel.TeamId, threadMembership, true) a.sanitizeProfiles(thread.Participants, false) thread.Post.SanitizeProps() @@ -2598,11 +2598,11 @@ func (a *App) markChannelAsUnreadFromPostCRTUnsupported(postID string, userID st if err != nil { return nil, err } - _, nErr = a.Srv().Store.Thread().UpdateMembership(threadMembership) + threadMembership, nErr = a.Srv().Store.Thread().UpdateMembership(threadMembership) if nErr != nil { return nil, model.NewAppError("MarkChannelAsUnreadFromPost", "app.channel.update_last_viewed_at_post.app_error", nil, nErr.Error(), http.StatusInternalServerError) } - thread, nErr := a.Srv().Store.Thread().GetThreadForUser(userID, channel.TeamId, threadId, true) + thread, nErr := a.Srv().Store.Thread().GetThreadForUser(channel.TeamId, threadMembership, 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 758b196a4e..8bb90974d6 100644 --- a/app/notification.go +++ b/app/notification.go @@ -464,7 +464,11 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod } if sendEvent { message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_THREAD_UPDATED, team.Id, "", uid, nil) - userThread, err := a.Srv().Store.Thread().GetThreadForUser(uid, channel.TeamId, post.RootId, true) + threadMembership, err := a.Srv().Store.Thread().GetMembershipForUser(uid, post.RootId) + if err != nil { + return nil, errors.Wrapf(err, "cannot get thread membership %q for user %q", post.RootId, uid) + } + userThread, err := a.Srv().Store.Thread().GetThreadForUser(channel.TeamId, threadMembership, true) if err != nil { return nil, errors.Wrapf(err, "cannot get thread %q for user %q", post.RootId, uid) } diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index 39fc59b4fc..e4b1aed8ca 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -9294,7 +9294,7 @@ func (a *OpenTracingAppLayer) GetTermsOfService(id string) (*model.TermsOfServic return resultVar0, resultVar1 } -func (a *OpenTracingAppLayer) GetThreadForUser(userID string, teamID string, threadId string, extended bool) (*model.ThreadResponse, *model.AppError) { +func (a *OpenTracingAppLayer) GetThreadForUser(teamID string, threadMembership *model.ThreadMembership, extended bool) (*model.ThreadResponse, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetThreadForUser") @@ -9306,7 +9306,29 @@ func (a *OpenTracingAppLayer) GetThreadForUser(userID string, teamID string, thr }() defer span.Finish() - resultVar0, resultVar1 := a.app.GetThreadForUser(userID, teamID, threadId, extended) + resultVar0, resultVar1 := a.app.GetThreadForUser(teamID, threadMembership, extended) + + if resultVar1 != nil { + span.LogFields(spanlog.Error(resultVar1)) + ext.Error.Set(span, true) + } + + return resultVar0, resultVar1 +} + +func (a *OpenTracingAppLayer) GetThreadMembershipForUser(userId string, threadId string) (*model.ThreadMembership, *model.AppError) { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetThreadMembershipForUser") + + a.ctx = newCtx + a.app.Srv().Store.SetContext(newCtx) + defer func() { + a.app.Srv().Store.SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0, resultVar1 := a.app.GetThreadMembershipForUser(userId, threadId) if resultVar1 != nil { span.LogFields(spanlog.Error(resultVar1)) diff --git a/app/post_test.go b/app/post_test.go index 70f93aef14..da948924cd 100644 --- a/app/post_test.go +++ b/app/post_test.go @@ -1952,21 +1952,27 @@ func TestFollowThreadSkipsParticipants(t *testing.T) { _, err = th.App.CreatePost(th.Context, &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) + threadMembership, err := th.App.GetThreadMembershipForUser(user.Id, p1.Id) + require.Nil(t, err) + thread, err := th.App.GetThreadForUser(th.BasicTeam.Id, threadMembership, 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(th.Context, &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) + threadMembership, err = th.App.GetThreadMembershipForUser(user.Id, p1.Id) + require.Nil(t, err) + thread, err = th.App.GetThreadForUser(th.BasicTeam.Id, threadMembership, 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) + threadMembership, err = th.App.GetThreadMembershipForUser(user2.Id, p1.Id) + require.Nil(t, err) + thread, err = th.App.GetThreadForUser(th.BasicTeam.Id, threadMembership, 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 { diff --git a/app/user.go b/app/user.go index e7af9d1cf6..4e9e3e97dc 100644 --- a/app/user.go +++ b/app/user.go @@ -2285,8 +2285,19 @@ func (a *App) GetThreadsForUser(userID, teamID string, options model.GetUserThre return threads, nil } -func (a *App) GetThreadForUser(userID, teamID, threadId string, extended bool) (*model.ThreadResponse, *model.AppError) { - thread, err := a.Srv().Store.Thread().GetThreadForUser(userID, teamID, threadId, extended) +func (a *App) GetThreadMembershipForUser(userId, threadId string) (*model.ThreadMembership, *model.AppError) { + threadMembership, err := a.Srv().Store.Thread().GetMembershipForUser(userId, threadId) + if err != nil { + return nil, model.NewAppError("GetThreadMembershipForUser", "app.user.get_thread_membership_for_user.app_error", nil, err.Error(), http.StatusInternalServerError) + } + if threadMembership == nil { + return nil, model.NewAppError("GetThreadMembershipForUser", "app.user.get_thread_membership_for_user.not_found", nil, "thread membership not found/followed", http.StatusNotFound) + } + return threadMembership, nil +} + +func (a *App) GetThreadForUser(teamID string, threadMembership *model.ThreadMembership, extended bool) (*model.ThreadResponse, *model.AppError) { + thread, err := a.Srv().Store.Thread().GetThreadForUser(teamID, threadMembership, extended) if err != nil { return nil, model.NewAppError("GetThreadForUser", "app.user.get_threads_for_user.app_error", nil, err.Error(), http.StatusInternalServerError) } @@ -2349,14 +2360,16 @@ func (a *App) UpdateThreadReadForUser(userID, teamID, threadID string, timestamp return nil, model.NewAppError("UpdateThreadsReadForUser", "app.user.update_threads_read_for_user.app_error", nil, nErr.Error(), http.StatusInternalServerError) } + membership.LastViewed = timestamp nErr = a.Srv().Store.Thread().MarkAsRead(userID, threadID, timestamp) if nErr != nil { return nil, model.NewAppError("UpdateThreadReadForUser", "app.user.update_thread_read_for_user.app_error", nil, nErr.Error(), http.StatusInternalServerError) } - thread, err := a.GetThreadForUser(userID, teamID, threadID, false) + thread, err := a.GetThreadForUser(teamID, membership, false) if err != nil { return nil, err } + message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_THREAD_READ_CHANGED, teamID, "", userID, nil) message.Add("thread_id", threadID) message.Add("timestamp", timestamp) diff --git a/i18n/en.json b/i18n/en.json index 239fa48b72..4c0371c82b 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -6354,6 +6354,14 @@ "id": "app.user.get_recently_active_users.app_error", "translation": "We encountered an error while finding the recently active users." }, + { + "id": "app.user.get_thread_membership_for_user.app_error", + "translation": "Unable to get user thread membership" + }, + { + "id": "app.user.get_thread_membership_for_user.not_found", + "translation": "User thread membership doesn't exist" + }, { "id": "app.user.get_threads_for_user.app_error", "translation": "Unable to get user threads" diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index 2ef44c99b8..2587d5fa9a 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -8866,7 +8866,7 @@ func (s *OpenTracingLayerThreadStore) GetThreadFollowers(threadID string) ([]str return result, err } -func (s *OpenTracingLayerThreadStore) GetThreadForUser(userID string, teamID string, threadId string, extended bool) (*model.ThreadResponse, error) { +func (s *OpenTracingLayerThreadStore) GetThreadForUser(teamID string, threadMembership *model.ThreadMembership, extended bool) (*model.ThreadResponse, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ThreadStore.GetThreadForUser") s.Root.Store.SetContext(newCtx) @@ -8875,7 +8875,7 @@ func (s *OpenTracingLayerThreadStore) GetThreadForUser(userID string, teamID str }() defer span.Finish() - result, err := s.ThreadStore.GetThreadForUser(userID, teamID, threadId, extended) + result, err := s.ThreadStore.GetThreadForUser(teamID, threadMembership, extended) 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 aeb32fe74e..68f1572058 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -9648,11 +9648,11 @@ func (s *RetryLayerThreadStore) GetThreadFollowers(threadID string) ([]string, e } -func (s *RetryLayerThreadStore) GetThreadForUser(userID string, teamID string, threadId string, extended bool) (*model.ThreadResponse, error) { +func (s *RetryLayerThreadStore) GetThreadForUser(teamID string, threadMembership *model.ThreadMembership, extended bool) (*model.ThreadResponse, error) { tries := 0 for { - result, err := s.ThreadStore.GetThreadForUser(userID, teamID, threadId, extended) + result, err := s.ThreadStore.GetThreadForUser(teamID, threadMembership, extended) if err == nil { return result, nil } diff --git a/store/sqlstore/thread_store.go b/store/sqlstore/thread_store.go index 41fb383020..ef7d5f1237 100644 --- a/store/sqlstore/thread_store.go +++ b/store/sqlstore/thread_store.go @@ -321,7 +321,11 @@ func (s *SqlThreadStore) GetThreadFollowers(threadID string) ([]string, error) { return users, nil } -func (s *SqlThreadStore) GetThreadForUser(userId, teamId, threadId string, extended bool) (*model.ThreadResponse, error) { +func (s *SqlThreadStore) GetThreadForUser(teamId string, threadMembership *model.ThreadMembership, extended bool) (*model.ThreadResponse, error) { + if !threadMembership.Following { + return nil, nil // in case the thread is not followed anymore - return nil error to be interpreted as 404 + } + type JoinedThread struct { PostId string Following bool @@ -334,38 +338,45 @@ func (s *SqlThreadStore) GetThreadForUser(userId, teamId, threadId string, exten model.Post } - unreadRepliesQuery := "SELECT COUNT(Posts.Id) From Posts Where Posts.RootId=ThreadMemberships.PostId AND Posts.CreateAt >= ThreadMemberships.LastViewed AND Posts.DeleteAt=0" + unreadRepliesQuery, unreadRepliesArgs := sq. + Select("COUNT(Posts.Id)"). + From("Posts"). + Where(sq.And{ + sq.Eq{"Posts.RootId": threadMembership.PostId}, + sq.GtOrEq{"Posts.CreateAt": threadMembership.LastViewed}, + sq.Eq{"Posts.DeleteAt": 0}, + }).MustSql() + fetchConditions := sq.And{ sq.Or{sq.Eq{"Channels.TeamId": teamId}, sq.Eq{"Channels.TeamId": ""}}, - sq.Eq{"ThreadMemberships.UserId": userId}, - sq.Eq{"Threads.PostId": threadId}, + sq.Eq{"Threads.PostId": threadMembership.PostId}, } var thread JoinedThread - query, args, _ := s.getQueryBuilder(). - Select("Threads.*, Posts.*, ThreadMemberships.LastViewed as LastViewedAt, ThreadMemberships.UnreadMentions as UnreadMentions, ThreadMemberships.Following"). + query, threadArgs, _ := s.getQueryBuilder(). + Select("Threads.*, Posts.*"). From("Threads"). Column(sq.Alias(sq.Expr(unreadRepliesQuery), "UnreadReplies")). LeftJoin("Posts ON Posts.Id = Threads.PostId"). LeftJoin("Channels ON Posts.ChannelId = Channels.Id"). - LeftJoin("ThreadMemberships ON ThreadMemberships.PostId = Threads.PostId"). Where(fetchConditions).ToSql() - err := s.GetReplica().SelectOne(&thread, query, args...) + args := append(unreadRepliesArgs, threadArgs...) + + err := s.GetReplica().SelectOne(&thread, query, args...) if err != nil { return nil, err } - if !thread.Following { - return nil, nil // in case the thread is not followed anymore - return nil error to be interpreted as 404 - } + thread.LastViewedAt = threadMembership.LastViewed + thread.UnreadMentions = threadMembership.UnreadMentions var users []*model.User if extended { var err error users, err = s.User().GetProfileByIds(context.Background(), thread.Participants, &store.UserGetByIdsOpts{}, true) if err != nil { - return nil, errors.Wrapf(err, "failed to get threads for user id=%s", userId) + return nil, errors.Wrapf(err, "failed to get thread for user id=%s", threadMembership.UserId) } } else { for _, userId := range thread.Participants { diff --git a/store/store.go b/store/store.go index c6193451c5..4fbbfc2fcb 100644 --- a/store/store.go +++ b/store/store.go @@ -283,7 +283,7 @@ type ThreadStore interface { Update(thread *model.Thread) (*model.Thread, error) Get(id string) (*model.Thread, error) GetThreadsForUser(userId, teamID string, opts model.GetUserThreadsOpts) (*model.Threads, error) - GetThreadForUser(userID, teamID, threadId string, extended bool) (*model.ThreadResponse, error) + GetThreadForUser(teamID string, threadMembership *model.ThreadMembership, extended bool) (*model.ThreadResponse, error) Delete(postID string) error GetPosts(threadID string, since int64) ([]*model.Post, error) diff --git a/store/storetest/mocks/ThreadStore.go b/store/storetest/mocks/ThreadStore.go index 345602ddee..339a761808 100644 --- a/store/storetest/mocks/ThreadStore.go +++ b/store/storetest/mocks/ThreadStore.go @@ -180,13 +180,13 @@ func (_m *ThreadStore) GetThreadFollowers(threadID string) ([]string, error) { return r0, r1 } -// GetThreadForUser provides a mock function with given fields: userID, teamID, threadId, extended -func (_m *ThreadStore) GetThreadForUser(userID string, teamID string, threadId string, extended bool) (*model.ThreadResponse, error) { - ret := _m.Called(userID, teamID, threadId, extended) +// GetThreadForUser provides a mock function with given fields: teamID, threadMembership, extended +func (_m *ThreadStore) GetThreadForUser(teamID string, threadMembership *model.ThreadMembership, extended bool) (*model.ThreadResponse, error) { + ret := _m.Called(teamID, threadMembership, extended) var r0 *model.ThreadResponse - if rf, ok := ret.Get(0).(func(string, string, string, bool) *model.ThreadResponse); ok { - r0 = rf(userID, teamID, threadId, extended) + if rf, ok := ret.Get(0).(func(string, *model.ThreadMembership, bool) *model.ThreadResponse); ok { + r0 = rf(teamID, threadMembership, extended) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*model.ThreadResponse) @@ -194,8 +194,8 @@ func (_m *ThreadStore) GetThreadForUser(userID string, teamID string, threadId s } var r1 error - if rf, ok := ret.Get(1).(func(string, string, string, bool) error); ok { - r1 = rf(userID, teamID, threadId, extended) + if rf, ok := ret.Get(1).(func(string, *model.ThreadMembership, bool) error); ok { + r1 = rf(teamID, threadMembership, extended) } else { r1 = ret.Error(1) } diff --git a/store/storetest/thread_store.go b/store/storetest/thread_store.go index 43bab5ba24..df1775cc06 100644 --- a/store/storetest/thread_store.go +++ b/store/storetest/thread_store.go @@ -339,14 +339,14 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) { newPosts := makeSomePosts() m, err := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, true, false, true, false, false) require.NoError(t, err) - th, err := ss.Thread().GetThreadForUser(newPosts[0].UserId, "", newPosts[0].Id, false) + th, err := ss.Thread().GetThreadForUser("", m, false) require.NoError(t, err) require.Equal(t, int64(2), th.UnreadReplies) m.LastViewed = newPosts[2].UpdateAt + 1 _, err = ss.Thread().UpdateMembership(m) require.NoError(t, err) - th, err = ss.Thread().GetThreadForUser(newPosts[0].UserId, "", newPosts[0].Id, false) + th, err = ss.Thread().GetThreadForUser("", m, false) require.NoError(t, err) require.Equal(t, int64(0), th.UnreadReplies) @@ -355,7 +355,7 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) { _, err = ss.Post().Update(editedPost, newPosts[2]) require.NoError(t, err) - th, err = ss.Thread().GetThreadForUser(newPosts[0].UserId, "", newPosts[0].Id, false) + th, err = ss.Thread().GetThreadForUser("", m, false) require.NoError(t, err) require.Equal(t, int64(0), th.UnreadReplies) }) diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index 1abca40c11..71c5f4cc28 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -7990,10 +7990,10 @@ func (s *TimerLayerThreadStore) GetThreadFollowers(threadID string) ([]string, e return result, err } -func (s *TimerLayerThreadStore) GetThreadForUser(userID string, teamID string, threadId string, extended bool) (*model.ThreadResponse, error) { +func (s *TimerLayerThreadStore) GetThreadForUser(teamID string, threadMembership *model.ThreadMembership, extended bool) (*model.ThreadResponse, error) { start := timemodule.Now() - result, err := s.ThreadStore.GetThreadForUser(userID, teamID, threadId, extended) + result, err := s.ThreadStore.GetThreadForUser(teamID, threadMembership, extended) elapsed := float64(timemodule.Since(start)) / float64(timemodule.Second) if s.Root.Metrics != nil {