From 9a4f3ce1e5b22a297eeb00b2aaff44f88304ae8c Mon Sep 17 00:00:00 2001 From: George Goldberg Date: Mon, 8 Oct 2018 16:39:03 +0100 Subject: MM-12251: Add flag to MoveChannel to remove all deactivated users. (#9515) --- app/channel.go | 8 +++- app/channel_test.go | 31 ++++++++++++++- cmd/mattermost/commands/channel.go | 9 +++-- i18n/en.json | 4 ++ store/sqlstore/channel_store.go | 26 +++++++++++++ store/store.go | 1 + store/storetest/channel_store.go | 73 +++++++++++++++++++++++++++++++++++ store/storetest/mocks/ChannelStore.go | 16 ++++++++ 8 files changed, 162 insertions(+), 6 deletions(-) diff --git a/app/channel.go b/app/channel.go index 54b589175..93037cf05 100644 --- a/app/channel.go +++ b/app/channel.go @@ -1670,7 +1670,13 @@ func (a *App) PermanentDeleteChannel(channel *model.Channel) *model.AppError { // This function is intended for use from the CLI. It is not robust against people joining the channel while the move // is in progress, and therefore should not be used from the API without first fixing this potential race condition. -func (a *App) MoveChannel(team *model.Team, channel *model.Channel, user *model.User) *model.AppError { +func (a *App) MoveChannel(team *model.Team, channel *model.Channel, user *model.User, removeDeactivatedMembers bool) *model.AppError { + if removeDeactivatedMembers { + if result := <-a.Srv.Store.Channel().RemoveAllDeactivatedMembers(channel.Id); result.Err != nil { + return result.Err + } + } + // Check that all channel members are in the destination team. channelMembers, err := a.GetChannelMembersPage(channel.Id, 0, 10000000) if err != nil { diff --git a/app/channel_test.go b/app/channel_test.go index 0501b9406..4b09bbb78 100644 --- a/app/channel_test.go +++ b/app/channel_test.go @@ -98,7 +98,7 @@ func TestMoveChannel(t *testing.T) { t.Fatal(err) } - if err := th.App.MoveChannel(targetTeam, channel1, th.BasicUser); err == nil { + if err := th.App.MoveChannel(targetTeam, channel1, th.BasicUser, false); err == nil { t.Fatal("Should have failed due to mismatched members.") } @@ -106,7 +106,34 @@ func TestMoveChannel(t *testing.T) { t.Fatal(err) } - if err := th.App.MoveChannel(targetTeam, channel1, th.BasicUser); err != nil { + if err := th.App.MoveChannel(targetTeam, channel1, th.BasicUser, false); err != nil { + t.Fatal(err) + } + + // Test moving a channel with a deactivated user who isn't in the destination team. + // It should fail, unless removeDeactivatedMembers is true. + deacivatedUser := th.CreateUser() + channel2 := th.CreateChannel(sourceTeam) + + if _, err := th.App.AddUserToTeam(sourceTeam.Id, deacivatedUser.Id, ""); err != nil { + t.Fatal(err) + } + if _, err := th.App.AddUserToChannel(th.BasicUser, channel2); err != nil { + t.Fatal(err) + } + if _, err := th.App.AddUserToChannel(deacivatedUser, channel2); err != nil { + t.Fatal(err) + } + + if _, err := th.App.UpdateActive(deacivatedUser, false); err != nil { + t.Fatal(err) + } + + if err := th.App.MoveChannel(targetTeam, channel2, th.BasicUser, false); err == nil { + t.Fatal("Should have failed due to mismatched deacivated member.") + } + + if err := th.App.MoveChannel(targetTeam, channel2, th.BasicUser, true); err != nil { t.Fatal(err) } } diff --git a/cmd/mattermost/commands/channel.go b/cmd/mattermost/commands/channel.go index 7fb8fea8c..82fb1e966 100644 --- a/cmd/mattermost/commands/channel.go +++ b/cmd/mattermost/commands/channel.go @@ -117,6 +117,7 @@ func init() { ChannelCreateCmd.Flags().Bool("private", false, "Create a private channel.") MoveChannelsCmd.Flags().String("username", "", "Required. Username who is moving the channel.") + MoveChannelsCmd.Flags().Bool("remove-deactivated-users", false, "Automatically remove any deactivated users from the channel before moving it.") DeleteChannelsCmd.Flags().Bool("confirm", false, "Confirm you really want to delete the channels.") @@ -366,6 +367,8 @@ func moveChannelsCmdF(command *cobra.Command, args []string) error { } user := getUserFromUserArg(a, username) + removeDeactivatedMembers, _ := command.Flags().GetBool("remove-deactivated-users") + channels := getChannelsFromChannelArgs(a, args[1:]) for i, channel := range channels { if channel == nil { @@ -373,7 +376,7 @@ func moveChannelsCmdF(command *cobra.Command, args []string) error { continue } originTeamID := channel.TeamId - if err := moveChannel(a, team, channel, user); err != nil { + if err := moveChannel(a, team, channel, user, removeDeactivatedMembers); err != nil { CommandPrintErrorln("Unable to move channel '" + channel.Name + "' error: " + err.Error()) } else { CommandPrettyPrintln("Moved channel '" + channel.Name + "' to " + team.Name + "(" + team.Id + ") from " + originTeamID + ".") @@ -383,10 +386,10 @@ func moveChannelsCmdF(command *cobra.Command, args []string) error { return nil } -func moveChannel(a *app.App, team *model.Team, channel *model.Channel, user *model.User) *model.AppError { +func moveChannel(a *app.App, team *model.Team, channel *model.Channel, user *model.User, removeDeactivatedMembers bool) *model.AppError { oldTeamId := channel.TeamId - if err := a.MoveChannel(team, channel, user); err != nil { + if err := a.MoveChannel(team, channel, user, removeDeactivatedMembers); err != nil { return err } diff --git a/i18n/en.json b/i18n/en.json index 7e74e4d89..7d8bd4122 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -4834,6 +4834,10 @@ "id": "plugin.api.update_user_status.bad_status", "translation": "Unable to set the user status. Unknown user status." }, + { + "id": "store.sql_channel.remove_all_deactivated_members.app_error", + "translation": "We could not remove the deactivated users from the channel" + }, { "id": "store.sql.convert_string_array", "translation": "FromDb: Unable to convert StringArray to *string" diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index 7a9e4e687..b1886c428 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -1478,6 +1478,32 @@ func (s SqlChannelStore) RemoveMember(channelId string, userId string) store.Sto }) } +func (s SqlChannelStore) RemoveAllDeactivatedMembers(channelId string) store.StoreChannel { + return store.Do(func(result *store.StoreResult) { + query := ` + DELETE + FROM + ChannelMembers + WHERE + UserId IN ( + SELECT + Id + FROM + Users + WHERE + Users.DeleteAt != 0 + ) + AND + ChannelMembers.ChannelId = :ChannelId + ` + + _, err := s.GetMaster().Exec(query, map[string]interface{}{"ChannelId": channelId}) + if err != nil { + result.Err = model.NewAppError("SqlChannelStore.RemoveAllDeactivatedMembers", "store.sql_channel.remove_all_deactivated_members.app_error", nil, "channel_id="+channelId+", "+err.Error(), http.StatusInternalServerError) + } + }) +} + func (s SqlChannelStore) PermanentDeleteMembersByUser(userId string) store.StoreChannel { return store.Do(func(result *store.StoreResult) { if _, err := s.GetMaster().Exec("DELETE FROM ChannelMembers WHERE UserId = :UserId", map[string]interface{}{"UserId": userId}); err != nil { diff --git a/store/store.go b/store/store.go index dd49e0c97..8a2051527 100644 --- a/store/store.go +++ b/store/store.go @@ -184,6 +184,7 @@ type ChannelStore interface { IsExperimentalPublicChannelsMaterializationEnabled() bool GetAllChannelsForExportAfter(limit int, afterId string) StoreChannel GetChannelMembersForExport(userId string, teamId string) StoreChannel + RemoveAllDeactivatedMembers(channelId string) StoreChannel } type ChannelMemberHistoryStore interface { diff --git a/store/storetest/channel_store.go b/store/storetest/channel_store.go index 7cb86242f..34efd810d 100644 --- a/store/storetest/channel_store.go +++ b/store/storetest/channel_store.go @@ -80,6 +80,7 @@ func TestChannelStore(t *testing.T, ss store.Store, s SqlSupplier) { t.Run("MaterializedPublicChannels", func(t *testing.T) { testMaterializedPublicChannels(t, ss, s) }) t.Run("GetAllChannelsForExportAfter", func(t *testing.T) { testChannelStoreGetAllChannelsForExportAfter(t, ss) }) t.Run("GetChannelMembersForExport", func(t *testing.T) { testChannelStoreGetChannelMembersForExport(t, ss) }) + t.Run("RemoveAllDeactivatedMembers", func(t *testing.T) { testChannelStoreRemoveAllDeactivatedMembers(t, ss) }) }) } } @@ -2806,3 +2807,75 @@ func testChannelStoreGetChannelMembersForExport(t *testing.T, ss store.Store) { assert.Equal(t, c1.Id, cmfe1.ChannelId) assert.Equal(t, u1.Id, cmfe1.UserId) } + +func testChannelStoreRemoveAllDeactivatedMembers(t *testing.T, ss store.Store) { + // Set up all the objects needed in the store. + t1 := model.Team{} + t1.DisplayName = "Name" + t1.Name = model.NewId() + t1.Email = MakeEmail() + t1.Type = model.TEAM_OPEN + store.Must(ss.Team().Save(&t1)) + + c1 := model.Channel{} + c1.TeamId = t1.Id + c1.DisplayName = "Channel1" + c1.Name = "zz" + model.NewId() + "b" + c1.Type = model.CHANNEL_OPEN + store.Must(ss.Channel().Save(&c1, -1)) + + u1 := model.User{} + u1.Email = MakeEmail() + u1.Nickname = model.NewId() + store.Must(ss.User().Save(&u1)) + + u2 := model.User{} + u2.Email = MakeEmail() + u2.Nickname = model.NewId() + store.Must(ss.User().Save(&u2)) + + u3 := model.User{} + u3.Email = MakeEmail() + u3.Nickname = model.NewId() + store.Must(ss.User().Save(&u3)) + + m1 := model.ChannelMember{} + m1.ChannelId = c1.Id + m1.UserId = u1.Id + m1.NotifyProps = model.GetDefaultChannelNotifyProps() + store.Must(ss.Channel().SaveMember(&m1)) + + m2 := model.ChannelMember{} + m2.ChannelId = c1.Id + m2.UserId = u2.Id + m2.NotifyProps = model.GetDefaultChannelNotifyProps() + store.Must(ss.Channel().SaveMember(&m2)) + + m3 := model.ChannelMember{} + m3.ChannelId = c1.Id + m3.UserId = u3.Id + m3.NotifyProps = model.GetDefaultChannelNotifyProps() + store.Must(ss.Channel().SaveMember(&m3)) + + // Get all the channel members. Check there are 3. + r1 := <-ss.Channel().GetMembers(c1.Id, 0, 1000) + assert.Nil(t, r1.Err) + d1 := r1.Data.(*model.ChannelMembers) + assert.Len(t, *d1, 3) + + // Deactivate users 1 & 2. + u1.DeleteAt = model.GetMillis() + u2.DeleteAt = model.GetMillis() + require.Nil(t, (<-ss.User().Update(&u1, true)).Err) + require.Nil(t, (<-ss.User().Update(&u2, true)).Err) + + // Remove all deactivated users from the channel. + assert.Nil(t, (<-ss.Channel().RemoveAllDeactivatedMembers(c1.Id)).Err) + + // Get all the channel members. Check there is now only 1: m3. + r2 := <-ss.Channel().GetMembers(c1.Id, 0, 1000) + assert.Nil(t, r1.Err) + d2 := r2.Data.(*model.ChannelMembers) + assert.Len(t, *d2, 1) + assert.Equal(t, (*d2)[0].UserId, u3.Id) +} diff --git a/store/storetest/mocks/ChannelStore.go b/store/storetest/mocks/ChannelStore.go index 9db85eacf..1c3a2a8f2 100644 --- a/store/storetest/mocks/ChannelStore.go +++ b/store/storetest/mocks/ChannelStore.go @@ -779,6 +779,22 @@ func (_m *ChannelStore) PermanentDeleteMembersByUser(userId string) store.StoreC return r0 } +// RemoveAllDeactivatedMembers provides a mock function with given fields: channelId +func (_m *ChannelStore) RemoveAllDeactivatedMembers(channelId string) store.StoreChannel { + ret := _m.Called(channelId) + + var r0 store.StoreChannel + if rf, ok := ret.Get(0).(func(string) store.StoreChannel); ok { + r0 = rf(channelId) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(store.StoreChannel) + } + } + + return r0 +} + // RemoveMember provides a mock function with given fields: channelId, userId func (_m *ChannelStore) RemoveMember(channelId string, userId string) store.StoreChannel { ret := _m.Called(channelId, userId) -- cgit v1.2.3-1-g7c22