From c79a8a8b4af988e2cef52d95e3a4434502a8affb Mon Sep 17 00:00:00 2001 From: Harshil Sharma <18575143+harshilsharma63@users.noreply.github.com> Date: Wed, 13 Nov 2024 12:41:31 +0530 Subject: [PATCH] MM-61484 - Deleting scheduled posts when permanently deleting a user (#29152) * Deleting scheduled posts when permanently deleting a user * Updated tests * CI * Testing CI * Restored a test change * Skipping flaky test --- .../api4/outgoing_oauth_connection_test.go | 4 + server/channels/app/user.go | 4 + server/channels/app/user_test.go | 34 ++++++++ .../opentracinglayer/opentracinglayer.go | 18 ++++ .../channels/store/retrylayer/retrylayer.go | 21 +++++ .../store/sqlstore/scheduled_post_store.go | 21 +++++ server/channels/store/store.go | 1 + .../storetest/mocks/ScheduledPostStore.go | 18 ++++ .../store/storetest/scheduled_post_store.go | 82 +++++++++++++++++++ .../channels/store/timerlayer/timerlayer.go | 16 ++++ server/i18n/en.json | 4 + 11 files changed, 223 insertions(+) diff --git a/server/channels/api4/outgoing_oauth_connection_test.go b/server/channels/api4/outgoing_oauth_connection_test.go index 46639d5294..d65700669a 100644 --- a/server/channels/api4/outgoing_oauth_connection_test.go +++ b/server/channels/api4/outgoing_oauth_connection_test.go @@ -594,6 +594,8 @@ func TestClientCreateOutgoingOAuthConnection(t *testing.T) { } func TestClientUpdateOutgoingOAuthConnection(t *testing.T) { + t.Skip("https://mattermost.atlassian.net/browse/MM-61690") + os.Setenv("MM_FEATUREFLAGS_OUTGOINGOAUTHCONNECTIONS", "true") defer os.Unsetenv("MM_FEATUREFLAGS_OUTGOINGOAUTHCONNECTIONS") th := Setup(t).InitBasic() @@ -676,6 +678,8 @@ func TestClientUpdateOutgoingOAuthConnection(t *testing.T) { } func TestClientDeleteOutgoingOAuthConnection(t *testing.T) { + t.Skip("https://mattermost.atlassian.net/browse/MM-61690") + os.Setenv("MM_FEATUREFLAGS_OUTGOINGOAUTHCONNECTIONS", "true") defer os.Unsetenv("MM_FEATUREFLAGS_OUTGOINGOAUTHCONNECTIONS") th := Setup(t).InitBasic() diff --git a/server/channels/app/user.go b/server/channels/app/user.go index 335a2e5828..90490c5091 100644 --- a/server/channels/app/user.go +++ b/server/channels/app/user.go @@ -1816,6 +1816,10 @@ func (a *App) PermanentDeleteUser(rctx request.CTX, user *model.User) *model.App return model.NewAppError("PermanentDeleteUser", "app.reaction.permanent_delete_by_user.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } + if err := a.Srv().Store().ScheduledPost().PermanentDeleteByUser(user.Id); err != nil { + return model.NewAppError("PermanentDeleteUser", "app.scheduled_post.permanent_delete_by_user.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + if err := a.Srv().Store().Bot().PermanentDelete(user.Id); err != nil { var invErr *store.ErrInvalidInput switch { diff --git a/server/channels/app/user_test.go b/server/channels/app/user_test.go index 56745f1091..cb3ff18099 100644 --- a/server/channels/app/user_test.go +++ b/server/channels/app/user_test.go @@ -6,6 +6,7 @@ package app import ( "bytes" "context" + "database/sql" "encoding/json" "errors" "path/filepath" @@ -1095,6 +1096,30 @@ func TestPermanentDeleteUser(t *testing.T) { assert.NoError(t, err1) assert.Equal(t, 0, len(bots2)) + scheduledPost1 := &model.ScheduledPost{ + Draft: model.Draft{ + ChannelId: th.BasicChannel.Id, + UserId: th.BasicUser.Id, + Message: "Scheduled post 1", + }, + ScheduledAt: model.GetMillis() + 1000000, + } + + createdScheduledPost1, appErr := th.App.SaveScheduledPost(th.Context, scheduledPost1, "") + require.Nil(t, appErr) + + scheduledPost2 := &model.ScheduledPost{ + Draft: model.Draft{ + ChannelId: th.BasicChannel.Id, + UserId: th.BasicUser.Id, + Message: "Scheduled post 2", + }, + ScheduledAt: model.GetMillis() + 1000000, + } + + createdScheduledPost2, appErr := th.App.SaveScheduledPost(th.Context, scheduledPost2, "") + require.Nil(t, appErr) + err = th.App.PermanentDeleteUser(th.Context, th.BasicUser) require.Nil(t, err, "Unable to delete user. err=%v", err) @@ -1114,6 +1139,15 @@ func TestPermanentDeleteUser(t *testing.T) { exists, err := th.App.FileExists(filepath.Join("users", user.Id)) require.Nil(t, err, "Unable to stat finfo. err=%v", err) require.False(t, exists, "Profile image wasn't deleted. err=%v", err) + + // verify scheduled posts have been deleted + fetchedScheduledPost, scheduledPostErr := th.App.Srv().Store().ScheduledPost().Get(createdScheduledPost1.Id) + require.ErrorIs(t, scheduledPostErr, sql.ErrNoRows) + require.Nil(t, fetchedScheduledPost) + + fetchedScheduledPost, scheduledPostErr = th.App.Srv().Store().ScheduledPost().Get(createdScheduledPost2.Id) + require.ErrorIs(t, scheduledPostErr, sql.ErrNoRows) + require.Nil(t, fetchedScheduledPost) } func TestPasswordRecovery(t *testing.T) { diff --git a/server/channels/store/opentracinglayer/opentracinglayer.go b/server/channels/store/opentracinglayer/opentracinglayer.go index bcdf1520af..70e94a15d2 100644 --- a/server/channels/store/opentracinglayer/opentracinglayer.go +++ b/server/channels/store/opentracinglayer/opentracinglayer.go @@ -8697,6 +8697,24 @@ func (s *OpenTracingLayerScheduledPostStore) GetScheduledPostsForUser(userId str return result, err } +func (s *OpenTracingLayerScheduledPostStore) PermanentDeleteByUser(userId string) error { + origCtx := s.Root.Store.Context() + span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ScheduledPostStore.PermanentDeleteByUser") + s.Root.Store.SetContext(newCtx) + defer func() { + s.Root.Store.SetContext(origCtx) + }() + + defer span.Finish() + err := s.ScheduledPostStore.PermanentDeleteByUser(userId) + if err != nil { + span.LogFields(spanlog.Error(err)) + ext.Error.Set(span, true) + } + + return err +} + func (s *OpenTracingLayerScheduledPostStore) PermanentlyDeleteScheduledPosts(scheduledPostIDs []string) error { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ScheduledPostStore.PermanentlyDeleteScheduledPosts") diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index d80e93b45e..ab24d83547 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -9912,6 +9912,27 @@ func (s *RetryLayerScheduledPostStore) GetScheduledPostsForUser(userId string, t } +func (s *RetryLayerScheduledPostStore) PermanentDeleteByUser(userId string) error { + + tries := 0 + for { + err := s.ScheduledPostStore.PermanentDeleteByUser(userId) + if err == nil { + return nil + } + if !isRepeatableError(err) { + return err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerScheduledPostStore) PermanentlyDeleteScheduledPosts(scheduledPostIDs []string) error { tries := 0 diff --git a/server/channels/store/sqlstore/scheduled_post_store.go b/server/channels/store/sqlstore/scheduled_post_store.go index 2bde8c52e4..df62f3c512 100644 --- a/server/channels/store/sqlstore/scheduled_post_store.go +++ b/server/channels/store/sqlstore/scheduled_post_store.go @@ -287,3 +287,24 @@ func (s *SqlScheduledPostStore) UpdateOldScheduledPosts(beforeTime int64) error return nil } + +func (s *SqlScheduledPostStore) PermanentDeleteByUser(userId string) error { + query := s.getQueryBuilder(). + Delete("ScheduledPosts"). + Where(sq.Eq{"UserId": userId}) + + sql, params, err := query.ToSql() + if err != nil { + errToReturn := errors.Wrapf(err, "PermanentDeleteByUser: failed to generate SQL query for permanently deleting scheduled posts by user") + s.Logger().Error(errToReturn.Error()) + return errToReturn + } + + if _, err := s.GetMasterX().Exec(sql, params...); err != nil { + errToReturn := errors.Wrapf(err, "PermanentDeleteByUser: failed to delete scheduled posts by user from database") + s.Logger().Error(errToReturn.Error()) + return errToReturn + } + + return nil +} diff --git a/server/channels/store/store.go b/server/channels/store/store.go index 08c59f9129..f421709b80 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -1065,6 +1065,7 @@ type ScheduledPostStore interface { UpdatedScheduledPost(scheduledPost *model.ScheduledPost) error Get(scheduledPostId string) (*model.ScheduledPost, error) UpdateOldScheduledPosts(beforeTime int64) error + PermanentDeleteByUser(userId string) error } // ChannelSearchOpts contains options for searching channels. diff --git a/server/channels/store/storetest/mocks/ScheduledPostStore.go b/server/channels/store/storetest/mocks/ScheduledPostStore.go index 5ad10df3b0..e5f6c662bc 100644 --- a/server/channels/store/storetest/mocks/ScheduledPostStore.go +++ b/server/channels/store/storetest/mocks/ScheduledPostStore.go @@ -152,6 +152,24 @@ func (_m *ScheduledPostStore) GetScheduledPostsForUser(userId string, teamId str return r0, r1 } +// PermanentDeleteByUser provides a mock function with given fields: userId +func (_m *ScheduledPostStore) PermanentDeleteByUser(userId string) error { + ret := _m.Called(userId) + + if len(ret) == 0 { + panic("no return value specified for PermanentDeleteByUser") + } + + var r0 error + if rf, ok := ret.Get(0).(func(string) error); ok { + r0 = rf(userId) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // PermanentlyDeleteScheduledPosts provides a mock function with given fields: scheduledPostIDs func (_m *ScheduledPostStore) PermanentlyDeleteScheduledPosts(scheduledPostIDs []string) error { ret := _m.Called(scheduledPostIDs) diff --git a/server/channels/store/storetest/scheduled_post_store.go b/server/channels/store/storetest/scheduled_post_store.go index 4515344745..aa9ca95410 100644 --- a/server/channels/store/storetest/scheduled_post_store.go +++ b/server/channels/store/storetest/scheduled_post_store.go @@ -20,6 +20,7 @@ func TestScheduledPostStore(t *testing.T, rctx request.CTX, ss store.Store, s Sq t.Run("PermanentlyDeleteScheduledPosts", func(t *testing.T) { testPermanentlyDeleteScheduledPosts(t, rctx, ss, s) }) t.Run("UpdatedScheduledPost", func(t *testing.T) { testUpdatedScheduledPost(t, rctx, ss, s) }) t.Run("UpdateOldScheduledPosts", func(t *testing.T) { testUpdateOldScheduledPosts(t, rctx, ss, s) }) + t.Run("PermanentDeleteByUser", func(t *testing.T) { testPermanentDeleteScheduledPostsByUser(t, rctx, ss, s) }) } func testCreateScheduledPost(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore) { @@ -470,3 +471,84 @@ func testUpdateOldScheduledPosts(t *testing.T, rctx request.CTX, ss store.Store, assert.Equal(t, "", scheduledPosts[3].ErrorCode) }) } + +func testPermanentDeleteScheduledPostsByUser(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore) { + t.Run("should delete all scheduled posts for a given user", func(t *testing.T) { + userId := model.NewId() + teamId := model.NewId() + + // Create a scheduled post for the user + scheduledPost := &model.ScheduledPost{ + Draft: model.Draft{ + CreateAt: model.GetMillis(), + UserId: userId, + ChannelId: model.NewId(), + Message: "this is a scheduled post", + }, + ScheduledAt: model.GetMillis() + 100000, + } + + createdScheduledPost, err := ss.ScheduledPost().CreateScheduledPost(scheduledPost) + assert.NoError(t, err) + assert.NotEmpty(t, createdScheduledPost.Id) + + // Delete scheduled posts for the user + err = ss.ScheduledPost().PermanentDeleteByUser(userId) + assert.NoError(t, err) + + // Verify that no scheduled posts exist for the user + scheduledPosts, err := ss.ScheduledPost().GetScheduledPostsForUser(userId, teamId) + assert.NoError(t, err) + assert.Empty(t, scheduledPosts) + }) + + t.Run("should not fail if no scheduled posts exist for the user", func(t *testing.T) { + userId := model.NewId() + + // Attempt to delete scheduled posts for a user with no scheduled posts + err := ss.ScheduledPost().PermanentDeleteByUser(userId) + assert.NoError(t, err) + }) + + t.Run("should handle multiple scheduled posts for the same user", func(t *testing.T) { + userId := model.NewId() + teamId := model.NewId() + + // Create multiple scheduled posts for the user + for i := 0; i < 3; i++ { + scheduledPost := &model.ScheduledPost{ + Draft: model.Draft{ + CreateAt: model.GetMillis(), + UserId: userId, + ChannelId: model.NewId(), + Message: "this is a scheduled post", + }, + ScheduledAt: model.GetMillis() + 100000, + } + + createdScheduledPost, err := ss.ScheduledPost().CreateScheduledPost(scheduledPost) + assert.NoError(t, err) + assert.NotEmpty(t, createdScheduledPost.Id) + } + + // Delete scheduled posts for the user + err := ss.ScheduledPost().PermanentDeleteByUser(userId) + assert.NoError(t, err) + + // Verify that no scheduled posts exist for the user + scheduledPosts, err := ss.ScheduledPost().GetScheduledPostsForUser(userId, teamId) + assert.NoError(t, err) + assert.Empty(t, scheduledPosts) + }) + + t.Run("should handle empty user id", func(t *testing.T) { + err := ss.ScheduledPost().PermanentDeleteByUser("") + assert.NoError(t, err) + }) + + t.Run("should handle non-existing user id", func(t *testing.T) { + nonExistingUserId := model.NewId() + err := ss.ScheduledPost().PermanentDeleteByUser(nonExistingUserId) + assert.NoError(t, err) + }) +} diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index d8b473dea6..d15801b4cd 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -7843,6 +7843,22 @@ func (s *TimerLayerScheduledPostStore) GetScheduledPostsForUser(userId string, t return result, err } +func (s *TimerLayerScheduledPostStore) PermanentDeleteByUser(userId string) error { + start := time.Now() + + err := s.ScheduledPostStore.PermanentDeleteByUser(userId) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("ScheduledPostStore.PermanentDeleteByUser", success, elapsed) + } + return err +} + func (s *TimerLayerScheduledPostStore) PermanentlyDeleteScheduledPosts(scheduledPostIDs []string) error { start := time.Now() diff --git a/server/i18n/en.json b/server/i18n/en.json index 84d234bf56..e9845f32b4 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -6549,6 +6549,10 @@ "other": "Failed to send {{.Count}} scheduled posts." } }, + { + "id": "app.scheduled_post.permanent_delete_by_user.app_error", + "translation": "Unable to delete scheduled posts for user." + }, { "id": "app.scheme.delete.app_error", "translation": "Unable to delete this scheme."