MM-41349: CRT, fix LastUpdated semantics (#19523)

* deadcode: remove UpdateChannelLastViewedAt

* deadcode: remove ThreadStore.(Save(Multiple)|Update|Delete)

* deadcode: followThead in App.MarkChannelAsUnreadFromPost

* document ThreadMembership, Thread structs

* maintain LastUpdated consistently

Whenever we touch a `ThreadMembership` record, we should be setting `LastUpdated` to the current timestamp. The mobile client relies on this to detect changes to these records.

* simplify: never updateThreads from `App.MarkChannelAsUnreadFromPost`

Change all invocations of `ChannelStore.UpdateLastViewedAtPost` from `App.MarkChannelAsUnreadFromPost` to pass `updateThreads` as `false`. When `ChannelStore.UpdateLastViewedAtPost` was invoked with `updateThreads` as `true`, it would in turn call `ThreadStore.UpdateUnreadsByChannel` but pass `updateViewedTimestamp` as `false`. This effectively updated the `LastUpdated` field of the corresponding thread memberships but never touched any of the actual data (such as `LastViewed`).

The overall CRT feature continued to work, because `App.MarkChannelAsUnreadFromPost` directly updates the relevant thread memberships via `ThreadStore.MaintainMembership`.

* deadcode: updateThreads in ChannelStore.UpdateLastViewedAtPost

* simplify: never updateThreads from App.SendNotifications

Change all invocations of `ChannelStore.IncrementMentionCount` from
`App.SendNotifications` to pass `updateThreads` as `false`. When `ChannelStore.IncrementMentionCount` was invoked with `updateThreads` as `true`, it would in turn call `ThreadStore.UpdateUnreadsByChannel` but pass `updateViewedTimestamp` as `false`. This effectively updated the `LastUpdated` field of the corresponding thread memberships but never touched any of the actual data (such as `UnreadMentions`).

The overall CRT feature continued to work, because `App.SendNotifications` directly updates the relevant thread memberships mention counts via `ThreadStore.MaintainMembership`.

* deadcode: updateThreads in ChannelStore.IncrementMentionCount

* fix & rename ThreadStore.UpdateUnreadsByChannel

Rename `ThreadStore.UpdateUnreadsByChannel` to `ThreadStore.UpdateLastViewedByThreadIds`, making it unconditionally set the `LastViewed` for the given threads (as well as `LastUpdated`).

All previous invocations of this method that passed `updateViewedTimestamp` have been previously removed.

* unrelated gofmt -w -s changes to satisfy linter

* always set LastUpdated to model.GetMillis()

* deadcode: ThreadStore.SaveMembership

* fix TestMarkUnreadWithThreads

* GetMasterX

store/storetest/channel_store.go

@@ -4790,16 +4790,16 @@ func testChannelStoreIncrementMentionCount(t *testing.T, ss store.Store) {
 	_, err := ss.Channel().SaveMember(&m1)
 	require.NoError(t, err)
 
-	err = ss.Channel().IncrementMentionCount(m1.ChannelId, m1.UserId, false, false)
+	err = ss.Channel().IncrementMentionCount(m1.ChannelId, m1.UserId, false)
 	require.NoError(t, err, "failed to update")
 
-	err = ss.Channel().IncrementMentionCount(m1.ChannelId, "missing id", false, false)
+	err = ss.Channel().IncrementMentionCount(m1.ChannelId, "missing id", false)
 	require.NoError(t, err, "failed to update")
 
-	err = ss.Channel().IncrementMentionCount("missing id", m1.UserId, false, false)
+	err = ss.Channel().IncrementMentionCount("missing id", m1.UserId, false)
 	require.NoError(t, err, "failed to update")
 
-	err = ss.Channel().IncrementMentionCount("missing id", "missing id", false, false)
+	err = ss.Channel().IncrementMentionCount("missing id", "missing id", false)
 	require.NoError(t, err, "failed to update")
 }
 

store/storetest/mocks/ChannelStore.go

@@ -1508,13 +1508,13 @@ func (_m *ChannelStore) GroupSyncedChannelCount() (int64, error) {
 	return r0, r1
 }
 
-// IncrementMentionCount provides a mock function with given fields: channelID, userID, updateThreads, isRoot
-func (_m *ChannelStore) IncrementMentionCount(channelID string, userID string, updateThreads bool, isRoot bool) error {
-	ret := _m.Called(channelID, userID, updateThreads, isRoot)
+// IncrementMentionCount provides a mock function with given fields: channelID, userID, isRoot
+func (_m *ChannelStore) IncrementMentionCount(channelID string, userID string, isRoot bool) error {
+	ret := _m.Called(channelID, userID, isRoot)
 
 	var r0 error
-	if rf, ok := ret.Get(0).(func(string, string, bool, bool) error); ok {
-		r0 = rf(channelID, userID, updateThreads, isRoot)
+	if rf, ok := ret.Get(0).(func(string, string, bool) error); ok {
+		r0 = rf(channelID, userID, isRoot)
 	} else {
 		r0 = ret.Error(0)
 	}
@@ -2031,13 +2031,13 @@ func (_m *ChannelStore) UpdateLastViewedAt(channelIds []string, userID string, u
 	return r0, r1
 }
 
-// UpdateLastViewedAtPost provides a mock function with given fields: unreadPost, userID, mentionCount, mentionCountRoot, updateThreads, setUnreadCountRoot
-func (_m *ChannelStore) UpdateLastViewedAtPost(unreadPost *model.Post, userID string, mentionCount int, mentionCountRoot int, updateThreads bool, setUnreadCountRoot bool) (*model.ChannelUnreadAt, error) {
-	ret := _m.Called(unreadPost, userID, mentionCount, mentionCountRoot, updateThreads, setUnreadCountRoot)
+// UpdateLastViewedAtPost provides a mock function with given fields: unreadPost, userID, mentionCount, mentionCountRoot, setUnreadCountRoot
+func (_m *ChannelStore) UpdateLastViewedAtPost(unreadPost *model.Post, userID string, mentionCount int, mentionCountRoot int, setUnreadCountRoot bool) (*model.ChannelUnreadAt, error) {
+	ret := _m.Called(unreadPost, userID, mentionCount, mentionCountRoot, setUnreadCountRoot)
 
 	var r0 *model.ChannelUnreadAt
-	if rf, ok := ret.Get(0).(func(*model.Post, string, int, int, bool, bool) *model.ChannelUnreadAt); ok {
-		r0 = rf(unreadPost, userID, mentionCount, mentionCountRoot, updateThreads, setUnreadCountRoot)
+	if rf, ok := ret.Get(0).(func(*model.Post, string, int, int, bool) *model.ChannelUnreadAt); ok {
+		r0 = rf(unreadPost, userID, mentionCount, mentionCountRoot, setUnreadCountRoot)
 	} else {
 		if ret.Get(0) != nil {
 			r0 = ret.Get(0).(*model.ChannelUnreadAt)
@@ -2045,8 +2045,8 @@ func (_m *ChannelStore) UpdateLastViewedAtPost(unreadPost *model.Post, userID st
 	}
 
 	var r1 error
-	if rf, ok := ret.Get(1).(func(*model.Post, string, int, int, bool, bool) error); ok {
-		r1 = rf(unreadPost, userID, mentionCount, mentionCountRoot, updateThreads, setUnreadCountRoot)
+	if rf, ok := ret.Get(1).(func(*model.Post, string, int, int, bool) error); ok {
+		r1 = rf(unreadPost, userID, mentionCount, mentionCountRoot, setUnreadCountRoot)
 	} else {
 		r1 = ret.Error(1)
 	}

store/storetest/mocks/ThreadStore.go

@@ -38,20 +38,6 @@ func (_m *ThreadStore) CollectThreadsWithNewerReplies(userId string, channelIds
 	return r0, r1
 }
 
-// Delete provides a mock function with given fields: postID
-func (_m *ThreadStore) Delete(postID string) error {
-	ret := _m.Called(postID)
-
-	var r0 error
-	if rf, ok := ret.Get(0).(func(string) error); ok {
-		r0 = rf(postID)
-	} else {
-		r0 = ret.Error(0)
-	}
-
-	return r0
-}
-
 // DeleteMembershipForUser provides a mock function with given fields: userId, postID
 func (_m *ThreadStore) DeleteMembershipForUser(userId string, postID string) error {
 	ret := _m.Called(userId, postID)
@@ -413,103 +399,18 @@ func (_m *ThreadStore) PermanentDeleteBatchThreadMembershipsForRetentionPolicies
 	return r0, r1, r2
 }
 
-// Save provides a mock function with given fields: thread
-func (_m *ThreadStore) Save(thread *model.Thread) (*model.Thread, error) {
-	ret := _m.Called(thread)
+// UpdateLastViewedByThreadIds provides a mock function with given fields: userId, threadIds, timestamp
+func (_m *ThreadStore) UpdateLastViewedByThreadIds(userId string, threadIds []string, timestamp int64) error {
+	ret := _m.Called(userId, threadIds, timestamp)
 
-	var r0 *model.Thread
-	if rf, ok := ret.Get(0).(func(*model.Thread) *model.Thread); ok {
-		r0 = rf(thread)
+	var r0 error
+	if rf, ok := ret.Get(0).(func(string, []string, int64) error); ok {
+		r0 = rf(userId, threadIds, timestamp)
 	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(*model.Thread)
-		}
+		r0 = ret.Error(0)
 	}
 
-	var r1 error
-	if rf, ok := ret.Get(1).(func(*model.Thread) error); ok {
-		r1 = rf(thread)
-	} else {
-		r1 = ret.Error(1)
-	}
-
-	return r0, r1
-}
-
-// SaveMembership provides a mock function with given fields: membership
-func (_m *ThreadStore) SaveMembership(membership *model.ThreadMembership) (*model.ThreadMembership, error) {
-	ret := _m.Called(membership)
-
-	var r0 *model.ThreadMembership
-	if rf, ok := ret.Get(0).(func(*model.ThreadMembership) *model.ThreadMembership); ok {
-		r0 = rf(membership)
-	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(*model.ThreadMembership)
-		}
-	}
-
-	var r1 error
-	if rf, ok := ret.Get(1).(func(*model.ThreadMembership) error); ok {
-		r1 = rf(membership)
-	} else {
-		r1 = ret.Error(1)
-	}
-
-	return r0, r1
-}
-
-// SaveMultiple provides a mock function with given fields: thread
-func (_m *ThreadStore) SaveMultiple(thread []*model.Thread) ([]*model.Thread, int, error) {
-	ret := _m.Called(thread)
-
-	var r0 []*model.Thread
-	if rf, ok := ret.Get(0).(func([]*model.Thread) []*model.Thread); ok {
-		r0 = rf(thread)
-	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).([]*model.Thread)
-		}
-	}
-
-	var r1 int
-	if rf, ok := ret.Get(1).(func([]*model.Thread) int); ok {
-		r1 = rf(thread)
-	} else {
-		r1 = ret.Get(1).(int)
-	}
-
-	var r2 error
-	if rf, ok := ret.Get(2).(func([]*model.Thread) error); ok {
-		r2 = rf(thread)
-	} else {
-		r2 = ret.Error(2)
-	}
-
-	return r0, r1, r2
-}
-
-// Update provides a mock function with given fields: thread
-func (_m *ThreadStore) Update(thread *model.Thread) (*model.Thread, error) {
-	ret := _m.Called(thread)
-
-	var r0 *model.Thread
-	if rf, ok := ret.Get(0).(func(*model.Thread) *model.Thread); ok {
-		r0 = rf(thread)
-	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(*model.Thread)
-		}
-	}
-
-	var r1 error
-	if rf, ok := ret.Get(1).(func(*model.Thread) error); ok {
-		r1 = rf(thread)
-	} else {
-		r1 = ret.Error(1)
-	}
-
-	return r0, r1
+	return r0
 }
 
 // UpdateMembership provides a mock function with given fields: membership
@@ -534,17 +435,3 @@ func (_m *ThreadStore) UpdateMembership(membership *model.ThreadMembership) (*mo
 
 	return r0, r1
 }
-
-// UpdateUnreadsByChannel provides a mock function with given fields: userId, changedThreads, timestamp, updateViewedTimestamp
-func (_m *ThreadStore) UpdateUnreadsByChannel(userId string, changedThreads []string, timestamp int64, updateViewedTimestamp bool) error {
-	ret := _m.Called(userId, changedThreads, timestamp, updateViewedTimestamp)
-
-	var r0 error
-	if rf, ok := ret.Get(0).(func(string, []string, int64, bool) error); ok {
-		r0 = rf(userId, changedThreads, timestamp, updateViewedTimestamp)
-	} else {
-		r0 = ret.Error(0)
-	}
-
-	return r0
-}

store/storetest/post_store.go

@@ -510,15 +510,10 @@ func testPostStoreGetForThread(t *testing.T, ss store.Store) {
 		_, err = ss.Post().Save(&model.Post{ChannelId: o1.ChannelId, UserId: model.NewId(), Message: NewTestId(), RootId: o1.Id})
 		require.NoError(t, err)
 
-		threadMembership := &model.ThreadMembership{
-			PostId:         o1.Id,
-			UserId:         o1.UserId,
-			Following:      true,
-			LastViewed:     0,
-			LastUpdated:    0,
-			UnreadMentions: 0,
-		}
-		_, err = ss.Thread().SaveMembership(threadMembership)
+		_, err = ss.Thread().MaintainMembership(o1.UserId, o1.Id, store.ThreadMembershipOpts{
+			Following:       true,
+			UpdateFollowing: true,
+		})
 		require.NoError(t, err)
 		r1, err := ss.Post().Get(context.Background(), o1.Id, false, true, false, o1.UserId)
 		require.NoError(t, err)
@@ -533,15 +528,10 @@ func testPostStoreGetForThread(t *testing.T, ss store.Store) {
 		_, err = ss.Post().Save(&model.Post{ChannelId: o1.ChannelId, UserId: model.NewId(), Message: NewTestId(), RootId: o1.Id})
 		require.NoError(t, err)
 
-		threadMembership := &model.ThreadMembership{
-			PostId:         o1.Id,
-			UserId:         o1.UserId,
-			Following:      false,
-			LastViewed:     0,
-			LastUpdated:    0,
-			UnreadMentions: 0,
-		}
-		_, err = ss.Thread().SaveMembership(threadMembership)
+		_, err = ss.Thread().MaintainMembership(o1.UserId, o1.Id, store.ThreadMembershipOpts{
+			Following:       false,
+			UpdateFollowing: true,
+		})
 		require.NoError(t, err)
 		r1, err := ss.Post().Get(context.Background(), o1.Id, false, true, false, o1.UserId)
 		require.NoError(t, err)

store/storetest/thread_store.go

@@ -16,13 +16,12 @@ import (
 )
 
 func TestThreadStore(t *testing.T, ss store.Store, s SqlStore) {
-	t.Run("ThreadSQLOperations", func(t *testing.T) { testThreadSQLOperations(t, ss, s) })
 	t.Run("ThreadStorePopulation", func(t *testing.T) { testThreadStorePopulation(t, ss) })
 	t.Run("ThreadStorePermanentDeleteBatchForRetentionPolicies", func(t *testing.T) {
 		testThreadStorePermanentDeleteBatchForRetentionPolicies(t, ss)
 	})
 	t.Run("ThreadStorePermanentDeleteBatchThreadMembershipsForRetentionPolicies", func(t *testing.T) {
-		testThreadStorePermanentDeleteBatchThreadMembershipsForRetentionPolicies(t, ss)
+		testThreadStorePermanentDeleteBatchThreadMembershipsForRetentionPolicies(t, ss, s)
 	})
 	t.Run("GetTeamsUnreadForUser", func(t *testing.T) { testGetTeamsUnreadForUser(t, ss) })
 }
@@ -263,61 +262,6 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) {
 		require.Nil(t, thread2)
 	})
 
-	t.Run("Thread last updated is changed when channel is updated after UpdateLastViewedAtPost", func(t *testing.T) {
-		newPosts := makeSomePosts()
-		opts := store.ThreadMembershipOpts{
-			Following:             true,
-			IncrementMentions:     false,
-			UpdateFollowing:       true,
-			UpdateViewedTimestamp: false,
-			UpdateParticipants:    false,
-		}
-		_, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, opts)
-		require.NoError(t, e)
-		m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id)
-		require.NoError(t, err1)
-		m.LastUpdated -= 1000
-		_, err := ss.Thread().UpdateMembership(m)
-		require.NoError(t, err)
-
-		_, err = ss.Channel().UpdateLastViewedAtPost(newPosts[0], newPosts[0].UserId, 0, 0, true, true)
-		require.NoError(t, err)
-
-		assert.Eventually(t, func() bool {
-			m2, err2 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id)
-			require.NoError(t, err2)
-			return m2.LastUpdated > m.LastUpdated
-		}, time.Second, 10*time.Millisecond)
-	})
-
-	t.Run("Thread last updated is changed when channel is updated after IncrementMentionCount", func(t *testing.T) {
-		newPosts := makeSomePosts()
-
-		opts := store.ThreadMembershipOpts{
-			Following:             true,
-			IncrementMentions:     false,
-			UpdateFollowing:       true,
-			UpdateViewedTimestamp: false,
-			UpdateParticipants:    false,
-		}
-		_, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, opts)
-		require.NoError(t, e)
-		m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id)
-		require.NoError(t, err1)
-		m.LastUpdated -= 1000
-		_, err := ss.Thread().UpdateMembership(m)
-		require.NoError(t, err)
-
-		err = ss.Channel().IncrementMentionCount(newPosts[0].ChannelId, newPosts[0].UserId, true, false)
-		require.NoError(t, err)
-
-		assert.Eventually(t, func() bool {
-			m2, err2 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id)
-			require.NoError(t, err2)
-			return m2.LastUpdated > m.LastUpdated
-		}, time.Second, 10*time.Millisecond)
-	})
-
 	t.Run("Thread last updated is changed when channel is updated after UpdateLastViewedAt", func(t *testing.T) {
 		newPosts := makeSomePosts()
 		opts := store.ThreadMembershipOpts{
@@ -394,33 +338,6 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) {
 		require.NotEqual(t, int64(0), tm.LastViewed)
 	})
 
-	t.Run("Thread last updated is changed when channel is updated after UpdateLastViewedAtPost for mark unread", func(t *testing.T) {
-		newPosts := makeSomePosts()
-		opts := store.ThreadMembershipOpts{
-			Following:             true,
-			IncrementMentions:     false,
-			UpdateFollowing:       true,
-			UpdateViewedTimestamp: false,
-			UpdateParticipants:    false,
-		}
-		_, e := ss.Thread().MaintainMembership(newPosts[0].UserId, newPosts[0].Id, opts)
-		require.NoError(t, e)
-		m, err1 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id)
-		require.NoError(t, err1)
-		m.LastUpdated += 1000
-		_, err := ss.Thread().UpdateMembership(m)
-		require.NoError(t, err)
-
-		_, err = ss.Channel().UpdateLastViewedAtPost(newPosts[0], newPosts[0].UserId, 0, 0, true, true)
-		require.NoError(t, err)
-
-		assert.Eventually(t, func() bool {
-			m2, err2 := ss.Thread().GetMembershipForUser(newPosts[0].UserId, newPosts[0].Id)
-			require.NoError(t, err2)
-			return m2.LastUpdated < m.LastUpdated
-		}, time.Second, 10*time.Millisecond)
-	})
-
 	t.Run("Updating post does not make thread unread", func(t *testing.T) {
 		newPosts := makeSomePosts()
 		opts := store.ThreadMembershipOpts{
@@ -503,24 +420,6 @@ func testThreadStorePopulation(t *testing.T, ss store.Store) {
 	})
 }
 
-func testThreadSQLOperations(t *testing.T, ss store.Store, s SqlStore) {
-	t.Run("Save", func(t *testing.T) {
-		threadToSave := &model.Thread{
-			PostId:       model.NewId(),
-			ChannelId:    model.NewId(),
-			LastReplyAt:  10,
-			ReplyCount:   5,
-			Participants: model.StringArray{model.NewId(), model.NewId()},
-		}
-		_, err := ss.Thread().Save(threadToSave)
-		require.NoError(t, err)
-
-		th, err := ss.Thread().Get(threadToSave.PostId)
-		require.NoError(t, err)
-		require.Equal(t, threadToSave, th)
-	})
-}
-
 func threadStoreCreateReply(t *testing.T, ss store.Store, channelID, postID, userID string, createAt int64) *model.Post {
 	reply, err := ss.Post().Save(&model.Post{
 		ChannelId: channelID,
@@ -607,7 +506,7 @@ func testThreadStorePermanentDeleteBatchForRetentionPolicies(t *testing.T, ss st
 	assert.Nil(t, thread, "thread should have been deleted by team policy")
 }
 
-func testThreadStorePermanentDeleteBatchThreadMembershipsForRetentionPolicies(t *testing.T, ss store.Store) {
+func testThreadStorePermanentDeleteBatchThreadMembershipsForRetentionPolicies(t *testing.T, ss store.Store, s SqlStore) {
 	const limit = 1000
 	userID := model.NewId()
 	createThreadMembership := func(userID, postID string) *model.ThreadMembership {
@@ -695,7 +594,7 @@ func testThreadStorePermanentDeleteBatchThreadMembershipsForRetentionPolicies(t
 	// Delete team policy and thread
 	err = ss.RetentionPolicy().Delete(teamPolicy.ID)
 	require.NoError(t, err)
-	err = ss.Thread().Delete(post.Id)
+	_, err = s.GetMasterX().Exec("DELETE FROM Threads WHERE PostId='" + post.Id + "'")
 	require.NoError(t, err)
 
 	deleted, err := ss.Thread().DeleteOrphanedRows(1000)

store/storetest/user_store.go

@@ -2315,7 +2315,7 @@ func testUserUnreadCount(t *testing.T, ss store.Store) {
 	// Post one message with mention to open channel
 	_, nErr = ss.Post().Save(&p1)
 	require.NoError(t, nErr)
-	nErr = ss.Channel().IncrementMentionCount(c1.Id, u2.Id, false, false)
+	nErr = ss.Channel().IncrementMentionCount(c1.Id, u2.Id, false)
 	require.NoError(t, nErr)
 
 	// Post 2 messages without mention to direct channel
@@ -2326,7 +2326,7 @@ func testUserUnreadCount(t *testing.T, ss store.Store) {
 
 	_, nErr = ss.Post().Save(&p2)
 	require.NoError(t, nErr)
-	nErr = ss.Channel().IncrementMentionCount(c2.Id, u2.Id, false, false)
+	nErr = ss.Channel().IncrementMentionCount(c2.Id, u2.Id, false)
 	require.NoError(t, nErr)
 
 	p3 := model.Post{}
@@ -2336,7 +2336,7 @@ func testUserUnreadCount(t *testing.T, ss store.Store) {
 	_, nErr = ss.Post().Save(&p3)
 	require.NoError(t, nErr)
 
-	nErr = ss.Channel().IncrementMentionCount(c2.Id, u2.Id, false, false)
+	nErr = ss.Channel().IncrementMentionCount(c2.Id, u2.Id, false)
 	require.NoError(t, nErr)
 
 	badge, unreadCountErr := ss.User().GetUnreadCount(u2.Id)