From c224ec2f23ac9dbd8f4bb1d52b3918fee0418a84 Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Thu, 21 May 2020 15:17:31 +0200 Subject: [PATCH 1/9] Add deck:transfer-ownership command Signed-off-by: Sergey Shliakhov --- appinfo/info.xml | 1 + lib/Command/TransferOwnership.php | 63 +++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 lib/Command/TransferOwnership.php diff --git a/appinfo/info.xml b/appinfo/info.xml index 05755a365..a40f79e99 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -50,6 +50,7 @@ OCA\Deck\Command\UserExport + OCA\Deck\Command\TransferOwnership diff --git a/lib/Command/TransferOwnership.php b/lib/Command/TransferOwnership.php new file mode 100644 index 000000000..e7b242efb --- /dev/null +++ b/lib/Command/TransferOwnership.php @@ -0,0 +1,63 @@ +setName('deck:transfer-ownership') + ->setDescription('Change owner of deck entities') + ->addArgument( + 'owner', + InputArgument::REQUIRED, + 'Owner uid' + ) + ->addArgument( + 'newOwner', + InputArgument::REQUIRED, + 'New owner uid' + ); + } + + protected function execute(InputInterface $input, OutputInterface $output) { + $owner = $input->getArgument('owner'); + $newOwner = $input->getArgument('newOwner'); + $db = \OC::$server->getDatabaseConnection(); + + $output->writeln("Transfer deck entities from $owner to $newOwner"); + $params = [ + 'owner' => $owner, + 'newOwner' => $newOwner + ]; + + $output->writeln('update oc_deck_assigned_users'); + $stmt = $db->prepare('UPDATE `oc_deck_assigned_users` SET `participant` = :newOwner WHERE `participant` = :owner'); + $stmt->execute($params); + + $output->writeln('update oc_deck_attachment'); + $stmt = $db->prepare('UPDATE `oc_deck_attachment` SET `created_by` = :newOwner WHERE `created_by` = :owner'); + $stmt->execute($params); + + $output->writeln('update oc_deck_boards'); + $stmt = $db->prepare('UPDATE `oc_deck_boards` SET `owner` = :newOwner WHERE `owner` = :owner'); + $stmt->execute($params); + + $output->writeln('update oc_deck_board_acl'); + $stmt = $db->prepare('UPDATE `oc_deck_board_acl` SET `participant` = :newOwner WHERE `participant` = :owner'); + $stmt->execute($params); + + $output->writeln('update oc_deck_cards'); + $stmt = $db->prepare('UPDATE `oc_deck_cards` SET `owner` = :newOwner WHERE `owner` = :owner'); + $stmt->execute($params); + + $output->writeln("Transfer deck entities from $owner to $newOwner completed"); + } + +} From d1d864145c3d90f79fcf11608a2cf6eead301717 Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Mon, 25 May 2020 09:46:57 +0200 Subject: [PATCH 2/9] Update docs Signed-off-by: Sergey Shliakhov --- docs/User_documentation_en.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/User_documentation_en.md b/docs/User_documentation_en.md index 75b0e57a5..40f137d5c 100644 --- a/docs/User_documentation_en.md +++ b/docs/User_documentation_en.md @@ -14,6 +14,7 @@ Overall, Deck is easy to use. You can create boards, add users, share the Deck, 3. [Handle cards options](#3-handle-cards-options) 4. [Archive old tasks](#4-archive-old-tasks) 5. [Manage your board](#5-manage-your-board) +6. [New owner for the deck entities](#6-new-owner-for-the-deck-entities) ### 1. Create my first board In this example, we're going to create a board and share it with an other nextcloud user. @@ -67,3 +68,6 @@ The **sharing tab** allows you to add users or even groups to your boards. **Deleted objects** allows you to return previously deleted stacks or cards. The **Timeline** allows you to see everything that happened in your boards. Everything! +### 6. New owner for the deck entities +You can transfer ownership of boards, cards, etc to a new user, using `occ` command `deck:transfer-ownership` +`$ php occ deck:transfer-ownership owner newOwner` From bdc50a7cb3f380ff1d915733675eb993ab742405 Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Thu, 4 Jun 2020 15:41:32 +0200 Subject: [PATCH 3/9] Move code to BoardService Signed-off-by: Sergey Shliakhov --- lib/Command/TransferOwnership.php | 52 +++++++++++-------------------- lib/Db/AclMapper.php | 17 ++++++++++ lib/Db/AssignedUsersMapper.php | 16 ++++++++++ lib/Db/BoardMapper.php | 16 ++++++++++ lib/Db/CardMapper.php | 16 ++++++++++ lib/Service/BoardService.php | 17 ++++++++++ 6 files changed, 101 insertions(+), 33 deletions(-) diff --git a/lib/Command/TransferOwnership.php b/lib/Command/TransferOwnership.php index e7b242efb..e2a7c67ce 100644 --- a/lib/Command/TransferOwnership.php +++ b/lib/Command/TransferOwnership.php @@ -1,8 +1,8 @@ boardService = $boardService; + } + protected function configure() { $this ->setName('deck:transfer-ownership') @@ -19,45 +28,22 @@ protected function configure() { InputArgument::REQUIRED, 'Owner uid' ) - ->addArgument( - 'newOwner', - InputArgument::REQUIRED, - 'New owner uid' - ); + ->addArgument( + 'newOwner', + InputArgument::REQUIRED, + 'New owner uid' + ); } protected function execute(InputInterface $input, OutputInterface $output) { $owner = $input->getArgument('owner'); $newOwner = $input->getArgument('newOwner'); - $db = \OC::$server->getDatabaseConnection(); - $output->writeln("Transfer deck entities from $owner to $newOwner"); - $params = [ - 'owner' => $owner, - 'newOwner' => $newOwner - ]; + $output->writeln("Transfer deck entities from $owner to $newOwner"); - $output->writeln('update oc_deck_assigned_users'); - $stmt = $db->prepare('UPDATE `oc_deck_assigned_users` SET `participant` = :newOwner WHERE `participant` = :owner'); - $stmt->execute($params); + $this->boardService->transferOwnership($owner, $newOwner); - $output->writeln('update oc_deck_attachment'); - $stmt = $db->prepare('UPDATE `oc_deck_attachment` SET `created_by` = :newOwner WHERE `created_by` = :owner'); - $stmt->execute($params); - - $output->writeln('update oc_deck_boards'); - $stmt = $db->prepare('UPDATE `oc_deck_boards` SET `owner` = :newOwner WHERE `owner` = :owner'); - $stmt->execute($params); - - $output->writeln('update oc_deck_board_acl'); - $stmt = $db->prepare('UPDATE `oc_deck_board_acl` SET `participant` = :newOwner WHERE `participant` = :owner'); - $stmt->execute($params); - - $output->writeln('update oc_deck_cards'); - $stmt = $db->prepare('UPDATE `oc_deck_cards` SET `owner` = :newOwner WHERE `owner` = :owner'); - $stmt->execute($params); - - $output->writeln("Transfer deck entities from $owner to $newOwner completed"); - } + $output->writeln("Transfer deck entities from $owner to $newOwner completed"); + } } diff --git a/lib/Db/AclMapper.php b/lib/Db/AclMapper.php index e0ed3c838..848678250 100644 --- a/lib/Db/AclMapper.php +++ b/lib/Db/AclMapper.php @@ -51,4 +51,21 @@ public function findByParticipant($type, $participant) { $sql = 'SELECT * from *PREFIX*deck_board_acl WHERE type = ? AND participant = ?'; return $this->findEntities($sql, [$type, $participant]); } + + /** + * @param $ownerId + * @param $newOwnerId + * @return void + */ + public function transferOwnership($ownerId, $newOwnerId) + { + $params = [ + 'owner' => $ownerId, + 'newOwner' => $newOwnerId, + 'type' => Acl::PERMISSION_TYPE_USER + ]; + $sql = "UPDATE `{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner AND `type` = :type"; + $stmt = $this->execute($sql, $params); + $stmt->closeCursor(); + } } diff --git a/lib/Db/AssignedUsersMapper.php b/lib/Db/AssignedUsersMapper.php index bd18f46cd..49abb972b 100644 --- a/lib/Db/AssignedUsersMapper.php +++ b/lib/Db/AssignedUsersMapper.php @@ -114,4 +114,20 @@ private function getOrigin(AssignedUsers $assignment) { } return null; } + + /** + * @param $ownerId + * @param $newOwnerId + * @return void + */ + public function transferOwnership($ownerId, $newOwnerId) + { + $params = [ + 'owner' => $ownerId, + 'newOwner' => $newOwnerId + ]; + $sql = "UPDATE `{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner"; + $stmt = $this->execute($sql, $params); + $stmt->closeCursor(); + } } diff --git a/lib/Db/BoardMapper.php b/lib/Db/BoardMapper.php index 43e070a6b..5e1f06c61 100644 --- a/lib/Db/BoardMapper.php +++ b/lib/Db/BoardMapper.php @@ -261,4 +261,20 @@ public function mapOwner(Board &$board) { return null; }); } + + /** + * @param $ownerId + * @param $newOwnerId + * @return void + */ + public function transferOwnership($ownerId, $newOwnerId) + { + $params = [ + 'owner' => $ownerId, + 'newOwner' => $newOwnerId + ]; + $sql = "UPDATE `{$this->tableName}` SET `owner` = :newOwner WHERE `owner` = :owner"; + $stmt = $this->execute($sql, $params); + $stmt->closeCursor(); + } } diff --git a/lib/Db/CardMapper.php b/lib/Db/CardMapper.php index ad2c97724..dcb524bac 100644 --- a/lib/Db/CardMapper.php +++ b/lib/Db/CardMapper.php @@ -205,4 +205,20 @@ public function mapOwner(Card &$card) { return null; }); } + + /** + * @param $ownerId + * @param $newOwnerId + * @return void + */ + public function transferOwnership($ownerId, $newOwnerId) + { + $params = [ + 'owner' => $ownerId, + 'newOwner' => $newOwnerId + ]; + $sql = "UPDATE `{$this->tableName}` SET `owner` = :newOwner WHERE `owner` = :owner"; + $stmt = $this->execute($sql, $params); + $stmt->closeCursor(); + } } diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index 868164b91..598c661ef 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -29,6 +29,7 @@ use OCA\Deck\Db\Acl; use OCA\Deck\Db\AclMapper; use OCA\Deck\Db\AssignedUsersMapper; +use OCA\Deck\Db\CardMapper; use OCA\Deck\Db\ChangeHelper; use OCA\Deck\Db\IPermissionMapper; use OCA\Deck\Db\Label; @@ -63,6 +64,7 @@ class BoardService { /** @var EventDispatcherInterface */ private $eventDispatcher; private $changeHelper; + private $cardMapper; public function __construct( BoardMapper $boardMapper, @@ -73,6 +75,7 @@ public function __construct( PermissionService $permissionService, NotificationHelper $notificationHelper, AssignedUsersMapper $assignedUsersMapper, + CardMapper $cardMapper, IUserManager $userManager, IGroupManager $groupManager, ActivityManager $activityManager, @@ -94,6 +97,7 @@ public function __construct( $this->eventDispatcher = $eventDispatcher; $this->changeHelper = $changeHelper; $this->userId = $userId; + $this->cardMapper = $cardMapper; } /** @@ -669,6 +673,19 @@ public function clone($id, $userId) { return $newBoard; } + /** + * @param $ownerId + * @param $newOwnerId + * @return void + */ + public function transferOwnership($owner, $newOwner) + { + $this->boardMapper->transferOwnership($owner, $newOwner); + $this->assignedUsersMapper->transferOwnership($owner, $newOwner); + $this->aclMapper->transferOwnership($owner, $newOwner); + $this->cardMapper->transferOwnership($owner, $newOwner); + } + private function enrichWithStacks($board, $since = -1) { $stacks = $this->stackMapper->findAll($board->getId(), null, null, $since); From 3b6b5ee83d73875c72cc720d887544c093a97c95 Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Sun, 7 Jun 2020 16:11:36 +0200 Subject: [PATCH 4/9] Add tests Signed-off-by: Sergey Shliakhov --- .../database/TransferOwnershipTest.php | 144 ++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 tests/integration/database/TransferOwnershipTest.php diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php new file mode 100644 index 000000000..132a79202 --- /dev/null +++ b/tests/integration/database/TransferOwnershipTest.php @@ -0,0 +1,144 @@ +getUserManager()->registerBackend($backend); + $backend->createUser(self::TEST_OWNER, self::TEST_OWNER); + $backend->createUser(self::TEST_NEW_OWNER, self::TEST_NEW_OWNER); + // create group + $groupBackend = new \Test\Util\Group\Dummy(); + $groupBackend->createGroup(self::TEST_GROUP); + $groupBackend->addToGroup(self::TEST_OWNER, self::TEST_GROUP); + \OC::$server->getGroupManager()->addBackend($groupBackend); + } + + public function setUp(): void { + parent::setUp(); + \OC::$server->getUserSession()->login(self::TEST_OWNER, self::TEST_OWNER); + $this->boardService = \OC::$server->query(BoardService::class); + $this->stackService = \OC::$server->query(StackService::class); + $this->cardService = \OC::$server->query(CardService::class); + $this->assignmentService = \OC::$server->query(AssignmentService::class); + $this->assignedUsersMapper = \OC::$server->query(AssignedUsersMapper::class); + $this->createBoardWithExampleData(); + } + + public function createBoardWithExampleData() { + $stacks = []; + $board = $this->boardService->create('Test', self::TEST_OWNER, '000000'); + $id = $board->getId(); + $this->boardService->addAcl($id, Acl::PERMISSION_TYPE_USER, self::TEST_OWNER, true, true, true); + $this->boardService->addAcl($id, Acl::PERMISSION_TYPE_GROUP, self::TEST_GROUP, true, true, true); + $stacks[] = $this->stackService->create('Stack A', $id, 1); + $stacks[] = $this->stackService->create('Stack B', $id, 1); + $stacks[] = $this->stackService->create('Stack C', $id, 1); + $cards[] = $this->cardService->create('Card 1', $stacks[0]->getId(), 'text', 0, self::TEST_OWNER); + $cards[] = $this->cardService->create('Card 2', $stacks[0]->getId(), 'text', 0, self::TEST_OWNER); + $this->assignmentService->assignUser($cards[0]->getId(), self::TEST_OWNER); + $this->board = $board; + $this->cards = $cards; + $this->stacks = $stacks; + } + + /** + * @covers ::transferOwnership + */ + public function testTransferBoardOwnership() + { + $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $board = $this->boardService->find($this->board->getId()); + $boardOwner = $board->getOwner(); + $this->assertEquals(self::TEST_NEW_OWNER, $boardOwner); + } + + /** + * @covers ::transferOwnership + */ + public function testTransferACLOwnership() + { + $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $board = $this->boardService->find($this->board->getId()); + $acl = $board->getAcl(); + $isTargetInAcl = (bool)array_filter($acl, function ($item) { + return $item->getParticipant() === self::TEST_NEW_OWNER && $item->getType() === Acl::PERMISSION_TYPE_USER; + }); + $this->assertTrue($isTargetInAcl); + } + + /** + * @covers ::transferOwnership + */ + public function testNoTransferAclOwnershipIfGroupType() + { + $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $board = $this->boardService->find($this->board->getId()); + $acl = $board->getAcl(); + $isGroupInAcl = (bool)array_filter($acl, function ($item) { + return $item->getParticipant() === self::TEST_GROUP && $item->getType() === Acl::PERMISSION_TYPE_GROUP; + }); + $this->assertTrue($isGroupInAcl); + } + /** + * @covers ::transferOwnership + */ + public function testTransferCardOwnership() + { + $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $card = $this->cardService->find($this->cards[0]->getId()); + $cardOwner = $card->getOwner(); + $this->assertEquals(self::TEST_NEW_OWNER, $cardOwner); + } + + /** + * @covers ::transferOwnership + */ + public function testReassignCardToNewOwner() + { + $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $assignedUsers = $this->assignedUsersMapper->find($this->cards[0]->getId()); + $participantsUIDs = []; + foreach ($assignedUsers as $user) { + $participantsUIDs[] = $user->getParticipant(); + } + $this->assertContains(self::TEST_NEW_OWNER, $participantsUIDs); + $this->assertNotContains(self::TEST_OWNER, $participantsUIDs); + } + + public function tearDown(): void { + $this->boardService->deleteForce($this->board->getId()); + parent::tearDown(); + } +} From d8ceb8cf6767c6c89d148e98fb951c3ba4c9335c Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Sun, 7 Jun 2020 16:18:38 +0200 Subject: [PATCH 5/9] Fix code style Signed-off-by: Sergey Shliakhov --- lib/Command/TransferOwnership.php | 5 +- lib/Db/AclMapper.php | 3 +- lib/Db/AssignedUsersMapper.php | 3 +- lib/Db/BoardMapper.php | 29 ++-- lib/Db/CardMapper.php | 3 +- lib/Service/BoardService.php | 3 +- .../database/TransferOwnershipTest.php | 133 +++++++++--------- 7 files changed, 83 insertions(+), 96 deletions(-) diff --git a/lib/Command/TransferOwnership.php b/lib/Command/TransferOwnership.php index e2a7c67ce..c3175477e 100644 --- a/lib/Command/TransferOwnership.php +++ b/lib/Command/TransferOwnership.php @@ -9,11 +9,9 @@ use Symfony\Component\Console\Output\OutputInterface; final class TransferOwnership extends Command { - protected $boardService; - public function __construct(BoardService $boardService) - { + public function __construct(BoardService $boardService) { parent::__construct(); $this->boardService = $boardService; @@ -45,5 +43,4 @@ protected function execute(InputInterface $input, OutputInterface $output) { $output->writeln("Transfer deck entities from $owner to $newOwner completed"); } - } diff --git a/lib/Db/AclMapper.php b/lib/Db/AclMapper.php index 848678250..f74af1142 100644 --- a/lib/Db/AclMapper.php +++ b/lib/Db/AclMapper.php @@ -57,8 +57,7 @@ public function findByParticipant($type, $participant) { * @param $newOwnerId * @return void */ - public function transferOwnership($ownerId, $newOwnerId) - { + public function transferOwnership($ownerId, $newOwnerId) { $params = [ 'owner' => $ownerId, 'newOwner' => $newOwnerId, diff --git a/lib/Db/AssignedUsersMapper.php b/lib/Db/AssignedUsersMapper.php index 49abb972b..b8d4c76f0 100644 --- a/lib/Db/AssignedUsersMapper.php +++ b/lib/Db/AssignedUsersMapper.php @@ -120,8 +120,7 @@ private function getOrigin(AssignedUsers $assignment) { * @param $newOwnerId * @return void */ - public function transferOwnership($ownerId, $newOwnerId) - { + public function transferOwnership($ownerId, $newOwnerId) { $params = [ 'owner' => $ownerId, 'newOwner' => $newOwnerId diff --git a/lib/Db/BoardMapper.php b/lib/Db/BoardMapper.php index 5e1f06c61..7d07de92b 100644 --- a/lib/Db/BoardMapper.php +++ b/lib/Db/BoardMapper.php @@ -262,19 +262,18 @@ public function mapOwner(Board &$board) { }); } - /** - * @param $ownerId - * @param $newOwnerId - * @return void - */ - public function transferOwnership($ownerId, $newOwnerId) - { - $params = [ - 'owner' => $ownerId, - 'newOwner' => $newOwnerId - ]; - $sql = "UPDATE `{$this->tableName}` SET `owner` = :newOwner WHERE `owner` = :owner"; - $stmt = $this->execute($sql, $params); - $stmt->closeCursor(); - } + /** + * @param $ownerId + * @param $newOwnerId + * @return void + */ + public function transferOwnership($ownerId, $newOwnerId) { + $params = [ + 'owner' => $ownerId, + 'newOwner' => $newOwnerId + ]; + $sql = "UPDATE `{$this->tableName}` SET `owner` = :newOwner WHERE `owner` = :owner"; + $stmt = $this->execute($sql, $params); + $stmt->closeCursor(); + } } diff --git a/lib/Db/CardMapper.php b/lib/Db/CardMapper.php index dcb524bac..2ec63dcc1 100644 --- a/lib/Db/CardMapper.php +++ b/lib/Db/CardMapper.php @@ -211,8 +211,7 @@ public function mapOwner(Card &$card) { * @param $newOwnerId * @return void */ - public function transferOwnership($ownerId, $newOwnerId) - { + public function transferOwnership($ownerId, $newOwnerId) { $params = [ 'owner' => $ownerId, 'newOwner' => $newOwnerId diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index 598c661ef..a4c5adfe8 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -678,8 +678,7 @@ public function clone($id, $userId) { * @param $newOwnerId * @return void */ - public function transferOwnership($owner, $newOwner) - { + public function transferOwnership($owner, $newOwner) { $this->boardMapper->transferOwnership($owner, $newOwner); $this->assignedUsersMapper->transferOwnership($owner, $newOwner); $this->aclMapper->transferOwnership($owner, $newOwner); diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php index 132a79202..0a8132532 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -25,12 +25,12 @@ class AssignedUsersMapperTest extends \Test\TestCase { protected $assignedUsersMapper; /** @var AssignmentService */ private $assignmentService; - /** @var Board */ - private $board; - private $cards; - private $stacks; + /** @var Board */ + private $board; + private $cards; + private $stacks; - public static function setUpBeforeClass(): void { + public static function setUpBeforeClass(): void { parent::setUpBeforeClass(); $backend = new \Test\Util\User\Dummy(); @@ -60,82 +60,77 @@ public function createBoardWithExampleData() { $stacks = []; $board = $this->boardService->create('Test', self::TEST_OWNER, '000000'); $id = $board->getId(); - $this->boardService->addAcl($id, Acl::PERMISSION_TYPE_USER, self::TEST_OWNER, true, true, true); - $this->boardService->addAcl($id, Acl::PERMISSION_TYPE_GROUP, self::TEST_GROUP, true, true, true); + $this->boardService->addAcl($id, Acl::PERMISSION_TYPE_USER, self::TEST_OWNER, true, true, true); + $this->boardService->addAcl($id, Acl::PERMISSION_TYPE_GROUP, self::TEST_GROUP, true, true, true); $stacks[] = $this->stackService->create('Stack A', $id, 1); $stacks[] = $this->stackService->create('Stack B', $id, 1); $stacks[] = $this->stackService->create('Stack C', $id, 1); $cards[] = $this->cardService->create('Card 1', $stacks[0]->getId(), 'text', 0, self::TEST_OWNER); $cards[] = $this->cardService->create('Card 2', $stacks[0]->getId(), 'text', 0, self::TEST_OWNER); - $this->assignmentService->assignUser($cards[0]->getId(), self::TEST_OWNER); - $this->board = $board; + $this->assignmentService->assignUser($cards[0]->getId(), self::TEST_OWNER); + $this->board = $board; $this->cards = $cards; $this->stacks = $stacks; } - /** - * @covers ::transferOwnership - */ - public function testTransferBoardOwnership() - { - $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); - $board = $this->boardService->find($this->board->getId()); - $boardOwner = $board->getOwner(); - $this->assertEquals(self::TEST_NEW_OWNER, $boardOwner); - } + /** + * @covers ::transferOwnership + */ + public function testTransferBoardOwnership() { + $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $board = $this->boardService->find($this->board->getId()); + $boardOwner = $board->getOwner(); + $this->assertEquals(self::TEST_NEW_OWNER, $boardOwner); + } - /** - * @covers ::transferOwnership - */ - public function testTransferACLOwnership() - { - $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); - $board = $this->boardService->find($this->board->getId()); - $acl = $board->getAcl(); - $isTargetInAcl = (bool)array_filter($acl, function ($item) { - return $item->getParticipant() === self::TEST_NEW_OWNER && $item->getType() === Acl::PERMISSION_TYPE_USER; - }); - $this->assertTrue($isTargetInAcl); - } + /** + * @covers ::transferOwnership + */ + public function testTransferACLOwnership() { + $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $board = $this->boardService->find($this->board->getId()); + $acl = $board->getAcl(); + $isTargetInAcl = (bool)array_filter($acl, function ($item) { + return $item->getParticipant() === self::TEST_NEW_OWNER && $item->getType() === Acl::PERMISSION_TYPE_USER; + }); + $this->assertTrue($isTargetInAcl); + } - /** - * @covers ::transferOwnership - */ - public function testNoTransferAclOwnershipIfGroupType() - { - $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); - $board = $this->boardService->find($this->board->getId()); - $acl = $board->getAcl(); - $isGroupInAcl = (bool)array_filter($acl, function ($item) { - return $item->getParticipant() === self::TEST_GROUP && $item->getType() === Acl::PERMISSION_TYPE_GROUP; - }); - $this->assertTrue($isGroupInAcl); - } - /** - * @covers ::transferOwnership - */ - public function testTransferCardOwnership() - { - $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); - $card = $this->cardService->find($this->cards[0]->getId()); - $cardOwner = $card->getOwner(); - $this->assertEquals(self::TEST_NEW_OWNER, $cardOwner); - } + /** + * @covers ::transferOwnership + */ + public function testNoTransferAclOwnershipIfGroupType() { + $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $board = $this->boardService->find($this->board->getId()); + $acl = $board->getAcl(); + $isGroupInAcl = (bool)array_filter($acl, function ($item) { + return $item->getParticipant() === self::TEST_GROUP && $item->getType() === Acl::PERMISSION_TYPE_GROUP; + }); + $this->assertTrue($isGroupInAcl); + } + /** + * @covers ::transferOwnership + */ + public function testTransferCardOwnership() { + $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $card = $this->cardService->find($this->cards[0]->getId()); + $cardOwner = $card->getOwner(); + $this->assertEquals(self::TEST_NEW_OWNER, $cardOwner); + } - /** - * @covers ::transferOwnership - */ - public function testReassignCardToNewOwner() - { - $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); - $assignedUsers = $this->assignedUsersMapper->find($this->cards[0]->getId()); - $participantsUIDs = []; - foreach ($assignedUsers as $user) { - $participantsUIDs[] = $user->getParticipant(); - } - $this->assertContains(self::TEST_NEW_OWNER, $participantsUIDs); - $this->assertNotContains(self::TEST_OWNER, $participantsUIDs); - } + /** + * @covers ::transferOwnership + */ + public function testReassignCardToNewOwner() { + $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $assignedUsers = $this->assignedUsersMapper->find($this->cards[0]->getId()); + $participantsUIDs = []; + foreach ($assignedUsers as $user) { + $participantsUIDs[] = $user->getParticipant(); + } + $this->assertContains(self::TEST_NEW_OWNER, $participantsUIDs); + $this->assertNotContains(self::TEST_OWNER, $participantsUIDs); + } public function tearDown(): void { $this->boardService->deleteForce($this->board->getId()); From 4d48b1dda17c6df3c73697d9755b6e60882b6285 Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Mon, 8 Jun 2020 14:06:50 +0200 Subject: [PATCH 6/9] Fix wrong class name Signed-off-by: Sergey Shliakhov --- .../database/TransferOwnershipTest.php | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php index 0a8132532..e49e409b4 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -10,7 +10,7 @@ * @group DB * @coversDefaultClass OCA\Deck\Service\BoardService */ -class AssignedUsersMapperTest extends \Test\TestCase { +class TransferOwnershipTest extends \Test\TestCase { private const TEST_OWNER = 'test-share-user1'; private const TEST_NEW_OWNER = 'target'; private const TEST_GROUP = 'test-share-user1'; @@ -132,6 +132,21 @@ public function testReassignCardToNewOwner() { $this->assertNotContains(self::TEST_OWNER, $participantsUIDs); } + /** + * @covers ::transferOwnership + */ + public function testReassignCardToNewParticipantOnlyIfParticipantHasUserType() { + $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $this->assignmentService->ass($cards[0]->getId(), self::TEST_OWNER); + $assignedUsers = $this->assignedUsersMapper->find($this->cards[0]->getId()); + $participantsUIDs = []; + foreach ($assignedUsers as $user) { + $participantsUIDs[] = $user->getParticipant(); + } + $this->assertContains(self::TEST_NEW_OWNER, $participantsUIDs); + $this->assertNotContains(self::TEST_OWNER, $participantsUIDs); + } + public function tearDown(): void { $this->boardService->deleteForce($this->board->getId()); parent::tearDown(); From caac805e75839dc4fbe8357bea2d7c0517cac273 Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Tue, 9 Jun 2020 05:21:24 +0200 Subject: [PATCH 7/9] Check type before transfer card participants ownership Signed-off-by: Sergey Shliakhov temp --- Makefile | 3 +-- lib/Db/AssignedUsersMapper.php | 5 +++-- tests/integration/config/behat.yml | 4 ++-- .../database/TransferOwnershipTest.php | 21 ++++++++++++++++++- tests/integration/run.sh | 2 +- tests/unit/Service/BoardServiceTest.php | 5 +++++ 6 files changed, 32 insertions(+), 8 deletions(-) diff --git a/Makefile b/Makefile index 54930c6b6..8d428a7d3 100644 --- a/Makefile +++ b/Makefile @@ -50,8 +50,7 @@ ifeq (, $(shell which phpunit 2> /dev/null)) php $(build_tools_directory)/phpunit.phar -c tests/phpunit.xml --coverage-clover build/php-unit.coverage.xml php $(build_tools_directory)/phpunit.phar -c tests/phpunit.integration.xml --coverage-clover build/php-integration.coverage.xml else - phpunit -c tests/phpunit.xml --coverage-clover build/php-unit.coverage.xml - phpunit -c tests/phpunit.integration.xml --coverage-clover build/php-integration.coverage.xml + phpunit -c tests/phpunit.integration.xml --testsuite=integration-database --coverage-clover build/php-integration.coverage.xml endif test-integration: diff --git a/lib/Db/AssignedUsersMapper.php b/lib/Db/AssignedUsersMapper.php index b8d4c76f0..136ab760e 100644 --- a/lib/Db/AssignedUsersMapper.php +++ b/lib/Db/AssignedUsersMapper.php @@ -123,9 +123,10 @@ private function getOrigin(AssignedUsers $assignment) { public function transferOwnership($ownerId, $newOwnerId) { $params = [ 'owner' => $ownerId, - 'newOwner' => $newOwnerId + 'newOwner' => $newOwnerId, + 'type' => AssignedUsers::TYPE_USER ]; - $sql = "UPDATE `{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner"; + $sql = "UPDATE `{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner AND `type`= :type"; $stmt = $this->execute($sql, $params); $stmt->closeCursor(); } diff --git a/tests/integration/config/behat.yml b/tests/integration/config/behat.yml index f557adde8..5006985ec 100644 --- a/tests/integration/config/behat.yml +++ b/tests/integration/config/behat.yml @@ -5,8 +5,8 @@ default: - '%paths.base%/../features/' contexts: - FeatureContext: - baseUrl: http://localhost:8080/index.php/ocs/ + baseUrl: http://localhost:9090/index.php/ocs/ admin: - admin - admin - regular_user_password: 123456 \ No newline at end of file + regular_user_password: 123456 diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php index e49e409b4..aff664640 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -13,6 +13,8 @@ class TransferOwnershipTest extends \Test\TestCase { private const TEST_OWNER = 'test-share-user1'; private const TEST_NEW_OWNER = 'target'; + private const TEST_NEW_OWNER_PARTICIPANT = 'target-participant'; + private const TEST_NEW_OWNER_IN_ACL = 'target-in-acl'; private const TEST_GROUP = 'test-share-user1'; /** @var BoardService */ @@ -38,6 +40,7 @@ public static function setUpBeforeClass(): void { \OC::$server->getUserManager()->registerBackend($backend); $backend->createUser(self::TEST_OWNER, self::TEST_OWNER); $backend->createUser(self::TEST_NEW_OWNER, self::TEST_NEW_OWNER); + $backend->createUser(self::TEST_NEW_OWNER_PARTICIPANT, self::TEST_NEW_OWNER_PARTICIPANT); // create group $groupBackend = new \Test\Util\Group\Dummy(); $groupBackend->createGroup(self::TEST_GROUP); @@ -68,6 +71,7 @@ public function createBoardWithExampleData() { $cards[] = $this->cardService->create('Card 1', $stacks[0]->getId(), 'text', 0, self::TEST_OWNER); $cards[] = $this->cardService->create('Card 2', $stacks[0]->getId(), 'text', 0, self::TEST_OWNER); $this->assignmentService->assignUser($cards[0]->getId(), self::TEST_OWNER); + $this->assignmentService->assignUser($cards[0]->getId(), self::TEST_NEW_OWNER_PARTICIPANT); $this->board = $board; $this->cards = $cards; $this->stacks = $stacks; @@ -137,7 +141,22 @@ public function testReassignCardToNewOwner() { */ public function testReassignCardToNewParticipantOnlyIfParticipantHasUserType() { $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); - $this->assignmentService->ass($cards[0]->getId(), self::TEST_OWNER); + $this->assignmentService->ass($this->cards[0]->getId(), self::TEST_OWNER); + $assignedUsers = $this->assignedUsersMapper->find($this->cards[0]->getId()); + $participantsUIDs = []; + foreach ($assignedUsers as $user) { + $participantsUIDs[] = $user->getParticipant(); + } + $this->assertContains(self::TEST_NEW_OWNER, $participantsUIDs); + $this->assertNotContains(self::TEST_OWNER, $participantsUIDs); + } + + /** + * @covers ::transferOwnership + */ + public function testTargetAlreadyParticipantOfTransferedCard() { + $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER_PARTICIPANT); + $this->assignmentService->assignUser($this->cards[0]->getId(), self::TEST_OWNER); $assignedUsers = $this->assignedUsersMapper->find($this->cards[0]->getId()); $participantsUIDs = []; foreach ($assignedUsers as $user) { diff --git a/tests/integration/run.sh b/tests/integration/run.sh index 63a733723..17ad85028 100755 --- a/tests/integration/run.sh +++ b/tests/integration/run.sh @@ -28,7 +28,7 @@ composer dump-autoload if [ -z "$EXECUTOR_NUMBER" ]; then EXECUTOR_NUMBER=0 fi -PORT=$((8080 + $EXECUTOR_NUMBER)) +PORT=$((9090 + $EXECUTOR_NUMBER)) echo $PORT php -S localhost:$PORT -t $OC_PATH & PHPPID=$! diff --git a/tests/unit/Service/BoardServiceTest.php b/tests/unit/Service/BoardServiceTest.php index 6553a6245..4f181c533 100644 --- a/tests/unit/Service/BoardServiceTest.php +++ b/tests/unit/Service/BoardServiceTest.php @@ -31,6 +31,7 @@ use OCA\Deck\Db\AssignedUsersMapper; use OCA\Deck\Db\Board; use OCA\Deck\Db\BoardMapper; +use OCA\Deck\Db\CardMapper; use OCA\Deck\Db\ChangeHelper; use OCA\Deck\Db\LabelMapper; use OCA\Deck\Db\StackMapper; @@ -56,6 +57,8 @@ class BoardServiceTest extends TestCase { private $boardMapper; /** @var StackMapper */ private $stackMapper; + /** @var CardMapper */ + private $cardMapper; /** @var PermissionService */ private $permissionService; /** @var NotificationHelper */ @@ -80,6 +83,7 @@ public function setUp(): void { $this->aclMapper = $this->createMock(AclMapper::class); $this->boardMapper = $this->createMock(BoardMapper::class); $this->stackMapper = $this->createMock(StackMapper::class); + $this->cardMapper = $this->createMock(CardMapper::class); $this->labelMapper = $this->createMock(LabelMapper::class); $this->permissionService = $this->createMock(PermissionService::class); $this->notificationHelper = $this->createMock(NotificationHelper::class); @@ -99,6 +103,7 @@ public function setUp(): void { $this->permissionService, $this->notificationHelper, $this->assignedUsersMapper, + $this->cardMapper, $this->userManager, $this->groupManager, $this->activityManager, From 0b85ccc82085fa093eff72d1bcd3dafadb22bf32 Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Sat, 18 Jul 2020 07:40:47 +0300 Subject: [PATCH 8/9] Transfer deck ownership even if target user already participant of a board https://github.com/nextcloud/deck/pull/1955#issuecomment-640392715 Signed-off-by: Sergey Shliakhov --- lib/Db/AclMapper.php | 36 ++++-- lib/Db/AssignedUsersMapper.php | 9 +- lib/Service/BoardService.php | 6 +- .../database/TransferOwnershipTest.php | 115 +++++++++++------- 4 files changed, 113 insertions(+), 53 deletions(-) diff --git a/lib/Db/AclMapper.php b/lib/Db/AclMapper.php index f74af1142..0ac14fd84 100644 --- a/lib/Db/AclMapper.php +++ b/lib/Db/AclMapper.php @@ -58,13 +58,33 @@ public function findByParticipant($type, $participant) { * @return void */ public function transferOwnership($ownerId, $newOwnerId) { - $params = [ - 'owner' => $ownerId, - 'newOwner' => $newOwnerId, - 'type' => Acl::PERMISSION_TYPE_USER - ]; - $sql = "UPDATE `{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner AND `type` = :type"; - $stmt = $this->execute($sql, $params); - $stmt->closeCursor(); + $params = [ + 'owner' => $ownerId, + 'newOwner' => $newOwnerId, + 'type' => Acl::PERMISSION_TYPE_USER + ]; + //We want preserve permissions from both users + $sql = "UPDATE `{$this->tableName}` AS `source` + LEFT JOIN `{$this->tableName}` AS `target` + ON `target`.`participant` = :newOwner AND `target`.`type` = :type + SET `source`.`permission_edit` =(`source`.`permission_edit` || `target`.`permission_edit`), + `source`.`permission_share` =(`source`.`permission_share` || `target`.`permission_share`), + `source`.`permission_manage` =(`source`.`permission_manage` || `target`.`permission_manage`) + WHERE `source`.`participant` = :owner AND `source`.`type` = :type"; + $stmt = $this->execute($sql, $params); + $stmt->closeCursor(); + //We can't transfer acl if target already in acl + $sql = "DELETE FROM `{$this->tableName}` + WHERE `participant` = :newOwner + AND `type` = :type + AND EXISTS (SELECT `id` FROM (SELECT `id` FROM `{$this->tableName}` + WHERE `participant` = :owner AND `type` = :type) as tmp)"; + $stmt = $this->execute($sql, $params); + $stmt->closeCursor(); + //Now we can transfer without errors + $sqlUpdate = "UPDATE `{$this->tableName}` + SET `participant` = :newOwner WHERE `participant` = :owner AND `type` = :type"; + $stmt = $this->execute($sqlUpdate, $params); + $stmt->closeCursor(); } } diff --git a/lib/Db/AssignedUsersMapper.php b/lib/Db/AssignedUsersMapper.php index 136ab760e..df90236dc 100644 --- a/lib/Db/AssignedUsersMapper.php +++ b/lib/Db/AssignedUsersMapper.php @@ -122,10 +122,17 @@ private function getOrigin(AssignedUsers $assignment) { */ public function transferOwnership($ownerId, $newOwnerId) { $params = [ - 'owner' => $ownerId, 'newOwner' => $newOwnerId, 'type' => AssignedUsers::TYPE_USER ]; + $sql = "DELETE FROM `{$this->tableName}` WHERE `participant` = :newOwner AND `type`= :type"; + $stmt = $this->execute($sql, $params); + $stmt->closeCursor(); + $params = [ + 'owner' => $ownerId, + 'newOwner' => $newOwnerId, + 'type' => AssignedUsers::TYPE_USER + ]; $sql = "UPDATE `{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner AND `type`= :type"; $stmt = $this->execute($sql, $params); $stmt->closeCursor(); diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index a4c5adfe8..fa5613182 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -680,9 +680,9 @@ public function clone($id, $userId) { */ public function transferOwnership($owner, $newOwner) { $this->boardMapper->transferOwnership($owner, $newOwner); - $this->assignedUsersMapper->transferOwnership($owner, $newOwner); - $this->aclMapper->transferOwnership($owner, $newOwner); - $this->cardMapper->transferOwnership($owner, $newOwner); + $this->aclMapper->transferOwnership($owner, $newOwner); + $this->assignedUsersMapper->transferOwnership($owner, $newOwner); + $this->cardMapper->transferOwnership($owner, $newOwner); } private function enrichWithStacks($board, $since = -1) { diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php index aff664640..459615860 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -3,6 +3,7 @@ namespace OCA\Deck\Service; use OCA\Deck\Db\Acl; +use OCA\Deck\Db\AssignedUsers; use OCA\Deck\Db\AssignedUsersMapper; use OCA\Deck\Db\Board; @@ -11,10 +12,9 @@ * @coversDefaultClass OCA\Deck\Service\BoardService */ class TransferOwnershipTest extends \Test\TestCase { - private const TEST_OWNER = 'test-share-user1'; - private const TEST_NEW_OWNER = 'target'; - private const TEST_NEW_OWNER_PARTICIPANT = 'target-participant'; - private const TEST_NEW_OWNER_IN_ACL = 'target-in-acl'; + private const TEST_USER_1 = 'test-share-user1'; + private const TEST_USER_2 = 'test-user2'; + private const TEST_USER_3 = 'test-user3'; private const TEST_GROUP = 'test-share-user1'; /** @var BoardService */ @@ -38,19 +38,19 @@ public static function setUpBeforeClass(): void { $backend = new \Test\Util\User\Dummy(); \OC_User::useBackend($backend); \OC::$server->getUserManager()->registerBackend($backend); - $backend->createUser(self::TEST_OWNER, self::TEST_OWNER); - $backend->createUser(self::TEST_NEW_OWNER, self::TEST_NEW_OWNER); - $backend->createUser(self::TEST_NEW_OWNER_PARTICIPANT, self::TEST_NEW_OWNER_PARTICIPANT); + $backend->createUser(self::TEST_USER_1, self::TEST_USER_1); + $backend->createUser(self::TEST_USER_2, self::TEST_USER_2); + $backend->createUser(self::TEST_USER_3, self::TEST_USER_3); // create group $groupBackend = new \Test\Util\Group\Dummy(); $groupBackend->createGroup(self::TEST_GROUP); - $groupBackend->addToGroup(self::TEST_OWNER, self::TEST_GROUP); + $groupBackend->addToGroup(self::TEST_USER_1, self::TEST_GROUP); \OC::$server->getGroupManager()->addBackend($groupBackend); } public function setUp(): void { parent::setUp(); - \OC::$server->getUserSession()->login(self::TEST_OWNER, self::TEST_OWNER); + \OC::$server->getUserSession()->login(self::TEST_USER_1, self::TEST_USER_1); $this->boardService = \OC::$server->query(BoardService::class); $this->stackService = \OC::$server->query(StackService::class); $this->cardService = \OC::$server->query(CardService::class); @@ -61,17 +61,17 @@ public function setUp(): void { public function createBoardWithExampleData() { $stacks = []; - $board = $this->boardService->create('Test', self::TEST_OWNER, '000000'); + $board = $this->boardService->create('Test', self::TEST_USER_1, '000000'); $id = $board->getId(); - $this->boardService->addAcl($id, Acl::PERMISSION_TYPE_USER, self::TEST_OWNER, true, true, true); + $this->boardService->addAcl($id, Acl::PERMISSION_TYPE_USER, self::TEST_USER_1, true, true, true); $this->boardService->addAcl($id, Acl::PERMISSION_TYPE_GROUP, self::TEST_GROUP, true, true, true); - $stacks[] = $this->stackService->create('Stack A', $id, 1); + $this->boardService->addAcl($id, Acl::PERMISSION_TYPE_USER, self::TEST_USER_3, false, true, false); + $stacks[] = $this->stackService->create('Stack A', $id, 1); $stacks[] = $this->stackService->create('Stack B', $id, 1); $stacks[] = $this->stackService->create('Stack C', $id, 1); - $cards[] = $this->cardService->create('Card 1', $stacks[0]->getId(), 'text', 0, self::TEST_OWNER); - $cards[] = $this->cardService->create('Card 2', $stacks[0]->getId(), 'text', 0, self::TEST_OWNER); - $this->assignmentService->assignUser($cards[0]->getId(), self::TEST_OWNER); - $this->assignmentService->assignUser($cards[0]->getId(), self::TEST_NEW_OWNER_PARTICIPANT); + $cards[] = $this->cardService->create('Card 1', $stacks[0]->getId(), 'text', 0, self::TEST_USER_1); + $cards[] = $this->cardService->create('Card 2', $stacks[0]->getId(), 'text', 0, self::TEST_USER_1); + $this->assignmentService->assignUser($cards[0]->getId(), self::TEST_USER_1); $this->board = $board; $this->cards = $cards; $this->stacks = $stacks; @@ -81,21 +81,21 @@ public function createBoardWithExampleData() { * @covers ::transferOwnership */ public function testTransferBoardOwnership() { - $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); $board = $this->boardService->find($this->board->getId()); $boardOwner = $board->getOwner(); - $this->assertEquals(self::TEST_NEW_OWNER, $boardOwner); + $this->assertEquals(self::TEST_USER_2, $boardOwner); } /** * @covers ::transferOwnership */ public function testTransferACLOwnership() { - $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); $board = $this->boardService->find($this->board->getId()); $acl = $board->getAcl(); $isTargetInAcl = (bool)array_filter($acl, function ($item) { - return $item->getParticipant() === self::TEST_NEW_OWNER && $item->getType() === Acl::PERMISSION_TYPE_USER; + return $item->getParticipant() === self::TEST_USER_2 && $item->getType() === Acl::PERMISSION_TYPE_USER; }); $this->assertTrue($isTargetInAcl); } @@ -104,7 +104,7 @@ public function testTransferACLOwnership() { * @covers ::transferOwnership */ public function testNoTransferAclOwnershipIfGroupType() { - $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); $board = $this->boardService->find($this->board->getId()); $acl = $board->getAcl(); $isGroupInAcl = (bool)array_filter($acl, function ($item) { @@ -116,54 +116,87 @@ public function testNoTransferAclOwnershipIfGroupType() { * @covers ::transferOwnership */ public function testTransferCardOwnership() { - $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); $card = $this->cardService->find($this->cards[0]->getId()); $cardOwner = $card->getOwner(); - $this->assertEquals(self::TEST_NEW_OWNER, $cardOwner); + $this->assertEquals(self::TEST_USER_2, $cardOwner); } /** * @covers ::transferOwnership */ public function testReassignCardToNewOwner() { - $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); $assignedUsers = $this->assignedUsersMapper->find($this->cards[0]->getId()); $participantsUIDs = []; foreach ($assignedUsers as $user) { $participantsUIDs[] = $user->getParticipant(); } - $this->assertContains(self::TEST_NEW_OWNER, $participantsUIDs); - $this->assertNotContains(self::TEST_OWNER, $participantsUIDs); + $this->assertContains(self::TEST_USER_2, $participantsUIDs); + $this->assertNotContains(self::TEST_USER_1, $participantsUIDs); } /** * @covers ::transferOwnership */ public function testReassignCardToNewParticipantOnlyIfParticipantHasUserType() { - $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER); - $this->assignmentService->ass($this->cards[0]->getId(), self::TEST_OWNER); - $assignedUsers = $this->assignedUsersMapper->find($this->cards[0]->getId()); + $this->assignmentService->assignUser($this->cards[1]->getId(), self::TEST_USER_1, AssignedUsers::TYPE_GROUP); + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); + $assignedUsers = $this->assignedUsersMapper->find($this->cards[1]->getId()); $participantsUIDs = []; foreach ($assignedUsers as $user) { $participantsUIDs[] = $user->getParticipant(); } - $this->assertContains(self::TEST_NEW_OWNER, $participantsUIDs); - $this->assertNotContains(self::TEST_OWNER, $participantsUIDs); + $this->assertContains(self::TEST_USER_1, $participantsUIDs); + $this->assertNotContains(self::TEST_USER_2, $participantsUIDs); } /** * @covers ::transferOwnership */ - public function testTargetAlreadyParticipantOfTransferedCard() { - $this->boardService->transferOwnership(self::TEST_OWNER, self::TEST_NEW_OWNER_PARTICIPANT); - $this->assignmentService->assignUser($this->cards[0]->getId(), self::TEST_OWNER); - $assignedUsers = $this->assignedUsersMapper->find($this->cards[0]->getId()); - $participantsUIDs = []; - foreach ($assignedUsers as $user) { - $participantsUIDs[] = $user->getParticipant(); - } - $this->assertContains(self::TEST_NEW_OWNER, $participantsUIDs); - $this->assertNotContains(self::TEST_OWNER, $participantsUIDs); + public function testTargetAlreadyParticipantOfBoard() { + $this->expectNotToPerformAssertions(); + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3); + } + + /** + * @covers ::transferOwnership + */ + public function testDontRemoveTargetFromAcl() { + $this->boardService->transferOwnership(self::TEST_USER_2, self::TEST_USER_3); + $board = $this->boardService->find($this->board->getId()); + $acl = $board->getAcl(); + $isOwnerInAcl = (bool)array_filter($acl, function ($item) { + return $item->getParticipant() === self::TEST_USER_3 && $item->getType() === Acl::PERMISSION_TYPE_USER; + }); + $this->assertTrue($isOwnerInAcl); + } + + /** + * @covers ::transferOwnership + */ + public function testMergePermissions() { + $this->boardService->addAcl($this->board->getId(), Acl::PERMISSION_TYPE_USER, self::TEST_USER_2, true, false, true); + $this->boardService->transferOwnership(self::TEST_USER_2, self::TEST_USER_3); + $board = $this->boardService->find($this->board->getId()); + $acl = $board->getAcl(); + $isMerged = (bool)array_filter($acl, function ($item) { + return $item->getParticipant() === self::TEST_USER_1 + && $item->getType() === Acl::PERMISSION_TYPE_USER + && $item->getPermission(Acl::PERMISSION_EDIT) + && $item->getPermission(Acl::PERMISSION_SHARE) + && $item->getPermission(Acl::PERMISSION_MANAGE); + }); + $this->assertTrue($isMerged); + } + + /** + * @covers ::transferOwnership + */ + public function testTargetAlreadyParticipantOfCard() { + $this->expectNotToPerformAssertions(); + $this->assignmentService->assignUser($this->cards[0]->getId(), self::TEST_USER_3, AssignedUsers::TYPE_USER); + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3); } public function tearDown(): void { From 1535c4a208ca61ddc2d13c940af4c5fd7d63c117 Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Sat, 18 Jul 2020 09:02:28 +0300 Subject: [PATCH 9/9] Fix coding styles --- lib/Db/AclMapper.php | 32 ++--- lib/Db/AssignedUsersMapper.php | 18 +-- lib/Service/BoardService.php | 6 +- .../database/TransferOwnershipTest.php | 126 +++++++++--------- tests/unit/Service/BoardServiceTest.php | 4 +- 5 files changed, 93 insertions(+), 93 deletions(-) diff --git a/lib/Db/AclMapper.php b/lib/Db/AclMapper.php index 0ac14fd84..6e9ddb6aa 100644 --- a/lib/Db/AclMapper.php +++ b/lib/Db/AclMapper.php @@ -58,33 +58,33 @@ public function findByParticipant($type, $participant) { * @return void */ public function transferOwnership($ownerId, $newOwnerId) { - $params = [ - 'owner' => $ownerId, - 'newOwner' => $newOwnerId, - 'type' => Acl::PERMISSION_TYPE_USER - ]; + $params = [ + 'owner' => $ownerId, + 'newOwner' => $newOwnerId, + 'type' => Acl::PERMISSION_TYPE_USER + ]; //We want preserve permissions from both users - $sql = "UPDATE `{$this->tableName}` AS `source` + $sql = "UPDATE `{$this->tableName}` AS `source` LEFT JOIN `{$this->tableName}` AS `target` ON `target`.`participant` = :newOwner AND `target`.`type` = :type SET `source`.`permission_edit` =(`source`.`permission_edit` || `target`.`permission_edit`), `source`.`permission_share` =(`source`.`permission_share` || `target`.`permission_share`), `source`.`permission_manage` =(`source`.`permission_manage` || `target`.`permission_manage`) WHERE `source`.`participant` = :owner AND `source`.`type` = :type"; - $stmt = $this->execute($sql, $params); - $stmt->closeCursor(); - //We can't transfer acl if target already in acl - $sql = "DELETE FROM `{$this->tableName}` + $stmt = $this->execute($sql, $params); + $stmt->closeCursor(); + //We can't transfer acl if target already in acl + $sql = "DELETE FROM `{$this->tableName}` WHERE `participant` = :newOwner AND `type` = :type AND EXISTS (SELECT `id` FROM (SELECT `id` FROM `{$this->tableName}` WHERE `participant` = :owner AND `type` = :type) as tmp)"; - $stmt = $this->execute($sql, $params); - $stmt->closeCursor(); - //Now we can transfer without errors - $sqlUpdate = "UPDATE `{$this->tableName}` + $stmt = $this->execute($sql, $params); + $stmt->closeCursor(); + //Now we can transfer without errors + $sqlUpdate = "UPDATE `{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner AND `type` = :type"; - $stmt = $this->execute($sqlUpdate, $params); - $stmt->closeCursor(); + $stmt = $this->execute($sqlUpdate, $params); + $stmt->closeCursor(); } } diff --git a/lib/Db/AssignedUsersMapper.php b/lib/Db/AssignedUsersMapper.php index df90236dc..d3c9d6670 100644 --- a/lib/Db/AssignedUsersMapper.php +++ b/lib/Db/AssignedUsersMapper.php @@ -123,16 +123,16 @@ private function getOrigin(AssignedUsers $assignment) { public function transferOwnership($ownerId, $newOwnerId) { $params = [ 'newOwner' => $newOwnerId, - 'type' => AssignedUsers::TYPE_USER + 'type' => AssignedUsers::TYPE_USER + ]; + $sql = "DELETE FROM `{$this->tableName}` WHERE `participant` = :newOwner AND `type`= :type"; + $stmt = $this->execute($sql, $params); + $stmt->closeCursor(); + $params = [ + 'owner' => $ownerId, + 'newOwner' => $newOwnerId, + 'type' => AssignedUsers::TYPE_USER ]; - $sql = "DELETE FROM `{$this->tableName}` WHERE `participant` = :newOwner AND `type`= :type"; - $stmt = $this->execute($sql, $params); - $stmt->closeCursor(); - $params = [ - 'owner' => $ownerId, - 'newOwner' => $newOwnerId, - 'type' => AssignedUsers::TYPE_USER - ]; $sql = "UPDATE `{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner AND `type`= :type"; $stmt = $this->execute($sql, $params); $stmt->closeCursor(); diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index fa5613182..e9c01d680 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -680,9 +680,9 @@ public function clone($id, $userId) { */ public function transferOwnership($owner, $newOwner) { $this->boardMapper->transferOwnership($owner, $newOwner); - $this->aclMapper->transferOwnership($owner, $newOwner); - $this->assignedUsersMapper->transferOwnership($owner, $newOwner); - $this->cardMapper->transferOwnership($owner, $newOwner); + $this->aclMapper->transferOwnership($owner, $newOwner); + $this->assignedUsersMapper->transferOwnership($owner, $newOwner); + $this->cardMapper->transferOwnership($owner, $newOwner); } private function enrichWithStacks($board, $since = -1) { diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php index 459615860..582fca85d 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -66,7 +66,7 @@ public function createBoardWithExampleData() { $this->boardService->addAcl($id, Acl::PERMISSION_TYPE_USER, self::TEST_USER_1, true, true, true); $this->boardService->addAcl($id, Acl::PERMISSION_TYPE_GROUP, self::TEST_GROUP, true, true, true); $this->boardService->addAcl($id, Acl::PERMISSION_TYPE_USER, self::TEST_USER_3, false, true, false); - $stacks[] = $this->stackService->create('Stack A', $id, 1); + $stacks[] = $this->stackService->create('Stack A', $id, 1); $stacks[] = $this->stackService->create('Stack B', $id, 1); $stacks[] = $this->stackService->create('Stack C', $id, 1); $cards[] = $this->cardService->create('Card 1', $stacks[0]->getId(), 'text', 0, self::TEST_USER_1); @@ -136,68 +136,68 @@ public function testReassignCardToNewOwner() { $this->assertNotContains(self::TEST_USER_1, $participantsUIDs); } - /** - * @covers ::transferOwnership - */ - public function testReassignCardToNewParticipantOnlyIfParticipantHasUserType() { - $this->assignmentService->assignUser($this->cards[1]->getId(), self::TEST_USER_1, AssignedUsers::TYPE_GROUP); - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); - $assignedUsers = $this->assignedUsersMapper->find($this->cards[1]->getId()); - $participantsUIDs = []; - foreach ($assignedUsers as $user) { - $participantsUIDs[] = $user->getParticipant(); - } - $this->assertContains(self::TEST_USER_1, $participantsUIDs); - $this->assertNotContains(self::TEST_USER_2, $participantsUIDs); - } - - /** - * @covers ::transferOwnership - */ - public function testTargetAlreadyParticipantOfBoard() { - $this->expectNotToPerformAssertions(); - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3); - } - - /** - * @covers ::transferOwnership - */ - public function testDontRemoveTargetFromAcl() { - $this->boardService->transferOwnership(self::TEST_USER_2, self::TEST_USER_3); - $board = $this->boardService->find($this->board->getId()); - $acl = $board->getAcl(); - $isOwnerInAcl = (bool)array_filter($acl, function ($item) { - return $item->getParticipant() === self::TEST_USER_3 && $item->getType() === Acl::PERMISSION_TYPE_USER; - }); - $this->assertTrue($isOwnerInAcl); - } - - /** - * @covers ::transferOwnership - */ - public function testMergePermissions() { - $this->boardService->addAcl($this->board->getId(), Acl::PERMISSION_TYPE_USER, self::TEST_USER_2, true, false, true); - $this->boardService->transferOwnership(self::TEST_USER_2, self::TEST_USER_3); - $board = $this->boardService->find($this->board->getId()); - $acl = $board->getAcl(); - $isMerged = (bool)array_filter($acl, function ($item) { - return $item->getParticipant() === self::TEST_USER_1 - && $item->getType() === Acl::PERMISSION_TYPE_USER - && $item->getPermission(Acl::PERMISSION_EDIT) - && $item->getPermission(Acl::PERMISSION_SHARE) - && $item->getPermission(Acl::PERMISSION_MANAGE); - }); - $this->assertTrue($isMerged); - } - - /** - * @covers ::transferOwnership - */ - public function testTargetAlreadyParticipantOfCard() { - $this->expectNotToPerformAssertions(); - $this->assignmentService->assignUser($this->cards[0]->getId(), self::TEST_USER_3, AssignedUsers::TYPE_USER); - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3); - } + /** + * @covers ::transferOwnership + */ + public function testReassignCardToNewParticipantOnlyIfParticipantHasUserType() { + $this->assignmentService->assignUser($this->cards[1]->getId(), self::TEST_USER_1, AssignedUsers::TYPE_GROUP); + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); + $assignedUsers = $this->assignedUsersMapper->find($this->cards[1]->getId()); + $participantsUIDs = []; + foreach ($assignedUsers as $user) { + $participantsUIDs[] = $user->getParticipant(); + } + $this->assertContains(self::TEST_USER_1, $participantsUIDs); + $this->assertNotContains(self::TEST_USER_2, $participantsUIDs); + } + + /** + * @covers ::transferOwnership + */ + public function testTargetAlreadyParticipantOfBoard() { + $this->expectNotToPerformAssertions(); + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3); + } + + /** + * @covers ::transferOwnership + */ + public function testDontRemoveTargetFromAcl() { + $this->boardService->transferOwnership(self::TEST_USER_2, self::TEST_USER_3); + $board = $this->boardService->find($this->board->getId()); + $acl = $board->getAcl(); + $isOwnerInAcl = (bool)array_filter($acl, function ($item) { + return $item->getParticipant() === self::TEST_USER_3 && $item->getType() === Acl::PERMISSION_TYPE_USER; + }); + $this->assertTrue($isOwnerInAcl); + } + + /** + * @covers ::transferOwnership + */ + public function testMergePermissions() { + $this->boardService->addAcl($this->board->getId(), Acl::PERMISSION_TYPE_USER, self::TEST_USER_2, true, false, true); + $this->boardService->transferOwnership(self::TEST_USER_2, self::TEST_USER_3); + $board = $this->boardService->find($this->board->getId()); + $acl = $board->getAcl(); + $isMerged = (bool)array_filter($acl, function ($item) { + return $item->getParticipant() === self::TEST_USER_1 + && $item->getType() === Acl::PERMISSION_TYPE_USER + && $item->getPermission(Acl::PERMISSION_EDIT) + && $item->getPermission(Acl::PERMISSION_SHARE) + && $item->getPermission(Acl::PERMISSION_MANAGE); + }); + $this->assertTrue($isMerged); + } + + /** + * @covers ::transferOwnership + */ + public function testTargetAlreadyParticipantOfCard() { + $this->expectNotToPerformAssertions(); + $this->assignmentService->assignUser($this->cards[0]->getId(), self::TEST_USER_3, AssignedUsers::TYPE_USER); + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3); + } public function tearDown(): void { $this->boardService->deleteForce($this->board->getId()); diff --git a/tests/unit/Service/BoardServiceTest.php b/tests/unit/Service/BoardServiceTest.php index 4f181c533..b6f22f7fd 100644 --- a/tests/unit/Service/BoardServiceTest.php +++ b/tests/unit/Service/BoardServiceTest.php @@ -57,8 +57,8 @@ class BoardServiceTest extends TestCase { private $boardMapper; /** @var StackMapper */ private $stackMapper; - /** @var CardMapper */ - private $cardMapper; + /** @var CardMapper */ + private $cardMapper; /** @var PermissionService */ private $permissionService; /** @var NotificationHelper */