From 3094f10557504cc402b822401a56bd49216681c6 Mon Sep 17 00:00:00 2001 From: Kyriakos Z <3829551+koox00@users.noreply.github.com> Date: Wed, 25 Jan 2023 14:45:45 +0200 Subject: [PATCH] MM-49845: fixes acknowledgements in getpostsince (#22124) * MM-49845: fixes acknowledgements in getpostsince GetPostsSince returns posts updated after a user requested date. If a user acknowledges a post the post's update at won't change thus not getting that post returned from GetPostsSince. This is troublesome particularly for the mobile client since it relies a lot to GetPostsSince for keeping the data up to date. This commit updates the post's update_at when a user acknowledges/un-acknowledges a post fixing this way the issue above. * Fixes linter * Apply suggestions from code review style changes Co-authored-by: Jesse Hallam * Rename ack to acknowledgement Co-authored-by: Jesse Hallam --- app/post_acknowledgements.go | 6 ++ app/post_acknowledgements_test.go | 44 +++++++++++++- store/sqlstore/post_acknowledgements_store.go | 57 +++++++++++++++++-- .../storetest/post_acknowledgements_store.go | 20 +++++-- 4 files changed, 114 insertions(+), 13 deletions(-) diff --git a/app/post_acknowledgements.go b/app/post_acknowledgements.go index a91f76d02b..2a16a1ecd3 100644 --- a/app/post_acknowledgements.go +++ b/app/post_acknowledgements.go @@ -42,6 +42,9 @@ func (a *App) SaveAcknowledgementForPost(c *request.Context, postID, userID stri } } + // The post is always modified since the UpdateAt always changes + a.invalidateCacheForChannelPosts(channel.Id) + a.Srv().Go(func() { a.sendAcknowledgementEvent(model.WebsocketEventAcknowledgementAdded, acknowledgement, post) }) @@ -85,6 +88,9 @@ func (a *App) DeleteAcknowledgementForPost(c *request.Context, postID, userID st return model.NewAppError("DeleteAcknowledgementForPost", "app.acknowledgement.delete.app_error", nil, "", http.StatusInternalServerError).Wrap(nErr) } + // The post is always modified since the UpdateAt always changes + a.invalidateCacheForChannelPosts(channel.Id) + a.Srv().Go(func() { a.sendAcknowledgementEvent(model.WebsocketEventAcknowledgementRemoved, oldAck, post) }) diff --git a/app/post_acknowledgements_test.go b/app/post_acknowledgements_test.go index f1306c29fd..776cf4cdc8 100644 --- a/app/post_acknowledgements_test.go +++ b/app/post_acknowledgements_test.go @@ -36,21 +36,41 @@ func testSaveAcknowledgementForPost(t *testing.T) { require.Equal(t, post.Id, acknowledgment.PostId) require.Equal(t, th.BasicUser.Id, acknowledgment.UserId) }) + + t.Run("saving acknowledgment should update the post's update_at", func(t *testing.T) { + post, err := th.App.CreatePostAsUser(th.Context, &model.Post{ + UserId: th.BasicUser.Id, + ChannelId: th.BasicChannel.Id, + Message: "message", + }, "", true) + + require.Nil(t, err) + + oldUpdateAt := post.UpdateAt + + _, err = th.App.SaveAcknowledgementForPost(th.Context, post.Id, th.BasicUser.Id) + require.Nil(t, err) + + post, err = th.App.GetSinglePost(post.Id, false) + require.Nil(t, err) + + require.Greater(t, post.UpdateAt, oldUpdateAt) + }) } func testDeleteAcknowledgementForPost(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() - post, err := th.App.CreatePostAsUser(th.Context, &model.Post{ + post, err1 := th.App.CreatePostAsUser(th.Context, &model.Post{ UserId: th.BasicUser.Id, ChannelId: th.BasicChannel.Id, CreateAt: model.GetMillis(), Message: "message", }, "", true) - require.Nil(t, err) + require.Nil(t, err1) t.Run("delete acknowledgment for post should delete acknowledgement", func(t *testing.T) { - _, err = th.App.SaveAcknowledgementForPost(th.Context, post.Id, th.BasicUser.Id) + _, err := th.App.SaveAcknowledgementForPost(th.Context, post.Id, th.BasicUser.Id) require.Nil(t, err) acknowledgments, err := th.App.GetAcknowledgementsForPost(post.Id) @@ -66,6 +86,24 @@ func testDeleteAcknowledgementForPost(t *testing.T) { require.Empty(t, acknowledgments) }) + t.Run("deleting acknowledgment should update the post's update_at", func(t *testing.T) { + _, err := th.App.SaveAcknowledgementForPost(th.Context, post.Id, th.BasicUser.Id) + require.Nil(t, err) + + post, err = th.App.GetSinglePost(post.Id, false) + require.Nil(t, err) + + oldUpdateAt := post.UpdateAt + + err = th.App.DeleteAcknowledgementForPost(th.Context, post.Id, th.BasicUser.Id) + require.Nil(t, err) + + post, err = th.App.GetSinglePost(post.Id, false) + require.Nil(t, err) + + require.Greater(t, post.UpdateAt, oldUpdateAt) + }) + t.Run("delete acknowledgment for post after 5 min after acknowledged should not delete", func(t *testing.T) { _, nErr := th.App.Srv().Store().PostAcknowledgement().Save(post.Id, th.BasicUser.Id, model.GetMillis()-int64(6*60*1000)) require.NoError(t, nErr) diff --git a/store/sqlstore/post_acknowledgements_store.go b/store/sqlstore/post_acknowledgements_store.go index d3de3addea..dad3d053df 100644 --- a/store/sqlstore/post_acknowledgements_store.go +++ b/store/sqlstore/post_acknowledgements_store.go @@ -58,6 +58,12 @@ func (s *SqlPostAcknowledgementStore) Save(postID, userID string, acknowledgedAt return nil, err } + transaction, err := s.GetMasterX().Beginx() + if err != nil { + return nil, errors.Wrap(err, "begin_transaction") + } + defer finalizeTransactionX(transaction, &err) + query := s.getQueryBuilder(). Insert("PostAcknowledgements"). Columns("PostId", "UserId", "AcknowledgedAt"). @@ -69,28 +75,54 @@ func (s *SqlPostAcknowledgementStore) Save(postID, userID string, acknowledgedAt query = query.SuffixExpr(sq.Expr("ON CONFLICT (postid, userid) DO UPDATE SET AcknowledgedAt = ?", acknowledgement.AcknowledgedAt)) } - _, err := s.GetMasterX().ExecBuilder(query) + _, err = transaction.ExecBuilder(query) if err != nil { return nil, err } + err = updatePost(transaction, acknowledgement.PostId) + if err != nil { + return nil, err + } + + err = transaction.Commit() + if err != nil { + return nil, errors.Wrap(err, "commit_transaction") + } + return acknowledgement, nil } -func (s *SqlPostAcknowledgementStore) Delete(ack *model.PostAcknowledgement) error { +func (s *SqlPostAcknowledgementStore) Delete(acknowledgement *model.PostAcknowledgement) error { + transaction, err := s.GetMasterX().Beginx() + if err != nil { + return errors.Wrap(err, "begin_transaction") + } + defer finalizeTransactionX(transaction, &err) + query := s.getQueryBuilder(). Update("PostAcknowledgements"). Set("AcknowledgedAt", 0). Where(sq.And{ - sq.Eq{"PostId": ack.PostId}, - sq.Eq{"UserId": ack.UserId}, + sq.Eq{"PostId": acknowledgement.PostId}, + sq.Eq{"UserId": acknowledgement.UserId}, }) - _, err := s.GetMasterX().ExecBuilder(query) + _, err = transaction.ExecBuilder(query) if err != nil { return err } + err = updatePost(transaction, acknowledgement.PostId) + if err != nil { + return err + } + + err = transaction.Commit() + if err != nil { + return errors.Wrap(err, "commit_transaction") + } + return nil } @@ -142,3 +174,18 @@ func (s *SqlPostAcknowledgementStore) GetForPosts(postIds []string) ([]*model.Po return acknowledgements, nil } + +func updatePost(transaction *sqlxTxWrapper, postId string) error { + _, err := transaction.Exec( + `UPDATE + Posts + SET + UpdateAt = ? + WHERE + Id = ?`, + model.GetMillis(), + postId, + ) + + return err +} diff --git a/store/storetest/post_acknowledgements_store.go b/store/storetest/post_acknowledgements_store.go index ac751119bf..1320ab3276 100644 --- a/store/storetest/post_acknowledgements_store.go +++ b/store/storetest/post_acknowledgements_store.go @@ -31,23 +31,33 @@ func testPostAcknowledgementsStoreSave(t *testing.T, ss store.Store) { PersistentNotifications: model.NewBool(false), }, } - _, err := ss.Post().Save(&p1) + post, err := ss.Post().Save(&p1) require.NoError(t, err) t.Run("consecutive saves should just update the acknowledged at", func(t *testing.T) { - _, err := ss.PostAcknowledgement().Save(p1.Id, userId1, 0) + _, err := ss.PostAcknowledgement().Save(post.Id, userId1, 0) require.NoError(t, err) - _, err = ss.PostAcknowledgement().Save(p1.Id, userId1, 0) + _, err = ss.PostAcknowledgement().Save(post.Id, userId1, 0) require.NoError(t, err) - ack1, err := ss.PostAcknowledgement().Save(p1.Id, userId1, 0) + ack1, err := ss.PostAcknowledgement().Save(post.Id, userId1, 0) require.NoError(t, err) - acknowledgements, err := ss.PostAcknowledgement().GetForPost(p1.Id) + acknowledgements, err := ss.PostAcknowledgement().GetForPost(post.Id) require.NoError(t, err) require.ElementsMatch(t, acknowledgements, []*model.PostAcknowledgement{ack1}) }) + + t.Run("saving should update the update at of the post", func(t *testing.T) { + oldUpdateAt := post.UpdateAt + _, err := ss.PostAcknowledgement().Save(post.Id, userId1, 0) + require.NoError(t, err) + + post, err = ss.Post().GetSingle(post.Id, false) + require.NoError(t, err) + require.Greater(t, post.UpdateAt, oldUpdateAt) + }) } func testPostAcknowledgementsStoreGetForPost(t *testing.T, ss store.Store) {