From c9000aa1973b92e6112da6398d0fbb51ec13ed46 Mon Sep 17 00:00:00 2001 From: Scott Bishel Date: Tue, 16 Apr 2024 15:36:47 -0600 Subject: [PATCH] MM-57563 - update method to allow self posts to sync, only if connected (#583) * update method to allow self posts to sync, only if connected * clean up function name * fix comment * add unit test * throw error if ChatMembersSpanPlatPlatforms only has single user * fix unit test * Update selective_sync_test.go reenable test --- server/handlers/getters_test.go | 2 +- server/handlers/handlers.go | 4 +- server/handlers/mocks/PluginIface.go | 4 +- server/message_hooks.go | 20 +++-- server/selective_sync.go | 12 +-- server/selective_sync_test.go | 106 +++++++++++++++------------ 6 files changed, 84 insertions(+), 64 deletions(-) diff --git a/server/handlers/getters_test.go b/server/handlers/getters_test.go index 7b847171b..06518fa9e 100644 --- a/server/handlers/getters_test.go +++ b/server/handlers/getters_test.go @@ -66,7 +66,7 @@ func (pm *pluginMock) GenerateRandomPassword() string { return "" } func (pm *pluginMock) GetSelectiveSync() bool { return pm.selectiveSync } -func (pm *pluginMock) ChatSpansPlatforms(channelID string) (bool, *model.AppError) { +func (pm *pluginMock) ChatShouldSync(channelID string) (bool, *model.AppError) { return true, nil } diff --git a/server/handlers/handlers.go b/server/handlers/handlers.go index 47c73b7f1..3fdb09aa3 100644 --- a/server/handlers/handlers.go +++ b/server/handlers/handlers.go @@ -50,7 +50,7 @@ type PluginIface interface { GetClientForUser(string) (msteams.Client, error) GetClientForTeamsUser(string) (msteams.Client, error) GenerateRandomPassword() string - ChatSpansPlatforms(channelID string) (bool, *model.AppError) + ChatShouldSync(channelID string) (bool, *model.AppError) GetSelectiveSync() bool IsRemoteUser(user *model.User) bool GetRemoteID() string @@ -316,7 +316,7 @@ func (ah *ActivityHandler) handleCreatedActivity(msg *clientmodels.Message, subs } if ah.plugin.GetSelectiveSync() { - if shouldSync, appErr := ah.plugin.ChatSpansPlatforms(channelID); appErr != nil { + if shouldSync, appErr := ah.plugin.ChatShouldSync(channelID); appErr != nil { ah.plugin.GetAPI().LogWarn("Failed to determine if shouldSyncChat", "channel_id", channelID, "error", appErr.Error()) return metrics.DiscardedReasonOther } else if !shouldSync { diff --git a/server/handlers/mocks/PluginIface.go b/server/handlers/mocks/PluginIface.go index 5ee03e4a4..22a8d2d4f 100644 --- a/server/handlers/mocks/PluginIface.go +++ b/server/handlers/mocks/PluginIface.go @@ -20,8 +20,8 @@ type PluginIface struct { mock.Mock } -// ChatSpansPlatforms provides a mock function with given fields: channelID -func (_m *PluginIface) ChatSpansPlatforms(channelID string) (bool, *model.AppError) { +// ChatShouldSync provides a mock function with given fields: channelID +func (_m *PluginIface) ChatShouldSync(channelID string) (bool, *model.AppError) { ret := _m.Called(channelID) var r0 bool diff --git a/server/message_hooks.go b/server/message_hooks.go index 845dd57b2..704a3f453 100644 --- a/server/message_hooks.go +++ b/server/message_hooks.go @@ -60,7 +60,7 @@ func (p *Plugin) MessageHasBeenDeleted(_ *plugin.Context, post *model.Post) { } if p.getConfiguration().SelectiveSync { - shouldSync, appErr := p.ChatSpansPlatforms(post.ChannelId) + shouldSync, appErr := p.ChatShouldSync(post.ChannelId) if appErr != nil { p.API.LogWarn("Failed to check if chat should be synced", "error", appErr.Error(), "post_id", post.Id, "channel_id", post.ChannelId) return @@ -119,14 +119,18 @@ func (p *Plugin) MessageHasBeenPosted(_ *plugin.Context, post *model.Post) { return } - chatMembersSpanPlatforms, appErr := p.ChatMembersSpanPlatforms(members) - if appErr != nil { - p.API.LogWarn("Failed to check if chat members span platforms", "error", appErr.Error(), "post_id", post.Id, "channel_id", post.ChannelId) - return - } + isSelfPost := len(members) == 1 + chatMembersSpanPlatforms := false + if !isSelfPost { + chatMembersSpanPlatforms, appErr = p.ChatMembersSpanPlatforms(members) + if appErr != nil { + p.API.LogWarn("Failed to check if chat members span platforms", "error", appErr.Error(), "post_id", post.Id, "channel_id", post.ChannelId) + return + } - if p.getConfiguration().SelectiveSync && !chatMembersSpanPlatforms { - return + if p.getConfiguration().SelectiveSync && !chatMembersSpanPlatforms { + return + } } dstUsers := []string{} diff --git a/server/selective_sync.go b/server/selective_sync.go index bc2311352..9742d08c1 100644 --- a/server/selective_sync.go +++ b/server/selective_sync.go @@ -6,25 +6,27 @@ import ( "github.com/mattermost/mattermost/server/public/model" ) -// ChatMembersSpanPlatforms determines if the members of the given channel span both Mattermost and +// ChatShouldSync determines if the members of the given channel span both Mattermost and // MS Teams. Chats between users on the same platform are skipped if selective sync is enabled. -func (p *Plugin) ChatSpansPlatforms(channelID string) (bool, *model.AppError) { +// Chats with only a single member are self chats and always sync. +func (p *Plugin) ChatShouldSync(channelID string) (bool, *model.AppError) { members, appErr := p.API.GetChannelMembers(channelID, 0, math.MaxInt32) if appErr != nil { return false, appErr } + if len(members) == 1 { + return true, nil + } return p.ChatMembersSpanPlatforms(members) } // ChatMembersSpanPlatforms determines if the given channel members span both Mattermost and // MS Teams. Chats between users on the same platform are skipped if selective sync is enabled. -// Chats with only a single member are self chats and always sync. func (p *Plugin) ChatMembersSpanPlatforms(members model.ChannelMembers) (bool, *model.AppError) { if len(members) == 1 { - return true, nil + return false, &model.AppError{Message: "Invalid function call, requires multiple members"} } - atLeastOneLocalUser := false atLeastOneRemoteUser := false for _, m := range members { diff --git a/server/selective_sync_test.go b/server/selective_sync_test.go index 00751cbb4..ecb886298 100644 --- a/server/selective_sync_test.go +++ b/server/selective_sync_test.go @@ -8,19 +8,31 @@ import ( "github.com/stretchr/testify/require" ) -func TestChatSpansPlatforms(t *testing.T) { +func TestChatShouldSync(t *testing.T) { th := setupTestHelper(t) t.Run("invalid channel id", func(t *testing.T) { - _, appErr := th.p.ChatSpansPlatforms("") + _, appErr := th.p.ChatShouldSync("") require.Error(t, appErr) }) t.Run("unknown channel id", func(t *testing.T) { - _, appErr := th.p.ChatSpansPlatforms(model.NewId()) + _, appErr := th.p.ChatShouldSync(model.NewId()) require.Error(t, appErr) }) + t.Run("dm with a single users", func(t *testing.T) { + team := th.SetupTeam(t) + user1 := th.SetupRemoteUser(t, team) + + channel, err := th.p.API.GetDirectChannel(user1.Id, user1.Id) + require.Nil(t, err) + + chatShouldSync, appErr := th.p.ChatShouldSync(channel.Id) + require.Nil(t, appErr) + assert.True(t, chatShouldSync) + }) + t.Run("dm between two local users", func(t *testing.T) { team := th.SetupTeam(t) user1 := th.SetupUser(t, team) @@ -29,9 +41,9 @@ func TestChatSpansPlatforms(t *testing.T) { channel, err := th.p.API.GetDirectChannel(user1.Id, user2.Id) require.Nil(t, err) - chatSpansPlatforms, appErr := th.p.ChatSpansPlatforms(channel.Id) + chatShouldSync, appErr := th.p.ChatShouldSync(channel.Id) require.Nil(t, appErr) - assert.False(t, chatSpansPlatforms) + assert.False(t, chatShouldSync) }) t.Run("dm between two remote users", func(t *testing.T) { @@ -52,9 +64,9 @@ func TestChatSpansPlatforms(t *testing.T) { channel, err := th.p.API.GetDirectChannel(user1.Id, user2.Id) require.Nil(t, err) - chatSpansPlatforms, appErr := th.p.ChatSpansPlatforms(channel.Id) + chatShouldSync, appErr := th.p.ChatShouldSync(channel.Id) require.Nil(t, appErr) - assert.False(t, chatSpansPlatforms) + assert.False(t, chatShouldSync) }) t.Run("dm between a local and a remote user", func(t *testing.T) { @@ -71,9 +83,9 @@ func TestChatSpansPlatforms(t *testing.T) { channel, err := th.p.API.GetDirectChannel(user1.Id, user2.Id) require.Nil(t, err) - chatSpansPlatforms, appErr := th.p.ChatSpansPlatforms(channel.Id) + chatShouldSync, appErr := th.p.ChatShouldSync(channel.Id) require.Nil(t, appErr) - assert.True(t, chatSpansPlatforms) + assert.True(t, chatShouldSync) }) t.Run("dm between a local and a local user with teams as primary platform", func(t *testing.T) { @@ -87,9 +99,9 @@ func TestChatSpansPlatforms(t *testing.T) { channel, err := th.p.API.GetDirectChannel(user1.Id, user2.Id) require.Nil(t, err) - chatSpansPlatforms, appErr := th.p.ChatSpansPlatforms(channel.Id) + chatShouldSync, appErr := th.p.ChatShouldSync(channel.Id) require.Nil(t, appErr) - assert.True(t, chatSpansPlatforms) + assert.True(t, chatShouldSync) }) t.Run("gm between three local users", func(t *testing.T) { @@ -101,9 +113,9 @@ func TestChatSpansPlatforms(t *testing.T) { channel, err := th.p.API.GetGroupChannel([]string{user1.Id, user2.Id, user3.Id}) require.Nil(t, err) - chatSpansPlatforms, appErr := th.p.ChatSpansPlatforms(channel.Id) + chatShouldSync, appErr := th.p.ChatShouldSync(channel.Id) require.Nil(t, appErr) - assert.False(t, chatSpansPlatforms) + assert.False(t, chatShouldSync) }) t.Run("gm between three remote users", func(t *testing.T) { @@ -129,9 +141,9 @@ func TestChatSpansPlatforms(t *testing.T) { channel, err := th.p.API.GetGroupChannel([]string{user1.Id, user2.Id, user3.Id}) require.Nil(t, err) - chatSpansPlatforms, appErr := th.p.ChatSpansPlatforms(channel.Id) + chatShouldSync, appErr := th.p.ChatShouldSync(channel.Id) require.Nil(t, appErr) - assert.False(t, chatSpansPlatforms) + assert.False(t, chatShouldSync) }) t.Run("gm between a mixture of local and remote users", func(t *testing.T) { @@ -153,9 +165,9 @@ func TestChatSpansPlatforms(t *testing.T) { channel, err := th.p.API.GetGroupChannel([]string{user1.Id, user2.Id, user3.Id}) require.Nil(t, err) - chatSpansPlatforms, appErr := th.p.ChatSpansPlatforms(channel.Id) + chatShouldSync, appErr := th.p.ChatShouldSync(channel.Id) require.Nil(t, appErr) - assert.True(t, chatSpansPlatforms) + assert.True(t, chatShouldSync) }) t.Run("gm between two local users and a local user with teams as primary platform", func(t *testing.T) { @@ -170,9 +182,9 @@ func TestChatSpansPlatforms(t *testing.T) { channel, appErr := th.p.API.GetGroupChannel([]string{user1.Id, user2.Id, user3.Id}) require.Nil(t, appErr) - chatSpansPlatforms, appErr := th.p.ChatSpansPlatforms(channel.Id) + chatShouldSync, appErr := th.p.ChatShouldSync(channel.Id) require.Nil(t, appErr) - assert.True(t, chatSpansPlatforms) + assert.True(t, chatShouldSync) }) } @@ -185,22 +197,24 @@ func TestChatMembersSpanPlatforms(t *testing.T) { require.False(t, chatMembersSpanPlatforms) }) - t.Run("users with empty id", func(t *testing.T) { - chatMembersSpanPlatforms, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{model.ChannelMember{UserId: ""}, model.ChannelMember{UserId: ""}}) - require.NotNil(t, appErr) - require.False(t, chatMembersSpanPlatforms) - }) - - t.Run("single local user", func(t *testing.T) { + t.Run("single user", func(t *testing.T) { team := th.SetupTeam(t) - user1 := th.SetupUser(t, team) + user1 := th.SetupRemoteUser(t, team) - chatSpansPlatforms, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ + chatShouldSync, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ model.ChannelMember{UserId: user1.Id}, }) - require.Nil(t, appErr) - assert.True(t, chatSpansPlatforms) + require.Error(t, appErr) + assert.False(t, chatShouldSync) + }) + + t.Run("user with empty id", func(t *testing.T) { + team := th.SetupTeam(t) + user1 := th.SetupRemoteUser(t, team) + chatMembersSpanPlatforms, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{model.ChannelMember{UserId: user1.Id}, model.ChannelMember{UserId: ""}}) + require.Error(t, appErr) + require.False(t, chatMembersSpanPlatforms) }) t.Run("dm between two local users", func(t *testing.T) { @@ -208,12 +222,12 @@ func TestChatMembersSpanPlatforms(t *testing.T) { user1 := th.SetupUser(t, team) user2 := th.SetupUser(t, team) - chatSpansPlatforms, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ + chatShouldSync, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ model.ChannelMember{UserId: user1.Id}, model.ChannelMember{UserId: user2.Id}, }) require.Nil(t, appErr) - assert.False(t, chatSpansPlatforms) + assert.False(t, chatShouldSync) }) t.Run("dm between two remote users", func(t *testing.T) { @@ -231,12 +245,12 @@ func TestChatMembersSpanPlatforms(t *testing.T) { user2, appErr = th.p.API.UpdateUser(user2) require.Nil(t, appErr) - chatSpansPlatforms, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ + chatShouldSync, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ model.ChannelMember{UserId: user1.Id}, model.ChannelMember{UserId: user2.Id}, }) require.Nil(t, appErr) - assert.False(t, chatSpansPlatforms) + assert.False(t, chatShouldSync) }) t.Run("dm between a local and a remote user", func(t *testing.T) { @@ -250,12 +264,12 @@ func TestChatMembersSpanPlatforms(t *testing.T) { user1, appErr = th.p.API.UpdateUser(user1) require.Nil(t, appErr) - chatSpansPlatforms, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ + chatShouldSync, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ model.ChannelMember{UserId: user1.Id}, model.ChannelMember{UserId: user2.Id}, }) require.Nil(t, appErr) - assert.True(t, chatSpansPlatforms) + assert.True(t, chatShouldSync) }) t.Run("dm between a local and a local user with teams as primary platform", func(t *testing.T) { @@ -269,12 +283,12 @@ func TestChatMembersSpanPlatforms(t *testing.T) { _, appErr := th.p.API.GetDirectChannel(user1.Id, user2.Id) require.Nil(t, appErr) - chatSpansPlatforms, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ + chatShouldSync, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ model.ChannelMember{UserId: user1.Id}, model.ChannelMember{UserId: user2.Id}, }) require.Nil(t, appErr) - assert.True(t, chatSpansPlatforms) + assert.True(t, chatShouldSync) }) t.Run("gm between three local users", func(t *testing.T) { @@ -283,13 +297,13 @@ func TestChatMembersSpanPlatforms(t *testing.T) { user2 := th.SetupUser(t, team) user3 := th.SetupUser(t, team) - chatSpansPlatforms, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ + chatShouldSync, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ model.ChannelMember{UserId: user1.Id}, model.ChannelMember{UserId: user2.Id}, model.ChannelMember{UserId: user3.Id}, }) require.Nil(t, appErr) - assert.False(t, chatSpansPlatforms) + assert.False(t, chatShouldSync) }) t.Run("gm between three remote users", func(t *testing.T) { @@ -312,13 +326,13 @@ func TestChatMembersSpanPlatforms(t *testing.T) { user3, appErr = th.p.API.UpdateUser(user3) require.Nil(t, appErr) - chatSpansPlatforms, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ + chatShouldSync, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ model.ChannelMember{UserId: user1.Id}, model.ChannelMember{UserId: user2.Id}, model.ChannelMember{UserId: user3.Id}, }) require.Nil(t, appErr) - assert.False(t, chatSpansPlatforms) + assert.False(t, chatShouldSync) }) t.Run("gm between a mixture of local and remote users", func(t *testing.T) { @@ -337,13 +351,13 @@ func TestChatMembersSpanPlatforms(t *testing.T) { user3, appErr = th.p.API.UpdateUser(user3) require.Nil(t, appErr) - chatSpansPlatforms, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ + chatShouldSync, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ model.ChannelMember{UserId: user1.Id}, model.ChannelMember{UserId: user2.Id}, model.ChannelMember{UserId: user3.Id}, }) require.Nil(t, appErr) - assert.True(t, chatSpansPlatforms) + assert.True(t, chatShouldSync) }) t.Run("gm between two local users and a local user with teams as primary platform", func(t *testing.T) { @@ -358,12 +372,12 @@ func TestChatMembersSpanPlatforms(t *testing.T) { _, appErr := th.p.API.GetGroupChannel([]string{user1.Id, user2.Id, user3.Id}) require.Nil(t, appErr) - chatSpansPlatforms, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ + chatShouldSync, appErr := th.p.ChatMembersSpanPlatforms(model.ChannelMembers{ model.ChannelMember{UserId: user1.Id}, model.ChannelMember{UserId: user2.Id}, model.ChannelMember{UserId: user3.Id}, }) require.Nil(t, appErr) - assert.True(t, chatSpansPlatforms) + assert.True(t, chatShouldSync) }) }