From f92e9a6cc50bf0971ec7bf666ed54393932960d5 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 10 Sep 2025 16:47:35 +0200 Subject: [PATCH 1/3] fix(threads): add threadId to objectId for notifications Signed-off-by: Anna Larch --- lib/Chat/ChatManager.php | 8 +++---- lib/Chat/Notifier.php | 24 ++++++++++++------- lib/Notification/Notifier.php | 7 +++++- .../features/bootstrap/FeatureContext.php | 15 +++++++----- .../features/chat-4/threads.feature | 16 +++++++++---- 5 files changed, 45 insertions(+), 25 deletions(-) diff --git a/lib/Chat/ChatManager.php b/lib/Chat/ChatManager.php index 9c0543e6c12..9df43cdf361 100644 --- a/lib/Chat/ChatManager.php +++ b/lib/Chat/ChatManager.php @@ -220,7 +220,7 @@ public function addSystemMessage( $federatedUsersDirectlyMentioned = $this->notifier->getMentionedCloudIds($captionComment); } if ($replyTo instanceof IComment) { - $alreadyNotifiedUsers = $this->notifier->notifyReplyToAuthor($chat, $comment, $replyTo, $silent); + $alreadyNotifiedUsers = $this->notifier->notifyReplyToAuthor($chat, $comment, $replyTo, $silent, $threadId); if ($replyTo->getActorType() === Attendee::ACTOR_USERS) { $usersDirectlyMentioned[] = $replyTo->getActorId(); } elseif ($replyTo->getActorType() === Attendee::ACTOR_FEDERATED_USERS) { @@ -229,7 +229,7 @@ public function addSystemMessage( } } - $alreadyNotifiedUsers = $this->notifier->notifyMentionedUsers($chat, $captionComment ?? $comment, $alreadyNotifiedUsers, $silent); + $alreadyNotifiedUsers = $this->notifier->notifyMentionedUsers($chat, $captionComment ?? $comment, $alreadyNotifiedUsers, $silent, threadId: $threadId); if (!empty($alreadyNotifiedUsers)) { $userIds = array_column($alreadyNotifiedUsers, 'id'); $this->participantService->markUsersAsMentioned($chat, Attendee::ACTOR_USERS, $userIds, (int)$comment->getId(), $usersDirectlyMentioned); @@ -454,7 +454,7 @@ public function sendMessage( $usersDirectlyMentioned = $this->notifier->getMentionedUserIds($comment); $federatedUsersDirectlyMentioned = $this->notifier->getMentionedCloudIds($comment); if ($replyTo instanceof IComment) { - $alreadyNotifiedUsers = $this->notifier->notifyReplyToAuthor($chat, $comment, $replyTo, $silent); + $alreadyNotifiedUsers = $this->notifier->notifyReplyToAuthor($chat, $comment, $replyTo, $silent, $threadId); if ($replyTo->getActorType() === Attendee::ACTOR_USERS) { $usersDirectlyMentioned[] = $replyTo->getActorId(); } elseif ($replyTo->getActorType() === Attendee::ACTOR_FEDERATED_USERS) { @@ -462,7 +462,7 @@ public function sendMessage( } } - $alreadyNotifiedUsers = $this->notifier->notifyMentionedUsers($chat, $comment, $alreadyNotifiedUsers, $silent, $participant); + $alreadyNotifiedUsers = $this->notifier->notifyMentionedUsers($chat, $comment, $alreadyNotifiedUsers, $silent, $participant, threadId: $threadId); if (!empty($alreadyNotifiedUsers)) { $userIds = array_column($alreadyNotifiedUsers, 'id'); $this->participantService->markUsersAsMentioned($chat, Attendee::ACTOR_USERS, $userIds, (int)$comment->getId(), $usersDirectlyMentioned); diff --git a/lib/Chat/Notifier.php b/lib/Chat/Notifier.php index a79420b33da..19453622dbf 100644 --- a/lib/Chat/Notifier.php +++ b/lib/Chat/Notifier.php @@ -69,7 +69,7 @@ public function __construct( * @return string[] Users that were mentioned * @psalm-return array */ - public function notifyMentionedUsers(Room $chat, IComment $comment, array $alreadyNotifiedUsers, bool $silent, ?Participant $participant = null): array { + public function notifyMentionedUsers(Room $chat, IComment $comment, array $alreadyNotifiedUsers, bool $silent, ?Participant $participant = null, ?int $threadId = null): array { $usersToNotify = $this->getUsersToNotify($chat, $comment, $alreadyNotifiedUsers, $participant); if (!$usersToNotify) { @@ -78,7 +78,7 @@ public function notifyMentionedUsers(Room $chat, IComment $comment, array $alrea $shouldFlush = false; if (!$silent) { - $notification = $this->createNotification($chat, $comment, 'mention'); + $notification = $this->createNotification($chat, $comment, 'mention', threadId: $threadId); $parameters = $notification->getSubjectParameters(); $shouldFlush = $this->notificationManager->defer(); } @@ -203,7 +203,7 @@ private function addMentionAllToList(Room $chat, array $list, ?Participant $part * @return array[] Actor that was replied to * @psalm-return array */ - public function notifyReplyToAuthor(Room $chat, IComment $comment, IComment $replyTo, bool $silent): array { + public function notifyReplyToAuthor(Room $chat, IComment $comment, IComment $replyTo, bool $silent, ?int $threadId = null): array { if ($replyTo->getActorType() !== Attendee::ACTOR_USERS && $replyTo->getActorType() !== Attendee::ACTOR_FEDERATED_USERS) { // No reply notification when the replyTo-author was not a user or federated user return []; @@ -225,7 +225,7 @@ public function notifyReplyToAuthor(Room $chat, IComment $comment, IComment $rep } if (!$silent) { - $notification = $this->createNotification($chat, $comment, 'reply'); + $notification = $this->createNotification($chat, $comment, 'reply', threadId: $threadId); $notification->setUser($replyTo->getActorId()); $notification->setPriorityNotification($shouldMentionedUserBeNotified === self::PRIORITY_IMPORTANT); $this->notificationManager->notify($notification); @@ -283,7 +283,7 @@ public function notifyOtherParticipant(Room $chat, IComment $comment, array $alr } } - $notification = $this->createNotification($chat, $comment, 'chat'); + $notification = $this->createNotification($chat, $comment, 'chat', threadId: $threadId); foreach ($participants as $participant) { $attendeeId = $participant->getAttendee()->getId(); $shouldParticipantBeNotified = $this->shouldParticipantBeNotified($participant, $comment, $alreadyNotifiedUsers); @@ -634,18 +634,24 @@ private function getMentionedTeamMembers(Room $chat, IComment $comment, array $l * Creates a notification for the given chat message comment and mentioned * user ID. */ - private function createNotification(Room $chat, IComment $comment, string $subject, array $subjectData = [], ?IComment $reaction = null): INotification { + private function createNotification(Room $chat, IComment $comment, string $subject, array $subjectData = [], ?IComment $reaction = null, ?int $threadId = null): INotification { $subjectData['userType'] = $reaction ? $reaction->getActorType() : $comment->getActorType(); $subjectData['userId'] = $reaction ? $reaction->getActorId() : $comment->getActorId(); + $messageData = [ + 'commentId' => $comment->getId(), + ]; + + if ($threadId !== null && $threadId !== 0) { + $messageData['threadId'] = $threadId; + } + $notification = $this->notificationManager->createNotification(); $notification ->setApp('spreed') ->setObject('chat', $chat->getToken()) ->setSubject($subject, $subjectData) - ->setMessage($comment->getVerb(), [ - 'commentId' => $comment->getId(), - ]) + ->setMessage($comment->getVerb(), $messageData) ->setDateTime($reaction ? $reaction->getCreationDateTime() : $comment->getCreationDateTime()); return $notification; diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index 283f3cc2853..9574b52df2e 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -603,7 +603,9 @@ protected function parseChatMessage(INotification $notification, Room $room, Par ]; // Set the link to the specific message - $notification->setLink($this->url->linkToRouteAbsolute('spreed.Page.showCall', ['token' => $room->getToken()]) . '#message_' . $message->getMessageId()); + $notification->setLink($this->url->linkToRouteAbsolute('spreed.Page.showCall', ['token' => $room->getToken()]) + . (isset($messageParameters['threadId']) ? '?threadId=' . $messageParameters['threadId'] : '') + . '#message_' . $message->getMessageId()); $now = $this->timeFactory->getDateTime(); $expireDate = $message->getExpirationDateTime(); @@ -632,6 +634,9 @@ protected function parseChatMessage(INotification $notification, Room $room, Par // Forward the message ID as well to the clients, so they can quote the message on replies $notification->setObject($notification->getObjectType(), $notification->getObjectId() . '/' . $message->getMessageId()); + if (isset($messageParameters['threadId'])) { + $notification->setObject($notification->getObjectType(), $notification->getObjectId() . '/' . $messageParameters['threadId']); + } } $richSubjectParameters = [ diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index 990f23bd7c0..c61694817a8 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -3493,14 +3493,17 @@ private function assertNotifications(array $notifications, TableNode $formData): $data = []; if (isset($expectedNotification['object_id'])) { if (str_contains($notification['object_id'], '/')) { - [$roomToken, $message] = explode('/', $notification['object_id']); - $messageText = self::$messageIdToText[$message] ?? 'UNKNOWN_MESSAGE'; - - $messageText = str_replace($this->localServerUrl, '{$LOCAL_URL}', $messageText); - $messageText = str_replace($this->remoteServerUrl, '{$REMOTE_URL}', $messageText); + $parts = explode('/', $notification['object_id']); + $roomToken = $parts[0]; + $message = $parts[1]; + $messageText = self::$messageIdToText[$message] ?? 'UNKNOWN_MESSAGE'; + $messageText = str_replace([$this->localServerUrl, $this->remoteServerUrl], ['{$LOCAL_URL}', '{$REMOTE_URL}'], $messageText); $data['object_id'] = self::$tokenToIdentifier[$roomToken] . '/' . $messageText; - } elseif (strpos($expectedNotification['object_id'], 'INVITE_ID') !== false) { + if (isset($parts[2])) { + $data['object_id'] .= '/' . self::$threadIdToTitle[$parts[2]]; + } + } elseif (str_contains($expectedNotification['object_id'], 'INVITE_ID')) { $data['object_id'] = 'INVITE_ID(' . self::$inviteIdToRemote[$notification['object_id']] . ')'; } else { [$roomToken,] = explode('/', $notification['object_id']); diff --git a/tests/integration/features/chat-4/threads.feature b/tests/integration/features/chat-4/threads.feature index 70c2567f55e..f8aa3a12c9c 100644 --- a/tests/integration/features/chat-4/threads.feature +++ b/tests/integration/features/chat-4/threads.feature @@ -83,15 +83,21 @@ Feature: chat-4/threads And user "participant2" sends reply "Message 1-2" on message "Message 1" to room "room" with 201 And user "participant1" has the following notifications | app | object_type | object_id | subject | - | spreed | chat | room/Message 1-2 | participant2-displayname replied to your message in conversation room | + | spreed | chat | room/Message 1-2/Thread 1 | participant2-displayname replied to your message in conversation room | And user "participant1" subscribes to thread "Message 1" in room "room" with notification level 1 with 200 | t.id | t.title | t.numReplies | t.lastMessage | a.notificationLevel | firstMessage | lastMessage | | Message 1 | Thread 1 | 2 | Message 1-2 | 1 | Message 1 | Message 1-2 | And user "participant2" sends reply "Message 1-3" on thread "Thread 1" to room "room" with 201 And user "participant1" has the following notifications - | app | object_type | object_id | subject | - | spreed | chat | room/Message 1-3 | participant2-displayname sent a message in conversation room | - | spreed | chat | room/Message 1-2 | participant2-displayname replied to your message in conversation room | + | app | object_type | object_id | subject | + | spreed | chat | room/Message 1-3/Thread 1 | participant2-displayname sent a message in conversation room | + | spreed | chat | room/Message 1-2/Thread 1 | participant2-displayname replied to your message in conversation room | + When user "participant2" sends reply "@participant1" on thread "Thread 1" to room "room" with 201 + Then user "participant1" has the following notifications + | app | object_type | object_id | subject | + | spreed | chat | room/@participant1/Thread 1 | participant2-displayname mentioned you in conversation room | + | spreed | chat | room/Message 1-3/Thread 1 | participant2-displayname sent a message in conversation room | + | spreed | chat | room/Message 1-2/Thread 1 | participant2-displayname replied to your message in conversation room | Scenario: Thread titles are trimmed Given user "participant1" creates room "room" (v4) @@ -214,7 +220,7 @@ Feature: chat-4/threads | Message 1 | room1 | Thread 1 | 1 | Message 2 | 0 | Message 1 | Message 2 | And user "participant1" has the following notifications | app | object_type | object_id | subject | - | spreed | chat | room1/Message 2 | participant2-displayname replied to your message in conversation room1 | + | spreed | chat | room1/Message 2/Thread 1 | participant2-displayname replied to your message in conversation room1 | Then user "participant2" sees the following subscribed threads | t.id | t.token | t.title | t.numReplies | t.lastMessage | a.notificationLevel | firstMessage | lastMessage | | Message 1 | room1 | Thread 1 | 1 | Message 2 | 0 | Message 1 | Message 2 | From 5158658961597d729e26f077ef434e27e4c9b639 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 17 Sep 2025 12:29:13 +0200 Subject: [PATCH 2/3] fix(threads): Correctly build parameter list Signed-off-by: Joas Schilling --- lib/Notification/Notifier.php | 11 ++++++++--- lib/Search/MessageSearch.php | 10 +++++++--- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index 9574b52df2e..5e6b63f7e1c 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -603,9 +603,14 @@ protected function parseChatMessage(INotification $notification, Room $room, Par ]; // Set the link to the specific message - $notification->setLink($this->url->linkToRouteAbsolute('spreed.Page.showCall', ['token' => $room->getToken()]) - . (isset($messageParameters['threadId']) ? '?threadId=' . $messageParameters['threadId'] : '') - . '#message_' . $message->getMessageId()); + $urlParams = [ + 'token' => $room->getToken(), + '_fragment' => 'message_' . $message->getMessageId(), + ]; + if (isset($messageParameters['threadId'])) { + $urlParams['threadId'] = $messageParameters['threadId']; + } + $notification->setLink($this->url->linkToRouteAbsolute('spreed.Page.showCall', $urlParams)); $now = $this->timeFactory->getDateTime(); $expireDate = $message->getExpirationDateTime(); diff --git a/lib/Search/MessageSearch.php b/lib/Search/MessageSearch.php index 3e5f3feac4c..ccaf12d0010 100644 --- a/lib/Search/MessageSearch.php +++ b/lib/Search/MessageSearch.php @@ -285,12 +285,18 @@ protected function commentToSearchResultEntry(Room $room, IUser $user, IComment } } + $urlParams = [ + 'token' => $room->getToken(), + '_fragment' => 'message_' . $id, + ]; $threadId = (int)$comment->getTopmostParentId() ?: (int)$comment->getId(); try { $thread = $this->threadService->findByThreadId($room->getId(), $threadId); + $urlParams['threadId'] = $thread->getId(); } catch (DoesNotExistException) { $thread = null; } + $entry = new SearchResultEntry( $iconUrl, str_replace( @@ -299,9 +305,7 @@ protected function commentToSearchResultEntry(Room $room, IUser $user, IComment $subline ), $messageStr, - $this->url->linkToRouteAbsolute('spreed.Page.showCall', ['token' => $room->getToken()]) - . ($thread !== null ? '?threadId=' . $thread->getId() : '') - . '#message_' . $comment->getId(), + $this->url->linkToRouteAbsolute('spreed.Page.showCall', $urlParams), 'icon-talk', // $iconClass, true ); From 0ac99ac54979b338e78a357bb6f85eef53a05a22 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 17 Sep 2025 12:52:29 +0200 Subject: [PATCH 3/3] fix(threads): Don't use thread id in notification when not a thread Signed-off-by: Joas Schilling --- lib/Chat/ChatManager.php | 20 ++++++++++++++++++-- lib/Chat/SystemMessage/Listener.php | 13 ------------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/lib/Chat/ChatManager.php b/lib/Chat/ChatManager.php index 9df43cdf361..f218a2916d8 100644 --- a/lib/Chat/ChatManager.php +++ b/lib/Chat/ChatManager.php @@ -206,6 +206,20 @@ public function addSystemMessage( // Update last_message $this->roomService->setLastMessage($chat, $comment); $this->unreadCountCache->clear($chat->getId() . '-'); + + if ($threadId !== 0) { + $isThread = $this->threadService->updateLastMessageInfoAfterReply($threadId, (int)$comment->getId()); + if ($isThread && $actorType === Attendee::ACTOR_USERS) { + try { + // Add to subscribed threads list + $participant = $this->participantService->getParticipant($chat, $actorId); + $this->threadService->ensureIsThreadAttendee($participant->getAttendee(), $threadId); + } catch (ParticipantNotFoundException) { + } + } elseif (!$isThread) { + $threadId = 0; + } + } } if ($sendNotifications) { @@ -429,8 +443,10 @@ public function sendMessage( $messageId = (int)$comment->getId(); $threadId = (int)$comment->getTopmostParentId(); if ($threadId !== 0) { - $this->threadService->updateLastMessageInfoAfterReply($threadId, $messageId); - if ($participant instanceof Participant) { + $isThread = $this->threadService->updateLastMessageInfoAfterReply($threadId, $messageId); + if (!$isThread) { + $threadId = 0; + } elseif ($participant instanceof Participant) { // Add to subscribed threads list $this->threadService->ensureIsThreadAttendee($participant->getAttendee(), $threadId); } diff --git a/lib/Chat/SystemMessage/Listener.php b/lib/Chat/SystemMessage/Listener.php index b8e4648c8bf..378e0358911 100644 --- a/lib/Chat/SystemMessage/Listener.php +++ b/lib/Chat/SystemMessage/Listener.php @@ -443,19 +443,6 @@ protected function fixMimeTypeOfVoiceMessage(ShareCreatedEvent|BeforeDuplicateSh silent: true, parent: $comment, ); - } else { - $threadId = (int)$comment->getTopmostParentId(); - if ($threadId !== 0) { - $isThread = $this->threadService->updateLastMessageInfoAfterReply($threadId, $messageId); - if ($isThread) { - try { - // Add to subscribed threads list - $participant = $this->participantService->getParticipant($room, $this->getUserId()); - $this->threadService->ensureIsThreadAttendee($participant->getAttendee(), $threadId); - } catch (ParticipantNotFoundException) { - } - } - } } }