From 7d2706898ebaff570c1d73058014a869d188c00a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 16 Apr 2024 13:07:07 +0200 Subject: [PATCH] fix(conversations): Include rooms in "modified since" updates when attendee info changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit E.g. favorite, read-marker, notification settings, … Signed-off-by: Joas Schilling --- lib/Controller/RoomController.php | 21 ++++++- .../Version19000Date20240416104656.php | 58 +++++++++++++++++++ lib/Model/Attendee.php | 7 +++ lib/Model/AttendeeMapper.php | 1 + lib/Model/SelectHelper.php | 1 + lib/Service/ParticipantService.php | 9 +++ .../features/bootstrap/FeatureContext.php | 18 +++++- .../features/chat-2/unread-messages.feature | 15 +++++ tests/php/Chat/ChatManagerTest.php | 3 + 9 files changed, 127 insertions(+), 6 deletions(-) create mode 100644 lib/Migration/Version19000Date20240416104656.php diff --git a/lib/Controller/RoomController.php b/lib/Controller/RoomController.php index b6df3269821..99cfdcd9335 100644 --- a/lib/Controller/RoomController.php +++ b/lib/Controller/RoomController.php @@ -214,9 +214,24 @@ public function getRooms(int $noStatusUpdate = 0, bool $includeStatus = false, i $rooms = $this->manager->getRoomsForUser($this->userId, $sessionIds, true); if ($modifiedSince !== 0) { - $rooms = array_filter($rooms, static function (Room $room) use ($includeStatus, $modifiedSince): bool { - return ($includeStatus && $room->getType() === Room::TYPE_ONE_TO_ONE) - || ($room->getLastActivity() && $room->getLastActivity()->getTimestamp() >= $modifiedSince); + $rooms = array_filter($rooms, function (Room $room) use ($includeStatus, $modifiedSince): bool { + if ($includeStatus && $room->getType() === Room::TYPE_ONE_TO_ONE) { + // Always include 1-1s to update the user status + return true; + } + if ($room->getCallFlag() !== Participant::FLAG_DISCONNECTED) { + // Always include active calls + return true; + } + if ($room->getLastActivity() && $room->getLastActivity()->getTimestamp() >= $modifiedSince) { + // Include rooms which had activity + return true; + } + + // Include rooms where only attendee level things changed, + // e.g. favorite, read-marker update, notification setting + $attendee = $room->getParticipant($this->userId)->getAttendee(); + return $attendee->getLastAttendeeActivity() >= $modifiedSince; }); } diff --git a/lib/Migration/Version19000Date20240416104656.php b/lib/Migration/Version19000Date20240416104656.php new file mode 100644 index 00000000000..7d3e1de3589 --- /dev/null +++ b/lib/Migration/Version19000Date20240416104656.php @@ -0,0 +1,58 @@ + + * + * @author Joas Schilling + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\Talk\Migration; + +use Closure; +use OCP\DB\ISchemaWrapper; +use OCP\DB\Types; +use OCP\Migration\IOutput; +use OCP\Migration\SimpleMigrationStep; + +/** + * Add a column to track the last read marker update so conversations marked + * as (un)read on other devices are properly updated with the "lazy" update. + */ +class Version19000Date20240416104656 extends SimpleMigrationStep { + /** + * @param IOutput $output + * @param Closure(): ISchemaWrapper $schemaClosure + * @param array $options + * @return null|ISchemaWrapper + */ + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + + $table = $schema->getTable('talk_attendees'); + $table->addColumn('last_attendee_activity', Types::BIGINT, [ + 'default' => 0, + 'unsigned' => true, + ]); + + return $schema; + } +} diff --git a/lib/Model/Attendee.php b/lib/Model/Attendee.php index 823ac6fe7c7..f61c2efba3f 100644 --- a/lib/Model/Attendee.php +++ b/lib/Model/Attendee.php @@ -70,6 +70,8 @@ * @method int getState() * @method void setUnreadMessages(int $unreadMessages) * @method int getUnreadMessages() + * @method void setLastAttendeeActivity(int $lastAttendeeActivity) + * @method int getLastAttendeeActivity() */ class Attendee extends Entity { public const ACTOR_USERS = 'users'; @@ -178,6 +180,9 @@ class Attendee extends Entity { /** @var int */ protected $unreadMessages; + /** @var int */ + protected $lastAttendeeActivity; + public function __construct() { $this->addType('roomId', 'int'); $this->addType('actorType', 'string'); @@ -201,6 +206,7 @@ public function __construct() { $this->addType('callId', 'string'); $this->addType('state', 'int'); $this->addType('unreadMessages', 'int'); + $this->addType('lastAttendeeActivity', 'int'); } public function getDisplayName(): string { @@ -232,6 +238,7 @@ public function asArray(): array { 'invited_cloud_id' => $this->getInvitedCloudId(), 'phone_number' => $this->getPhoneNumber(), 'call_id' => $this->getCallId(), + 'last_attendee_activity' => $this->getLastAttendeeActivity(), ]; } } diff --git a/lib/Model/AttendeeMapper.php b/lib/Model/AttendeeMapper.php index 2cdab0b1901..be911f50d4c 100644 --- a/lib/Model/AttendeeMapper.php +++ b/lib/Model/AttendeeMapper.php @@ -301,6 +301,7 @@ public function createAttendeeFromRow(array $row): Attendee { 'call_id' => $row['call_id'], 'state' => (int) $row['state'], 'unread_messages' => (int) $row['unread_messages'], + 'last_attendee_activity' => (int) $row['last_attendee_activity'], ]); } } diff --git a/lib/Model/SelectHelper.php b/lib/Model/SelectHelper.php index 537992db42e..978178b3462 100644 --- a/lib/Model/SelectHelper.php +++ b/lib/Model/SelectHelper.php @@ -90,6 +90,7 @@ public function selectAttendeesTable(IQueryBuilder $query, string $alias = 'a'): ->addSelect($alias . 'call_id') ->addSelect($alias . 'state') ->addSelect($alias . 'unread_messages') + ->addSelect($alias . 'last_attendee_activity') ->selectAlias($alias . 'id', 'a_id'); } diff --git a/lib/Service/ParticipantService.php b/lib/Service/ParticipantService.php index ed4c6a8880e..638196a8319 100644 --- a/lib/Service/ParticipantService.php +++ b/lib/Service/ParticipantService.php @@ -145,6 +145,7 @@ public function updateParticipantType(Room $room, Participant $participant, int $attendee->setPermissions(Attendee::PERMISSIONS_DEFAULT); } + $attendee->setLastAttendeeActivity($this->timeFactory->getTime()); $this->attendeeMapper->update($attendee); // XOR so we don't move the participant in and out when they are changed from moderator to owner or vice-versa @@ -232,6 +233,7 @@ public function updatePermissions(Room $room, Participant $participant, string $ if ($attendee->getParticipantType() === Participant::USER_SELF_JOINED) { $attendee->setParticipantType(Participant::USER); } + $attendee->setLastAttendeeActivity($this->timeFactory->getTime()); $this->attendeeMapper->update($attendee); $event = new ParticipantModifiedEvent($room, $participant, AParticipantModifiedEvent::PROPERTY_PERMISSIONS, $newPermissions, $oldPermissions); @@ -247,6 +249,7 @@ public function updateAllPermissions(Room $room, string $method, int $newState): public function updateLastReadMessage(Participant $participant, int $lastReadMessage): void { $attendee = $participant->getAttendee(); $attendee->setLastReadMessage($lastReadMessage); + $attendee->setLastAttendeeActivity($this->timeFactory->getTime()); $this->attendeeMapper->update($attendee); } @@ -256,12 +259,14 @@ public function updateUnreadInfoForProxyParticipant(Participant $participant, in $attendee->setLastReadMessage($lastReadMessageId); $attendee->setLastMentionMessage($hasMention ? 1 : 0); $attendee->setLastMentionDirect($hadDirectMention ? 1 : 0); + $attendee->setLastAttendeeActivity($this->timeFactory->getTime()); $this->attendeeMapper->update($attendee); } public function updateFavoriteStatus(Participant $participant, bool $isFavorite): void { $attendee = $participant->getAttendee(); $attendee->setFavorite($isFavorite); + $attendee->setLastAttendeeActivity($this->timeFactory->getTime()); $this->attendeeMapper->update($attendee); } @@ -281,6 +286,7 @@ public function updateNotificationLevel(Participant $participant, int $level): v $attendee = $participant->getAttendee(); $attendee->setNotificationLevel($level); + $attendee->setLastAttendeeActivity($this->timeFactory->getTime()); $this->attendeeMapper->update($attendee); } @@ -298,6 +304,7 @@ public function updateNotificationCalls(Participant $participant, int $level): v $attendee = $participant->getAttendee(); $attendee->setNotificationCalls($level); + $attendee->setLastAttendeeActivity($this->timeFactory->getTime()); $this->attendeeMapper->update($attendee); } @@ -794,6 +801,7 @@ public function generatePinForParticipant(Room $room, Participant $participant): && ($attendee->getActorType() === Attendee::ACTOR_USERS || $attendee->getActorType() === Attendee::ACTOR_EMAILS) && !$attendee->getPin()) { $attendee->setPin($this->generatePin()); + $attendee->setLastAttendeeActivity($this->timeFactory->getTime()); $this->attendeeMapper->update($attendee); } } @@ -1330,6 +1338,7 @@ public function resetChatDetails(Room $room): void { ->set('last_read_message', $update->createNamedParameter(0, IQueryBuilder::PARAM_INT)) ->set('last_mention_message', $update->createNamedParameter(0, IQueryBuilder::PARAM_INT)) ->set('last_mention_direct', $update->createNamedParameter(0, IQueryBuilder::PARAM_INT)) + ->set('last_attendee_activity', $update->createNamedParameter($this->timeFactory->getTime(), IQueryBuilder::PARAM_INT)) ->where($update->expr()->eq('room_id', $update->createNamedParameter($room->getId(), IQueryBuilder::PARAM_INT))); $update->executeStatement(); } diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index 26822f9334e..544739cf561 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -73,6 +73,8 @@ class FeatureContext implements Context, SnippetAcceptingContext { protected static array $phoneNumberToActorId; /** @var array|null */ protected static ?array $nextChatRequestParameters = null; + /** @var array */ + protected static array $modifiedSince; protected static array $permissionsMap = [ @@ -185,6 +187,7 @@ public function setUp() { self::$questionToPollId = []; self::$lastNotifications = []; self::$phoneNumberToActorId = []; + self::$modifiedSince = []; $this->createdUsers = []; $this->createdGroups = []; @@ -288,17 +291,26 @@ public function userCanFindListedRoomsWithTerm(string $user, string $term, strin } /** - * @Then /^user "([^"]*)" is participant of the following (unordered )?(note-to-self )?rooms \((v4)\)$/ + * @Then /^user "([^"]*)" is participant of the following (unordered )?(note-to-self )?(modified-since )?rooms \((v4)\)$/ * * @param string $user * @param string $shouldOrder * @param string $apiVersion * @param TableNode|null $formData */ - public function userIsParticipantOfRooms(string $user, string $shouldOrder, string $shouldFilter, string $apiVersion, TableNode $formData = null): void { + public function userIsParticipantOfRooms(string $user, string $shouldOrder, string $shouldFilter, string $modifiedSince, string $apiVersion, TableNode $formData = null): void { + $parameters = ''; + if ($modifiedSince !== '') { + if (!isset(self::$modifiedSince[$user])) { + throw new \RuntimeException('Must run once without "modified-since" before'); + } + $parameters .= '?modifiedSince=' . self::$modifiedSince[$user]; + } + $this->setCurrentUser($user); - $this->sendRequest('GET', '/apps/spreed/api/' . $apiVersion . '/room'); + $this->sendRequest('GET', '/apps/spreed/api/' . $apiVersion . '/room' . $parameters); $this->assertStatusCode($this->response, 200); + self::$modifiedSince[$user] = time(); $rooms = $this->getDataFromResponse($this->response); diff --git a/tests/integration/features/chat-2/unread-messages.feature b/tests/integration/features/chat-2/unread-messages.feature index 17032492c2d..ca91b95e474 100644 --- a/tests/integration/features/chat-2/unread-messages.feature +++ b/tests/integration/features/chat-2/unread-messages.feature @@ -148,9 +148,24 @@ Feature: chat-2/unread-messages And user "participant1" sends message "Message 1" to room "group room" with 201 And user "participant1" sends message "Message 2" to room "group room" with 201 And user "participant1" sends message "Message 3" to room "group room" with 201 + Then user "participant2" is participant of the following rooms (v4) + | id | unreadMessages | + | group room | 3 | + And wait for 2 seconds + Then user "participant2" is participant of the following modified-since rooms (v4) And user "participant2" reads message "Message 3" in room "group room" with 200 + And wait for 2 seconds + Then user "participant2" is participant of the following modified-since rooms (v4) + | id | unreadMessages | + | group room | 0 | + Then user "participant2" is participant of the following modified-since rooms (v4) When user "participant1" marks room "group room" as unread with 200 And user "participant2" marks room "group room" as unread with 200 + And wait for 2 seconds + Then user "participant2" is participant of the following modified-since rooms (v4) + | id | unreadMessages | + | group room | 1 | + Then user "participant2" is participant of the following modified-since rooms (v4) Then user "participant1" is participant of room "group room" (v4) | unreadMessages | | 1 | diff --git a/tests/php/Chat/ChatManagerTest.php b/tests/php/Chat/ChatManagerTest.php index 92246fd25a9..b5ca34f2c44 100644 --- a/tests/php/Chat/ChatManagerTest.php +++ b/tests/php/Chat/ChatManagerTest.php @@ -449,6 +449,7 @@ public function testDeleteMessage(): void { 'invited_cloud_id' => '', 'state' => Invitation::STATE_ACCEPTED, 'unread_messages' => 0, + 'last_attendee_activity' => 0, ]); $chat = $this->createMock(Room::class); $chat->expects($this->any()) @@ -510,6 +511,7 @@ public function testDeleteMessageFileShare(): void { 'invited_cloud_id' => '', 'state' => Invitation::STATE_ACCEPTED, 'unread_messages' => 0, + 'last_attendee_activity' => 0, ]); $chat = $this->createMock(Room::class); $chat->expects($this->any()) @@ -593,6 +595,7 @@ public function testDeleteMessageFileShareNotFound(): void { 'invited_cloud_id' => '', 'state' => Invitation::STATE_ACCEPTED, 'unread_messages' => 0, + 'last_attendee_activity' => 0, ]); $chat = $this->createMock(Room::class); $chat->expects($this->any())