MM-23896: Fix clearing of batched emails on user activity (#14340)

On user activity, we were clearing the job.pendingNotifications map.
But we had already created a copy of the notifications slice while
iterating the map. Therefore, if we pass the copied slice, it would
still have the old notifications which were originally deleted.

The unit tests would not catch this because it was testing the
job.pendingNotifications map and not actually checking if the email
handler was being called or not. We fix that now.

Co-authored-by: mattermod <mattermod@users.noreply.github.com>

app/email_batching.go

@@ -182,13 +182,13 @@ func (job *EmailBatchingJob) checkPendingNotifications(now time.Time, handler fu
 			}
 		}
 
-		// send the email notification if it's been long enough
-		if now.Sub(time.Unix(batchStartTime/1000, 0)) > time.Duration(interval)*time.Second {
+		// send the email notification if there are notifications to send AND it's been long enough
+		if len(job.pendingNotifications[userId]) > 0 && now.Sub(time.Unix(batchStartTime/1000, 0)) > time.Duration(interval)*time.Second {
 			job.server.Go(func(userId string, notifications []*batchedNotification) func() {
 				return func() {
 					handler(userId, notifications)
 				}
-			}(userId, notifications))
+			}(userId, job.pendingNotifications[userId]))
 			delete(job.pendingNotifications, userId)
 		}
 	}

app/email_batching_test.go

@@ -4,6 +4,7 @@
 package app
 
 import (
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -115,7 +116,28 @@ func TestCheckPendingNotifications(t *testing.T) {
 	_, err = th.App.Srv().Store.Channel().UpdateMember(channelMember)
 	require.Nil(t, err)
 
-	job.checkPendingNotifications(time.Unix(10002, 0), func(string, []*batchedNotification) {})
+	// We reset the interval to something shorter
+	err = th.App.Srv().Store.Preference().Save(&model.Preferences{{
+		UserId:   th.BasicUser.Id,
+		Category: model.PREFERENCE_CATEGORY_NOTIFICATIONS,
+		Name:     model.PREFERENCE_NAME_EMAIL_INTERVAL,
+		Value:    "10",
+	}})
+	require.Nil(t, err)
+
+	var wasCalled int32
+	job.checkPendingNotifications(time.Unix(10050, 0), func(string, []*batchedNotification) {
+		atomic.StoreInt32(&wasCalled, int32(1))
+	})
+
+	// A hack to check whether the handler was called.
+	// It's not straightforward to just wait for it using a channel because the test should
+	// NOT call the handler, and it will be called only if the test fails.
+	time.Sleep(1 * time.Second)
+	// We do a check outside the email handler, because otherwise, failing from
+	// inside the handler doesn't let the .Go() function exit cleanly, and it gets
+	// stuck during server shutdown, trying to wait for the goroutine to exit
+	require.Equal(t, atomic.LoadInt32(&wasCalled), int32(0), "email handler should not have been called")
 
 	require.Nil(t, job.pendingNotifications[th.BasicUser.Id])
 	require.Empty(t, job.pendingNotifications[th.BasicUser.Id], "should've remove queued post since user acted")