fix(chat): Better handling of captioned messages in federated conversationsbugfix/noid/improve-message-when-sharing-a-file

Signed-off-by: Joas Schilling <coding@schilljs.com>

3 files changed, 39 insertions, 29 deletions

diff --git a/lib/Chat/Parser/SystemMessage.php b/lib/Chat/Parser/SystemMessage.php
index ddea3bd34..c5cf51d91 100644
--- a/lib/Chat/Parser/SystemMessage.php
+++ b/lib/Chat/Parser/SystemMessage.php
@@ -485,7 +485,7 @@ class SystemMessage implements IEventListener {
 			}
 		} 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,19 @@ class SystemMessage implements IEventListener {
 				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');
+				}
+				$parsedMessage = '*' . $parsedMessage . '*';
+
+				$metaData = $parameters['metaData'] ?? [];
+				if (isset($metaData['caption']) && $metaData['caption'] !== '') {
+					$parsedMessage .= "\n\n" . $metaData['caption'];
 				}
 			}
 		} elseif ($message === 'object_shared') {
@@ -724,10 +733,10 @@ class SystemMessage implements IEventListener {
 	 * @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 +776,8 @@ class SystemMessage implements IEventListener {
 			$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 d5d472001..d1fccd56b 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 57921a0db..f44a4955f 100644
--- a/tests/php/Chat/Parser/SystemMessageTest.php
+++ b/tests/php/Chat/Parser/SystemMessageTest.php
@@ -356,11 +356,11 @@ class SystemMessageTest extends TestCase {
 				['actor' => ['id' => 'actor', 'type' => 'user'], 'file' => ['id' => 'file-from-share']],
 			],
 			['file_shared', ['share' => InvalidPathException::class], 'recipient',
-				'{actor} shared a file which is no longer available',
+				'*{actor} shared a file which is no longer available*',
 				['actor' => ['id' => 'actor', 'type' => 'user']],
 			],
 			['file_shared', ['share' => NotFoundException::class], 'actor',
-				'You shared a file which is no longer available',
+				'*You shared a file which is no longer available*',
 				['actor' => ['id' => 'actor', 'type' => 'user']],
 			],
 			['read_only', [], 'recipient',
@@ -525,12 +525,12 @@ class SystemMessageTest extends TestCase {
 			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 @@ class SystemMessageTest extends TestCase {
 	}
 
 	public function testGetFileFromShareForGuest(): void {
+		$room = $this->createMock(Room::class);
 		$node = $this->createMock(Node::class);
 		$node->expects($this->once())
 			->method('getId')
@@ -639,9 +640,6 @@ class SystemMessageTest extends TestCase {
 			->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 @@ class SystemMessageTest extends TestCase {
 			'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 @@ class SystemMessageTest extends TestCase {
 			->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 @@ class SystemMessageTest extends TestCase {
 			'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 @@ class SystemMessageTest extends TestCase {
 			'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 @@ class SystemMessageTest extends TestCase {
 
 		$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 {