PLT-7444: If there is activity in Mattermost before the email batch is sent, do not send the email (#7342)

* Changed email batching short-circuit logic to look at last viewed at timestamp in channel member struct instead of in user's status struct, since the latter is only updated if the user's status is set to online

* Fixed unit tests

* Reduced right-hand drift

* Reduced total number of store calls by loading all channel member objects for user exactly once per team that the user received notifications for

app/email_batching.go

@@ -122,24 +122,42 @@ func (job *EmailBatchingJob) handleNewNotifications() {
 }
 
 func (job *EmailBatchingJob) checkPendingNotifications(now time.Time, handler func(string, []*batchedNotification)) {
-	// look for users who've acted since pending posts were received
 	for userId, notifications := range job.pendingNotifications {
-		schan := Srv.Store.Status().Get(userId)
-		pchan := Srv.Store.Preference().Get(userId, model.PREFERENCE_CATEGORY_NOTIFICATIONS, model.PREFERENCE_NAME_EMAIL_INTERVAL)
 		batchStartTime := notifications[0].post.CreateAt
+		inspectedTeamNames := make(map[string]string)
+		for _, notification := range notifications {
+			// at most, we'll do one check for each team that notifications were sent for
+			if inspectedTeamNames[notification.teamName] != "" {
+				continue
+			}
+			tchan := Srv.Store.Team().GetByName(notifications[0].teamName)
+			if result := <-tchan; result.Err != nil {
+				l4g.Error("Unable to find Team id for notification", result.Err)
+				continue
+			} else if team, ok := result.Data.(*model.Team); ok {
+				inspectedTeamNames[notification.teamName] = team.Id
+			}
 
-		// check if the user has been active and would've seen any new posts
+			// if the user has viewed any channels in this team since the notification was queued, delete
-		if result := <-schan; result.Err != nil {
+			// all queued notifications
-			l4g.Error(utils.T("api.email_batching.check_pending_emails.status.app_error"), result.Err)
+			mchan := Srv.Store.Channel().GetMembersForUser(inspectedTeamNames[notification.teamName], userId)
-			delete(job.pendingNotifications, userId)
+			if result := <-mchan; result.Err != nil {
-			continue
+				l4g.Error("Unable to find ChannelMembers for user", result.Err)
-		} else if status := result.Data.(*model.Status); status.LastActivityAt >= batchStartTime {
+				continue
-			delete(job.pendingNotifications, userId)
+			} else if channelMembers, ok := result.Data.(*model.ChannelMembers); ok {
-			continue
+				for _, channelMember := range *channelMembers {
+					if channelMember.LastViewedAt >= batchStartTime {
+						l4g.Info("Deleted notifications for user %s", userId)
+						delete(job.pendingNotifications, userId)
+						break
+					}
+				}
+			}
 		}
 
 		// get how long we need to wait to send notifications to the user
 		var interval int64
+		pchan := Srv.Store.Preference().Get(userId, model.PREFERENCE_CATEGORY_NOTIFICATIONS, model.PREFERENCE_NAME_EMAIL_INTERVAL)
 		if result := <-pchan; result.Err != nil {
 			// use the default batching interval if an error ocurrs while fetching user preferences
 			interval, _ = strconv.ParseInt(model.PREFERENCE_EMAIL_INTERVAL_BATCHING_SECONDS, 10, 64)

app/email_batching_test.go

@@ -93,26 +93,26 @@ func TestHandleNewNotifications(t *testing.T) {
 }
 
 func TestCheckPendingNotifications(t *testing.T) {
-	Setup()
+	th := Setup().InitBasic()
-
-	id1 := model.NewId()
 
 	job := MakeEmailBatchingJob(128)
-	job.pendingNotifications[id1] = []*batchedNotification{
+	job.pendingNotifications[th.BasicUser.Id] = []*batchedNotification{
 		{
 			post: &model.Post{
-				UserId:   id1,
+				UserId:   th.BasicUser.Id,
+				ChannelId: th.BasicChannel.Id,
 				CreateAt: 10000000,
 			},
+			teamName: th.BasicTeam.Name,
 		},
 	}
 
-	store.Must(Srv.Store.Status().SaveOrUpdate(&model.Status{
+	channelMember := store.Must(Srv.Store.Channel().GetMember(th.BasicChannel.Id, th.BasicUser.Id)).(*model.ChannelMember)
-		UserId:         id1,
+	channelMember.LastViewedAt = 9999999
-		LastActivityAt: 9999000,
+	store.Must(Srv.Store.Channel().UpdateMember(channelMember))
-	}))
+
 	store.Must(Srv.Store.Preference().Save(&model.Preferences{{
-		UserId:   id1,
+		UserId:   th.BasicUser.Id,
 		Category: model.PREFERENCE_CATEGORY_NOTIFICATIONS,
 		Name:     model.PREFERENCE_NAME_EMAIL_INTERVAL,
 		Value:    "60",
@@ -121,37 +121,40 @@ func TestCheckPendingNotifications(t *testing.T) {
 	// test that notifications aren't sent before interval
 	job.checkPendingNotifications(time.Unix(10001, 0), func(string, []*batchedNotification) {})
 
-	if job.pendingNotifications[id1] == nil || len(job.pendingNotifications[id1]) != 1 {
+	if job.pendingNotifications[th.BasicUser.Id] == nil || len(job.pendingNotifications[th.BasicUser.Id]) != 1 {
-		t.Fatal("should'nt have sent queued post")
+		t.Fatal("shouldn't have sent queued post")
 	}
 
 	// test that notifications are cleared if the user has acted
-	store.Must(Srv.Store.Status().SaveOrUpdate(&model.Status{
+	channelMember = store.Must(Srv.Store.Channel().GetMember(th.BasicChannel.Id, th.BasicUser.Id)).(*model.ChannelMember)
-		UserId:         id1,
+	channelMember.LastViewedAt = 10001000
-		LastActivityAt: 10001000,
+	store.Must(Srv.Store.Channel().UpdateMember(channelMember))
-	}))
 
 	job.checkPendingNotifications(time.Unix(10002, 0), func(string, []*batchedNotification) {})
 
-	if job.pendingNotifications[id1] != nil && len(job.pendingNotifications[id1]) != 0 {
+	if job.pendingNotifications[th.BasicUser.Id] != nil && len(job.pendingNotifications[th.BasicUser.Id]) != 0 {
 		t.Fatal("should've remove queued post since user acted")
 	}
 
 	// test that notifications are sent if enough time passes since the first message
-	job.pendingNotifications[id1] = []*batchedNotification{
+	job.pendingNotifications[th.BasicUser.Id] = []*batchedNotification{
 		{
 			post: &model.Post{
-				UserId:   id1,
+				UserId:   th.BasicUser.Id,
+				ChannelId: th.BasicChannel.Id,
 				CreateAt: 10060000,
 				Message:  "post1",
 			},
+			teamName: th.BasicTeam.Name,
 		},
 		{
 			post: &model.Post{
-				UserId:   id1,
+				UserId:   th.BasicUser.Id,
+				ChannelId: th.BasicChannel.Id,
 				CreateAt: 10090000,
 				Message:  "post2",
 			},
+			teamName: th.BasicTeam.Name,
 		},
 	}
 
@@ -170,7 +173,7 @@ func TestCheckPendingNotifications(t *testing.T) {
 		timeout <- true
 	}()
 
-	if job.pendingNotifications[id1] != nil && len(job.pendingNotifications[id1]) != 0 {
+	if job.pendingNotifications[th.BasicUser.Id] != nil && len(job.pendingNotifications[th.BasicUser.Id]) != 0 {
 		t.Fatal("should've remove queued posts when sending messages")
 	}
 
@@ -197,34 +200,34 @@ func TestCheckPendingNotifications(t *testing.T) {
  * Ensures that email batch interval defaults to 15 minutes for users that haven't explicitly set this preference
  */
 func TestCheckPendingNotificationsDefaultInterval(t *testing.T) {
-	Setup()
+	th := Setup().InitBasic()
-	id1 := model.NewId()
 	job := MakeEmailBatchingJob(128)
 
 	// bypasses recent user activity check
-	store.Must(Srv.Store.Status().SaveOrUpdate(&model.Status{
+	channelMember := store.Must(Srv.Store.Channel().GetMember(th.BasicChannel.Id, th.BasicUser.Id)).(*model.ChannelMember)
-		UserId:         id1,
+	channelMember.LastViewedAt = 9999000
-		LastActivityAt: 9999000,
+	store.Must(Srv.Store.Channel().UpdateMember(channelMember))
-	}))
 
-	job.pendingNotifications[id1] = []*batchedNotification{
+	job.pendingNotifications[th.BasicUser.Id] = []*batchedNotification{
 		{
 			post: &model.Post{
-				UserId:   id1,
+				UserId:   th.BasicUser.Id,
+				ChannelId: th.BasicChannel.Id,
 				CreateAt: 10000000,
 			},
+			teamName: th.BasicTeam.Name,
 		},
 	}
 
 	// notifications should not be sent 1s after post was created, because default batch interval is 15mins
 	job.checkPendingNotifications(time.Unix(10001, 0), func(string, []*batchedNotification) {})
-	if job.pendingNotifications[id1] == nil || len(job.pendingNotifications[id1]) != 1 {
+	if job.pendingNotifications[th.BasicUser.Id] == nil || len(job.pendingNotifications[th.BasicUser.Id]) != 1 {
 		t.Fatal("shouldn't have sent queued post")
 	}
 
 	// notifications should be sent 901s after post was created, because default batch interval is 15mins
 	job.checkPendingNotifications(time.Unix(10901, 0), func(string, []*batchedNotification) {})
-	if job.pendingNotifications[id1] != nil || len(job.pendingNotifications[id1]) != 0 {
+	if job.pendingNotifications[th.BasicUser.Id] != nil || len(job.pendingNotifications[th.BasicUser.Id]) != 0 {
 		t.Fatal("should have sent queued post")
 	}
 }
@@ -233,42 +236,42 @@ func TestCheckPendingNotificationsDefaultInterval(t *testing.T) {
  * Ensures that email batch interval defaults to 15 minutes if user preference is invalid
  */
 func TestCheckPendingNotificationsCantParseInterval(t *testing.T) {
-	Setup()
+	th := Setup().InitBasic()
-	id1 := model.NewId()
 	job := MakeEmailBatchingJob(128)
 
 	// bypasses recent user activity check
-	store.Must(Srv.Store.Status().SaveOrUpdate(&model.Status{
+	channelMember := store.Must(Srv.Store.Channel().GetMember(th.BasicChannel.Id, th.BasicUser.Id)).(*model.ChannelMember)
-		UserId:         id1,
+	channelMember.LastViewedAt = 9999000
-		LastActivityAt: 9999000,
+	store.Must(Srv.Store.Channel().UpdateMember(channelMember))
-	}))
 
 	// preference value is not an integer, so we'll fall back to the default 15min value
 	store.Must(Srv.Store.Preference().Save(&model.Preferences{{
-		UserId:   id1,
+		UserId:   th.BasicUser.Id,
 		Category: model.PREFERENCE_CATEGORY_NOTIFICATIONS,
 		Name:     model.PREFERENCE_NAME_EMAIL_INTERVAL,
 		Value:    "notAnIntegerValue",
 	}}))
 
-	job.pendingNotifications[id1] = []*batchedNotification{
+	job.pendingNotifications[th.BasicUser.Id] = []*batchedNotification{
 		{
 			post: &model.Post{
-				UserId:   id1,
+				UserId:   th.BasicUser.Id,
+				ChannelId: th.BasicChannel.Id,
 				CreateAt: 10000000,
 			},
+			teamName: th.BasicTeam.Name,
 		},
 	}
 
 	// notifications should not be sent 1s after post was created, because default batch interval is 15mins
 	job.checkPendingNotifications(time.Unix(10001, 0), func(string, []*batchedNotification) {})
-	if job.pendingNotifications[id1] == nil || len(job.pendingNotifications[id1]) != 1 {
+	if job.pendingNotifications[th.BasicUser.Id] == nil || len(job.pendingNotifications[th.BasicUser.Id]) != 1 {
 		t.Fatal("shouldn't have sent queued post")
 	}
 
 	// notifications should be sent 901s after post was created, because default batch interval is 15mins
 	job.checkPendingNotifications(time.Unix(10901, 0), func(string, []*batchedNotification) {})
-	if job.pendingNotifications[id1] != nil || len(job.pendingNotifications[id1]) != 0 {
+	if job.pendingNotifications[th.BasicUser.Id] != nil || len(job.pendingNotifications[th.BasicUser.Id]) != 0 {
 		t.Fatal("should have sent queued post")
 	}
 }

i18n/en.json

@@ -971,10 +971,6 @@
     "id": "api.email_batching.check_pending_emails.finished_running",
     "translation": "Email batching job ran. %v user(s) still have notifications pending."
   },
-  {
-    "id": "api.email_batching.check_pending_emails.status.app_error",
-    "translation": "Unable to find status of recipient for batched email notification"
-  },
   {
     "id": "api.email_batching.render_batched_post.channel.app_error",
     "translation": "Unable to find channel of post for batched email notification"