-
Notifications
You must be signed in to change notification settings - Fork 544
fix(conversations): system messages won't bump activity in room / thread order anymore #17977
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
682cd0d
6f7f684
d028ad8
739857c
c08c7aa
f0cb33a
50d3fe3
3ad92d6
d8c57dc
6c80235
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -19,7 +19,7 @@ public function __construct( | |||||||||||||||
| ?Participant $participant = null, | ||||||||||||||||
| bool $silent = false, | ||||||||||||||||
| ?IComment $parent = null, | ||||||||||||||||
| protected bool $skipLastActivityUpdate = false, | ||||||||||||||||
| private bool $skipLastActivityUpdate = true, | ||||||||||||||||
| ) { | ||||||||||||||||
| parent::__construct( | ||||||||||||||||
| $room, | ||||||||||||||||
|
|
@@ -44,4 +44,12 @@ public function __construct( | |||||||||||||||
| public function shouldSkipLastActivityUpdate(): bool { | ||||||||||||||||
| return $this->skipLastActivityUpdate; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /** | ||||||||||||||||
| * public setter for shouldSkipLastAcitvityUpdate | ||||||||||||||||
| * @param bool $pShouldSkipLastActivity | ||||||||||||||||
| */ | ||||||||||||||||
| public function setShouldSkipLastActivityUpdate(bool $pShouldSkipLastActivity) { | ||||||||||||||||
| $this->$skipLastActivityUpdate = $pShouldSkipLastactivity; | ||||||||||||||||
|
Comment on lines
+50
to
+53
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,7 +21,7 @@ class AttendeesAddedEvent extends AttendeesEvent { | |||||||||
| public function __construct( | ||||||||||
| Room $room, | ||||||||||
| array $attendees, | ||||||||||
| private readonly bool $skipLastMessageUpdate = false, | ||||||||||
| private bool $skipLastMessageUpdate = true, | ||||||||||
| ) { | ||||||||||
| parent::__construct($room, $attendees); | ||||||||||
| } | ||||||||||
|
|
@@ -30,6 +30,10 @@ public function shouldSkipLastMessageUpdate(): bool { | |||||||||
| return $this->skipLastMessageUpdate; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| public function setShouldSkipLastActivityUpdate(bool $pShouldSkipLastActivityUpdate) { | ||||||||||
| $this->shouldSkipLastMessageUpdate = $pShouldSkipLastActivityUpdate; | ||||||||||
|
Comment on lines
+33
to
+34
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| } | ||||||||||
|
|
||||||||||
| public function setLastMessage(IComment $lastMessage): void { | ||||||||||
| $this->lastMessage = $lastMessage; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
|
|
||
| namespace OCA\Talk\Migration; | ||
|
|
||
| use Closure; | ||
| use OCP\DB\ISchemaWrapper; | ||
| use OCP\DB\Types; | ||
| use OCP\Migration\IOutput; | ||
| use OCP\Migration\SimpleMigrationStep; | ||
| use OCP\DB\QueryBuilder\IQueryBuilder; | ||
| use OCP\IDBConnection; | ||
| use Override; | ||
|
|
||
| class Version24000Date20260510193300 extends SimpleMigrationStep { | ||
|
|
||
| public function __construct( | ||
| private readonly IDBConnection $connection, | ||
| ) { | ||
| } | ||
|
|
||
| /** | ||
| * @param IOutput $output | ||
| * @param Closure(): ISchemaWrapper $schemaClosure | ||
| * @param array $options | ||
| * @return null|ISchemaWrapper | ||
| */ | ||
| #[Override] | ||
| public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { | ||
| /** @var ISchemaWrapper $schema */ | ||
| $schema = $schemaClosure(); | ||
|
|
||
| $table = $schema->getTable('talk_rooms'); | ||
|
|
||
| if (!$table->hasColumn('last_metadata_activity')) { | ||
| $table->addColumn('last_metadata_activity', Types::DATETIME, [ | ||
| 'notnull' => false, | ||
| ]); | ||
| $table->addIndex(['last_metadata_activity'], 'talkroom_lastmetadataactive'); | ||
|
|
||
| } | ||
|
|
||
| return $schema; | ||
| } | ||
|
|
||
| /** | ||
| * @param IOutput $output | ||
| * @param Closure(): ISchemaWrapper $schemaClosure | ||
| * @param array $options | ||
| */ | ||
| #[Override] | ||
| public function postSchemaChange(IOutput $output, \Closure $schemaClosure, array $options) : void { | ||
| $update = $this->connection->getQueryBuilder(); | ||
| $update->update('talk_rooms') | ||
| ->set('last_metadata_activity', 'last_activity'); | ||
| $update->executeStatement(); | ||
| } | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -503,8 +503,10 @@ | |
| * isCustomAvatar: bool, | ||
| * // Flag if the conversation is favorited by the user | ||
| * isFavorite: bool, | ||
| * // Timestamp of the last activity in the conversation, in seconds and UTC time zone | ||
| * // Timestamp of the last message activity in the conversation, in seconds and UTC time zone | ||
| * lastActivity: int, | ||
| * // Timestamp of the last activity (metadata, not messages) in the conversation, in seconds and UTC time zone | ||
| * lastMetadataActivity: int | ||
| * // ID of the last message read by every user that has read privacy set to public in a room. When the user themself has it set to private the value is `0` (only available with `chat-read-status` capability) | ||
| * lastCommonReadMessage: int, | ||
| * // Last message in a conversation if available, otherwise empty. **Note:** Even when given the message will not contain the `parent` or `reactionsSelf` attribute due to performance reasons | ||
|
|
@@ -725,8 +727,10 @@ | |
| * title: string, | ||
| * // ID of the last message in the thread | ||
| * lastMessageId: non-negative-int, | ||
| * // UNIX timestamp of the last activity in the thread | ||
| * // UNIX timestamp of the last message activity in the thread | ||
| * lastActivity: non-negative-int, | ||
| * // UNIX timestamp of the last metadata, not message, activity in the thread | ||
|
sudormant marked this conversation as resolved.
|
||
| * lastMetadataActivity: int | ||
|
Comment on lines
+732
to
+733
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you reverted the thread change by now, so this sould be reverted as well |
||
| * // Number of replies in the thread | ||
| * numReplies: non-negative-int, | ||
| * } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -737,25 +737,37 @@ public function addUsers(Room $room, array $participants, ?IUser $addedBy = null | |
| $event = new AttendeesAddedEvent($room, $attendees, true); | ||
| $this->dispatcher->dispatchTyped($event); | ||
|
|
||
| $lastMessage = $event->getLastMessage(); | ||
| if ($lastMessage instanceof IComment) { | ||
| $this->updateRoomLastMessage($room, $lastMessage); | ||
| // getLastMessage doesn't exist for all event types, e.g. room creation | ||
| if (($event->getLastMessage() ?? null) !== null) { | ||
| $lastMessage = $event->getLastMessage(); | ||
| // TODO: is there any better getter / way to access message type? | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this whole logic is "local" to the event. so it only contains last messages of adding users and can be applied/skipped unconditionally. |
||
| $lastMessageType = json_decode($lastMessage->getMessage(), true)['message'] ?? ''; | ||
| if ($lastMessage instanceof IComment || !in_array($lastMessageType, ['user_added', 'user_removed', 'moderator_promoted', 'moderator_demoted'], true)) { | ||
| // do not update the room message to the status message, so the conversion / thread will not be bumped by activity to the top | ||
| // left to do is to update the last message in the preview in the room list, without bumping it up. | ||
| } elseif ($lastMessage instanceof IComment) { | ||
| $this->updateRoomLastMessage($room, $lastMessage); | ||
| } | ||
| } | ||
| $now = $this->timeFactory->getDateTime(); | ||
| $room->setLastMetadataActivity($now); | ||
|
|
||
| return $attendees; | ||
| } | ||
|
|
||
| protected function updateRoomLastMessage(Room $room, IComment $message): void { | ||
| /** @var RoomService $roomService */ | ||
| $roomService = Server::get(RoomService::class); | ||
| $roomService->setLastMessage($room, $message); | ||
| $roomService->setLastMessageInfo($room, (int)$message->getId(), $message->getCreationDateTime()); | ||
| $now = $this->timeFactory->getDateTime(); | ||
| $room->setLastMetadataActivity($now); | ||
|
|
||
| $lastMessageCache = $this->cacheFactory->createDistributed(CachePrefix::CHAT_LAST_MESSAGE_ID); | ||
| $lastMessageCache->remove($room->getToken()); | ||
| $unreadCountCache = $this->cacheFactory->createDistributed(CachePrefix::CHAT_UNREAD_COUNT); | ||
| $unreadCountCache->clear($room->getId() . '-'); | ||
|
|
||
| $event = new SystemMessagesMultipleSentEvent($room, $message); | ||
| $event = new SystemMessagesMultipleSentEvent($room, $message, skipLastActivityUpdate: true); | ||
| $this->dispatcher->dispatchTyped($event); | ||
| } | ||
|
|
||
|
|
@@ -1255,6 +1267,9 @@ public function removeUser(Room $room, IUser $user, string $reason): void { | |
| return; | ||
| } | ||
|
|
||
| $now = $this->timeFactory->getDateTime(); | ||
| $room->setLastMetadataActivity($now); | ||
|
|
||
| $attendee = $participant->getAttendee(); | ||
| $sessions = $this->sessionService->getAllSessionsForAttendee($attendee); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we keep this we need to check all the calls in this file. E.g. currently posting a file does not update the last message anymore.
Other cases where I would still bump would be: lobby_none, lobby_timer_reached, breakout_rooms_started, breakout_rooms_stopped, call_left, call_joined, call_started, setCallRecording