From 7de54f063cb260dfcae6c21563df3a044e2160b1 Mon Sep 17 00:00:00 2001 From: enahum Date: Thu, 2 Mar 2017 18:38:38 -0300 Subject: 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 | 72 +++++++++--- app/notification_test.go | 290 +++++++++++++++++++++++++++++++++++++++++++++++ app/status.go | 18 --- 3 files changed, 348 insertions(+), 32 deletions(-) (limited to 'app') diff --git a/app/notification.go b/app/notification.go index 56b5ec336..e0c5d3d5b 100644 --- a/app/notification.go +++ b/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) { - sendPushNotification(post, profileMap[id], channel, senderName[id], channelMemberNotifyPropsMap[id], true) + if ShouldSendPushNotification(profileMap[id], channelMemberNotifyPropsMap[id], true, status, post) { + 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) { - sendPushNotification(post, profileMap[id], channel, senderName[id], channelMemberNotifyPropsMap[id], false) + if ShouldSendPushNotification(profileMap[id], channelMemberNotifyPropsMap[id], false, status, post) { + 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 +} diff --git a/app/notification_test.go b/app/notification_test.go index 10eb09247..3768a95c7 100644 --- a/app/notification_test.go +++ b/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") + } +} diff --git a/app/status.go b/app/status.go index bef17165f..51e7d1ed8 100644 --- a/app/status.go +++ b/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 -} -- cgit v1.2.3-1-g7c22