Merge pull request #18 from nextcloud/feature/noid/stablilization

add unit tests and check failed recipients
2018-06-15 10:57:05 +02:00 · 2018-06-15 10:57:05 +02:00 · 548199a0ff
parent e3df83adf9 3f95d819e9
commit 548199a0ff
7 changed files with 616 additions and 17 deletions
--- a/.drone.yml
+++ b/.drone.yml
@ -15,10 +15,128 @@ pipeline:
      - bash ./before_install.sh $APP_NAME $CORE_BRANCH $DB
      - cd ../server
      - php ./build/signed-off-checker.php
-    secrets: [ github_token ]
    when:
      matrix:
        TESTS: signed-off-check
+  check-app-compatbility:
+    image: nextcloudci/php7.0:php7.0-2
+    environment:
+      - APP_NAME=data_request
+      - CORE_BRANCH=stable13
+      - DB=sqlite
+    commands:
+        # Pre-setup steps
+        - wget https://raw.githubusercontent.com/nextcloud/travis_ci/master/before_install.sh
+        - bash ./before_install.sh $APP_NAME $CORE_BRANCH $DB
+        - cd ../server
+
+        # Code checker
+        - ./occ app:check-code $APP_NAME
+        - cd apps/$APP_NAME/
+    when:
+      matrix:
+        TESTS: check-app-compatbility
+  syntax-php5.6:
+    image: nextcloudci/php5.6:php5.6-8
+    environment:
+      - APP_NAME=data_request
+      - CORE_BRANCH=stable13
+      - DB=sqlite
+    commands:
+      # Pre-setup steps
+      - wget https://raw.githubusercontent.com/nextcloud/travis_ci/master/before_install.sh
+      - bash ./before_install.sh $APP_NAME $CORE_BRANCH $DB
+      - cd ../server
+      - composer install
+      - ./lib/composer/bin/parallel-lint apps/$APP_NAME/
+    when:
+      matrix:
+        TESTS: syntax-php5.6
+  syntax-php7.0:
+    image: nextcloudci/php7.0:php7.0-2
+    environment:
+      - APP_NAME=data_request
+      - CORE_BRANCH=stable13
+      - DB=sqlite
+    commands:
+      # Pre-setup steps
+      - wget https://raw.githubusercontent.com/nextcloud/travis_ci/master/before_install.sh
+      - bash ./before_install.sh $APP_NAME $CORE_BRANCH $DB
+      - cd ../server
+      - composer install
+      - ./lib/composer/bin/parallel-lint apps/$APP_NAME/
+    when:
+      matrix:
+        TESTS: syntax-php7.0
+  php5.6:
+    image: nextcloudci/php5.6:php5.6-8
+    environment:
+      - APP_NAME=data_request
+      - CORE_BRANCH=stable13
+      - DB=sqlite
+    commands:
+      - apt update && apt-get -y install php5-xdebug
+
+      # Pre-setup steps
+      - wget https://raw.githubusercontent.com/nextcloud/travis_ci/master/before_install.sh
+      - bash ./before_install.sh $APP_NAME $CORE_BRANCH $DB
+      - cd ../server/
+      - ./occ app:enable $APP_NAME
+      - cd apps/$APP_NAME
+
+      # Run phpunit tests
+      - cd tests/unit/
+      - phpunit --configuration phpunit.xml
+    when:
+      matrix:
+        TESTS: php5.6
+  php7.0:
+    image: nextcloudci/php7.0:php7.0-2
+    environment:
+      - APP_NAME=data_request
+      - CORE_BRANCH=stable13
+      - DB=sqlite
+    commands:
+      # Pre-setup steps
+      - wget https://raw.githubusercontent.com/nextcloud/travis_ci/master/before_install.sh
+      - bash ./before_install.sh $APP_NAME $CORE_BRANCH $DB
+      - cd ../server/
+      - ./occ app:enable $APP_NAME
+      - cd apps/$APP_NAME
+
+      # Run phpunit tests
+      - cd tests/unit/
+      - phpunit --configuration phpunit.xml
+    when:
+      matrix:
+        TESTS: php7.0
+  php7.1:
+    image: nextcloudci/php7.1:php7.1-15
+    environment:
+      - APP_NAME=data_request
+      - CORE_BRANCH=stable13
+      - DB=sqlite
+    commands:
+      # Pre-setup steps
+      - wget https://raw.githubusercontent.com/nextcloud/travis_ci/master/before_install.sh
+      - bash ./before_install.sh $APP_NAME $CORE_BRANCH $DB
+      - cd ../server/
+      - ./occ app:enable $APP_NAME
+      - cd apps/$APP_NAME
+
+      # Run phpunit tests
+      - cd tests/unit/
+      - phpunit --configuration phpunit.xml
+    when:
+      matrix:
+        TESTS: php7.1
+
 matrix:
  include:
    - TESTS: signed-off-check
+    - TESTS: check-app-compatbility
+    - TESTS: syntax-php5.6
+    - TESTS: syntax-php7.0
+    - TESTS: php5.6
+    - TESTS: php7.0
+    - TESTS: php7.1
--- a/lib/Controller/DataRequestController.php
+++ b/lib/Controller/DataRequestController.php
@ -52,15 +52,9 @@ class DataRequestController extends OCSController {
 	 * @PasswordConfirmationRequired
 	 */
 	public function export() {
-		try {
+		return $this->processRequest(function() {
 			$this->dataRequest->sendExportRequest();
-			return new DataResponse();
-		} catch(HintedRuntime $e) {
-			return new DataResponse(
-				['error' => $e->getHint()],
-				Http::STATUS_INTERNAL_SERVER_ERROR
-			);
-		}
+		});
 	}

 	/**
@ -68,8 +62,14 @@ class DataRequestController extends OCSController {
 	 * @PasswordConfirmationRequired
 	 */
 	public function deletion() {
-		try {
+		return $this->processRequest(function() {
 			$this->dataRequest->sendDeleteRequest();
+		});
+	}
+
+	protected function processRequest(callable $serviceMethod) {
+		try {
+			$serviceMethod();
 			return new DataResponse();
 		} catch(HintedRuntime $e) {
 			return new DataResponse(
--- a/lib/Services/Request.php
+++ b/lib/Services/Request.php
@ -50,14 +50,25 @@ class Request {
 	private $requester;
 	/** @var IL10N */
 	private $l;
+	/** @var Defaults */
+	private $defaults;

-	public function __construct(IGroupManager $groupManager, IMailer $mailer, IFactory $l10nFactory, IConfig $config, IUserSession $userSession, IL10N $l) {
+	public function __construct(
+		IGroupManager $groupManager,
+		IMailer $mailer,
+		IFactory $l10nFactory,
+		IConfig $config,
+		IUserSession $userSession,
+		IL10N $l,
+		Defaults $defaults
+	) {
 		$this->groupManager = $groupManager;
 		$this->mailer = $mailer;
 		$this->l10nFactory = $l10nFactory;
 		$this->config = $config;
 		$this->requester = $userSession->getUser();
 		$this->l = $l;
+		$this->defaults = $defaults;
 	}

 	public function sendExportRequest() {
@ -95,21 +106,23 @@ class Request {
 	}

 	protected function craftEmailTo(IUser $admin, IEMailTemplate $template) {
-		$defaults = new Defaults();
 		$senderAddress = Util::getDefaultEmailAddress('no-reply');
-		$senderName = $defaults->getName();
+		$senderName = $this->defaults->getName();

 		$message = $this->mailer->createMessage();
-		$message->setTo([$admin->getEMailAddress() => $admin->getDisplayName()]);
+		$message->setTo([$admin->getEMailAddress () => $admin->getDisplayName()]);
 		$message->setSubject($template->renderSubject());
 		$message->setHtmlBody($template->renderHtml());
 		$message->setPlainBody($template->renderText());
 		$message->setFrom([$senderAddress => $senderName]);

 		try {
-			$this->mailer->send($message);
+			$failedRecipients = $this->mailer->send($message);
+			if(count($failedRecipients) > 0) {
+				return false;
+			}
 		} catch (\Exception $e) {
-			return $e;
+			return false;
 		}

 		return true;
@ -147,7 +160,7 @@ class Request {

 	protected function getAdmins() {
 		$admins = $this->groupManager->get('admin')->searchUsers('');
-		$admins =  array_filter($admins, function(IUser $admin) {
+		$admins = array_filter($admins, function(IUser $admin) {
 			return $admin->getEMailAddress() !== null;
 		});
 		if(empty($admins)) {
--- a/tests/unit/Controller/DataRequestControllerTest.php
+++ b/tests/unit/Controller/DataRequestControllerTest.php
@ -0,0 +1,120 @@
+<?php
+/**
+ * @copyright Copyright (c) 2018 Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @author Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OCA\DataRequest\Tests\unit\Controller;
+
+use OCA\DataRequest\Controller\DataRequestController;
+use OCA\DataRequest\Exceptions\HintedRuntime;
+use OCA\DataRequest\Services\Request;
+use OCP\AppFramework\Http\DataResponse;
+use OCP\IRequest;
+
+class DataRequestControllerTest extends \Test\TestCase {
+
+	/** @var Request|\PHPUnit_Framework_MockObject_MockObject */
+	protected $requestService;
+
+	/** @var DataRequestController */
+	protected $controller;
+
+	public function setUp() {
+		parent::setUp();
+
+		$this->requestService = $this->createMock(Request::class);
+
+		/** @var IRequest $request */
+		$request = $this->createMock(IRequest::class);
+
+		$this->controller = new DataRequestController(
+			'data_request',
+			$request,
+			'PUT, POST, GET, DELETE, PATCH',
+			'Authorization, Content-Type, Accept',
+			1728000,
+			$this->requestService
+		);
+	}
+
+	public function processDataProvider() {
+		return [
+			[
+				'sendExportRequest',
+				true,
+				500
+			],
+			[
+				'sendExportRequest',
+				false,
+				200
+			],
+			[
+				'sendDeleteRequest',
+				true,
+				500
+			],
+			[
+				'sendDeleteRequest',
+				false,
+				200
+			]
+		];
+	}
+
+	/**
+	 * @dataProvider processDataProvider
+	 * @param string $serviceMethod
+	 * @param bool $causeException
+	 * @param int $expectStatus
+	 */
+	public function testRequests($serviceMethod, $causeException, $expectStatus) {
+		$mocker = $this->requestService->expects($this->once())
+			->method($serviceMethod);
+
+		if($causeException) {
+			/** @var HintedRuntime|\PHPUnit_Framework_MockObject_MockObject $exception */
+			$exception = $this->createMock(HintedRuntime::class);
+			$exception->expects($this->once())
+				->method('getHint')
+				->willReturn('Some hint');
+
+			$mocker->willThrowException($exception);
+		}
+
+		switch($serviceMethod) {
+			case 'sendExportRequest':
+				$response = $this->controller->export();
+				break;
+			case 'sendDeleteRequest':
+				$response = $this->controller->deletion();
+				break;
+			default:
+				throw new \RuntimeException('No valid test case');
+		}
+
+		$this->assertInstanceOf(DataResponse::class, $response);
+		$this->assertSame($expectStatus, $response->getStatus());
+		if($expectStatus >= 500) {
+			$this->assertSame('Some hint', $response->getData()['error']);
+		}
+	}
+}
--- a/tests/unit/Services/RequestTest.php
+++ b/tests/unit/Services/RequestTest.php
@ -0,0 +1,297 @@
+<?php
+
+use OCA\DataRequest\Exceptions\HintedRuntime;
+use OCA\DataRequest\Services\Request;
+use OCP\Defaults;
+use OCP\IConfig;
+use OCP\IGroup;
+use OCP\IGroupManager;
+use OCP\IL10N;
+use OCP\IUser;
+use OCP\IUserSession;
+use OCP\L10N\IFactory;
+use OCP\Mail\IEMailTemplate;
+use OCP\Mail\IMailer;
+
+/**
+ * @copyright Copyright (c) 2018 Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @author Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+class RequestTest extends \Test\TestCase {
+	/** @var IGroupManager|PHPUnit_Framework_MockObject_MockObject */
+	protected $groupManager;
+	/** @var IMailer|PHPUnit_Framework_MockObject_MockObject */
+	protected $mailer;
+	/** @var IFactory|PHPUnit_Framework_MockObject_MockObject */
+	protected $l10nFactory;
+	/** @var IConfig|PHPUnit_Framework_MockObject_MockObject */
+	protected $config;
+	/** @var IUserSession|PHPUnit_Framework_MockObject_MockObject */
+	protected $session;
+	/** @var IL10N|PHPUnit_Framework_MockObject_MockObject */
+	protected $l;
+	/** @var Request */
+	protected $service;
+	/** @var IUser|PHPUnit_Framework_MockObject_MockObject */
+	protected $user;
+	/** @var Defaults|PHPUnit_Framework_MockObject_MockObject */
+	protected $defaults;
+
+	public function setUp() {
+		parent::setUp();
+
+		$this->groupManager = $this->createMock(IGroupManager::class);
+		$this->mailer = $this->createMock(IMailer::class);
+		$this->l10nFactory = $this->createMock(IFactory::class);
+		$this->config = $this->createMock(IConfig::class);
+
+		$this->user = $this->createMock(IUser::class);
+
+		$this->session = $this->createMock(IUserSession::class);
+		$this->session->expects($this->any())
+			->method('getUser')
+			->willReturn($this->user);
+
+		$this->l = $this->createMock(IL10N::class);
+
+		$this->defaults = $this->createMock(Defaults::class);
+
+		$this->service = new Request(
+			$this->groupManager,
+			$this->mailer,
+			$this->l10nFactory,
+			$this->config,
+			$this->session,
+			$this->l,
+			$this->defaults
+		);
+	}
+
+	public function templateProvider() {
+		return [
+			[
+				'getExportTemplate',
+				'data_request.Export',
+				'Personal data export request'
+			],
+			[
+				'getDeletionTemplate',
+				'data_request.Deletion',
+				'Account deletion request'
+			]
+		];
+	}
+
+	/**
+	 * @dataProvider templateProvider
+	 * @param $method
+	 * @param $templateId
+	 * @param $expectedSubject
+	 */
+	public function testTemplates($method, $templateId, $expectedSubject) {
+		$adminUid = 'elu-thingol';
+		$adminName = 'Elu Thingol';
+		$adminLang = 'qya';
+		$admin = $this->createMock(IUser::class);
+		$admin->expects($this->any())
+			->method('getUID')
+			->willReturn($adminUid);
+		$admin->expects($this->any())
+			->method('getDisplayName')
+			->willReturn($adminName);
+
+		$this->config->expects($this->atLeastOnce())
+			->method('getSystemValue')
+			->with('default_language')
+			->willReturn('tlh');
+		$this->config->expects($this->atLeastOnce())
+			->method('getUserValue')
+			->with($adminUid, 'core', 'lang', 'tlh')
+			->willReturn($adminLang);
+
+		$l = $this->createMock(IL10N::class);
+		$l->expects($this->atLeast(3))
+			->method('t')
+			->willReturnArgument(0);
+
+		$this->l10nFactory->expects($this->once())
+			->method('get')
+			->with('data_request', $adminLang)
+			->willReturn($l);
+
+		$template = $this->createMock(IEMailTemplate::class);
+		$template->expects($this->once())
+			->method('setSubject')
+			->with($expectedSubject);
+		$template->expects($this->once())
+			->method('addHeader');
+		$template->expects($this->once())
+			->method('addHeading');
+		$template->expects($this->once())
+			->method('addBodyText');
+		$template->expects($this->once())
+			->method('addFooter');
+
+		$this->mailer->expects($this->once())
+			->method('createEMailTemplate')
+			->with($templateId, [])
+			->willReturn($template);
+
+		$result = $this->invokePrivate($this->service, $method, [$admin]);
+
+		$this->assertSame($template, $result);
+	}
+
+
+	public function adminProvider() {
+		$admin1 = $this->createMock(IUser::class);
+
+		$admin2 = $this->createMock(IUser::class);
+		$admin2->expects($this->any())
+			->method('getEMailAddress')
+			->willReturn('admin2@sindar.gov');
+
+		$admin3 = $this->createMock(IUser::class);
+		$admin3->expects($this->any())
+			->method('getEMailAddress')
+			->willReturn('admin3@sindar.gov');
+
+		$admin4 = $this->createMock(IUser::class);
+
+		$admin5 = $this->createMock(IUser::class);
+		$admin5->expects($this->any())
+			->method('getEMailAddress')
+			->willReturn('admin5@sindar.gov');
+
+		return [
+			[
+				[ $admin1 ],
+				0
+			],
+			[
+				[ $admin2 ],
+				1
+			],
+			[
+				[ $admin3, $admin4, $admin5 ], // for whatever reasons, reusing $admin1 and $admin2 would fail on CI
+				2
+			]
+		];
+	}
+
+	/**
+	 * @dataProvider adminProvider
+	 * @param $admins
+	 * @param $adminsWithEmail
+	 */
+	public function testGetAdmins($admins, $adminsWithEmail) {
+		$adminGroup = $this->createMock(IGroup::class);
+		$adminGroup->expects($this->once())
+			->method('searchUsers')
+			->with('')
+			->willReturn($admins);
+
+		$this->groupManager->expects($this->once())
+			->method('get')
+			->with('admin')
+			->willReturn($adminGroup);
+
+		if($adminsWithEmail === 0) {
+			$this->expectException(HintedRuntime::class);
+		}
+		$result = $this->invokePrivate($this->service, 'getAdmins');
+
+		$this->assertSame($adminsWithEmail, count($result));
+	}
+
+	public function mailerSendProvider() {
+		return [
+			[
+				false, [], true
+			],
+			[
+				false, ['elu-thingol@sindar.gov'], false
+			],
+			[
+				true, [], false
+			]
+		];
+	}
+
+	/**
+	 * @dataProvider mailerSendProvider
+	 * @param $sendThrowsException
+	 * @param $sendResult
+	 * @param $expectedResult
+	 */
+	public function testSendMail($sendThrowsException, $sendResult, $expectedResult) {
+		$adminName = 'Elu Thingol';
+		$adminMail = 'elu-thingol@sindar.gov';
+		$admin = $this->createMock(IUser::class);
+		$admin->expects($this->any())
+			->method('getEMailAddress')
+			->willReturn($adminMail);
+		$admin->expects($this->any())
+			->method('getDisplayName')
+			->willReturn($adminName);
+
+		$template = $this->createMock(IEMailTemplate::class);
+		$template->expects($this->once())
+			->method('renderSubject');
+		$template->expects($this->once())
+			->method('renderHtml');
+		$template->expects($this->once())
+			->method('renderText');
+
+		$message = $this->createMock(\OC\Mail\Message::class);
+		$message->expects($this->once())
+			->method('setTo')
+			->with([$adminMail => $adminName]);
+		$message->expects($this->once())
+			->method('setSubject');
+		$message->expects($this->once())
+			->method('setHtmlBody');
+		$message->expects($this->once())
+			->method('setPlainBody');
+		$message->expects($this->once())
+			->method('setFrom');
+
+		$this->mailer->expects($this->once())
+			->method('createMessage')
+			->willReturn($message);
+
+		$sendMocker = $this->mailer->expects($this->once())
+			->method('send')
+			->with($message);
+		if($sendThrowsException) {
+			$sendMocker->willThrowException(new \Exception('Expected Exception'));
+		} else {
+			$sendMocker->willReturn($sendResult);
+		}
+
+		$this->defaults->expects($this->atLeastOnce())
+			->method('getName')
+			->willReturn('Cloud of Sindar');
+
+		$result = $this->invokePrivate($this->service, 'craftEmailTo', [$admin, $template]);
+		$this->assertSame($expectedResult, $result);
+	}
+}
--- a/tests/unit/bootstrap.php
+++ b/tests/unit/bootstrap.php
@ -0,0 +1,28 @@
+<?php
+/**
+ * @copyright Copyright (c) 2016 Lukas Reschke <lukas@statuscode.ch>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+if (!defined('PHPUNIT_RUN')) {
+	define('PHPUNIT_RUN', 1);
+}
+require_once __DIR__.'/../../../../lib/base.php';
+\OC::$loader->addValidRoot(\OC::$SERVERROOT . '/tests');
+\OC_App::loadApp('data_request');
+OC_Hook::clear();
--- a/tests/unit/phpunit.xml
+++ b/tests/unit/phpunit.xml
@ -0,0 +1,23 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<phpunit bootstrap="bootstrap.php"
+		 strict="true"
+		 verbose="true"
+		 timeoutForSmallTests="900"
+		 timeoutForMediumTests="900"
+		 timeoutForLargeTests="900"
+>
+	<testsuite name='Data Request App Tests'>
+		<directory suffix='Test.php'>.</directory>
+	</testsuite>
+	<!-- filters for code coverage -->
+	<filter>
+		<whitelist>
+			<directory suffix=".php">../../../data_request/appinfo</directory>
+			<directory suffix=".php">../../../data_request/lib</directory>
+		</whitelist>
+	</filter>
+	<logging>
+		<!-- and this is where your report will be written -->
+		<log type="coverage-clover" target="./clover.xml"/>
+	</logging>
+</phpunit>