From ca44157d6c92563247c39e2bd37b084ebcaeab3d Mon Sep 17 00:00:00 2001 From: Petar Spanja Date: Mon, 15 Apr 2013 14:22:23 +0200 Subject: [PATCH 1/8] EZP-20677: fixed User data not being deleted upon deletion of User Content Also: removed unnecessary validation of $user->id (mixed not numeric) --- eZ/Publish/Core/Repository/UserService.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/eZ/Publish/Core/Repository/UserService.php b/eZ/Publish/Core/Repository/UserService.php index 94610321284..056edcb2d49 100644 --- a/eZ/Publish/Core/Repository/UserService.php +++ b/eZ/Publish/Core/Repository/UserService.php @@ -650,15 +650,13 @@ public function loadUserByCredentials( $login, $password ) */ public function deleteUser( APIUser $user ) { - if ( !is_numeric( $user->id ) ) - throw new InvalidArgumentValue( "id", $user->id, "User" ); - $loadedUser = $this->loadUser( $user->id ); $this->repository->beginTransaction(); try { $this->repository->getContentService()->deleteContent( $loadedUser->getVersionInfo()->getContentInfo() ); + $this->userHandler->delete( $loadedUser->id ); $this->repository->commit(); } catch ( \Exception $e ) From a19f6e3e415e55315825aade07f0bb100575f240 Mon Sep 17 00:00:00 2001 From: Petar Spanja Date: Mon, 15 Apr 2013 14:26:06 +0200 Subject: [PATCH 2/8] EZP-20677: fixed throw on missing user or role assignment --- .../Core/Persistence/InMemory/UserHandler.php | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/eZ/Publish/Core/Persistence/InMemory/UserHandler.php b/eZ/Publish/Core/Persistence/InMemory/UserHandler.php index 0e0a140f3c6..4153a0a4f42 100644 --- a/eZ/Publish/Core/Persistence/InMemory/UserHandler.php +++ b/eZ/Publish/Core/Persistence/InMemory/UserHandler.php @@ -129,13 +129,18 @@ public function update( User $user ) * Delete user with the given ID. * * @param mixed $userId - * - * @todo Throw on missing user? */ public function delete( $userId ) { - $this->backend->delete( 'User', $userId ); - $this->backend->deleteByMatch( 'User\\RoleAssignment', array( 'contentId' => $userId ) ); + try + { + $this->backend->delete( 'User', $userId ); + $this->backend->deleteByMatch( 'User\\RoleAssignment', array( 'contentId' => $userId ) ); + } + catch ( NotFound $e ) + { + // Do nothing, we do not throw if User or RoleAssignments do not exist + } } /** From 6c5febe54b1706c1ece149fc949de7d92844e6af Mon Sep 17 00:00:00 2001 From: Petar Spanja Date: Mon, 15 Apr 2013 15:26:03 +0200 Subject: [PATCH 3/8] EZP-20677: tested deleteUser() with mocks, removed tests duplicated in integration suite --- .../Tests/Service/Integration/UserBase.php | 22 --- .../Tests/Service/Mock/UserTest.php | 152 ++++++++++++++++++ 2 files changed, 152 insertions(+), 22 deletions(-) create mode 100644 eZ/Publish/Core/Repository/Tests/Service/Mock/UserTest.php diff --git a/eZ/Publish/Core/Repository/Tests/Service/Integration/UserBase.php b/eZ/Publish/Core/Repository/Tests/Service/Integration/UserBase.php index b98c3ba9858..2d8d1939ca1 100644 --- a/eZ/Publish/Core/Repository/Tests/Service/Integration/UserBase.php +++ b/eZ/Publish/Core/Repository/Tests/Service/Integration/UserBase.php @@ -606,28 +606,6 @@ public function testLoadUserByCredentialsThrowsNotFoundExceptionBadPassword() $userService->loadUserByCredentials( 'admin', 'some_password' ); } - /** - * Test deleting a user - * @covers \eZ\Publish\API\Repository\UserService::deleteUser - */ - public function testDeleteUser() - { - $userService = $this->repository->getUserService(); - - $user = $userService->loadUser( 14 ); - $userService->deleteUser( $user ); - - try - { - $userService->loadUser( 14 ); - self::fail( "failed deleting a user" ); - } - catch ( NotFoundException $e ) - { - // Do nothing - } - } - /** * Test updating a user * @covers \eZ\Publish\API\Repository\UserService::updateUser diff --git a/eZ/Publish/Core/Repository/Tests/Service/Mock/UserTest.php b/eZ/Publish/Core/Repository/Tests/Service/Mock/UserTest.php new file mode 100644 index 00000000000..0eb569e3e17 --- /dev/null +++ b/eZ/Publish/Core/Repository/Tests/Service/Mock/UserTest.php @@ -0,0 +1,152 @@ +getRepositoryMock(); + $userService = $this->getPartlyMockedUserService( array( "loadUser" ) ); + $contentService = $this->getMock( "eZ\\Publish\\API\\Repository\\ContentService" ); + $userHandler = $this->getPersistenceMock()->userHandler(); + + $user = $this->getMock( "eZ\\Publish\\API\\Repository\\Values\\User\\User" ); + $loadedUser = $this->getMock( "eZ\\Publish\\API\\Repository\\Values\\User\\User" ); + $versionInfo = $this->getMock( "eZ\\Publish\\API\\Repository\\Values\\Content\\VersionInfo" ); + $contentInfo = $this->getMock( "eZ\\Publish\\API\\Repository\\Values\\Content\\ContentInfo" ); + + $user->expects( $this->once() ) + ->method( "__get" ) + ->with( "id" ) + ->will( $this->returnValue( 42 ) ); + + $versionInfo->expects( $this->once() ) + ->method( "getContentInfo" ) + ->will( $this->returnValue( $contentInfo ) ); + + $loadedUser->expects( $this->once() ) + ->method( "getVersionInfo" ) + ->will( $this->returnValue( $versionInfo ) ); + + $loadedUser->expects( $this->once() ) + ->method( "__get" ) + ->with( "id" ) + ->will( $this->returnValue( 42 ) ); + + $userService->expects( $this->once() ) + ->method( "loadUser" ) + ->with( 42 ) + ->will( $this->returnValue( $loadedUser ) ); + + $repository->expects( $this->once() )->method( "beginTransaction" ); + + $contentService->expects( $this->once() ) + ->method( "deleteContent" ) + ->with( $contentInfo ); + + $repository->expects( $this->once() ) + ->method( "getContentService" ) + ->will( $this->returnValue( $contentService ) ); + + /** @var \PHPUnit_Framework_MockObject_MockObject $userHandler */ + $userHandler->expects( $this->once() ) + ->method( "delete" ) + ->with( 42 ); + + $repository->expects( $this->once() )->method( "commit" ); + + /** @var \eZ\Publish\API\Repository\Values\User\User $user */ + $userService->deleteUser( $user ); + } + + /** + * Test for the deleteUser() method. + * + * @covers \eZ\Publish\Core\Repository\UserService::deleteUser + * @expectedException \Exception + */ + public function testDeleteUserWithRollback() + { + $repository = $this->getRepositoryMock(); + $userService = $this->getPartlyMockedUserService( array( "loadUser" ) ); + $contentService = $this->getMock( "eZ\\Publish\\API\\Repository\\ContentService" ); + + $user = $this->getMock( "eZ\\Publish\\API\\Repository\\Values\\User\\User" ); + $loadedUser = $this->getMock( "eZ\\Publish\\API\\Repository\\Values\\User\\User" ); + $versionInfo = $this->getMock( "eZ\\Publish\\API\\Repository\\Values\\Content\\VersionInfo" ); + $contentInfo = $this->getMock( "eZ\\Publish\\API\\Repository\\Values\\Content\\ContentInfo" ); + + $user->expects( $this->once() ) + ->method( "__get" ) + ->with( "id" ) + ->will( $this->returnValue( 42 ) ); + + $versionInfo->expects( $this->once() ) + ->method( "getContentInfo" ) + ->will( $this->returnValue( $contentInfo ) ); + + $loadedUser->expects( $this->once() ) + ->method( "getVersionInfo" ) + ->will( $this->returnValue( $versionInfo ) ); + + $userService->expects( $this->once() ) + ->method( "loadUser" ) + ->with( 42 ) + ->will( $this->returnValue( $loadedUser ) ); + + $repository->expects( $this->once() )->method( "beginTransaction" ); + + $contentService->expects( $this->once() ) + ->method( "deleteContent" ) + ->with( $contentInfo ) + ->will( $this->throwException( new \Exception ) ); + + $repository->expects( $this->once() ) + ->method( "getContentService" ) + ->will( $this->returnValue( $contentService ) ); + + $repository->expects( $this->once() )->method( "rollback" ); + + /** @var \eZ\Publish\API\Repository\Values\User\User $user */ + $userService->deleteUser( $user ); + } + + /** + * Returns the User service to test with $methods mocked + * + * Injected Repository comes from {@see getRepositoryMock()} and persistence handler from {@see getPersistenceMock()} + * + * @param string[] $methods + * + * @return \eZ\Publish\Core\Repository\UserService|\PHPUnit_Framework_MockObject_MockObject + */ + protected function getPartlyMockedUserService( array $methods = null ) + { + return $this->getMock( + "eZ\\Publish\\Core\\Repository\\UserService", + $methods, + array( + $this->getRepositoryMock(), + $this->getPersistenceMock()->userHandler() + ) + ); + } +} From 9bf3a3aa6d1038a48f0c24cfa8ecd13dcf93ddea Mon Sep 17 00:00:00 2001 From: Petar Spanja Date: Mon, 15 Apr 2013 15:27:37 +0200 Subject: [PATCH 4/8] EZP-20677: added clean up of URL aliases for deleted locations --- eZ/Publish/Core/Repository/ContentService.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/eZ/Publish/Core/Repository/ContentService.php b/eZ/Publish/Core/Repository/ContentService.php index 078be7773b8..696938efdde 100644 --- a/eZ/Publish/Core/Repository/ContentService.php +++ b/eZ/Publish/Core/Repository/ContentService.php @@ -913,7 +913,13 @@ public function deleteContent( ContentInfo $contentInfo ) $this->repository->beginTransaction(); try { + // Load Locations first as deleting Content also deletes belonging Locations + $spiLocations = $this->persistenceHandler->locationHandler()->loadLocationsByContent( $contentInfo->id ); $this->persistenceHandler->contentHandler()->deleteContent( $contentInfo->id ); + foreach ( $spiLocations as $spiLocation ) + { + $this->persistenceHandler->urlAliasHandler()->locationDeleted( $spiLocation->id ); + } $this->repository->commit(); } catch ( Exception $e ) From b405ddf635679aa0c800f515d0213b5cc75299a2 Mon Sep 17 00:00:00 2001 From: Petar Spanja Date: Mon, 15 Apr 2013 15:28:35 +0200 Subject: [PATCH 5/8] Added reloading given $contentInfo --- eZ/Publish/Core/Repository/ContentService.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/eZ/Publish/Core/Repository/ContentService.php b/eZ/Publish/Core/Repository/ContentService.php index 696938efdde..6ce60466da1 100644 --- a/eZ/Publish/Core/Repository/ContentService.php +++ b/eZ/Publish/Core/Repository/ContentService.php @@ -907,6 +907,8 @@ protected function publishUrlAliasesForContent( APIContent $content, $updatePath */ public function deleteContent( ContentInfo $contentInfo ) { + $contentInfo = $this->internalLoadContentInfo( $contentInfo->id ); + if ( !$this->repository->canUser( 'content', 'remove', $contentInfo ) ) throw new UnauthorizedException( 'content', 'remove' ); From eb5a8201976a2020e9fd4af3ecce7f0aaa6d3612 Mon Sep 17 00:00:00 2001 From: Petar Spanja Date: Mon, 15 Apr 2013 15:29:43 +0200 Subject: [PATCH 6/8] EZP-20677: tested deleteContent() with mocks, removed tests duplicated in integration suite --- .../Tests/Service/Integration/ContentBase.php | 46 ------ .../Tests/Service/Mock/ContentTest.php | 138 ++++++++++++++++++ 2 files changed, 138 insertions(+), 46 deletions(-) diff --git a/eZ/Publish/Core/Repository/Tests/Service/Integration/ContentBase.php b/eZ/Publish/Core/Repository/Tests/Service/Integration/ContentBase.php index e4f07c92249..e57df649d0c 100644 --- a/eZ/Publish/Core/Repository/Tests/Service/Integration/ContentBase.php +++ b/eZ/Publish/Core/Repository/Tests/Service/Integration/ContentBase.php @@ -2184,52 +2184,6 @@ public function testDeleteVersionThrowsUnauthorizedException() $contentService->deleteVersion( $draftContent->versionInfo ); } - /** - * Test for the deleteContent() method. - * - * @depends testLoadContent - * @covers \eZ\Publish\Core\Repository\ContentService::deleteContent - */ - public function testDeleteContent() - { - /* BEGIN: Use Case */ - $contentService = $this->repository->getContentService(); - - $contentInfo = $contentService->loadContentInfo( 4 ); - - $contentService->deleteContent( $contentInfo ); - /* END: Use Case */ - - try - { - $contentService->loadContent( $contentInfo->id ); - - $this->fail( "Content was not successfully deleted!" ); - } - catch ( NotFoundException $e ) - { - // Do nothing - } - } - - /** - * Test for the deleteContent() method. - * - * @covers \eZ\Publish\Core\Repository\ContentService::deleteContent - * @expectedException \eZ\Publish\API\Repository\Exceptions\UnauthorizedException - */ - public function testDeleteContentThrowsUnauthorizedException() - { - $contentService = $this->repository->getContentService(); - - $contentInfo = $contentService->loadContentInfo( 4 ); - - // Set anonymous as current user - $this->repository->setCurrentUser( $this->getStubbedUser( 10 ) ); - - $contentService->deleteContent( $contentInfo ); - } - /** * Test for the copyContent() method. * diff --git a/eZ/Publish/Core/Repository/Tests/Service/Mock/ContentTest.php b/eZ/Publish/Core/Repository/Tests/Service/Mock/ContentTest.php index 2b9b2ae4c66..ee63b71a16a 100644 --- a/eZ/Publish/Core/Repository/Tests/Service/Mock/ContentTest.php +++ b/eZ/Publish/Core/Repository/Tests/Service/Mock/ContentTest.php @@ -12,6 +12,7 @@ use eZ\Publish\Core\Repository\Tests\Service\Mock\Base as BaseServiceMockTest; use eZ\Publish\Core\Repository\Values\Content\VersionInfo; use eZ\Publish\API\Repository\Values\Content\ContentInfo; +use eZ\Publish\SPI\Persistence\Content\Location as SPILocation; /** * Mock test case for Content service @@ -101,6 +102,143 @@ public function testLoadContentByVersionInfo() ); } + /** + * Test for the deleteContent() method. + * + * @covers \eZ\Publish\Core\Repository\ContentService::deleteContent + * @expectedException \eZ\Publish\Core\Base\Exceptions\UnauthorizedException + */ + public function testDeleteContentThrowsUnauthorizedException() + { + $repository = $this->getRepositoryMock(); + $contentService = $this->getPartlyMockedContentService( array( "internalLoadContentInfo" ) ); + $contentInfo = $this->getMock( "eZ\\Publish\\API\\Repository\\Values\\Content\\ContentInfo" ); + + $contentInfo->expects( $this->any() ) + ->method( "__get" ) + ->with( "id" ) + ->will( $this->returnValue( 42 ) ); + + $contentService->expects( $this->once() ) + ->method( "internalLoadContentInfo" ) + ->with( 42 ) + ->will( $this->returnValue( $contentInfo ) ); + + $repository->expects( $this->once() ) + ->method( "canUser" ) + ->with( "content", "remove" ) + ->will( $this->returnValue( false ) ); + + /** @var \eZ\Publish\API\Repository\Values\Content\ContentInfo $contentInfo */ + $contentService->deleteContent( $contentInfo ); + } + + /** + * Test for the deleteContent() method. + * + * @covers \eZ\Publish\Core\Repository\ContentService::deleteContent + */ + public function testDeleteContent() + { + $repository = $this->getRepositoryMock(); + + $repository->expects( $this->once() ) + ->method( "canUser" ) + ->with( "content", "remove" ) + ->will( $this->returnValue( true ) ); + + $contentService = $this->getPartlyMockedContentService( array( "internalLoadContentInfo" ) ); + /** @var \PHPUnit_Framework_MockObject_MockObject $urlAliasHandler */ + $urlAliasHandler = $this->getPersistenceMock()->urlAliasHandler(); + /** @var \PHPUnit_Framework_MockObject_MockObject $locationHandler */ + $locationHandler = $this->getPersistenceMock()->locationHandler(); + /** @var \PHPUnit_Framework_MockObject_MockObject $contentHandler */ + $contentHandler = $this->getPersistenceMock()->contentHandler(); + + $contentInfo = $this->getMock( "eZ\\Publish\\API\\Repository\\Values\\Content\\ContentInfo" ); + + $contentService->expects( $this->once() ) + ->method( "internalLoadContentInfo" ) + ->with( 42 ) + ->will( $this->returnValue( $contentInfo ) ); + + $contentInfo->expects( $this->any() ) + ->method( "__get" ) + ->with( "id" ) + ->will( $this->returnValue( 42 ) ); + + $repository->expects( $this->once() )->method( "beginTransaction" ); + + $spiLocations = array( + new SPILocation( array( "id" => 1 ) ), + new SPILocation( array( "id" => 2 ) ), + ); + $locationHandler->expects( $this->once() ) + ->method( "loadLocationsByContent" ) + ->with( 42 ) + ->will( $this->returnValue( $spiLocations ) ); + + $contentHandler->expects( $this->once() ) + ->method( "deleteContent" ) + ->with( 42 ); + + foreach ( $spiLocations as $index => $spiLocation ) + { + $urlAliasHandler->expects( $this->at( $index ) ) + ->method( "locationDeleted" ) + ->with( $spiLocation->id ); + } + + $repository->expects( $this->once() )->method( "commit" ); + + /** @var \eZ\Publish\API\Repository\Values\Content\ContentInfo $contentInfo */ + $contentService->deleteContent( $contentInfo ); + } + + /** + * Test for the deleteContent() method. + * + * @covers \eZ\Publish\Core\Repository\ContentService::deleteContent + * @expectedException \Exception + */ + public function testDeleteContentWithRollback() + { + $repository = $this->getRepositoryMock(); + + $repository->expects( $this->once() ) + ->method( "canUser" ) + ->with( "content", "remove" ) + ->will( $this->returnValue( true ) ); + + $contentService = $this->getPartlyMockedContentService( array( "internalLoadContentInfo" ) ); + /** @var \PHPUnit_Framework_MockObject_MockObject $locationHandler */ + $locationHandler = $this->getPersistenceMock()->locationHandler(); + + $contentInfo = $this->getMock( "eZ\\Publish\\API\\Repository\\Values\\Content\\ContentInfo" ); + + $contentService->expects( $this->once() ) + ->method( "internalLoadContentInfo" ) + ->with( 42 ) + ->will( $this->returnValue( $contentInfo ) ); + + $contentInfo->expects( $this->any() ) + ->method( "__get" ) + ->with( "id" ) + ->will( $this->returnValue( 42 ) ); + + $repository->expects( $this->once() )->method( "beginTransaction" ); + + $locationHandler->expects( $this->once() ) + ->method( "loadLocationsByContent" ) + ->with( 42 ) + ->will( $this->throwException( new \Exception ) ); + + $repository->expects( $this->once() )->method( "rollback" ); + + /** @var \eZ\Publish\API\Repository\Values\Content\ContentInfo $contentInfo */ + $contentService->deleteContent( $contentInfo ); + } + /** * Returns the content service to test with $methods mocked * From 917aa32072d8939b13393566b6b16e94164a2e09 Mon Sep 17 00:00:00 2001 From: Petar Spanja Date: Mon, 15 Apr 2013 15:35:01 +0200 Subject: [PATCH 7/8] Fixed FQN in phpdoc, removed extra empty line --- eZ/Publish/Core/Repository/Tests/Service/Mock/UrlAliasTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/eZ/Publish/Core/Repository/Tests/Service/Mock/UrlAliasTest.php b/eZ/Publish/Core/Repository/Tests/Service/Mock/UrlAliasTest.php index 0d32217939b..054284a4c1a 100644 --- a/eZ/Publish/Core/Repository/Tests/Service/Mock/UrlAliasTest.php +++ b/eZ/Publish/Core/Repository/Tests/Service/Mock/UrlAliasTest.php @@ -1,6 +1,6 @@ Date: Mon, 15 Apr 2013 15:36:22 +0200 Subject: [PATCH 8/8] EZP-20677: added clearing User Content SPI cache after deleting user data --- .../Core/Persistence/Cache/Tests/UserHandlerTest.php | 8 +++++++- eZ/Publish/Core/Persistence/Cache/UserHandler.php | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/eZ/Publish/Core/Persistence/Cache/Tests/UserHandlerTest.php b/eZ/Publish/Core/Persistence/Cache/Tests/UserHandlerTest.php index eb0a4974435..e04e894e5f2 100644 --- a/eZ/Publish/Core/Persistence/Cache/Tests/UserHandlerTest.php +++ b/eZ/Publish/Core/Persistence/Cache/Tests/UserHandlerTest.php @@ -106,12 +106,18 @@ public function testDelete() $this->cacheMock ->expects( $this->at( 0 ) ) ->method( 'clear' ) - ->with( 'user', 'role', 'assignments', 'byGroup', 14 ) + ->with( 'content', 14 ) ->will( $this->returnValue( true ) ); $this->cacheMock ->expects( $this->at( 1 ) ) ->method( 'clear' ) + ->with( 'user', 'role', 'assignments', 'byGroup', 14 ) + ->will( $this->returnValue( true ) ); + + $this->cacheMock + ->expects( $this->at( 2 ) ) + ->method( 'clear' ) ->with( 'user', 'role', 'assignments', 'byGroup', 'inherited', 14 ) ->will( $this->returnValue( true ) ); diff --git a/eZ/Publish/Core/Persistence/Cache/UserHandler.php b/eZ/Publish/Core/Persistence/Cache/UserHandler.php index 290886d3dd0..2f5e2188bf4 100644 --- a/eZ/Publish/Core/Persistence/Cache/UserHandler.php +++ b/eZ/Publish/Core/Persistence/Cache/UserHandler.php @@ -68,6 +68,7 @@ public function delete( $userId ) $return = $this->persistenceFactory->getUserHandler()->delete( $userId ); // user id == content id == group id + $this->cache->clear( 'content', $userId ); $this->cache->clear( 'user', 'role', 'assignments', 'byGroup', $userId ); $this->cache->clear( 'user', 'role', 'assignments', 'byGroup', 'inherited', $userId );