Populate the mention-notification with the actual message

Signed-off-by: Joas Schilling <coding@schilljs.com>
2018-11-08 11:54:41 +01:00 · 2018-11-08 11:54:41 +01:00 · 1b4f1b9a63
commit 1b4f1b9a63
parent 78fd8ab0fd
2 changed files with 159 additions and 51 deletions
--- 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 @@ class Notifier implements INotifier {
 		$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 @@ class Notifier implements INotifier {
 				}
 				$node = $nodes[0];

-				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()]),
-								],
-							]
-						);
-				} 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()]),
-								],
-							]
-						);
+				$path = rtrim($node->getPath(), '/');
+				if (strpos($path, '/' . $notification->getUser() . '/files/') === 0) {
+					// Remove /user/files/...
+					$fullPath = $path;
+					list(,,, $path) = explode('/', $fullPath, 4);
 				}
-				$notification->setIcon($this->url->getAbsoluteURL($this->url->imagePath('core', 'actions/comment.svg')))
+				$subjectParameters = [
+					'file' => [
+						'type' => 'file',
+						'id' => $comment->getObjectId(),
+						'name' => $node->getName(),
+						'path' => $path,
+						'link' => $this->url->linkToRouteAbsolute('files.viewcontroller.showFile', ['fileid' => $comment->getObjectId()]),
+					],
+				];
+
+				if ($isDeletedActor) {
+					$subject = $l->t('You were mentioned on “{file}”, in a comment by a user that has since been deleted');
+				} else {
+					$subject = $l->t('{user} mentioned you in a comment on “{file}”');
+					$subjectParameters['user'] = [
+						'type' => 'user',
+						'id' => $comment->getActorId(),
+						'name' => $displayName,
+					];
+				}
+				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 @@ class Notifier implements INotifier {
 			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);
 	}
 }
--- a/apps/comments/tests/Unit/Notification/NotifierTest.php
+++ b/apps/comments/tests/Unit/Notification/NotifierTest.php
@ -93,13 +93,15 @@ class NotifierTest extends TestCase {
 	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 @@ class NotifierTest extends TestCase {
 			->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 @@ class NotifierTest extends TestCase {
 			->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 @@ class NotifierTest extends TestCase {
 			->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 @@ class NotifierTest extends TestCase {
 			->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 @@ class NotifierTest extends TestCase {
 		$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 @@ class NotifierTest extends TestCase {
 			->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 @@ class NotifierTest extends TestCase {
 			->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 @@ class NotifierTest extends TestCase {
 			->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);
 	}