[MM-57067][MM-57006][MM-57328] Acknowledge websocket POSTED events th…

…at may result in a notification on the web client (mattermost#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 <[email protected]>

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,13 +28,15 @@ 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';
 
 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};
     };