fix(conversations): system messages won't bump activity in room / thread order anymore#17977
fix(conversations): system messages won't bump activity in room / thread order anymore#17977sudormant wants to merge 10 commits into
Conversation
…ctivity flag for event dispatcher from false to true. This should help with chat state changes (users removed / added, moderators promoted / demoted) from triggering a reordering in the room list in Nextcloud Talk. Signed-off-by: Christian Lorang <madcatcl2@gmx.de>
…tMetadaActivity for Rooms and Threads, similar to lastActivity. This also adds a database migration for the oc_talk_rooms and oc_talk_threads tables as well. Functions are introduced and prepared for later use. The use of the field lastActivity to signal when the last real message in a conversation appeared remains unchanged to keep the API stable. The intended use of this feature is to better distinguish between real messages (lastActivity) to notify and bump conversations in the thread list to the top, and other status / metadata related messages (lastMetadataActivity) like room state and participant list changes that shall get synced and be updated in the clients, but not trigger an activity bump of its conversations in the thread list. Signed-off-by: Christian Lorang <madcatcl2@gmx.de>
…t will still go by last message activity, after introducing lastMetadataActivity, and not sort by last metadata changes in rooms like participant list updates. Signed-off-by: Christian Lorang <madcatcl2@gmx.de>
…oms and Threads to use / set lastMetadataActivity instaed of lastActivity where appropriate, i.e. where system messages are signalled and not real chat messages. Signed-off-by: Christian Lorang <madcatcl2@gmx.de>
: system messages like user added or removed, moderator promoted or demoted trigger an activity bump of the conversation in the list and move it to the top. This fix keeps activity bumps for real chat messages but won't bump the activity anymore for the aforementioned system messages. This fix also incorporates use of the new lastMetadataActivity field to update this information instead of lastActivity accordingly and can be expanded later on to handle other kind of messages differently. No changes client side are needed. Signed-off-by: Christian Lorang <madcatcl2@gmx.de>
nickvergessen
added
bug
feature: chat 💬
nickvergessen
left a comment
nickvergessen
left a comment
There was a problem hiding this comment.
I don't (with a quick look) see an issue with the thread handling at the moment. So in terms of separation of concern, I would simply skip that for now to keep the scope of the PR smaller.
If there is a similar/further issue with threads in the same way, feel free to outline steps, I tried editing, deleting a message and doing a reaction, but nothing bumped the thread unexpectingly?
| if (isset($host['lastMetadataActivity']) && $host['lastMetadataActivity'] !== 0 && $host['lastMetadataActivity'] !== ((int)$local->getLastMetadataActivity()?->getTimestamp())) { | ||
| $lastMetadataActivity = $this->timeFactory->getDateTime('@' . $host['lastMetadataActivity']); | ||
| $this->setLastMetadataActivity($local, $lastMetadataActivity); | ||
| $changed[] = ARoomSyncEvent::PROPERTY_LAST_METADATA_ACTIVITY; |
There was a problem hiding this comment.
Would need handling in OCA\Talk\Federation\CloudFederationProviderTalk
| } | ||
|
|
||
| protected function attendeesAddedEvent(AttendeesAddedEvent $event): void { | ||
| $event->shouldSkipLastMessageUpdate = true; |
There was a problem hiding this comment.
A parallel PR made this read-only and private, so I guess we need to drop read-only and add a setter
There was a problem hiding this comment.
I committed another change to use getter / setters in this place.
I removed readonly also but kept it private, so this should now be compatible to the other pull request?
Signed-off-by: Christian Lorang <madcatcl2@gmx.de>
…s only to rooms, remove same handling from threads Signed-off-by: Christian Lorang <madcatcl2@gmx.de>
…ssageUpdate to be compatible with another pull request making this field private Signed-off-by: Christian Lorang <madcatcl2@gmx.de>
…e (like room creation) when deciding whether to bump activity Signed-off-by: Christian Lorang <madcatcl2@gmx.de>
Co-authored-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com> Signed-off-by: sudormant <madcatcl2@gmx.de>
Yes, there were no problems with the activity handling in Threads or federated Threads. I had found it prudent to adapt all areas using lastActivity markers to handle the new split situation with lastActivity and lastMetadataactivity to keep the code and behavior going forward consistent. When testing my changes removing the aforementioned parts for Threads I also caught a regression and added another commit for it: $event doesn't have getLastMessage for all event types, e.g. room creation, and handled it for now. |
| * @param bool $pShouldSkipLastActivity | ||
| */ | ||
| public function setShouldSkipLastActivityUpdate(bool $pShouldSkipLastActivity) { | ||
| $this->$skipLastActivityUpdate = $pShouldSkipLastactivity; |
There was a problem hiding this comment.
| * @param bool $pShouldSkipLastActivity | |
| */ | |
| public function setShouldSkipLastActivityUpdate(bool $pShouldSkipLastActivity) { | |
| $this->$skipLastActivityUpdate = $pShouldSkipLastactivity; | |
| */ | |
| public function setShouldSkipLastActivityUpdate(bool $shouldSkipLastActivity) { | |
| $this->skipLastActivityUpdate = $shouldSkipLastActivity; |
| public function setShouldSkipLastActivityUpdate(bool $pShouldSkipLastActivityUpdate) { | ||
| $this->shouldSkipLastMessageUpdate = $pShouldSkipLastActivityUpdate; |
There was a problem hiding this comment.
| public function setShouldSkipLastActivityUpdate(bool $pShouldSkipLastActivityUpdate) { | |
| $this->shouldSkipLastMessageUpdate = $pShouldSkipLastActivityUpdate; | |
| public function setShouldSkipLastMessageUpdate(bool $shouldSkipLastMessageUpdate) { | |
| $this->shouldSkipLastMessageUpdate = $shouldSkipLastMessageUpdate; |
| array $parameters = [], | ||
| ?Participant $participant = null, | ||
| bool $shouldSkipLastMessageUpdate = false, | ||
| bool $shouldSkipLastMessageUpdate = true, |
There was a problem hiding this comment.
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
| * // UNIX timestamp of the last metadata, not message, activity in the thread | ||
| * lastMetadataActivity: int |
There was a problem hiding this comment.
you reverted the thread change by now, so this sould be reverted as well
| $thread->setId($threadId); | ||
| $thread->setName($title); | ||
| $thread->setRoomId($room->getId()); | ||
| $thread->setLastActivity($this->timeFactory->getDateTime()); |
| // 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? |
There was a problem hiding this comment.
this whole logic is "local" to the event. so it only contains last messages of adding users and can be applied/skipped unconditionally.
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
2 participants
☑️ Resolves
AI (if applicable)
-> none used
UI
🛠️ API Checklist
Test Plan
🏁 Checklist
docs/has been updated or is not requiredThis Pull request solves the issue in #11882:
Currently, system messages like user_added, user_removed, moderator_promoted, moderator_demoted bumped activity / reordered the room / thread list on each update.
With these changes, system messages are still signaled to the client, but won't bump the activity anymore.
This also introduces a differentiation between lastActivity and a new attribute lastMetadaActivity. lastActivity will still be updated on new chat messages, all other state changes like participant list will only change lastMetadataActivity. Later on this can be useful to handle different kinds of events independently on the backend.
The API to the frontend remains unchanged and there are no changes necessary client-side.
I tested it in my local development setup, @nickvergessen please review & check if the changes now reflect the desired behavior or if we need further changes. Thanks!