From 8c5e5aceb84948e6d6496dec183905990f0787b9 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 3 May 2024 11:25:07 +0200 Subject: [PATCH] fix(chat): Better handling of captioned messages in federated conversations Signed-off-by: Joas Schilling --- lib/Chat/Parser/SystemMessage.php | 18 +++++++-- .../features/sharing-1/delete.feature | 7 ++-- tests/php/Chat/Parser/SystemMessageTest.php | 38 +++++++++---------- 3 files changed, 36 insertions(+), 27 deletions(-) diff --git a/lib/Chat/Parser/SystemMessage.php b/lib/Chat/Parser/SystemMessage.php index ddea3bd344a4..e38c833e23f4 100644 --- a/lib/Chat/Parser/SystemMessage.php +++ b/lib/Chat/Parser/SystemMessage.php @@ -485,7 +485,7 @@ protected function parseMessage(Message $chatMessage): void { } } elseif ($message === 'file_shared') { try { - $parsedParameters['file'] = $this->getFileFromShare($participant, $parameters['share']); + $parsedParameters['file'] = $this->getFileFromShare($room, $participant, $parameters['share']); $parsedMessage = '{file}'; $metaData = $parameters['metaData'] ?? []; if (isset($metaData['messageType'])) { @@ -505,10 +505,18 @@ protected function parseMessage(Message $chatMessage): void { if (isset($metaData['caption']) && $metaData['caption'] !== '') { $parsedMessage = $metaData['caption']; } - } catch (\Exception $e) { + } catch (\Exception) { + $chatMessage->setMessageType(ChatManager::VERB_MESSAGE); $parsedMessage = $this->l->t('{actor} shared a file which is no longer available'); if ($currentUserIsActor) { $parsedMessage = $this->l->t('You shared a file which is no longer available'); + } elseif ($currentActorType === Attendee::ACTOR_FEDERATED_USERS) { + $parsedMessage = $this->l->t('File shares are currently not supported in federated conversations'); + } + + $metaData = $parameters['metaData'] ?? []; + if (isset($metaData['caption']) && $metaData['caption'] !== '') { + $parsedMessage = '*' . $parsedMessage . "*\n\n" . $metaData['caption']; } } } elseif ($message === 'object_shared') { @@ -724,10 +732,10 @@ protected function parseDeletedMessage(Message $chatMessage): void { * @throws NotFoundException * @throws ShareNotFound */ - protected function getFileFromShare(?Participant $participant, string $shareId): array { + protected function getFileFromShare(Room $room, ?Participant $participant, string $shareId): array { $share = $this->shareProvider->getShareById((int) $shareId); - if ($participant && !$participant->isGuest()) { + if ($participant && $participant->getAttendee()->getActorType() === Attendee::ACTOR_USERS) { if ($share->getShareOwner() !== $participant->getAttendee()->getActorId()) { $userFolder = $this->rootFolder->getUserFolder($participant->getAttendee()->getActorId()); if ($userFolder instanceof Node) { @@ -767,6 +775,8 @@ protected function getFileFromShare(?Participant $participant, string $shareId): $url = $this->url->linkToRouteAbsolute('files.viewcontroller.showFile', [ 'fileid' => $node->getId(), ]); + } elseif ($participant && $room->getType() !== Room::TYPE_PUBLIC && $participant->getAttendee()->getActorType() === Attendee::ACTOR_FEDERATED_USERS) { + throw new ShareNotFound(); } else { $node = $share->getNode(); $name = $node->getName(); diff --git a/tests/integration/features/sharing-1/delete.feature b/tests/integration/features/sharing-1/delete.feature index d5d472001ec5..d1fccd56b2b1 100644 --- a/tests/integration/features/sharing-1/delete.feature +++ b/tests/integration/features/sharing-1/delete.feature @@ -384,7 +384,6 @@ Feature: delete | poll | 0 | | voice | 0 | | recording | 0 | - And user "participant1" sees the following system messages in room "public room" with 200 - | room | actorType | actorId | actorDisplayName | systemMessage | - | public room | users | participant1 | participant1-displayname | file_shared | - | public room | users | participant1 | participant1-displayname | conversation_created | + And user "participant1" sees the following messages in room "public room" with 200 + | room | actorType | actorId | actorDisplayName | message | messageParameters | + | public room | users | participant1 | participant1-displayname | You shared a file which is no longer available | {"actor":{"type":"user","id":"participant1","name":"participant1-displayname"}} | diff --git a/tests/php/Chat/Parser/SystemMessageTest.php b/tests/php/Chat/Parser/SystemMessageTest.php index 57921a0dba23..808d0fcd8a78 100644 --- a/tests/php/Chat/Parser/SystemMessageTest.php +++ b/tests/php/Chat/Parser/SystemMessageTest.php @@ -525,12 +525,12 @@ public function testParseMessage(string $message, array $parameters, ?string $re if (is_subclass_of($parameters['share'], \Exception::class)) { $parser->expects($this->once()) ->method('getFileFromShare') - ->with($participant, $parameters['share']) + ->with($room, $participant, $parameters['share']) ->willThrowException(new $parameters['share']()); } else { $parser->expects($this->once()) ->method('getFileFromShare') - ->with($participant, $parameters['share']) + ->with($room, $participant, $parameters['share']) ->willReturn(['id' => 'file-from-share']); } } else { @@ -588,6 +588,7 @@ public function testParseMessageThrows(?string $return): void { } public function testGetFileFromShareForGuest(): void { + $room = $this->createMock(Room::class); $node = $this->createMock(Node::class); $node->expects($this->once()) ->method('getId') @@ -639,9 +640,6 @@ public function testGetFileFromShareForGuest(): void { ->willReturn(['width' => 1234, 'height' => 4567]); $participant = $this->createMock(Participant::class); - $participant->expects($this->once()) - ->method('isGuest') - ->willReturn(true); $parser = $this->getParser(); $this->assertSame([ @@ -657,10 +655,11 @@ public function testGetFileFromShareForGuest(): void { 'preview-available' => 'yes', 'width' => '1234', 'height' => '4567', - ], self::invokePrivate($parser, 'getFileFromShare', [$participant, '23'])); + ], self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23'])); } public function testGetFileFromShareForOwner(): void { + $room = $this->createMock(Room::class); $node = $this->createMock(Node::class); $node->expects($this->exactly(2)) ->method('getId') @@ -713,9 +712,6 @@ public function testGetFileFromShareForOwner(): void { ->method('getMetadataPhotosSizeForFileId'); $participant = $this->createMock(Participant::class); - $participant->expects($this->once()) - ->method('isGuest') - ->willReturn(false); $attendee = Attendee::fromRow([ 'actor_type' => 'users', 'actor_id' => 'owner', @@ -736,19 +732,17 @@ public function testGetFileFromShareForOwner(): void { 'permissions' => '27', 'mimetype' => 'httpd/unix-directory', 'preview-available' => 'no', - ], self::invokePrivate($parser, 'getFileFromShare', [$participant, '23'])); + ], self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23'])); } public function testGetFileFromShareForRecipient(): void { + $room = $this->createMock(Room::class); $share = $this->createMock(IShare::class); $share->expects($this->any()) ->method('getNodeId') ->willReturn(54); $participant = $this->createMock(Participant::class); - $participant->expects($this->once()) - ->method('isGuest') - ->willReturn(false); $attendee = Attendee::fromRow([ 'actor_type' => 'users', 'actor_id' => 'user', @@ -823,19 +817,17 @@ public function testGetFileFromShareForRecipient(): void { 'permissions' => '27', 'mimetype' => 'application/octet-stream', 'preview-available' => 'no', - ], self::invokePrivate($parser, 'getFileFromShare', [$participant, '23'])); + ], self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23'])); } public function testGetFileFromShareForRecipientThrows(): void { + $room = $this->createMock(Room::class); $share = $this->createMock(IShare::class); $share->expects($this->any()) ->method('getNodeId') ->willReturn(54); $participant = $this->createMock(Participant::class); - $participant->expects($this->once()) - ->method('isGuest') - ->willReturn(false); $attendee = Attendee::fromRow([ 'actor_type' => 'users', 'actor_id' => 'user', @@ -865,19 +857,27 @@ public function testGetFileFromShareForRecipientThrows(): void { $parser = $this->getParser(); $this->expectException(NotFoundException::class); - self::invokePrivate($parser, 'getFileFromShare', [$participant, '23']); + self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23']); } public function testGetFileFromShareThrows(): void { + $room = $this->createMock(Room::class); $this->shareProvider->expects($this->once()) ->method('getShareById') ->with('23') ->willThrowException(new ShareNotFound()); $participant = $this->createMock(Participant::class); + $attendee = Attendee::fromRow([ + 'actor_type' => 'users', + 'actor_id' => 'user', + ]); + $participant->expects($this->any()) + ->method('getAttendee') + ->willReturn($attendee); $parser = $this->getParser(); $this->expectException(ShareNotFound::class); - self::invokePrivate($parser, 'getFileFromShare', [$participant, '23']); + self::invokePrivate($parser, 'getFileFromShare', [$room, $participant, '23']); } public static function dataGetActor(): array {