fix(chat): Fix marking the first message of a chat unread

Signed-off-by: Joas Schilling <coding@schilljs.com>

lib/Chat/ChatManager.php

@@ -78,6 +78,24 @@ class ChatManager {
 	public const VERB_RECORD_AUDIO = 'record-audio';
 	public const VERB_RECORD_VIDEO = 'record-video';
 
+	/**
+	 * Last read message ID of -1 is set on the attendee table as default.
+	 * The real value is inserted on user request after the migration from
+	 * `comments_read_markers` to `talk_attendees` with @see Version7000Date20190724121136
+	 *
+	 * @since 21.0.0 (But -1 was used in the database since 7.0.0)
+	 */
+	public const UNREAD_MIGRATION = -1;
+
+	/**
+	 * Frontend and Desktop don't get chat context with ID 0,
+	 * so we collectively tested and decided that -2 should be used instead,
+	 * when marking the first message in a chat as unread.
+	 *
+	 * @since 21.0.0
+	 */
+	public const UNREAD_FIRST_MESSAGE = -2;
+
 	protected ICache $cache;
 	protected ICache $unreadCountCache;
 

lib/Controller/ChatController.php

@@ -1210,7 +1210,7 @@ public function clearHistory(): DataResponse {
 	 * Set the read marker to a specific message
 	 *
 	 * @param int|null $lastReadMessage ID if the last read message (Optional only with `chat-read-last` capability)
-	 * @psalm-param non-negative-int|null $lastReadMessage
+	 * @psalm-param int<-2, max>|null $lastReadMessage
 	 * @return DataResponse<Http::STATUS_OK, TalkRoom, array{X-Chat-Last-Common-Read?: numeric-string}>
 	 *
 	 * 200: Read marker set successfully
@@ -1220,6 +1220,15 @@ public function clearHistory(): DataResponse {
 	#[RequireAuthenticatedParticipant]
 	public function setReadMarker(?int $lastReadMessage = null): DataResponse {
 		$setToMessage = $lastReadMessage ?? $this->room->getLastMessageId();
+		if ($setToMessage === 0) {
+			/**
+			 * Frontend and Desktop don't get chat context with ID 0,
+			 * so we collectively tested and decided that @see ChatManager::UNREAD_FIRST_MESSAGE
+			 * should be used instead.
+			 */
+			$setToMessage = ChatManager::UNREAD_FIRST_MESSAGE;
+		}
+
 		if ($setToMessage === $this->room->getLastMessageId()
 			&& $this->participant->getAttendee()->getActorType() === Attendee::ACTOR_USERS) {
 			$this->notifier->markMentionNotificationsRead($this->room, $this->participant->getAttendee()->getActorId());
@@ -1266,8 +1275,6 @@ public function markUnread(): DataResponse {
 		}
 
 		$message = $this->room->getLastMessage();
-		$unreadId = 0;
-
 		if ($message instanceof IComment) {
 			try {
 				$previousMessage = $this->chatManager->getPreviousMessageWithVerb(
@@ -1276,14 +1283,36 @@ public function markUnread(): DataResponse {
 					[ChatManager::VERB_MESSAGE, ChatManager::VERB_OBJECT_SHARED],
 					$message->getVerb() === ChatManager::VERB_MESSAGE || $message->getVerb() === ChatManager::VERB_OBJECT_SHARED
 				);
-				$unreadId = (int)$previousMessage->getId();
-			} catch (NotFoundException $e) {
-				// No chat message found, only system messages.
-				// Marking unread from beginning
+				return $this->setReadMarker((int)$previousMessage->getId());
+			} catch (NotFoundException) {
+				// No chat message found, try system messages …
+			}
+
+			try {
+				$messages = $this->chatManager->getHistory(
+					$this->room,
+					(int)$message->getId(),
+					1,
+					false,
+				);
+
+				if (empty($messages)) {
+					throw new NotFoundException('No comments found');
+				}
+
+				$previousMessage = array_pop($messages);
+				return $this->setReadMarker((int)$previousMessage->getId());
+			} catch (NotFoundException) {
+				/**
+				 * Neither system messages found, fall back to `-1`.
+				 * This can happen when you:
+				 * - Set up message expiration
+				 * - Clear the chat history afterwards
+				 */
 			}
 		}
 
-		return $this->setReadMarker($unreadId);
+		return $this->setReadMarker(ChatManager::UNREAD_FIRST_MESSAGE);
 	}
 
 	/**

lib/Service/RoomFormatter.php

@@ -293,7 +293,7 @@ public function formatRoomV4(
 				$roomData['unreadMessages'] = $attendee->getUnreadMessages();
 			} elseif ($currentUser instanceof IUser) {
 				$lastReadMessage = $attendee->getLastReadMessage();
-				if ($lastReadMessage === -1) {
+				if ($lastReadMessage === ChatManager::UNREAD_MIGRATION) {
 					/*
 					 * Because the migration from the old comment_read_markers was
 					 * not possible in a programmatic way with a reasonable O(1) or O(n)
@@ -396,6 +396,12 @@ public function formatRoomV4(
 			} catch (DoesNotExistException) {
 			}
 		}
+		if ($currentUser instanceof IUser
+			&& $attendee->getActorType() === Attendee::ACTOR_USERS
+			&& $roomData['lastReadMessage'] === ChatManager::UNREAD_FIRST_MESSAGE
+			&& $roomData['unreadMessages'] === 0) {
+			$roomData['unreadMessages'] = 1;
+		}
 
 		if ($room->isFederatedConversation()) {
 			$roomData['attendeeId'] = (int)$attendee->getRemoteId();

openapi-full.json

@@ -7382,7 +7382,7 @@
                                         "format": "int64",
                                         "nullable": true,
                                         "description": "ID if the last read message (Optional only with `chat-read-last` capability)",
-                                        "minimum": 0
+                                        "minimum": -2
                                     }
                                 }
                             }

openapi.json

@@ -7269,7 +7269,7 @@
                                         "format": "int64",
                                         "nullable": true,
                                         "description": "ID if the last read message (Optional only with `chat-read-last` capability)",
-                                        "minimum": 0
+                                        "minimum": -2
                                     }
                                 }
                             }

tests/integration/features/bootstrap/FeatureContext.php

@@ -557,7 +557,7 @@ private function assertRooms(array $rooms, TableNode $formData, bool $shouldOrde
 				$data['lastMessageActorId'] = str_replace(rtrim($this->remoteServerUrl, '/'), '{$REMOTE_URL}', $data['lastMessageActorId']);
 			}
 			if (isset($expectedRoom['lastReadMessage'])) {
-				$data['lastReadMessage'] = self::$messageIdToText[(int)$room['lastReadMessage']] ?? (!$room['lastReadMessage'] ? 'ZERO': 'UNKNOWN_MESSAGE');
+				$data['lastReadMessage'] = self::$messageIdToText[(int)$room['lastReadMessage']] ?? ($room['lastReadMessage'] === -2 ? 'FIRST_MESSAGE_UNREAD': 'UNKNOWN_MESSAGE');
 			}
 			if (isset($expectedRoom['unreadMessages'])) {
 				$data['unreadMessages'] = (int)$room['unreadMessages'];

tests/integration/features/chat-4/unread-messages.feature

@@ -220,3 +220,37 @@ Feature: chat-2/unread-messages
       | unreadMessages |
       | 1              |
 
+  Scenario: marking first message as unread falls back to system message
+    Given user "participant1" creates room "room" (v4)
+      | roomType | 2 |
+      | roomName | room |
+    And user "participant1" adds user "participant2" to room "room" with 200 (v4)
+    And user "participant1" sends message "Message 1" to room "room" with 201
+    And user "participant1" sees the following system messages in room "room" with 200 (v1)
+      | room | actorType     | actorId      | systemMessage        | message                      | silent | messageParameters |
+      | room | users         | participant1 | user_added           | You added {user}             | !ISSET | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"},"user":{"type":"user","id":"participant2","name":"participant2-displayname"}} |
+      | room | users         | participant1 | conversation_created | You created the conversation | !ISSET | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} |
+    Then user "participant2" is participant of the following rooms (v4)
+      | id   | unreadMessages | lastReadMessage      |
+      | room | 1              | conversation_created |
+    And wait for 1 seconds
+    And user "participant2" marks room "room" as unread with 200
+    Then user "participant2" is participant of room "room" (v4)
+      | unreadMessages | lastReadMessage |
+      | 1              | user_added      |
+
+  Scenario: marking first message as unread falls back to FIRST_MESSAGE_UNREAD
+    Given user "participant1" creates room "room" (v4)
+      | roomType | 2 |
+      | roomName | room |
+    And user "participant1" sees the following system messages in room "room" with 200 (v1)
+      | room | actorType     | actorId      | systemMessage        | message                      | silent | messageParameters |
+      | room | users         | participant1 | conversation_created | You created the conversation | !ISSET | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} |
+    Then user "participant1" is participant of the following rooms (v4)
+      | id   | unreadMessages | lastReadMessage      |
+      | room | 0              | conversation_created |
+    And wait for 1 seconds
+    And user "participant1" marks room "room" as unread with 200
+    Then user "participant1" is participant of room "room" (v4)
+      | unreadMessages | lastReadMessage      |
+      | 1              | FIRST_MESSAGE_UNREAD |