From 4a23d4b282d84536415912392169fa871b20c8a9 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Tue, 3 Dec 2019 14:51:50 -0500 Subject: [PATCH] MM-20681 Fix custom post types not marking channel unread when using Mark as Unread (#13247) * MM-20681 Fix custom post types not marking channel unread when using Mark as Unread * Fix inverted if statements --- app/post.go | 2 +- model/post.go | 13 +++ store/sqlstore/channel_store.go | 37 ++++--- store/sqlstore/post_store.go | 10 +- store/store.go | 2 +- store/storetest/channel_store.go | 145 ++++++++++++++++++++++++++ store/storetest/mocks/ChannelStore.go | 8 +- 7 files changed, 192 insertions(+), 25 deletions(-) diff --git a/app/post.go b/app/post.go index 610ab690a8..07ed78b461 100644 --- a/app/post.go +++ b/app/post.go @@ -1194,7 +1194,7 @@ func (a *App) countMentionsFromPost(user *model.User, post *model.Post) (int, *m return 0, countErr } - return int(count), countErr + return count, countErr } channelMember, err := a.GetChannelMember(channel.Id, user.Id) diff --git a/model/post.go b/model/post.go index a991b45102..93ab7ec708 100644 --- a/model/post.go +++ b/model/post.go @@ -350,6 +350,19 @@ func (o *Post) IsSystemMessage() bool { return len(o.Type) >= len(POST_SYSTEM_MESSAGE_PREFIX) && o.Type[:len(POST_SYSTEM_MESSAGE_PREFIX)] == POST_SYSTEM_MESSAGE_PREFIX } +func (o *Post) IsJoinLeaveMessage() bool { + return o.Type == POST_JOIN_LEAVE || + o.Type == POST_ADD_REMOVE || + o.Type == POST_JOIN_CHANNEL || + o.Type == POST_LEAVE_CHANNEL || + o.Type == POST_JOIN_TEAM || + o.Type == POST_LEAVE_TEAM || + o.Type == POST_ADD_TO_CHANNEL || + o.Type == POST_REMOVE_FROM_CHANNEL || + o.Type == POST_ADD_TO_TEAM || + o.Type == POST_REMOVE_FROM_TEAM +} + func (p *Post) Patch(patch *PostPatch) { if patch.IsPinned != nil { p.IsPinned = *patch.IsPinned diff --git a/store/sqlstore/channel_store.go b/store/sqlstore/channel_store.go index 0b8f5f50ca..989f3b5ed9 100644 --- a/store/sqlstore/channel_store.go +++ b/store/sqlstore/channel_store.go @@ -1759,31 +1759,44 @@ func (s SqlChannelStore) UpdateLastViewedAt(channelIds []string, userId string) } // CountPostsAfter returns the number of posts in the given channel created after but not including the given timestamp. If given a non-empty user ID, only counts posts made by that user. -func (s SqlChannelStore) CountPostsAfter(channelId string, timestamp int64, userId string) (int64, *model.AppError) { - countUnreadQuery := ` +func (s SqlChannelStore) CountPostsAfter(channelId string, timestamp int64, userId string) (int, *model.AppError) { + joinLeavePostTypes, params := MapStringsToQueryParams([]string{ + // These types correspond to the ones checked by Post.IsJoinLeaveMessage + model.POST_JOIN_LEAVE, + model.POST_ADD_REMOVE, + model.POST_JOIN_CHANNEL, + model.POST_LEAVE_CHANNEL, + model.POST_JOIN_TEAM, + model.POST_LEAVE_TEAM, + model.POST_ADD_TO_CHANNEL, + model.POST_REMOVE_FROM_CHANNEL, + model.POST_ADD_TO_TEAM, + model.POST_REMOVE_FROM_TEAM, + }, "PostType") + + query := ` SELECT count(*) FROM Posts WHERE ChannelId = :ChannelId AND CreateAt > :CreateAt - AND Type = '' -- This line causes MM-20681 + AND Type NOT IN ` + joinLeavePostTypes + ` AND DeleteAt = 0 ` - countParams := map[string]interface{}{ - "ChannelId": channelId, - "CreateAt": timestamp, - } + + params["ChannelId"] = channelId + params["CreateAt"] = timestamp if userId != "" { - countUnreadQuery += " AND UserId = :UserId" - countParams["UserId"] = userId + query += " AND UserId = :UserId" + params["UserId"] = userId } - unread, err := s.GetReplica().SelectInt(countUnreadQuery, countParams) + unread, err := s.GetReplica().SelectInt(query, params) if err != nil { - return 0, model.NewAppError("SqlChannelStore.CountPostsAfter", "store.sql_channel.count_posts_since.app_error", countParams, fmt.Sprintf("channel_id=%s, timestamp=%d, err=%s", channelId, timestamp, err), http.StatusInternalServerError) + return 0, model.NewAppError("SqlChannelStore.CountPostsAfter", "store.sql_channel.count_posts_since.app_error", nil, fmt.Sprintf("channel_id=%s, timestamp=%d, err=%s", channelId, timestamp, err), http.StatusInternalServerError) } - return unread, nil + return int(unread), nil } // UpdateLastViewedAtPost updates a ChannelMember as if the user last read the channel at the time of the given post. diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index 19f81c1855..fee8ffbbd1 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -102,11 +102,7 @@ func (s *SqlPostStore) Save(post *model.Post) (*model.Post, *model.AppError) { time := post.UpdateAt - if post.Type != model.POST_JOIN_LEAVE && post.Type != model.POST_ADD_REMOVE && - post.Type != model.POST_JOIN_CHANNEL && post.Type != model.POST_LEAVE_CHANNEL && - post.Type != model.POST_JOIN_TEAM && post.Type != model.POST_LEAVE_TEAM && - post.Type != model.POST_ADD_TO_CHANNEL && post.Type != model.POST_REMOVE_FROM_CHANNEL && - post.Type != model.POST_ADD_TO_TEAM && post.Type != model.POST_REMOVE_FROM_TEAM { + if !post.IsJoinLeaveMessage() { if _, err := s.GetMaster().Exec("UPDATE Channels SET LastPostAt = GREATEST(:LastPostAt, LastPostAt), TotalMsgCount = TotalMsgCount + 1 WHERE Id = :ChannelId", map[string]interface{}{"LastPostAt": time, "ChannelId": post.ChannelId}); err != nil { mlog.Error("Error updating Channel LastPostAt.", mlog.Err(err)) } @@ -203,7 +199,7 @@ func (s *SqlPostStore) GetFlaggedPostsForTeam(userId, teamId string, offset int, query := ` SELECT - A.*, (SELECT count(Posts.Id) FROM Posts WHERE Posts.RootId = A.Id AND Posts.DeleteAt = 0) as ReplyCount + A.*, (SELECT count(Posts.Id) FROM Posts WHERE Posts.RootId = A.Id AND Posts.DeleteAt = 0) as ReplyCount FROM (SELECT * @@ -245,7 +241,7 @@ func (s *SqlPostStore) GetFlaggedPostsForChannel(userId, channelId string, offse var posts []*model.Post query := ` SELECT - *, (SELECT count(Posts.Id) FROM Posts WHERE Posts.RootId = p.Id AND Posts.DeleteAt = 0) as ReplyCount + *, (SELECT count(Posts.Id) FROM Posts WHERE Posts.RootId = p.Id AND Posts.DeleteAt = 0) as ReplyCount FROM Posts p WHERE Id IN (SELECT Name FROM Preferences WHERE UserId = :UserId AND Category = :Category) diff --git a/store/store.go b/store/store.go index c8d46f9e77..6dd4f7d031 100644 --- a/store/store.go +++ b/store/store.go @@ -162,7 +162,7 @@ type ChannelStore interface { PermanentDeleteMembersByChannel(channelId string) *model.AppError UpdateLastViewedAt(channelIds []string, userId string) (map[string]int64, *model.AppError) UpdateLastViewedAtPost(unreadPost *model.Post, userID string, mentionCount int) (*model.ChannelUnreadAt, *model.AppError) - CountPostsAfter(channelId string, timestamp int64, userId string) (int64, *model.AppError) + CountPostsAfter(channelId string, timestamp int64, userId string) (int, *model.AppError) IncrementMentionCount(channelId string, userId string) *model.AppError AnalyticsTypeCount(teamId string, channelType string) (int64, *model.AppError) GetMembersForUser(teamId string, userId string) (*model.ChannelMembers, *model.AppError) diff --git a/store/storetest/channel_store.go b/store/storetest/channel_store.go index cb6cf78b9b..baaa67578d 100644 --- a/store/storetest/channel_store.go +++ b/store/storetest/channel_store.go @@ -57,6 +57,7 @@ func TestChannelStore(t *testing.T, ss store.Store, s SqlSupplier) { t.Run("GetChannelCounts", func(t *testing.T) { testChannelStoreGetChannelCounts(t, ss) }) t.Run("GetMembersForUser", func(t *testing.T) { testChannelStoreGetMembersForUser(t, ss) }) t.Run("GetMembersForUserWithPagination", func(t *testing.T) { testChannelStoreGetMembersForUserWithPagination(t, ss) }) + t.Run("CountPostsAfter", func(t *testing.T) { testCountPostsAfter(t, ss) }) t.Run("UpdateLastViewedAt", func(t *testing.T) { testChannelStoreUpdateLastViewedAt(t, ss) }) t.Run("IncrementMentionCount", func(t *testing.T) { testChannelStoreIncrementMentionCount(t, ss) }) t.Run("UpdateChannelMember", func(t *testing.T) { testUpdateChannelMember(t, ss) }) @@ -1594,6 +1595,150 @@ func testChannelStoreGetMembersForUserWithPagination(t *testing.T, ss store.Stor assert.Len(t, *members, 1) } +func testCountPostsAfter(t *testing.T, ss store.Store) { + t.Run("should count all posts with or without the given user ID", func(t *testing.T) { + userId1 := model.NewId() + userId2 := model.NewId() + + channelId := model.NewId() + + p1, err := ss.Post().Save(&model.Post{ + UserId: userId1, + ChannelId: channelId, + CreateAt: 1000, + }) + require.Nil(t, err) + + _, err = ss.Post().Save(&model.Post{ + UserId: userId1, + ChannelId: channelId, + CreateAt: 1001, + }) + require.Nil(t, err) + + _, err = ss.Post().Save(&model.Post{ + UserId: userId2, + ChannelId: channelId, + CreateAt: 1002, + }) + require.Nil(t, err) + + count, err := ss.Channel().CountPostsAfter(channelId, p1.CreateAt-1, "") + require.Nil(t, err) + assert.Equal(t, 3, count) + + count, err = ss.Channel().CountPostsAfter(channelId, p1.CreateAt, "") + require.Nil(t, err) + assert.Equal(t, 2, count) + + count, err = ss.Channel().CountPostsAfter(channelId, p1.CreateAt-1, userId1) + require.Nil(t, err) + assert.Equal(t, 2, count) + + count, err = ss.Channel().CountPostsAfter(channelId, p1.CreateAt, userId1) + require.Nil(t, err) + assert.Equal(t, 1, count) + }) + + t.Run("should not count deleted posts", func(t *testing.T) { + userId1 := model.NewId() + + channelId := model.NewId() + + p1, err := ss.Post().Save(&model.Post{ + UserId: userId1, + ChannelId: channelId, + CreateAt: 1000, + }) + require.Nil(t, err) + + _, err = ss.Post().Save(&model.Post{ + UserId: userId1, + ChannelId: channelId, + CreateAt: 1001, + DeleteAt: 1001, + }) + require.Nil(t, err) + + count, err := ss.Channel().CountPostsAfter(channelId, p1.CreateAt-1, "") + require.Nil(t, err) + assert.Equal(t, 1, count) + + count, err = ss.Channel().CountPostsAfter(channelId, p1.CreateAt, "") + require.Nil(t, err) + assert.Equal(t, 0, count) + }) + + t.Run("should count system/bot messages, but not join/leave messages", func(t *testing.T) { + userId1 := model.NewId() + + channelId := model.NewId() + + p1, err := ss.Post().Save(&model.Post{ + UserId: userId1, + ChannelId: channelId, + CreateAt: 1000, + }) + require.Nil(t, err) + + _, err = ss.Post().Save(&model.Post{ + UserId: userId1, + ChannelId: channelId, + CreateAt: 1001, + Type: model.POST_JOIN_CHANNEL, + }) + require.Nil(t, err) + + _, err = ss.Post().Save(&model.Post{ + UserId: userId1, + ChannelId: channelId, + CreateAt: 1002, + Type: model.POST_REMOVE_FROM_CHANNEL, + }) + require.Nil(t, err) + + _, err = ss.Post().Save(&model.Post{ + UserId: userId1, + ChannelId: channelId, + CreateAt: 1003, + Type: model.POST_LEAVE_TEAM, + }) + require.Nil(t, err) + + p5, err := ss.Post().Save(&model.Post{ + UserId: userId1, + ChannelId: channelId, + CreateAt: 1004, + Type: model.POST_HEADER_CHANGE, + }) + require.Nil(t, err) + + _, err = ss.Post().Save(&model.Post{ + UserId: userId1, + ChannelId: channelId, + CreateAt: 1005, + Type: "custom_nps_survey", + }) + require.Nil(t, err) + + count, err := ss.Channel().CountPostsAfter(channelId, p1.CreateAt-1, "") + require.Nil(t, err) + assert.Equal(t, 3, count) + + count, err = ss.Channel().CountPostsAfter(channelId, p1.CreateAt, "") + require.Nil(t, err) + assert.Equal(t, 2, count) + + count, err = ss.Channel().CountPostsAfter(channelId, p5.CreateAt-1, "") + require.Nil(t, err) + assert.Equal(t, 2, count) + + count, err = ss.Channel().CountPostsAfter(channelId, p5.CreateAt, "") + require.Nil(t, err) + assert.Equal(t, 1, count) + }) +} + func testChannelStoreUpdateLastViewedAt(t *testing.T, ss store.Store) { o1 := model.Channel{} o1.TeamId = model.NewId() diff --git a/store/storetest/mocks/ChannelStore.go b/store/storetest/mocks/ChannelStore.go index 308cbaaa1c..f7106cec6d 100644 --- a/store/storetest/mocks/ChannelStore.go +++ b/store/storetest/mocks/ChannelStore.go @@ -133,14 +133,14 @@ func (_m *ChannelStore) ClearCaches() { } // CountPostsAfter provides a mock function with given fields: channelId, timestamp, userId -func (_m *ChannelStore) CountPostsAfter(channelId string, timestamp int64, userId string) (int64, *model.AppError) { +func (_m *ChannelStore) CountPostsAfter(channelId string, timestamp int64, userId string) (int, *model.AppError) { ret := _m.Called(channelId, timestamp, userId) - var r0 int64 - if rf, ok := ret.Get(0).(func(string, int64, string) int64); ok { + var r0 int + if rf, ok := ret.Get(0).(func(string, int64, string) int); ok { r0 = rf(channelId, timestamp, userId) } else { - r0 = ret.Get(0).(int64) + r0 = ret.Get(0).(int) } var r1 *model.AppError