From 2fdda01b4b75cccc0a7d7c541612121c82f64e7f Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 26 Aug 2016 10:52:06 +0200 Subject: [PATCH] Sharee API to AppFramework * Move to OCSController * Move to Controller folder * Use automatic DI * Use function parameters * Updated tests --- apps/files_sharing/appinfo/routes.php | 25 ++---- .../ShareesAPIController.php} | 58 ++++++------ .../ShareesAPIControllerTest.php} | 89 +++++++++---------- 3 files changed, 79 insertions(+), 93 deletions(-) rename apps/files_sharing/lib/{API/Sharees.php => Controller/ShareesAPIController.php} (92%) rename apps/files_sharing/tests/{API/ShareesTest.php => Controller/ShareesAPIControllerTest.php} (95%) diff --git a/apps/files_sharing/appinfo/routes.php b/apps/files_sharing/appinfo/routes.php index ee7ea55d50..de6967c7d6 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 similarity index 92% rename from apps/files_sharing/lib/API/Sharees.php rename to apps/files_sharing/lib/Controller/ShareesAPIController.php index a7eb13708f..b884aa9f1d 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 * */ -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 similarity index 95% rename from apps/files_sharing/tests/API/ShareesTest.php rename to apps/files_sharing/tests/Controller/ShareesAPIControllerTest.php index 041b45216a..5cb073ecf0 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() { @@ -1445,20 +1450,6 @@ class ShareesTest extends TestCase { $this->assertEquals($expected, $this->invokePrivate($this->sharees, 'isV2')); } - /** - * @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 *