Skip to content

Commit

Permalink
MM-57563 - update method to allow self posts to sync, only if connect…
Browse files Browse the repository at this point in the history 
…ed (#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
  • Loading branch information
@sbishel
sbishel authored Apr 16, 2024
1 parent 28db673 commit c9000aa
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 64 deletions.
2 changes: 1 addition & 1 deletion server/handlers/getters_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
4 changes: 2 additions & 2 deletions server/handlers/handlers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions server/handlers/mocks/PluginIface.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 12 additions & 8 deletions server/message_hooks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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{}
Expand Down
12 changes: 7 additions & 5 deletions server/selective_sync.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
106 changes: 60 additions & 46 deletions server/selective_sync_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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)
})
}

Expand All @@ -185,35 +197,37 @@ 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) {
team := th.SetupTeam(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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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)
})
}

0 comments on commit c9000aa

Please sign in to comment.