From 8bc7b538a9ef43d3dcd0ca94f43b7517d56b0730 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 13 Mar 2024 16:57:53 +0100 Subject: [PATCH] fix(federation): Fix unread marker handling in federated chats Signed-off-by: Joas Schilling --- appinfo/info.xml | 2 +- lib/Federation/BackendNotifier.php | 2 +- lib/Federation/CloudFederationProviderTalk.php | 4 +++- lib/Federation/FederationManager.php | 1 + .../Proxy/TalkV1/Controller/ChatController.php | 9 ++++++++- .../Proxy/TalkV1/Notifier/MessageSentListener.php | 3 ++- lib/Model/Attendee.php | 12 ++++++++++++ lib/Model/AttendeeMapper.php | 2 ++ lib/Model/SelectHelper.php | 2 ++ lib/Notification/FederationChatNotifier.php | 2 +- lib/Service/ParticipantService.php | 7 ++++--- lib/Service/RoomFormatter.php | 13 +++++++------ tests/php/Chat/ChatManagerTest.php | 7 +++++++ 13 files changed, 51 insertions(+), 15 deletions(-) diff --git a/appinfo/info.xml b/appinfo/info.xml index d0d84c689732..16f164bbc22d 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -16,7 +16,7 @@ And in the works for the [coming versions](https://github.com/nextcloud/spreed/m ]]> - 19.0.0-beta.1.1 + 19.0.0-beta.1.2 agpl Daniel Calviño Sánchez diff --git a/lib/Federation/BackendNotifier.php b/lib/Federation/BackendNotifier.php index cb2102afe965..c45bedff61cb 100644 --- a/lib/Federation/BackendNotifier.php +++ b/lib/Federation/BackendNotifier.php @@ -292,7 +292,7 @@ public function sendRoomModifiedUpdate( * Sent from Host server to Remote participant server * * @param array{remoteMessageId: int, actorType: string, actorId: string, actorDisplayName: string, messageType: string, systemMessage: string, expirationDatetime: string, message: string, messageParameter: string, creationDatetime: string, metaData: string} $messageData - * @param array{unreadMessages: int, unreadMention: bool, unreadMentionDirect: bool} $unreadInfo + * @param array{unreadMessages: int, unreadMention: bool, unreadMentionDirect: bool, lastReadMessage: int} $unreadInfo */ public function sendMessageUpdate( string $remoteServer, diff --git a/lib/Federation/CloudFederationProviderTalk.php b/lib/Federation/CloudFederationProviderTalk.php index 8b3340388fe3..cb161b1193ba 100644 --- a/lib/Federation/CloudFederationProviderTalk.php +++ b/lib/Federation/CloudFederationProviderTalk.php @@ -215,6 +215,7 @@ private function shareAccepted(int $id, array $notification): array { if (!empty($notification['displayName'])) { $attendee->setDisplayName($notification['displayName']); + $attendee->setState(Invitation::STATE_ACCEPTED); $this->attendeeMapper->update($attendee); } @@ -325,7 +326,7 @@ private function roomModified(int $remoteAttendeeId, array $notification): array /** * @param int $remoteAttendeeId - * @param array{remoteServerUrl: string, sharedSecret: string, remoteToken: string, messageData: array{remoteMessageId: int, actorType: string, actorId: string, actorDisplayName: string, messageType: string, systemMessage: string, expirationDatetime: string, message: string, messageParameter: string, creationDatetime: string, metaData: string}, unreadInfo: array{unreadMessages: int, unreadMention: bool, unreadMentionDirect: bool}} $notification + * @param array{remoteServerUrl: string, sharedSecret: string, remoteToken: string, messageData: array{remoteMessageId: int, actorType: string, actorId: string, actorDisplayName: string, messageType: string, systemMessage: string, expirationDatetime: string, message: string, messageParameter: string, creationDatetime: string, metaData: string}, unreadInfo: array{unreadMessages: int, unreadMention: bool, unreadMentionDirect: bool, lastReadMessage: int}} $notification * @return array * @throws ActionNotSupportedException * @throws AuthenticationFailedException @@ -416,6 +417,7 @@ private function messagePosted(int $remoteAttendeeId, array $notification): arra $notification['unreadInfo']['unreadMessages'], $notification['unreadInfo']['unreadMention'], $notification['unreadInfo']['unreadMentionDirect'], + $notification['unreadInfo']['lastReadMessage'], ); $this->federationChatNotifier->handleChatMessage($room, $participant, $message, $notification); diff --git a/lib/Federation/FederationManager.php b/lib/Federation/FederationManager.php index 88b56f4caeef..d5753aad8f34 100644 --- a/lib/Federation/FederationManager.php +++ b/lib/Federation/FederationManager.php @@ -147,6 +147,7 @@ public function acceptRemoteRoomShare(IUser $user, int $shareId): Participant { 'accessToken' => $invitation->getAccessToken(), 'remoteId' => $invitation->getRemoteAttendeeId(), 'invitedCloudId' => $invitation->getLocalCloudId(), + 'lastReadMessage' => $room->getLastMessageId(), ] ]; $attendees = $this->participantService->addUsers($room, $participant, $user); diff --git a/lib/Federation/Proxy/TalkV1/Controller/ChatController.php b/lib/Federation/Proxy/TalkV1/Controller/ChatController.php index 366c4bd552f2..4ceb5a4a311c 100644 --- a/lib/Federation/Proxy/TalkV1/Controller/ChatController.php +++ b/lib/Federation/Proxy/TalkV1/Controller/ChatController.php @@ -173,7 +173,12 @@ public function receiveMessages( ); if ($lookIntoFuture && $setReadMarker) { - $this->participantService->updateUnreadInfoForProxyParticipant($participant, 0, false, false); + $this->participantService->updateUnreadInfoForProxyParticipant($participant, + 0, + false, + false, + (int) ($proxy->getHeader('X-Chat-Last-Given') ?: $lastKnownMessageId), + ); } if ($proxy->getStatusCode() === Http::STATUS_NOT_MODIFIED) { @@ -384,6 +389,7 @@ public function setReadMarker(Room $room, Participant $participant, string $resp $data['unreadMessages'], $data['unreadMention'], $data['unreadMentionDirect'], + $data['lastReadMessage'], ); $headers = $lastCommonRead = []; @@ -424,6 +430,7 @@ public function markUnread(Room $room, Participant $participant, string $respons $data['unreadMessages'], $data['unreadMention'], $data['unreadMentionDirect'], + $data['lastReadMessage'], ); $headers = $lastCommonRead = []; diff --git a/lib/Federation/Proxy/TalkV1/Notifier/MessageSentListener.php b/lib/Federation/Proxy/TalkV1/Notifier/MessageSentListener.php index 7c12c9b32ed8..6f1277c1b348 100644 --- a/lib/Federation/Proxy/TalkV1/Notifier/MessageSentListener.php +++ b/lib/Federation/Proxy/TalkV1/Notifier/MessageSentListener.php @@ -114,9 +114,10 @@ public function handle(Event $event): void { $lastMentionDirect = $attendee->getLastMentionDirect(); $unreadInfo = [ + 'lastReadMessage' => $lastReadMessage, 'unreadMessages' => $this->chatManager->getUnreadCount($event->getRoom(), $lastReadMessage), 'unreadMention' => $lastMention !== 0 && $lastReadMessage < $lastMention, - 'unreadMentionDirect' => $lastMentionDirect !== 0 && $lastReadMessage < $lastMentionDirect + 'unreadMentionDirect' => $lastMentionDirect !== 0 && $lastReadMessage < $lastMentionDirect, ]; $success = $this->backendNotifier->sendMessageUpdate( diff --git a/lib/Model/Attendee.php b/lib/Model/Attendee.php index dc2fd2dc6449..823ac6fe7c7c 100644 --- a/lib/Model/Attendee.php +++ b/lib/Model/Attendee.php @@ -66,6 +66,10 @@ * @method null|string getPhoneNumber() * @method void setCallId(?string $callId) * @method null|string getCallId() + * @method void setState(int $state) + * @method int getState() + * @method void setUnreadMessages(int $unreadMessages) + * @method int getUnreadMessages() */ class Attendee extends Entity { public const ACTOR_USERS = 'users'; @@ -168,6 +172,12 @@ class Attendee extends Entity { /** @var null|string */ protected $callId; + /** @var int */ + protected $state; + + /** @var int */ + protected $unreadMessages; + public function __construct() { $this->addType('roomId', 'int'); $this->addType('actorType', 'string'); @@ -189,6 +199,8 @@ public function __construct() { $this->addType('invitedCloudId', 'string'); $this->addType('phoneNumber', 'string'); $this->addType('callId', 'string'); + $this->addType('state', 'int'); + $this->addType('unreadMessages', 'int'); } public function getDisplayName(): string { diff --git a/lib/Model/AttendeeMapper.php b/lib/Model/AttendeeMapper.php index 268f5eab9f6f..2cdab0b1901b 100644 --- a/lib/Model/AttendeeMapper.php +++ b/lib/Model/AttendeeMapper.php @@ -299,6 +299,8 @@ public function createAttendeeFromRow(array $row): Attendee { 'invited_cloud_id' => (string) $row['invited_cloud_id'], 'phone_number' => $row['phone_number'], 'call_id' => $row['call_id'], + 'state' => (int) $row['state'], + 'unread_messages' => (int) $row['unread_messages'], ]); } } diff --git a/lib/Model/SelectHelper.php b/lib/Model/SelectHelper.php index a2513dda7e4e..11304a0def51 100644 --- a/lib/Model/SelectHelper.php +++ b/lib/Model/SelectHelper.php @@ -87,6 +87,8 @@ public function selectAttendeesTable(IQueryBuilder $query, string $alias = 'a'): ->addSelect($alias . 'invited_cloud_id') ->addSelect($alias . 'phone_number') ->addSelect($alias . 'call_id') + ->addSelect($alias . 'state') + ->addSelect($alias . 'unread_messages') ->selectAlias($alias . 'id', 'a_id'); } diff --git a/lib/Notification/FederationChatNotifier.php b/lib/Notification/FederationChatNotifier.php index ef56078b1814..08b6fc094b5a 100644 --- a/lib/Notification/FederationChatNotifier.php +++ b/lib/Notification/FederationChatNotifier.php @@ -45,7 +45,7 @@ public function __construct( } /** - * @param array{remoteServerUrl: string, sharedSecret: string, remoteToken: string, messageData: array{remoteMessageId: int, actorType: string, actorId: string, actorDisplayName: string, messageType: string, systemMessage: string, expirationDatetime: string, message: string, messageParameter: string, creationDatetime: string, metaData: string}, unreadInfo: array{unreadMessages: int, unreadMention: bool, unreadMentionDirect: bool}} $inboundNotification + * @param array{remoteServerUrl: string, sharedSecret: string, remoteToken: string, messageData: array{remoteMessageId: int, actorType: string, actorId: string, actorDisplayName: string, messageType: string, systemMessage: string, expirationDatetime: string, message: string, messageParameter: string, creationDatetime: string, metaData: string}, unreadInfo: array{unreadMessages: int, unreadMention: bool, unreadMentionDirect: bool, lastReadMessage: int}} $inboundNotification */ public function handleChatMessage(Room $room, Participant $participant, ProxyCacheMessage $message, array $inboundNotification): void { /** @var array{silent?: bool, last_edited_time?: int, last_edited_by_type?: string, last_edited_by_id?: string, replyToActorType?: string, replyToActorId?: string} $metaData */ diff --git a/lib/Service/ParticipantService.php b/lib/Service/ParticipantService.php index 5060e80401ed..d68ed521b808 100644 --- a/lib/Service/ParticipantService.php +++ b/lib/Service/ParticipantService.php @@ -249,9 +249,10 @@ public function updateLastReadMessage(Participant $participant, int $lastReadMes $this->attendeeMapper->update($attendee); } - public function updateUnreadInfoForProxyParticipant(Participant $participant, int $unreadMessageCount, bool $hasMention, bool $hadDirectMention): void { + public function updateUnreadInfoForProxyParticipant(Participant $participant, int $unreadMessageCount, bool $hasMention, bool $hadDirectMention, int $lastReadMessageId): void { $attendee = $participant->getAttendee(); - $attendee->setLastReadMessage($unreadMessageCount); + $attendee->setUnreadMessages($unreadMessageCount); + $attendee->setLastReadMessage($lastReadMessageId); $attendee->setLastMentionMessage($hasMention ? 1 : 0); $attendee->setLastMentionDirect($hadDirectMention ? 1 : 0); $this->attendeeMapper->update($attendee); @@ -505,7 +506,7 @@ public function addUsers(Room $room, array $participants, ?IUser $addedBy = null } $attendee->setParticipantType($participant['participantType'] ?? Participant::USER); $attendee->setPermissions(Attendee::PERMISSIONS_DEFAULT); - $attendee->setLastReadMessage($lastMessage); + $attendee->setLastReadMessage($participant['lastReadMessage'] ?? $lastMessage); $attendee->setReadPrivacy($readPrivacy); $attendees[] = $attendee; } diff --git a/lib/Service/RoomFormatter.php b/lib/Service/RoomFormatter.php index a14ce1957b49..1f6fee7b747c 100644 --- a/lib/Service/RoomFormatter.php +++ b/lib/Service/RoomFormatter.php @@ -292,12 +292,11 @@ public function formatRoomV4( if ($attendee->getActorType() === Attendee::ACTOR_USERS) { $currentUser = $this->userManager->get($attendee->getActorId()); - if ($room->getRemoteServer() !== '') { - // For proxy conversations the information is the real counter, - // not the message ID requiring math afterward. - $roomData['unreadMessages'] = $attendee->getLastReadMessage(); - $roomData['unreadMention'] = (bool) $attendee->getLastMentionMessage(); - $roomData['unreadMentionDirect'] = (bool) $attendee->getLastMentionDirect(); + if ($room->isFederatedConversation()) { + $roomData['lastReadMessage'] = $attendee->getLastReadMessage(); + $roomData['unreadMention'] = (bool)$attendee->getLastMentionMessage(); + $roomData['unreadMentionDirect'] = (bool)$attendee->getLastMentionDirect(); + $roomData['unreadMessages'] = $attendee->getUnreadMessages(); } elseif ($currentUser instanceof IUser) { $lastReadMessage = $attendee->getLastReadMessage(); if ($lastReadMessage === -1) { @@ -338,6 +337,7 @@ public function formatRoomV4( $lastReadMessage = $attendee->getLastReadMessage(); $lastMention = $attendee->getLastMentionMessage(); $lastMentionDirect = $attendee->getLastMentionDirect(); + $roomData['lastReadMessage'] = $lastReadMessage; $roomData['unreadMessages'] = $this->chatManager->getUnreadCount($room, $lastReadMessage); $roomData['unreadMention'] = $lastMention !== 0 && $lastReadMessage < $lastMention; $roomData['unreadMentionDirect'] = $lastMentionDirect !== 0 && $lastReadMessage < $lastMentionDirect; @@ -388,6 +388,7 @@ public function formatRoomV4( $lastMessage, ); } elseif ($room->getRemoteServer() !== '') { + $roomData['lastCommonReadMessage'] = 0; try { $cachedMessage = $this->proxyCacheMessageMapper->findByRemote( $room->getRemoteServer(), diff --git a/tests/php/Chat/ChatManagerTest.php b/tests/php/Chat/ChatManagerTest.php index b8defc616b41..92246fd25a9c 100644 --- a/tests/php/Chat/ChatManagerTest.php +++ b/tests/php/Chat/ChatManagerTest.php @@ -29,6 +29,7 @@ use OCA\Talk\Chat\Notifier; use OCA\Talk\Model\Attendee; use OCA\Talk\Model\AttendeeMapper; +use OCA\Talk\Model\Invitation; use OCA\Talk\Participant; use OCA\Talk\Room; use OCA\Talk\Service\AttachmentService; @@ -446,6 +447,8 @@ public function testDeleteMessage(): void { 'phone_number' => '', 'call_id' => '', 'invited_cloud_id' => '', + 'state' => Invitation::STATE_ACCEPTED, + 'unread_messages' => 0, ]); $chat = $this->createMock(Room::class); $chat->expects($this->any()) @@ -505,6 +508,8 @@ public function testDeleteMessageFileShare(): void { 'phone_number' => '', 'call_id' => '', 'invited_cloud_id' => '', + 'state' => Invitation::STATE_ACCEPTED, + 'unread_messages' => 0, ]); $chat = $this->createMock(Room::class); $chat->expects($this->any()) @@ -586,6 +591,8 @@ public function testDeleteMessageFileShareNotFound(): void { 'phone_number' => '', 'call_id' => '', 'invited_cloud_id' => '', + 'state' => Invitation::STATE_ACCEPTED, + 'unread_messages' => 0, ]); $chat = $this->createMock(Room::class); $chat->expects($this->any())