From 3668673d7b3f64a5aaa80f569856b88460d9a522 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Mar 2017 12:47:26 +0100 Subject: [PATCH 1/3] Create a notification when the update server couldn't be reached for some days Signed-off-by: Joas Schilling --- .../lib/Notification/BackgroundJob.php | 54 ++++++++- .../lib/Notification/Notifier.php | 19 +++- .../tests/Notification/BackgroundJobTest.php | 107 +++++++++++------- .../tests/Notification/NotifierTest.php | 6 + lib/private/Updater/VersionCheck.php | 20 ++-- 5 files changed, 155 insertions(+), 51 deletions(-) diff --git a/apps/updatenotification/lib/Notification/BackgroundJob.php b/apps/updatenotification/lib/Notification/BackgroundJob.php index 83a9bdb599..c3dd748116 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,11 +91,59 @@ class BackgroundJob extends TimedJob { $updater = $this->createVersionCheck(); $status = $updater->check(); - if (isset($status['version'])) { + 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 (isset($status['version'])) { + $this->config->setAppValue('updatenotification', 'update_check_errors', 0); + $this->clearErrorNotifications(); + $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 */ diff --git a/apps/updatenotification/lib/Notification/Notifier.php b/apps/updatenotification/lib/Notification/Notifier.php index 079ec4c5e0..bef57b0d86 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 57771ec0ae..468f4cf959 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($this->never()) + ->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 e809ce1163..34486bf5ba 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 ae3840a7fa..3518f40bd4 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(); } } From 2249c77a06d7238a8f4234f636f4cdddd07d56a4 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Fri, 17 Mar 2017 14:40:39 -0600 Subject: [PATCH 2/3] Also remove failure notification if no update is available * error happened and vanished the notification should also vanish even if no update is available Signed-off-by: Morris Jobke --- apps/updatenotification/lib/Notification/BackgroundJob.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/apps/updatenotification/lib/Notification/BackgroundJob.php b/apps/updatenotification/lib/Notification/BackgroundJob.php index c3dd748116..9155b9d255 100644 --- a/apps/updatenotification/lib/Notification/BackgroundJob.php +++ b/apps/updatenotification/lib/Notification/BackgroundJob.php @@ -98,11 +98,13 @@ class BackgroundJob extends TimedJob { if (in_array($errors, $this->connectionNotifications, true)) { $this->sendErrorNotifications($errors); } - } else if (isset($status['version'])) { + } else if (is_array($status)) { $this->config->setAppValue('updatenotification', 'update_check_errors', 0); $this->clearErrorNotifications(); - $this->createNotifications('core', $status['version'], $status['versionstring']); + if (isset($status['version'])) { + $this->createNotifications('core', $status['version'], $status['versionstring']); + } } } From 27e3a977aa47d7d78ad94eaf10aae98398fa26b6 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 20 Mar 2017 12:31:15 +0100 Subject: [PATCH 3/3] Fix unit test Signed-off-by: Joas Schilling --- .../updatenotification/tests/Notification/BackgroundJobTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/updatenotification/tests/Notification/BackgroundJobTest.php b/apps/updatenotification/tests/Notification/BackgroundJobTest.php index 468f4cf959..92a8a687f5 100644 --- a/apps/updatenotification/tests/Notification/BackgroundJobTest.php +++ b/apps/updatenotification/tests/Notification/BackgroundJobTest.php @@ -166,7 +166,7 @@ class BackgroundJobTest extends TestCase { if ($version === null) { $job->expects($this->never()) ->method('createNotifications'); - $job->expects($this->never()) + $job->expects($versionCheck === null ? $this->never() : $this->once()) ->method('clearErrorNotifications'); } else if ($version === false) { $job->expects($this->never())