From 1eb6cebcae7979a92e72ff1b078bdccea280f873 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 2 Nov 2017 01:43:55 +0100 Subject: [PATCH 1/9] Add notifications for mentions in chat messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a user is mentioned in a chat message she will now receive a notification about that. If a chat room is deleted all the pending notifications will be also removed. The rich subject of the notifications use "call" instead of "room" or "chat" due to "call" being already available in "lib/public/RichObjectStrings/Definitions.php" (in Nextcloud server), although the Notifications app does not handle it in any special way though ("apps/notifications/js/richObjectStringParser.js"). Note that, unlike with the current code for invitation notifications, no checks are made on whether "call" is available or not, as the chat is available only from Nextcloud 13 and onwards. Currently every user mentioned in a chat message is notified, but this will be tightened in later commits (for example, if a user is mentioned in a chat room to which she is not able to join). Signed-off-by: Daniel Calviño Sánchez --- lib/Chat/ChatManager.php | 15 +- lib/Chat/Notifier.php | 153 +++++++++++++++ lib/Notification/Notifier.php | 126 +++++++++++- tests/php/Chat/ChatManagerTest.php | 17 +- tests/php/Chat/NotifierTest.php | 249 ++++++++++++++++++++++++ tests/php/Notification/NotifierTest.php | 193 ++++++++++++++++++ 6 files changed, 749 insertions(+), 4 deletions(-) create mode 100644 lib/Chat/Notifier.php create mode 100644 tests/php/Chat/NotifierTest.php diff --git a/lib/Chat/ChatManager.php b/lib/Chat/ChatManager.php index 118b4f1c8bf..0b6c5156ec7 100644 --- a/lib/Chat/ChatManager.php +++ b/lib/Chat/ChatManager.php @@ -31,17 +31,26 @@ * sendMessage() saves a comment using the ICommentsManager, while * receiveMessages() tries to read comments from ICommentsManager (with a little * wait between reads) until comments are found or until the timeout expires. + * + * When a message is saved the mentioned users are notified as needed, and + * pending notifications are removed if the messages are deleted. */ class ChatManager { /** @var ICommentsManager */ private $commentsManager; + /** @var Notifier */ + private $notifier; + /** * @param ICommentsManager $commentsManager + * @param Notifier $notifier */ - public function __construct(ICommentsManager $commentsManager) { + public function __construct(ICommentsManager $commentsManager, + Notifier $notifier) { $this->commentsManager = $commentsManager; + $this->notifier = $notifier; } /** @@ -62,6 +71,8 @@ public function sendMessage($chatId, $actorType, $actorId, $message, \DateTime $ $comment->setVerb('comment'); $this->commentsManager->save($comment); + + $this->notifier->notifyMentionedUsers($comment); } /** @@ -127,6 +138,8 @@ public function receiveMessages($chatId, $timeout, $offset = 0, \DateTime $notOl */ public function deleteMessages($chatId) { $this->commentsManager->deleteCommentsAtObject('chat', $chatId); + + $this->notifier->removePendingNotificationsForRoom($chatId); } } diff --git a/lib/Chat/Notifier.php b/lib/Chat/Notifier.php new file mode 100644 index 00000000000..21885bed879 --- /dev/null +++ b/lib/Chat/Notifier.php @@ -0,0 +1,153 @@ +. + * + */ + +namespace OCA\Spreed\Chat; + +use OCP\Comments\IComment; +use OCP\Notification\IManager as INotificationManager; +use OCP\Notification\INotification; +use OCP\IUserManager; + +/** + * Helper class for notifications related to user mentions in chat messages. + * + * This class uses the NotificationManager to create and remove the + * notifications as needed; OCA\Spreed\Notification\Notifier is the one that + * prepares the notifications for display. + */ +class Notifier { + + /** @var INotificationManager */ + private $notificationManager; + + /** @var IUserManager */ + private $userManager; + + /** + * @param INotificationManager $notificationManager + * @param IUserManager $userManager + */ + public function __construct(INotificationManager $notificationManager, + IUserManager $userManager) { + $this->notificationManager = $notificationManager; + $this->userManager = $userManager; + } + + /** + * Notifies the user mentioned in the comment. + * + * The comment must be a chat message comment. That is, its "objectId" must + * be the room ID. + * + * @param IComment $comment + */ + public function notifyMentionedUsers(IComment $comment) { + $mentionedUserIds = $this->getMentionedUserIds($comment); + if (empty($mentionedUserIds)) { + return; + } + + foreach ($mentionedUserIds as $mentionedUserId) { + if ($this->shouldUserBeNotified($mentionedUserId, $comment)) { + $notification = $this->createNotification($comment, $mentionedUserId); + + $this->notificationManager->notify($notification); + } + } + } + + /** + * Removes all the pending notifications for the room with the given ID. + * + * @param string $roomId + */ + public function removePendingNotificationsForRoom($roomId) { + $notification = $this->notificationManager->createNotification(); + + $notification + ->setApp('spreed') + ->setObject('room', $roomId); + + $this->notificationManager->markProcessed($notification); + } + + /** + * Returns the IDs of the users mentioned in the given comment. + * + * @param IComment $comment + * @return string[] the mentioned user IDs + */ + private function getMentionedUserIds(IComment $comment) { + $mentions = $comment->getMentions(); + + if (empty($mentions)) { + return []; + } + + $userIds = []; + foreach ($mentions as $mention) { + if ($mention['type'] === 'user') { + $userIds[] = $mention['id']; + } + } + + return $userIds; + } + + /** + * Creates a notification for the given chat message comment and mentioned + * user ID. + * + * @param IComment $comment + * @param string $mentionedUserId + * @return INotification + */ + private function createNotification(IComment $comment, $mentionedUserId) { + $notification = $this->notificationManager->createNotification(); + $notification + ->setApp('spreed') + ->setObject('room', $comment->getObjectId()) + ->setUser($mentionedUserId) + ->setSubject('mention', [ + 'userType' => $comment->getActorType(), + 'userId' => $comment->getActorId(), + ]) + ->setDateTime($comment->getCreationDateTime()); + + return $notification; + } + + private function shouldUserBeNotified($userId, IComment $comment) { + if ($userId === $comment->getActorId()) { + // Do not notify the user if she mentioned herself + return false; + } + + if (!$this->userManager->userExists($userId)) { + return false; + } + + return true; + } + +} diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index 4d157991902..0c4b29d0d57 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -92,16 +92,138 @@ public function prepare(INotification $notification, $languageCode) { ->setIcon($this->url->getAbsoluteURL($this->url->imagePath('spreed', 'app.svg'))) ->setLink($this->url->linkToRouteAbsolute('spreed.Page.index') . '?token=' . $room->getToken()); - if ($notification->getSubject() === 'invitation') { + $subject = $notification->getSubject(); + if ($subject === 'invitation') { return $this->parseInvitation($notification, $room, $l); } - if ($notification->getSubject() === 'call') { + if ($subject === 'call') { return $this->parseCall($notification, $room, $l); } + if ($subject === 'mention') { + return $this->parseMention($notification, $room, $l); + } throw new \InvalidArgumentException('Unknown subject'); } + /** + * @param INotification $notification + * @param Room $room + * @param IL10N $l + * @return INotification + * @throws \InvalidArgumentException + */ + protected function parseMention(INotification $notification, Room $room, IL10N $l) { + if ($notification->getObjectType() !== 'room') { + throw new \InvalidArgumentException('Unknown object type'); + } + + $subjectParameters = $notification->getSubjectParameters(); + + $richSubjectUser = null; + $isGuest = false; + if ($subjectParameters['userType'] === 'users') { + $userId = $subjectParameters['userId']; + $user = $this->userManager->get($userId); + + if ($user instanceof IUser) { + $richSubjectUser = [ + 'type' => 'user', + 'id' => $userId, + 'name' => $user->getDisplayName(), + ]; + } + } else { + $isGuest = true; + } + + $richSubjectCall = null; + if ($room->getName() !== '') { + $richSubjectCall = [ + 'type' => 'call', + 'id' => $room->getId(), + 'name' => $room->getName(), + 'call-type' => $this->getRoomType($room), + ]; + } + + if ($room->getType() === Room::ONE_TO_ONE_CALL) { + $notification + ->setParsedSubject( + $l->t('%s mentioned you in a private chat', [$user->getDisplayName()]) + ) + ->setRichSubject( + $l->t('{user} mentioned you in a private chat'), [ + 'user' => $richSubjectUser + ] + ); + + } else if (in_array($room->getType(), [Room::GROUP_CALL, Room::PUBLIC_CALL], true)) { + if ($richSubjectUser && $richSubjectCall) { + $notification + ->setParsedSubject( + $l->t('%s mentioned you in a group chat: %s', [$user->getDisplayName(), $room->getName()]) + ) + ->setRichSubject( + $l->t('{user} mentioned you in a group chat: {call}'), [ + 'user' => $richSubjectUser, + 'call' => $richSubjectCall + ] + ); + } else if ($richSubjectUser && !$richSubjectCall) { + $notification + ->setParsedSubject( + $l->t('%s mentioned you in a group chat', [$user->getDisplayName()]) + ) + ->setRichSubject( + $l->t('{user} mentioned you in a group chat'), [ + 'user' => $richSubjectUser + ] + ); + } else if (!$richSubjectUser && !$isGuest && $richSubjectCall) { + $notification + ->setParsedSubject( + $l->t('A (now) deleted user mentioned you in a group chat: %s', [$room->getName()]) + ) + ->setRichSubject( + $l->t('A (now) deleted user mentioned you in a group chat: {call}'), [ + 'call' => $richSubjectCall + ] + ); + } else if (!$richSubjectUser && !$isGuest && !$richSubjectCall) { + $notification + ->setParsedSubject( + $l->t('A (now) deleted user mentioned you in a group chat') + ) + ->setRichSubject( + $l->t('A (now) deleted user mentioned you in a group chat') + ); + } else if (!$richSubjectUser && $isGuest && $richSubjectCall) { + $notification + ->setParsedSubject( + $l->t('A guest mentioned you in a group chat: %s', [$room->getName()]) + ) + ->setRichSubject( + $l->t('A guest mentioned you in a group chat: {call}'), [ + 'call' => $richSubjectCall + ] + ); + } else if (!$richSubjectUser && $isGuest && !$richSubjectCall) { + $notification + ->setParsedSubject( + $l->t('A guest mentioned you in a group chat') + ) + ->setRichSubject( + $l->t('A guest mentioned you in a group chat') + ); + } + } else { + throw new \InvalidArgumentException('Unknown room type'); + } + + return $notification; + } + /** * @param Room $room * @return string diff --git a/tests/php/Chat/ChatManagerTest.php b/tests/php/Chat/ChatManagerTest.php index 5e7d7ad41df..131b11d99ab 100644 --- a/tests/php/Chat/ChatManagerTest.php +++ b/tests/php/Chat/ChatManagerTest.php @@ -24,6 +24,7 @@ namespace OCA\Spreed\Tests\php\Chat; use OCA\Spreed\Chat\ChatManager; +use OCA\Spreed\Chat\Notifier; use OCP\Comments\IComment; use OCP\Comments\ICommentsManager; @@ -32,6 +33,9 @@ class ChatManagerTest extends \Test\TestCase { /** @var OCP\Comments\ICommentsManager|\PHPUnit_Framework_MockObject_MockObject */ protected $commentsManager; + /** @var \OCA\Spreed\Chat\Notifier|\PHPUnit_Framework_MockObject_MockObject */ + protected $notifier; + /** @var \OCA\Spreed\Chat\ChatManager */ protected $chatManager; @@ -40,7 +44,10 @@ public function setUp() { $this->commentsManager = $this->createMock(ICommentsManager::class); - $this->chatManager = new ChatManager($this->commentsManager); + $this->notifier = $this->createMock(Notifier::class); + + $this->chatManager = new ChatManager($this->commentsManager, + $this->notifier); } private function newComment($id, $actorType, $actorId, $creationDateTime, $message) { @@ -87,6 +94,10 @@ public function testSendMessage() { ->method('save') ->with($comment); + $this->notifier->expects($this->once()) + ->method('notifyMentionedUsers') + ->with($comment); + $this->chatManager->sendMessage('testChatId', 'users', 'testUser', 'testMessage', $creationDateTime); } @@ -190,6 +201,10 @@ public function testDeleteMessages() { ->method('deleteCommentsAtObject') ->with('chat', 'testChatId'); + $this->notifier->expects($this->once()) + ->method('removePendingNotificationsForRoom') + ->with('testChatId'); + $this->chatManager->deleteMessages('testChatId'); } diff --git a/tests/php/Chat/NotifierTest.php b/tests/php/Chat/NotifierTest.php new file mode 100644 index 00000000000..5e3279df703 --- /dev/null +++ b/tests/php/Chat/NotifierTest.php @@ -0,0 +1,249 @@ +. + * + */ + +namespace OCA\Spreed\Tests\php\Chat; + +use OCA\Spreed\Chat\Notifier; +use OCP\Comments\IComment; +use OCP\Notification\IManager as INotificationManager; +use OCP\Notification\INotification; +use OCP\IUserManager; + +class NotifierTest extends \Test\TestCase { + + /** @var \OCP\Notification\IManager|\PHPUnit_Framework_MockObject_MockObject */ + protected $notificationManager; + + /** @var \OCP\IUserManager|\PHPUnit_Framework_MockObject_MockObject */ + protected $userManager; + + /** @var \OCA\Spreed\Chat\Notifier */ + protected $notifier; + + public function setUp() { + parent::setUp(); + + $this->notificationManager = $this->createMock(INotificationManager::class); + + $this->userManager = $this->createMock(IUserManager::class); + $this->userManager + ->method('userExists') + ->will($this->returnCallback(function($userId) { + if ($userId === 'unknownUser') { + return false; + } + + return true; + })); + + $this->notifier = new Notifier($this->notificationManager, + $this->userManager); + } + + private function newComment($id, $actorType, $actorId, $creationDateTime, $message) { + // $mentionMatches[0] contains the whole matches, while + // $mentionMatches[1] contains the matched subpattern. + $mentionMatches = []; + preg_match_all('/@([a-zA-Z0-9]+)/', $message, $mentionMatches); + + $mentions = array_map(function($mentionMatch) { + return [ 'type' => 'user', 'id' => $mentionMatch ]; + }, $mentionMatches[1]); + + $comment = $this->createMock(IComment::class); + + $comment->method('getId')->willReturn($id); + $comment->method('getActorType')->willReturn($actorType); + $comment->method('getActorId')->willReturn($actorId); + $comment->method('getCreationDateTime')->willReturn($creationDateTime); + $comment->method('getMessage')->willReturn($message); + $comment->method('getMentions')->willReturn($mentions); + + return $comment; + } + + private function newNotification($comment) { + $notification = $this->createMock(INotification::class); + + $notification->expects($this->once()) + ->method('setApp') + ->with('spreed') + ->willReturnSelf(); + + $notification->expects($this->once()) + ->method('setObject') + ->with('room', $comment->getObjectId()) + ->willReturnSelf(); + + $notification->expects($this->once()) + ->method('setSubject') + ->with('mention', [ + 'userType' => $comment->getActorType(), + 'userId' => $comment->getActorId(), + ]) + ->willReturnSelf(); + + $notification->expects($this->once()) + ->method('setDateTime') + ->with($comment->getCreationDateTime()) + ->willReturnSelf(); + + return $notification; + } + + public function testNotifyMentionedUsers() { + $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'Mention @anotherUser'); + + $notification = $this->newNotification($comment); + + $this->notificationManager->expects($this->once()) + ->method('createNotification') + ->willReturn($notification); + + $notification->expects($this->once()) + ->method('setUser') + ->with('anotherUser') + ->willReturnSelf(); + + $this->notificationManager->expects($this->once()) + ->method('notify') + ->with($notification); + + $this->notifier->notifyMentionedUsers($comment); + } + + public function testNotifyMentionedUsersByGuest() { + $comment = $this->newComment(108, 'guests', 'testSpreedSession', new \DateTime('@' . 1000000016), 'Mention @anotherUser'); + + $notification = $this->newNotification($comment); + + $this->notificationManager->expects($this->once()) + ->method('createNotification') + ->willReturn($notification); + + $notification->expects($this->once()) + ->method('setUser') + ->with('anotherUser') + ->willReturnSelf(); + + $this->notificationManager->expects($this->once()) + ->method('notify') + ->with($notification); + + $this->notifier->notifyMentionedUsers($comment); + } + + public function testNotifyMentionedUsersToSelf() { + $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'Mention @testUser'); + + $this->notificationManager->expects($this->never()) + ->method('createNotification'); + + $this->notificationManager->expects($this->never()) + ->method('notify'); + + $this->notifier->notifyMentionedUsers($comment); + } + + public function testNotifyMentionedUsersToUnknownUser() { + $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'Mention @unknownUser'); + + $this->notificationManager->expects($this->never()) + ->method('createNotification'); + + $this->notificationManager->expects($this->never()) + ->method('notify'); + + $this->notifier->notifyMentionedUsers($comment); + } + + public function testNotifyMentionedUsersNoMentions() { + $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'No mentions'); + + $this->notificationManager->expects($this->never()) + ->method('createNotification'); + + $this->notificationManager->expects($this->never()) + ->method('notify'); + + $this->notifier->notifyMentionedUsers($comment); + } + + public function testNotifyMentionedUsersSeveralMentions() { + $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'Mention @anotherUser, and @unknownUser, and @testUser, and @userAbleToJoin'); + + $anotherUserNotification = $this->newNotification($comment); + $userAbleToJoinNotification = $this->newNotification($comment); + + $this->notificationManager->expects($this->exactly(2)) + ->method('createNotification') + ->will($this->onConsecutiveCalls( + $anotherUserNotification, + $userAbleToJoinNotification + )); + + $anotherUserNotification->expects($this->once()) + ->method('setUser') + ->with('anotherUser') + ->willReturnSelf(); + + $userAbleToJoinNotification->expects($this->once()) + ->method('setUser') + ->with('userAbleToJoin') + ->willReturnSelf(); + + $this->notificationManager->expects($this->exactly(2)) + ->method('notify') + ->withConsecutive( + [ $anotherUserNotification ], + [ $userAbleToJoinNotification ] + ); + + $this->notifier->notifyMentionedUsers($comment); + } + + public function testRemovePendingNotificationsForRoom() { + $notification = $this->createMock(INotification::class); + + $this->notificationManager->expects($this->once()) + ->method('createNotification') + ->willReturn($notification); + + $notification->expects($this->once()) + ->method('setApp') + ->with('spreed') + ->willReturnSelf(); + + $notification->expects($this->once()) + ->method('setObject') + ->with('room', 'testChatId') + ->willReturnSelf(); + + $this->notificationManager->expects($this->once()) + ->method('markProcessed') + ->with($notification); + + $this->notifier->removePendingNotificationsForRoom('testChatId'); + } + +} diff --git a/tests/php/Notification/NotifierTest.php b/tests/php/Notification/NotifierTest.php index b1f4614de67..188dd82f059 100644 --- a/tests/php/Notification/NotifierTest.php +++ b/tests/php/Notification/NotifierTest.php @@ -259,6 +259,198 @@ public function testPrepareGroup($type, $uid, $displayName, $name, $parsedSubjec $this->notifier->prepare($n, 'de'); } + public function dataPrepareMention() { + return [ + [ + Room::ONE_TO_ONE_CALL, ['userType' => 'users', 'userId' => 'testUser'], 'Test user', '', + 'Test user mentioned you in a private chat', + ['{user} mentioned you in a private chat', + ['user' => ['type' => 'user', 'id' => 'testUser', 'name' => 'Test user']] + ] + ], + // If the user is deleted in a one to one chat the chat is also + // deleted, and that in turn would delete the pending notification. + [ + Room::GROUP_CALL, ['userType' => 'users', 'userId' => 'testUser'], 'Test user', '', + 'Test user mentioned you in a group chat', + ['{user} mentioned you in a group chat', + ['user' => ['type' => 'user', 'id' => 'testUser', 'name' => 'Test user']] + ] + ], + [ + Room::GROUP_CALL, ['userType' => 'users', 'userId' => 'testUser'], null, '', + 'A (now) deleted user mentioned you in a group chat', + ['A (now) deleted user mentioned you in a group chat', + [] + ], + true + ], + [ + Room::GROUP_CALL, ['userType' => 'users', 'userId' => 'testUser'], 'Test user', 'Room name', + 'Test user mentioned you in a group chat: Room name', + ['{user} mentioned you in a group chat: {call}', + [ + 'user' => ['type' => 'user', 'id' => 'testUser', 'name' => 'Test user'], + 'call' => ['type' => 'call', 'id' => 'testRoomId', 'name' => 'Room name', 'call-type' => 'group'] + ] + ] + ], + [ + Room::GROUP_CALL, ['userType' => 'users', 'userId' => 'testUser'], null, 'Room name', + 'A (now) deleted user mentioned you in a group chat: Room name', + ['A (now) deleted user mentioned you in a group chat: {call}', + [ + 'call' => ['type' => 'call', 'id' => 'testRoomId', 'name' => 'Room name', 'call-type' => 'group'] + ] + ], + true + ], + [ + Room::PUBLIC_CALL, ['userType' => 'users', 'userId' => 'testUser'], 'Test user', '', + 'Test user mentioned you in a group chat', + ['{user} mentioned you in a group chat', + ['user' => ['type' => 'user', 'id' => 'testUser', 'name' => 'Test user']] + ] + ], + [ + Room::PUBLIC_CALL, ['userType' => 'users', 'userId' => 'testUser'], null, '', + 'A (now) deleted user mentioned you in a group chat', + ['A (now) deleted user mentioned you in a group chat', + [] + ], + true + ], + [ + Room::PUBLIC_CALL, ['userType' => 'guests', 'userId' => 'testSpreedSession'], null, '', + 'A guest mentioned you in a group chat', + ['A guest mentioned you in a group chat', + [] + ] + ], + [ + Room::PUBLIC_CALL, ['userType' => 'users', 'userId' => 'testUser'], 'Test user', 'Room name', + 'Test user mentioned you in a group chat: Room name', + ['{user} mentioned you in a group chat: {call}', + [ + 'user' => ['type' => 'user', 'id' => 'testUser', 'name' => 'Test user'], + 'call' => ['type' => 'call', 'id' => 'testRoomId', 'name' => 'Room name', 'call-type' => 'public'] + ] + ] + ], + [ + Room::PUBLIC_CALL, ['userType' => 'users', 'userId' => 'testUser'], null, 'Room name', + 'A (now) deleted user mentioned you in a group chat: Room name', + ['A (now) deleted user mentioned you in a group chat: {call}', + [ + 'call' => ['type' => 'call', 'id' => 'testRoomId', 'name' => 'Room name', 'call-type' => 'public'] + ] + ], + true + ], + [ + Room::PUBLIC_CALL, ['userType' => 'guests', 'userId' => 'testSpreedSession'], null, 'Room name', + 'A guest mentioned you in a group chat: Room name', + ['A guest mentioned you in a group chat: {call}', + ['call' => ['type' => 'call', 'id' => 'testRoomId', 'name' => 'Room name', 'call-type' => 'public']] + ] + ] + ]; + } + + /** + * @dataProvider dataPrepareMention + * @param int $roomType + * @param array $subjectParameters + * @param string $displayName + * @param string $roomName + * @param string $parsedSubject + * @param array $richSubject + * @param bool $deletedUser + */ + public function testPrepareMention($roomType, $subjectParameters, $displayName, $roomName, $parsedSubject, $richSubject, $deletedUser = false) { + $notification = $this->createMock(INotification::class); + $l = $this->createMock(IL10N::class); + $l->expects($this->exactly(2)) + ->method('t') + ->will($this->returnCallback(function($text, $parameters = []) { + return vsprintf($text, $parameters); + })); + + $room = $this->createMock(Room::class); + $room->expects($this->atLeastOnce()) + ->method('getType') + ->willReturn($roomType); + $room->expects($this->atLeastOnce()) + ->method('getName') + ->willReturn($roomName); + if ($roomName !== '') { + $room->expects($this->atLeastOnce()) + ->method('getId') + ->willReturn('testRoomId'); + } + $this->manager->expects($this->once()) + ->method('getRoomById') + ->willReturn($room); + + $this->lFactory->expects($this->once()) + ->method('get') + ->with('spreed', 'de') + ->willReturn($l); + + $user = $this->createMock(IUser::class); + if ($subjectParameters['userType'] === 'users' && !$deletedUser) { + $user->expects($this->exactly(2)) + ->method('getDisplayName') + ->willReturn($displayName); + $this->userManager->expects($this->once()) + ->method('get') + ->with($subjectParameters['userId']) + ->willReturn($user); + } else if ($subjectParameters['userType'] === 'users' && $deletedUser) { + $user->expects($this->never()) + ->method('getDisplayName'); + $this->userManager->expects($this->once()) + ->method('get') + ->with($subjectParameters['userId']) + ->willReturn(null); + } else { + $user->expects($this->never()) + ->method('getDisplayName'); + $this->userManager->expects($this->never()) + ->method('get'); + } + + $notification->expects($this->once()) + ->method('setIcon') + ->willReturnSelf(); + $notification->expects($this->once()) + ->method('setLink') + ->willReturnSelf(); + $notification->expects($this->once()) + ->method('setParsedSubject') + ->with($parsedSubject) + ->willReturnSelf(); + $notification->expects($this->once()) + ->method('setRichSubject') + ->with($richSubject[0], $richSubject[1]) + ->willReturnSelf(); + + $notification->expects($this->once()) + ->method('getApp') + ->willReturn('spreed'); + $notification->expects($this->once()) + ->method('getSubject') + ->willReturn('mention'); + $notification->expects($this->once()) + ->method('getSubjectParameters') + ->willReturn($subjectParameters); + $notification->expects($this->once()) + ->method('getObjectType') + ->willReturn('room'); + + $this->assertEquals($notification, $this->notifier->prepare($notification, 'de')); + } + public function dataPrepareThrows() { return [ ['Incorrect app', 'invalid-app', null, null, null, null], @@ -266,6 +458,7 @@ public function dataPrepareThrows() { ['Unknown subject', 'spreed', true, 'invalid-subject', null, null], ['Unknown object type', 'spreed', true, 'invitation', null, 'invalid-object-type'], ['Calling user does not exist anymore', 'spreed', true, 'invitation', ['admin'], 'room'], + ['Unknown object type', 'spreed', true, 'mention', null, 'invalid-object-type'], ]; } From 1e0bcabdeb9a659dab9900860ef58df823638cf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 2 Nov 2017 02:20:01 +0100 Subject: [PATCH 2/9] Add the message to the mention notifications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The chat message can be much longer than the maximum length allowed for notification messages, so the notification message is trimmed if needed. When the notification is being prepared to be displayed it is taken into account if the message was trimmed, and in that case an ellipsis is added at the start and/or the end (depending on the case) of the message. Note that the original notification message does not contain the ellipsis characters, as depending on the language a whitespace should be added or not before and after an ellipsis, and the language in which the notification will be displayed is only known at the time of displaying it, but not when it is saved. Signed-off-by: Daniel Calviño Sánchez --- lib/Chat/Notifier.php | 52 ++++++++++++ lib/Notification/Notifier.php | 12 +++ tests/php/Chat/NotifierTest.php | 101 ++++++++++++++++++++++++ tests/php/Notification/NotifierTest.php | 75 +++++++++++------- 4 files changed, 212 insertions(+), 28 deletions(-) diff --git a/lib/Chat/Notifier.php b/lib/Chat/Notifier.php index 21885bed879..bab216f5b3c 100644 --- a/lib/Chat/Notifier.php +++ b/lib/Chat/Notifier.php @@ -134,9 +134,61 @@ private function createNotification(IComment $comment, $mentionedUserId) { ]) ->setDateTime($comment->getCreationDateTime()); + $notificationMessage = $this->getNotificationMessage($comment, $mentionedUserId); + if (count($notificationMessage) === 1) { + $notification->setMessage($notificationMessage[0]); + } else { + $notification->setMessage($notificationMessage[0], $notificationMessage[1]); + } + return $notification; } + /** + * Returns the message for a notification from the message of the comment. + * + * The message is returned as an array; the first element is the message + * itself, and the second element is another array that contains the + * parameters for the message. If no parameters are needed then the returned + * array has a single element. + * + * The message of a comment can be much longer than the maximum allowed + * length for the message of a notification so, if needed, the comment + * message is trimmed around the first mention to the user. In that case + * the "ellipsisStart" and/or "ellipsisEnd" (depending on the case) are + * returned as the parameters. + * + * @param IComment $comment + * @param string $mentionedUserId + * @return array the first element is a message suitable to be stored in a + * notification, and the second are the parameters, if any. + */ + private function getNotificationMessage(IComment $comment, $mentionedUserId) { + $maximumLength = 64; + + $message = $comment->getMessage(); + + $messageLength = strlen($message); + if ($messageLength <= $maximumLength) { + return [$message]; + } + + $mention = '@' . $mentionedUserId; + $mentionLength = strlen($mention); + // Only the first mention is taken into account + $mentionMiddleIndex = strpos($message, $mention) + $mentionLength / 2; + + if ($mentionMiddleIndex <= $maximumLength / 2) { + return [substr($message, 0, $maximumLength), ['ellipsisEnd']]; + } + + if ($mentionMiddleIndex >= ($messageLength - $maximumLength / 2)) { + return [substr($message, -$maximumLength), ['ellipsisStart']]; + } + + return [substr($message, $mentionMiddleIndex - ($maximumLength / 2), $maximumLength), ['ellipsisStart', 'ellipsisEnd']]; + } + private function shouldUserBeNotified($userId, IComment $comment) { if ($userId === $comment->getActorId()) { // Do not notify the user if she mentioned herself diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index 0c4b29d0d57..aa766870841 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -147,6 +147,18 @@ protected function parseMention(INotification $notification, Room $room, IL10N $ ]; } + $messageParameters = $notification->getMessageParameters(); + + $parsedMessage = $notification->getMessage(); + if (in_array('ellipsisStart', $messageParameters) && !in_array('ellipsisEnd', $messageParameters)) { + $parsedMessage = $l->t('… %s', $parsedMessage); + } else if (!in_array('ellipsisStart', $messageParameters) && in_array('ellipsisEnd', $messageParameters)) { + $parsedMessage = $l->t('%s …', $parsedMessage); + } else if (in_array('ellipsisStart', $messageParameters) && in_array('ellipsisEnd', $messageParameters)) { + $parsedMessage = $l->t('… %s …', $parsedMessage); + } + $notification->setParsedMessage($parsedMessage); + if ($room->getType() === Room::ONE_TO_ONE_CALL) { $notification ->setParsedSubject( diff --git a/tests/php/Chat/NotifierTest.php b/tests/php/Chat/NotifierTest.php index 5e3279df703..915b8ef3a4e 100644 --- a/tests/php/Chat/NotifierTest.php +++ b/tests/php/Chat/NotifierTest.php @@ -125,6 +125,11 @@ public function testNotifyMentionedUsers() { ->with('anotherUser') ->willReturnSelf(); + $notification->expects($this->once()) + ->method('setMessage') + ->with($comment->getMessage()) + ->willReturnSelf(); + $this->notificationManager->expects($this->once()) ->method('notify') ->with($notification); @@ -146,6 +151,92 @@ public function testNotifyMentionedUsersByGuest() { ->with('anotherUser') ->willReturnSelf(); + $notification->expects($this->once()) + ->method('setMessage') + ->with($comment->getMessage()) + ->willReturnSelf(); + + $this->notificationManager->expects($this->once()) + ->method('notify') + ->with($notification); + + $this->notifier->notifyMentionedUsers($comment); + } + + public function testNotifyMentionedUsersWithLongMessageStartMention() { + $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), + '123456789 @anotherUserWithOddLengthName 123456789-123456789-123456789-123456789-123456789-123456789'); + + $notification = $this->newNotification($comment); + + $this->notificationManager->expects($this->once()) + ->method('createNotification') + ->willReturn($notification); + + $notification->expects($this->once()) + ->method('setUser') + ->with('anotherUserWithOddLengthName') + ->willReturnSelf(); + + $notification->expects($this->once()) + ->method('setMessage') + ->with('123456789 @anotherUserWithOddLengthName 123456789-123456789-1234', ['ellipsisEnd']) + ->willReturnSelf(); + + $this->notificationManager->expects($this->once()) + ->method('notify') + ->with($notification); + + $this->notifier->notifyMentionedUsers($comment); + } + + public function testNotifyMentionedUsersWithLongMessageMiddleMention() { + $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), + '123456789-123456789-123456789-1234 @anotherUserWithOddLengthName 6789-123456789-123456789-123456789'); + + $notification = $this->newNotification($comment); + + $this->notificationManager->expects($this->once()) + ->method('createNotification') + ->willReturn($notification); + + $notification->expects($this->once()) + ->method('setUser') + ->with('anotherUserWithOddLengthName') + ->willReturnSelf(); + + $notification->expects($this->once()) + ->method('setMessage') + ->with('89-123456789-1234 @anotherUserWithOddLengthName 6789-123456789-1', ['ellipsisStart', 'ellipsisEnd']) + ->willReturnSelf(); + + $this->notificationManager->expects($this->once()) + ->method('notify') + ->with($notification); + + $this->notifier->notifyMentionedUsers($comment); + } + + public function testNotifyMentionedUsersWithLongMessageEndMention() { + $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), + '123456789-123456789-123456789-123456789-123456789-123456789 @anotherUserWithOddLengthName 123456789'); + + $notification = $this->newNotification($comment); + + $this->notificationManager->expects($this->once()) + ->method('createNotification') + ->willReturn($notification); + + $notification->expects($this->once()) + ->method('setUser') + ->with('anotherUserWithOddLengthName') + ->willReturnSelf(); + + $notification->expects($this->once()) + ->method('setMessage') + ->with('6789-123456789-123456789 @anotherUserWithOddLengthName 123456789', ['ellipsisStart']) + ->willReturnSelf(); + $this->notificationManager->expects($this->once()) ->method('notify') ->with($notification); @@ -207,11 +298,21 @@ public function testNotifyMentionedUsersSeveralMentions() { ->with('anotherUser') ->willReturnSelf(); + $anotherUserNotification->expects($this->once()) + ->method('setMessage') + ->with('Mention @anotherUser, and @unknownUser, and @testUser, and @user') + ->willReturnSelf(); + $userAbleToJoinNotification->expects($this->once()) ->method('setUser') ->with('userAbleToJoin') ->willReturnSelf(); + $userAbleToJoinNotification->expects($this->once()) + ->method('setMessage') + ->with('notherUser, and @unknownUser, and @testUser, and @userAbleToJoin') + ->willReturnSelf(); + $this->notificationManager->expects($this->exactly(2)) ->method('notify') ->withConsecutive( diff --git a/tests/php/Notification/NotifierTest.php b/tests/php/Notification/NotifierTest.php index 188dd82f059..3bb6c22a65f 100644 --- a/tests/php/Notification/NotifierTest.php +++ b/tests/php/Notification/NotifierTest.php @@ -262,97 +262,104 @@ public function testPrepareGroup($type, $uid, $displayName, $name, $parsedSubjec public function dataPrepareMention() { return [ [ - Room::ONE_TO_ONE_CALL, ['userType' => 'users', 'userId' => 'testUser'], 'Test user', '', + Room::ONE_TO_ONE_CALL, ['userType' => 'users', 'userId' => 'testUser'], ['ellipsisStart', 'ellipsisEnd'], 'Test user', '', 'Test user mentioned you in a private chat', ['{user} mentioned you in a private chat', ['user' => ['type' => 'user', 'id' => 'testUser', 'name' => 'Test user']] - ] + ], + '… message …' ], // If the user is deleted in a one to one chat the chat is also // deleted, and that in turn would delete the pending notification. [ - Room::GROUP_CALL, ['userType' => 'users', 'userId' => 'testUser'], 'Test user', '', + Room::GROUP_CALL, ['userType' => 'users', 'userId' => 'testUser'], [], 'Test user', '', 'Test user mentioned you in a group chat', ['{user} mentioned you in a group chat', ['user' => ['type' => 'user', 'id' => 'testUser', 'name' => 'Test user']] - ] + ], + 'message' ], [ - Room::GROUP_CALL, ['userType' => 'users', 'userId' => 'testUser'], null, '', + Room::GROUP_CALL, ['userType' => 'users', 'userId' => 'testUser'], ['ellipsisStart'], null, '', 'A (now) deleted user mentioned you in a group chat', ['A (now) deleted user mentioned you in a group chat', [] ], - true - ], + '… message', + true], [ - Room::GROUP_CALL, ['userType' => 'users', 'userId' => 'testUser'], 'Test user', 'Room name', + Room::GROUP_CALL, ['userType' => 'users', 'userId' => 'testUser'], ['ellipsisEnd'], 'Test user', 'Room name', 'Test user mentioned you in a group chat: Room name', ['{user} mentioned you in a group chat: {call}', [ 'user' => ['type' => 'user', 'id' => 'testUser', 'name' => 'Test user'], 'call' => ['type' => 'call', 'id' => 'testRoomId', 'name' => 'Room name', 'call-type' => 'group'] ] - ] + ], + 'message …' ], [ - Room::GROUP_CALL, ['userType' => 'users', 'userId' => 'testUser'], null, 'Room name', + Room::GROUP_CALL, ['userType' => 'users', 'userId' => 'testUser'], ['ellipsisStart', 'ellipsisEnd'], null, 'Room name', 'A (now) deleted user mentioned you in a group chat: Room name', ['A (now) deleted user mentioned you in a group chat: {call}', [ 'call' => ['type' => 'call', 'id' => 'testRoomId', 'name' => 'Room name', 'call-type' => 'group'] ] ], - true - ], + '… message …', + true], [ - Room::PUBLIC_CALL, ['userType' => 'users', 'userId' => 'testUser'], 'Test user', '', + Room::PUBLIC_CALL, ['userType' => 'users', 'userId' => 'testUser'], [], 'Test user', '', 'Test user mentioned you in a group chat', ['{user} mentioned you in a group chat', ['user' => ['type' => 'user', 'id' => 'testUser', 'name' => 'Test user']] - ] + ], + 'message' ], [ - Room::PUBLIC_CALL, ['userType' => 'users', 'userId' => 'testUser'], null, '', + Room::PUBLIC_CALL, ['userType' => 'users', 'userId' => 'testUser'], ['ellipsisStart'], null, '', 'A (now) deleted user mentioned you in a group chat', ['A (now) deleted user mentioned you in a group chat', [] ], - true - ], + '… message', + true], [ - Room::PUBLIC_CALL, ['userType' => 'guests', 'userId' => 'testSpreedSession'], null, '', + Room::PUBLIC_CALL, ['userType' => 'guests', 'userId' => 'testSpreedSession'], ['ellipsisEnd'], null, '', 'A guest mentioned you in a group chat', ['A guest mentioned you in a group chat', [] - ] + ], + 'message …' ], [ - Room::PUBLIC_CALL, ['userType' => 'users', 'userId' => 'testUser'], 'Test user', 'Room name', + Room::PUBLIC_CALL, ['userType' => 'users', 'userId' => 'testUser'], ['ellipsisStart', 'ellipsisEnd'], 'Test user', 'Room name', 'Test user mentioned you in a group chat: Room name', ['{user} mentioned you in a group chat: {call}', [ 'user' => ['type' => 'user', 'id' => 'testUser', 'name' => 'Test user'], 'call' => ['type' => 'call', 'id' => 'testRoomId', 'name' => 'Room name', 'call-type' => 'public'] ] - ] + ], + '… message …' ], [ - Room::PUBLIC_CALL, ['userType' => 'users', 'userId' => 'testUser'], null, 'Room name', + Room::PUBLIC_CALL, ['userType' => 'users', 'userId' => 'testUser'], [], null, 'Room name', 'A (now) deleted user mentioned you in a group chat: Room name', ['A (now) deleted user mentioned you in a group chat: {call}', [ 'call' => ['type' => 'call', 'id' => 'testRoomId', 'name' => 'Room name', 'call-type' => 'public'] ] ], - true - ], + 'message', + true], [ - Room::PUBLIC_CALL, ['userType' => 'guests', 'userId' => 'testSpreedSession'], null, 'Room name', + Room::PUBLIC_CALL, ['userType' => 'guests', 'userId' => 'testSpreedSession'], ['ellipsisStart', 'ellipsisEnd'], null, 'Room name', 'A guest mentioned you in a group chat: Room name', ['A guest mentioned you in a group chat: {call}', ['call' => ['type' => 'call', 'id' => 'testRoomId', 'name' => 'Room name', 'call-type' => 'public']] - ] + ], + '… message …' ] ]; } @@ -361,16 +368,18 @@ public function dataPrepareMention() { * @dataProvider dataPrepareMention * @param int $roomType * @param array $subjectParameters + * @param array $messageParameters * @param string $displayName * @param string $roomName * @param string $parsedSubject * @param array $richSubject + * @param string $parsedMessage * @param bool $deletedUser */ - public function testPrepareMention($roomType, $subjectParameters, $displayName, $roomName, $parsedSubject, $richSubject, $deletedUser = false) { + public function testPrepareMention($roomType, $subjectParameters, $messageParameters, $displayName, $roomName, $parsedSubject, $richSubject, $parsedMessage, $deletedUser = false) { $notification = $this->createMock(INotification::class); $l = $this->createMock(IL10N::class); - $l->expects($this->exactly(2)) + $l->expects($this->atLeast(2)) ->method('t') ->will($this->returnCallback(function($text, $parameters = []) { return vsprintf($text, $parameters); @@ -434,6 +443,10 @@ public function testPrepareMention($roomType, $subjectParameters, $displayName, ->method('setRichSubject') ->with($richSubject[0], $richSubject[1]) ->willReturnSelf(); + $notification->expects($this->once()) + ->method('setParsedMessage') + ->with($parsedMessage) + ->willReturnSelf(); $notification->expects($this->once()) ->method('getApp') @@ -447,6 +460,12 @@ public function testPrepareMention($roomType, $subjectParameters, $displayName, $notification->expects($this->once()) ->method('getObjectType') ->willReturn('room'); + $notification->expects($this->once()) + ->method('getMessage') + ->willReturn('message'); + $notification->expects($this->once()) + ->method('getMessageParameters') + ->willReturn($messageParameters); $this->assertEquals($notification, $this->notifier->prepare($notification, 'de')); } From c97c9c6b0eb58ff86f27cb8edf2832a06e7b40a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Thu, 2 Nov 2017 04:53:51 +0100 Subject: [PATCH 3/9] Do not notify mentioned users that can not participate in the chat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now mentioned users are notified only if they are already participants of a private chat (a one to one chat, a group chat, or a password protected public chat), or if the message was written in a public chat. Signed-off-by: Daniel Calviño Sánchez --- lib/Chat/Notifier.php | 53 +++++++- tests/php/Chat/NotifierTest.php | 206 +++++++++++++++++++++++++++++++- 2 files changed, 257 insertions(+), 2 deletions(-) diff --git a/lib/Chat/Notifier.php b/lib/Chat/Notifier.php index bab216f5b3c..c46f1e3ae20 100644 --- a/lib/Chat/Notifier.php +++ b/lib/Chat/Notifier.php @@ -23,6 +23,9 @@ namespace OCA\Spreed\Chat; +use OCA\Spreed\Exceptions\ParticipantNotFoundException; +use OCA\Spreed\Manager; +use OCA\Spreed\Room; use OCP\Comments\IComment; use OCP\Notification\IManager as INotificationManager; use OCP\Notification\INotification; @@ -43,14 +46,20 @@ class Notifier { /** @var IUserManager */ private $userManager; + /** @var Manager */ + private $manager; + /** * @param INotificationManager $notificationManager * @param IUserManager $userManager + * @param Manager $manager */ public function __construct(INotificationManager $notificationManager, - IUserManager $userManager) { + IUserManager $userManager, + Manager $manager) { $this->notificationManager = $notificationManager; $this->userManager = $userManager; + $this->manager = $manager; } /** @@ -59,6 +68,9 @@ public function __construct(INotificationManager $notificationManager, * The comment must be a chat message comment. That is, its "objectId" must * be the room ID. * + * Not every user mentioned in the message is notified, but only those that + * are able to participate in the room. + * * @param IComment $comment */ public function notifyMentionedUsers(IComment $comment) { @@ -199,6 +211,45 @@ private function shouldUserBeNotified($userId, IComment $comment) { return false; } + $roomId = $comment->getObjectId(); + if (!$this->isUserAbleToParticipateInRoom($userId, $roomId)) { + return false; + } + + return true; + } + + /** + * Returns whether the user with the given ID can participate in the room + * with the given ID or not. + * + * A user can participate in a one to one or a group room if she is a + * participant already. For public rooms, a user can participate if the room + * has no password, or if it has a password and the user is a participant + * already. + * + * @param string $userId + * @param int $roomId + * @return bool true if the user is active in the room, false otherwise. + */ + private function isUserAbleToParticipateInRoom($userId, $roomId) { + $room = $this->manager->getRoomById($roomId); + + $roomType = $room->getType(); + if ($roomType === Room::UNKNOWN_CALL) { + return false; + } + + if ($roomType === Room::PUBLIC_CALL && !$room->hasPassword()) { + return true; + } + + try { + $room->getParticipant($userId); + } catch (ParticipantNotFoundException $exception) { + return false; + } + return true; } diff --git a/tests/php/Chat/NotifierTest.php b/tests/php/Chat/NotifierTest.php index 915b8ef3a4e..1c269b97c6b 100644 --- a/tests/php/Chat/NotifierTest.php +++ b/tests/php/Chat/NotifierTest.php @@ -24,6 +24,9 @@ namespace OCA\Spreed\Tests\php\Chat; use OCA\Spreed\Chat\Notifier; +use OCA\Spreed\Exceptions\ParticipantNotFoundException; +use OCA\Spreed\Manager; +use OCA\Spreed\Room; use OCP\Comments\IComment; use OCP\Notification\IManager as INotificationManager; use OCP\Notification\INotification; @@ -37,6 +40,9 @@ class NotifierTest extends \Test\TestCase { /** @var \OCP\IUserManager|\PHPUnit_Framework_MockObject_MockObject */ protected $userManager; + /** @var \OCA\Spreed\Manager|\PHPUnit_Framework_MockObject_MockObject */ + protected $manager; + /** @var \OCA\Spreed\Chat\Notifier */ protected $notifier; @@ -56,8 +62,11 @@ public function setUp() { return true; })); + $this->manager = $this->createMock(Manager::class); + $this->notifier = new Notifier($this->notificationManager, - $this->userManager); + $this->userManager, + $this->manager); } private function newComment($id, $actorType, $actorId, $creationDateTime, $message) { @@ -73,6 +82,7 @@ private function newComment($id, $actorType, $actorId, $creationDateTime, $messa $comment = $this->createMock(IComment::class); $comment->method('getId')->willReturn($id); + $comment->method('getObjectId')->willReturn('roomId'); $comment->method('getActorType')->willReturn($actorType); $comment->method('getActorId')->willReturn($actorId); $comment->method('getCreationDateTime')->willReturn($creationDateTime); @@ -130,6 +140,21 @@ public function testNotifyMentionedUsers() { ->with($comment->getMessage()) ->willReturnSelf(); + $room = $this->createMock(Room::class); + $this->manager->expects($this->once()) + ->method('getRoomById') + ->with('roomId') + ->willReturn($room); + + $room->expects($this->once()) + ->method('getType') + ->willReturn(Room::ONE_TO_ONE_CALL); + + $room->expects($this->once()) + ->method('getParticipant') + ->with('anotherUser') + ->willReturn(true); + $this->notificationManager->expects($this->once()) ->method('notify') ->with($notification); @@ -156,6 +181,20 @@ public function testNotifyMentionedUsersByGuest() { ->with($comment->getMessage()) ->willReturnSelf(); + $room = $this->createMock(Room::class); + $this->manager->expects($this->once()) + ->method('getRoomById') + ->with('roomId') + ->willReturn($room); + + $room->expects($this->once()) + ->method('getType') + ->willReturn(Room::PUBLIC_CALL); + + $room->expects($this->once()) + ->method('hasPassword') + ->willReturn(false); + $this->notificationManager->expects($this->once()) ->method('notify') ->with($notification); @@ -183,6 +222,21 @@ public function testNotifyMentionedUsersWithLongMessageStartMention() { ->with('123456789 @anotherUserWithOddLengthName 123456789-123456789-1234', ['ellipsisEnd']) ->willReturnSelf(); + $room = $this->createMock(Room::class); + $this->manager->expects($this->once()) + ->method('getRoomById') + ->with('roomId') + ->willReturn($room); + + $room->expects($this->once()) + ->method('getType') + ->willReturn(Room::GROUP_CALL); + + $room->expects($this->once()) + ->method('getParticipant') + ->with('anotherUserWithOddLengthName') + ->willReturn(true); + $this->notificationManager->expects($this->once()) ->method('notify') ->with($notification); @@ -210,6 +264,21 @@ public function testNotifyMentionedUsersWithLongMessageMiddleMention() { ->with('89-123456789-1234 @anotherUserWithOddLengthName 6789-123456789-1', ['ellipsisStart', 'ellipsisEnd']) ->willReturnSelf(); + $room = $this->createMock(Room::class); + $this->manager->expects($this->once()) + ->method('getRoomById') + ->with('roomId') + ->willReturn($room); + + $room->expects($this->once()) + ->method('getType') + ->willReturn(Room::GROUP_CALL); + + $room->expects($this->once()) + ->method('getParticipant') + ->with('anotherUserWithOddLengthName') + ->willReturn(true); + $this->notificationManager->expects($this->once()) ->method('notify') ->with($notification); @@ -237,6 +306,21 @@ public function testNotifyMentionedUsersWithLongMessageEndMention() { ->with('6789-123456789-123456789 @anotherUserWithOddLengthName 123456789', ['ellipsisStart']) ->willReturnSelf(); + $room = $this->createMock(Room::class); + $this->manager->expects($this->once()) + ->method('getRoomById') + ->with('roomId') + ->willReturn($room); + + $room->expects($this->once()) + ->method('getType') + ->willReturn(Room::GROUP_CALL); + + $room->expects($this->once()) + ->method('getParticipant') + ->with('anotherUserWithOddLengthName') + ->willReturn(true); + $this->notificationManager->expects($this->once()) ->method('notify') ->with($notification); @@ -268,6 +352,112 @@ public function testNotifyMentionedUsersToUnknownUser() { $this->notifier->notifyMentionedUsers($comment); } + public function testNotifyMentionedUsersToUserNotInvitedToPrivateChat() { + $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'Mention @userNotInOneToOneChat'); + + $this->notificationManager->expects($this->never()) + ->method('createNotification'); + + $room = $this->createMock(Room::class); + $this->manager->expects($this->once()) + ->method('getRoomById') + ->with('roomId') + ->willReturn($room); + + $room->expects($this->once()) + ->method('getType') + ->willReturn(Room::ONE_TO_ONE_CALL); + + $room->expects($this->once()) + ->method('getParticipant') + ->with('userNotInOneToOneChat') + ->will($this->throwException(new ParticipantNotFoundException())); + + $this->notificationManager->expects($this->never()) + ->method('createNotification'); + + $this->notificationManager->expects($this->never()) + ->method('notify'); + + $this->notifier->notifyMentionedUsers($comment); + } + + public function testNotifyMentionedUsersToUserNotInvitedToPasswordProtectedPublicChat() { + $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'Mention @userNotInvitedToPasswordProtectedPublicChat'); + + $this->notificationManager->expects($this->never()) + ->method('createNotification'); + + $room = $this->createMock(Room::class); + $this->manager->expects($this->once()) + ->method('getRoomById') + ->with('roomId') + ->willReturn($room); + + $room->expects($this->once()) + ->method('getType') + ->willReturn(Room::PUBLIC_CALL); + + $room->expects($this->once()) + ->method('hasPassword') + ->willReturn(true); + + $room->expects($this->once()) + ->method('getParticipant') + ->with('userNotInvitedToPasswordProtectedPublicChat') + ->will($this->throwException(new ParticipantNotFoundException())); + + $this->notificationManager->expects($this->never()) + ->method('notify'); + + $this->notifier->notifyMentionedUsers($comment); + } + + public function testNotifyMentionedUsersToUserInvitedToPasswordProtectedPublicChat() { + $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'Mention @userInvitedToPasswordProtectedPublicChat'); + + $notification = $this->newNotification($comment); + + $this->notificationManager->expects($this->once()) + ->method('createNotification') + ->willReturn($notification); + + $notification->expects($this->once()) + ->method('setUser') + ->with('userInvitedToPasswordProtectedPublicChat') + ->willReturnSelf(); + + $notification->expects($this->once()) + ->method('setMessage') + ->with($comment->getMessage()) + ->willReturnSelf(); + + $room = $this->createMock(Room::class); + $this->manager->expects($this->once()) + ->method('getRoomById') + ->with('roomId') + ->willReturn($room); + + $room->expects($this->once()) + ->method('getType') + ->willReturn(Room::PUBLIC_CALL); + + $room->expects($this->once()) + ->method('hasPassword') + ->willReturn(true); + + $room->expects($this->once()) + ->method('getParticipant') + ->with('userInvitedToPasswordProtectedPublicChat') + ->willReturn(true); + + $this->notificationManager->expects($this->once()) + ->method('notify') + ->with($notification); + + $this->notifier->notifyMentionedUsers($comment); + } + public function testNotifyMentionedUsersNoMentions() { $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'No mentions'); @@ -313,6 +503,20 @@ public function testNotifyMentionedUsersSeveralMentions() { ->with('notherUser, and @unknownUser, and @testUser, and @userAbleToJoin') ->willReturnSelf(); + $room = $this->createMock(Room::class); + $this->manager->expects($this->exactly(2)) + ->method('getRoomById') + ->with('roomId') + ->willReturn($room); + + $room->expects($this->exactly(2)) + ->method('getType') + ->willReturn(Room::PUBLIC_CALL); + + $room->expects($this->exactly(2)) + ->method('hasPassword') + ->willReturn(false); + $this->notificationManager->expects($this->exactly(2)) ->method('notify') ->withConsecutive( From 9a780f461e643d0cb0f651abf01f4625feb73a65 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Nov 2017 10:35:29 +0100 Subject: [PATCH 4/9] Add missing import Signed-off-by: Joas Schilling --- lib/Chat/ChatManager.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/Chat/ChatManager.php b/lib/Chat/ChatManager.php index 0b6c5156ec7..81aa0f468bf 100644 --- a/lib/Chat/ChatManager.php +++ b/lib/Chat/ChatManager.php @@ -23,6 +23,7 @@ namespace OCA\Spreed\Chat; +use OCP\Comments\IComment; use OCP\Comments\ICommentsManager; /** From 6b528fb476cec5dfc65f848de019cd036eb63ceb Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Nov 2017 11:03:57 +0100 Subject: [PATCH 5/9] Use the dark icon which is visible Signed-off-by: Joas Schilling --- lib/Notification/Notifier.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index aa766870841..6e3caba2083 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -89,7 +89,7 @@ public function prepare(INotification $notification, $languageCode) { } $notification - ->setIcon($this->url->getAbsoluteURL($this->url->imagePath('spreed', 'app.svg'))) + ->setIcon($this->url->getAbsoluteURL($this->url->imagePath('spreed', 'app-dark.svg'))) ->setLink($this->url->linkToRouteAbsolute('spreed.Page.index') . '?token=' . $room->getToken()); $subject = $notification->getSubject(); From af85b84388dc8b7e8869c35254d99fb8cd4bc7fa Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Nov 2017 13:14:02 +0100 Subject: [PATCH 6/9] Only notify participants when they are not active Also removes notifications for public rooms where the user was not invited. Signed-off-by: Joas Schilling --- lib/Chat/Notifier.php | 56 ++++------- tests/php/Chat/NotifierTest.php | 172 ++++++++++++-------------------- 2 files changed, 84 insertions(+), 144 deletions(-) diff --git a/lib/Chat/Notifier.php b/lib/Chat/Notifier.php index c46f1e3ae20..9e5dfa1cbc2 100644 --- a/lib/Chat/Notifier.php +++ b/lib/Chat/Notifier.php @@ -24,6 +24,7 @@ namespace OCA\Spreed\Chat; use OCA\Spreed\Exceptions\ParticipantNotFoundException; +use OCA\Spreed\Exceptions\RoomNotFoundException; use OCA\Spreed\Manager; use OCA\Spreed\Room; use OCP\Comments\IComment; @@ -201,56 +202,37 @@ private function getNotificationMessage(IComment $comment, $mentionedUserId) { return [substr($message, $mentionMiddleIndex - ($maximumLength / 2), $maximumLength), ['ellipsisStart', 'ellipsisEnd']]; } - private function shouldUserBeNotified($userId, IComment $comment) { - if ($userId === $comment->getActorId()) { - // Do not notify the user if she mentioned herself - return false; - } - - if (!$this->userManager->userExists($userId)) { - return false; - } - - $roomId = $comment->getObjectId(); - if (!$this->isUserAbleToParticipateInRoom($userId, $roomId)) { - return false; - } - - return true; - } - /** - * Returns whether the user with the given ID can participate in the room - * with the given ID or not. + * Determinates whether a user should be notified about the mention: * - * A user can participate in a one to one or a group room if she is a - * participant already. For public rooms, a user can participate if the room - * has no password, or if it has a password and the user is a participant - * already. + * 1. The user did not mention themself + * 2. The user must exist + * 3. The user must be a participant of the room + * 4. The user must not be active in the room * * @param string $userId - * @param int $roomId - * @return bool true if the user is active in the room, false otherwise. + * @param IComment $comment + * @return bool */ - private function isUserAbleToParticipateInRoom($userId, $roomId) { - $room = $this->manager->getRoomById($roomId); - - $roomType = $room->getType(); - if ($roomType === Room::UNKNOWN_CALL) { + private function shouldUserBeNotified($userId, IComment $comment) { + if ($userId === $comment->getActorId()) { + // Do not notify the user if they mentioned themself return false; } - if ($roomType === Room::PUBLIC_CALL && !$room->hasPassword()) { - return true; + if (!$this->userManager->userExists($userId)) { + return false; } try { - $room->getParticipant($userId); - } catch (ParticipantNotFoundException $exception) { + $room = $this->manager->getRoomById($comment->getObjectId()); + $participant = $room->getParticipant($userId); + } catch (RoomNotFoundException $e) { + return false; + } catch (ParticipantNotFoundException $e) { return false; } - return true; + return $participant->getSessionId() === '0'; } - } diff --git a/tests/php/Chat/NotifierTest.php b/tests/php/Chat/NotifierTest.php index 1c269b97c6b..3fd7216d299 100644 --- a/tests/php/Chat/NotifierTest.php +++ b/tests/php/Chat/NotifierTest.php @@ -26,6 +26,7 @@ use OCA\Spreed\Chat\Notifier; use OCA\Spreed\Exceptions\ParticipantNotFoundException; use OCA\Spreed\Manager; +use OCA\Spreed\Participant; use OCA\Spreed\Room; use OCP\Comments\IComment; use OCP\Notification\IManager as INotificationManager; @@ -146,14 +147,15 @@ public function testNotifyMentionedUsers() { ->with('roomId') ->willReturn($room); - $room->expects($this->once()) - ->method('getType') - ->willReturn(Room::ONE_TO_ONE_CALL); + $participant = $this->createMock(Participant::class); + $participant->expects($this->once()) + ->method('getSessionId') + ->willReturn('0'); $room->expects($this->once()) ->method('getParticipant') ->with('anotherUser') - ->willReturn(true); + ->willReturn($participant); $this->notificationManager->expects($this->once()) ->method('notify') @@ -162,6 +164,35 @@ public function testNotifyMentionedUsers() { $this->notifier->notifyMentionedUsers($comment); } + public function testNotifyMentionedUsersActive() { + $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'Mention @anotherUser'); + + $this->notificationManager->expects($this->never()) + ->method('createNotification'); + + $this->notificationManager->expects($this->never()) + ->method('notify'); + + + $room = $this->createMock(Room::class); + $this->manager->expects($this->once()) + ->method('getRoomById') + ->with('roomId') + ->willReturn($room); + + $participant = $this->createMock(Participant::class); + $participant->expects($this->once()) + ->method('getSessionId') + ->willReturn('1'); + + $room->expects($this->once()) + ->method('getParticipant') + ->with('anotherUser') + ->willReturn($participant); + + $this->notifier->notifyMentionedUsers($comment); + } + public function testNotifyMentionedUsersByGuest() { $comment = $this->newComment(108, 'guests', 'testSpreedSession', new \DateTime('@' . 1000000016), 'Mention @anotherUser'); @@ -187,13 +218,15 @@ public function testNotifyMentionedUsersByGuest() { ->with('roomId') ->willReturn($room); - $room->expects($this->once()) - ->method('getType') - ->willReturn(Room::PUBLIC_CALL); + $participant = $this->createMock(Participant::class); + $participant->expects($this->once()) + ->method('getSessionId') + ->willReturn('0'); $room->expects($this->once()) - ->method('hasPassword') - ->willReturn(false); + ->method('getParticipant') + ->with('anotherUser') + ->willReturn($participant); $this->notificationManager->expects($this->once()) ->method('notify') @@ -228,14 +261,15 @@ public function testNotifyMentionedUsersWithLongMessageStartMention() { ->with('roomId') ->willReturn($room); - $room->expects($this->once()) - ->method('getType') - ->willReturn(Room::GROUP_CALL); + $participant = $this->createMock(Participant::class); + $participant->expects($this->once()) + ->method('getSessionId') + ->willReturn('0'); $room->expects($this->once()) ->method('getParticipant') ->with('anotherUserWithOddLengthName') - ->willReturn(true); + ->willReturn($participant); $this->notificationManager->expects($this->once()) ->method('notify') @@ -270,14 +304,15 @@ public function testNotifyMentionedUsersWithLongMessageMiddleMention() { ->with('roomId') ->willReturn($room); - $room->expects($this->once()) - ->method('getType') - ->willReturn(Room::GROUP_CALL); + $participant = $this->createMock(Participant::class); + $participant->expects($this->once()) + ->method('getSessionId') + ->willReturn('0'); $room->expects($this->once()) ->method('getParticipant') ->with('anotherUserWithOddLengthName') - ->willReturn(true); + ->willReturn($participant); $this->notificationManager->expects($this->once()) ->method('notify') @@ -312,14 +347,15 @@ public function testNotifyMentionedUsersWithLongMessageEndMention() { ->with('roomId') ->willReturn($room); - $room->expects($this->once()) - ->method('getType') - ->willReturn(Room::GROUP_CALL); + $participant = $this->createMock(Participant::class); + $participant->expects($this->once()) + ->method('getSessionId') + ->willReturn('0'); $room->expects($this->once()) ->method('getParticipant') ->with('anotherUserWithOddLengthName') - ->willReturn(true); + ->willReturn($participant); $this->notificationManager->expects($this->once()) ->method('notify') @@ -352,7 +388,7 @@ public function testNotifyMentionedUsersToUnknownUser() { $this->notifier->notifyMentionedUsers($comment); } - public function testNotifyMentionedUsersToUserNotInvitedToPrivateChat() { + public function testNotifyMentionedUsersToUserNotInvitedToChat() { $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'Mention @userNotInOneToOneChat'); $this->notificationManager->expects($this->never()) @@ -364,10 +400,6 @@ public function testNotifyMentionedUsersToUserNotInvitedToPrivateChat() { ->with('roomId') ->willReturn($room); - $room->expects($this->once()) - ->method('getType') - ->willReturn(Room::ONE_TO_ONE_CALL); - $room->expects($this->once()) ->method('getParticipant') ->with('userNotInOneToOneChat') @@ -382,82 +414,6 @@ public function testNotifyMentionedUsersToUserNotInvitedToPrivateChat() { $this->notifier->notifyMentionedUsers($comment); } - public function testNotifyMentionedUsersToUserNotInvitedToPasswordProtectedPublicChat() { - $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'Mention @userNotInvitedToPasswordProtectedPublicChat'); - - $this->notificationManager->expects($this->never()) - ->method('createNotification'); - - $room = $this->createMock(Room::class); - $this->manager->expects($this->once()) - ->method('getRoomById') - ->with('roomId') - ->willReturn($room); - - $room->expects($this->once()) - ->method('getType') - ->willReturn(Room::PUBLIC_CALL); - - $room->expects($this->once()) - ->method('hasPassword') - ->willReturn(true); - - $room->expects($this->once()) - ->method('getParticipant') - ->with('userNotInvitedToPasswordProtectedPublicChat') - ->will($this->throwException(new ParticipantNotFoundException())); - - $this->notificationManager->expects($this->never()) - ->method('notify'); - - $this->notifier->notifyMentionedUsers($comment); - } - - public function testNotifyMentionedUsersToUserInvitedToPasswordProtectedPublicChat() { - $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'Mention @userInvitedToPasswordProtectedPublicChat'); - - $notification = $this->newNotification($comment); - - $this->notificationManager->expects($this->once()) - ->method('createNotification') - ->willReturn($notification); - - $notification->expects($this->once()) - ->method('setUser') - ->with('userInvitedToPasswordProtectedPublicChat') - ->willReturnSelf(); - - $notification->expects($this->once()) - ->method('setMessage') - ->with($comment->getMessage()) - ->willReturnSelf(); - - $room = $this->createMock(Room::class); - $this->manager->expects($this->once()) - ->method('getRoomById') - ->with('roomId') - ->willReturn($room); - - $room->expects($this->once()) - ->method('getType') - ->willReturn(Room::PUBLIC_CALL); - - $room->expects($this->once()) - ->method('hasPassword') - ->willReturn(true); - - $room->expects($this->once()) - ->method('getParticipant') - ->with('userInvitedToPasswordProtectedPublicChat') - ->willReturn(true); - - $this->notificationManager->expects($this->once()) - ->method('notify') - ->with($notification); - - $this->notifier->notifyMentionedUsers($comment); - } - public function testNotifyMentionedUsersNoMentions() { $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'No mentions'); @@ -509,13 +465,15 @@ public function testNotifyMentionedUsersSeveralMentions() { ->with('roomId') ->willReturn($room); - $room->expects($this->exactly(2)) - ->method('getType') - ->willReturn(Room::PUBLIC_CALL); + $participant = $this->createMock(Participant::class); + $participant->expects($this->exactly(2)) + ->method('getSessionId') + ->willReturn('0'); $room->expects($this->exactly(2)) - ->method('hasPassword') - ->willReturn(false); + ->method('getParticipant') + ->withConsecutive(['anotherUser'], ['userAbleToJoin']) + ->willReturn($participant); $this->notificationManager->expects($this->exactly(2)) ->method('notify') From 2376d68d687205336ca387c62cac7200d7adb6df Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 20 Nov 2017 10:17:23 +0100 Subject: [PATCH 7/9] Also notify people which are in the call/room since they might not read the chat Signed-off-by: Joas Schilling --- lib/Chat/Notifier.php | 4 +-- tests/php/Chat/NotifierTest.php | 47 --------------------------------- 2 files changed, 2 insertions(+), 49 deletions(-) diff --git a/lib/Chat/Notifier.php b/lib/Chat/Notifier.php index 9e5dfa1cbc2..273d01922ee 100644 --- a/lib/Chat/Notifier.php +++ b/lib/Chat/Notifier.php @@ -226,13 +226,13 @@ private function shouldUserBeNotified($userId, IComment $comment) { try { $room = $this->manager->getRoomById($comment->getObjectId()); - $participant = $room->getParticipant($userId); + $room->getParticipant($userId); } catch (RoomNotFoundException $e) { return false; } catch (ParticipantNotFoundException $e) { return false; } - return $participant->getSessionId() === '0'; + return true; } } diff --git a/tests/php/Chat/NotifierTest.php b/tests/php/Chat/NotifierTest.php index 3fd7216d299..b17840e5f7c 100644 --- a/tests/php/Chat/NotifierTest.php +++ b/tests/php/Chat/NotifierTest.php @@ -148,9 +148,6 @@ public function testNotifyMentionedUsers() { ->willReturn($room); $participant = $this->createMock(Participant::class); - $participant->expects($this->once()) - ->method('getSessionId') - ->willReturn('0'); $room->expects($this->once()) ->method('getParticipant') @@ -164,35 +161,6 @@ public function testNotifyMentionedUsers() { $this->notifier->notifyMentionedUsers($comment); } - public function testNotifyMentionedUsersActive() { - $comment = $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'Mention @anotherUser'); - - $this->notificationManager->expects($this->never()) - ->method('createNotification'); - - $this->notificationManager->expects($this->never()) - ->method('notify'); - - - $room = $this->createMock(Room::class); - $this->manager->expects($this->once()) - ->method('getRoomById') - ->with('roomId') - ->willReturn($room); - - $participant = $this->createMock(Participant::class); - $participant->expects($this->once()) - ->method('getSessionId') - ->willReturn('1'); - - $room->expects($this->once()) - ->method('getParticipant') - ->with('anotherUser') - ->willReturn($participant); - - $this->notifier->notifyMentionedUsers($comment); - } - public function testNotifyMentionedUsersByGuest() { $comment = $this->newComment(108, 'guests', 'testSpreedSession', new \DateTime('@' . 1000000016), 'Mention @anotherUser'); @@ -219,9 +187,6 @@ public function testNotifyMentionedUsersByGuest() { ->willReturn($room); $participant = $this->createMock(Participant::class); - $participant->expects($this->once()) - ->method('getSessionId') - ->willReturn('0'); $room->expects($this->once()) ->method('getParticipant') @@ -262,9 +227,6 @@ public function testNotifyMentionedUsersWithLongMessageStartMention() { ->willReturn($room); $participant = $this->createMock(Participant::class); - $participant->expects($this->once()) - ->method('getSessionId') - ->willReturn('0'); $room->expects($this->once()) ->method('getParticipant') @@ -305,9 +267,6 @@ public function testNotifyMentionedUsersWithLongMessageMiddleMention() { ->willReturn($room); $participant = $this->createMock(Participant::class); - $participant->expects($this->once()) - ->method('getSessionId') - ->willReturn('0'); $room->expects($this->once()) ->method('getParticipant') @@ -348,9 +307,6 @@ public function testNotifyMentionedUsersWithLongMessageEndMention() { ->willReturn($room); $participant = $this->createMock(Participant::class); - $participant->expects($this->once()) - ->method('getSessionId') - ->willReturn('0'); $room->expects($this->once()) ->method('getParticipant') @@ -466,9 +422,6 @@ public function testNotifyMentionedUsersSeveralMentions() { ->willReturn($room); $participant = $this->createMock(Participant::class); - $participant->expects($this->exactly(2)) - ->method('getSessionId') - ->willReturn('0'); $room->expects($this->exactly(2)) ->method('getParticipant') From f4308ef428939f50fc4548fbc62c805338ee18ca Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 20 Nov 2017 10:36:18 +0100 Subject: [PATCH 8/9] Code cleanup Signed-off-by: Joas Schilling --- lib/Controller/ChatController.php | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/Controller/ChatController.php b/lib/Controller/ChatController.php index 0bd2644d8d5..b9073c07e15 100644 --- a/lib/Controller/ChatController.php +++ b/lib/Controller/ChatController.php @@ -30,8 +30,10 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCSController; +use OCP\Comments\IComment; use OCP\IRequest; use OCP\ISession; +use OCP\IUser; use OCP\IUserManager; class ChatController extends OCSController { @@ -137,7 +139,7 @@ public function sendMessage($token, $message) { // fits (except if there is no session, as the actorId should be // empty in that case but sha1('') would generate a hash too // instead of returning an empty string). - $actorId = $actorId? sha1($actorId): $actorId; + $actorId = $actorId ? sha1($actorId) : $actorId; } else { $actorType = 'users'; $actorId = $this->userId; @@ -149,7 +151,7 @@ public function sendMessage($token, $message) { $creationDateTime = new \DateTime('now', new \DateTimeZone('UTC')); - $this->chatManager->sendMessage(strval($room->getId()), $actorType, $actorId, $message, $creationDateTime); + $this->chatManager->sendMessage((string) $room->getId(), $actorType, $actorId, $message, $creationDateTime); return new DataResponse([], Http::STATUS_CREATED); } @@ -175,10 +177,10 @@ public function sendMessage($token, $message) { * sent. * * @param string $token the room token - * @param int offset optional, the first N messages to ignore - * @param int notOlderThanTimestamp optional, timestamp in seconds and UTC + * @param int $offset optional, the first N messages to ignore + * @param int $notOlderThanTimestamp optional, timestamp in seconds and UTC * time zone - * @param int timeout optional, the number of seconds to wait for new + * @param int $timeout optional, the number of seconds to wait for new * messages (30 by default, 60 at most) * @return DataResponse an array of chat messages, or "404 Not found" if the * room token was not valid; each chat message is an array with @@ -196,7 +198,7 @@ public function receiveMessages($token, $offset = 0, $notOlderThanTimestamp = 0, if ($notOlderThanTimestamp > 0) { $notOlderThan = new \DateTime(); $notOlderThan->setTimestamp($notOlderThanTimestamp); - $notOlderThan->setTimeZone(new \DateTimeZone('UTC')); + $notOlderThan->setTimezone(new \DateTimeZone('UTC')); } $maximumTimeout = 60; @@ -204,15 +206,13 @@ public function receiveMessages($token, $offset = 0, $notOlderThanTimestamp = 0, $timeout = $maximumTimeout; } - $comments = $this->chatManager->receiveMessages(strval($room->getId()), $timeout, $offset, $notOlderThan); + $comments = $this->chatManager->receiveMessages((string) $room->getId(), $timeout, $offset, $notOlderThan); - $userManager = $this->userManager; - - return new DataResponse(array_map(function($comment) use ($token, $userManager) { + return new DataResponse(array_map(function(IComment $comment) use ($token) { $displayName = null; if ($comment->getActorType() === 'users') { - $user = $userManager->get($comment->getActorId()); - $displayName = is_null($user) ? null : $user->getDisplayName(); + $user = $this->userManager->get($comment->getActorId()); + $displayName = $user instanceof IUser ? $user->getDisplayName() : null; } return [ From 335ad722c2e0e70067b906f39ad1f2e5617d10fc Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 20 Nov 2017 10:36:40 +0100 Subject: [PATCH 9/9] Mark mention notifications as read, when the user pulls the chat messages of the room Signed-off-by: Joas Schilling --- lib/Chat/ChatManager.php | 5 ++++- lib/Chat/Notifier.php | 23 +++++++++++++++++++++ lib/Controller/ChatController.php | 2 +- tests/php/Chat/ChatManagerTest.php | 18 +++++++++++++--- tests/php/Controller/ChatControllerTest.php | 10 ++++----- 5 files changed, 48 insertions(+), 10 deletions(-) diff --git a/lib/Chat/ChatManager.php b/lib/Chat/ChatManager.php index 81aa0f468bf..9938f00f0b5 100644 --- a/lib/Chat/ChatManager.php +++ b/lib/Chat/ChatManager.php @@ -90,6 +90,7 @@ public function sendMessage($chatId, $actorType, $actorId, $message, \DateTime $ * maximum time to wait must be set using the $timeout parameter. * * @param string $chatId + * @param string $userId * @param int $timeout the maximum number of seconds to wait for messages * @param int $offset optional, starting point * @param \DateTime|null $notOlderThan optional, the date and time of the @@ -98,7 +99,7 @@ public function sendMessage($chatId, $actorType, $actorId, $message, \DateTime $ * creation date and message are relevant), or an empty array if the * timeout expired. */ - public function receiveMessages($chatId, $timeout, $offset = 0, \DateTime $notOlderThan = null) { + public function receiveMessages($chatId, $userId, $timeout, $offset = 0, \DateTime $notOlderThan = null) { $comments = []; $commentsFound = false; @@ -114,6 +115,8 @@ public function receiveMessages($chatId, $timeout, $offset = 0, \DateTime $notOl } } + $this->notifier->markMentionNotificationsRead($chatId, $userId); + if ($commentsFound) { // The limit and offset of getForObject can not be based on the // number of comments, as more comments may have been added between diff --git a/lib/Chat/Notifier.php b/lib/Chat/Notifier.php index 273d01922ee..8e748e27e25 100644 --- a/lib/Chat/Notifier.php +++ b/lib/Chat/Notifier.php @@ -104,6 +104,29 @@ public function removePendingNotificationsForRoom($roomId) { $this->notificationManager->markProcessed($notification); } + /** + * Removes all the pending mention notifications for the room + * + * @param string $roomId + * @param string $userId + */ + public function markMentionNotificationsRead($roomId, $userId) { + + if ($userId === null || $userId === '') { + return; + } + + $notification = $this->notificationManager->createNotification(); + + $notification + ->setApp('spreed') + ->setObject('room', $roomId) + ->setSubject('mention') + ->setUser($userId); + + $this->notificationManager->markProcessed($notification); + } + /** * Returns the IDs of the users mentioned in the given comment. * diff --git a/lib/Controller/ChatController.php b/lib/Controller/ChatController.php index b9073c07e15..fee134b0c90 100644 --- a/lib/Controller/ChatController.php +++ b/lib/Controller/ChatController.php @@ -206,7 +206,7 @@ public function receiveMessages($token, $offset = 0, $notOlderThanTimestamp = 0, $timeout = $maximumTimeout; } - $comments = $this->chatManager->receiveMessages((string) $room->getId(), $timeout, $offset, $notOlderThan); + $comments = $this->chatManager->receiveMessages((string) $room->getId(), $this->userId, $timeout, $offset, $notOlderThan); return new DataResponse(array_map(function(IComment $comment) use ($token) { $displayName = null; diff --git a/tests/php/Chat/ChatManagerTest.php b/tests/php/Chat/ChatManagerTest.php index 131b11d99ab..22e470a704a 100644 --- a/tests/php/Chat/ChatManagerTest.php +++ b/tests/php/Chat/ChatManagerTest.php @@ -121,8 +121,12 @@ public function testReceiveMessages() { $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'testMessage1') ]); + $this->notifier->expects($this->once()) + ->method('markMentionNotificationsRead') + ->with('testChatId', 'userId'); + $timeout = 42; - $comments = $this->chatManager->receiveMessages('testChatId', $timeout, $offset, $notOlderThan); + $comments = $this->chatManager->receiveMessages('testChatId', 'userId', $timeout, $offset, $notOlderThan); $expected = [ $this->newComment(110, 'users', 'testUnknownUser', new \DateTime('@' . 1000000042), 'testMessage3'), $this->newComment(109, 'guests', 'testSpreedSession', new \DateTime('@' . 1000000023), 'testMessage2') @@ -154,8 +158,12 @@ public function testReceiveMessagesMoreCommentsThanExpected() { $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'testMessage1') ]); + $this->notifier->expects($this->once()) + ->method('markMentionNotificationsRead') + ->with('testChatId', 'userId'); + $timeout = 42; - $comments = $this->chatManager->receiveMessages('testChatId', $timeout, $offset, $notOlderThan); + $comments = $this->chatManager->receiveMessages('testChatId', 'userId', $timeout, $offset, $notOlderThan); $expected = [ $this->newComment(111, 'users', 'testUser', new \DateTime('@' . 1000000108), 'testMessage4'), $this->newComment(110, 'users', 'testUnknownUser', new \DateTime('@' . 1000000042), 'testMessage3'), @@ -185,8 +193,12 @@ public function testReceiveMessagesNoOffset() { $this->newComment(108, 'users', 'testUser', new \DateTime('@' . 1000000016), 'testMessage1') ]); + $this->notifier->expects($this->once()) + ->method('markMentionNotificationsRead') + ->with('testChatId', 'userId'); + $timeout = 42; - $comments = $this->chatManager->receiveMessages('testChatId', $timeout, $offset, $notOlderThan); + $comments = $this->chatManager->receiveMessages('testChatId', 'userId', $timeout, $offset, $notOlderThan); $expected = [ $this->newComment(110, 'users', 'testUnknownUser', new \DateTime('@' . 1000000042), 'testMessage3'), $this->newComment(109, 'guests', 'testSpreedSession', new \DateTime('@' . 1000000023), 'testMessage2'), diff --git a/tests/php/Controller/ChatControllerTest.php b/tests/php/Controller/ChatControllerTest.php index e349a75d729..827f4ac2c8b 100644 --- a/tests/php/Controller/ChatControllerTest.php +++ b/tests/php/Controller/ChatControllerTest.php @@ -319,7 +319,7 @@ public function testReceiveMessagesByUser() { $timestamp = 1000000000; $this->chatManager->expects($this->once()) ->method('receiveMessages') - ->with('1234', $timeout, $offset, new \DateTime('@' . $timestamp)) + ->with('1234', $this->userId, $timeout, $offset, new \DateTime('@' . $timestamp)) ->willReturn([ $this->newComment(111, 'users', 'testUser', new \DateTime('@' . 1000000016), 'testMessage4'), $this->newComment(110, 'users', 'testUnknownUser', new \DateTime('@' . 1000000015), 'testMessage3'), @@ -377,7 +377,7 @@ public function testReceiveMessagesByUserNotJoinedButInRoom() { $timestamp = 1000000000; $this->chatManager->expects($this->once()) ->method('receiveMessages') - ->with('1234', $timeout, $offset, new \DateTime('@' . $timestamp)) + ->with('1234', $this->userId, $timeout, $offset, new \DateTime('@' . $timestamp)) ->willReturn([ $this->newComment(111, 'users', 'testUser', new \DateTime('@' . 1000000016), 'testMessage4'), $this->newComment(110, 'users', 'testUnknownUser', new \DateTime('@' . 1000000015), 'testMessage3'), @@ -465,7 +465,7 @@ public function testReceiveMessagesByGuest() { $timestamp = 1000000000; $this->chatManager->expects($this->once()) ->method('receiveMessages') - ->with('1234', $timeout, $offset, new \DateTime('@' . $timestamp)) + ->with('1234', null, $timeout, $offset, new \DateTime('@' . $timestamp)) ->willReturn([ $this->newComment(111, 'users', 'testUser', new \DateTime('@' . 1000000016), 'testMessage4'), $this->newComment(110, 'users', 'testUnknownUser', new \DateTime('@' . 1000000015), 'testMessage3'), @@ -546,7 +546,7 @@ public function testReceiveMessagesTimeoutExpired() { $timestamp = 1000000000; $this->chatManager->expects($this->once()) ->method('receiveMessages') - ->with('1234', $timeout, $offset, new \DateTime('@' . $timestamp)) + ->with('1234', $this->userId, $timeout, $offset, new \DateTime('@' . $timestamp)) ->willReturn([]); $response = $this->controller->receiveMessages('testToken', $offset, $timestamp, $timeout); @@ -579,7 +579,7 @@ public function testReceiveMessagesTimeoutTooLarge() { $timestamp = 1000000000; $this->chatManager->expects($this->once()) ->method('receiveMessages') - ->with('1234', $maximumTimeout, $offset, new \DateTime('@' . $timestamp)) + ->with('1234', $this->userId, $maximumTimeout, $offset, new \DateTime('@' . $timestamp)) ->willReturn([]); $response = $this->controller->receiveMessages('testToken', $offset, $timestamp, $timeout);