diff --git a/app/email_batching.go b/app/email_batching.go index c3f2a2bd83..56c3889ec5 100644 --- a/app/email_batching.go +++ b/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) } } diff --git a/app/email_batching_test.go b/app/email_batching_test.go index 4f6cdf96d8..a4ca0f9dad 100644 --- a/app/email_batching_test.go +++ b/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")