Merge pull request #11861 from nextcloud/bugfix/noid/mark-notifications-for-federated-rooms-also-read

fix(federation): Mark notifications for federated rooms read similarly

lib/Controller/ChatController.php

@@ -30,6 +30,7 @@ use OCA\Talk\Chat\AutoComplete\SearchPlugin;
 use OCA\Talk\Chat\AutoComplete\Sorter;
 use OCA\Talk\Chat\ChatManager;
 use OCA\Talk\Chat\MessageParser;
+use OCA\Talk\Chat\Notifier;
 use OCA\Talk\Chat\ReactionManager;
 use OCA\Talk\Exceptions\CannotReachRemoteException;
 use OCA\Talk\Federation\Authenticator;
@@ -130,6 +131,7 @@ class ChatController extends AEnvironmentAwareController {
 		private IL10N $l,
 		protected Authenticator $federationAuthenticator,
 		protected ProxyCacheMessageService $pcmService,
+		protected Notifier $notifier,
 	) {
 		parent::__construct($appName, $request);
 	}
@@ -1086,14 +1088,19 @@ class ChatController extends AEnvironmentAwareController {
 	#[PublicPage]
 	#[RequireAuthenticatedParticipant]
 	public function setReadMarker(?int $lastReadMessage = null): DataResponse {
+		$setToMessage = $lastReadMessage ?? $this->room->getLastMessageId();
+		if ($setToMessage === $this->room->getLastMessageId()
+			&& $this->participant->getAttendee()->getActorType() === Attendee::ACTOR_USERS) {
+			$this->notifier->markMentionNotificationsRead($this->room, $this->participant->getAttendee()->getActorId());
+		}
+
 		if ($this->room->isFederatedConversation()) {
 			/** @var \OCA\Talk\Federation\Proxy\TalkV1\Controller\ChatController $proxy */
 			$proxy = \OCP\Server::get(\OCA\Talk\Federation\Proxy\TalkV1\Controller\ChatController::class);
 			return $proxy->setReadMarker($this->room, $this->participant, $this->getResponseFormat(), $lastReadMessage);
 		}
 
-		$lastReadMessage = $lastReadMessage ?? $this->room->getLastMessageId();
-		$this->participantService->updateLastReadMessage($this->participant, $lastReadMessage);
+		$this->participantService->updateLastReadMessage($this->participant, $setToMessage);
 		$attendee = $this->participant->getAttendee();
 
 		$headers = $lastCommonRead = [];

lib/Federation/Proxy/TalkV1/Controller/ChatController.php

@@ -26,9 +26,11 @@ declare(strict_types=1);
 
 namespace OCA\Talk\Federation\Proxy\TalkV1\Controller;
 
+use OCA\Talk\Chat\Notifier;
 use OCA\Talk\Exceptions\CannotReachRemoteException;
 use OCA\Talk\Federation\Proxy\TalkV1\ProxyRequest;
 use OCA\Talk\Federation\Proxy\TalkV1\UserConverter;
+use OCA\Talk\Model\Attendee;
 use OCA\Talk\Participant;
 use OCA\Talk\ResponseDefinitions;
 use OCA\Talk\Room;
@@ -52,6 +54,7 @@ class ChatController {
 		protected UserConverter $userConverter,
 		protected ParticipantService $participantService,
 		protected RoomFormatter $roomFormatter,
+		protected Notifier $notifier,
 		ICacheFactory $cacheFactory,
 	) {
 		$this->proxyCacheMessages = $cacheFactory->isAvailable() ? $cacheFactory->createDistributed('talk/pcm/') : null;
@@ -139,6 +142,11 @@ class ChatController {
 		int $markNotificationsAsRead): DataResponse {
 		$cacheKey = sha1(json_encode([$room->getRemoteServer(), $room->getRemoteToken()]));
 
+
+		if ($lookIntoFuture && $markNotificationsAsRead && $participant->getAttendee()->getActorType() === Attendee::ACTOR_USERS) {
+			$this->notifier->markMentionNotificationsRead($room, $participant->getAttendee()->getActorId());
+		}
+
 		if ($lookIntoFuture) {
 			if ($this->proxyCacheMessages instanceof ICache) {
 				for ($i = 0; $i <= $timeout; $i++) {
@@ -232,6 +240,10 @@ class ChatController {
 			],
 		);
 
+		if ($participant->getAttendee()->getActorType() === Attendee::ACTOR_USERS) {
+			$this->notifier->markMentionNotificationsRead($room, $participant->getAttendee()->getActorId());
+		}
+
 		if ($proxy->getStatusCode() === Http::STATUS_NOT_MODIFIED) {
 			return new DataResponse([], Http::STATUS_NOT_MODIFIED);
 		}

lib/Notification/Listener.php

@@ -160,7 +160,7 @@ class Listener implements IEventListener {
 	 * Reaction: "{user} reacted with {reaction} in {call}"
 	 *
 	 * We should not mark reactions read based on the read-status of the comment
-	 * they apply to, but the point in time when the reaction as done.
+	 * they apply to, but the point in time when the reaction was done.
 	 * However, these messages are not visible and don't update the read marker,
 	 * so we purge them on joining the conversation.
 	 * This already happened before on the initial loading of a chat with

tests/integration/features/federation/chat.feature

@@ -226,6 +226,13 @@ Feature: federation/chat
       | spreed | chat        | room/Hi @all bye         | participant1-displayname mentioned everyone in conversation room       | Hi room bye |
       | spreed | chat        | room/Hi @"federated_user/participant2@{$REMOTE_URL}" bye | participant1-displayname mentioned you in conversation room | Hi @participant2-displayname bye |
       | spreed | chat        | room/Message 1-1         | participant1-displayname replied to your message in conversation room  | Message 1-1 |
+    When next message request has the following parameters set
+      | timeout                  | 0         |
+      | lookIntoFuture           | 1         |
+      | lastKnownMessageId       | Hi @all bye         |
+    And user "participant2" sees the following messages in room "LOCAL::room" with 304
+    Then user "participant2" has the following notifications
+      | app    | object_type | object_id                | subject                                                                | message     |
 
   Scenario: Mentioning a federated user as a guest also triggers a notification for them
     Given the following "spreed" app config is set
@@ -248,9 +255,13 @@ Feature: federation/chat
     Given user "participant2" leaves room "LOCAL::room" with 200 (v4)
     And user "guest" joins room "room" with 200 (v4)
     When user "guest" sends message 'Hi @"federated_user/participant2@{$REMOTE_URL}" bye' to room "room" with 201
+    When user "guest" sends message "Message 2" to room "room" with 201
     Then user "participant2" has the following notifications
       | app    | object_type | object_id                | subject                                                                | message     |
       | spreed | chat        | room/Hi @"federated_user/participant2@{$REMOTE_URL}" bye | A guest mentioned you in conversation room | Hi @participant2-displayname bye |
+    Then user "participant2" reads message "Message 2" in room "LOCAL::room" with 200 (v1)
+    Then user "participant2" has the following notifications
+      | app    | object_type | object_id                | subject                                                                | message     |
 
   Scenario: Mentioning a federated user as a federated user that is a local user to the mentioned one also triggers a notification for them
     Given the following "spreed" app config is set

tests/php/Controller/ChatControllerTest.php

@@ -26,6 +26,7 @@ namespace OCA\Talk\Tests\php\Controller;
 use OCA\Talk\Chat\AutoComplete\SearchPlugin;
 use OCA\Talk\Chat\ChatManager;
 use OCA\Talk\Chat\MessageParser;
+use OCA\Talk\Chat\Notifier;
 use OCA\Talk\Chat\ReactionManager;
 use OCA\Talk\Controller\ChatController;
 use OCA\Talk\Federation\Authenticator;
@@ -117,6 +118,7 @@ class ChatControllerTest extends TestCase {
 	private $l;
 	private Authenticator|MockObject $federationAuthenticator;
 	private ProxyCacheMessageService|MockObject $pcmService;
+	private Notifier|MockObject $notifier;
 
 	/** @var Room|MockObject */
 	protected $room;
@@ -157,6 +159,7 @@ class ChatControllerTest extends TestCase {
 		$this->l = $this->createMock(IL10N::class);
 		$this->federationAuthenticator = $this->createMock(Authenticator::class);
 		$this->pcmService = $this->createMock(ProxyCacheMessageService::class);
+		$this->notifier = $this->createMock(Notifier::class);
 
 		$this->room = $this->createMock(Room::class);
 
@@ -202,6 +205,7 @@ class ChatControllerTest extends TestCase {
 			$this->l,
 			$this->federationAuthenticator,
 			$this->pcmService,
+			$this->notifier,
 		);
 	}