From 13616cac0f31521a2d582d5434968ef7ae433d30 Mon Sep 17 00:00:00 2001 From: Eli Yukelzon Date: Sun, 31 Jan 2021 12:28:14 +0200 Subject: [PATCH] MM-31711 - Implement cursor paging for threads (#16748) Co-authored-by: Mattermod --- api4/user.go | 20 +++--- api4/user_test.go | 110 +++++++++++++++++++-------------- i18n/en.json | 4 ++ model/client4.go | 7 ++- model/thread.go | 9 ++- store/sqlstore/thread_store.go | 19 +++++- 6 files changed, 103 insertions(+), 66 deletions(-) diff --git a/api4/user.go b/api4/user.go index dcd07f6a6d..dc3a440d1a 100644 --- a/api4/user.go +++ b/api4/user.go @@ -2855,7 +2855,8 @@ func getThreadsForUser(c *Context, w http.ResponseWriter, r *http.Request) { options := model.GetUserThreadsOpts{ Since: 0, - Page: 0, + Before: "", + After: "", PageSize: 30, Unread: false, Extended: false, @@ -2872,18 +2873,15 @@ func getThreadsForUser(c *Context, w http.ResponseWriter, r *http.Request) { options.Since = since } - pageString := r.URL.Query().Get("page") - if pageString != "" { - page, parseError := strconv.ParseUint(pageString, 10, 64) - if parseError != nil { - c.SetInvalidParam("page") - return - } - options.Page = page + options.Before = r.URL.Query().Get("before") + options.After = r.URL.Query().Get("after") + // parameters are mutually exclusive + if options.Before != "" && options.After != "" { + c.Err = model.NewAppError("api.getThreadsForUser", "api.getThreadsForUser.bad_params", nil, "", http.StatusBadRequest) + return } - pageSizeString := r.URL.Query().Get("pageSize") - if pageString != "" { + if pageSizeString != "" { pageSize, parseError := strconv.ParseUint(pageSizeString, 10, 64) if parseError != nil { c.SetInvalidParam("pageSize") diff --git a/api4/user_test.go b/api4/user_test.go index d41978224e..13262a279c 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -5261,7 +5261,13 @@ func TestUpdatePassword(t *testing.T) { func TestGetThreadsForUser(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 + }) t.Run("empty", func(t *testing.T) { Client := th.Client @@ -5271,10 +5277,7 @@ func TestGetThreadsForUser(t *testing.T) { defer th.App.Srv().Store.Post().PermanentDeleteByUser(th.BasicUser.Id) - uss, resp := th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Page: 0, - PageSize: 30, - }) + uss, resp := th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{}) require.Nil(t, resp.Error) require.Len(t, uss.Threads, 0) }) @@ -5291,10 +5294,7 @@ func TestGetThreadsForUser(t *testing.T) { defer th.App.Srv().Store.Post().PermanentDeleteByUser(th.BasicUser.Id) - uss, resp := th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Page: 0, - PageSize: 30, - }) + uss, resp := th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{}) require.Nil(t, resp.Error) require.Len(t, uss.Threads, 1) require.Equal(t, uss.Threads[0].PostId, rpost.Id) @@ -5314,8 +5314,6 @@ func TestGetThreadsForUser(t *testing.T) { defer th.App.Srv().Store.Post().PermanentDeleteByUser(th.BasicUser.Id) uss, resp := th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Page: 0, - PageSize: 30, Extended: true, }) require.Nil(t, resp.Error) @@ -5338,9 +5336,7 @@ func TestGetThreadsForUser(t *testing.T) { defer th.App.Srv().Store.Post().PermanentDeleteByUser(th.BasicUser.Id) uss, resp := th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Page: 0, - PageSize: 30, - Deleted: false, + Deleted: false, }) require.Nil(t, resp.Error) require.Len(t, uss.Threads, 1) @@ -5353,17 +5349,13 @@ func TestGetThreadsForUser(t *testing.T) { require.Nil(t, resp2.Error) uss, resp = th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Page: 0, - PageSize: 30, - Deleted: false, + Deleted: false, }) require.Nil(t, resp.Error) require.Len(t, uss.Threads, 0) uss, resp = th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Page: 0, - PageSize: 30, - Deleted: true, + Deleted: true, }) require.Nil(t, resp.Error) require.Len(t, uss.Threads, 1) @@ -5390,9 +5382,7 @@ func TestGetThreadsForUser(t *testing.T) { defer th.App.Srv().Store.Post().PermanentDeleteByUser(th.BasicUser.Id) uss, resp := th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Page: 0, - PageSize: 30, - Deleted: false, + Deleted: false, }) require.Nil(t, resp.Error) require.Len(t, uss.Threads, 30) @@ -5401,6 +5391,45 @@ func TestGetThreadsForUser(t *testing.T) { require.Equal(t, uss.Threads[0].ReplyCount, int64(1)) require.Equal(t, uss.Threads[0].Participants[0].Id, th.BasicUser.Id) }) + + t.Run("paged, 10 threads before/after", func(t *testing.T) { + Client := th.Client + + var rootIds []*model.Post + for i := 0; i < 30; i++ { + time.Sleep(1) + rpost, _ := postAndCheck(t, Client, &model.Post{ChannelId: th.BasicChannel.Id, Message: fmt.Sprintf("testMsg-%d", i)}) + rootIds = append(rootIds, rpost) + time.Sleep(1) + postAndCheck(t, Client, &model.Post{ChannelId: th.BasicChannel.Id, Message: fmt.Sprintf("testReply-%d", i), RootId: rpost.Id}) + } + rootId := rootIds[15].Id // middle point + rootIdBefore := rootIds[14].Id + rootIdAfter := rootIds[16].Id + + defer th.App.Srv().Store.Post().PermanentDeleteByUser(th.BasicUser.Id) + + uss, resp := th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ + Deleted: false, + PageSize: 10, + Before: rootId, + }) + + require.Nil(t, resp.Error) + require.Len(t, uss.Threads, 10) + require.Equal(t, uss.Threads[0].PostId, rootIdBefore) + + uss2, resp2 := th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ + Deleted: false, + PageSize: 10, + After: rootId, + }) + require.Nil(t, resp2.Error) + require.Len(t, uss2.Threads, 10) + + require.Equal(t, uss2.Threads[0].PostId, rootIdAfter) + + }) } func TestThreadSocketEvents(t *testing.T) { @@ -5515,9 +5544,7 @@ func TestFollowThreads(t *testing.T) { defer th.App.Srv().Store.Post().PermanentDeleteByUser(th.BasicUser.Id) var uss *model.Threads uss, resp = th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Page: 0, - PageSize: 30, - Deleted: false, + Deleted: false, }) CheckNoError(t, resp) require.Len(t, uss.Threads, 1) @@ -5527,9 +5554,7 @@ func TestFollowThreads(t *testing.T) { CheckOKStatus(t, resp) uss, resp = th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Page: 0, - PageSize: 30, - Deleted: false, + Deleted: false, }) CheckNoError(t, resp) require.Len(t, uss.Threads, 0) @@ -5539,9 +5564,7 @@ func TestFollowThreads(t *testing.T) { CheckOKStatus(t, resp) uss, resp = th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Page: 0, - PageSize: 30, - Deleted: false, + Deleted: false, }) CheckNoError(t, resp) require.Len(t, uss.Threads, 1) @@ -5676,7 +5699,7 @@ func TestThreadCounts(t *testing.T) { checkThreadListReplies(t, th, th.Client, th.BasicUser.Id, 2, 1, &model.GetUserThreadsOpts{ Deleted: false, }) - // with Deleted we should get the same as before deleting, minus the reply in the deleted thread + // with Deleted we should get the same as before deleting checkThreadListReplies(t, th, th.Client, th.BasicUser.Id, 3, 2, &model.GetUserThreadsOpts{ Deleted: true, }) @@ -5733,9 +5756,7 @@ func TestMaintainUnreadMentionsInThread(t *testing.T) { }) checkThreadList := func(client *model.Client4, userId string, expectedMentions, expectedThreads int) (*model.Threads, *model.Response) { uss, resp := client.GetUserThreads(userId, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Page: 0, - PageSize: 30, - Deleted: false, + Deleted: false, }) CheckNoError(t, resp) require.Len(t, uss.Threads, expectedThreads) @@ -5807,9 +5828,7 @@ func TestReadThreads(t *testing.T) { var uss, uss2 *model.Threads uss, resp = th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Page: 0, - PageSize: 30, - Deleted: false, + Deleted: false, }) CheckNoError(t, resp) require.Len(t, uss.Threads, 1) @@ -5820,9 +5839,7 @@ func TestReadThreads(t *testing.T) { CheckOKStatus(t, resp) uss2, resp = th.Client.GetUserThreads(th.BasicUser.Id, th.BasicTeam.Id, model.GetUserThreadsOpts{ - Page: 0, - PageSize: 30, - Deleted: false, + Deleted: false, }) CheckNoError(t, resp) require.Len(t, uss2.Threads, 1) @@ -5833,16 +5850,15 @@ func TestReadThreads(t *testing.T) { defer th.App.Srv().Store.Post().PermanentDeleteByUser(th.BasicUser.Id) defer th.App.Srv().Store.Post().PermanentDeleteByUser(th.SystemAdminUser.Id) - rpost, _ := postAndCheck(t, Client, &model.Post{ChannelId: th.BasicChannel.Id, Message: "testMsg"}) - postAndCheck(t, th.SystemAdminClient, &model.Post{ChannelId: th.BasicChannel.Id, Message: "testReply", RootId: rpost.Id}) + rpost, _ := postAndCheck(t, Client, &model.Post{ChannelId: th.BasicChannel.Id, Message: "testMsgC1"}) + postAndCheck(t, th.SystemAdminClient, &model.Post{ChannelId: th.BasicChannel.Id, Message: "testReplyC1", RootId: rpost.Id}) - rrpost, _ := postAndCheck(t, Client, &model.Post{ChannelId: th.BasicChannel2.Id, Message: "testMsg"}) - postAndCheck(t, th.SystemAdminClient, &model.Post{ChannelId: th.BasicChannel2.Id, Message: "testReply", RootId: rrpost.Id}) + rrpost, _ := postAndCheck(t, Client, &model.Post{ChannelId: th.BasicChannel2.Id, Message: "testMsgC2"}) + postAndCheck(t, th.SystemAdminClient, &model.Post{ChannelId: th.BasicChannel2.Id, Message: "testReplyC2", RootId: rrpost.Id}) uss, _ := checkThreadListReplies(t, th, th.Client, th.BasicUser.Id, 2, 2, nil) - time.Sleep(1) - resp := th.Client.UpdateThreadReadForUser(th.BasicUser.Id, th.BasicTeam.Id, rrpost.Id, model.GetMillis()) + resp := th.Client.UpdateThreadReadForUser(th.BasicUser.Id, th.BasicTeam.Id, rrpost.Id, model.GetMillis()+10) CheckNoError(t, resp) CheckOKStatus(t, resp) diff --git a/i18n/en.json b/i18n/en.json index 61edfae2fb..671ad26077 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1444,6 +1444,10 @@ "id": "api.file.write_file.app_error", "translation": "Unable to write the file." }, + { + "id": "api.getThreadsForUser.bad_params", + "translation": "Before and After parameters to getThreadsForUser are mutually exclusive" + }, { "id": "api.image.get.app_error", "translation": "Requested image url cannot be parsed." diff --git a/model/client4.go b/model/client4.go index 70e90f980a..58d23163c3 100644 --- a/model/client4.go +++ b/model/client4.go @@ -5794,8 +5794,11 @@ func (c *Client4) GetUserThreads(userId, teamId string, options GetUserThreadsOp if options.Since != 0 { v.Set("since", fmt.Sprintf("%d", options.Since)) } - if options.Page != 0 { - v.Set("page", fmt.Sprintf("%d", options.Page)) + if options.Before != "" { + v.Set("before", options.Before) + } + if options.After != "" { + v.Set("after", options.After) } if options.PageSize != 0 { v.Set("pageSize", fmt.Sprintf("%d", options.PageSize)) diff --git a/model/thread.go b/model/thread.go index aaef61d1ac..fb7ca9f991 100644 --- a/model/thread.go +++ b/model/thread.go @@ -34,9 +34,6 @@ type Threads struct { } type GetUserThreadsOpts struct { - // Page specifies which part of the results to return, by PageSize. Default = 0 - Page uint64 - // PageSize specifies the size of the returned chunk of results. Default = 30 PageSize uint64 @@ -49,6 +46,12 @@ type GetUserThreadsOpts struct { // Since filters the threads based on their LastUpdateAt timestamp. Since uint64 + // Before specifies thread id as a cursor for pagination and will return `PageSize` threads before the cursor + Before string + + // After specifies thread id as a cursor for pagination and will return `PageSize` threads after the cursor + After string + // Unread will make sure that only threads with unread replies are returned Unread bool } diff --git a/store/sqlstore/thread_store.go b/store/sqlstore/thread_store.go index e34c5bebda..e14e0f0766 100644 --- a/store/sqlstore/thread_store.go +++ b/store/sqlstore/thread_store.go @@ -188,10 +188,23 @@ func (s *SqlThreadStore) GetThreadsForUser(userId, teamId string, opts model.Get if opts.Since > 0 { newFetchConditions = sq.And{newFetchConditions, sq.GtOrEq{"ThreadMemberships.LastUpdated": opts.Since}} } + order := "DESC" + if opts.Before != "" { + newFetchConditions = sq.And{ + newFetchConditions, + sq.Expr(`LastReplyAt < (SELECT LastReplyAt FROM Threads WHERE PostId = ?)`, opts.Before), + } + } + if opts.After != "" { + order = "ASC" + newFetchConditions = sq.And{ + newFetchConditions, + sq.Expr(`LastReplyAt > (SELECT LastReplyAt FROM Threads WHERE PostId = ?)`, opts.After), + } + } if opts.Unread { newFetchConditions = sq.And{newFetchConditions, sq.Expr("ThreadMemberships.LastViewed < Threads.LastReplyAt")} } - var threads []*JoinedThread query, args, _ := s.getQueryBuilder(). Select("Threads.*, Posts.*, ThreadMemberships.LastViewed as LastViewedAt, ThreadMemberships.UnreadMentions as UnreadMentions"). @@ -201,9 +214,9 @@ func (s *SqlThreadStore) GetThreadsForUser(userId, teamId string, opts model.Get LeftJoin("Channels ON Posts.ChannelId = Channels.Id"). LeftJoin("ThreadMemberships ON ThreadMemberships.PostId = Threads.PostId"). Where(newFetchConditions). - OrderBy("Threads.LastReplyAt DESC"). - Offset(pageSize * opts.Page). + OrderBy("Threads.LastReplyAt " + order). Limit(pageSize).ToSql() + _, err := s.GetReplica().Select(&threads, query, args...) threadsChan <- store.StoreResult{Data: threads, NErr: err} close(threadsChan)