From 0f16c59b0a621681c8130297f8be1bec7525c032 Mon Sep 17 00:00:00 2001 From: Kyriakos Z <3829551+koox00@users.noreply.github.com> Date: Fri, 15 Oct 2021 19:40:09 +0300 Subject: [PATCH] MM-36862: should remove user from participants (#18451) * MM-36862: should remove user from participants When deleting a reply in a thread we should also delete the participant from the participants array. This should happen if they have no other replies in that thread. This commit fixes that. * Adds warning logs * Delete Post: fetches participants only when needed * Minor refactor * Remove participant should check for error in count * Fixes error when binary_parameters=true * Fixes: removes ArrayToJSON so we can handle the error * Satisfies the linter * Better error handling * Satisfy govet Co-authored-by: Mattermod --- store/sqlstore/post_store.go | 76 ++++++++++++++++++++++++++++++--- store/storetest/thread_store.go | 64 ++++++++++++++++++--------- 2 files changed, 112 insertions(+), 28 deletions(-) diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index ad0a37e6ae..5f05eb0d59 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -6,6 +6,7 @@ package sqlstore import ( "context" "database/sql" + "encoding/json" "fmt" "reflect" "regexp" @@ -695,8 +696,9 @@ func (s *SqlPostStore) Delete(postID string, time int64, deleteByID string) erro return errors.Wrap(err, "failed to update Posts") } + ids := postIds{} // TODO: change this to later delete thread directly from postID - rootID, err := s.GetReplica().SelectStr("SELECT RootId FROM Posts WHERE Id = :Id", map[string]interface{}{"Id": postID}) + err = s.GetReplica().SelectOne(&ids, "SELECT RootId, UserId FROM Posts WHERE Id = :Id", map[string]interface{}{"Id": postID}) if err != nil { if err == sql.ErrNoRows { return store.NewErrNotFound("Post", postID) @@ -705,7 +707,7 @@ func (s *SqlPostStore) Delete(postID string, time int64, deleteByID string) erro return errors.Wrapf(err, "failed to delete Post with id=%s", postID) } - return s.cleanupThreads(postID, rootID, false) + return s.cleanupThreads(postID, ids.RootId, false, ids.UserId) } func (s *SqlPostStore) permanentDelete(postId string) error { @@ -714,7 +716,7 @@ func (s *SqlPostStore) permanentDelete(postId string) error { if err != nil && err != sql.ErrNoRows { return errors.Wrapf(err, "failed to get Post with id=%s", postId) } - if err = s.cleanupThreads(post.Id, post.RootId, true); err != nil { + if err = s.cleanupThreads(post.Id, post.RootId, true, post.UserId); err != nil { return errors.Wrapf(err, "failed to cleanup threads for Post with id=%s", postId) } @@ -739,7 +741,7 @@ func (s *SqlPostStore) permanentDeleteAllCommentByUser(userId string) error { } for _, ids := range results { - if err = s.cleanupThreads(ids.Id, ids.RootId, true); err != nil { + if err = s.cleanupThreads(ids.Id, ids.RootId, true, userId); err != nil { return err } } @@ -795,7 +797,7 @@ func (s *SqlPostStore) PermanentDeleteByChannel(channelId string) error { } for _, ids := range results { - if err = s.cleanupThreads(ids.Id, ids.RootId, true); err != nil { + if err = s.cleanupThreads(ids.Id, ids.RootId, true, ids.UserId); err != nil { return err } } @@ -2375,7 +2377,7 @@ func (s *SqlPostStore) GetOldestEntityCreationTime() (int64, error) { return oldest, nil } -func (s *SqlPostStore) cleanupThreads(postId, rootId string, permanent bool) error { +func (s *SqlPostStore) cleanupThreads(postId, rootId string, permanent bool, userId string) error { if permanent { if _, err := s.GetMaster().Exec("DELETE FROM Threads WHERE PostId = :Id", map[string]interface{}{"Id": postId}); err != nil { return errors.Wrap(err, "failed to delete Threads") @@ -2386,7 +2388,67 @@ func (s *SqlPostStore) cleanupThreads(postId, rootId string, permanent bool) err return nil } if rootId != "" { - _, err := s.GetMaster().Exec(`UPDATE Threads SET ReplyCount = ReplyCount - 1 WHERE PostId = :Id AND ReplyCount > 0`, map[string]interface{}{"Id": rootId}) + queryString, args, err := s.getQueryBuilder(). + Select("COUNT(Id)"). + From("Posts"). + Where(sq.And{ + sq.Eq{"RootId": rootId}, + sq.Eq{"UserId": userId}, + sq.Eq{"DeleteAt": 0}, + }). + ToSql() + + if err != nil { + return errors.Wrap(err, "failed to create SQL query to count user's posts") + } + + count, err := s.GetReplica().SelectInt(queryString, args...) + + if err != nil { + return errors.Wrap(err, "failed to count user's posts in thread") + } + + updateQuery := s.getQueryBuilder().Update("Threads") + + if count == 0 { + var participants model.StringArray + err = s.getQueryBuilder(). + Select("Participants"). + From("Threads"). + Where(sq.Eq{"PostId": rootId}). + RunWith(s.GetReplica()). + QueryRow(). + Scan(&participants) + + if err != nil { + return errors.Wrap(err, "failed getting thread participants") + } + + if participants.Contains(userId) { + participants = participants.Remove(userId) + var participantsJSON []byte + participantsJSON, err = json.Marshal(participants) + if err != nil { + return errors.Wrap(err, "failed marshalling thread participants") + } + updateQuery = updateQuery.Set("Participants", string(participantsJSON)) + } + } + + updateQueryString, updateArgs, err := updateQuery. + Set("ReplyCount", sq.Expr("ReplyCount - 1")). + Where(sq.And{ + sq.Eq{"PostId": rootId}, + sq.Gt{"ReplyCount": 0}, + }). + ToSql() + + if err != nil { + return errors.Wrap(err, "failed to create SQL query to update thread") + } + + _, err = s.GetMaster().Exec(updateQueryString, updateArgs...) + if err != nil { return errors.Wrap(err, "failed to update Threads") } diff --git a/store/storetest/thread_store.go b/store/storetest/thread_store.go index cf02dad3c1..1feaf4e45e 100644 --- a/store/storetest/thread_store.go +++ b/store/storetest/thread_store.go @@ -130,7 +130,7 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) { require.NoError(t, err, "couldn't get thread") require.NotNil(t, thread) require.Equal(t, int64(1), thread.ReplyCount) - require.ElementsMatch(t, model.StringArray{newPosts[0].UserId, newPosts[1].UserId}, thread.Participants) + require.ElementsMatch(t, model.StringArray{newPosts[0].UserId}, thread.Participants) }) t.Run("Update reply should update the UpdateAt of the thread", func(t *testing.T) { @@ -181,33 +181,55 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) { }) t.Run("Deleting reply should update the thread", func(t *testing.T) { - rootPost := model.Post{} - rootPost.RootId = model.NewId() - rootPost.ChannelId = model.NewId() - rootPost.UserId = model.NewId() - rootPost.Message = NewTestId() - - replyPost := model.Post{} - replyPost.ChannelId = rootPost.ChannelId - replyPost.UserId = model.NewId() - replyPost.Message = NewTestId() - replyPost.RootId = rootPost.RootId - - newPosts, _, err := ss.Post().SaveMultiple([]*model.Post{&rootPost, &replyPost}) + o1 := model.Post{} + o1.ChannelId = model.NewId() + o1.UserId = model.NewId() + o1.Message = NewTestId() + rootPost, err := ss.Post().Save(&o1) require.NoError(t, err) - thread1, err := ss.Thread().Get(newPosts[0].RootId) + o2 := model.Post{} + o2.RootId = rootPost.Id + o2.ChannelId = rootPost.ChannelId + o2.UserId = model.NewId() + o2.Message = NewTestId() + replyPost, err := ss.Post().Save(&o2) require.NoError(t, err) - require.EqualValues(t, thread1.ReplyCount, 2) - require.Len(t, thread1.Participants, 2) + + o3 := model.Post{} + o3.RootId = rootPost.Id + o3.ChannelId = rootPost.ChannelId + o3.UserId = o2.UserId + o3.Message = NewTestId() + replyPost2, err := ss.Post().Save(&o3) + require.NoError(t, err) + + o4 := model.Post{} + o4.RootId = rootPost.Id + o4.ChannelId = rootPost.ChannelId + o4.UserId = model.NewId() + o4.Message = NewTestId() + replyPost3, err := ss.Post().Save(&o4) + require.NoError(t, err) + + thread, err := ss.Thread().Get(rootPost.Id) + require.NoError(t, err) + require.EqualValues(t, thread.ReplyCount, 3) + require.EqualValues(t, thread.Participants, model.StringArray{replyPost.UserId, replyPost3.UserId}) + + err = ss.Post().Delete(replyPost2.Id, 123, model.NewId()) + require.NoError(t, err) + thread, err = ss.Thread().Get(rootPost.Id) + require.NoError(t, err) + require.EqualValues(t, thread.ReplyCount, 2) + require.EqualValues(t, thread.Participants, model.StringArray{replyPost.UserId, replyPost3.UserId}) err = ss.Post().Delete(replyPost.Id, 123, model.NewId()) require.NoError(t, err) - - thread2, err := ss.Thread().Get(rootPost.RootId) + thread, err = ss.Thread().Get(rootPost.Id) require.NoError(t, err) - require.EqualValues(t, thread2.ReplyCount, 1) - require.Len(t, thread2.Participants, 2) + require.EqualValues(t, thread.ReplyCount, 1) + require.EqualValues(t, thread.Participants, model.StringArray{replyPost3.UserId}) }) t.Run("Deleting root post should delete the thread", func(t *testing.T) {