From 4fdda099a97d0f07dcc3b7b3a892814854cab441 Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Thu, 14 Dec 2023 11:08:59 +0100 Subject: [PATCH 1/4] fix(shares): pass parent message id with metadata when uploading Signed-off-by: Maksim Sukharev (cherry picked from commit 3f443b3e0b4fa6e550f8c8d21bdc0eea420e1565) --- src/components/NewMessage/NewMessage.vue | 3 ++- src/store/fileUploadStore.js | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/components/NewMessage/NewMessage.vue b/src/components/NewMessage/NewMessage.vue index eeead3bcd09..79f1801e55d 100644 --- a/src/components/NewMessage/NewMessage.vue +++ b/src/components/NewMessage/NewMessage.vue @@ -54,7 +54,7 @@
- @@ -517,6 +517,7 @@ export default { if (this.upload) { // Clear input content from store this.$store.dispatch('setCurrentMessageInput', { token: this.token, text: '' }) + this.$store.dispatch('removeMessageToBeReplied', this.token) if (this.$store.getters.getInitialisedUploads(this.$store.getters.currentUploadId).length) { // If dialog contains files to upload, delegate sending diff --git a/src/store/fileUploadStore.js b/src/store/fileUploadStore.js index 5478be2f588..11fc99477e4 100644 --- a/src/store/fileUploadStore.js +++ b/src/store/fileUploadStore.js @@ -375,6 +375,9 @@ const actions = { if (options?.silent) { Object.assign(rawMetadata, { silent: options.silent }) } + if (temporaryMessage.parent) { + Object.assign(rawMetadata, { replyTo: temporaryMessage.parent.id }) + } const metadata = JSON.stringify(rawMetadata) try { From 2f1dc48d07b9162be1e2126ef97564979d14dda1 Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Thu, 14 Dec 2023 11:50:58 +0100 Subject: [PATCH 2/4] fix(shares): retreive parent id from metadata Signed-off-by: Maksim Sukharev (cherry picked from commit b4b2d375cf8a9d05d44c3dbda9219b3c99c314ad) --- lib/Chat/SystemMessage/Listener.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/Chat/SystemMessage/Listener.php b/lib/Chat/SystemMessage/Listener.php index f671dff8a72..3759d472104 100644 --- a/lib/Chat/SystemMessage/Listener.php +++ b/lib/Chat/SystemMessage/Listener.php @@ -455,12 +455,14 @@ protected function sendSystemMessage(Room $room, string $message, array $paramet $referenceId = (string) $referenceId; } + $parent = $parameters['metaData']['replyTo'] ?? null; + return $this->chatManager->addSystemMessage( $room, $actorType, $actorId, json_encode(['message' => $message, 'parameters' => $parameters]), $this->timeFactory->getDateTime(), $message === 'file_shared', $referenceId, - null, + $parent, $shouldSkipLastMessageUpdate, $silent, ); From b5618425f16d09f7b907b99e7f1a78d8c1365c20 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 15 Dec 2023 08:29:50 +0100 Subject: [PATCH 3/4] fix(shares): Verify replying captions are replyable and add tests Signed-off-by: Joas Schilling (cherry picked from commit 9cc8f782ac9322e451c7acdde2921723ae9163b5) --- docs/chat.md | 1 + lib/Chat/SystemMessage/Listener.php | 22 ++++++++++- .../features/bootstrap/FeatureContext.php | 4 ++ .../features/bootstrap/SharingContext.php | 15 ++++++- .../features/chat-1/file-share.feature | 39 +++++++++++++++++++ tests/php/Chat/SystemMessage/ListenerTest.php | 13 +++++++ 6 files changed, 91 insertions(+), 3 deletions(-) diff --git a/docs/chat.md b/docs/chat.md index 3c9fe3bbe14..b9ad01e7213 100644 --- a/docs/chat.md +++ b/docs/chat.md @@ -195,6 +195,7 @@ See [OCP\RichObjectStrings\Definitions](https://github.com/nextcloud/server/blob |---------------|--------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | `messageType` | string | A message type to show the message in different styles. Currently known: `voice-message` and `comment` | | `caption` | string | A caption message that should be shown together with the shared file (only available with `media-caption` capability) | +| `replyTo` | int | The message ID this caption message is a reply to (only allowed for messages from the same conversation and when the message type is not `system` or `command`) | | `silent` | bool | If sent silent the message will not create chat notifications even for mentions (only available with `media-caption` capability, yes `media-caption` not `silent-send`) | * Response: [See official OCS Share API docs](https://docs.nextcloud.com/server/latest/developer_manual/client_apis/OCS/ocs-share-api.html?highlight=sharing#create-a-new-share) diff --git a/lib/Chat/SystemMessage/Listener.php b/lib/Chat/SystemMessage/Listener.php index 3759d472104..f35a264a05b 100644 --- a/lib/Chat/SystemMessage/Listener.php +++ b/lib/Chat/SystemMessage/Listener.php @@ -27,6 +27,7 @@ use DateInterval; use OCA\Talk\Chat\ChatManager; +use OCA\Talk\Chat\MessageParser; use OCA\Talk\Events\AAttendeeRemovedEvent; use OCA\Talk\Events\AParticipantModifiedEvent; use OCA\Talk\Events\ARoomModifiedEvent; @@ -51,8 +52,10 @@ use OCA\Talk\Webinary; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Comments\IComment; +use OCP\Comments\NotFoundException; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; +use OCP\IL10N; use OCP\IRequest; use OCP\ISession; use OCP\IUser; @@ -75,6 +78,8 @@ public function __construct( protected ITimeFactory $timeFactory, protected Manager $manager, protected ParticipantService $participantService, + protected MessageParser $messageParser, + protected IL10N $l, ) { } @@ -455,14 +460,27 @@ protected function sendSystemMessage(Room $room, string $message, array $paramet $referenceId = (string) $referenceId; } - $parent = $parameters['metaData']['replyTo'] ?? null; + $replyTo = $parameters['metaData']['replyTo'] ?? null; + if ($replyTo !== null) { + try { + $parentComment = $this->chatManager->getParentComment($room, (string) $replyTo); + $parentMessage = $this->messageParser->createMessage($room, $participant, $parentComment, $this->l); + $this->messageParser->parseMessage($parentMessage); + if (!$parentMessage->isReplyable()) { + $replyTo = null; + } + } catch (NotFoundException) { + $replyTo = null; + } + + } return $this->chatManager->addSystemMessage( $room, $actorType, $actorId, json_encode(['message' => $message, 'parameters' => $parameters]), $this->timeFactory->getDateTime(), $message === 'file_shared', $referenceId, - $parent, + $replyTo, $shouldSkipLastMessageUpdate, $silent, ); diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index 126e152587f..1a4bc71f26b 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -125,6 +125,10 @@ public static function getTokenForIdentifier(string $identifier) { return self::$identifierToToken[$identifier]; } + public static function getMessageIdForText(string $text): int { + return self::$textToMessageId[$text]; + } + public static function getActorIdForPhoneNumber(string $phoneNumber): string { return self::$phoneNumberToActorId[$phoneNumber]; } diff --git a/tests/integration/features/bootstrap/SharingContext.php b/tests/integration/features/bootstrap/SharingContext.php index 151c25b6296..70b47abd69e 100644 --- a/tests/integration/features/bootstrap/SharingContext.php +++ b/tests/integration/features/bootstrap/SharingContext.php @@ -775,15 +775,28 @@ private function userSharesWith(string $user, string $path, string $shareType, s $parameters[] = 'shareType=' . $shareType; $parameters[] = 'shareWith=' . $shareWith; + $talkMetaData = []; + if ($body instanceof TableNode) { foreach ($body->getRowsHash() as $key => $value) { if ($key === 'expireDate' && $value !== 'invalid date') { $value = date('Y-m-d', strtotime($value)); } - $parameters[] = $key . '=' . $value; + if ($key === 'talkMetaData.replyTo') { + $value = FeatureContext::getMessageIdForText($value); + } + if (str_starts_with($key, 'talkMetaData.')) { + $talkMetaData[substr($key, 13)] = $value; + } else { + $parameters[] = $key . '=' . $value; + } } } + if (!empty($talkMetaData)) { + $parameters[] = 'talkMetaData=' . json_encode($talkMetaData); + } + $url .= '?' . implode('&', $parameters); $this->sendingTo('POST', $url); diff --git a/tests/integration/features/chat-1/file-share.feature b/tests/integration/features/chat-1/file-share.feature index 96f4237ee57..0b8b01d001e 100644 --- a/tests/integration/features/chat-1/file-share.feature +++ b/tests/integration/features/chat-1/file-share.feature @@ -42,6 +42,45 @@ Feature: chat/file-share | room | actorType | actorId | actorDisplayName | message | messageParameters | | public room | users | participant1 | participant1-displayname | {mention-user1} | "IGNORE" | + Scenario: Captioned message as a reply + Given user "participant1" creates room "public room" (v4) + | roomType | 3 | + | roomName | room | + And user "participant1" adds user "participant2" to room "public room" with 200 (v4) + And user "participant2" sends message "Message 1" to room "public room" with 201 + Then user "participant1" sees the following messages in room "public room" with 200 + | room | actorType | actorId | actorDisplayName | message | messageParameters | parentMessage | + | public room | users | participant2 | participant2-displayname | Message 1 | [] | | + When user "participant1" shares "welcome.txt" with room "public room" + | talkMetaData.caption | @participant2 | + | talkMetaData.replyTo | Message 1 | + Then user "participant1" sees the following messages in room "public room" with 200 + | room | actorType | actorId | actorDisplayName | message | messageParameters | parentMessage | + | public room | users | participant1 | participant1-displayname | {mention-user1} | "IGNORE" | Message 1 | + | public room | users | participant2 | participant2-displayname | Message 1 | [] | | + + Scenario: Captioned message can not reply cross chats + Given user "participant1" creates room "room1" (v4) + | roomType | 3 | + | roomName | room | + Given user "participant1" creates room "room2" (v4) + | roomType | 3 | + | roomName | room | + And user "participant1" adds user "participant2" to room "room1" with 200 (v4) + And user "participant2" sends message "Message 1" to room "room1" with 201 + Then user "participant1" sees the following messages in room "room1" with 200 + | room | actorType | actorId | actorDisplayName | message | messageParameters | parentMessage | + | room1 | users | participant2 | participant2-displayname | Message 1 | [] | | + When user "participant1" shares "welcome.txt" with room "room2" + | talkMetaData.caption | @participant2 | + | talkMetaData.replyTo | Message 1 | + Then user "participant1" sees the following messages in room "room1" with 200 + | room | actorType | actorId | actorDisplayName | message | messageParameters | parentMessage | + | room1 | users | participant2 | participant2-displayname | Message 1 | [] | | + Then user "participant1" sees the following messages in room "room2" with 200 + | room | actorType | actorId | actorDisplayName | message | messageParameters | parentMessage | + | room2 | users | participant1 | participant1-displayname | {mention-user1} | "IGNORE" | | + Scenario: Can not share a file without chat permission Given user "participant1" creates room "public room" (v4) | roomType | 3 | diff --git a/tests/php/Chat/SystemMessage/ListenerTest.php b/tests/php/Chat/SystemMessage/ListenerTest.php index e859ca24297..8667ce3e6bf 100644 --- a/tests/php/Chat/SystemMessage/ListenerTest.php +++ b/tests/php/Chat/SystemMessage/ListenerTest.php @@ -22,6 +22,7 @@ namespace OCA\Talk\Tests\php\Chat\SystemMessage; use OCA\Talk\Chat\ChatManager; +use OCA\Talk\Chat\MessageParser; use OCA\Talk\Chat\SystemMessage\Listener; use OCA\Talk\Events\AParticipantModifiedEvent; use OCA\Talk\Events\ARoomModifiedEvent; @@ -37,6 +38,7 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\Comments\IComment; use OCP\EventDispatcher\IEventDispatcher; +use OCP\IL10N; use OCP\IRequest; use OCP\ISession; use OCP\IUser; @@ -71,6 +73,8 @@ class ListenerTest extends TestCase { protected $manager; /** @var ParticipantService|MockObject */ protected $participantService; + /** @var MessageParser|MockObject */ + protected $messageParser; protected ?array $handlers = null; protected ?\DateTime $dummyTime = null; @@ -94,6 +98,13 @@ protected function setUp(): void { $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->manager = $this->createMock(Manager::class); $this->participantService = $this->createMock(ParticipantService::class); + $this->messageParser = $this->createMock(MessageParser::class); + $l = $this->createMock(IL10N::class); + $l->expects($this->any()) + ->method('t') + ->willReturnCallback(function ($string, $args) { + return vsprintf($string, $args); + }); $this->handlers = []; @@ -112,6 +123,8 @@ protected function setUp(): void { $this->timeFactory, $this->manager, $this->participantService, + $this->messageParser, + $l, ); } From 68f01947d524c99c354a309a5ed7ea2fa50343fa Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 15 Dec 2023 09:37:11 +0100 Subject: [PATCH 4/4] feat(shares): Add notifications for caption mentions and replies Signed-off-by: Joas Schilling (cherry picked from commit bfa2c83fcee9d6b510b608b1c0474fb6c6cf0afb) --- lib/Chat/ChatManager.php | 37 ++++++++++++++-- lib/Chat/ReactionManager.php | 4 +- lib/Chat/SystemMessage/Listener.php | 11 ++--- .../features/chat-1/notifications.feature | 42 +++++++++++++++++++ tests/php/Chat/ChatManagerTest.php | 4 +- 5 files changed, 85 insertions(+), 13 deletions(-) diff --git a/lib/Chat/ChatManager.php b/lib/Chat/ChatManager.php index 6ef082b4e46..eef6972066d 100644 --- a/lib/Chat/ChatManager.php +++ b/lib/Chat/ChatManager.php @@ -140,7 +140,7 @@ public function addSystemMessage( \DateTime $creationDateTime, bool $sendNotifications, ?string $referenceId = null, - ?int $parentId = null, + ?IComment $replyTo = null, bool $shouldSkipLastMessageUpdate = false, bool $silent = false, ): IComment { @@ -153,8 +153,8 @@ public function addSystemMessage( $comment->setReferenceId($referenceId); } } - if ($parentId !== null) { - $comment->setParentId((string) $parentId); + if ($replyTo !== null) { + $comment->setParentId($replyTo->getId()); } $messageDecoded = json_decode($message, true); @@ -168,6 +168,8 @@ public function addSystemMessage( $this->setMessageExpiration($chat, $comment); + $shouldFlush = $this->notificationManager->defer(); + $event = new BeforeSystemMessageSentEvent($chat, $comment, silent: $silent, skipLastActivityUpdate: $shouldSkipLastMessageUpdate); $this->dispatcher->dispatchTyped($event); $event = new ChatEvent($chat, $comment, $shouldSkipLastMessageUpdate, $silent); @@ -182,6 +184,29 @@ public function addSystemMessage( } if ($sendNotifications) { + /** @var ?IComment $captionComment */ + $captionComment = null; + $alreadyNotifiedUsers = $usersDirectlyMentioned = []; + if ($messageType === 'file_shared') { + if (isset($messageDecoded['parameters']['metaData']['caption'])) { + $captionComment = clone $comment; + $captionComment->setMessage($messageDecoded['parameters']['metaData']['caption'], self::MAX_CHAT_LENGTH); + $usersDirectlyMentioned = $this->notifier->getMentionedUserIds($captionComment); + } + if ($replyTo instanceof IComment) { + $alreadyNotifiedUsers = $this->notifier->notifyReplyToAuthor($chat, $comment, $replyTo, $silent); + if ($replyTo->getActorType() === Attendee::ACTOR_USERS) { + $usersDirectlyMentioned[] = $replyTo->getActorId(); + } + } + } + + $alreadyNotifiedUsers = $this->notifier->notifyMentionedUsers($chat, $captionComment ?? $comment, $alreadyNotifiedUsers, $silent); + if (!empty($alreadyNotifiedUsers)) { + $userIds = array_column($alreadyNotifiedUsers, 'id'); + $this->participantService->markUsersAsMentioned($chat, $userIds, (int) $comment->getId(), $usersDirectlyMentioned); + } + $this->notifier->notifyOtherParticipant($chat, $comment, [], $silent); } @@ -203,6 +228,10 @@ public function addSystemMessage( } $this->cache->remove($chat->getToken()); + if ($shouldFlush) { + $this->notificationManager->flush(); + } + if ($messageType === 'object_shared' || $messageType === 'file_shared') { $this->attachmentService->createAttachmentEntry($chat, $comment, $messageType, $messageDecoded['parameters'] ?? []); } @@ -466,7 +495,7 @@ public function deleteMessage(Room $chat, IComment $comment, Participant $partic $this->timeFactory->getDateTime(), false, null, - (int) $comment->getId(), + $comment, true ); } diff --git a/lib/Chat/ReactionManager.php b/lib/Chat/ReactionManager.php index e825d7ec6ab..b1f174b6e8e 100644 --- a/lib/Chat/ReactionManager.php +++ b/lib/Chat/ReactionManager.php @@ -110,7 +110,7 @@ public function addReactionMessage(Room $chat, string $actorType, string $actorI */ public function deleteReactionMessage(Room $chat, string $actorType, string $actorId, int $messageId, string $reaction): IComment { // Just to verify that messageId is part of the room and throw error if not. - $this->getCommentToReact($chat, (string) $messageId); + $parentComment = $this->getCommentToReact($chat, (string) $messageId); $comment = $this->commentsManager->getReactionComment( $messageId, @@ -136,7 +136,7 @@ public function deleteReactionMessage(Room $chat, string $actorType, string $act $this->timeFactory->getDateTime(), false, null, - $messageId, + $parentComment, true ); diff --git a/lib/Chat/SystemMessage/Listener.php b/lib/Chat/SystemMessage/Listener.php index f35a264a05b..80af7f69b64 100644 --- a/lib/Chat/SystemMessage/Listener.php +++ b/lib/Chat/SystemMessage/Listener.php @@ -460,17 +460,17 @@ protected function sendSystemMessage(Room $room, string $message, array $paramet $referenceId = (string) $referenceId; } + $parent = null; $replyTo = $parameters['metaData']['replyTo'] ?? null; if ($replyTo !== null) { try { $parentComment = $this->chatManager->getParentComment($room, (string) $replyTo); $parentMessage = $this->messageParser->createMessage($room, $participant, $parentComment, $this->l); $this->messageParser->parseMessage($parentMessage); - if (!$parentMessage->isReplyable()) { - $replyTo = null; + if ($parentMessage->isReplyable()) { + $parent = $parentComment; } } catch (NotFoundException) { - $replyTo = null; } } @@ -478,9 +478,10 @@ protected function sendSystemMessage(Room $room, string $message, array $paramet return $this->chatManager->addSystemMessage( $room, $actorType, $actorId, json_encode(['message' => $message, 'parameters' => $parameters]), - $this->timeFactory->getDateTime(), $message === 'file_shared', + $this->timeFactory->getDateTime(), + $message === 'file_shared', $referenceId, - $replyTo, + $parent, $shouldSkipLastMessageUpdate, $silent, ); diff --git a/tests/integration/features/chat-1/notifications.feature b/tests/integration/features/chat-1/notifications.feature index 0549f2b4df9..8d66570e29a 100644 --- a/tests/integration/features/chat-1/notifications.feature +++ b/tests/integration/features/chat-1/notifications.feature @@ -373,6 +373,48 @@ Feature: chat/notifications | app | object_type | object_id | subject | | spreed | chat | room/Hi @all @participant2 @"group/attendees1" bye | participant1-displayname replied to your message in conversation room | + Scenario: Replying with a captioned file gives a reply notification + When 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" adds group "attendees1" to room "room" with 200 (v4) + # Join and leave to clear the invite notification + Given user "participant2" joins room "room" with 200 (v4) + Given user "participant2" leaves room "room" with 200 (v4) + When user "participant2" sends message "Message 1" to room "room" with 201 + Then user "participant1" sees the following messages in room "room" with 200 + | room | actorType | actorId | actorDisplayName | message | messageParameters | parentMessage | + | room | users | participant2 | participant2-displayname | Message 1 | [] | | + When user "participant1" shares "welcome.txt" with room "room" + | talkMetaData.caption | Caption 1-1 | + | talkMetaData.replyTo | Message 1 | + Then user "participant1" sees the following messages in room "room" with 200 + | room | actorType | actorId | actorDisplayName | message | messageParameters | parentMessage | + | room | users | participant1 | participant1-displayname | Caption 1-1 | "IGNORE" | Message 1 | + | room | users | participant2 | participant2-displayname | Message 1 | [] | | + Then user "participant2" has the following notifications + | app | object_type | object_id | subject | + | spreed | chat | room/Caption 1-1 | participant1-displayname replied to your message in conversation room | + + Scenario: Mentions in captions trigger normal mention notifications + When 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" adds group "attendees1" to room "room" with 200 (v4) + # Join and leave to clear the invite notification + Given user "participant2" joins room "room" with 200 (v4) + Given user "participant2" leaves room "room" with 200 (v4) + When user "participant1" shares "welcome.txt" with room "room" + | talkMetaData.caption | @participant2 | + Then user "participant1" sees the following messages in room "room" with 200 + | room | actorType | actorId | actorDisplayName | message | messageParameters | parentMessage | + | room | users | participant1 | participant1-displayname | {mention-user1} | "IGNORE" | | + Then user "participant2" has the following notifications + | app | object_type | object_id | subject | + | spreed | chat | room/{mention-user1} | participant1-displayname mentioned you in conversation room | + Scenario: Delete notification when the message is deleted When user "participant1" creates room "one-to-one room" (v4) | roomType | 1 | diff --git a/tests/php/Chat/ChatManagerTest.php b/tests/php/Chat/ChatManagerTest.php index 99fd88e2bb0..cd05ddaaa48 100644 --- a/tests/php/Chat/ChatManagerTest.php +++ b/tests/php/Chat/ChatManagerTest.php @@ -473,7 +473,7 @@ public function testDeleteMessage(): void { $chatManager = $this->getManager(['addSystemMessage']); $chatManager->expects($this->once()) ->method('addSystemMessage') - ->with($chat, Attendee::ACTOR_USERS, 'user', $this->anything(), $this->anything(), false, null, 123456) + ->with($chat, Attendee::ACTOR_USERS, 'user', $this->anything(), $this->anything(), false, null, $comment) ->willReturn($systemMessage); $this->assertSame($systemMessage, $chatManager->deleteMessage($chat, $comment, $participant, $date)); @@ -553,7 +553,7 @@ public function testDeleteMessageFileShare(): void { $chatManager = $this->getManager(['addSystemMessage']); $chatManager->expects($this->once()) ->method('addSystemMessage') - ->with($chat, Attendee::ACTOR_USERS, 'user', $this->anything(), $this->anything(), false, null, 123456) + ->with($chat, Attendee::ACTOR_USERS, 'user', $this->anything(), $this->anything(), false, null, $comment) ->willReturn($systemMessage); $this->assertSame($systemMessage, $chatManager->deleteMessage($chat, $comment, $participant, $date));