MM-20649 Fix incorrect mention count when marking a DM channel as unread (#13245)

* MM-20649 Fix incorrect mention count when marking a DM channel as unread

* Satisfy govet

* Update missed test

app/channel_test.go

@@ -1132,12 +1132,12 @@ func TestMarkChannelAsUnreadFromPost(t *testing.T) {
 		th.CreatePost(dc)
 		th.CreatePost(dc)
 
-		response, err := th.App.MarkChannelAsUnreadFromPost(dm1.Id, u1.Id)
+		response, err := th.App.MarkChannelAsUnreadFromPost(dm1.Id, u2.Id)
 		assert.Nil(t, err)
 		assert.Equal(t, int64(0), response.MsgCount)
 		assert.Equal(t, int64(3), response.MentionCount)
 
-		unread, err := th.App.GetChannelUnread(dc.Id, u1.Id)
+		unread, err := th.App.GetChannelUnread(dc.Id, u2.Id)
 		require.Nil(t, err)
 		assert.Equal(t, int64(3), unread.MsgCount)
 		assert.Equal(t, int64(3), unread.MentionCount)

app/post.go

@@ -1180,7 +1180,7 @@ func (a *App) MaxPostSize() int {
 }
 
 // countMentionsFromPost returns the number of posts in the post's channel that mention the user after and including the
-// given post. Returns the number of mentions or store.MentionAllPosts if the post is in a direct message channel.
+// given post.
 func (a *App) countMentionsFromPost(user *model.User, post *model.Post) (int, *model.AppError) {
 	channel, err := a.GetChannel(post.ChannelId)
 	if err != nil {
@@ -1188,7 +1188,13 @@ func (a *App) countMentionsFromPost(user *model.User, post *model.Post) (int, *m
 	}
 
 	if channel.Type == model.CHANNEL_DIRECT {
-		return store.MentionAllPosts, nil
+		// In a DM channel, every post made by the other user is a mention
+		count, countErr := a.Srv.Store.Channel().CountPostsAfter(post.ChannelId, post.CreateAt-1, channel.GetOtherUserIdForDM(user.Id))
+		if countErr != nil {
+			return 0, countErr
+		}
+
+		return int(count), countErr
 	}
 
 	channelMember, err := a.GetChannelMember(channel.Id, user.Id)

app/post_test.go

@@ -16,7 +16,6 @@ import (
 	"github.com/mattermost/mattermost-server/v5/einterfaces/mocks"
 	"github.com/mattermost/mattermost-server/v5/model"
 	"github.com/mattermost/mattermost-server/v5/plugin/plugintest/mock"
-	"github.com/mattermost/mattermost-server/v5/store"
 	"github.com/mattermost/mattermost-server/v5/store/storetest"
 )
 
@@ -1261,7 +1260,7 @@ func TestCountMentionsFromPost(t *testing.T) {
 		assert.Equal(t, 2, count)
 	})
 
-	t.Run("should return store.MentionAllPosts for a direct channel", func(t *testing.T) {
+	t.Run("should return the number of posts made by the other user for a direct channel", func(t *testing.T) {
 		th := Setup(t).InitBasic()
 		defer th.TearDown()
 
@@ -1278,10 +1277,22 @@ func TestCountMentionsFromPost(t *testing.T) {
 		}, channel, false)
 		require.Nil(t, err)
 
+		_, err = th.App.CreatePost(&model.Post{
+			UserId:    user1.Id,
+			ChannelId: channel.Id,
+			Message:   "test2",
+		}, channel, false)
+		require.Nil(t, err)
+
 		count, err := th.App.countMentionsFromPost(user2, post1)
 
 		assert.Nil(t, err)
-		assert.Equal(t, store.MentionAllPosts, count)
+		assert.Equal(t, 2, count)
+
+		count, err = th.App.countMentionsFromPost(user1, post1)
+
+		assert.Nil(t, err)
+		assert.Equal(t, 0, count)
 	})
 
 	t.Run("should not count mentions from the before the given post", func(t *testing.T) {

store/sqlstore/channel_store.go

@@ -1019,7 +1019,7 @@ func (s SqlChannelStore) GetPublicChannelsForTeam(teamId string, offset int, lim
 			PublicChannels pc ON (pc.Id = Channels.Id)
 		WHERE
 			pc.TeamId = :TeamId
-		AND pc.DeleteAt = 0 
+		AND pc.DeleteAt = 0
 		ORDER BY pc.DisplayName
 		LIMIT :Limit
 		OFFSET :Offset
@@ -1229,15 +1229,15 @@ func (s SqlChannelStore) GetDeleted(teamId string, offset int, limit int, userId
 	channels := &model.ChannelList{}
 
 	query := `
-		SELECT * FROM Channels 
+		SELECT * FROM Channels
-		WHERE (TeamId = :TeamId OR TeamId = '') 
+		WHERE (TeamId = :TeamId OR TeamId = '')
-		AND DeleteAt != 0 
+		AND DeleteAt != 0
 		AND Type != 'P'
 		UNION
-			SELECT * FROM Channels 
+			SELECT * FROM Channels
-			WHERE (TeamId = :TeamId OR TeamId = '') 
+			WHERE (TeamId = :TeamId OR TeamId = '')
-			AND DeleteAt != 0 
+			AND DeleteAt != 0
-			AND Type = 'P' 
+			AND Type = 'P'
 			AND Id IN (SELECT ChannelId FROM ChannelMembers WHERE UserId = :UserId)
 		ORDER BY DisplayName LIMIT :Limit OFFSET :Offset
 	`
@@ -1758,25 +1758,30 @@ func (s SqlChannelStore) UpdateLastViewedAt(channelIds []string, userId string)
 	return times, nil
 }
 
-// countPostsAfter returns the number of posts in the given channel created after but not including the given timestamp.
+// 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, since int64) (int64, *model.AppError) {
+func (s SqlChannelStore) CountPostsAfter(channelId string, timestamp int64, userId string) (int64, *model.AppError) {
 	countUnreadQuery := `
 	SELECT count(*)
 	FROM Posts
 	WHERE
-		ChannelId = :channelId
+		ChannelId = :ChannelId
-		AND CreateAt > :createAt
+		AND CreateAt > :CreateAt
-		AND Type = ''
+		AND Type = '' -- This line causes MM-20681
 		AND DeleteAt = 0
 	`
 	countParams := map[string]interface{}{
-		"channelId": channelID,
+		"ChannelId": channelId,
-		"createAt":  since,
+		"CreateAt":  timestamp,
+	}
+
+	if userId != "" {
+		countUnreadQuery += " AND UserId = :UserId"
+		countParams["UserId"] = userId
 	}
 
 	unread, err := s.GetReplica().SelectInt(countUnreadQuery, countParams)
 	if err != nil {
-		return 0, model.NewAppError("SqlChannelStore.countPostsAfter", "store.sql_channel.count_posts_since.app_error", countParams, fmt.Sprintf("channel_id=%s, since=%d, err=%s", channelID, since, err), http.StatusInternalServerError)
+		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 unread, nil
 }
@@ -1787,16 +1792,11 @@ func (s SqlChannelStore) countPostsAfter(channelID string, since int64) (int64,
 func (s SqlChannelStore) UpdateLastViewedAtPost(unreadPost *model.Post, userID string, mentionCount int) (*model.ChannelUnreadAt, *model.AppError) {
 	unreadDate := unreadPost.CreateAt - 1
 
-	unread, appErr := s.countPostsAfter(unreadPost.ChannelId, unreadDate)
+	unread, appErr := s.CountPostsAfter(unreadPost.ChannelId, unreadDate, "")
 	if appErr != nil {
 		return nil, appErr
 	}
 
-	if mentionCount == store.MentionAllPosts {
-		// Treat every unread post as a mention (like in a DM channel)
-		mentionCount = int(unread)
-	}
-
 	params := map[string]interface{}{
 		"mentions":     mentionCount,
 		"unreadCount":  unread,

store/store.go

@@ -9,10 +9,6 @@ import (
 	"github.com/mattermost/mattermost-server/v5/model"
 )
 
-const (
-	MentionAllPosts = -1
-)
-
 type StoreResult struct {
 	Data interface{}
 	Err  *model.AppError
@@ -166,6 +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)
 	IncrementMentionCount(channelId string, userId string) *model.AppError
 	AnalyticsTypeCount(teamId string, channelType string) (int64, *model.AppError)
 	GetMembersForUser(teamId string, userId string) (*model.ChannelMembers, *model.AppError)

store/storetest/mocks/ChannelStore.go

@@ -132,6 +132,29 @@ func (_m *ChannelStore) ClearCaches() {
 	_m.Called()
 }
 
+// CountPostsAfter provides a mock function with given fields: channelId, timestamp, userId
+func (_m *ChannelStore) CountPostsAfter(channelId string, timestamp int64, userId string) (int64, *model.AppError) {
+	ret := _m.Called(channelId, timestamp, userId)
+
+	var r0 int64
+	if rf, ok := ret.Get(0).(func(string, int64, string) int64); ok {
+		r0 = rf(channelId, timestamp, userId)
+	} else {
+		r0 = ret.Get(0).(int64)
+	}
+
+	var r1 *model.AppError
+	if rf, ok := ret.Get(1).(func(string, int64, string) *model.AppError); ok {
+		r1 = rf(channelId, timestamp, userId)
+	} else {
+		if ret.Get(1) != nil {
+			r1 = ret.Get(1).(*model.AppError)
+		}
+	}
+
+	return r0, r1
+}
+
 // CreateDirectChannel provides a mock function with given fields: userId, otherUserId
 func (_m *ChannelStore) CreateDirectChannel(userId *model.User, otherUserId *model.User) (*model.Channel, *model.AppError) {
 	ret := _m.Called(userId, otherUserId)
@@ -769,20 +792,6 @@ func (_m *ChannelStore) GetGuestCount(channelId string, allowFromCache bool) (in
 	return r0, r1
 }
 
-// GetGuestCountFromCache provides a mock function with given fields: channelId
-func (_m *ChannelStore) GetGuestCountFromCache(channelId string) int64 {
-	ret := _m.Called(channelId)
-
-	var r0 int64
-	if rf, ok := ret.Get(0).(func(string) int64); ok {
-		r0 = rf(channelId)
-	} else {
-		r0 = ret.Get(0).(int64)
-	}
-
-	return r0
-}
-
 // GetMember provides a mock function with given fields: channelId, userId
 func (_m *ChannelStore) GetMember(channelId string, userId string) (*model.ChannelMember, *model.AppError) {
 	ret := _m.Called(channelId, userId)