[MM-36544][MM-37439] Don't re-follow on reply to unfollowed thread (#18020)

Summary
If a user has unfollowed a thread, another user's reply in the thread should not cause the first user to re-follow the thread. The first user will be re-followed if they are mentioned in the thread.
Simplify and make flexible the logic surrounding following and remove some duplicate code.

Ticket Link
https://mattermost.atlassian.net/browse/MM-36544
https://mattermost.atlassian.net/browse/MM-37439

app/notification.go

@@ -212,14 +212,16 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod
 						return
 					}
 				}
+
+				updateFollowing := *a.Config().ServiceSettings.ThreadAutoFollow
 				if mentionType == ThreadMention || mentionType == CommentMention {
 					incrementMentions = false
+					updateFollowing = false
 				}
-
 				opts := store.ThreadMembershipOpts{
 					Following:             true,
 					IncrementMentions:     incrementMentions,
-					UpdateFollowing:       *a.Config().ServiceSettings.ThreadAutoFollow,
+					UpdateFollowing:       updateFollowing,
 					UpdateViewedTimestamp: userID == post.UserId,
 					UpdateParticipants:    userID == post.UserId,
 				}

app/post.go

@@ -362,7 +362,8 @@ func (a *App) CreatePost(c *request.Context, post *model.Post, channel *model.Ch
 	// Make sure poster is following the thread
 	if *a.Config().ServiceSettings.ThreadAutoFollow && rpost.RootId != "" {
 		_, err := a.Srv().Store.Thread().MaintainMembership(user.Id, rpost.RootId, store.ThreadMembershipOpts{
-			Following: true,
+			Following:       true,
+			UpdateFollowing: true,
 		})
 		if err != nil {
 			mlog.Warn("Failed to update thread membership", mlog.Err(err))

app/post_test.go

@@ -2358,7 +2358,8 @@ func TestAutofollowOnPostingAfterUnfollow(t *testing.T) {
 
 	// unfollow thread
 	m, nErr := th.App.Srv().Store.Thread().MaintainMembership(user.Id, p1.Id, store.ThreadMembershipOpts{
-		Following: false,
+		Following:       false,
+		UpdateFollowing: true,
 	})
 	require.NoError(t, nErr)
 	require.False(t, m.Following)
@@ -2398,3 +2399,54 @@ func TestGetPostIfAuthorized(t *testing.T) {
 	_, err = th.App.GetPostIfAuthorized(post.Id, session1)
 	require.Nil(t, err)
 }
+
+func TestShouldNotRefollowOnOthersReply(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+	os.Setenv("MM_FEATUREFLAGS_COLLAPSEDTHREADS", "true")
+	defer os.Unsetenv("MM_FEATUREFLAGS_COLLAPSEDTHREADS")
+
+	th.App.UpdateConfig(func(cfg *model.Config) {
+		*cfg.ServiceSettings.ThreadAutoFollow = true
+		*cfg.ServiceSettings.CollapsedThreads = model.CollapsedThreadsDefaultOn
+	})
+
+	channel := th.BasicChannel
+	user := th.BasicUser
+	user2 := th.BasicUser2
+	appErr := th.App.JoinChannel(th.Context, channel, user.Id)
+	require.Nil(t, appErr)
+	appErr = th.App.JoinChannel(th.Context, channel, user2.Id)
+	require.Nil(t, appErr)
+	p1, err := th.App.CreatePost(th.Context, &model.Post{UserId: user.Id, ChannelId: channel.Id, Message: "Hi @" + user2.Username}, channel, false, false)
+	require.Nil(t, err)
+	_, err = th.App.CreatePost(th.Context, &model.Post{RootId: p1.Id, UserId: user2.Id, ChannelId: channel.Id, Message: "Hola"}, channel, false, false)
+	require.Nil(t, err)
+
+	// User2 unfollows thread
+	m, nErr := th.App.Srv().Store.Thread().MaintainMembership(user2.Id, p1.Id, store.ThreadMembershipOpts{
+		Following:       false,
+		UpdateFollowing: true,
+	})
+	require.NoError(t, nErr)
+	require.False(t, m.Following)
+
+	// user posts in the thread
+	_, err = th.App.CreatePost(th.Context, &model.Post{RootId: p1.Id, UserId: user.Id, ChannelId: channel.Id, Message: "another reply"}, channel, false, false)
+	require.Nil(t, err)
+
+	// User2 should still not be following the thread because they manually
+	// unfollowed the thread
+	m, err = th.App.GetThreadMembershipForUser(user2.Id, p1.Id)
+	require.Nil(t, err)
+	require.False(t, m.Following)
+
+	// user posts in the thread mentioning user2
+	_, err = th.App.CreatePost(th.Context, &model.Post{RootId: p1.Id, UserId: user.Id, ChannelId: channel.Id, Message: "reply with mention @" + user2.Username}, channel, false, false)
+	require.Nil(t, err)
+
+	// User2 should now be following the thread because they were explicitly mentioned
+	m, err = th.App.GetThreadMembershipForUser(user2.Id, p1.Id)
+	require.Nil(t, err)
+	require.True(t, m.Following)
+}

app/user.go

@@ -2271,7 +2271,7 @@ func (a *App) UpdateThreadReadForUser(userID, teamID, threadID string, timestamp
 
 	opts := store.ThreadMembershipOpts{
 		Following:       true,
-		UpdateFollowing: false,
+		UpdateFollowing: true,
 	}
 	membership, storeErr := a.Srv().Store.Thread().MaintainMembership(userID, threadID, opts)
 	if storeErr != nil {
@@ -2286,7 +2286,6 @@ func (a *App) UpdateThreadReadForUser(userID, teamID, threadID string, timestamp
 	if err != nil {
 		return nil, err
 	}
-	membership.Following = true
 	_, nErr := a.Srv().Store.Thread().UpdateMembership(membership)
 	if nErr != nil {
 		return nil, model.NewAppError("UpdateThreadReadForUser", "app.user.update_thread_read_for_user.app_error", nil, nErr.Error(), http.StatusInternalServerError)

store/sqlstore/thread_store.go

@@ -611,7 +611,7 @@ func (s *SqlThreadStore) MaintainMembership(userId, postId string, opts store.Th
 	// b. mention count changed
 	// c. user viewed a thread
 	if err == nil {
-		followingNeedsUpdate := (opts.UpdateFollowing && !membership.Following || membership.Following != opts.Following)
+		followingNeedsUpdate := (opts.UpdateFollowing && (membership.Following != opts.Following))
 		if followingNeedsUpdate || opts.IncrementMentions || opts.UpdateViewedTimestamp {
 			if followingNeedsUpdate {
 				membership.Following = opts.Following

store/store.go

@@ -934,7 +934,7 @@ type ThreadMembershipOpts struct {
 	// IncrementMentions indicates whether or not the mentions count for
 	// the thread should be incremented.
 	IncrementMentions bool
-	// UpdateFollowing indicates whether or not a membership update should be forced.
+	// UpdateFollowing indicates whether or not the following state should be changed.
 	UpdateFollowing bool
 	// UpdateViewedTimestamp indicates whether or not the LastViewed field of the
 	// membership should be updated.