Merge pull request #2982 from nextcloud/better-notification-links

Set the link of the notification on render instead of creation

7 files changed, 77 insertions, 74 deletions

diff --git a/apps/comments/lib/Notification/Listener.php b/apps/comments/lib/Notification/Listener.php
index d30c59c93d5..365f93ce8dd 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;
 	}
 
 	/**
@@ -100,11 +93,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 170538512d8..a9daef3031f 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;
diff --git a/apps/comments/tests/Unit/Notification/ListenerTest.php b/apps/comments/tests/Unit/Notification/ListenerTest.php
index 3007b78cb3d..ef84d1c60de 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
 		);
 	}
 
diff --git a/apps/updatenotification/lib/Notification/BackgroundJob.php b/apps/updatenotification/lib/Notification/BackgroundJob.php
index 7bcc0e86905..83a9bdb599a 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,10 +49,7 @@ class BackgroundJob extends TimedJob {
 	/** @var IClientService */
 	protected $client;
 
-	/** @var IURLGenerator */
-	protected $urlGenerator;
-
-	/** @var IUser[] */
+	/** @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) {
@@ -97,8 +90,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 +102,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 +112,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 +128,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 00cc94095ca..079ec4c5e0a 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,6 +89,10 @@ class Notifier implements INotifier {
 
 			$parameters = $notification->getSubjectParameters();
 			$notification->setParsedSubject($l->t('Update to %1$s is available.', [$parameters['version']]));
+
+			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'];
@@ -92,6 +109,10 @@ class Notifier implements INotifier {
 						'name' => $appName,
 					]
 				]);
+
+			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')));
@@ -113,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());
 	}
diff --git a/apps/updatenotification/tests/Notification/BackgroundJobTest.php b/apps/updatenotification/tests/Notification/BackgroundJobTest.php
index 911b1cc8e2f..57771ec0ae9 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 421fcada689..e809ce11635 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();