From e675c2a66c27605ff54a428e8f3ee76637649e25 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 9 Jan 2017 11:18:33 +0100 Subject: [PATCH 1/5] Set the comment link on render instead of creation Signed-off-by: Joas Schilling --- apps/comments/lib/Notification/Listener.php | 6 +----- apps/comments/lib/Notification/Notifier.php | 6 +++++- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/comments/lib/Notification/Listener.php b/apps/comments/lib/Notification/Listener.php index d30c59c93d..2f94b8a3d7 100644 --- a/apps/comments/lib/Notification/Listener.php +++ b/apps/comments/lib/Notification/Listener.php @@ -100,11 +100,7 @@ class Listener { ->setApp('comments') ->setObject('comment', $comment->getId()) ->setSubject('mention', [ $comment->getObjectType(), $comment->getObjectId() ]) - ->setDateTime($comment->getCreationDateTime()) - ->setLink($this->urlGenerator->linkToRouteAbsolute( - 'comments.Notifications.view', - ['id' => $comment->getId()]) - ); + ->setDateTime($comment->getCreationDateTime()); return $notification; } diff --git a/apps/comments/lib/Notification/Notifier.php b/apps/comments/lib/Notification/Notifier.php index 170538512d..a9daef3031 100644 --- a/apps/comments/lib/Notification/Notifier.php +++ b/apps/comments/lib/Notification/Notifier.php @@ -139,7 +139,11 @@ class Notifier implements INotifier { ] ); } - $notification->setIcon($this->url->getAbsoluteURL($this->url->imagePath('core', 'actions/comment.svg'))); + $notification->setIcon($this->url->getAbsoluteURL($this->url->imagePath('core', 'actions/comment.svg'))) + ->setLink($this->url->linkToRouteAbsolute( + 'comments.Notifications.view', + ['id' => $comment->getId()]) + ); return $notification; break; From acbf0d151ff4c720532b1befc0471b18457c337b Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 9 Jan 2017 11:18:59 +0100 Subject: [PATCH 2/5] Set the link of the notification on render instead of creation Signed-off-by: Joas Schilling --- .../lib/Notification/BackgroundJob.php | 14 +++++--------- .../lib/Notification/Notifier.php | 3 +++ 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/apps/updatenotification/lib/Notification/BackgroundJob.php b/apps/updatenotification/lib/Notification/BackgroundJob.php index 7bcc0e8690..2ee3d766ea 100644 --- a/apps/updatenotification/lib/Notification/BackgroundJob.php +++ b/apps/updatenotification/lib/Notification/BackgroundJob.php @@ -54,7 +54,7 @@ class BackgroundJob extends TimedJob { /** @var IURLGenerator */ protected $urlGenerator; - /** @var IUser[] */ + /** @var string[] */ protected $users; /** @@ -97,8 +97,7 @@ class BackgroundJob extends TimedJob { $status = $updater->check(); if (isset($status['version'])) { - $url = $this->urlGenerator->linkToRouteAbsolute('settings.AdminSettings.index') . '#updater'; - $this->createNotifications('core', $status['version'], $url, $status['versionstring']); + $this->createNotifications('core', $status['version'], $status['versionstring']); } } @@ -110,8 +109,7 @@ class BackgroundJob extends TimedJob { foreach ($apps as $app) { $update = $this->isUpdateAvailable($app); if ($update !== false) { - $url = $this->urlGenerator->linkToRouteAbsolute('settings.AppSettings.viewApps') . '#app-' . $app; - $this->createNotifications($app, $update, $url); + $this->createNotifications($app, $update); } } } @@ -121,10 +119,9 @@ class BackgroundJob extends TimedJob { * * @param string $app * @param string $version - * @param string $url * @param string $visibleVersion */ - protected function createNotifications($app, $version, $url, $visibleVersion = '') { + protected function createNotifications($app, $version, $visibleVersion = '') { $lastNotification = $this->config->getAppValue('updatenotification', $app, false); if ($lastNotification === $version) { // We already notified about this update @@ -138,8 +135,7 @@ class BackgroundJob extends TimedJob { $notification = $this->notificationManager->createNotification(); $notification->setApp('updatenotification') ->setDateTime(new \DateTime()) - ->setObject($app, $version) - ->setLink($url); + ->setObject($app, $version); if ($visibleVersion !== '') { $notification->setSubject('update_available', ['version' => $visibleVersion]); diff --git a/apps/updatenotification/lib/Notification/Notifier.php b/apps/updatenotification/lib/Notification/Notifier.php index 00cc94095c..b23b7fe84d 100644 --- a/apps/updatenotification/lib/Notification/Notifier.php +++ b/apps/updatenotification/lib/Notification/Notifier.php @@ -76,6 +76,7 @@ class Notifier implements INotifier { $parameters = $notification->getSubjectParameters(); $notification->setParsedSubject($l->t('Update to %1$s is available.', [$parameters['version']])); + $notification->setLink($this->url->linkToRouteAbsolute('settings.AdminSettings.index') . '#updater'); } else { $appInfo = $this->getAppInfo($notification->getObjectType()); $appName = ($appInfo === null) ? $notification->getObjectType() : $appInfo['name']; @@ -92,6 +93,8 @@ class Notifier implements INotifier { 'name' => $appName, ] ]); + + $notification->setLink($this->url->linkToRouteAbsolute('settings.AppSettings.viewApps') . '#app-' . $notification->getObjectType()); } $notification->setIcon($this->url->getAbsoluteURL($this->url->imagePath('updatenotification', 'notification.svg'))); From 315645ada343946aa3c27f874b36b7ded6b3ff50 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 9 Jan 2017 11:24:09 +0100 Subject: [PATCH 3/5] Only add the link when the user can follow it. Signed-off-by: Joas Schilling --- .../lib/Notification/Notifier.php | 37 +++++++++++++++++-- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/apps/updatenotification/lib/Notification/Notifier.php b/apps/updatenotification/lib/Notification/Notifier.php index b23b7fe84d..079ec4c5e0 100644 --- a/apps/updatenotification/lib/Notification/Notifier.php +++ b/apps/updatenotification/lib/Notification/Notifier.php @@ -24,7 +24,10 @@ namespace OCA\UpdateNotification\Notification; +use OCP\IGroupManager; use OCP\IURLGenerator; +use OCP\IUser; +use OCP\IUserSession; use OCP\L10N\IFactory; use OCP\Notification\IManager; use OCP\Notification\INotification; @@ -41,6 +44,12 @@ class Notifier implements INotifier { /** @var IFactory */ protected $l10NFactory; + /** @var IUserSession */ + protected $userSession; + + /** @var IGroupManager */ + protected $groupManager; + /** @var string[] */ protected $appVersions; @@ -50,11 +59,15 @@ class Notifier implements INotifier { * @param IURLGenerator $url * @param IManager $notificationManager * @param IFactory $l10NFactory + * @param IUserSession $userSession + * @param IGroupManager $groupManager */ - public function __construct(IURLGenerator $url, IManager $notificationManager, IFactory $l10NFactory) { + public function __construct(IURLGenerator $url, IManager $notificationManager, IFactory $l10NFactory, IUserSession $userSession, IGroupManager $groupManager) { $this->url = $url; $this->notificationManager = $notificationManager; $this->l10NFactory = $l10NFactory; + $this->userSession = $userSession; + $this->groupManager = $groupManager; $this->appVersions = $this->getAppVersions(); } @@ -76,7 +89,10 @@ class Notifier implements INotifier { $parameters = $notification->getSubjectParameters(); $notification->setParsedSubject($l->t('Update to %1$s is available.', [$parameters['version']])); - $notification->setLink($this->url->linkToRouteAbsolute('settings.AdminSettings.index') . '#updater'); + + if ($this->isAdmin()) { + $notification->setLink($this->url->linkToRouteAbsolute('settings.AdminSettings.index') . '#updater'); + } } else { $appInfo = $this->getAppInfo($notification->getObjectType()); $appName = ($appInfo === null) ? $notification->getObjectType() : $appInfo['name']; @@ -94,7 +110,9 @@ class Notifier implements INotifier { ] ]); - $notification->setLink($this->url->linkToRouteAbsolute('settings.AppSettings.viewApps') . '#app-' . $notification->getObjectType()); + if ($this->isAdmin()) { + $notification->setLink($this->url->linkToRouteAbsolute('settings.AppSettings.viewApps') . '#app-' . $notification->getObjectType()); + } } $notification->setIcon($this->url->getAbsoluteURL($this->url->imagePath('updatenotification', 'notification.svg'))); @@ -116,6 +134,19 @@ class Notifier implements INotifier { } } + /** + * @return bool + */ + protected function isAdmin() { + $user = $this->userSession->getUser(); + + if ($user instanceof IUser) { + return $this->groupManager->isAdmin($user->getUID()); + } + + return false; + } + protected function getCoreVersions() { return implode('.', \OCP\Util::getVersion()); } From bb84db02cb70233b5e989f596a8eda059ba38dc7 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 9 Jan 2017 12:03:34 +0100 Subject: [PATCH 4/5] Cleanup comment tests Signed-off-by: Joas Schilling --- apps/comments/lib/Notification/Listener.php | 9 +-------- apps/comments/tests/Unit/Notification/ListenerTest.php | 8 +++----- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/apps/comments/lib/Notification/Listener.php b/apps/comments/lib/Notification/Listener.php index 2f94b8a3d7..365f93ce8d 100644 --- a/apps/comments/lib/Notification/Listener.php +++ b/apps/comments/lib/Notification/Listener.php @@ -23,7 +23,6 @@ namespace OCA\Comments\Notification; use OCP\Comments\CommentsEvent; use OCP\Comments\IComment; -use OCP\IURLGenerator; use OCP\IUserManager; use OCP\Notification\IManager; @@ -34,25 +33,19 @@ class Listener { /** @var IUserManager */ protected $userManager; - /** @var IURLGenerator */ - protected $urlGenerator; - /** * Listener constructor. * * @param IManager $notificationManager * @param IUserManager $userManager - * @param IURLGenerator $urlGenerator */ public function __construct( IManager $notificationManager, - IUserManager $userManager, - IURLGenerator $urlGenerator + IUserManager $userManager ) { $this->notificationManager = $notificationManager; $this->userManager = $userManager; - $this->urlGenerator = $urlGenerator; } /** diff --git a/apps/comments/tests/Unit/Notification/ListenerTest.php b/apps/comments/tests/Unit/Notification/ListenerTest.php index 3007b78cb3..ef84d1c60d 100644 --- a/apps/comments/tests/Unit/Notification/ListenerTest.php +++ b/apps/comments/tests/Unit/Notification/ListenerTest.php @@ -46,14 +46,12 @@ class ListenerTest extends TestCase { protected function setUp() { parent::setUp(); - $this->notificationManager = $this->getMockBuilder('\OCP\Notification\IManager')->getMock(); - $this->userManager = $this->getMockBuilder('\OCP\IUserManager')->getMock(); - $this->urlGenerator = $this->getMockBuilder('OCP\IURLGenerator')->getMock(); + $this->notificationManager = $this->createMock(\OCP\Notification\IManager::class); + $this->userManager = $this->createMock(\OCP\IUserManager::class); $this->listener = new Listener( $this->notificationManager, - $this->userManager, - $this->urlGenerator + $this->userManager ); } From b29149402c5edc54c679d27853636be69bba37ea Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 9 Jan 2017 12:09:47 +0100 Subject: [PATCH 5/5] Fix update notification tests Signed-off-by: Joas Schilling --- .../lib/Notification/BackgroundJob.php | 9 +--- .../tests/Notification/BackgroundJobTest.php | 49 +++++-------------- .../tests/Notification/NotifierTest.php | 14 +++++- 3 files changed, 27 insertions(+), 45 deletions(-) diff --git a/apps/updatenotification/lib/Notification/BackgroundJob.php b/apps/updatenotification/lib/Notification/BackgroundJob.php index 2ee3d766ea..83a9bdb599 100644 --- a/apps/updatenotification/lib/Notification/BackgroundJob.php +++ b/apps/updatenotification/lib/Notification/BackgroundJob.php @@ -30,8 +30,6 @@ use OCP\Http\Client\IClientService; use OCP\IConfig; use OCP\IGroup; use OCP\IGroupManager; -use OCP\IURLGenerator; -use OCP\IUser; use OCP\Notification\IManager; class BackgroundJob extends TimedJob { @@ -51,9 +49,6 @@ class BackgroundJob extends TimedJob { /** @var IClientService */ protected $client; - /** @var IURLGenerator */ - protected $urlGenerator; - /** @var string[] */ protected $users; @@ -65,9 +60,8 @@ class BackgroundJob extends TimedJob { * @param IGroupManager $groupManager * @param IAppManager $appManager * @param IClientService $client - * @param IURLGenerator $urlGenerator */ - public function __construct(IConfig $config, IManager $notificationManager, IGroupManager $groupManager, IAppManager $appManager, IClientService $client, IURLGenerator $urlGenerator) { + public function __construct(IConfig $config, IManager $notificationManager, IGroupManager $groupManager, IAppManager $appManager, IClientService $client) { // Run once a day $this->setInterval(60 * 60 * 24); @@ -76,7 +70,6 @@ class BackgroundJob extends TimedJob { $this->groupManager = $groupManager; $this->appManager = $appManager; $this->client = $client; - $this->urlGenerator = $urlGenerator; } protected function run($argument) { diff --git a/apps/updatenotification/tests/Notification/BackgroundJobTest.php b/apps/updatenotification/tests/Notification/BackgroundJobTest.php index 911b1cc8e2..57771ec0ae 100644 --- a/apps/updatenotification/tests/Notification/BackgroundJobTest.php +++ b/apps/updatenotification/tests/Notification/BackgroundJobTest.php @@ -45,18 +45,15 @@ class BackgroundJobTest extends TestCase { protected $appManager; /** @var IClientService|\PHPUnit_Framework_MockObject_MockObject */ protected $client; - /** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */ - protected $urlGenerator; public function setUp() { parent::setUp(); - $this->config = $this->getMockBuilder('OCP\IConfig')->getMock(); - $this->notificationManager = $this->getMockBuilder('OCP\Notification\IManager')->getMock(); - $this->groupManager = $this->getMockBuilder('OCP\IGroupManager')->getMock(); - $this->appManager = $this->getMockBuilder('OCP\App\IAppManager')->getMock(); - $this->client = $this->getMockBuilder('OCP\Http\Client\IClientService')->getMock(); - $this->urlGenerator = $this->getMockBuilder('OCP\IURLGenerator')->getMock(); + $this->config = $this->createMock(\OCP\IConfig::class); + $this->notificationManager = $this->createMock(\OCP\Notification\IManager::class); + $this->groupManager = $this->createMock(\OCP\IGroupManager::class); + $this->appManager = $this->createMock(\OCP\App\IAppManager::class); + $this->client = $this->createMock(\OCP\Http\Client\IClientService::class); } /** @@ -70,8 +67,7 @@ class BackgroundJobTest extends TestCase { $this->notificationManager, $this->groupManager, $this->appManager, - $this->client, - $this->urlGenerator + $this->client ); } { return $this->getMockBuilder('OCA\UpdateNotification\Notification\BackgroundJob') @@ -81,7 +77,6 @@ class BackgroundJobTest extends TestCase { $this->groupManager, $this->appManager, $this->client, - $this->urlGenerator, ]) ->setMethods($methods) ->getMock(); @@ -160,20 +155,12 @@ class BackgroundJobTest extends TestCase { } if ($notification === null) { - $this->urlGenerator->expects($this->never()) - ->method('linkToRouteAbsolute'); - $job->expects($this->never()) ->method('createNotifications'); } else { - $this->urlGenerator->expects($this->once()) - ->method('linkToRouteAbsolute') - ->with('settings.AdminSettings.index') - ->willReturn('admin-url'); - $job->expects($this->once()) ->method('createNotifications') - ->willReturn('core', $notification, 'admin-url#updater', $readableVersion); + ->willReturn('core', $notification, $readableVersion); } $this->invokePrivate($job, 'checkCoreUpdate'); @@ -188,7 +175,7 @@ class BackgroundJobTest extends TestCase { ['app2', '1.9.2'], ], [ - ['app2', '1.9.2', 'apps-url#app-app2'], + ['app2', '1.9.2'], ], ], ]; @@ -215,11 +202,6 @@ class BackgroundJobTest extends TestCase { ->method('isUpdateAvailable') ->willReturnMap($isUpdateAvailable); - $this->urlGenerator->expects($this->exactly(sizeof($notifications))) - ->method('linkToRouteAbsolute') - ->with('settings.AppSettings.viewApps') - ->willReturn('apps-url'); - $mockedMethod = $job->expects($this->exactly(sizeof($notifications))) ->method('createNotifications'); call_user_func_array([$mockedMethod, 'withConsecutive'], $notifications); @@ -229,9 +211,9 @@ class BackgroundJobTest extends TestCase { public function dataCreateNotifications() { return [ - ['app1', '1.0.0', 'link1', '1.0.0', false, false, null, null], - ['app2', '1.0.1', 'link2', '1.0.0', '1.0.0', true, ['user1'], [['user1']]], - ['app3', '1.0.1', 'link3', false, false, true, ['user2', 'user3'], [['user2'], ['user3']]], + ['app1', '1.0.0', '1.0.0', false, false, null, null], + ['app2', '1.0.1', '1.0.0', '1.0.0', true, ['user1'], [['user1']]], + ['app3', '1.0.1', false, false, true, ['user2', 'user3'], [['user2'], ['user3']]], ]; } @@ -240,14 +222,13 @@ class BackgroundJobTest extends TestCase { * * @param string $app * @param string $version - * @param string $url * @param string|false $lastNotification * @param string|false $callDelete * @param bool $createNotification * @param string[]|null $users * @param array|null $userNotifications */ - public function testCreateNotifications($app, $version, $url, $lastNotification, $callDelete, $createNotification, $users, $userNotifications) { + public function testCreateNotifications($app, $version, $lastNotification, $callDelete, $createNotification, $users, $userNotifications) { $job = $this->getJob([ 'deleteOutdatedNotifications', 'getUsersToNotify', @@ -299,10 +280,6 @@ class BackgroundJobTest extends TestCase { ->method('setSubject') ->with('update_available') ->willReturnSelf(); - $notification->expects($this->once()) - ->method('setLink') - ->with($url) - ->willReturnSelf(); if ($userNotifications !== null) { $mockedMethod = $notification->expects($this->exactly(sizeof($userNotifications))) @@ -323,7 +300,7 @@ class BackgroundJobTest extends TestCase { ->method('createNotification'); } - $this->invokePrivate($job, 'createNotifications', [$app, $version, $url]); + $this->invokePrivate($job, 'createNotifications', [$app, $version]); } public function dataGetUsersToNotify() { diff --git a/apps/updatenotification/tests/Notification/NotifierTest.php b/apps/updatenotification/tests/Notification/NotifierTest.php index 421fcada68..e809ce1163 100644 --- a/apps/updatenotification/tests/Notification/NotifierTest.php +++ b/apps/updatenotification/tests/Notification/NotifierTest.php @@ -24,7 +24,9 @@ namespace OCA\UpdateNotification\Tests\Notification; use OCA\UpdateNotification\Notification\Notifier; +use OCP\IGroupManager; use OCP\IURLGenerator; +use OCP\IUserSession; use OCP\L10N\IFactory; use OCP\Notification\IManager; use OCP\Notification\INotification; @@ -38,6 +40,10 @@ class NotifierTest extends TestCase { protected $notificationManager; /** @var IFactory|\PHPUnit_Framework_MockObject_MockObject */ protected $l10nFactory; + /** @var IUserSession|\PHPUnit_Framework_MockObject_MockObject */ + protected $userSession; + /** @var IGroupManager|\PHPUnit_Framework_MockObject_MockObject */ + protected $groupManager; public function setUp() { parent::setUp(); @@ -45,6 +51,8 @@ class NotifierTest extends TestCase { $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->notificationManager = $this->createMock(IManager::class); $this->l10nFactory = $this->createMock(IFactory::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->groupManager = $this->createMock(IGroupManager::class); } /** @@ -56,7 +64,9 @@ class NotifierTest extends TestCase { return new Notifier( $this->urlGenerator, $this->notificationManager, - $this->l10nFactory + $this->l10nFactory, + $this->userSession, + $this->groupManager ); } { return $this->getMockBuilder(Notifier::class) @@ -64,6 +74,8 @@ class NotifierTest extends TestCase { $this->urlGenerator, $this->notificationManager, $this->l10nFactory, + $this->userSession, + $this->groupManager, ]) ->setMethods($methods) ->getMock();