Merge pull request #3895 from nextcloud/create-notification-when-update-server-not-reachable

Create a notification when the update server couldn't be reached for …

5 files changed, 158 insertions, 52 deletions

diff --git a/apps/updatenotification/lib/Notification/BackgroundJob.php b/apps/updatenotification/lib/Notification/BackgroundJob.php
index 83a9bdb599a..9155b9d2556 100644
--- a/apps/updatenotification/lib/Notification/BackgroundJob.php
+++ b/apps/updatenotification/lib/Notification/BackgroundJob.php
@@ -34,6 +34,8 @@ use OCP\Notification\IManager;
 
 class BackgroundJob extends TimedJob {
 
+	protected $connectionNotifications = [3, 7, 14, 30];
+
 	/** @var IConfig */
 	protected $config;
 
@@ -81,7 +83,7 @@ class BackgroundJob extends TimedJob {
 	 * Check for ownCloud update
 	 */
 	protected function checkCoreUpdate() {
-		if (in_array($this->getChannel(), ['daily', 'git'])) {
+		if (in_array($this->getChannel(), ['daily', 'git'], true)) {
 			// "These aren't the update channels you're looking for." - Ben Obi-Wan Kenobi
 			return;
 		}
@@ -89,12 +91,62 @@ class BackgroundJob extends TimedJob {
 		$updater = $this->createVersionCheck();
 
 		$status = $updater->check();
-		if (isset($status['version'])) {
-			$this->createNotifications('core', $status['version'], $status['versionstring']);
+		if ($status === false) {
+			$errors = 1 + (int) $this->config->getAppValue('updatenotification', 'update_check_errors', 0);
+			$this->config->setAppValue('updatenotification', 'update_check_errors', $errors);
+
+			if (in_array($errors, $this->connectionNotifications, true)) {
+				$this->sendErrorNotifications($errors);
+			}
+		} else if (is_array($status)) {
+			$this->config->setAppValue('updatenotification', 'update_check_errors', 0);
+			$this->clearErrorNotifications();
+
+			if (isset($status['version'])) {
+				$this->createNotifications('core', $status['version'], $status['versionstring']);
+			}
 		}
 	}
 
 	/**
+	 * Send a message to the admin when the update server could not be reached
+	 * @param int $numDays
+	 */
+	protected function sendErrorNotifications($numDays) {
+		$this->clearErrorNotifications();
+
+		$notification = $this->notificationManager->createNotification();
+		try {
+			$notification->setApp('updatenotification')
+				->setDateTime(new \DateTime())
+				->setObject('updatenotification', 'error')
+				->setSubject('connection_error', ['days' => $numDays]);
+
+			foreach ($this->getUsersToNotify() as $uid) {
+				$notification->setUser($uid);
+				$this->notificationManager->notify($notification);
+			}
+		} catch (\InvalidArgumentException $e) {
+			return;
+		}
+	}
+
+	/**
+	 * Remove error notifications again
+	 */
+	protected function clearErrorNotifications() {
+		$notification = $this->notificationManager->createNotification();
+		try {
+			$notification->setApp('updatenotification')
+				->setSubject('connection_error')
+				->setObject('updatenotification', 'error');
+		} catch (\InvalidArgumentException $e) {
+			return;
+		}
+		$this->notificationManager->markProcessed($notification);
+	}
+
+	/**
 	 * Check all installed apps for updates
 	 */
 	protected function checkAppUpdates() {
diff --git a/apps/updatenotification/lib/Notification/Notifier.php b/apps/updatenotification/lib/Notification/Notifier.php
index 079ec4c5e0a..bef57b0d867 100644
--- a/apps/updatenotification/lib/Notification/Notifier.php
+++ b/apps/updatenotification/lib/Notification/Notifier.php
@@ -24,6 +24,7 @@
 namespace OCA\UpdateNotification\Notification;
 
 
+use OCP\IConfig;
 use OCP\IGroupManager;
 use OCP\IURLGenerator;
 use OCP\IUser;
@@ -38,6 +39,9 @@ class Notifier implements INotifier {
 	/** @var IURLGenerator */
 	protected $url;
 
+	/** @var IConfig */
+	protected $config;
+
 	/** @var IManager */
 	protected $notificationManager;
 
@@ -57,14 +61,16 @@ class Notifier implements INotifier {
 	 * Notifier constructor.
 	 *
 	 * @param IURLGenerator $url
+	 * @param IConfig $config
 	 * @param IManager $notificationManager
 	 * @param IFactory $l10NFactory
 	 * @param IUserSession $userSession
 	 * @param IGroupManager $groupManager
 	 */
-	public function __construct(IURLGenerator $url, IManager $notificationManager, IFactory $l10NFactory, IUserSession $userSession, IGroupManager $groupManager) {
+	public function __construct(IURLGenerator $url, IConfig $config, IManager $notificationManager, IFactory $l10NFactory, IUserSession $userSession, IGroupManager $groupManager) {
 		$this->url = $url;
 		$this->notificationManager = $notificationManager;
+		$this->config = $config;
 		$this->l10NFactory = $l10NFactory;
 		$this->userSession = $userSession;
 		$this->groupManager = $groupManager;
@@ -84,7 +90,16 @@ class Notifier implements INotifier {
 		}
 
 		$l = $this->l10NFactory->get('updatenotification', $languageCode);
-		if ($notification->getObjectType() === 'core') {
+		if ($notification->getSubject() === 'connection_error') {
+			$errors = (int) $this->config->getAppValue('updatenotification', 'update_check_errors', 0);
+			if ($errors === 0) {
+				$this->notificationManager->markProcessed($notification);
+				throw new \InvalidArgumentException();
+			}
+
+			$notification->setParsedSubject($l->t('The update server could not be reached since %d days to check for new updates.', [$errors]))
+				->setParsedMessage($l->t('Please check the nextcloud and server log files for errors.'));
+		} elseif ($notification->getObjectType() === 'core') {
 			$this->updateAlreadyInstalledCheck($notification, $this->getCoreVersions());
 
 			$parameters = $notification->getSubjectParameters();
diff --git a/apps/updatenotification/tests/Notification/BackgroundJobTest.php b/apps/updatenotification/tests/Notification/BackgroundJobTest.php
index 57771ec0ae9..92a8a687f5a 100644
--- a/apps/updatenotification/tests/Notification/BackgroundJobTest.php
+++ b/apps/updatenotification/tests/Notification/BackgroundJobTest.php
@@ -28,10 +28,12 @@ use OCP\App\IAppManager;
 use OCP\Http\Client\IClientService;
 use OCP\IConfig;
 use OCP\IGroupManager;
-use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\Notification\IManager;
 use Test\TestCase;
+use OC\Updater\VersionCheck;
+use OCP\Notification\INotification;
+use OCP\IGroup;
 
 class BackgroundJobTest extends TestCase {
 
@@ -49,11 +51,11 @@ class BackgroundJobTest extends TestCase {
 	public function setUp() {
 		parent::setUp();
 
-		$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);
+		$this->config = $this->createMock(IConfig::class);
+		$this->notificationManager = $this->createMock(IManager::class);
+		$this->groupManager = $this->createMock(IGroupManager::class);
+		$this->appManager = $this->createMock(IAppManager::class);
+		$this->client = $this->createMock(IClientService::class);
 	}
 
 	/**
@@ -70,7 +72,7 @@ class BackgroundJobTest extends TestCase {
 				$this->client
 			);
 		} {
-			return $this->getMockBuilder('OCA\UpdateNotification\Notification\BackgroundJob')
+			return $this->getMockBuilder(BackgroundJob::class)
 				->setConstructorArgs([
 					$this->config,
 					$this->notificationManager,
@@ -94,28 +96,34 @@ class BackgroundJobTest extends TestCase {
 		$job->expects($this->once())
 			->method('checkAppUpdates');
 
-		$this->invokePrivate($job, 'run', [null]);
+		self::invokePrivate($job, 'run', [null]);
 	}
 
 	public function dataCheckCoreUpdate() {
 		return [
-			['daily', null, null, null],
-			['git', null, null, null],
-			['beta', false, null, null],
+			['daily', null, null, null, null],
+			['git', null, null, null, null],
+			['beta', [], null, null, null],
+			['beta', false, false, null, null],
+			['beta', false, false, null, 13],
 			['beta', [
 				'version' => '9.2.0',
 				'versionstring' => 'Nextcloud 11.0.0',
-			], '9.2.0', 'Nextcloud 11.0.0'],
-			['stable', false, null, null],
+			], '9.2.0', 'Nextcloud 11.0.0', null],
+			['stable', [], null, null, null],
+			['stable', false, false, null, null],
+			['stable', false, false, null, 6],
 			['stable', [
 				'version' => '9.2.0',
 				'versionstring' => 'Nextcloud 11.0.0',
-			], '9.2.0', 'Nextcloud 11.0.0'],
-			['production', false, null, null],
+			], '9.2.0', 'Nextcloud 11.0.0', null],
+			['production', [], null, null, null],
+			['production', false, false, null, null],
+			['production', false, false, null, 2],
 			['production', [
 				'version' => '9.2.0',
 				'versionstring' => 'Nextcloud 11.0.0',
-			], '9.2.0', 'Nextcloud 11.0.0'],
+			], '9.2.0', 'Nextcloud 11.0.0', null],
 		];
 	}
 
@@ -124,14 +132,17 @@ class BackgroundJobTest extends TestCase {
 	 *
 	 * @param string $channel
 	 * @param mixed $versionCheck
-	 * @param null|string $notification
+	 * @param null|string $version
 	 * @param null|string $readableVersion
+	 * @param null|int $errorDays
 	 */
-	public function testCheckCoreUpdate($channel, $versionCheck, $notification, $readableVersion) {
+	public function testCheckCoreUpdate($channel, $versionCheck, $version, $readableVersion, $errorDays) {
 		$job = $this->getJob([
 			'getChannel',
 			'createVersionCheck',
 			'createNotifications',
+			'clearErrorNotifications',
+			'sendErrorNotifications',
 		]);
 
 		$job->expects($this->once())
@@ -142,9 +153,7 @@ class BackgroundJobTest extends TestCase {
 			$job->expects($this->never())
 				->method('createVersionCheck');
 		} else {
-			$check = $this->getMockBuilder('OC\Updater\VersionCheck')
-				->disableOriginalConstructor()
-				->getMock();
+			$check = $this->createMock(VersionCheck::class);
 			$check->expects($this->once())
 				->method('check')
 				->willReturn($versionCheck);
@@ -154,16 +163,38 @@ class BackgroundJobTest extends TestCase {
 				->willReturn($check);
 		}
 
-		if ($notification === null) {
+		if ($version === null) {
+			$job->expects($this->never())
+				->method('createNotifications');
+			$job->expects($versionCheck === null ? $this->never() : $this->once())
+				->method('clearErrorNotifications');
+		} else if ($version === false) {
 			$job->expects($this->never())
 				->method('createNotifications');
+			$job->expects($this->never())
+				->method('clearErrorNotifications');
+
+			$this->config->expects($this->once())
+				->method('getAppValue')
+				->willReturn($errorDays);
+			$this->config->expects($this->once())
+				->method('setAppValue')
+				->with('updatenotification', 'update_check_errors', $errorDays + 1);
+			$job->expects($errorDays !== null ? $this->once() : $this->never())
+				->method('sendErrorNotifications')
+				->with($errorDays + 1);
 		} else {
+			$this->config->expects($this->once())
+				->method('setAppValue')
+				->with('updatenotification', 'update_check_errors', 0);
+			$job->expects($this->once())
+				->method('clearErrorNotifications');
 			$job->expects($this->once())
 				->method('createNotifications')
-				->willReturn('core', $notification, $readableVersion);
+				->willReturn('core', $version, $readableVersion);
 		}
 
-		$this->invokePrivate($job, 'checkCoreUpdate');
+		self::invokePrivate($job, 'checkCoreUpdate');
 	}
 
 	public function dataCheckAppUpdates() {
@@ -198,15 +229,15 @@ class BackgroundJobTest extends TestCase {
 			->method('getInstalledApps')
 			->willReturn($apps);
 
-		$job->expects($this->exactly(sizeof($apps)))
+		$job->expects($this->exactly(count($apps)))
 			->method('isUpdateAvailable')
 			->willReturnMap($isUpdateAvailable);
 
-		$mockedMethod = $job->expects($this->exactly(sizeof($notifications)))
+		$mockedMethod = $job->expects($this->exactly(count($notifications)))
 			->method('createNotifications');
 		call_user_func_array([$mockedMethod, 'withConsecutive'], $notifications);
 
-		$this->invokePrivate($job, 'checkAppUpdates');
+		self::invokePrivate($job, 'checkAppUpdates');
 	}
 
 	public function dataCreateNotifications() {
@@ -264,7 +295,7 @@ class BackgroundJobTest extends TestCase {
 		}
 
 		if ($createNotification) {
-			$notification = $this->getMockBuilder('OCP\Notification\INotification')->getMock();
+			$notification = $this->createMock(INotification::class);
 			$notification->expects($this->once())
 				->method('setApp')
 				->with('updatenotification')
@@ -282,12 +313,12 @@ class BackgroundJobTest extends TestCase {
 				->willReturnSelf();
 
 			if ($userNotifications !== null) {
-				$mockedMethod = $notification->expects($this->exactly(sizeof($userNotifications)))
+				$mockedMethod = $notification->expects($this->exactly(count($userNotifications)))
 					->method('setUser')
 					->willReturnSelf();
 				call_user_func_array([$mockedMethod, 'withConsecutive'], $userNotifications);
 
-				$this->notificationManager->expects($this->exactly(sizeof($userNotifications)))
+				$this->notificationManager->expects($this->exactly(count($userNotifications)))
 					->method('notify')
 					->willReturn($notification);
 			}
@@ -300,7 +331,7 @@ class BackgroundJobTest extends TestCase {
 				->method('createNotification');
 		}
 
-		$this->invokePrivate($job, 'createNotifications', [$app, $version]);
+		self::invokePrivate($job, 'createNotifications', [$app, $version]);
 	}
 
 	public function dataGetUsersToNotify() {
@@ -336,15 +367,15 @@ class BackgroundJobTest extends TestCase {
 			}
 			$groupMap[] = [$gid, $group];
 		}
-		$this->groupManager->expects($this->exactly(sizeof($groups)))
+		$this->groupManager->expects($this->exactly(count($groups)))
 			->method('get')
 			->willReturnMap($groupMap);
 
-		$result = $this->invokePrivate($job, 'getUsersToNotify');
+		$result = self::invokePrivate($job, 'getUsersToNotify');
 		$this->assertEquals($expected, $result);
 
 		// Test caching
-		$result = $this->invokePrivate($job, 'getUsersToNotify');
+		$result = self::invokePrivate($job, 'getUsersToNotify');
 		$this->assertEquals($expected, $result);
 	}
 
@@ -361,7 +392,7 @@ class BackgroundJobTest extends TestCase {
 	 * @param string $version
 	 */
 	public function testDeleteOutdatedNotifications($app, $version) {
-		$notification = $this->getMockBuilder('OCP\Notification\INotification')->getMock();
+		$notification = $this->createMock(INotification::class);
 		$notification->expects($this->once())
 			->method('setApp')
 			->with('updatenotification')
@@ -379,7 +410,7 @@ class BackgroundJobTest extends TestCase {
 			->with($notification);
 
 		$job = $this->getJob();
-		$this->invokePrivate($job, 'deleteOutdatedNotifications', [$app, $version]);
+		self::invokePrivate($job, 'deleteOutdatedNotifications', [$app, $version]);
 	}
 
 	/**
@@ -389,7 +420,7 @@ class BackgroundJobTest extends TestCase {
 	protected function getUsers(array $userIds) {
 		$users = [];
 		foreach ($userIds as $uid) {
-			$user = $this->getMockBuilder('OCP\IUser')->getMock();
+			$user = $this->createMock(IUser::class);
 			$user->expects($this->any())
 				->method('getUID')
 				->willReturn($uid);
@@ -403,7 +434,7 @@ class BackgroundJobTest extends TestCase {
 	 * @return \OCP\IGroup|\PHPUnit_Framework_MockObject_MockObject
 	 */
 	protected function getGroup($gid) {
-		$group = $this->getMockBuilder('OCP\IGroup')->getMock();
+		$group = $this->createMock(IGroup::class);
 		$group->expects($this->any())
 			->method('getGID')
 			->willReturn($gid);
diff --git a/apps/updatenotification/tests/Notification/NotifierTest.php b/apps/updatenotification/tests/Notification/NotifierTest.php
index e809ce11635..34486bf5ba5 100644
--- a/apps/updatenotification/tests/Notification/NotifierTest.php
+++ b/apps/updatenotification/tests/Notification/NotifierTest.php
@@ -24,6 +24,7 @@ namespace OCA\UpdateNotification\Tests\Notification;
 
 
 use OCA\UpdateNotification\Notification\Notifier;
+use OCP\IConfig;
 use OCP\IGroupManager;
 use OCP\IURLGenerator;
 use OCP\IUserSession;
@@ -36,6 +37,8 @@ class NotifierTest extends TestCase {
 
 	/** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */
 	protected $urlGenerator;
+	/** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */
+	protected $config;
 	/** @var IManager|\PHPUnit_Framework_MockObject_MockObject */
 	protected $notificationManager;
 	/** @var IFactory|\PHPUnit_Framework_MockObject_MockObject */
@@ -49,6 +52,7 @@ class NotifierTest extends TestCase {
 		parent::setUp();
 
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
+		$this->config = $this->createMock(IConfig::class);
 		$this->notificationManager = $this->createMock(IManager::class);
 		$this->l10nFactory = $this->createMock(IFactory::class);
 		$this->userSession = $this->createMock(IUserSession::class);
@@ -63,6 +67,7 @@ class NotifierTest extends TestCase {
 		if (empty($methods)) {
 			return new Notifier(
 				$this->urlGenerator,
+				$this->config,
 				$this->notificationManager,
 				$this->l10nFactory,
 				$this->userSession,
@@ -72,6 +77,7 @@ class NotifierTest extends TestCase {
 			return $this->getMockBuilder(Notifier::class)
 				->setConstructorArgs([
 					$this->urlGenerator,
+					$this->config,
 					$this->notificationManager,
 					$this->l10nFactory,
 					$this->userSession,
diff --git a/lib/private/Updater/VersionCheck.php b/lib/private/Updater/VersionCheck.php
index ae3840a7fa5..3518f40bd4f 100644
--- a/lib/private/Updater/VersionCheck.php
+++ b/lib/private/Updater/VersionCheck.php
@@ -82,7 +82,12 @@ class VersionCheck {
 		$url = $updaterUrl . '?version=' . $versionString;
 
 		$tmp = [];
-		$xml = $this->getUrlContent($url);
+		try {
+			$xml = $this->getUrlContent($url);
+		} catch (\Exception $e) {
+			return false;
+		}
+
 		if ($xml) {
 			$loadEntities = libxml_disable_entity_loader(true);
 			$data = @simplexml_load_string($xml);
@@ -108,16 +113,13 @@ class VersionCheck {
 	/**
 	 * @codeCoverageIgnore
 	 * @param string $url
-	 * @return bool|resource|string
+	 * @return resource|string
+	 * @throws \Exception
 	 */
 	protected function getUrlContent($url) {
-		try {
-			$client = $this->clientService->newClient();
-			$response = $client->get($url);
-			return $response->getBody();
-		} catch (\Exception $e) {
-			return false;
-		}
+		$client = $this->clientService->newClient();
+		$response = $client->get($url);
+		return $response->getBody();
 	}
 }