From 28cb8d0441a085017424c3719ba6203b516304e1 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Wed, 14 Feb 2024 20:59:46 +0530 Subject: [PATCH] MM-56579: Include deactivated users from DM posts (#25985) There were 2 separate bugs here: 1. We were not including deactivated users while fetching posts from DM channels. 2. We were not respecting the includeArchivedChannels flag while fetching DM channels. We fix both of these issues here. ```release-note Include posts from deactivated users in DM channel export. Also respect the --include-archived-channels flag for DM channels. ``` https://mattermost.atlassian.net/browse/MM-56579 Co-authored-by: Mattermost Build --- server/channels/app/export.go | 6 +++--- server/channels/app/export_test.go | 18 ++++++++-------- .../opentracinglayer/opentracinglayer.go | 4 ++-- .../channels/store/retrylayer/retrylayer.go | 4 ++-- server/channels/store/sqlstore/post_store.go | 10 ++++++--- server/channels/store/store.go | 2 +- .../store/storetest/mocks/PostStore.go | 18 ++++++++-------- server/channels/store/storetest/post_store.go | 21 ++++++++----------- .../channels/store/timerlayer/timerlayer.go | 4 ++-- 9 files changed, 44 insertions(+), 43 deletions(-) diff --git a/server/channels/app/export.go b/server/channels/app/export.go index 81de9fceb4..dc00aee041 100644 --- a/server/channels/app/export.go +++ b/server/channels/app/export.go @@ -125,7 +125,7 @@ func (a *App) BulkExport(ctx request.CTX, writer io.Writer, outPath string, job } ctx.Logger().Info("Bulk export: exporting direct posts") - directAttachments, err := a.exportAllDirectPosts(ctx, job, writer, opts.IncludeAttachments) + directAttachments, err := a.exportAllDirectPosts(ctx, job, writer, opts.IncludeAttachments, opts.IncludeArchivedChannels) if err != nil { return err } @@ -729,7 +729,7 @@ func (a *App) buildFavoritedByList(channelID string) ([]string, *model.AppError) return userIDs, nil } -func (a *App) exportAllDirectPosts(ctx request.CTX, job *model.Job, writer io.Writer, withAttachments bool) ([]imports.AttachmentImportData, *model.AppError) { +func (a *App) exportAllDirectPosts(ctx request.CTX, job *model.Job, writer io.Writer, withAttachments, includeArchivedChannels bool) ([]imports.AttachmentImportData, *model.AppError) { var attachments []imports.AttachmentImportData afterId := strings.Repeat("0", 26) var postProcessCount uint64 @@ -742,7 +742,7 @@ func (a *App) exportAllDirectPosts(ctx request.CTX, job *model.Job, writer io.Wr logCheckpoint = time.Now() } - posts, err := a.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, afterId) + posts, err := a.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, afterId, includeArchivedChannels) if err != nil { return nil, model.NewAppError("exportAllDirectPosts", "app.post.get_direct_posts.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } diff --git a/server/channels/app/export_test.go b/server/channels/app/export_test.go index 31f159b711..6e3a4fb513 100644 --- a/server/channels/app/export_test.go +++ b/server/channels/app/export_test.go @@ -465,7 +465,7 @@ func TestExportDMandGMPost(t *testing.T) { } th1.App.CreatePost(th1.Context, p4, gmChannel, false, true) - posts, err := th1.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000") + posts, err := th1.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000", false) require.NoError(t, err) assert.Equal(t, 4, len(posts)) @@ -478,7 +478,7 @@ func TestExportDMandGMPost(t *testing.T) { th2 := Setup(t) defer th2.TearDown() - posts, err = th2.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000") + posts, err = th2.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000", false) require.NoError(t, err) assert.Equal(t, 0, len(posts)) @@ -487,7 +487,7 @@ func TestExportDMandGMPost(t *testing.T) { assert.Nil(t, appErr) assert.Equal(t, 0, i) - posts, err = th2.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000") + posts, err = th2.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000", false) require.NoError(t, err) // Adding some determinism so its possible to assert on slice index @@ -538,7 +538,7 @@ func TestExportPostWithProps(t *testing.T) { } th1.App.CreatePost(th1.Context, p2, gmChannel, false, true) - posts, err := th1.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000") + posts, err := th1.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000", false) require.NoError(t, err) assert.Len(t, posts, 2) require.NotEmpty(t, posts[0].Props) @@ -553,7 +553,7 @@ func TestExportPostWithProps(t *testing.T) { th2 := Setup(t) defer th2.TearDown() - posts, err = th2.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000") + posts, err = th2.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000", false) require.NoError(t, err) assert.Len(t, posts, 0) @@ -562,7 +562,7 @@ func TestExportPostWithProps(t *testing.T) { assert.Nil(t, appErr) assert.Equal(t, 0, i) - posts, err = th2.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000") + posts, err = th2.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000", false) require.NoError(t, err) // Adding some determinism so its possible to assert on slice index @@ -586,7 +586,7 @@ func TestExportDMPostWithSelf(t *testing.T) { err := th1.App.BulkExport(th1.Context, &b, "somePath", nil, model.BulkExportOpts{}) require.Nil(t, err) - posts, nErr := th1.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000") + posts, nErr := th1.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000", false) require.NoError(t, nErr) assert.Equal(t, 1, len(posts)) @@ -595,7 +595,7 @@ func TestExportDMPostWithSelf(t *testing.T) { th2 := Setup(t) defer th2.TearDown() - posts, nErr = th2.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000") + posts, nErr = th2.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000", false) require.NoError(t, nErr) assert.Equal(t, 0, len(posts)) @@ -604,7 +604,7 @@ func TestExportDMPostWithSelf(t *testing.T) { assert.Nil(t, err) assert.Equal(t, 0, i) - posts, nErr = th2.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000") + posts, nErr = th2.App.Srv().Store().Post().GetDirectPostParentsForExportAfter(1000, "0000000", false) require.NoError(t, nErr) assert.Equal(t, 1, len(posts)) assert.Equal(t, 1, len((*posts[0].ChannelMembers))) diff --git a/server/channels/store/opentracinglayer/opentracinglayer.go b/server/channels/store/opentracinglayer/opentracinglayer.go index c33100c110..5883ec90d5 100644 --- a/server/channels/store/opentracinglayer/opentracinglayer.go +++ b/server/channels/store/opentracinglayer/opentracinglayer.go @@ -6192,7 +6192,7 @@ func (s *OpenTracingLayerPostStore) Get(ctx context.Context, id string, opts mod return result, err } -func (s *OpenTracingLayerPostStore) GetDirectPostParentsForExportAfter(limit int, afterID string) ([]*model.DirectPostForExport, error) { +func (s *OpenTracingLayerPostStore) GetDirectPostParentsForExportAfter(limit int, afterID string, includeArchivedChannels bool) ([]*model.DirectPostForExport, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "PostStore.GetDirectPostParentsForExportAfter") s.Root.Store.SetContext(newCtx) @@ -6201,7 +6201,7 @@ func (s *OpenTracingLayerPostStore) GetDirectPostParentsForExportAfter(limit int }() defer span.Finish() - result, err := s.PostStore.GetDirectPostParentsForExportAfter(limit, afterID) + result, err := s.PostStore.GetDirectPostParentsForExportAfter(limit, afterID, includeArchivedChannels) if err != nil { span.LogFields(spanlog.Error(err)) ext.Error.Set(span, true) diff --git a/server/channels/store/retrylayer/retrylayer.go b/server/channels/store/retrylayer/retrylayer.go index 89587f1f24..d10a5c02ae 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -7028,11 +7028,11 @@ func (s *RetryLayerPostStore) Get(ctx context.Context, id string, opts model.Get } -func (s *RetryLayerPostStore) GetDirectPostParentsForExportAfter(limit int, afterID string) ([]*model.DirectPostForExport, error) { +func (s *RetryLayerPostStore) GetDirectPostParentsForExportAfter(limit int, afterID string, includeArchivedChannels bool) ([]*model.DirectPostForExport, error) { tries := 0 for { - result, err := s.PostStore.GetDirectPostParentsForExportAfter(limit, afterID) + result, err := s.PostStore.GetDirectPostParentsForExportAfter(limit, afterID, includeArchivedChannels) if err == nil { return result, nil } diff --git a/server/channels/store/sqlstore/post_store.go b/server/channels/store/sqlstore/post_store.go index 1ac4bffae8..e3a64b2aed 100644 --- a/server/channels/store/sqlstore/post_store.go +++ b/server/channels/store/sqlstore/post_store.go @@ -2686,7 +2686,7 @@ func (s *SqlPostStore) GetRepliesForExport(rootId string) ([]*model.ReplyForExpo return posts, nil } -func (s *SqlPostStore) GetDirectPostParentsForExportAfter(limit int, afterId string) ([]*model.DirectPostForExport, error) { +func (s *SqlPostStore) GetDirectPostParentsForExportAfter(limit int, afterId string, includeArchivedChannels bool) ([]*model.DirectPostForExport, error) { query := s.getQueryBuilder(). Select("p.*", "Users.Username as User"). From("Posts p"). @@ -2696,13 +2696,17 @@ func (s *SqlPostStore) GetDirectPostParentsForExportAfter(limit int, afterId str sq.Gt{"p.Id": afterId}, sq.Eq{"p.RootId": ""}, sq.Eq{"p.DeleteAt": 0}, - sq.Eq{"Channels.DeleteAt": 0}, - sq.Eq{"Users.DeleteAt": 0}, sq.Eq{"Channels.Type": []model.ChannelType{model.ChannelTypeDirect, model.ChannelTypeGroup}}, }). OrderBy("p.Id"). Limit(uint64(limit)) + if !includeArchivedChannels { + query = query.Where( + sq.Eq{"Channels.DeleteAt": 0}, + ) + } + queryString, args, err := query.ToSql() if err != nil { return nil, errors.Wrap(err, "post_tosql") diff --git a/server/channels/store/store.go b/server/channels/store/store.go index ffba76942d..2ea88f50ca 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -390,7 +390,7 @@ type PostStore interface { GetMaxPostSize() int GetParentsForExportAfter(limit int, afterID string, includeArchivedChannels bool) ([]*model.PostForExport, error) GetRepliesForExport(parentID string) ([]*model.ReplyForExport, error) - GetDirectPostParentsForExportAfter(limit int, afterID string) ([]*model.DirectPostForExport, error) + GetDirectPostParentsForExportAfter(limit int, afterID string, includeArchivedChannels bool) ([]*model.DirectPostForExport, error) SearchPostsForUser(rctx request.CTX, paramsList []*model.SearchParams, userID, teamID string, page, perPage int) (*model.PostSearchResults, error) GetOldestEntityCreationTime() (int64, error) HasAutoResponsePostByUserSince(options model.GetPostsSinceOptions, userId string) (bool, error) diff --git a/server/channels/store/storetest/mocks/PostStore.go b/server/channels/store/storetest/mocks/PostStore.go index 3e841c30ae..8f92606387 100644 --- a/server/channels/store/storetest/mocks/PostStore.go +++ b/server/channels/store/storetest/mocks/PostStore.go @@ -141,25 +141,25 @@ func (_m *PostStore) Get(ctx context.Context, id string, opts model.GetPostsOpti return r0, r1 } -// GetDirectPostParentsForExportAfter provides a mock function with given fields: limit, afterID -func (_m *PostStore) GetDirectPostParentsForExportAfter(limit int, afterID string) ([]*model.DirectPostForExport, error) { - ret := _m.Called(limit, afterID) +// GetDirectPostParentsForExportAfter provides a mock function with given fields: limit, afterID, includeArchivedChannels +func (_m *PostStore) GetDirectPostParentsForExportAfter(limit int, afterID string, includeArchivedChannels bool) ([]*model.DirectPostForExport, error) { + ret := _m.Called(limit, afterID, includeArchivedChannels) var r0 []*model.DirectPostForExport var r1 error - if rf, ok := ret.Get(0).(func(int, string) ([]*model.DirectPostForExport, error)); ok { - return rf(limit, afterID) + if rf, ok := ret.Get(0).(func(int, string, bool) ([]*model.DirectPostForExport, error)); ok { + return rf(limit, afterID, includeArchivedChannels) } - if rf, ok := ret.Get(0).(func(int, string) []*model.DirectPostForExport); ok { - r0 = rf(limit, afterID) + if rf, ok := ret.Get(0).(func(int, string, bool) []*model.DirectPostForExport); ok { + r0 = rf(limit, afterID, includeArchivedChannels) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]*model.DirectPostForExport) } } - if rf, ok := ret.Get(1).(func(int, string) error); ok { - r1 = rf(limit, afterID) + if rf, ok := ret.Get(1).(func(int, string, bool) error); ok { + r1 = rf(limit, afterID, includeArchivedChannels) } else { r1 = ret.Error(1) } diff --git a/server/channels/store/storetest/post_store.go b/server/channels/store/storetest/post_store.go index 4d4b364501..dc26a3085d 100644 --- a/server/channels/store/storetest/post_store.go +++ b/server/channels/store/storetest/post_store.go @@ -4457,7 +4457,7 @@ func testPostStoreGetDirectPostParentsForExportAfter(t *testing.T, rctx request. p1, nErr = ss.Post().Save(p1) require.NoError(t, nErr) - r1, nErr := ss.Post().GetDirectPostParentsForExportAfter(10000, strings.Repeat("0", 26)) + r1, nErr := ss.Post().GetDirectPostParentsForExportAfter(10000, strings.Repeat("0", 26), false) assert.NoError(t, nErr) assert.Equal(t, p1.Message, r1[0].Message) @@ -4514,20 +4514,17 @@ func testPostStoreGetDirectPostParentsForExportAfterDeleted(t *testing.T, rctx r p1.UserId = u1.Id p1.Message = NewTestId() p1.CreateAt = 1000 - p1, nErr = ss.Post().Save(p1) + _, nErr = ss.Post().Save(p1) require.NoError(t, nErr) - o1a := p1.Clone() - o1a.DeleteAt = 1 - o1a.Message = p1.Message + "BBBBBBBBBB" - _, nErr = ss.Post().Update(rctx, o1a, p1) - require.NoError(t, nErr) - - r1, nErr := ss.Post().GetDirectPostParentsForExportAfter(10000, strings.Repeat("0", 26)) + r1, nErr := ss.Post().GetDirectPostParentsForExportAfter(10000, strings.Repeat("0", 26), false) assert.NoError(t, nErr) - assert.Equal(t, 0, len(r1)) + r1, nErr = ss.Post().GetDirectPostParentsForExportAfter(10000, strings.Repeat("0", 26), true) + assert.NoError(t, nErr) + assert.Equal(t, 1, len(r1)) + // Manually truncate Channels table until testlib can handle cleanups s.GetMasterX().Exec("TRUNCATE Channels") } @@ -4583,7 +4580,7 @@ func testPostStoreGetDirectPostParentsForExportAfterBatched(t *testing.T, rctx r sort.Slice(postIds, func(i, j int) bool { return postIds[i] < postIds[j] }) // Get all posts - r1, err := ss.Post().GetDirectPostParentsForExportAfter(10000, strings.Repeat("0", 26)) + r1, err := ss.Post().GetDirectPostParentsForExportAfter(10000, strings.Repeat("0", 26), false) assert.NoError(t, err) assert.Equal(t, len(postIds), len(r1)) var exportedPostIds []string @@ -4594,7 +4591,7 @@ func testPostStoreGetDirectPostParentsForExportAfterBatched(t *testing.T, rctx r assert.ElementsMatch(t, postIds, exportedPostIds) // Get 100 - r1, err = ss.Post().GetDirectPostParentsForExportAfter(100, strings.Repeat("0", 26)) + r1, err = ss.Post().GetDirectPostParentsForExportAfter(100, strings.Repeat("0", 26), false) assert.NoError(t, err) assert.Equal(t, 100, len(r1)) exportedPostIds = []string{} diff --git a/server/channels/store/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index 26034920b7..000181784f 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -5610,10 +5610,10 @@ func (s *TimerLayerPostStore) Get(ctx context.Context, id string, opts model.Get return result, err } -func (s *TimerLayerPostStore) GetDirectPostParentsForExportAfter(limit int, afterID string) ([]*model.DirectPostForExport, error) { +func (s *TimerLayerPostStore) GetDirectPostParentsForExportAfter(limit int, afterID string, includeArchivedChannels bool) ([]*model.DirectPostForExport, error) { start := time.Now() - result, err := s.PostStore.GetDirectPostParentsForExportAfter(limit, afterID) + result, err := s.PostStore.GetDirectPostParentsForExportAfter(limit, afterID, includeArchivedChannels) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil {