Fix push notifications where channel is set to all activity (#5594)

* Fix push notifications where channel is set to all activity

* feedback review

* moved push notification logic to DoesStatusAllowPushNotification

* Have every option handled in ShouldSendPushNotification

* unit tests

app/notification.go

@@ -102,7 +102,8 @@ func SendNotifications(post *model.Post, team *model.Team, channel *model.Channe
 
 		// find which users in the channel are set up to always receive mobile notifications
 		for _, profile := range profileMap {
-			if profile.NotifyProps[model.PUSH_NOTIFY_PROP] == model.USER_NOTIFY_ALL &&
+			if (profile.NotifyProps[model.PUSH_NOTIFY_PROP] == model.USER_NOTIFY_ALL ||
+				channelMemberNotifyPropsMap[profile.Id][model.PUSH_NOTIFY_PROP] == model.CHANNEL_NOTIFY_ALL) &&
 				(post.UserId != profile.Id || post.Props["from_webhook"] == "true") &&
 				!post.IsSystemMessage() {
 				allActivityPushUserIds = append(allActivityPushUserIds, profile.Id)
@@ -257,8 +258,8 @@ func SendNotifications(post *model.Post, team *model.Team, channel *model.Channe
 				status = &model.Status{UserId: id, Status: model.STATUS_OFFLINE, Manual: false, LastActivityAt: 0, ActiveChannel: ""}
 			}
 
-			if DoesStatusAllowPushNotification(profileMap[id], status, post.ChannelId) {
+			if ShouldSendPushNotification(profileMap[id], channelMemberNotifyPropsMap[id], true, status, post) {
-				sendPushNotification(post, profileMap[id], channel, senderName[id], channelMemberNotifyPropsMap[id], true)
+				sendPushNotification(post, profileMap[id], channel, senderName[id], true)
 			}
 		}
 
@@ -270,8 +271,8 @@ func SendNotifications(post *model.Post, team *model.Team, channel *model.Channe
 					status = &model.Status{UserId: id, Status: model.STATUS_OFFLINE, Manual: false, LastActivityAt: 0, ActiveChannel: ""}
 				}
 
-				if DoesStatusAllowPushNotification(profileMap[id], status, post.ChannelId) {
+				if ShouldSendPushNotification(profileMap[id], channelMemberNotifyPropsMap[id], false, status, post) {
-					sendPushNotification(post, profileMap[id], channel, senderName[id], channelMemberNotifyPropsMap[id], false)
+					sendPushNotification(post, profileMap[id], channel, senderName[id], false)
 				}
 			}
 		}
@@ -455,7 +456,7 @@ func GetMessageForNotification(post *model.Post, translateFunc i18n.TranslateFun
 	}
 }
 
-func sendPushNotification(post *model.Post, user *model.User, channel *model.Channel, senderName string, channelNotifyProps model.StringMap, wasMentioned bool) *model.AppError {
+func sendPushNotification(post *model.Post, user *model.User, channel *model.Channel, senderName string, wasMentioned bool) *model.AppError {
 	sessions, err := getMobileAppSessions(user.Id)
 	if err != nil {
 		return err
@@ -463,14 +464,6 @@ func sendPushNotification(post *model.Post, user *model.User, channel *model.Cha
 
 	var channelName string
 
-	if channelNotify, ok := channelNotifyProps[model.PUSH_NOTIFY_PROP]; ok {
-		if channelNotify == model.USER_NOTIFY_NONE {
-			return nil
-		} else if channelNotify == model.USER_NOTIFY_MENTION && !wasMentioned {
-			return nil
-		}
-	}
-
 	if channel.Type == model.CHANNEL_DIRECT {
 		channelName = senderName
 	} else {
@@ -744,3 +737,54 @@ func GetMentionKeywordsInChannel(profiles map[string]*model.User) map[string][]s
 
 	return keywords
 }
+
+func ShouldSendPushNotification(user *model.User, channelNotifyProps model.StringMap, wasMentioned bool, status *model.Status, post *model.Post) bool {
+	return DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, wasMentioned) &&
+		DoesStatusAllowPushNotification(user.NotifyProps, status, post.ChannelId)
+}
+
+func DoesNotifyPropsAllowPushNotification(user *model.User, channelNotifyProps model.StringMap, post *model.Post, wasMentioned bool) bool {
+	userNotifyProps := user.NotifyProps
+	userNotify := userNotifyProps[model.PUSH_NOTIFY_PROP]
+	channelNotify, ok := channelNotifyProps[model.PUSH_NOTIFY_PROP]
+
+	if post.IsSystemMessage() {
+		return false
+	}
+
+	if channelNotify == model.USER_NOTIFY_NONE {
+		return false
+	}
+
+	if channelNotify == model.CHANNEL_NOTIFY_MENTION && !wasMentioned {
+		return false
+	}
+
+	if userNotify == model.USER_NOTIFY_MENTION && (!ok || channelNotify == model.CHANNEL_NOTIFY_DEFAULT) && !wasMentioned {
+		return false
+	}
+
+	if (userNotify == model.USER_NOTIFY_ALL || channelNotify == model.CHANNEL_NOTIFY_ALL) &&
+		(post.UserId != user.Id || post.Props["from_webhook"] == "true") {
+		return true
+	}
+
+	if userNotify == model.USER_NOTIFY_NONE &&
+		(!ok || channelNotify == model.CHANNEL_NOTIFY_DEFAULT) {
+		return false
+	}
+
+	return true
+}
+
+func DoesStatusAllowPushNotification(userNotifyProps model.StringMap, status *model.Status, channelId string) bool {
+	if pushStatus, ok := userNotifyProps["push_status"]; (pushStatus == model.STATUS_ONLINE || !ok) && (status.ActiveChannel != channelId || model.GetMillis()-status.LastActivityAt > model.STATUS_CHANNEL_TIMEOUT) {
+		return true
+	} else if pushStatus == model.STATUS_AWAY && (status.Status == model.STATUS_AWAY || status.Status == model.STATUS_OFFLINE) {
+		return true
+	} else if pushStatus == model.STATUS_OFFLINE && status.Status == model.STATUS_OFFLINE {
+		return true
+	}
+
+	return false
+}

app/notification_test.go

@@ -311,3 +311,293 @@ func TestGetMentionKeywords(t *testing.T) {
 		t.Fatal("should've mentioned user3 and user4 with @all")
 	}
 }
+
+func TestDoesNotifyPropsAllowPushNotification(t *testing.T) {
+	userNotifyProps := make(map[string]string)
+	channelNotifyProps := make(map[string]string)
+
+	user := &model.User{Id: model.NewId(), Email: "unit@test.com"}
+
+	post := &model.Post{UserId: user.Id, ChannelId: model.NewId()}
+
+	// When the post is a System Message
+	systemPost := &model.Post{UserId: user.Id, Type: model.POST_JOIN_CHANNEL}
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_ALL
+	user.NotifyProps = userNotifyProps
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, systemPost, false) {
+		t.Fatal("Should have returned false")
+	}
+
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, systemPost, true) {
+		t.Fatal("Should have returned false")
+	}
+
+	// When default is ALL and no channel props is set
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned true")
+	}
+
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned true")
+	}
+
+	// When default is MENTION and no channel props is set
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_MENTION
+	user.NotifyProps = userNotifyProps
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned false")
+	}
+
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned true")
+	}
+
+	// When default is NONE and no channel props is set
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_NONE
+	user.NotifyProps = userNotifyProps
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned false")
+	}
+
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned false")
+	}
+
+	// WHEN default is ALL and channel is DEFAULT
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_ALL
+	user.NotifyProps = userNotifyProps
+	channelNotifyProps[model.PUSH_NOTIFY_PROP] = model.CHANNEL_NOTIFY_DEFAULT
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned true")
+	}
+
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned true")
+	}
+
+	// WHEN default is MENTION and channel is DEFAULT
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_MENTION
+	user.NotifyProps = userNotifyProps
+	channelNotifyProps[model.PUSH_NOTIFY_PROP] = model.CHANNEL_NOTIFY_DEFAULT
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned false")
+	}
+
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned true")
+	}
+
+	// WHEN default is NONE and channel is DEFAULT
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_NONE
+	user.NotifyProps = userNotifyProps
+	channelNotifyProps[model.PUSH_NOTIFY_PROP] = model.CHANNEL_NOTIFY_DEFAULT
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned false")
+	}
+
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned false")
+	}
+
+	// WHEN default is ALL and channel is ALL
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_ALL
+	user.NotifyProps = userNotifyProps
+	channelNotifyProps[model.PUSH_NOTIFY_PROP] = model.CHANNEL_NOTIFY_ALL
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned true")
+	}
+
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned true")
+	}
+
+	// WHEN default is MENTION and channel is ALL
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_MENTION
+	user.NotifyProps = userNotifyProps
+	channelNotifyProps[model.PUSH_NOTIFY_PROP] = model.CHANNEL_NOTIFY_ALL
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned true")
+	}
+
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned true")
+	}
+
+	// WHEN default is NONE and channel is ALL
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_NONE
+	user.NotifyProps = userNotifyProps
+	channelNotifyProps[model.PUSH_NOTIFY_PROP] = model.CHANNEL_NOTIFY_ALL
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned true")
+	}
+
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned true")
+	}
+
+	// WHEN default is ALL and channel is MENTION
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_ALL
+	user.NotifyProps = userNotifyProps
+	channelNotifyProps[model.PUSH_NOTIFY_PROP] = model.CHANNEL_NOTIFY_MENTION
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned false")
+	}
+
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned true")
+	}
+
+	// WHEN default is MENTION and channel is MENTION
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_MENTION
+	user.NotifyProps = userNotifyProps
+	channelNotifyProps[model.PUSH_NOTIFY_PROP] = model.CHANNEL_NOTIFY_MENTION
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned false")
+	}
+
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned true")
+	}
+
+	// WHEN default is NONE and channel is MENTION
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_NONE
+	user.NotifyProps = userNotifyProps
+	channelNotifyProps[model.PUSH_NOTIFY_PROP] = model.CHANNEL_NOTIFY_MENTION
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned false")
+	}
+
+	if !DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned true")
+	}
+
+	// WHEN default is ALL and channel is NONE
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_ALL
+	user.NotifyProps = userNotifyProps
+	channelNotifyProps[model.PUSH_NOTIFY_PROP] = model.CHANNEL_NOTIFY_NONE
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned false")
+	}
+
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned false")
+	}
+
+	// WHEN default is MENTION and channel is NONE
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_MENTION
+	user.NotifyProps = userNotifyProps
+	channelNotifyProps[model.PUSH_NOTIFY_PROP] = model.CHANNEL_NOTIFY_NONE
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned false")
+	}
+
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned false")
+	}
+
+	// WHEN default is NONE and channel is NONE
+	userNotifyProps[model.PUSH_NOTIFY_PROP] = model.USER_NOTIFY_NONE
+	user.NotifyProps = userNotifyProps
+	channelNotifyProps[model.PUSH_NOTIFY_PROP] = model.CHANNEL_NOTIFY_NONE
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, false) {
+		t.Fatal("Should have returned false")
+	}
+
+	if DoesNotifyPropsAllowPushNotification(user, channelNotifyProps, post, true) {
+		t.Fatal("Should have returned false")
+	}
+}
+
+func TestDoesStatusAllowPushNotification(t *testing.T) {
+	userNotifyProps := make(map[string]string)
+	userId := model.NewId()
+	channelId := model.NewId()
+
+	offline := &model.Status{UserId: userId, Status: model.STATUS_OFFLINE, Manual: false, LastActivityAt: 0, ActiveChannel: ""}
+	away := &model.Status{UserId: userId, Status: model.STATUS_AWAY, Manual: false, LastActivityAt: 0, ActiveChannel: ""}
+	online := &model.Status{UserId: userId, Status: model.STATUS_ONLINE, Manual: false, LastActivityAt: model.GetMillis(), ActiveChannel: ""}
+
+	userNotifyProps["push_status"] = model.STATUS_ONLINE
+	// WHEN props is ONLINE and user is offline
+	if !DoesStatusAllowPushNotification(userNotifyProps, offline, channelId) {
+		t.Fatal("Should have been true")
+	}
+
+	if !DoesStatusAllowPushNotification(userNotifyProps, offline, "") {
+		t.Fatal("Should have been true")
+	}
+
+	// WHEN props is ONLINE and user is away
+	if !DoesStatusAllowPushNotification(userNotifyProps, away, channelId) {
+		t.Fatal("Should have been true")
+	}
+
+	if !DoesStatusAllowPushNotification(userNotifyProps, away, "") {
+		t.Fatal("Should have been true")
+	}
+
+	// WHEN props is ONLINE and user is online
+	if !DoesStatusAllowPushNotification(userNotifyProps, online, channelId) {
+		t.Fatal("Should have been true")
+	}
+
+	if DoesStatusAllowPushNotification(userNotifyProps, online, "") {
+		t.Fatal("Should have been false")
+	}
+
+	userNotifyProps["push_status"] = model.STATUS_AWAY
+	// WHEN props is AWAY and user is offline
+	if !DoesStatusAllowPushNotification(userNotifyProps, offline, channelId) {
+		t.Fatal("Should have been true")
+	}
+
+	if !DoesStatusAllowPushNotification(userNotifyProps, offline, "") {
+		t.Fatal("Should have been true")
+	}
+
+	// WHEN props is AWAY and user is away
+	if !DoesStatusAllowPushNotification(userNotifyProps, away, channelId) {
+		t.Fatal("Should have been true")
+	}
+
+	if !DoesStatusAllowPushNotification(userNotifyProps, away, "") {
+		t.Fatal("Should have been true")
+	}
+
+	// WHEN props is AWAY and user is online
+	if DoesStatusAllowPushNotification(userNotifyProps, online, channelId) {
+		t.Fatal("Should have been false")
+	}
+
+	if DoesStatusAllowPushNotification(userNotifyProps, online, "") {
+		t.Fatal("Should have been false")
+	}
+
+	userNotifyProps["push_status"] = model.STATUS_OFFLINE
+	// WHEN props is AWAY and user is offline
+	if !DoesStatusAllowPushNotification(userNotifyProps, offline, channelId) {
+		t.Fatal("Should have been true")
+	}
+
+	if !DoesStatusAllowPushNotification(userNotifyProps, offline, "") {
+		t.Fatal("Should have been true")
+	}
+
+	// WHEN props is AWAY and user is away
+	if DoesStatusAllowPushNotification(userNotifyProps, away, channelId) {
+		t.Fatal("Should have been false")
+	}
+
+	if DoesStatusAllowPushNotification(userNotifyProps, away, "") {
+		t.Fatal("Should have been false")
+	}
+
+	// WHEN props is AWAY and user is online
+	if DoesStatusAllowPushNotification(userNotifyProps, online, channelId) {
+		t.Fatal("Should have been false")
+	}
+
+	if DoesStatusAllowPushNotification(userNotifyProps, online, "") {
+		t.Fatal("Should have been false")
+	}
+}

app/status.go

@@ -235,21 +235,3 @@ func GetStatus(userId string) (*model.Status, *model.AppError) {
 func IsUserAway(lastActivityAt int64) bool {
 	return model.GetMillis()-lastActivityAt >= *utils.Cfg.TeamSettings.UserStatusAwayTimeout*1000
 }
-
-func DoesStatusAllowPushNotification(user *model.User, status *model.Status, channelId string) bool {
-	props := user.NotifyProps
-
-	if props["push"] == "none" {
-		return false
-	}
-
-	if pushStatus, ok := props["push_status"]; (pushStatus == model.STATUS_ONLINE || !ok) && (status.ActiveChannel != channelId || model.GetMillis()-status.LastActivityAt > model.STATUS_CHANNEL_TIMEOUT) {
-		return true
-	} else if pushStatus == model.STATUS_AWAY && (status.Status == model.STATUS_AWAY || status.Status == model.STATUS_OFFLINE) {
-		return true
-	} else if pushStatus == model.STATUS_OFFLINE && status.Status == model.STATUS_OFFLINE {
-		return true
-	}
-
-	return false
-}