From 6c2a5555b85bd15106df5f4f631bb6e945a187b8 Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Mon, 17 Sep 2018 14:50:12 -0400 Subject: MM-11700 Clean up handling of user display names for notifications (#9343) * MM-11700 Clean up handling of user display names for notifications --- app/import_functions.go | 6 +- app/import_functions_test.go | 16 ++--- app/notification.go | 96 ++++++++++++++----------- app/notification_email.go | 23 ++++-- app/notification_push.go | 34 +++++---- app/notification_test.go | 162 +++++++++++++++++++++++++++++++++++++++++++ model/preference.go | 4 ++ 7 files changed, 269 insertions(+), 72 deletions(-) diff --git a/app/import_functions.go b/app/import_functions.go index 98306d3a7..cd047ba71 100644 --- a/app/import_functions.go +++ b/app/import_functions.go @@ -514,7 +514,7 @@ func (a *App) ImportUser(data *UserImportData, dryRun bool) *model.AppError { preferences = append(preferences, model.Preference{ UserId: savedUser.Id, Category: model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, - Name: "use_military_time", + Name: model.PREFERENCE_NAME_USE_MILITARY_TIME, Value: *data.UseMilitaryTime, }) } @@ -523,7 +523,7 @@ func (a *App) ImportUser(data *UserImportData, dryRun bool) *model.AppError { preferences = append(preferences, model.Preference{ UserId: savedUser.Id, Category: model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, - Name: "collapse_previews", + Name: model.PREFERENCE_NAME_COLLAPSE_SETTING, Value: *data.CollapsePreviews, }) } @@ -532,7 +532,7 @@ func (a *App) ImportUser(data *UserImportData, dryRun bool) *model.AppError { preferences = append(preferences, model.Preference{ UserId: savedUser.Id, Category: model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, - Name: "message_display", + Name: model.PREFERENCE_NAME_MESSAGE_DISPLAY, Value: *data.MessageDisplay, }) } diff --git a/app/import_functions_test.go b/app/import_functions_test.go index c76915ac6..2e710a3df 100644 --- a/app/import_functions_test.go +++ b/app/import_functions_test.go @@ -1091,10 +1091,10 @@ func TestImportImportUser(t *testing.T) { } checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_THEME, "", *data.Theme) - checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, "use_military_time", *data.UseMilitaryTime) - checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, "collapse_previews", *data.CollapsePreviews) - checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, "message_display", *data.MessageDisplay) - checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, "channel_display_mode", *data.ChannelDisplayMode) + checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, model.PREFERENCE_NAME_USE_MILITARY_TIME, *data.UseMilitaryTime) + checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, model.PREFERENCE_NAME_COLLAPSE_SETTING, *data.CollapsePreviews) + checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, model.PREFERENCE_NAME_MESSAGE_DISPLAY, *data.MessageDisplay) + checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, model.PREFERENCE_NAME_CHANNEL_DISPLAY_MODE, *data.ChannelDisplayMode) checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_TUTORIAL_STEPS, user.Id, *data.TutorialStep) checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_ADVANCED_SETTINGS, "feature_enabled_markdown_preview", *data.UseMarkdownPreview) checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_ADVANCED_SETTINGS, "formatting", *data.UseFormatting) @@ -1117,10 +1117,10 @@ func TestImportImportUser(t *testing.T) { // Check their values again. checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_THEME, "", *data.Theme) - checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, "use_military_time", *data.UseMilitaryTime) - checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, "collapse_previews", *data.CollapsePreviews) - checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, "message_display", *data.MessageDisplay) - checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, "channel_display_mode", *data.ChannelDisplayMode) + checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, model.PREFERENCE_NAME_USE_MILITARY_TIME, *data.UseMilitaryTime) + checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, model.PREFERENCE_NAME_COLLAPSE_SETTING, *data.CollapsePreviews) + checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, model.PREFERENCE_NAME_MESSAGE_DISPLAY, *data.MessageDisplay) + checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, model.PREFERENCE_NAME_CHANNEL_DISPLAY_MODE, *data.ChannelDisplayMode) checkPreference(t, th.App, user.Id, model.PREFERENCE_CATEGORY_TUTORIAL_STEPS, user.Id, *data.TutorialStep) // Set Notify Props diff --git a/app/notification.go b/app/notification.go index 92b69039b..8092ba436 100644 --- a/app/notification.go +++ b/app/notification.go @@ -157,32 +157,11 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod updateMentionChans = append(updateMentionChans, a.Srv.Store.Channel().IncrementMentionCount(post.ChannelId, id)) } - var senderUsername string - senderName := "" - channelName := "" - if post.IsSystemMessage() { - senderName = utils.T("system.message.name") - } else { - if value, ok := post.Props["override_username"]; ok && post.Props["from_webhook"] == "true" && channel.Type != model.CHANNEL_DIRECT { - senderName = value.(string) - senderUsername = value.(string) - } else { - senderName = sender.Username - senderUsername = sender.Username - } - } - - if channel.Type == model.CHANNEL_GROUP { - userList := []*model.User{} - for _, u := range profileMap { - if u.Id != sender.Id { - userList = append(userList, u) - } - } - userList = append(userList, sender) - channelName = model.GetGroupDisplayNameFromUsers(userList, false) - } else { - channelName = channel.DisplayName + notification := &postNotification{ + post: post, + channel: channel, + profileMap: profileMap, + sender: sender, } if a.Config().EmailSettings.SendEmailNotifications { @@ -227,7 +206,7 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod autoResponderRelated := status.Status == model.STATUS_OUT_OF_OFFICE || post.Type == model.POST_AUTO_RESPONDER if userAllowsEmails && status.Status != model.STATUS_ONLINE && profileMap[id].DeleteAt == 0 && !autoResponderRelated { - a.sendNotificationEmail(post, profileMap[id], channel, team, channelName, senderName, sender) + a.sendNotificationEmail(notification, profileMap[id], team) } } } @@ -310,12 +289,8 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod } a.sendPushNotification( - post, + notification, profileMap[id], - channel, - channelName, - sender, - senderName, mentionedUserIds[id], (channelNotification || hereNotification || allNotification), replyToThreadType, @@ -337,12 +312,8 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod if ShouldSendPushNotification(profileMap[id], channelMemberNotifyPropsMap[id], false, status, post) { a.sendPushNotification( - post, + notification, profileMap[id], - channel, - channelName, - sender, - senderName, false, false, "", @@ -355,9 +326,9 @@ func (a *App) SendNotifications(post *model.Post, team *model.Team, channel *mod message := model.NewWebSocketEvent(model.WEBSOCKET_EVENT_POSTED, "", post.ChannelId, "", nil) message.Add("post", a.PostWithProxyAddedToImageURLs(post).ToJson()) message.Add("channel_type", channel.Type) - message.Add("channel_display_name", channelName) + message.Add("channel_display_name", notification.GetChannelName(model.SHOW_USERNAME, "")) message.Add("channel_name", channel.Name) - message.Add("sender_name", senderUsername) + message.Add("sender_name", notification.GetSenderName(model.SHOW_USERNAME, a.Config().ServiceSettings.EnablePostUsernameOverride)) message.Add("team_id", team.Id) if len(post.FileIds) != 0 && fchan != nil { @@ -628,3 +599,50 @@ func (a *App) GetMentionKeywordsInChannel(profiles map[string]*model.User, lookF return keywords } + +// Represents either an email or push notification and contains the fields required to send it to any user. +type postNotification struct { + channel *model.Channel + post *model.Post + profileMap map[string]*model.User + sender *model.User +} + +// Returns the name of the channel for this notification. For direct messages, this is the sender's name +// preceeded by an at sign. For group messages, this is a comma-separated list of the members of the +// channel, with an option to exclude the recipient of the message from that list. +func (n *postNotification) GetChannelName(userNameFormat string, excludeId string) string { + switch n.channel.Type { + case model.CHANNEL_DIRECT: + return fmt.Sprintf("@%s", n.sender.GetDisplayName(userNameFormat)) + case model.CHANNEL_GROUP: + names := []string{} + for _, user := range n.profileMap { + if user.Id != excludeId { + names = append(names, user.GetDisplayName(userNameFormat)) + } + } + + sort.Strings(names) + + return strings.Join(names, ", ") + default: + return n.channel.DisplayName + } +} + +// Returns the name of the sender of this notification, accounting for things like system messages +// and whether or not the username has been overridden by an integration. +func (n *postNotification) GetSenderName(userNameFormat string, overridesAllowed bool) string { + if n.post.IsSystemMessage() { + return utils.T("system.message.name") + } + + if overridesAllowed && n.channel.Type != model.CHANNEL_DIRECT { + if value, ok := n.post.Props["override_username"]; ok && n.post.Props["from_webhook"] == "true" { + return value.(string) + } + } + + return n.sender.GetDisplayName(userNameFormat) +} diff --git a/app/notification_email.go b/app/notification_email.go index b3835a4ee..cfc1bb4fd 100644 --- a/app/notification_email.go +++ b/app/notification_email.go @@ -17,7 +17,10 @@ import ( "github.com/nicksnyder/go-i18n/i18n" ) -func (a *App) sendNotificationEmail(post *model.Post, user *model.User, channel *model.Channel, team *model.Team, channelName string, senderName string, sender *model.User) *model.AppError { +func (a *App) sendNotificationEmail(notification *postNotification, user *model.User, team *model.Team) *model.AppError { + channel := notification.channel + post := notification.post + if channel.IsGroupOrDirect() { if result := <-a.Srv.Store.Team().GetTeamsByUserId(user.Id); result.Err != nil { return result.Err @@ -41,6 +44,7 @@ func (a *App) sendNotificationEmail(post *model.Post, user *model.User, channel } } } + if *a.Config().EmailSettings.EnableEmailBatching { var sendBatched bool if result := <-a.Srv.Store.Preference().Get(user.Id, model.PREFERENCE_CATEGORY_NOTIFICATIONS, model.PREFERENCE_NAME_EMAIL_INTERVAL); result.Err != nil { @@ -60,14 +64,25 @@ func (a *App) sendNotificationEmail(post *model.Post, user *model.User, channel // fall back to sending a single email if we can't batch it for some reason } - var useMilitaryTime bool translateFunc := utils.GetUserTranslations(user.Locale) - if result := <-a.Srv.Store.Preference().Get(user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, "use_military_time"); result.Err != nil { + + var useMilitaryTime bool + if result := <-a.Srv.Store.Preference().Get(user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, model.PREFERENCE_NAME_USE_MILITARY_TIME); result.Err != nil { useMilitaryTime = true } else { useMilitaryTime = result.Data.(model.Preference).Value == "true" } + var nameFormat string + if result := <-a.Srv.Store.Preference().Get(user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, model.PREFERENCE_NAME_NAME_FORMAT); result.Err != nil { + nameFormat = *a.Config().TeamSettings.TeammateNameDisplay + } else { + nameFormat = result.Data.(model.Preference).Value + } + + channelName := notification.GetChannelName(nameFormat, "") + senderName := notification.GetSenderName(nameFormat, a.Config().ServiceSettings.EnablePostUsernameOverride) + emailNotificationContentsType := model.EMAIL_NOTIFICATION_CONTENTS_FULL if license := a.License(); license != nil && *license.Features.EmailNotificationContents { emailNotificationContentsType = *a.Config().EmailSettings.EmailNotificationContentsType @@ -79,7 +94,7 @@ func (a *App) sendNotificationEmail(post *model.Post, user *model.User, channel } else if channel.Type == model.CHANNEL_GROUP { subjectText = getGroupMessageNotificationEmailSubject(user, post, translateFunc, a.Config().TeamSettings.SiteName, channelName, emailNotificationContentsType, useMilitaryTime) } else if *a.Config().EmailSettings.UseChannelInEmailNotifications { - subjectText = getNotificationEmailSubject(user, post, translateFunc, a.Config().TeamSettings.SiteName, team.DisplayName+" ("+channel.DisplayName+")", useMilitaryTime) + subjectText = getNotificationEmailSubject(user, post, translateFunc, a.Config().TeamSettings.SiteName, team.DisplayName+" ("+channelName+")", useMilitaryTime) } else { subjectText = getNotificationEmailSubject(user, post, translateFunc, a.Config().TeamSettings.SiteName, team.DisplayName, useMilitaryTime) } diff --git a/app/notification_push.go b/app/notification_push.go index 517988a97..5c7df0da7 100644 --- a/app/notification_push.go +++ b/app/notification_push.go @@ -15,13 +15,23 @@ import ( "github.com/nicksnyder/go-i18n/i18n" ) -func (a *App) sendPushNotification(post *model.Post, user *model.User, channel *model.Channel, channelName string, sender *model.User, senderName string, - explicitMention, channelWideMention bool, replyToThreadType string) *model.AppError { +func (a *App) sendPushNotification(notification *postNotification, user *model.User, explicitMention, channelWideMention bool, replyToThreadType string) *model.AppError { + channel := notification.channel + post := notification.post + cfg := a.Config() - contentsConfig := *cfg.EmailSettings.PushNotificationContents - teammateNameConfig := *cfg.TeamSettings.TeammateNameDisplay + + var nameFormat string + if result := <-a.Srv.Store.Preference().Get(user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, model.PREFERENCE_NAME_NAME_FORMAT); result.Err != nil { + nameFormat = *a.Config().TeamSettings.TeammateNameDisplay + } else { + nameFormat = result.Data.(model.Preference).Value + } + + channelName := notification.GetChannelName(nameFormat, user.Id) + senderName := notification.GetSenderName(nameFormat, cfg.ServiceSettings.EnablePostUsernameOverride) + sessions, err := a.getMobileAppSessions(user.Id) - sentBySystem := senderName == utils.T("system.message.name") if err != nil { return err } @@ -43,25 +53,13 @@ func (a *App) sendPushNotification(post *model.Post, user *model.User, channel * msg.RootId = post.RootId msg.SenderId = post.UserId - if !sentBySystem { - senderName = sender.GetDisplayName(teammateNameConfig) - preference, prefError := a.GetPreferenceByCategoryAndNameForUser(user.Id, model.PREFERENCE_CATEGORY_DISPLAY_SETTINGS, "name_format") - if prefError == nil && preference.Value != teammateNameConfig { - senderName = sender.GetDisplayName(preference.Value) - } - } - - if channel.Type == model.CHANNEL_DIRECT { - channelName = fmt.Sprintf("@%v", senderName) - } - + contentsConfig := *cfg.EmailSettings.PushNotificationContents if contentsConfig != model.GENERIC_NO_CHANNEL_NOTIFICATION || channel.Type == model.CHANNEL_DIRECT { msg.ChannelName = channelName } if ou, ok := post.Props["override_username"].(string); ok && cfg.ServiceSettings.EnablePostUsernameOverride { msg.OverrideUsername = ou - senderName = ou } if oi, ok := post.Props["override_icon_url"].(string); ok && cfg.ServiceSettings.EnablePostIconOverride { diff --git a/app/notification_test.go b/app/notification_test.go index bdc38e455..fa7bf7209 100644 --- a/app/notification_test.go +++ b/app/notification_test.go @@ -817,3 +817,165 @@ func TestGetMentionsEnabledFields(t *testing.T) { assert.EqualValues(t, 4, len(mentionEnabledFields)) assert.EqualValues(t, expectedFields, mentionEnabledFields) } + +func TestPostNotificationGetChannelName(t *testing.T) { + sender := &model.User{Id: model.NewId(), Username: "sender", FirstName: "Sender", LastName: "Sender", Nickname: "Sender"} + recipient := &model.User{Id: model.NewId(), Username: "recipient", FirstName: "Recipient", LastName: "Recipient", Nickname: "Recipient"} + otherUser := &model.User{Id: model.NewId(), Username: "other", FirstName: "Other", LastName: "Other", Nickname: "Other"} + profileMap := map[string]*model.User{ + sender.Id: sender, + recipient.Id: recipient, + otherUser.Id: otherUser, + } + + for name, testCase := range map[string]struct { + channel *model.Channel + nameFormat string + recipientId string + expected string + }{ + "regular channel": { + channel: &model.Channel{Type: model.CHANNEL_OPEN, Name: "channel", DisplayName: "My Channel"}, + expected: "My Channel", + }, + "direct channel, unspecified": { + channel: &model.Channel{Type: model.CHANNEL_DIRECT}, + expected: "@sender", + }, + "direct channel, username": { + channel: &model.Channel{Type: model.CHANNEL_DIRECT}, + nameFormat: model.SHOW_USERNAME, + expected: "@sender", + }, + "direct channel, full name": { + channel: &model.Channel{Type: model.CHANNEL_DIRECT}, + nameFormat: model.SHOW_FULLNAME, + expected: "@Sender Sender", + }, + "direct channel, nickname": { + channel: &model.Channel{Type: model.CHANNEL_DIRECT}, + nameFormat: model.SHOW_NICKNAME_FULLNAME, + expected: "@Sender", + }, + "group channel, unspecified": { + channel: &model.Channel{Type: model.CHANNEL_GROUP}, + expected: "other, sender", + }, + "group channel, username": { + channel: &model.Channel{Type: model.CHANNEL_GROUP}, + nameFormat: model.SHOW_USERNAME, + expected: "other, sender", + }, + "group channel, full name": { + channel: &model.Channel{Type: model.CHANNEL_GROUP}, + nameFormat: model.SHOW_FULLNAME, + expected: "Other Other, Sender Sender", + }, + "group channel, nickname": { + channel: &model.Channel{Type: model.CHANNEL_GROUP}, + nameFormat: model.SHOW_NICKNAME_FULLNAME, + expected: "Other, Sender", + }, + "group channel, not excluding current user": { + channel: &model.Channel{Type: model.CHANNEL_GROUP}, + nameFormat: model.SHOW_NICKNAME_FULLNAME, + expected: "Other, Sender", + recipientId: "", + }, + } { + t.Run(name, func(t *testing.T) { + notification := &postNotification{ + channel: testCase.channel, + sender: sender, + profileMap: profileMap, + } + + recipientId := recipient.Id + if testCase.recipientId != "" { + recipientId = testCase.recipientId + } + + assert.Equal(t, testCase.expected, notification.GetChannelName(testCase.nameFormat, recipientId)) + }) + } +} + +func TestPostNotificationGetSenderName(t *testing.T) { + th := Setup() + defer th.TearDown() + + defaultChannel := &model.Channel{Type: model.CHANNEL_OPEN} + defaultPost := &model.Post{Props: model.StringInterface{}} + sender := &model.User{Id: model.NewId(), Username: "sender", FirstName: "Sender", LastName: "Sender", Nickname: "Sender"} + + overriddenPost := &model.Post{ + Props: model.StringInterface{ + "override_username": "Overridden", + "from_webhook": "true", + }, + } + + for name, testCase := range map[string]struct { + channel *model.Channel + post *model.Post + nameFormat string + allowOverrides bool + expected string + }{ + "name format unspecified": { + expected: sender.Username, + }, + "name format username": { + nameFormat: model.SHOW_USERNAME, + expected: sender.Username, + }, + "name format full name": { + nameFormat: model.SHOW_FULLNAME, + expected: sender.FirstName + " " + sender.LastName, + }, + "name format nickname": { + nameFormat: model.SHOW_NICKNAME_FULLNAME, + expected: sender.Nickname, + }, + "system message": { + post: &model.Post{Type: model.POST_SYSTEM_MESSAGE_PREFIX + "custom"}, + expected: utils.T("system.message.name"), + }, + "overridden username": { + post: overriddenPost, + allowOverrides: true, + expected: overriddenPost.Props["override_username"].(string), + }, + "overridden username, direct channel": { + channel: &model.Channel{Type: model.CHANNEL_DIRECT}, + post: overriddenPost, + allowOverrides: true, + expected: sender.Username, + }, + "overridden username, overrides disabled": { + post: overriddenPost, + allowOverrides: false, + expected: sender.Username, + }, + } { + t.Run(name, func(t *testing.T) { + channel := defaultChannel + if testCase.channel != nil { + channel = testCase.channel + } + + post := defaultPost + if testCase.post != nil { + post = testCase.post + } + + notification := &postNotification{ + channel: channel, + post: post, + sender: sender, + } + + assert.Equal(t, testCase.expected, notification.GetSenderName(testCase.nameFormat, testCase.allowOverrides)) + }) + } +} diff --git a/model/preference.go b/model/preference.go index c7639db96..6f13c38e5 100644 --- a/model/preference.go +++ b/model/preference.go @@ -21,7 +21,11 @@ const ( PREFERENCE_CATEGORY_SIDEBAR_SETTINGS = "sidebar_settings" PREFERENCE_CATEGORY_DISPLAY_SETTINGS = "display_settings" + PREFERENCE_NAME_CHANNEL_DISPLAY_MODE = "channel_display_mode" PREFERENCE_NAME_COLLAPSE_SETTING = "collapse_previews" + PREFERENCE_NAME_MESSAGE_DISPLAY = "message_display" + PREFERENCE_NAME_NAME_FORMAT = "name_format" + PREFERENCE_NAME_USE_MILITARY_TIME = "use_military_time" PREFERENCE_CATEGORY_THEME = "theme" // the name for theme props is the team id -- cgit v1.2.3-1-g7c22