From 8364e30a2ace037f0aac74c4ae98a2e23de7c523 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Sat, 20 Jan 2024 08:47:47 +0530 Subject: [PATCH] MM-55524: Include deactivated users in DM export (#25695) We were incorrectly excluding deactivated users while getting channel members for a DM channel, whereas we were actually exporting all users in the users array. We fix this and also correctly honor the includeArchivedChannels flag as well. https://mattermost.atlassian.net/browse/MM-55524 ```release-note Include deactivated members in a favorited DM channel export. ``` --------- Co-authored-by: Mattermost Build --- server/channels/app/export.go | 6 ++--- server/channels/app/export_test.go | 26 +++++++++---------- .../opentracinglayer/opentracinglayer.go | 4 +-- .../channels/store/retrylayer/retrylayer.go | 4 +-- .../channels/store/sqlstore/channel_store.go | 14 +++++----- server/channels/store/store.go | 2 +- .../channels/store/storetest/channel_store.go | 13 +++++++--- .../store/storetest/mocks/ChannelStore.go | 18 ++++++------- .../channels/store/timerlayer/timerlayer.go | 4 +-- 9 files changed, 49 insertions(+), 42 deletions(-) diff --git a/server/channels/app/export.go b/server/channels/app/export.go index dfb5cafea1..81de9fceb4 100644 --- a/server/channels/app/export.go +++ b/server/channels/app/export.go @@ -120,7 +120,7 @@ func (a *App) BulkExport(ctx request.CTX, writer io.Writer, outPath string, job } ctx.Logger().Info("Bulk export: exporting direct channels") - if err = a.exportAllDirectChannels(ctx, job, writer); err != nil { + if err = a.exportAllDirectChannels(ctx, job, writer, opts.IncludeArchivedChannels); err != nil { return err } @@ -663,11 +663,11 @@ func (a *App) copyEmojiImages(emojiId string, emojiImagePath string, pathToDir s return nil } -func (a *App) exportAllDirectChannels(ctx request.CTX, job *model.Job, writer io.Writer) *model.AppError { +func (a *App) exportAllDirectChannels(ctx request.CTX, job *model.Job, writer io.Writer, includeArchivedChannels bool) *model.AppError { afterId := strings.Repeat("0", 26) cnt := 0 for { - channels, err := a.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, afterId) + channels, err := a.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, afterId, includeArchivedChannels) if err != nil { return model.NewAppError("exportAllDirectChannels", "app.channel.get_all_direct.app_error", nil, "", http.StatusInternalServerError).Wrap(err) } diff --git a/server/channels/app/export_test.go b/server/channels/app/export_test.go index 4242a47eac..31f159b711 100644 --- a/server/channels/app/export_test.go +++ b/server/channels/app/export_test.go @@ -249,14 +249,14 @@ func TestExportDMChannel(t *testing.T) { err := th1.App.BulkExport(th1.Context, &b, "somePath", nil, model.BulkExportOpts{}) require.Nil(t, err) - channels, nErr := th1.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000") + channels, nErr := th1.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) require.NoError(t, nErr) assert.Equal(t, 1, len(channels)) th2 := Setup(t).InitBasic() defer th2.TearDown() - channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000") + channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) require.NoError(t, nErr) assert.Equal(t, 0, len(channels)) @@ -266,7 +266,7 @@ func TestExportDMChannel(t *testing.T) { assert.Equal(t, 0, i) // Ensure the Members of the imported DM channel is the same was from the exported - channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000") + channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) require.NoError(t, nErr) require.Equal(t, 1, len(channels)) assert.ElementsMatch(t, []string{th1.BasicUser.Username, th1.BasicUser2.Username}, *channels[0].Members) @@ -285,7 +285,7 @@ func TestExportDMChannel(t *testing.T) { // DM Channel th1.CreateDmChannel(th1.BasicUser2) - channels, nErr := th1.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000") + channels, nErr := th1.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) require.NoError(t, nErr) assert.Equal(t, 1, len(channels)) @@ -303,7 +303,7 @@ func TestExportDMChannel(t *testing.T) { err, _ = th2.App.BulkImport(th2.Context, &b, nil, true, 5) require.Nil(t, err) - channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000") + channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) require.NoError(t, nErr) assert.Empty(t, channels) }) @@ -320,14 +320,14 @@ func TestExportDMChannelToSelf(t *testing.T) { err := th1.App.BulkExport(th1.Context, &b, "somePath", nil, model.BulkExportOpts{}) require.Nil(t, err) - channels, nErr := th1.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000") + channels, nErr := th1.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) require.NoError(t, nErr) assert.Equal(t, 1, len(channels)) th2 := Setup(t) defer th2.TearDown() - channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000") + channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) require.NoError(t, nErr) assert.Equal(t, 0, len(channels)) @@ -336,7 +336,7 @@ func TestExportDMChannelToSelf(t *testing.T) { assert.Nil(t, err) assert.EqualValues(t, 0, i) - channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000") + channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) require.NoError(t, nErr) assert.Equal(t, 1, len(channels)) assert.Equal(t, 1, len((*channels[0].Members))) @@ -358,7 +358,7 @@ func TestExportGMChannel(t *testing.T) { err := th1.App.BulkExport(th1.Context, &b, "somePath", nil, model.BulkExportOpts{}) require.Nil(t, err) - channels, nErr := th1.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000") + channels, nErr := th1.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) require.NoError(t, nErr) assert.Equal(t, 1, len(channels)) @@ -367,7 +367,7 @@ func TestExportGMChannel(t *testing.T) { th2 := Setup(t) defer th2.TearDown() - channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000") + channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) require.NoError(t, nErr) assert.Equal(t, 0, len(channels)) } @@ -390,7 +390,7 @@ func TestExportGMandDMChannels(t *testing.T) { err := th1.App.BulkExport(th1.Context, &b, "somePath", nil, model.BulkExportOpts{}) require.Nil(t, err) - channels, nErr := th1.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000") + channels, nErr := th1.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) require.NoError(t, nErr) assert.Equal(t, 2, len(channels)) @@ -399,7 +399,7 @@ func TestExportGMandDMChannels(t *testing.T) { th2 := Setup(t) defer th2.TearDown() - channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000") + channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) require.NoError(t, nErr) assert.Equal(t, 0, len(channels)) @@ -409,7 +409,7 @@ func TestExportGMandDMChannels(t *testing.T) { assert.Equal(t, 0, i) // Ensure the Members of the imported GM channel is the same was from the exported - channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000") + channels, nErr = th2.App.Srv().Store().Channel().GetAllDirectChannelsForExportAfter(1000, "00000000", false) require.NoError(t, nErr) // Adding some determinism so its possible to assert on slice index diff --git a/server/channels/store/opentracinglayer/opentracinglayer.go b/server/channels/store/opentracinglayer/opentracinglayer.go index e3421f19e8..d8c3b82b73 100644 --- a/server/channels/store/opentracinglayer/opentracinglayer.go +++ b/server/channels/store/opentracinglayer/opentracinglayer.go @@ -1073,7 +1073,7 @@ func (s *OpenTracingLayerChannelStore) GetAllChannelsForExportAfter(limit int, a return result, err } -func (s *OpenTracingLayerChannelStore) GetAllDirectChannelsForExportAfter(limit int, afterID string) ([]*model.DirectChannelForExport, error) { +func (s *OpenTracingLayerChannelStore) GetAllDirectChannelsForExportAfter(limit int, afterID string, includeArchivedChannels bool) ([]*model.DirectChannelForExport, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ChannelStore.GetAllDirectChannelsForExportAfter") s.Root.Store.SetContext(newCtx) @@ -1082,7 +1082,7 @@ func (s *OpenTracingLayerChannelStore) GetAllDirectChannelsForExportAfter(limit }() defer span.Finish() - result, err := s.ChannelStore.GetAllDirectChannelsForExportAfter(limit, afterID) + result, err := s.ChannelStore.GetAllDirectChannelsForExportAfter(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 99c28058a3..498de578c8 100644 --- a/server/channels/store/retrylayer/retrylayer.go +++ b/server/channels/store/retrylayer/retrylayer.go @@ -1175,11 +1175,11 @@ func (s *RetryLayerChannelStore) GetAllChannelsForExportAfter(limit int, afterID } -func (s *RetryLayerChannelStore) GetAllDirectChannelsForExportAfter(limit int, afterID string) ([]*model.DirectChannelForExport, error) { +func (s *RetryLayerChannelStore) GetAllDirectChannelsForExportAfter(limit int, afterID string, includeArchivedChannels bool) ([]*model.DirectChannelForExport, error) { tries := 0 for { - result, err := s.ChannelStore.GetAllDirectChannelsForExportAfter(limit, afterID) + result, err := s.ChannelStore.GetAllDirectChannelsForExportAfter(limit, afterID, includeArchivedChannels) if err == nil { return result, nil } diff --git a/server/channels/store/sqlstore/channel_store.go b/server/channels/store/sqlstore/channel_store.go index b89df6a19f..9f0a0e7c8c 100644 --- a/server/channels/store/sqlstore/channel_store.go +++ b/server/channels/store/sqlstore/channel_store.go @@ -4192,19 +4192,24 @@ func (s SqlChannelStore) GetChannelMembersForExport(userId string, teamId string return members, nil } -func (s SqlChannelStore) GetAllDirectChannelsForExportAfter(limit int, afterId string) ([]*model.DirectChannelForExport, error) { +func (s SqlChannelStore) GetAllDirectChannelsForExportAfter(limit int, afterId string, includeArchivedChannels bool) ([]*model.DirectChannelForExport, error) { directChannelsForExport := []*model.DirectChannelForExport{} query := s.getQueryBuilder(). Select("Channels.*"). From("Channels"). Where(sq.And{ sq.Gt{"Channels.Id": afterId}, - sq.Eq{"Channels.DeleteAt": int(0)}, sq.Eq{"Channels.Type": []model.ChannelType{model.ChannelTypeDirect, model.ChannelTypeGroup}}, }). OrderBy("Channels.Id"). Limit(uint64(limit)) + if !includeArchivedChannels { + query = query.Where( + sq.Eq{"Channels.DeleteAt": int(0)}, + ) + } + queryString, args, err := query.ToSql() if err != nil { return nil, errors.Wrap(err, "channel_tosql") @@ -4222,10 +4227,7 @@ func (s SqlChannelStore) GetAllDirectChannelsForExportAfter(limit int, afterId s Select("u.Username as Username, ChannelId, UserId, cm.Roles as Roles, LastViewedAt, MsgCount, MentionCount, MentionCountRoot, COALESCE(UrgentMentionCount, 0) UrgentMentionCount, cm.NotifyProps as NotifyProps, LastUpdateAt, SchemeUser, SchemeAdmin, (SchemeGuest IS NOT NULL AND SchemeGuest) as SchemeGuest"). From("ChannelMembers cm"). Join("Users u ON ( u.Id = cm.UserId )"). - Where(sq.And{ - sq.Eq{"cm.ChannelId": channelIds}, - sq.Eq{"u.DeleteAt": int(0)}, - }) + Where(sq.Eq{"cm.ChannelId": channelIds}) queryString, args, err = query.ToSql() if err != nil { diff --git a/server/channels/store/store.go b/server/channels/store/store.go index f2e7b7613a..2749bf714a 100644 --- a/server/channels/store/store.go +++ b/server/channels/store/store.go @@ -294,7 +294,7 @@ type ChannelStore interface { DeleteSidebarCategory(categoryID string) error DeleteAllSidebarChannelForChannel(channelID string) error GetAllChannelsForExportAfter(limit int, afterID string) ([]*model.ChannelForExport, error) - GetAllDirectChannelsForExportAfter(limit int, afterID string) ([]*model.DirectChannelForExport, error) + GetAllDirectChannelsForExportAfter(limit int, afterID string, includeArchivedChannels bool) ([]*model.DirectChannelForExport, error) GetChannelMembersForExport(userID string, teamID string, includeArchivedChannel bool) ([]*model.ChannelMemberForExport, error) RemoveAllDeactivatedMembers(ctx request.CTX, channelID string) error GetChannelsBatchForIndexing(startTime int64, startChannelID string, limit int) ([]*model.Channel, error) diff --git a/server/channels/store/storetest/channel_store.go b/server/channels/store/storetest/channel_store.go index 0e0049ff6b..514f1135fc 100644 --- a/server/channels/store/storetest/channel_store.go +++ b/server/channels/store/storetest/channel_store.go @@ -7746,7 +7746,7 @@ func testChannelStoreExportAllDirectChannels(t *testing.T, rctx request.CTX, ss ss.Channel().SaveDirectChannel(rctx, &o1, &m1, &m2) - d1, nErr := ss.Channel().GetAllDirectChannelsForExportAfter(10000, strings.Repeat("0", 26)) + d1, nErr := ss.Channel().GetAllDirectChannelsForExportAfter(10000, strings.Repeat("0", 26), false) assert.NoError(t, nErr) assert.Len(t, d1, 2) @@ -7809,7 +7809,7 @@ func testChannelStoreExportAllDirectChannelsExcludePrivateAndPublic(t *testing.T ss.Channel().SaveDirectChannel(rctx, &o1, &m1, &m2) - d1, nErr := ss.Channel().GetAllDirectChannelsForExportAfter(10000, strings.Repeat("0", 26)) + d1, nErr := ss.Channel().GetAllDirectChannelsForExportAfter(10000, strings.Repeat("0", 26), false) assert.NoError(t, nErr) assert.Len(t, d1, 1) assert.Equal(t, o1.DisplayName, d1[0].DisplayName) @@ -7837,6 +7837,7 @@ func testChannelStoreExportAllDirectChannelsDeletedChannel(t *testing.T, rctx re u2 := &model.User{} u2.Email = MakeEmail() + u2.DeleteAt = 123000 u2.Nickname = model.NewId() _, err = ss.User().Save(u2) require.NoError(t, err) @@ -7859,10 +7860,14 @@ func testChannelStoreExportAllDirectChannelsDeletedChannel(t *testing.T, rctx re nErr = ss.Channel().SetDeleteAt(o1.Id, 1, 1) require.NoError(t, nErr, "channel should have been deleted") - d1, nErr := ss.Channel().GetAllDirectChannelsForExportAfter(10000, strings.Repeat("0", 26)) + d1, nErr := ss.Channel().GetAllDirectChannelsForExportAfter(10000, strings.Repeat("0", 26), false) assert.NoError(t, nErr) + assert.Len(t, d1, 0) - assert.Equal(t, 0, len(d1)) + d1, nErr = ss.Channel().GetAllDirectChannelsForExportAfter(10000, strings.Repeat("0", 26), true) + assert.NoError(t, nErr) + assert.Len(t, d1, 1) + assert.Len(t, *d1[0].Members, 2) // Manually truncate Channels table until testlib can handle cleanups s.GetMasterX().Exec("TRUNCATE Channels") diff --git a/server/channels/store/storetest/mocks/ChannelStore.go b/server/channels/store/storetest/mocks/ChannelStore.go index 8e6a6901de..80dc1c37ba 100644 --- a/server/channels/store/storetest/mocks/ChannelStore.go +++ b/server/channels/store/storetest/mocks/ChannelStore.go @@ -586,25 +586,25 @@ func (_m *ChannelStore) GetAllChannelsForExportAfter(limit int, afterID string) return r0, r1 } -// GetAllDirectChannelsForExportAfter provides a mock function with given fields: limit, afterID -func (_m *ChannelStore) GetAllDirectChannelsForExportAfter(limit int, afterID string) ([]*model.DirectChannelForExport, error) { - ret := _m.Called(limit, afterID) +// GetAllDirectChannelsForExportAfter provides a mock function with given fields: limit, afterID, includeArchivedChannels +func (_m *ChannelStore) GetAllDirectChannelsForExportAfter(limit int, afterID string, includeArchivedChannels bool) ([]*model.DirectChannelForExport, error) { + ret := _m.Called(limit, afterID, includeArchivedChannels) var r0 []*model.DirectChannelForExport var r1 error - if rf, ok := ret.Get(0).(func(int, string) ([]*model.DirectChannelForExport, error)); ok { - return rf(limit, afterID) + if rf, ok := ret.Get(0).(func(int, string, bool) ([]*model.DirectChannelForExport, error)); ok { + return rf(limit, afterID, includeArchivedChannels) } - if rf, ok := ret.Get(0).(func(int, string) []*model.DirectChannelForExport); ok { - r0 = rf(limit, afterID) + if rf, ok := ret.Get(0).(func(int, string, bool) []*model.DirectChannelForExport); ok { + r0 = rf(limit, afterID, includeArchivedChannels) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]*model.DirectChannelForExport) } } - 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/timerlayer/timerlayer.go b/server/channels/store/timerlayer/timerlayer.go index 21fb9ca1c1..fa069c99ba 100644 --- a/server/channels/store/timerlayer/timerlayer.go +++ b/server/channels/store/timerlayer/timerlayer.go @@ -1013,10 +1013,10 @@ func (s *TimerLayerChannelStore) GetAllChannelsForExportAfter(limit int, afterID return result, err } -func (s *TimerLayerChannelStore) GetAllDirectChannelsForExportAfter(limit int, afterID string) ([]*model.DirectChannelForExport, error) { +func (s *TimerLayerChannelStore) GetAllDirectChannelsForExportAfter(limit int, afterID string, includeArchivedChannels bool) ([]*model.DirectChannelForExport, error) { start := time.Now() - result, err := s.ChannelStore.GetAllDirectChannelsForExportAfter(limit, afterID) + result, err := s.ChannelStore.GetAllDirectChannelsForExportAfter(limit, afterID, includeArchivedChannels) elapsed := float64(time.Since(start)) / float64(time.Second) if s.Root.Metrics != nil {