From d58b04896586fd0e006872ef943f5a18672fdf4b Mon Sep 17 00:00:00 2001 From: Devin Binnie <52460000+devinbinnie@users.noreply.github.com> Date: Thu, 26 Sep 2024 09:02:11 -0400 Subject: [PATCH] [MM-60603] Don't follow threads when marking them as read on focus (#28263) * [MM-60603] Don't follow threads when marking them as read on focus * Fix tests * Fix lint * Fix the original bug in the API call --- server/channels/api4/user.go | 8 ++++ server/channels/api4/user_test.go | 48 ++++++++++---------- server/channels/app/user.go | 11 ++--- server/channels/app/user_test.go | 44 ++---------------- webapp/channels/src/actions/views/threads.ts | 4 +- 5 files changed, 44 insertions(+), 71 deletions(-) diff --git a/server/channels/api4/user.go b/server/channels/api4/user.go index 60f7faf925..c9eef925ad 100644 --- a/server/channels/api4/user.go +++ b/server/channels/api4/user.go @@ -3344,6 +3344,14 @@ func setUnreadThreadByPostId(c *Context, w http.ResponseWriter, r *http.Request) return } + // We want to make sure the thread is followed when marking as unread + // https://mattermost.atlassian.net/browse/MM-36430 + err := c.App.UpdateThreadFollowForUser(c.Params.UserId, c.Params.TeamId, c.Params.ThreadId, true) + if err != nil { + c.Err = err + return + } + thread, err := c.App.UpdateThreadReadForUserByPost(c.AppContext, c.AppContext.Session().Id, c.Params.UserId, c.Params.TeamId, c.Params.ThreadId, c.Params.PostId) if err != nil { c.Err = err diff --git a/server/channels/api4/user_test.go b/server/channels/api4/user_test.go index 16220e4cdf..75a8ee0ac2 100644 --- a/server/channels/api4/user_test.go +++ b/server/channels/api4/user_test.go @@ -6855,29 +6855,6 @@ func TestThreadSocketEvents(t *testing.T) { require.Truef(t, caught, "User should have received %s event", model.WebsocketEventThreadUpdated) }) - resp, err = th.Client.UpdateThreadFollowForUser(context.Background(), th.BasicUser.Id, th.BasicTeam.Id, rpost.Id, false) - require.NoError(t, err) - CheckOKStatus(t, resp) - - t.Run("Listed for follow event", func(t *testing.T) { - var caught bool - func() { - for { - select { - case ev := <-userWSClient.EventChannel: - if ev.EventType() == model.WebsocketEventThreadFollowChanged { - caught = true - require.Equal(t, ev.GetData()["state"], false) - require.Equal(t, ev.GetData()["reply_count"], float64(1)) - } - case <-time.After(2 * time.Second): - return - } - } - }() - require.Truef(t, caught, "User should have received %s event", model.WebsocketEventThreadFollowChanged) - }) - _, resp, err = th.Client.UpdateThreadReadForUser(context.Background(), th.BasicUser.Id, th.BasicTeam.Id, rpost.Id, replyPost.CreateAt+1) require.NoError(t, err) CheckOKStatus(t, resp) @@ -6907,6 +6884,31 @@ func TestThreadSocketEvents(t *testing.T) { require.Truef(t, caught, "User should have received %s event", model.WebsocketEventThreadReadChanged) }) + resp, err = th.Client.UpdateThreadFollowForUser(context.Background(), th.BasicUser.Id, th.BasicTeam.Id, rpost.Id, false) + require.NoError(t, err) + CheckOKStatus(t, resp) + + t.Run("Listed for follow event", func(t *testing.T) { + var caught bool + func() { + for { + select { + case ev := <-userWSClient.EventChannel: + if ev.EventType() == model.WebsocketEventThreadFollowChanged { + caught = true + require.Equal(t, ev.GetData()["state"], false) + require.Equal(t, ev.GetData()["reply_count"], float64(1)) + } + case <-time.After(2 * time.Second): + return + } + } + }() + require.Truef(t, caught, "User should have received %s event", model.WebsocketEventThreadFollowChanged) + }) + + _, err = th.Client.UpdateThreadFollowForUser(context.Background(), th.BasicUser.Id, th.BasicTeam.Id, rpost.Id, true) + require.NoError(t, err) _, resp, err = th.Client.SetThreadUnreadByPostId(context.Background(), th.BasicUser.Id, th.BasicTeam.Id, rpost.Id, rpost.Id) require.NoError(t, err) CheckOKStatus(t, resp) diff --git a/server/channels/app/user.go b/server/channels/app/user.go index d6a70a38e6..62f9d0004e 100644 --- a/server/channels/app/user.go +++ b/server/channels/app/user.go @@ -2831,13 +2831,10 @@ func (a *App) UpdateThreadReadForUser(c request.CTX, currentSessionId, userID, t return nil, err } - opts := store.ThreadMembershipOpts{ - Following: true, - UpdateFollowing: true, - } - membership, storeErr := a.Srv().Store().Thread().MaintainMembership(userID, threadID, opts) - if storeErr != nil { - return nil, model.NewAppError("UpdateThreadReadForUser", "app.user.update_thread_read_for_user.app_error", nil, "", http.StatusInternalServerError).Wrap(storeErr) + // If the thread doesn't have a membership, we shouldn't try to mark it as unread + membership, err := a.GetThreadMembershipForUser(userID, threadID) + if err != nil { + return nil, err } previousUnreadMentions := membership.UnreadMentions diff --git a/server/channels/app/user_test.go b/server/channels/app/user_test.go index 4c8c49304d..bb9691cf70 100644 --- a/server/channels/app/user_test.go +++ b/server/channels/app/user_test.go @@ -8,7 +8,6 @@ import ( "context" "encoding/json" "errors" - "net/http" "path/filepath" "strings" "testing" @@ -1930,7 +1929,7 @@ func TestPatchUser(t *testing.T) { } func TestUpdateThreadReadForUser(t *testing.T) { - t.Run("Ensure thread membership is created and followed", func(t *testing.T) { + t.Run("Ensure thread membership exists before updating read", func(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() th.App.UpdateConfig(func(cfg *model.Config) { @@ -1947,48 +1946,13 @@ func TestUpdateThreadReadForUser(t *testing.T) { require.Zero(t, threads.Total) _, appErr = th.App.UpdateThreadReadForUser(th.Context, "currentSessionId", th.BasicUser.Id, th.BasicChannel.TeamId, rootPost.Id, replyPost.CreateAt) - require.Nil(t, appErr) - - threads, appErr = th.App.GetThreadsForUser(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{}) - require.Nil(t, appErr) - assert.NotZero(t, threads.Total) - - threadMembership, appErr := th.App.GetThreadMembershipForUser(th.BasicUser.Id, rootPost.Id) - require.Nil(t, appErr) - require.NotNil(t, threadMembership) - assert.True(t, threadMembership.Following) - - _, appErr = th.App.GetThreadMembershipForUser(th.BasicUser.Id, "notfound") require.NotNil(t, appErr) - assert.Equal(t, http.StatusNotFound, appErr.StatusCode) - }) - t.Run("Ensure no panic on error", func(t *testing.T) { - th := SetupWithStoreMock(t) - defer th.TearDown() - - mockStore := th.App.Srv().Store().(*storemocks.Store) - mockUserStore := storemocks.UserStore{} - mockUserStore.On("Count", mock.Anything).Return(int64(10), nil) - mockUserStore.On("Get", mock.Anything, "user1").Return(&model.User{Id: "user1"}, nil) - - mockThreadStore := storemocks.ThreadStore{} - mockThreadStore.On("MaintainMembership", "user1", "postid", mock.Anything).Return(nil, errors.New("error")) - - var err error - th.App.ch.srv.userService, err = users.New(users.ServiceConfig{ - UserStore: &mockUserStore, - SessionStore: &storemocks.SessionStore{}, - OAuthStore: &storemocks.OAuthStore{}, - ConfigFn: th.App.ch.srv.platform.Config, - LicenseFn: th.App.ch.srv.License, - }) + _, err := th.App.Srv().Store().Thread().MaintainMembership(th.BasicUser.Id, rootPost.Id, store.ThreadMembershipOpts{Following: true, UpdateFollowing: true}) require.NoError(t, err) - mockStore.On("User").Return(&mockUserStore) - mockStore.On("Thread").Return(&mockThreadStore) - _, err = th.App.UpdateThreadReadForUser(th.Context, "currentSessionId", "user1", "team1", "postid", 100) - require.Error(t, err) + _, appErr = th.App.UpdateThreadReadForUser(th.Context, "currentSessionId", th.BasicUser.Id, th.BasicChannel.TeamId, rootPost.Id, replyPost.CreateAt) + require.Nil(t, appErr) }) } diff --git a/webapp/channels/src/actions/views/threads.ts b/webapp/channels/src/actions/views/threads.ts index 1200f3da01..006e980ced 100644 --- a/webapp/channels/src/actions/views/threads.ts +++ b/webapp/channels/src/actions/views/threads.ts @@ -5,6 +5,7 @@ import {batchActions} from 'redux-batched-actions'; import {updateThreadRead} from 'mattermost-redux/actions/threads'; import {getCurrentTeamId} from 'mattermost-redux/selectors/entities/teams'; +import {getThread} from 'mattermost-redux/selectors/entities/threads'; import {getCurrentUserId} from 'mattermost-redux/selectors/entities/users'; import type {ThunkActionFunc} from 'mattermost-redux/types/actions'; @@ -56,8 +57,9 @@ export function markThreadAsRead(threadId: string): ThunkActionFunc