From 636d168b84b032e856e12ebbf9b24d6127fe1a08 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Wed, 22 Apr 2020 12:28:31 -0300 Subject: [PATCH] MM-23926: avoid setting last viewed for bots (#14253) The current code path for `CreatePostAsUser` tries to update the `LastViewedAt` for bots, which logs a warning message if the bot isn't actually in the channel. This pull request changes the semantics of bot posting to not update the bot's `LastViewedAt` timestamp, avoiding this log altogether. It matches the semantics of `from_webhook`, but notably makes bots slightly less like users in that they no longer "read" channels when they post. This seems reasonable, but I'm both looking for validation of this semantic change in addition to the code review. Fixes: https://mattermost.atlassian.net/browse/MM-23926 Co-authored-by: mattermod --- app/post.go | 7 ++- app/post_test.go | 124 ++++++++++++++++++++++++++++++++++++++++++ testlib/assertions.go | 21 +++++++ 3 files changed, 150 insertions(+), 2 deletions(-) diff --git a/app/post.go b/app/post.go index a8a346c9cc..2554df9b21 100644 --- a/app/post.go +++ b/app/post.go @@ -72,8 +72,11 @@ func (a *App) CreatePostAsUser(post *model.Post, currentSessionId string) (*mode return nil, err } - // Update the LastViewAt only if the post does not have from_webhook prop set (eg. Zapier app) - if _, ok := post.GetProps()["from_webhook"]; !ok { + // Update the LastViewAt only if the post does not have from_webhook prop set (e.g. Zapier app), + // or if it does not have from_bot set (e.g. from discovering the user is a bot within CreatePost). + _, fromWebhook := post.GetProps()["from_webhook"] + _, fromBot := post.GetProps()["from_bot"] + if !fromWebhook && !fromBot { if _, err := a.MarkChannelsAsViewed([]string{post.ChannelId}, post.UserId, currentSessionId); err != nil { mlog.Error( "Encountered error updating last viewed", diff --git a/app/post_test.go b/app/post_test.go index 7b8e606d55..aaf8d54285 100644 --- a/app/post_test.go +++ b/app/post_test.go @@ -13,11 +13,13 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/mattermost/mattermost-server/v5/mlog" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/plugin/plugintest/mock" "github.com/mattermost/mattermost-server/v5/services/searchengine/mocks" "github.com/mattermost/mattermost-server/v5/store/storetest" storemocks "github.com/mattermost/mattermost-server/v5/store/storetest/mocks" + "github.com/mattermost/mattermost-server/v5/testlib" ) func TestCreatePostDeduplicate(t *testing.T) { @@ -821,6 +823,128 @@ func TestPatchPost(t *testing.T) { }) } +func TestCreatePostAsUser(t *testing.T) { + t.Run("marks channel as viewed for regular user", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + post := &model.Post{ + ChannelId: th.BasicChannel.Id, + Message: "test", + UserId: th.BasicUser.Id, + } + + channelMemberBefore, appErr := th.App.Srv().Store.Channel().GetMember(th.BasicChannel.Id, th.BasicUser.Id) + require.Nil(t, appErr) + + time.Sleep(1 * time.Millisecond) + _, appErr = th.App.CreatePostAsUser(post, "") + require.Nil(t, appErr) + + channelMemberAfter, appErr := th.App.Srv().Store.Channel().GetMember(th.BasicChannel.Id, th.BasicUser.Id) + require.Nil(t, appErr) + + require.Greater(t, channelMemberAfter.LastViewedAt, channelMemberBefore.LastViewedAt) + }) + + t.Run("does not mark channel as viewed for webhook from user", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + post := &model.Post{ + ChannelId: th.BasicChannel.Id, + Message: "test", + UserId: th.BasicUser.Id, + } + post.AddProp("from_webhook", "true") + + channelMemberBefore, appErr := th.App.Srv().Store.Channel().GetMember(th.BasicChannel.Id, th.BasicUser.Id) + require.Nil(t, appErr) + + time.Sleep(1 * time.Millisecond) + _, appErr = th.App.CreatePostAsUser(post, "") + require.Nil(t, appErr) + + channelMemberAfter, appErr := th.App.Srv().Store.Channel().GetMember(th.BasicChannel.Id, th.BasicUser.Id) + require.Nil(t, appErr) + + require.Equal(t, channelMemberAfter.LastViewedAt, channelMemberBefore.LastViewedAt) + }) + + t.Run("does not mark channel as viewed for bot user in channel", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + bot := th.CreateBot() + + botUser, appErr := th.App.GetUser(bot.UserId) + require.Nil(t, appErr) + + th.LinkUserToTeam(botUser, th.BasicTeam) + th.AddUserToChannel(botUser, th.BasicChannel) + + post := &model.Post{ + ChannelId: th.BasicChannel.Id, + Message: "test", + UserId: bot.UserId, + } + + channelMemberBefore, appErr := th.App.Srv().Store.Channel().GetMember(th.BasicChannel.Id, th.BasicUser.Id) + require.Nil(t, appErr) + + time.Sleep(1 * time.Millisecond) + _, appErr = th.App.CreatePostAsUser(post, "") + require.Nil(t, appErr) + + channelMemberAfter, appErr := th.App.Srv().Store.Channel().GetMember(th.BasicChannel.Id, th.BasicUser.Id) + require.Nil(t, appErr) + + require.Equal(t, channelMemberAfter.LastViewedAt, channelMemberBefore.LastViewedAt) + }) + + t.Run("logs warning for user not in channel", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + user := th.CreateUser() + th.LinkUserToTeam(user, th.BasicTeam) + + post := &model.Post{ + ChannelId: th.BasicChannel.Id, + Message: "test", + UserId: user.Id, + } + + _, appErr := th.App.CreatePostAsUser(post, "") + require.Nil(t, appErr) + + testlib.AssertLog(t, th.LogBuffer, mlog.LevelWarn, "Failed to get membership") + }) + + t.Run("does not log warning for bot user not in channel", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + bot := th.CreateBot() + + botUser, appErr := th.App.GetUser(bot.UserId) + require.Nil(t, appErr) + + th.LinkUserToTeam(botUser, th.BasicTeam) + + post := &model.Post{ + ChannelId: th.BasicChannel.Id, + Message: "test", + UserId: bot.UserId, + } + + _, appErr = th.App.CreatePostAsUser(post, "") + require.Nil(t, appErr) + + testlib.AssertNoLog(t, th.LogBuffer, mlog.LevelWarn, "Failed to get membership") + }) +} + func TestPatchPostInArchivedChannel(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() diff --git a/testlib/assertions.go b/testlib/assertions.go index 4fb56ba45a..cb2ab5ce30 100644 --- a/testlib/assertions.go +++ b/testlib/assertions.go @@ -31,3 +31,24 @@ func AssertLog(t *testing.T, logs *bytes.Buffer, level, message string) { t.Fatalf("failed to find %s log message: %s", level, message) } + +// AssertNoLog asserts that a JSON-encoded buffer of logs does not contains one with the given level and message. +func AssertNoLog(t *testing.T, logs *bytes.Buffer, level, message string) { + dec := json.NewDecoder(logs) + for { + var log struct { + Level string + Msg string + } + if err := dec.Decode(&log); err == io.EOF { + break + } else if err != nil { + continue + } + + if log.Level == level && log.Msg == message { + t.Fatalf("found %s log message: %s", level, message) + return + } + } +}