From 8589476229c48dbb7ec15b6ee3164c483ce96d0b Mon Sep 17 00:00:00 2001 From: Devin Binnie <52460000+devinbinnie@users.noreply.github.com> Date: Thu, 11 Apr 2024 16:18:14 -0400 Subject: [PATCH] [MM-57067][MM-57006][MM-57328] Acknowledge websocket POSTED events that may result in a notification on the web client (#26604) * Acknowledge POSTED events that may result in a notification * Remove temporary metrics * Add Desktop App support * Merge'd * Add some tests * PR feedback * Rework window is focused check * Oops * Move mentions/followers ACK to posted ACK broadcast hook --------- Co-authored-by: Mattermost Build --- server/channels/app/notification.go | 29 ++++++++ server/channels/app/notification_test.go | 13 ++++ server/channels/app/web_broadcast_hooks.go | 53 ++++++++++++++ .../channels/app/web_broadcast_hooks_test.go | 56 +++++++++++++++ server/channels/wsapi/system.go | 17 +++++ server/public/model/websocket_message.go | 1 + webapp/channels/src/actions/new_post.test.ts | 2 +- webapp/channels/src/actions/new_post.ts | 12 +++- .../src/actions/notification_actions.jsx | 72 +++++++++++++------ .../channels/src/components/login/login.tsx | 2 +- webapp/channels/src/utils/desktop_api.ts | 7 +- webapp/channels/src/utils/notifications.ts | 11 +-- webapp/platform/client/src/websocket.ts | 12 ++++ 13 files changed, 255 insertions(+), 32 deletions(-) diff --git a/server/channels/app/notification.go b/server/channels/app/notification.go index 423ad706609..6f79f1e62d2 100644 --- a/server/channels/app/notification.go +++ b/server/channels/app/notification.go @@ -676,6 +676,18 @@ func (a *App) SendNotifications(c request.CTX, post *model.Post, team *model.Tea useAddFollowersHook(message, notificationsForCRT.Desktop) } + // Collect user IDs of whom we want to acknowledge the websocket event for notification metrics + usersToAck := []string{} + for id, profile := range profileMap { + userNotificationLevel := profile.NotifyProps[model.DesktopNotifyProp] + channelNotificationLevel := channelMemberNotifyPropsMap[id][model.DesktopNotifyProp] + + if ShouldAckWebsocketNotification(channel.Type, userNotificationLevel, channelNotificationLevel) { + usersToAck = append(usersToAck, id) + } + } + usePostedAckHook(message, post.UserId, channel.Type, usersToAck) + published, err := a.publishWebsocketEventForPermalinkPost(c, post, message) if err != nil { a.NotificationsLog().Error("Couldn't send websocket notification for permalink post", @@ -1680,3 +1692,20 @@ func shouldChannelMemberNotifyCRT(userNotifyProps model.StringMap, channelMember return } + +func ShouldAckWebsocketNotification(channelType model.ChannelType, userNotificationLevel, channelNotificationLevel string) bool { + if channelNotificationLevel == model.ChannelNotifyAll { + // Should ACK on if we notify for all messages in the channel + return true + } else if channelNotificationLevel == model.ChannelNotifyDefault && userNotificationLevel == model.UserNotifyAll { + // Should ACK on if we notify for all messages and the channel settings are unchanged + return true + } else if channelType == model.ChannelTypeGroup && + ((channelNotificationLevel == model.ChannelNotifyDefault && userNotificationLevel == model.UserNotifyMention) || + channelNotificationLevel == model.ChannelNotifyMention) { + // Should ACK for group channels where default settings are in place (should be notified) + return true + } + + return false +} diff --git a/server/channels/app/notification_test.go b/server/channels/app/notification_test.go index 623613fec4f..f126d7947c6 100644 --- a/server/channels/app/notification_test.go +++ b/server/channels/app/notification_test.go @@ -3049,3 +3049,16 @@ func TestRemoveNotifications(t *testing.T) { require.Equal(t, int64(0), thread.UnreadReplies) }) } + +func TestShouldAckWebsocketNotification(t *testing.T) { + t.Run("should return true if channel notify level is ALL", func(t *testing.T) { + assert.True(t, ShouldAckWebsocketNotification(model.ChannelTypeOpen, model.UserNotifyNone, model.ChannelNotifyAll)) + }) + t.Run("should return true if user notify level is ALL and the channel is unchanged", func(t *testing.T) { + assert.True(t, ShouldAckWebsocketNotification(model.ChannelTypeOpen, model.UserNotifyAll, model.ChannelNotifyDefault)) + }) + t.Run("should return true if its a group channel, and the level is mention", func(t *testing.T) { + assert.True(t, ShouldAckWebsocketNotification(model.ChannelTypeGroup, model.UserNotifyMention, model.ChannelNotifyDefault)) + assert.True(t, ShouldAckWebsocketNotification(model.ChannelTypeGroup, model.UserNotifyNone, model.ChannelNotifyMention)) + }) +} diff --git a/server/channels/app/web_broadcast_hooks.go b/server/channels/app/web_broadcast_hooks.go index 46c4e1c8d39..944ebc93c08 100644 --- a/server/channels/app/web_broadcast_hooks.go +++ b/server/channels/app/web_broadcast_hooks.go @@ -16,12 +16,14 @@ import ( const ( broadcastAddMentions = "add_mentions" broadcastAddFollowers = "add_followers" + broadcastPostedAck = "posted_ack" ) func (s *Server) makeBroadcastHooks() map[string]platform.BroadcastHook { return map[string]platform.BroadcastHook{ broadcastAddMentions: &addMentionsBroadcastHook{}, broadcastAddFollowers: &addFollowersBroadcastHook{}, + broadcastPostedAck: &postedAckBroadcastHook{}, } } @@ -69,6 +71,57 @@ func useAddFollowersHook(message *model.WebSocketEvent, followers model.StringAr }) } +type postedAckBroadcastHook struct{} + +func usePostedAckHook(message *model.WebSocketEvent, postedUserId string, channelType model.ChannelType, usersToNotify []string) { + message.GetBroadcast().AddHook(broadcastPostedAck, map[string]any{ + "posted_user_id": postedUserId, + "channel_type": channelType, + "users": usersToNotify, + }) +} + +func (h *postedAckBroadcastHook) Process(msg *platform.HookedWebSocketEvent, webConn *platform.WebConn, args map[string]any) error { + // Add if we have mentions or followers + // This works since we currently do have an order for broadcast hooks, but this probably should be reworked going forward + if msg.Get("followers") != nil || msg.Get("mentions") != nil { + msg.Add("should_ack", true) + return nil + } + + postedUserId, err := getTypedArg[string](args, "posted_user_id") + if err != nil { + return errors.Wrap(err, "Invalid posted_user_id value passed to postedAckBroadcastHook") + } + + // Don't ACK your own posts + if postedUserId == webConn.UserId { + return nil + } + + channelType, err := getTypedArg[model.ChannelType](args, "channel_type") + if err != nil { + return errors.Wrap(err, "Invalid channel_type value passed to postedAckBroadcastHook") + } + + // Always ACK direct channels + if channelType == model.ChannelTypeDirect { + msg.Add("should_ack", true) + return nil + } + + users, err := getTypedArg[model.StringArray](args, "users") + if err != nil { + return errors.Wrap(err, "Invalid users value passed to addFollowersBroadcastHook") + } + + if len(users) > 0 && slices.Contains(users, webConn.UserId) { + msg.Add("should_ack", true) + } + + return nil +} + // getTypedArg returns a correctly typed hook argument with the given key, reinterpreting the type using JSON encoding // if necessary. This is needed because broadcast hook args are JSON encoded in a multi-server environment, and any // type information is lost because those types aren't known at decode time. diff --git a/server/channels/app/web_broadcast_hooks_test.go b/server/channels/app/web_broadcast_hooks_test.go index a4c88d8cda1..cbfa71963c4 100644 --- a/server/channels/app/web_broadcast_hooks_test.go +++ b/server/channels/app/web_broadcast_hooks_test.go @@ -83,6 +83,62 @@ func TestAddFollowersHook_Process(t *testing.T) { }) } +func TestPostedAckHook_Process(t *testing.T) { + hook := &postedAckBroadcastHook{} + userID := model.NewId() + webConn := &platform.WebConn{ + UserId: userID, + } + + t.Run("should ack if user is in the list of users to notify", func(t *testing.T) { + msg := platform.MakeHookedWebSocketEvent(model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, "")) + + hook.Process(msg, webConn, map[string]any{ + "posted_user_id": model.NewId(), + "channel_type": model.ChannelTypeOpen, + "users": []string{userID}, + }) + + assert.True(t, msg.Event().GetData()["should_ack"].(bool)) + }) + + t.Run("should not ack if user is not in the list of users to notify", func(t *testing.T) { + msg := platform.MakeHookedWebSocketEvent(model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, "")) + + hook.Process(msg, webConn, map[string]any{ + "posted_user_id": model.NewId(), + "channel_type": model.ChannelTypeOpen, + "users": []string{}, + }) + + assert.Nil(t, msg.Event().GetData()["should_ack"]) + }) + + t.Run("should not ack if you are the user who posted", func(t *testing.T) { + msg := platform.MakeHookedWebSocketEvent(model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, "")) + + hook.Process(msg, webConn, map[string]any{ + "posted_user_id": userID, + "channel_type": model.ChannelTypeOpen, + "users": []string{userID}, + }) + + assert.Nil(t, msg.Event().GetData()["should_ack"]) + }) + + t.Run("should ack if the channel is a DM", func(t *testing.T) { + msg := platform.MakeHookedWebSocketEvent(model.NewWebSocketEvent(model.WebsocketEventPosted, "", "", "", nil, "")) + + hook.Process(msg, webConn, map[string]any{ + "posted_user_id": model.NewId(), + "channel_type": model.ChannelTypeDirect, + "users": []string{}, + }) + + assert.True(t, msg.Event().GetData()["should_ack"].(bool)) + }) +} + func TestAddMentionsAndAddFollowersHooks(t *testing.T) { addMentionsHook := &addMentionsBroadcastHook{} addFollowersHook := &addFollowersBroadcastHook{} diff --git a/server/channels/wsapi/system.go b/server/channels/wsapi/system.go index c2e1f5ffd12..8efb1923bb6 100644 --- a/server/channels/wsapi/system.go +++ b/server/channels/wsapi/system.go @@ -5,10 +5,12 @@ package wsapi import ( "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/mlog" ) func (api *API) InitSystem() { api.Router.Handle("ping", api.APIWebSocketHandler(ping)) + api.Router.Handle(model.WebsocketPostedNotifyAck, api.APIWebSocketHandler(api.websocketNotificationAck)) } func ping(req *model.WebSocketRequest) (map[string]any, *model.AppError) { @@ -20,3 +22,18 @@ func ping(req *model.WebSocketRequest) (map[string]any, *model.AppError) { return data, nil } + +func (api *API) websocketNotificationAck(req *model.WebSocketRequest) (map[string]any, *model.AppError) { + // Log the ACKs if necessary + api.App.NotificationsLog().Debug("Websocket notification acknowledgment", + mlog.String("type", model.TypeWebsocket), + mlog.String("user_id", req.Session.UserId), + mlog.Any("user_agent", req.Data["user_agent"]), + mlog.Any("post_id", req.Data["post_id"]), + mlog.Any("result", req.Data["result"]), + mlog.Any("reason", req.Data["reason"]), + mlog.Any("data", req.Data["data"]), + ) + + return nil, nil +} diff --git a/server/public/model/websocket_message.go b/server/public/model/websocket_message.go index 51278b11607..507fc5c77fc 100644 --- a/server/public/model/websocket_message.go +++ b/server/public/model/websocket_message.go @@ -92,6 +92,7 @@ const ( WebsocketEventChannelBookmarkDeleted = "channel_bookmark_deleted" WebsocketEventChannelBookmarkSorted = "channel_bookmark_sorted" WebsocketPresenceIndicator WebsocketEventType = "presence" + WebsocketPostedNotifyAck = "posted_notify_ack" ) type WebSocketMessage interface { diff --git a/webapp/channels/src/actions/new_post.test.ts b/webapp/channels/src/actions/new_post.test.ts index 9bfb30f69bf..24c1a008502 100644 --- a/webapp/channels/src/actions/new_post.test.ts +++ b/webapp/channels/src/actions/new_post.test.ts @@ -96,7 +96,7 @@ describe('actions/new_post', () => { test('completePostReceive', async () => { const testStore = mockStore(initialState); const newPost = {id: 'new_post_id', channel_id: 'current_channel_id', message: 'new message', type: Constants.PostTypes.ADD_TO_CHANNEL, user_id: 'some_user_id', create_at: POST_CREATED_TIME, props: {addedUserId: 'other_user_id'}} as unknown as Post; - const websocketProps = {team_id: 'team_id', mentions: ['current_user_id']}; + const websocketProps = {team_id: 'team_id', mentions: ['current_user_id'], should_ack: false}; await testStore.dispatch(NewPostActions.completePostReceive(newPost, websocketProps)); expect(testStore.getActions()).toEqual([ diff --git a/webapp/channels/src/actions/new_post.ts b/webapp/channels/src/actions/new_post.ts index bdbc1a1cd24..4ce59a6bd01 100644 --- a/webapp/channels/src/actions/new_post.ts +++ b/webapp/channels/src/actions/new_post.ts @@ -28,6 +28,7 @@ import {sendDesktopNotification} from 'actions/notification_actions.jsx'; import {updateThreadLastOpened} from 'actions/views/threads'; import {isThreadOpen, makeGetThreadLastViewedAt} from 'selectors/views/threads'; +import WebSocketClient from 'client/web_websocket_client'; import {ActionTypes} from 'utils/constants'; import type {GlobalState} from 'types/store'; @@ -35,6 +36,7 @@ import type {GlobalState} from 'types/store'; export type NewPostMessageProps = { mentions: string[]; team_id: string; + should_ack: boolean; } export function completePostReceive(post: Post, websocketMessageProps: NewPostMessageProps, fetchedChannelMember?: boolean): ActionFuncAsync { @@ -65,7 +67,8 @@ export function completePostReceive(post: Post, websocketMessageProps: NewPostMe PostActions.receivedNewPost(post, collapsedThreadsEnabled), ); - const isCRTReplyByCurrentUser = isCRTReply && post.user_id === getCurrentUserId(state); + const currentUserId = getCurrentUserId(state); + const isCRTReplyByCurrentUser = isCRTReply && post.user_id === currentUserId; if (!isCRTReplyByCurrentUser) { actions.push( ...setChannelReadAndViewed(dispatch, getState, post as Post, websocketMessageProps, fetchedChannelMember), @@ -77,7 +80,12 @@ export function completePostReceive(post: Post, websocketMessageProps: NewPostMe dispatch(setThreadRead(post)); } - dispatch(sendDesktopNotification(post, websocketMessageProps)); + const {result, reason, data} = await dispatch(sendDesktopNotification(post, websocketMessageProps)); + + // Only ACK for posts that require it + if (websocketMessageProps.should_ack) { + WebSocketClient.acknowledgePostedNotification(post.id, result, reason, data); + } return {data: true}; }; diff --git a/webapp/channels/src/actions/notification_actions.jsx b/webapp/channels/src/actions/notification_actions.jsx index 18718e63164..e6abadf0e33 100644 --- a/webapp/channels/src/actions/notification_actions.jsx +++ b/webapp/channels/src/actions/notification_actions.jsx @@ -49,7 +49,7 @@ const getNotificationSoundFromChannelMemberAndUser = (member, user) => { }; /** - * @returns {import('mattermost-redux/types/actions').ThunkActionFunc} + * @returns {import('mattermost-redux/types/actions').ThunkActionFunc, GlobalState>} */ export function sendDesktopNotification(post, msgProps) { return async (dispatch, getState) => { @@ -57,11 +57,11 @@ export function sendDesktopNotification(post, msgProps) { const currentUserId = getCurrentUserId(state); if ((currentUserId === post.user_id && post.props.from_webhook !== 'true')) { - return; + return {result: 'not_sent', reason: 'own_post'}; } if (isSystemMessage(post) && !isUserAddedInChannel(post, currentUserId)) { - return; + return {result: 'not_sent', reason: 'system_message'}; } let userFromPost = getUser(state, post.user_id); @@ -91,8 +91,16 @@ export function sendDesktopNotification(post, msgProps) { const member = getMyChannelMember(state, post.channel_id); const isCrtReply = isCollapsedThreadsEnabled(state) && post.root_id !== ''; - if (!member || isChannelMuted(member) || userStatus === UserStatuses.DND || userStatus === UserStatuses.OUT_OF_OFFICE) { - return; + if (!member) { + return {result: 'not_sent', reason: 'no_member'}; + } + + if (isChannelMuted(member)) { + return {result: 'not_sent', reason: 'channel_muted'}; + } + + if (userStatus === UserStatuses.DND || userStatus === UserStatuses.OUT_OF_OFFICE) { + return {result: 'not_sent', reason: 'user_status', data: userStatus}; } const channelNotifyProp = member?.notify_props?.desktop || NotificationLevels.DEFAULT; @@ -107,7 +115,7 @@ export function sendDesktopNotification(post, msgProps) { } if (notifyLevel === NotificationLevels.NONE) { - return; + return {result: 'not_sent', reason: 'notify_level', data: notifyLevel}; } else if (channel?.type === 'G' && notifyLevel === NotificationLevels.MENTION) { // Compose the whole text in the message, including interactive messages. let text = post.message; @@ -181,13 +189,13 @@ export function sendDesktopNotification(post, msgProps) { } if (!isExplicitlyMentioned) { - return; + return {result: 'not_sent', reason: 'not_explicitly_mentioned', data: mentionableText}; } } else if (notifyLevel === NotificationLevels.MENTION && mentions.indexOf(user.id) === -1 && msgProps.channel_type !== Constants.DM_CHANNEL) { - return; + return {result: 'not_sent', reason: 'not_mentioned'}; } else if (isCrtReply && notifyLevel === NotificationLevels.ALL && followers.indexOf(currentUserId) === -1) { // if user is not following the thread don't notify - return; + return {result: 'not_sent', reason: 'not_following_thread'}; } const config = getConfig(state); @@ -265,12 +273,23 @@ export function sendDesktopNotification(post, msgProps) { const channelId = channel ? channel.id : null; let notify = false; - if (isCrtReply) { - notify = !isThreadOpen(state, post.root_id); + let notifyResult = {result: 'not_sent', reason: 'unknown'}; + if (state.views.browser.focused) { + notifyResult = {result: 'not_sent', reason: 'window_is_focused'}; + if (isCrtReply) { + notify = !isThreadOpen(state, post.root_id); + if (!notify) { + notifyResult = {result: 'not_sent', reason: 'thread_is_open', data: post.root_id}; + } + } else { + notify = activeChannel && activeChannel.id !== channelId; + if (!notify) { + notifyResult = {result: 'not_sent', reason: 'channel_is_open', data: activeChannel?.id}; + } + } } else { - notify = activeChannel && activeChannel.id !== channelId; + notify = true; } - notify = notify || !state.views.browser.focused; let soundName = getNotificationSoundFromChannelMemberAndUser(member, user); @@ -286,29 +305,39 @@ export function sendDesktopNotification(post, msgProps) { const hookResult = await dispatch(runDesktopNotificationHooks(post, msgProps, channel, teamId, args)); if (hookResult.error) { dispatch(logError(hookResult.error)); - return; + return {result: 'error', reason: 'desktop_notification_hook', data: String(hookResult.error)}; } let silent = false; ({title, body, silent, soundName, url, notify} = hookResult.args); if (notify) { - dispatch(notifyMe(title, body, channel, teamId, silent, soundName, url)); + const result = dispatch(notifyMe(title, body, channel, teamId, silent, soundName, url)); //Don't add extra sounds on native desktop clients if (sound && !isDesktopApp() && !isMobileApp()) { NotificationSounds.ding(soundName); } + + return result; + } + + if (args.notify && !notify) { + notifyResult = {result: 'not_sent', reason: 'desktop_notification_hook', data: String(hookResult)}; } + + return notifyResult; }; } -export const notifyMe = (title, body, channel, teamId, silent, soundName, url) => (dispatch) => { +export const notifyMe = (title, body, channel, teamId, silent, soundName, url) => async (dispatch) => { // handle notifications in desktop app if (isDesktopApp()) { - DesktopApp.dispatchNotification(title, body, channel.id, teamId, silent, soundName, url); - } else { - showNotification({ + return DesktopApp.dispatchNotification(title, body, channel.id, teamId, silent, soundName, url); + } + + try { + return await showNotification({ title, body, requireInteraction: false, @@ -317,8 +346,9 @@ export const notifyMe = (title, body, channel, teamId, silent, soundName, url) = window.focus(); getHistory().push(url); }, - }).catch((error) => { - dispatch(logError(error)); }); + } catch (error) { + dispatch(logError(error)); + return {result: 'error', reason: 'notification_api', data: String(error)}; } }; diff --git a/webapp/channels/src/components/login/login.tsx b/webapp/channels/src/components/login/login.tsx index af7cc9aa561..0ad7c8e8dd6 100644 --- a/webapp/channels/src/components/login/login.tsx +++ b/webapp/channels/src/components/login/login.tsx @@ -267,7 +267,7 @@ const Login = ({onCustomizeHeader}: LoginProps) => { closeSessionExpiredNotification.current = undefined; } }, - }).then((closeNotification) => { + }).then(({callback: closeNotification}) => { closeSessionExpiredNotification.current = closeNotification; }).catch(() => { // Ignore the failure to display the notification. diff --git a/webapp/channels/src/utils/desktop_api.ts b/webapp/channels/src/utils/desktop_api.ts index ad692a87d42..5ef2c477773 100644 --- a/webapp/channels/src/utils/desktop_api.ts +++ b/webapp/channels/src/utils/desktop_api.ts @@ -156,7 +156,7 @@ class DesktopAppAPI { * One-ways */ - dispatchNotification = ( + dispatchNotification = async ( title: string, body: string, channelId: string, @@ -166,8 +166,8 @@ class DesktopAppAPI { url: string, ) => { if (window.desktopAPI?.sendNotification) { - window.desktopAPI.sendNotification(title, body, channelId, teamId, url, silent, soundName); - return; + const result = await window.desktopAPI.sendNotification(title, body, channelId, teamId, url, silent, soundName); + return result ?? {result: 'unsupported', reason: 'desktop_app_unsupported'}; } // get the desktop app to trigger the notification @@ -186,6 +186,7 @@ class DesktopAppAPI { }, window.location.origin, ); + return {result: 'unsupported', reason: 'desktop_app_unsupported'}; }; doBrowserHistoryPush = (path: string) => { diff --git a/webapp/channels/src/utils/notifications.ts b/webapp/channels/src/utils/notifications.ts index a06aceb82af..7373bae0d0d 100644 --- a/webapp/channels/src/utils/notifications.ts +++ b/webapp/channels/src/utils/notifications.ts @@ -53,7 +53,7 @@ export async function showNotification( if (Notification.permission !== 'granted' && requestedNotificationPermission) { // User didn't allow notifications - return () => {}; + return {result: 'not_sent', reason: 'notifications_permission_previously_denied', data: Notification.permission, callback: () => {}}; } requestedNotificationPermission = true; @@ -68,7 +68,7 @@ export async function showNotification( if (permission !== 'granted') { // User has denied notification for the site - return () => {}; + return {result: 'not_sent', reason: 'notifications_permission_denied', data: permission, callback: () => {}}; } const notification = new Notification(title, { @@ -94,7 +94,10 @@ export async function showNotification( }, Constants.DEFAULT_NOTIFICATION_DURATION); } - return () => { - notification.close(); + return { + result: 'success', + callback: () => { + notification.close(); + }, }; } diff --git a/webapp/platform/client/src/websocket.ts b/webapp/platform/client/src/websocket.ts index cf4fd3f6088..237c1d6c56b 100644 --- a/webapp/platform/client/src/websocket.ts +++ b/webapp/platform/client/src/websocket.ts @@ -412,6 +412,18 @@ export default class WebSocketClient { this.sendMessage('user_update_active_status', data, callback); } + acknowledgePostedNotification(postId: string, result: 'error' | 'not_sent' | 'unsupported' | 'success', reason?: string, postedData?: string) { + const data = { + post_id: postId, + user_agent: window.navigator.userAgent, + result, + reason, + data: postedData, + }; + + this.sendMessage('posted_notify_ack', data); + } + getStatuses(callback?: () => void) { this.sendMessage('get_statuses', null, callback); }