Sharee API to AppFramework

* Move to OCSController
* Move to Controller folder
* Use automatic DI
* Use function parameters
* Updated tests

3 files changed, 79 insertions, 93 deletions

diff --git a/apps/files_sharing/appinfo/routes.php b/apps/files_sharing/appinfo/routes.php
index ee7ea55d506..de6967c7d63 100644
--- a/apps/files_sharing/appinfo/routes.php
+++ b/apps/files_sharing/appinfo/routes.php
@@ -69,6 +69,14 @@ $application->registerRoutes($this, [
 			'url'  => '/api/v1/shares/{id}',
 			'verb' => 'DELETE',
 		],
+		/*
+		 * OCS Sharee API
+		 */
+		[
+			'name' => 'ShareesAPI#search',
+			'url' => '/api/v1/sharees',
+			'verb' => 'GET',
+		],
 	],
 ]);
 
@@ -118,20 +126,3 @@ API::register('delete',
 		'/apps/files_sharing/api/v1/remote_shares/{id}',
 		array('\OCA\Files_Sharing\API\Remote', 'unshare'),
 		'files_sharing');
-
-
-$sharees = new \OCA\Files_Sharing\API\Sharees(\OC::$server->getGroupManager(),
-                                              \OC::$server->getUserManager(),
-                                              \OC::$server->getContactsManager(),
-                                              \OC::$server->getConfig(),
-                                              \OC::$server->getUserSession(),
-                                              \OC::$server->getURLGenerator(),
-                                              \OC::$server->getRequest(),
-                                              \OC::$server->getLogger(),
-                                              \OC::$server->getShareManager());
-
-API::register('get',
-		'/apps/files_sharing/api/v1/sharees',
-		[$sharees, 'search'],
-		'files_sharing', API::USER_AUTH);
-
diff --git a/apps/files_sharing/lib/API/Sharees.php b/apps/files_sharing/lib/Controller/ShareesAPIController.php
index a7eb13708f3..b884aa9f1d4 100644
--- a/apps/files_sharing/lib/API/Sharees.php
+++ b/apps/files_sharing/lib/Controller/ShareesAPIController.php
@@ -22,9 +22,11 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>
  *
  */
-namespace OCA\Files_Sharing\API;
+namespace OCA\Files_Sharing\Controller;
 
 use OCP\AppFramework\Http;
+use OCP\AppFramework\OCS\OCSBadRequestException;
+use OCP\AppFramework\OCSController;
 use OCP\Contacts\IManager;
 use OCP\IGroup;
 use OCP\IGroupManager;
@@ -37,7 +39,7 @@ use OCP\IUserSession;
 use OCP\IURLGenerator;
 use OCP\Share;
 
-class Sharees {
+class ShareesAPIController extends OCSController {
 
 	/** @var IGroupManager */
 	protected $groupManager;
@@ -54,9 +56,6 @@ class Sharees {
 	/** @var IUserSession */
 	protected $userSession;
 
-	/** @var IRequest */
-	protected $request;
-
 	/** @var IURLGenerator */
 	protected $urlGenerator;
 
@@ -93,32 +92,35 @@ class Sharees {
 	protected $reachedEndFor = [];
 
 	/**
+	 * @param string $appName
+	 * @param IRequest $request
 	 * @param IGroupManager $groupManager
 	 * @param IUserManager $userManager
 	 * @param IManager $contactsManager
 	 * @param IConfig $config
 	 * @param IUserSession $userSession
 	 * @param IURLGenerator $urlGenerator
-	 * @param IRequest $request
 	 * @param ILogger $logger
 	 * @param \OCP\Share\IManager $shareManager
 	 */
-	public function __construct(IGroupManager $groupManager,
+	public function __construct($appName,
+								IRequest $request,
+								IGroupManager $groupManager,
 								IUserManager $userManager,
 								IManager $contactsManager,
 								IConfig $config,
 								IUserSession $userSession,
 								IURLGenerator $urlGenerator,
-								IRequest $request,
 								ILogger $logger,
 								\OCP\Share\IManager $shareManager) {
+		parent::__construct($appName, $request);
+
 		$this->groupManager = $groupManager;
 		$this->userManager = $userManager;
 		$this->contactsManager = $contactsManager;
 		$this->config = $config;
 		$this->userSession = $userSession;
 		$this->urlGenerator = $urlGenerator;
-		$this->request = $request;
 		$this->logger = $logger;
 		$this->shareManager = $shareManager;
 	}
@@ -401,19 +403,22 @@ class Sharees {
 	}
 
 	/**
-	 * @return \OC_OCS_Result
+	 * @NoAdminRequired
+	 *
+	 * @param string $search
+	 * @param string $itemType
+	 * @param int $page
+	 * @param int $perPage
+	 * @param int|int[] $shareType
+	 * @return Http\DataResponse
+	 * @throws OCSBadRequestException
 	 */
-	public function search() {
-		$search = isset($_GET['search']) ? (string) $_GET['search'] : '';
-		$itemType = isset($_GET['itemType']) ? (string) $_GET['itemType'] : null;
-		$page = isset($_GET['page']) ? (int) $_GET['page'] : 1;
-		$perPage = isset($_GET['perPage']) ? (int) $_GET['perPage'] : 200;
-
+	public function search($search = '', $itemType = null, $page = 1, $perPage = 200, $shareType = null) {
 		if ($perPage <= 0) {
-			return new \OC_OCS_Result(null, Http::STATUS_BAD_REQUEST, 'Invalid perPage argument');
+			throw new OCSBadRequestException('Invalid perPage argument');
 		}
 		if ($page <= 0) {
-			return new \OC_OCS_Result(null, Http::STATUS_BAD_REQUEST, 'Invalid page');
+			throw new OCSBadRequestException('Invalid page');
 		}
 
 		$shareTypes = [
@@ -426,12 +431,11 @@ class Sharees {
 
 		$shareTypes[] = Share::SHARE_TYPE_REMOTE;
 
-		if (isset($_GET['shareType']) && is_array($_GET['shareType'])) {
-			$shareTypes = array_intersect($shareTypes, $_GET['shareType']);
+		if (is_array($shareType)) {
+			$shareTypes = array_intersect($shareTypes, $shareType);
 			sort($shareTypes);
-
-		} else if (isset($_GET['shareType']) && is_numeric($_GET['shareType'])) {
-			$shareTypes = array_intersect($shareTypes, [(int) $_GET['shareType']]);
+		} else if (is_numeric($shareType)) {
+			$shareTypes = array_intersect($shareTypes, [(int) $shareType]);
 			sort($shareTypes);
 		}
 
@@ -471,12 +475,13 @@ class Sharees {
 	 * @param array $shareTypes
 	 * @param int $page
 	 * @param int $perPage
-	 * @return \OC_OCS_Result
+	 * @return Http\DataResponse
+	 * @throws OCSBadRequestException
 	 */
 	protected function searchSharees($search, $itemType, array $shareTypes, $page, $perPage) {
 		// Verify arguments
 		if ($itemType === null) {
-			return new \OC_OCS_Result(null, Http::STATUS_BAD_REQUEST, 'Missing itemType');
+			throw new OCSBadRequestException('Missing itemType');
 		}
 
 		// Get users
@@ -494,8 +499,7 @@ class Sharees {
 			$this->getRemote($search);
 		}
 
-		$response = new \OC_OCS_Result($this->result);
-		$response->setItemsPerPage($perPage);
+		$response = new Http\DataResponse($this->result);
 
 		if (sizeof($this->reachedEndFor) < 3) {
 			$response->addHeader('Link', $this->getPaginationLink($page, [
diff --git a/apps/files_sharing/tests/API/ShareesTest.php b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php
index 041b45216af..5cb073ecf08 100644
--- a/apps/files_sharing/tests/API/ShareesTest.php
+++ b/apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php
@@ -23,11 +23,12 @@
  *
  */
 
-namespace OCA\Files_Sharing\Tests\API;
+namespace OCA\Files_Sharing\Tests\Controller;
 
-use OCA\Files_Sharing\API\Sharees;
+use OCA\Files_Sharing\Controller\ShareesAPIController;
 use OCA\Files_Sharing\Tests\TestCase;
 use OCP\AppFramework\Http;
+use OCP\AppFramework\OCS\OCSBadRequestException;
 use OCP\Share;
 
 /**
@@ -37,7 +38,7 @@ use OCP\Share;
  *
  * @package OCA\Files_Sharing\Tests\API
  */
-class ShareesTest extends TestCase {
+class ShareesAPIControllerTest extends TestCase {
 	/** @var Sharees */
 	protected $sharees;
 
@@ -86,14 +87,15 @@ class ShareesTest extends TestCase {
 			->disableOriginalConstructor()
 			->getMock();
 
-		$this->sharees = new Sharees(
+		$this->sharees = new ShareesAPIController(
+			'files_sharing',
+			$this->request,
 			$this->groupManager,
 			$this->userManager,
 			$this->contactsManager,
 			$this->getMockBuilder('OCP\IConfig')->disableOriginalConstructor()->getMock(),
 			$this->session,
 			$this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(),
-			$this->request,
 			$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(),
 			$this->shareManager
 		);
@@ -1084,8 +1086,11 @@ class ShareesTest extends TestCase {
 	 * @param bool $allowGroupSharing
 	 */
 	public function testSearch($getData, $apiSetting, $enumSetting, $remoteSharingEnabled, $search, $itemType, $shareTypes, $page, $perPage, $shareWithGroupOnly, $shareeEnumeration, $allowGroupSharing) {
-		$oldGet = $_GET;
-		$_GET = $getData;
+		$search = isset($getData['search']) ? $getData['search'] : '';
+		$itemType = isset($getData['itemType']) ? $getData['itemType'] : null;
+		$page = isset($getData['page']) ? $getData['page'] : 1;
+		$perPage = isset($getData['perPage']) ? $getData['perPage'] : 200;
+		$shareType = isset($getData['shareType']) ? $getData['shareType'] : null;
 
 		$config = $this->getMockBuilder('OCP\IConfig')
 			->disableOriginalConstructor()
@@ -1102,15 +1107,17 @@ class ShareesTest extends TestCase {
 			->method('allowGroupSharing')
 			->willReturn($allowGroupSharing);
 
-		$sharees = $this->getMockBuilder('\OCA\Files_Sharing\API\Sharees')
+		/** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\Controller\ShareesAPIController $sharees */
+		$sharees = $this->getMockBuilder('\OCA\Files_Sharing\Controller\ShareesAPIController')
 			->setConstructorArgs([
+				'files_sharing',
+				$this->getMockBuilder('OCP\IRequest')->disableOriginalConstructor()->getMock(),
 				$this->groupManager,
 				$this->userManager,
 				$this->contactsManager,
 				$config,
 				$this->session,
 				$this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(),
-				$this->getMockBuilder('OCP\IRequest')->disableOriginalConstructor()->getMock(),
 				$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(),
 				$this->shareManager
 			])
@@ -1129,20 +1136,17 @@ class ShareesTest extends TestCase {
 				$this->assertSame($shareTypes, $ishareTypes);
 				$this->assertSame($page, $ipage);
 				$this->assertSame($perPage, $iperPage);
-				return new \OC_OCS_Result([]);
+				return new Http\DataResponse();
 			});
 		$sharees->expects($this->any())
 			->method('isRemoteSharingAllowed')
 			->with($itemType)
 			->willReturn($remoteSharingEnabled);
 
-		/** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\API\Sharees $sharees */
-		$this->assertInstanceOf('\OC_OCS_Result', $sharees->search());
+		$this->assertInstanceOf('\OCP\AppFramework\Http\DataResponse', $sharees->search($search, $itemType, $page, $perPage, $shareType));
 
 		$this->assertSame($shareWithGroupOnly, $this->invokePrivate($sharees, 'shareWithGroupOnly'));
 		$this->assertSame($shareeEnumeration, $this->invokePrivate($sharees, 'shareeEnumeration'));
-
-		$_GET = $oldGet;
 	}
 
 	public function dataSearchInvalid() {
@@ -1178,8 +1182,8 @@ class ShareesTest extends TestCase {
 	 * @param string $message
 	 */
 	public function testSearchInvalid($getData, $message) {
-		$oldGet = $_GET;
-		$_GET = $getData;
+		$page = isset($getData['page']) ? $getData['page'] : 1;
+		$perPage = isset($getData['perPage']) ? $getData['perPage'] : 200;
 
 		$config = $this->getMockBuilder('OCP\IConfig')
 			->disableOriginalConstructor()
@@ -1187,15 +1191,17 @@ class ShareesTest extends TestCase {
 		$config->expects($this->never())
 			->method('getAppValue');
 
-		$sharees = $this->getMockBuilder('\OCA\Files_Sharing\API\Sharees')
+		/** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\Controller\ShareesAPIController $sharees */
+		$sharees = $this->getMockBuilder('\OCA\Files_Sharing\Controller\ShareesAPIController')
 			->setConstructorArgs([
+				'files_sharing',
+				$this->getMockBuilder('OCP\IRequest')->disableOriginalConstructor()->getMock(),
 				$this->groupManager,
 				$this->userManager,
 				$this->contactsManager,
 				$config,
 				$this->session,
 				$this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(),
-				$this->getMockBuilder('OCP\IRequest')->disableOriginalConstructor()->getMock(),
 				$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(),
 				$this->shareManager
 			])
@@ -1206,13 +1212,12 @@ class ShareesTest extends TestCase {
 		$sharees->expects($this->never())
 			->method('isRemoteSharingAllowed');
 
-		/** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\API\Sharees $sharees */
-		$ocs = $sharees->search();
-		$this->assertInstanceOf('\OC_OCS_Result', $ocs);
-
-		$this->assertOCSError($ocs, $message);
-
-		$_GET = $oldGet;
+		try {
+			$sharees->search('', null, $page, $perPage, null);
+			$this->fail();
+		} catch (OCSBadRequestException $e) {
+			$this->assertEquals($message, $e->getMessage());
+		}
 	}
 
 	public function dataIsRemoteSharingAllowed() {
@@ -1339,16 +1344,17 @@ class ShareesTest extends TestCase {
 	 */
 	public function testSearchSharees($searchTerm, $itemType, array $shareTypes, $page, $perPage, $shareWithGroupOnly,
 									  $mockedUserResult, $mockedGroupsResult, $mockedRemotesResult, $expected, $nextLink) {
-		/** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\API\Sharees $sharees */
-		$sharees = $this->getMockBuilder('\OCA\Files_Sharing\API\Sharees')
+		/** @var \PHPUnit_Framework_MockObject_MockObject|\OCA\Files_Sharing\Controller\ShareesAPIController $sharees */
+		$sharees = $this->getMockBuilder('\OCA\Files_Sharing\Controller\ShareesAPIController')
 			->setConstructorArgs([
+				'files_sharing',
+				$this->getMockBuilder('OCP\IRequest')->disableOriginalConstructor()->getMock(),
 				$this->groupManager,
 				$this->userManager,
 				$this->contactsManager,
 				$this->getMockBuilder('OCP\IConfig')->disableOriginalConstructor()->getMock(),
 				$this->session,
 				$this->getMockBuilder('OCP\IURLGenerator')->disableOriginalConstructor()->getMock(),
-				$this->getMockBuilder('OCP\IRequest')->disableOriginalConstructor()->getMock(),
 				$this->getMockBuilder('OCP\ILogger')->disableOriginalConstructor()->getMock(),
 				$this->shareManager
 			])
@@ -1379,9 +1385,8 @@ class ShareesTest extends TestCase {
 				$this->invokePrivate($sharees, 'result', [$result]);
 			});
 
-		/** @var \OC_OCS_Result $ocs */
 		$ocs = $this->invokePrivate($sharees, 'searchSharees', [$searchTerm, $itemType, $shareTypes, $page, $perPage, $shareWithGroupOnly]);
-		$this->assertInstanceOf('\OC_OCS_Result', $ocs);
+		$this->assertInstanceOf('\OCP\AppFramework\Http\DataResponse', $ocs);
 		$this->assertEquals($expected, $ocs->getData());
 
 		// Check if next link is set
@@ -1393,12 +1398,12 @@ class ShareesTest extends TestCase {
 		}
 	}
 
+	/**
+	 * @expectedException \OCP\AppFramework\OCS\OCSBadRequestException
+	 * @expectedExceptionMessage Missing itemType
+	 */
 	public function testSearchShareesNoItemType() {
-		/** @var \OC_OCS_Result $ocs */
-		$ocs = $this->invokePrivate($this->sharees, 'searchSharees', ['', null, [], [], 0, 0, false]);
-		$this->assertInstanceOf('\OC_OCS_Result', $ocs);
-
-		$this->assertOCSError($ocs, 'Missing itemType');
+		$this->invokePrivate($this->sharees, 'searchSharees', ['', null, [], [], 0, 0, false]);
 	}
 
 	public function dataGetPaginationLink() {
@@ -1446,20 +1451,6 @@ class ShareesTest extends TestCase {
 	}
 
 	/**
-	 * @param \OC_OCS_Result $ocs
-	 * @param string $message
-	 */
-	protected function assertOCSError(\OC_OCS_Result $ocs, $message) {
-		$this->assertSame(Http::STATUS_BAD_REQUEST, $ocs->getStatusCode(), 'Expected status code 400');
-		$this->assertSame([], $ocs->getData(), 'Expected that no data is send');
-
-		$meta = $ocs->getMeta();
-		$this->assertNotEmpty($meta);
-		$this->assertArrayHasKey('message', $meta);
-		$this->assertSame($message, $meta['message']);
-	}
-
-	/**
 	 * @dataProvider dataTestSplitUserRemote
 	 *
 	 * @param string $remote