From 3ff7ac6ea47fa7a06e5dce0cc9ba931c797390ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Mo=CC=88ller?= Date: Thu, 5 Feb 2015 11:11:47 +0100 Subject: [PATCH 01/12] Fix: Move plugin functionality into appropriate service --- module/User/config/module.config.php | 5 +- .../src/User/Controller/UserController.php | 16 ++++- .../User/Controller/UserControllerFactory.php | 26 ++++++++ .../Controller/UserControllerTest.php | 33 ++++------ .../Mvc/Controller/Plugin/ListModule.php | 66 ------------------- .../Controller/Plugin/ListModuleFactory.php | 34 ---------- .../ZfModule/src/ZfModule/Service/Module.php | 33 ++++++++++ 7 files changed, 90 insertions(+), 123 deletions(-) create mode 100644 module/User/src/User/Controller/UserControllerFactory.php delete mode 100644 module/ZfModule/src/ZfModule/Mvc/Controller/Plugin/ListModule.php delete mode 100644 module/ZfModule/src/ZfModule/Mvc/Controller/Plugin/ListModuleFactory.php diff --git a/module/User/config/module.config.php b/module/User/config/module.config.php index a3343bf0..a13ae494 100644 --- a/module/User/config/module.config.php +++ b/module/User/config/module.config.php @@ -5,9 +5,12 @@ return [ 'controllers' => [ - 'invokables' => [ + 'aliases' => [ 'zfcuser' => Controller\UserController::class, ], + 'factories' => [ + Controller\UserController::class => Controller\UserControllerFactory::class, + ], ], 'view_manager' => [ 'template_map' => [ diff --git a/module/User/src/User/Controller/UserController.php b/module/User/src/User/Controller/UserController.php index 92adb323..2b088201 100644 --- a/module/User/src/User/Controller/UserController.php +++ b/module/User/src/User/Controller/UserController.php @@ -5,6 +5,7 @@ use Zend\View\Model\ViewModel; use ZfcUser\Controller\Plugin\ZfcUserAuthentication; use ZfcUser\Controller\UserController as ZfcUserController; +use ZfModule\Service; /** * @method array listModule(array $options) @@ -12,6 +13,19 @@ */ class UserController extends ZfcUserController { + /** + * @var Service\Module + */ + private $moduleService; + + /** + * @param Service\Module $moduleService + */ + public function __construct(Service\Module $moduleService) + { + $this->moduleService = $moduleService; + } + public function indexAction() { if (!$this->zfcUserAuthentication()->hasIdentity()) { @@ -19,7 +33,7 @@ public function indexAction() } $viewModel = new ViewModel([ - 'modules' => $this->listModule([ + 'modules' => $this->moduleService->listModule([ 'user' => true, ]), ]); diff --git a/module/User/src/User/Controller/UserControllerFactory.php b/module/User/src/User/Controller/UserControllerFactory.php new file mode 100644 index 00000000..f5f5ae16 --- /dev/null +++ b/module/User/src/User/Controller/UserControllerFactory.php @@ -0,0 +1,26 @@ +getServiceLocator(); + + /* @var Service\Module $moduleService */ + $moduleService = $serviceManager->get('zfmodule_service_module'); + + return new UserController($moduleService); + } +} diff --git a/module/User/test/UserTest/Integration/Controller/UserControllerTest.php b/module/User/test/UserTest/Integration/Controller/UserControllerTest.php index 8d41d11c..517b618d 100644 --- a/module/User/test/UserTest/Integration/Controller/UserControllerTest.php +++ b/module/User/test/UserTest/Integration/Controller/UserControllerTest.php @@ -3,15 +3,13 @@ namespace UserTest\Integration\Controller; use ApplicationTest\Integration\Util\Bootstrap; -use User\Controller; use User\Entity\User; use User\View\Helper\UserOrganizations; use Zend\Authentication\AuthenticationService; use Zend\Http; -use Zend\Mvc; use Zend\Test\PHPUnit\Controller\AbstractHttpControllerTestCase; use Zend\View; -use ZfModule\Mvc\Controller\Plugin\ListModule; +use ZfModule\Service; use ZfModule\View\Helper\TotalModules; /** @@ -74,38 +72,31 @@ public function testIndexActionSetsModulesIfAuthenticated() ->willReturn(new User()) ; - $serviceManager = $this->getApplicationServiceLocator(); - - $serviceManager - ->setAllowOverride(true) - ->setService( - 'zfcuser_auth_service', - $authenticationService - ) - ; - - $listModule = $this->getMockBuilder(ListModule::class) + $moduleService = $this->getMockBuilder(Service\Module::class) ->disableOriginalConstructor() ->getMock() ; - $listModule + $moduleService ->expects($this->once()) - ->method('__invoke') + ->method('listModule') ->with($this->equalTo([ 'user' => true, ])) ->willReturn([]) ; - /* @var Mvc\Controller\PluginManager $controllerPluginManager */ - $controllerPluginManager = $serviceManager->get('ControllerPluginManager'); + $serviceManager = $this->getApplicationServiceLocator(); - $controllerPluginManager + $serviceManager ->setAllowOverride(true) ->setService( - 'listModule', - $listModule + 'zfcuser_auth_service', + $authenticationService + ) + ->setService( + 'zfmodule_service_module', + $moduleService ) ; diff --git a/module/ZfModule/src/ZfModule/Mvc/Controller/Plugin/ListModule.php b/module/ZfModule/src/ZfModule/Mvc/Controller/Plugin/ListModule.php deleted file mode 100644 index 0ff094a1..00000000 --- a/module/ZfModule/src/ZfModule/Mvc/Controller/Plugin/ListModule.php +++ /dev/null @@ -1,66 +0,0 @@ -moduleMapper = $moduleMapper; - $this->githubClient = $githubClient; - } - - /** - * __invoke - * - * @access public - * @param array $options array of options - * @return array Array of modules - */ - public function __invoke($options = null) - { - //need to fetch top lvl ServiceLocator - $user = isset($options['user']) ? $options['user'] : false; - - //limit modules to only user modules - if ($user) { - $repositories = $this->githubClient->api('current_user')->repos([ - 'type' => 'all', - 'per_page' => 100, - ]); - - $modules = []; - foreach ($repositories as $repository) { - if (!$repository->fork && $repository->permissions->push) { - $module = $this->moduleMapper->findByName($repository->name); - if ($module) { - $modules[] = $module; - } - } - } - } else { - $limit = isset($options['limit']) ? $options['limit'] : null; - $modules = $this->moduleMapper->findAll($limit, 'created_at', 'DESC'); - } - - return $modules; - } -} diff --git a/module/ZfModule/src/ZfModule/Mvc/Controller/Plugin/ListModuleFactory.php b/module/ZfModule/src/ZfModule/Mvc/Controller/Plugin/ListModuleFactory.php deleted file mode 100644 index 6ac6ba09..00000000 --- a/module/ZfModule/src/ZfModule/Mvc/Controller/Plugin/ListModuleFactory.php +++ /dev/null @@ -1,34 +0,0 @@ -getServiceLocator(); - - /* @var Mapper\Module $moduleMapper */ - $moduleMapper = $serviceManager->get('zfmodule_mapper_module'); - - /* @var Client $githubClient */ - $githubClient = $serviceManager->get('EdpGithub\Client'); - - return new ListModule( - $moduleMapper, - $githubClient - ); - } -} diff --git a/module/ZfModule/src/ZfModule/Service/Module.php b/module/ZfModule/src/ZfModule/Service/Module.php index 44e1177d..856ac22d 100644 --- a/module/ZfModule/src/ZfModule/Service/Module.php +++ b/module/ZfModule/src/ZfModule/Service/Module.php @@ -80,4 +80,37 @@ public function isModule(stdClass $repository) return false; } + + /** + * @param array $options array of options + * @return array Array of modules + */ + public function listModule($options = null) + { + //need to fetch top lvl ServiceLocator + $user = isset($options['user']) ? $options['user'] : false; + + //limit modules to only user modules + if ($user) { + $repositories = $this->githubClient->api('current_user')->repos([ + 'type' => 'all', + 'per_page' => 100, + ]); + + $modules = []; + foreach ($repositories as $repository) { + if (!$repository->fork && $repository->permissions->push) { + $module = $this->moduleMapper->findByName($repository->name); + if ($module) { + $modules[] = $module; + } + } + } + } else { + $limit = isset($options['limit']) ? $options['limit'] : null; + $modules = $this->moduleMapper->findAll($limit, 'created_at', 'DESC'); + } + + return $modules; + } } From f918297d45283314668c55b8566d10a4500bc3e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Mo=CC=88ller?= Date: Thu, 5 Feb 2015 19:12:58 +0100 Subject: [PATCH 02/12] Enhancement: Add tests --- .../test/ZfModuleTest/Service/ModuleTest.php | 352 ++++++++++++++++++ 1 file changed, 352 insertions(+) create mode 100644 module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php diff --git a/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php b/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php new file mode 100644 index 00000000..626f2db0 --- /dev/null +++ b/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php @@ -0,0 +1,352 @@ +getMockBuilder(Entity\Module::class)->getMock(); + + $modules = [ + $module, + ]; + + $moduleMapper = $this->getMockBuilder(Mapper\Module::class)->getMock(); + + $moduleMapper + ->expects($this->once()) + ->method('findAll') + ->with( + $this->equalTo(null), + $this->equalTo('created_at'), + $this->equalTo('DESC') + ) + ->willReturn($modules) + ; + + $githubClient = $this->getMockBuilder(Client::class)->getMock(); + + $service = new Service\Module( + $moduleMapper, + $githubClient + ); + + $this->assertSame($modules, $service->listModule()); + } + + public function testListModuleWithUserOptionSetToFalseListsAllModulesFromDatabase() + { + $module = $this->getMockBuilder(Entity\Module::class)->getMock(); + + $modules = [ + $module, + ]; + + $moduleMapper = $this->getMockBuilder(Mapper\Module::class)->getMock(); + + $moduleMapper + ->expects($this->once()) + ->method('findAll') + ->with( + $this->equalTo(null), + $this->equalTo('created_at'), + $this->equalTo('DESC') + ) + ->willReturn($modules) + ; + + $githubClient = $this->getMockBuilder(Client::class)->getMock(); + + $service = new Service\Module( + $moduleMapper, + $githubClient + ); + + $options = [ + 'user' => false, + ]; + + $this->assertSame($modules, $service->listModule($options)); + } + + public function testListModuleWithUserOptionSetToFalseAndLimitListsCurrentUsersModulesFromDatabaseLimited() + { + $limit = 10; + + $module = $this->getMockBuilder(Entity\Module::class)->getMock(); + + $modules = [ + $module, + ]; + + $moduleMapper = $this->getMockBuilder(Mapper\Module::class)->getMock(); + + $moduleMapper + ->expects($this->once()) + ->method('findAll') + ->with( + $this->equalTo($limit), + $this->equalTo('created_at'), + $this->equalTo('DESC') + ) + ->willReturn($modules) + ; + + $githubClient = $this->getMockBuilder(Client::class)->getMock(); + + $service = new Service\Module( + $moduleMapper, + $githubClient + ); + + $options = [ + 'user' => false, + 'limit' => $limit, + ]; + + $this->assertSame($modules, $service->listModule($options)); + } + + public function testListModuleWithUserOptionSetToTrueListsCurrentUsersModulesFromApiFoundInDatabase() + { + $name = 'foo'; + + $repository = new stdClass(); + $repository->fork = false; + $repository->permissions = new stdClass(); + $repository->permissions->push = true; + $repository->name = $name; + + $module = $this->getMockBuilder(Entity\Module::class)->getMock(); + + $modules = [ + $module, + ]; + + $moduleMapper = $this->getMockBuilder(Mapper\Module::class)->getMock(); + + $moduleMapper + ->expects($this->once()) + ->method('findByName') + ->with($this->equalTo($name)) + ->willReturn($module) + ; + + $currentUserService = $this->getMockBuilder(Api\CurrentUser::class)->getMock(); + + $currentUserService + ->expects($this->once()) + ->method('repos') + ->with($this->logicalAnd( + $this->arrayHasKey('type'), + $this->arrayHasKey('per_page') + )) + ->willReturnCallback(function ($params) use ($repository) { + if ('all' === $params['type'] && 100 === $params['per_page']) { + return [ + $repository, + ]; + } + + return null; + }) + ; + + $githubClient = $this->getMockBuilder(Client::class)->getMock(); + + $githubClient + ->expects($this->once()) + ->method('api') + ->with($this->equalTo('current_user')) + ->willReturn($currentUserService) + ; + + $service = new Service\Module( + $moduleMapper, + $githubClient + ); + + $options = [ + 'user' => true, + ]; + + $this->assertSame($modules, $service->listModule($options)); + } + + public function testListModuleWithUserOptionSetToTrueDoesNotLookupModulesFromApiWhereUserHasNoPushPrivilege() + { + $repository = new stdClass(); + $repository->fork = false; + $repository->permissions = new stdClass(); + $repository->permissions->push = false; + + $moduleMapper = $this->getMockBuilder(Mapper\Module::class)->getMock(); + + $moduleMapper + ->expects($this->never()) + ->method('findByName') + ; + + $currentUserService = $this->getMockBuilder(Api\CurrentUser::class)->getMock(); + + $currentUserService + ->expects($this->once()) + ->method('repos') + ->with($this->logicalAnd( + $this->arrayHasKey('type'), + $this->arrayHasKey('per_page') + )) + ->willReturnCallback(function ($params) use ($repository) { + if ('all' === $params['type'] && 100 === $params['per_page']) { + return [ + $repository, + ]; + } + + return null; + }) + ; + + $githubClient = $this->getMockBuilder(Client::class)->getMock(); + + $githubClient + ->expects($this->once()) + ->method('api') + ->with($this->equalTo('current_user')) + ->willReturn($currentUserService) + ; + + $service = new Service\Module( + $moduleMapper, + $githubClient + ); + + $options = [ + 'user' => true, + ]; + + $this->assertSame([], $service->listModule($options)); + } + + public function testListModuleWithUserOptionSetToTrueDoesNotLookupModulesFromApiThatAreForks() + { + $repository = new stdClass(); + $repository->fork = true; + + $moduleMapper = $this->getMockBuilder(Mapper\Module::class)->getMock(); + + $moduleMapper + ->expects($this->never()) + ->method('findByName') + ; + + $currentUserService = $this->getMockBuilder(Api\CurrentUser::class)->getMock(); + + $currentUserService + ->expects($this->once()) + ->method('repos') + ->with($this->logicalAnd( + $this->arrayHasKey('type'), + $this->arrayHasKey('per_page') + )) + ->willReturnCallback(function ($params) use ($repository) { + if ('all' === $params['type'] && 100 === $params['per_page']) { + return [ + $repository, + ]; + } + + return null; + }) + ; + + $githubClient = $this->getMockBuilder(Client::class)->getMock(); + + $githubClient + ->expects($this->once()) + ->method('api') + ->with($this->equalTo('current_user')) + ->willReturn($currentUserService) + ; + + $service = new Service\Module( + $moduleMapper, + $githubClient + ); + + $options = [ + 'user' => true, + ]; + + $this->assertSame([], $service->listModule($options)); + } + + public function testListModuleWithUserOptionSetToTrueDoesNotListModulesFromApiNotFoundInDatabase() + { + $name = 'foo'; + + $repository = new stdClass(); + $repository->fork = false; + $repository->permissions = new stdClass(); + $repository->permissions->push = true; + $repository->name = $name; + + $moduleMapper = $this->getMockBuilder(Mapper\Module::class)->getMock(); + + $moduleMapper + ->expects($this->once()) + ->method('findByName') + ->with($this->equalTo($name)) + ->willReturn(null) + ; + + $currentUserService = $this->getMockBuilder(Api\CurrentUser::class)->getMock(); + + $currentUserService + ->expects($this->once()) + ->method('repos') + ->with($this->logicalAnd( + $this->arrayHasKey('type'), + $this->arrayHasKey('per_page') + )) + ->willReturnCallback(function ($params) use ($repository) { + if ('all' === $params['type'] && 100 === $params['per_page']) { + return [ + $repository, + ]; + } + + return null; + }) + ; + + $githubClient = $this->getMockBuilder(Client::class)->getMock(); + + $githubClient + ->expects($this->once()) + ->method('api') + ->with($this->equalTo('current_user')) + ->willReturn($currentUserService) + ; + + $service = new Service\Module( + $moduleMapper, + $githubClient + ); + + $options = [ + 'user' => true, + ]; + + $this->assertSame([], $service->listModule($options)); + } +} From 65905d341bc639d9d8190a7abd022fbc552ecc61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Mo=CC=88ller?= Date: Thu, 5 Feb 2015 19:13:48 +0100 Subject: [PATCH 03/12] Fix: Remove comments --- module/ZfModule/src/ZfModule/Service/Module.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/module/ZfModule/src/ZfModule/Service/Module.php b/module/ZfModule/src/ZfModule/Service/Module.php index 856ac22d..4cf47d82 100644 --- a/module/ZfModule/src/ZfModule/Service/Module.php +++ b/module/ZfModule/src/ZfModule/Service/Module.php @@ -87,10 +87,8 @@ public function isModule(stdClass $repository) */ public function listModule($options = null) { - //need to fetch top lvl ServiceLocator $user = isset($options['user']) ? $options['user'] : false; - //limit modules to only user modules if ($user) { $repositories = $this->githubClient->api('current_user')->repos([ 'type' => 'all', From 52c0e61b35570b6634254af9142a4fa7f29d0ed5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Mo=CC=88ller?= Date: Thu, 5 Feb 2015 19:14:45 +0100 Subject: [PATCH 04/12] Enhancement: Return early --- .../ZfModule/src/ZfModule/Service/Module.php | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/module/ZfModule/src/ZfModule/Service/Module.php b/module/ZfModule/src/ZfModule/Service/Module.php index 4cf47d82..2e8e4d34 100644 --- a/module/ZfModule/src/ZfModule/Service/Module.php +++ b/module/ZfModule/src/ZfModule/Service/Module.php @@ -89,24 +89,29 @@ public function listModule($options = null) { $user = isset($options['user']) ? $options['user'] : false; - if ($user) { - $repositories = $this->githubClient->api('current_user')->repos([ - 'type' => 'all', - 'per_page' => 100, - ]); - - $modules = []; - foreach ($repositories as $repository) { - if (!$repository->fork && $repository->permissions->push) { - $module = $this->moduleMapper->findByName($repository->name); - if ($module) { - $modules[] = $module; - } + if (!$user) { + $limit = isset($options['limit']) ? $options['limit'] : null; + + return $this->moduleMapper->findAll( + $limit, + 'created_at', + 'DESC' + ); + } + + $repositories = $this->githubClient->api('current_user')->repos([ + 'type' => 'all', + 'per_page' => 100, + ]); + + $modules = []; + foreach ($repositories as $repository) { + if (!$repository->fork && $repository->permissions->push) { + $module = $this->moduleMapper->findByName($repository->name); + if ($module) { + $modules[] = $module; } } - } else { - $limit = isset($options['limit']) ? $options['limit'] : null; - $modules = $this->moduleMapper->findAll($limit, 'created_at', 'DESC'); } return $modules; From db2f9d8eb2bbe798c87c0cd0e28e04f88d76683a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Mo=CC=88ller?= Date: Thu, 5 Feb 2015 19:24:15 +0100 Subject: [PATCH 05/12] Enhancement: Use array_walk for filtering/mapping --- .../ZfModule/src/ZfModule/Service/Module.php | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/module/ZfModule/src/ZfModule/Service/Module.php b/module/ZfModule/src/ZfModule/Service/Module.php index 2e8e4d34..98fa472f 100644 --- a/module/ZfModule/src/ZfModule/Service/Module.php +++ b/module/ZfModule/src/ZfModule/Service/Module.php @@ -105,14 +105,23 @@ public function listModule($options = null) ]); $modules = []; - foreach ($repositories as $repository) { - if (!$repository->fork && $repository->permissions->push) { - $module = $this->moduleMapper->findByName($repository->name); - if ($module) { - $modules[] = $module; - } + + array_walk($repositories, function ($repository) use (&$modules) { + if (true === $repository->fork) { + return; } - } + + if (false === $repository->permissions->push) { + return; + } + + $module = $this->moduleMapper->findByName($repository->name); + if (null === $module) { + return; + } + + array_push($modules, $module); + }); return $modules; } From e636caeb3e2fb5870d75b5c70ea336cd7e1b057d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Mo=CC=88ller?= Date: Thu, 5 Feb 2015 19:29:17 +0100 Subject: [PATCH 06/12] Enhancement: Extract methods --- .../ZfModule/src/ZfModule/Service/Module.php | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/module/ZfModule/src/ZfModule/Service/Module.php b/module/ZfModule/src/ZfModule/Service/Module.php index 98fa472f..b32cae26 100644 --- a/module/ZfModule/src/ZfModule/Service/Module.php +++ b/module/ZfModule/src/ZfModule/Service/Module.php @@ -92,13 +92,30 @@ public function listModule($options = null) if (!$user) { $limit = isset($options['limit']) ? $options['limit'] : null; - return $this->moduleMapper->findAll( - $limit, - 'created_at', - 'DESC' - ); + return $this->listAllModules($limit); } + return $this->listUserModules(); + } + + /** + * @param $limit + * @return \Zend\Db\ResultSet\HydratingResultSet + */ + public function listAllModules($limit) + { + return $this->moduleMapper->findAll( + $limit, + 'created_at', + 'DESC' + ); + } + + /** + * @return array + */ + public function listUserModules() + { $repositories = $this->githubClient->api('current_user')->repos([ 'type' => 'all', 'per_page' => 100, From 4309a1ae27334ee07705dc88bcdc59491a81b391 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Mo=CC=88ller?= Date: Thu, 5 Feb 2015 19:34:51 +0100 Subject: [PATCH 07/12] Enhancement: Remove listModule() and use appropriate method directly --- .../src/User/Controller/UserController.php | 4 +- .../Controller/UserControllerTest.php | 5 +- .../ZfModule/src/ZfModule/Service/Module.php | 24 +----- .../test/ZfModuleTest/Service/ModuleTest.php | 82 +++---------------- 4 files changed, 19 insertions(+), 96 deletions(-) diff --git a/module/User/src/User/Controller/UserController.php b/module/User/src/User/Controller/UserController.php index 2b088201..a505b733 100644 --- a/module/User/src/User/Controller/UserController.php +++ b/module/User/src/User/Controller/UserController.php @@ -33,9 +33,7 @@ public function indexAction() } $viewModel = new ViewModel([ - 'modules' => $this->moduleService->listModule([ - 'user' => true, - ]), + 'modules' => $this->moduleService->listUserModules(), ]); $viewModel->setTemplate('zfc-user/user/index'); diff --git a/module/User/test/UserTest/Integration/Controller/UserControllerTest.php b/module/User/test/UserTest/Integration/Controller/UserControllerTest.php index 517b618d..5523f9fd 100644 --- a/module/User/test/UserTest/Integration/Controller/UserControllerTest.php +++ b/module/User/test/UserTest/Integration/Controller/UserControllerTest.php @@ -79,10 +79,7 @@ public function testIndexActionSetsModulesIfAuthenticated() $moduleService ->expects($this->once()) - ->method('listModule') - ->with($this->equalTo([ - 'user' => true, - ])) + ->method('listUserModules') ->willReturn([]) ; diff --git a/module/ZfModule/src/ZfModule/Service/Module.php b/module/ZfModule/src/ZfModule/Service/Module.php index b32cae26..a9b52913 100644 --- a/module/ZfModule/src/ZfModule/Service/Module.php +++ b/module/ZfModule/src/ZfModule/Service/Module.php @@ -5,6 +5,7 @@ use EdpGithub\Client; use stdClass; use ZfcBase\EventManager\EventProvider; +use ZfModule\Entity; use ZfModule\Mapper; class Module extends EventProvider @@ -81,28 +82,11 @@ public function isModule(stdClass $repository) return false; } - /** - * @param array $options array of options - * @return array Array of modules - */ - public function listModule($options = null) - { - $user = isset($options['user']) ? $options['user'] : false; - - if (!$user) { - $limit = isset($options['limit']) ? $options['limit'] : null; - - return $this->listAllModules($limit); - } - - return $this->listUserModules(); - } - /** * @param $limit - * @return \Zend\Db\ResultSet\HydratingResultSet + * @return Entity\Module[] */ - public function listAllModules($limit) + public function listAllModules($limit = null) { return $this->moduleMapper->findAll( $limit, @@ -112,7 +96,7 @@ public function listAllModules($limit) } /** - * @return array + * @return Entity\Module[] */ public function listUserModules() { diff --git a/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php b/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php index 626f2db0..2207f8fc 100644 --- a/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php +++ b/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php @@ -12,7 +12,7 @@ class ModuleTest extends PHPUnit_Framework_TestCase { - public function testListModuleWithoutArgumentListsAllModulesFromDatabase() + public function testListAllModulesWithoutArgumentListsAllModulesFromDatabase() { $module = $this->getMockBuilder(Entity\Module::class)->getMock(); @@ -40,47 +40,12 @@ public function testListModuleWithoutArgumentListsAllModulesFromDatabase() $githubClient ); - $this->assertSame($modules, $service->listModule()); + $this->assertSame($modules, $service->listAllModules()); } - public function testListModuleWithUserOptionSetToFalseListsAllModulesFromDatabase() + public function testListAllModulesWithArgumentListsModulesFromDatabaseLimited() { - $module = $this->getMockBuilder(Entity\Module::class)->getMock(); - - $modules = [ - $module, - ]; - - $moduleMapper = $this->getMockBuilder(Mapper\Module::class)->getMock(); - - $moduleMapper - ->expects($this->once()) - ->method('findAll') - ->with( - $this->equalTo(null), - $this->equalTo('created_at'), - $this->equalTo('DESC') - ) - ->willReturn($modules) - ; - - $githubClient = $this->getMockBuilder(Client::class)->getMock(); - - $service = new Service\Module( - $moduleMapper, - $githubClient - ); - - $options = [ - 'user' => false, - ]; - - $this->assertSame($modules, $service->listModule($options)); - } - - public function testListModuleWithUserOptionSetToFalseAndLimitListsCurrentUsersModulesFromDatabaseLimited() - { - $limit = 10; + $limit = 9000; $module = $this->getMockBuilder(Entity\Module::class)->getMock(); @@ -108,15 +73,10 @@ public function testListModuleWithUserOptionSetToFalseAndLimitListsCurrentUsersM $githubClient ); - $options = [ - 'user' => false, - 'limit' => $limit, - ]; - - $this->assertSame($modules, $service->listModule($options)); + $this->assertSame($modules, $service->listAllModules($limit)); } - public function testListModuleWithUserOptionSetToTrueListsCurrentUsersModulesFromApiFoundInDatabase() + public function testListUserModuleListsCurrentUsersModulesFromApiFoundInDatabase() { $name = 'foo'; @@ -175,14 +135,10 @@ public function testListModuleWithUserOptionSetToTrueListsCurrentUsersModulesFro $githubClient ); - $options = [ - 'user' => true, - ]; - - $this->assertSame($modules, $service->listModule($options)); + $this->assertSame($modules, $service->listUserModules()); } - public function testListModuleWithUserOptionSetToTrueDoesNotLookupModulesFromApiWhereUserHasNoPushPrivilege() + public function testListUserModulesDoesNotLookupModulesFromApiWhereUserHasNoPushPrivilege() { $repository = new stdClass(); $repository->fork = false; @@ -230,14 +186,10 @@ public function testListModuleWithUserOptionSetToTrueDoesNotLookupModulesFromApi $githubClient ); - $options = [ - 'user' => true, - ]; - - $this->assertSame([], $service->listModule($options)); + $this->assertSame([], $service->listUserModules()); } - public function testListModuleWithUserOptionSetToTrueDoesNotLookupModulesFromApiThatAreForks() + public function testListUsersModuleDoesNotLookupModulesFromApiThatAreForks() { $repository = new stdClass(); $repository->fork = true; @@ -283,14 +235,10 @@ public function testListModuleWithUserOptionSetToTrueDoesNotLookupModulesFromApi $githubClient ); - $options = [ - 'user' => true, - ]; - - $this->assertSame([], $service->listModule($options)); + $this->assertSame([], $service->listUserModules()); } - public function testListModuleWithUserOptionSetToTrueDoesNotListModulesFromApiNotFoundInDatabase() + public function testListUserModulesDoesNotListModulesFromApiNotFoundInDatabase() { $name = 'foo'; @@ -343,10 +291,6 @@ public function testListModuleWithUserOptionSetToTrueDoesNotListModulesFromApiNo $githubClient ); - $options = [ - 'user' => true, - ]; - - $this->assertSame([], $service->listModule($options)); + $this->assertSame([], $service->listUserModules()); } } From 2139bbaee8b9ea9922f0a219005f154bfb1ecc48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Mo=CC=88ller?= Date: Thu, 5 Feb 2015 19:43:46 +0100 Subject: [PATCH 08/12] Enhancement: Rename methods --- module/User/src/User/Controller/UserController.php | 2 +- .../Integration/Controller/UserControllerTest.php | 2 +- module/ZfModule/src/ZfModule/Service/Module.php | 4 ++-- .../test/ZfModuleTest/Service/ModuleTest.php | 12 ++++++------ 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/module/User/src/User/Controller/UserController.php b/module/User/src/User/Controller/UserController.php index a505b733..04bdaf49 100644 --- a/module/User/src/User/Controller/UserController.php +++ b/module/User/src/User/Controller/UserController.php @@ -33,7 +33,7 @@ public function indexAction() } $viewModel = new ViewModel([ - 'modules' => $this->moduleService->listUserModules(), + 'modules' => $this->moduleService->currentUserModules(), ]); $viewModel->setTemplate('zfc-user/user/index'); diff --git a/module/User/test/UserTest/Integration/Controller/UserControllerTest.php b/module/User/test/UserTest/Integration/Controller/UserControllerTest.php index 5523f9fd..a4e7cc2d 100644 --- a/module/User/test/UserTest/Integration/Controller/UserControllerTest.php +++ b/module/User/test/UserTest/Integration/Controller/UserControllerTest.php @@ -79,7 +79,7 @@ public function testIndexActionSetsModulesIfAuthenticated() $moduleService ->expects($this->once()) - ->method('listUserModules') + ->method('currentUserModules') ->willReturn([]) ; diff --git a/module/ZfModule/src/ZfModule/Service/Module.php b/module/ZfModule/src/ZfModule/Service/Module.php index a9b52913..5238d2e1 100644 --- a/module/ZfModule/src/ZfModule/Service/Module.php +++ b/module/ZfModule/src/ZfModule/Service/Module.php @@ -86,7 +86,7 @@ public function isModule(stdClass $repository) * @param $limit * @return Entity\Module[] */ - public function listAllModules($limit = null) + public function allModules($limit = null) { return $this->moduleMapper->findAll( $limit, @@ -98,7 +98,7 @@ public function listAllModules($limit = null) /** * @return Entity\Module[] */ - public function listUserModules() + public function currentUserModules() { $repositories = $this->githubClient->api('current_user')->repos([ 'type' => 'all', diff --git a/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php b/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php index 2207f8fc..13009321 100644 --- a/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php +++ b/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php @@ -40,7 +40,7 @@ public function testListAllModulesWithoutArgumentListsAllModulesFromDatabase() $githubClient ); - $this->assertSame($modules, $service->listAllModules()); + $this->assertSame($modules, $service->allModules()); } public function testListAllModulesWithArgumentListsModulesFromDatabaseLimited() @@ -73,7 +73,7 @@ public function testListAllModulesWithArgumentListsModulesFromDatabaseLimited() $githubClient ); - $this->assertSame($modules, $service->listAllModules($limit)); + $this->assertSame($modules, $service->allModules($limit)); } public function testListUserModuleListsCurrentUsersModulesFromApiFoundInDatabase() @@ -135,7 +135,7 @@ public function testListUserModuleListsCurrentUsersModulesFromApiFoundInDatabase $githubClient ); - $this->assertSame($modules, $service->listUserModules()); + $this->assertSame($modules, $service->currentUserModules()); } public function testListUserModulesDoesNotLookupModulesFromApiWhereUserHasNoPushPrivilege() @@ -186,7 +186,7 @@ public function testListUserModulesDoesNotLookupModulesFromApiWhereUserHasNoPush $githubClient ); - $this->assertSame([], $service->listUserModules()); + $this->assertSame([], $service->currentUserModules()); } public function testListUsersModuleDoesNotLookupModulesFromApiThatAreForks() @@ -235,7 +235,7 @@ public function testListUsersModuleDoesNotLookupModulesFromApiThatAreForks() $githubClient ); - $this->assertSame([], $service->listUserModules()); + $this->assertSame([], $service->currentUserModules()); } public function testListUserModulesDoesNotListModulesFromApiNotFoundInDatabase() @@ -291,6 +291,6 @@ public function testListUserModulesDoesNotListModulesFromApiNotFoundInDatabase() $githubClient ); - $this->assertSame([], $service->listUserModules()); + $this->assertSame([], $service->currentUserModules()); } } From 59648ce74566b48f28ea94272865f89d79ab8c6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Mo=CC=88ller?= Date: Fri, 6 Feb 2015 23:07:13 +0100 Subject: [PATCH 09/12] Fix: findByName() returns false if nothing found --- module/ZfModule/src/ZfModule/Service/Module.php | 2 +- module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/module/ZfModule/src/ZfModule/Service/Module.php b/module/ZfModule/src/ZfModule/Service/Module.php index 5238d2e1..f04d024d 100644 --- a/module/ZfModule/src/ZfModule/Service/Module.php +++ b/module/ZfModule/src/ZfModule/Service/Module.php @@ -117,7 +117,7 @@ public function currentUserModules() } $module = $this->moduleMapper->findByName($repository->name); - if (null === $module) { + if (!($module instanceof Entity\Module)) { return; } diff --git a/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php b/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php index 13009321..1ae501ff 100644 --- a/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php +++ b/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php @@ -254,7 +254,7 @@ public function testListUserModulesDoesNotListModulesFromApiNotFoundInDatabase() ->expects($this->once()) ->method('findByName') ->with($this->equalTo($name)) - ->willReturn(null) + ->willReturn(false) ; $currentUserService = $this->getMockBuilder(Api\CurrentUser::class)->getMock(); From 84b52ae9304a0cc015ef98285954284acdaaf82a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Mo=CC=88ller?= Date: Fri, 6 Feb 2015 23:59:15 +0100 Subject: [PATCH 10/12] Fix: Method actually returns a RepositoryCollection --- .../ZfModule/src/ZfModule/Service/Module.php | 6 ++- .../Mock/Collection/RepositoryCollection.php | 49 +++++++++++++++++++ .../test/ZfModuleTest/Service/ModuleTest.php | 35 +++++++------ 3 files changed, 73 insertions(+), 17 deletions(-) create mode 100644 module/ZfModule/test/ZfModuleTest/Mock/Collection/RepositoryCollection.php diff --git a/module/ZfModule/src/ZfModule/Service/Module.php b/module/ZfModule/src/ZfModule/Service/Module.php index f04d024d..13fb630b 100644 --- a/module/ZfModule/src/ZfModule/Service/Module.php +++ b/module/ZfModule/src/ZfModule/Service/Module.php @@ -3,6 +3,7 @@ namespace ZfModule\Service; use EdpGithub\Client; +use EdpGithub\Collection\RepositoryCollection; use stdClass; use ZfcBase\EventManager\EventProvider; use ZfModule\Entity; @@ -100,13 +101,16 @@ public function allModules($limit = null) */ public function currentUserModules() { - $repositories = $this->githubClient->api('current_user')->repos([ + /* @var RepositoryCollection $repositoryCollection */ + $repositoryCollection = $this->githubClient->api('current_user')->repos([ 'type' => 'all', 'per_page' => 100, ]); $modules = []; + $repositories = iterator_to_array($repositoryCollection); + array_walk($repositories, function ($repository) use (&$modules) { if (true === $repository->fork) { return; diff --git a/module/ZfModule/test/ZfModuleTest/Mock/Collection/RepositoryCollection.php b/module/ZfModule/test/ZfModuleTest/Mock/Collection/RepositoryCollection.php new file mode 100644 index 00000000..1cbbbbb3 --- /dev/null +++ b/module/ZfModule/test/ZfModuleTest/Mock/Collection/RepositoryCollection.php @@ -0,0 +1,49 @@ +repositories = $repositories; + } + + public function current() + { + return current($this->repositories); + } + + public function next() + { + next($this->repositories); + } + + public function key() + { + return key($this->repositories); + } + + public function valid() + { + $key = key($this->repositories); + + if (null === $key || false === $key) { + return false; + } + + return true; + } + + public function rewind() + { + reset($this->repositories); + } +} diff --git a/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php b/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php index 1ae501ff..ea554946 100644 --- a/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php +++ b/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php @@ -9,6 +9,7 @@ use ZfModule\Entity; use ZfModule\Mapper; use ZfModule\Service; +use ZfModuleTest\Mock; class ModuleTest extends PHPUnit_Framework_TestCase { @@ -111,13 +112,14 @@ public function testListUserModuleListsCurrentUsersModulesFromApiFoundInDatabase $this->arrayHasKey('per_page') )) ->willReturnCallback(function ($params) use ($repository) { + + $repositories = []; + if ('all' === $params['type'] && 100 === $params['per_page']) { - return [ - $repository, - ]; + array_push($repositories, $repository); } - return null; + return new Mock\Collection\RepositoryCollection($repositories); }) ; @@ -162,13 +164,14 @@ public function testListUserModulesDoesNotLookupModulesFromApiWhereUserHasNoPush $this->arrayHasKey('per_page') )) ->willReturnCallback(function ($params) use ($repository) { + + $repositories = []; + if ('all' === $params['type'] && 100 === $params['per_page']) { - return [ - $repository, - ]; + array_push($repositories, $repository); } - return null; + return new Mock\Collection\RepositoryCollection($repositories); }) ; @@ -211,13 +214,13 @@ public function testListUsersModuleDoesNotLookupModulesFromApiThatAreForks() $this->arrayHasKey('per_page') )) ->willReturnCallback(function ($params) use ($repository) { + $repositories = []; + if ('all' === $params['type'] && 100 === $params['per_page']) { - return [ - $repository, - ]; + array_push($repositories, $repository); } - return null; + return new Mock\Collection\RepositoryCollection($repositories); }) ; @@ -267,13 +270,13 @@ public function testListUserModulesDoesNotListModulesFromApiNotFoundInDatabase() $this->arrayHasKey('per_page') )) ->willReturnCallback(function ($params) use ($repository) { + $repositories = []; + if ('all' === $params['type'] && 100 === $params['per_page']) { - return [ - $repository, - ]; + array_push($repositories, $repository); } - return null; + return new Mock\Collection\RepositoryCollection($repositories); }) ; From 611bef7d10b15cff79b5f458c0c78eae404fd2d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Mo=CC=88ller?= Date: Sat, 7 Feb 2015 00:01:53 +0100 Subject: [PATCH 11/12] Fix: Iterate using foreach() again --- module/ZfModule/src/ZfModule/Service/Module.php | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/module/ZfModule/src/ZfModule/Service/Module.php b/module/ZfModule/src/ZfModule/Service/Module.php index 13fb630b..92d346aa 100644 --- a/module/ZfModule/src/ZfModule/Service/Module.php +++ b/module/ZfModule/src/ZfModule/Service/Module.php @@ -109,24 +109,22 @@ public function currentUserModules() $modules = []; - $repositories = iterator_to_array($repositoryCollection); - - array_walk($repositories, function ($repository) use (&$modules) { + foreach ($repositoryCollection as $repository) { if (true === $repository->fork) { - return; + continue; } if (false === $repository->permissions->push) { - return; + continue; } $module = $this->moduleMapper->findByName($repository->name); if (!($module instanceof Entity\Module)) { - return; + continue; } array_push($modules, $module); - }); + } return $modules; } From a7607887883bc8eee0908976489063132ae24950 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Mo=CC=88ller?= Date: Sat, 7 Feb 2015 00:22:01 +0100 Subject: [PATCH 12/12] Enhancement: Simplify mocking --- .../test/ZfModuleTest/Service/ModuleTest.php | 74 +++++-------------- 1 file changed, 20 insertions(+), 54 deletions(-) diff --git a/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php b/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php index ea554946..c0cb9bd5 100644 --- a/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php +++ b/module/ZfModule/test/ZfModuleTest/Service/ModuleTest.php @@ -107,20 +107,11 @@ public function testListUserModuleListsCurrentUsersModulesFromApiFoundInDatabase $currentUserService ->expects($this->once()) ->method('repos') - ->with($this->logicalAnd( - $this->arrayHasKey('type'), - $this->arrayHasKey('per_page') - )) - ->willReturnCallback(function ($params) use ($repository) { - - $repositories = []; - - if ('all' === $params['type'] && 100 === $params['per_page']) { - array_push($repositories, $repository); - } - - return new Mock\Collection\RepositoryCollection($repositories); - }) + ->with($this->equalTo([ + 'type' => 'all', + 'per_page' => 100, + ])) + ->willReturn(new Mock\Collection\RepositoryCollection([$repository])) ; $githubClient = $this->getMockBuilder(Client::class)->getMock(); @@ -159,20 +150,11 @@ public function testListUserModulesDoesNotLookupModulesFromApiWhereUserHasNoPush $currentUserService ->expects($this->once()) ->method('repos') - ->with($this->logicalAnd( - $this->arrayHasKey('type'), - $this->arrayHasKey('per_page') - )) - ->willReturnCallback(function ($params) use ($repository) { - - $repositories = []; - - if ('all' === $params['type'] && 100 === $params['per_page']) { - array_push($repositories, $repository); - } - - return new Mock\Collection\RepositoryCollection($repositories); - }) + ->with($this->equalTo([ + 'type' => 'all', + 'per_page' => 100, + ])) + ->willReturn(new Mock\Collection\RepositoryCollection([$repository])) ; $githubClient = $this->getMockBuilder(Client::class)->getMock(); @@ -209,19 +191,11 @@ public function testListUsersModuleDoesNotLookupModulesFromApiThatAreForks() $currentUserService ->expects($this->once()) ->method('repos') - ->with($this->logicalAnd( - $this->arrayHasKey('type'), - $this->arrayHasKey('per_page') - )) - ->willReturnCallback(function ($params) use ($repository) { - $repositories = []; - - if ('all' === $params['type'] && 100 === $params['per_page']) { - array_push($repositories, $repository); - } - - return new Mock\Collection\RepositoryCollection($repositories); - }) + ->with($this->equalTo([ + 'type' => 'all', + 'per_page' => 100, + ])) + ->willReturn(new Mock\Collection\RepositoryCollection([$repository])) ; $githubClient = $this->getMockBuilder(Client::class)->getMock(); @@ -265,19 +239,11 @@ public function testListUserModulesDoesNotListModulesFromApiNotFoundInDatabase() $currentUserService ->expects($this->once()) ->method('repos') - ->with($this->logicalAnd( - $this->arrayHasKey('type'), - $this->arrayHasKey('per_page') - )) - ->willReturnCallback(function ($params) use ($repository) { - $repositories = []; - - if ('all' === $params['type'] && 100 === $params['per_page']) { - array_push($repositories, $repository); - } - - return new Mock\Collection\RepositoryCollection($repositories); - }) + ->with($this->equalTo([ + 'type' => 'all', + 'per_page' => 100, + ])) + ->willReturn(new Mock\Collection\RepositoryCollection([$repository])) ; $githubClient = $this->getMockBuilder(Client::class)->getMock();