From 1b4f1b9a63ef9278fbf84d67878c6a483c76e50d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 8 Nov 2018 11:54:41 +0100 Subject: [PATCH] Populate the mention-notification with the actual message Signed-off-by: Joas Schilling --- apps/comments/lib/Notification/Notifier.php | 127 ++++++++++++------ .../tests/Unit/Notification/NotifierTest.php | 79 +++++++++-- 2 files changed, 157 insertions(+), 49 deletions(-) diff --git a/apps/comments/lib/Notification/Notifier.php b/apps/comments/lib/Notification/Notifier.php index ead68840a4f37..2132f05ef884a 100644 --- a/apps/comments/lib/Notification/Notifier.php +++ b/apps/comments/lib/Notification/Notifier.php @@ -24,10 +24,12 @@ namespace OCA\Comments\Notification; +use OCP\Comments\IComment; use OCP\Comments\ICommentsManager; use OCP\Comments\NotFoundException; use OCP\Files\IRootFolder; use OCP\IURLGenerator; +use OCP\IUser; use OCP\IUserManager; use OCP\L10N\IFactory; use OCP\Notification\INotification; @@ -83,14 +85,14 @@ public function prepare(INotification $notification, $languageCode) { $l = $this->l10nFactory->get('comments', $languageCode); $displayName = $comment->getActorId(); $isDeletedActor = $comment->getActorType() === ICommentsManager::DELETED_USER; - if($comment->getActorType() === 'users') { + if ($comment->getActorType() === 'users') { $commenter = $this->userManager->get($comment->getActorId()); - if(!is_null($commenter)) { + if ($commenter instanceof IUser) { $displayName = $commenter->getDisplayName(); } } - switch($notification->getSubject()) { + switch ($notification->getSubject()) { case 'mention': $parameters = $notification->getSubjectParameters(); if($parameters[0] !== 'files') { @@ -103,47 +105,38 @@ public function prepare(INotification $notification, $languageCode) { } $node = $nodes[0]; + $path = rtrim($node->getPath(), '/'); + if (strpos($path, '/' . $notification->getUser() . '/files/') === 0) { + // Remove /user/files/... + $fullPath = $path; + list(,,, $path) = explode('/', $fullPath, 4); + } + $subjectParameters = [ + 'file' => [ + 'type' => 'file', + 'id' => $comment->getObjectId(), + 'name' => $node->getName(), + 'path' => $path, + 'link' => $this->url->linkToRouteAbsolute('files.viewcontroller.showFile', ['fileid' => $comment->getObjectId()]), + ], + ]; + if ($isDeletedActor) { - $notification->setParsedSubject($l->t( - 'You were mentioned on “%s”, in a comment by a user that has since been deleted', - [$node->getName()] - )) - ->setRichSubject( - $l->t('You were mentioned on “{file}”, in a comment by a user that has since been deleted'), - [ - 'file' => [ - 'type' => 'file', - 'id' => $comment->getObjectId(), - 'name' => $node->getName(), - 'path' => $node->getPath(), - 'link' => $this->url->linkToRouteAbsolute('files.viewcontroller.showFile', ['fileid' => $comment->getObjectId()]), - ], - ] - ); + $subject = $l->t('You were mentioned on “{file}”, in a comment by a user that has since been deleted'); } else { - $notification->setParsedSubject($l->t( - '%1$s mentioned you in a comment on “%2$s”', - [$displayName, $node->getName()] - )) - ->setRichSubject( - $l->t('{user} mentioned you in a comment on “{file}”'), - [ - 'user' => [ - 'type' => 'user', - 'id' => $comment->getActorId(), - 'name' => $displayName, - ], - 'file' => [ - 'type' => 'file', - 'id' => $comment->getObjectId(), - 'name' => $node->getName(), - 'path' => $node->getPath(), - 'link' => $this->url->linkToRouteAbsolute('files.viewcontroller.showFile', ['fileid' => $comment->getObjectId()]), - ], - ] - ); + $subject = $l->t('{user} mentioned you in a comment on “{file}”'); + $subjectParameters['user'] = [ + 'type' => 'user', + 'id' => $comment->getActorId(), + 'name' => $displayName, + ]; } - $notification->setIcon($this->url->getAbsoluteURL($this->url->imagePath('core', 'actions/comment.svg'))) + list($message, $messageParameters) = $this->commentToRichMessage($comment); + $notification->setRichSubject($subject, $subjectParameters) + ->setParsedSubject($this->richToParsed($subject, $subjectParameters)) + ->setRichMessage($message, $messageParameters) + ->setParsedMessage($this->richToParsed($message, $messageParameters)) + ->setIcon($this->url->getAbsoluteURL($this->url->imagePath('core', 'actions/comment.svg'))) ->setLink($this->url->linkToRouteAbsolute( 'comments.Notifications.view', ['id' => $comment->getId()]) @@ -155,6 +148,58 @@ public function prepare(INotification $notification, $languageCode) { default: throw new \InvalidArgumentException('Invalid subject'); } + } + + public function commentToRichMessage(IComment $comment): array { + $message = $comment->getMessage(); + $messageParameters = []; + $mentionTypeCount = []; + $mentions = $comment->getMentions(); + foreach ($mentions as $mention) { + if ($mention['type'] === 'user') { + $user = $this->userManager->get($mention['id']); + if (!$user instanceof IUser) { + continue; + } + } + if (!array_key_exists($mention['type'], $mentionTypeCount)) { + $mentionTypeCount[$mention['type']] = 0; + } + $mentionTypeCount[$mention['type']]++; + // To keep a limited character set in parameter IDs ([a-zA-Z0-9-]) + // the mention parameter ID does not include the mention ID (which + // could contain characters like '@' for user IDs) but a one-based + // index of the mentions of that type. + $mentionParameterId = 'mention-' . $mention['type'] . $mentionTypeCount[$mention['type']]; + $message = str_replace('@' . $mention['id'], '{' . $mentionParameterId . '}', $message); + try { + $displayName = $this->commentsManager->resolveDisplayName($mention['type'], $mention['id']); + } catch (\OutOfBoundsException $e) { + // There is no registered display name resolver for the mention + // type, so the client decides what to display. + $displayName = ''; + } + $messageParameters[$mentionParameterId] = [ + 'type' => $mention['type'], + 'id' => $mention['id'], + 'name' => $displayName + ]; + } + return [$message, $messageParameters]; + } + public function richToParsed(string $message, array $parameters): string { + $placeholders = $replacements = []; + foreach ($parameters as $placeholder => $parameter) { + $placeholders[] = '{' . $placeholder . '}'; + if ($parameter['type'] === 'user') { + $replacements[] = '@' . $parameter['name']; + } else if ($parameter['type'] === 'file') { + $replacements[] = $parameter['path']; + } else { + $replacements[] = $parameter['name']; + } + } + return str_replace($placeholders, $replacements, $message); } } diff --git a/apps/comments/tests/Unit/Notification/NotifierTest.php b/apps/comments/tests/Unit/Notification/NotifierTest.php index 07dcbfdd8490c..6eceed44919f0 100644 --- a/apps/comments/tests/Unit/Notification/NotifierTest.php +++ b/apps/comments/tests/Unit/Notification/NotifierTest.php @@ -93,13 +93,15 @@ protected function setUp() { public function testPrepareSuccess() { $fileName = 'Gre\'thor.odp'; $displayName = 'Huraga'; - $message = 'Huraga mentioned you in a comment on “Gre\'thor.odp”'; + $message = '@Huraga mentioned you in a comment on “Gre\'thor.odp”'; /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $user */ $user = $this->createMock(IUser::class); $user->expects($this->once()) ->method('getDisplayName') ->willReturn($displayName); + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $you */ + $you = $this->createMock(IUser::class); /** @var Node|\PHPUnit_Framework_MockObject_MockObject $node */ $node = $this->createMock(Node::class); @@ -107,6 +109,10 @@ public function testPrepareSuccess() { ->expects($this->atLeastOnce()) ->method('getName') ->willReturn($fileName); + $node + ->expects($this->atLeastOnce()) + ->method('getPath') + ->willReturn('/you/files/' . $fileName); $userFolder = $this->createMock(Folder::class); $this->folder->expects($this->once()) @@ -118,7 +124,7 @@ public function testPrepareSuccess() { ->with('678') ->willReturn([$node]); - $this->notification->expects($this->once()) + $this->notification->expects($this->exactly(2)) ->method('getUser') ->willReturn('you'); $this->notification @@ -143,6 +149,16 @@ public function testPrepareSuccess() { ->method('setRichSubject') ->with('{user} mentioned you in a comment on “{file}”', $this->anything()) ->willReturnSelf(); + $this->notification + ->expects($this->once()) + ->method('setRichMessage') + ->with('Hi {mention-user1}!', ['mention-user1' => ['type' => 'user', 'id' => 'you', 'name' => 'Your name']]) + ->willReturnSelf(); + $this->notification + ->expects($this->once()) + ->method('setParsedMessage') + ->with('Hi @Your name!') + ->willReturnSelf(); $this->notification ->expects($this->once()) ->method('setIcon') @@ -171,17 +187,32 @@ public function testPrepareSuccess() { ->expects($this->any()) ->method('getActorType') ->willReturn('users'); + $this->comment + ->expects($this->any()) + ->method('getMessage') + ->willReturn('Hi @you!'); + $this->comment + ->expects($this->any()) + ->method('getMentions') + ->willReturn([['type' => 'user', 'id' => 'you']]); $this->commentsManager ->expects($this->once()) ->method('get') ->willReturn($this->comment); + $this->commentsManager + ->expects($this->once()) + ->method('resolveDisplayName') + ->with('user', 'you') + ->willReturn('Your name'); $this->userManager - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('get') - ->with('huraga') - ->willReturn($user); + ->willReturnMap([ + ['huraga', $user], + ['you', $you], + ]); $this->notifier->prepare($this->notification, $this->lc); } @@ -190,12 +221,19 @@ public function testPrepareSuccessDeletedUser() { $fileName = 'Gre\'thor.odp'; $message = 'You were mentioned on “Gre\'thor.odp”, in a comment by a user that has since been deleted'; + /** @var IUser|\PHPUnit_Framework_MockObject_MockObject $you */ + $you = $this->createMock(IUser::class); + /** @var Node|\PHPUnit_Framework_MockObject_MockObject $node */ $node = $this->createMock(Node::class); $node ->expects($this->atLeastOnce()) ->method('getName') ->willReturn($fileName); + $node + ->expects($this->atLeastOnce()) + ->method('getPath') + ->willReturn('/you/files/' . $fileName); $userFolder = $this->createMock(Folder::class); $this->folder->expects($this->once()) @@ -207,7 +245,7 @@ public function testPrepareSuccessDeletedUser() { ->with('678') ->willReturn([$node]); - $this->notification->expects($this->once()) + $this->notification->expects($this->exactly(2)) ->method('getUser') ->willReturn('you'); $this->notification @@ -232,6 +270,16 @@ public function testPrepareSuccessDeletedUser() { ->method('setRichSubject') ->with('You were mentioned on “{file}”, in a comment by a user that has since been deleted', $this->anything()) ->willReturnSelf(); + $this->notification + ->expects($this->once()) + ->method('setRichMessage') + ->with('Hi {mention-user1}!', ['mention-user1' => ['type' => 'user', 'id' => 'you', 'name' => 'Your name']]) + ->willReturnSelf(); + $this->notification + ->expects($this->once()) + ->method('setParsedMessage') + ->with('Hi @Your name!') + ->willReturnSelf(); $this->notification ->expects($this->once()) ->method('setIcon') @@ -260,15 +308,30 @@ public function testPrepareSuccessDeletedUser() { ->expects($this->any()) ->method('getActorType') ->willReturn(ICommentsManager::DELETED_USER); + $this->comment + ->expects($this->any()) + ->method('getMessage') + ->willReturn('Hi @you!'); + $this->comment + ->expects($this->any()) + ->method('getMentions') + ->willReturn([['type' => 'user', 'id' => 'you']]); $this->commentsManager ->expects($this->once()) ->method('get') ->willReturn($this->comment); + $this->commentsManager + ->expects($this->once()) + ->method('resolveDisplayName') + ->with('user', 'you') + ->willReturn('Your name'); $this->userManager - ->expects($this->never()) - ->method('get'); + ->expects($this->once()) + ->method('get') + ->with('you') + ->willReturn($you); $this->notifier->prepare($this->notification, $this->lc); }