[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 <build@mattermost.com>

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
+}

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))
+	})
+}

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.

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{}

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
+}

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 {

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([

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<boolean, GlobalState> {
@@ -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};
     };

webapp/channels/src/actions/notification_actions.jsx

@@ -49,7 +49,7 @@ const getNotificationSoundFromChannelMemberAndUser = (member, user) => {
 };
 
 /**
- * @returns {import('mattermost-redux/types/actions').ThunkActionFunc<void>}
+ * @returns {import('mattermost-redux/types/actions').ThunkActionFunc<Promise<NotificationResult>, 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)};
     }
 };

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.

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) => {

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();
+        },
     };
 }

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);
     }