From 18cd3a1d07c7fdf88063174f0390d647b834ccda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Tue, 2 Jun 2020 14:16:46 +0200 Subject: [PATCH] Fixing reply count on new posts (#14312) * Fixing reply count on new posts * Fixing tests * Fixing post reply count on getPostsAround Co-authored-by: mattermod --- store/sqlstore/post_store.go | 15 +++---- store/storetest/post_store.go | 76 ++++++++++++++++++++++++++++++++++- 2 files changed, 81 insertions(+), 10 deletions(-) diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index e203252ecd..2cb9a863ca 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -204,13 +204,13 @@ func (s *SqlPostStore) Save(post *model.Post) (*model.Post, *model.AppError) { func (s *SqlPostStore) populateReplyCount(posts []*model.Post) *model.AppError { rootIds := []string{} for _, post := range posts { - rootIds = append(rootIds, post.Id) + rootIds = append(rootIds, post.RootId) } countList := []struct { RootId string Count int64 }{} - query := s.getQueryBuilder().Select("RootId, COUNT(Id)").From("Posts").Where(sq.Eq{"RootId": rootIds}).Where(sq.Eq{"DeleteAt": 0}).GroupBy("RootId") + query := s.getQueryBuilder().Select("RootId, COUNT(Id) AS Count").From("Posts").Where(sq.Eq{"RootId": rootIds}).Where(sq.Eq{"DeleteAt": 0}).GroupBy("RootId") queryString, args, err := query.ToSql() if err != nil { @@ -703,11 +703,9 @@ func (s *SqlPostStore) getPostsAround(before bool, options model.GetPostsOptions direction = ">" sort = "ASC" } - replyCountSubQuery := s.getQueryBuilder().Select("COUNT(Posts.Id)").From("Posts").Where(sq.Expr("p.RootId = '' AND RootId = p.Id AND DeleteAt = 0")) + replyCountSubQuery := s.getQueryBuilder().Select("COUNT(Posts.Id)").From("Posts").Where(sq.Expr("Posts.RootId = (CASE WHEN p.RootId = '' THEN p.Id ELSE p.RootId END) AND Posts.DeleteAt = 0")) query := s.getQueryBuilder().Select("p.*") - if options.SkipFetchThreads { - query = query.Column(sq.Alias(replyCountSubQuery, "ReplyCount")) - } + query = query.Column(sq.Alias(replyCountSubQuery, "ReplyCount")) query = query.From("Posts p"). Where(sq.And{ sq.Expr(`CreateAt `+direction+` (SELECT CreateAt FROM Posts WHERE Id = ?)`, options.PostId), @@ -742,9 +740,8 @@ func (s *SqlPostStore) getPostsAround(before bool, options model.GetPostsOptions idQuery := sq.Or{ sq.Eq{"Id": rootIds}, } - if options.SkipFetchThreads { - rootQuery = rootQuery.Column(sq.Alias(replyCountSubQuery, "ReplyCount")) - } else { + rootQuery = rootQuery.Column(sq.Alias(replyCountSubQuery, "ReplyCount")) + if !options.SkipFetchThreads { idQuery = append(idQuery, sq.Eq{"RootId": rootIds}) // preserve original behaviour } diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index 6d1539672a..8ebddc6c1d 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -62,8 +62,41 @@ func testPostStoreSave(t *testing.T, ss store.Store) { o1.UserId = model.NewId() o1.Message = "zz" + model.NewId() + "b" - _, err := ss.Post().Save(&o1) + p, err := ss.Post().Save(&o1) require.Nil(t, err, "couldn't save item") + assert.Equal(t, int64(0), p.ReplyCount) + }) + + t.Run("Save replies", func(t *testing.T) { + o1 := model.Post{} + o1.ChannelId = model.NewId() + o1.UserId = model.NewId() + o1.RootId = model.NewId() + o1.Message = "zz" + model.NewId() + "b" + + o2 := model.Post{} + o2.ChannelId = model.NewId() + o2.UserId = model.NewId() + o2.RootId = o1.RootId + o2.Message = "zz" + model.NewId() + "b" + + o3 := model.Post{} + o3.ChannelId = model.NewId() + o3.UserId = model.NewId() + o3.RootId = model.NewId() + o3.Message = "zz" + model.NewId() + "b" + + p1, err := ss.Post().Save(&o1) + require.Nil(t, err, "couldn't save item") + assert.Equal(t, int64(1), p1.ReplyCount) + + p2, err := ss.Post().Save(&o2) + require.Nil(t, err, "couldn't save item") + assert.Equal(t, int64(2), p2.ReplyCount) + + p3, err := ss.Post().Save(&o3) + require.Nil(t, err, "couldn't save item") + assert.Equal(t, int64(1), p3.ReplyCount) }) t.Run("Try to save existing post", func(t *testing.T) { @@ -196,6 +229,39 @@ func testPostStoreSaveMultiple(t *testing.T, ss store.Store) { } }) + t.Run("Save replies", func(t *testing.T) { + o1 := model.Post{} + o1.ChannelId = model.NewId() + o1.UserId = model.NewId() + o1.RootId = model.NewId() + o1.Message = "zz" + model.NewId() + "b" + + o2 := model.Post{} + o2.ChannelId = model.NewId() + o2.UserId = model.NewId() + o2.RootId = o1.RootId + o2.Message = "zz" + model.NewId() + "b" + + o3 := model.Post{} + o3.ChannelId = model.NewId() + o3.UserId = model.NewId() + o3.RootId = model.NewId() + o3.Message = "zz" + model.NewId() + "b" + + o4 := model.Post{} + o4.ChannelId = model.NewId() + o4.UserId = model.NewId() + o4.Message = "zz" + model.NewId() + "b" + + newPosts, err := ss.Post().SaveMultiple([]*model.Post{&o1, &o2, &o3, &o4}) + require.Nil(t, err, "couldn't save item") + assert.Len(t, newPosts, 4) + assert.Equal(t, int64(2), newPosts[0].ReplyCount) + assert.Equal(t, int64(2), newPosts[1].ReplyCount) + assert.Equal(t, int64(1), newPosts[2].ReplyCount) + assert.Equal(t, int64(0), newPosts[3].ReplyCount) + }) + t.Run("Try to save mixed, already saved and not saved posts", func(t *testing.T) { newPosts, err := ss.Post().SaveMultiple([]*model.Post{&p4, &p3}) require.NotNil(t, err) @@ -906,6 +972,7 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { UserId: userId, Message: "message", }) + post1.ReplyCount = 1 require.Nil(t, err) time.Sleep(time.Millisecond) @@ -915,6 +982,7 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { Message: "message", }) require.Nil(t, err) + post2.ReplyCount = 2 time.Sleep(time.Millisecond) post3, err := ss.Post().Save(&model.Post{ @@ -925,6 +993,7 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { Message: "message", }) require.Nil(t, err) + post3.ReplyCount = 1 time.Sleep(time.Millisecond) post4, err := ss.Post().Save(&model.Post{ @@ -935,6 +1004,7 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { Message: "message", }) require.Nil(t, err) + post4.ReplyCount = 2 time.Sleep(time.Millisecond) post5, err := ss.Post().Save(&model.Post{ @@ -952,6 +1022,7 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { RootId: post2.Id, Message: "message", }) + post6.ReplyCount = 2 require.Nil(t, err) // Adding a post to a thread changes the UpdateAt timestamp of the parent post @@ -1023,6 +1094,7 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { Message: "post3", }) require.Nil(t, err) + post3.ReplyCount = 1 time.Sleep(time.Millisecond) post4, err := ss.Post().Save(&model.Post{ @@ -1033,6 +1105,7 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { Message: "post4", }) require.Nil(t, err) + post4.ReplyCount = 2 time.Sleep(time.Millisecond) post5, err := ss.Post().Save(&model.Post{ @@ -1050,6 +1123,7 @@ func testPostStoreGetPostsBeforeAfter(t *testing.T, ss store.Store) { RootId: post2.Id, Message: "post6", }) + post6.ReplyCount = 2 require.Nil(t, err) // Adding a post to a thread changes the UpdateAt timestamp of the parent post