From 5d0b8d878bfa18fec547c8f0eace96a5db5fb833 Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Thu, 21 May 2020 15:17:31 +0200 Subject: [PATCH 01/29] 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 a0706cb09..d6f9d68a6 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -44,6 +44,7 @@ OCA\Deck\Command\UserExport OCA\Deck\Command\BoardImport + 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 19a2aeb5e598c5c717344d3ac68121b61597305d Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Mon, 25 May 2020 09:46:57 +0200 Subject: [PATCH 02/29] Update docs Signed-off-by: Sergey Shliakhov fix: conflicts --- docs/User_documentation_en.md | 5 +++ lib/Command/TransferOwnership.php | 52 +++++++++++-------------------- lib/Db/AclMapper.php | 17 ++++++++++ lib/Db/AssignmentMapper.php | 16 ++++++++++ lib/Db/BoardMapper.php | 16 ++++++++++ lib/Db/CardMapper.php | 16 ++++++++++ lib/Service/BoardService.php | 18 +++++++++++ 7 files changed, 107 insertions(+), 33 deletions(-) diff --git a/docs/User_documentation_en.md b/docs/User_documentation_en.md index 3a6f9dc18..9aacbd82a 100644 --- a/docs/User_documentation_en.md +++ b/docs/User_documentation_en.md @@ -16,6 +16,7 @@ Overall, Deck is easy to use. You can create boards, add users, share the Deck, 5. [Manage your board](#5-manage-your-board) 6. [Import boards](#6-import-boards) 7. [Search](#7-search) +8. [New owner for the deck entities](#8-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. @@ -159,3 +160,7 @@ For example the search `project tag:ToDo assigned:alice assigned:bob` will retur Other text tokens will be used to perform a case-insensitive search on the card title and description In addition wuotes can be used to pass a query with spaces, e.g. `"Exact match with spaces"` or `title:"My card"`. + +### 8. 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` 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 @@ final class TransferOwnership extends Command { 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 4eb6a59c6..721ff1b6e 100644 --- a/lib/Db/AclMapper.php +++ b/lib/Db/AclMapper.php @@ -57,4 +57,21 @@ class AclMapper extends DeckMapper implements IPermissionMapper { $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/AssignmentMapper.php b/lib/Db/AssignmentMapper.php index 275dd1f77..5ed3ce115 100644 --- a/lib/Db/AssignmentMapper.php +++ b/lib/Db/AssignmentMapper.php @@ -146,4 +146,20 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { } 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 6b08000ba..4e0af34fe 100644 --- a/lib/Db/BoardMapper.php +++ b/lib/Db/BoardMapper.php @@ -475,4 +475,20 @@ class BoardMapper extends QBMapper implements IPermissionMapper { 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 699c40a06..3bf88da4b 100644 --- a/lib/Db/CardMapper.php +++ b/lib/Db/CardMapper.php @@ -586,4 +586,20 @@ class CardMapper extends QBMapper implements IPermissionMapper { 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 568299c53..9f81cc4c7 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -30,6 +30,7 @@ use OCA\Deck\AppInfo\Application; use OCA\Deck\Db\Acl; use OCA\Deck\Db\AclMapper; use OCA\Deck\Db\AssignmentMapper; +use OCA\Deck\Db\CardMapper; use OCA\Deck\Db\ChangeHelper; use OCA\Deck\Db\IPermissionMapper; use OCA\Deck\Db\Label; @@ -69,6 +70,8 @@ class BoardService { private $activityManager; private $eventDispatcher; private $changeHelper; + private $cardMapper; + private $boardsCache = null; private $urlGenerator; @@ -83,6 +86,7 @@ class BoardService { PermissionService $permissionService, NotificationHelper $notificationHelper, AssignmentMapper $assignedUsersMapper, + CardMapper $cardMapper, IUserManager $userManager, IGroupManager $groupManager, ActivityManager $activityManager, @@ -107,6 +111,7 @@ class BoardService { $this->changeHelper = $changeHelper; $this->userId = $userId; $this->urlGenerator = $urlGenerator; + $this->cardMapper = $cardMapper; } /** @@ -673,6 +678,19 @@ class BoardService { 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 3d269e28f46d8c397a62aa0bf81486d084c8b40b Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Sun, 7 Jun 2020 16:11:36 +0200 Subject: [PATCH 03/29] 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 b45c454ce2577fbb631beb9a840cf265a5c90a2b Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Sun, 7 Jun 2020 16:18:38 +0200 Subject: [PATCH 04/29] Fix code style Signed-off-by: Sergey Shliakhov --- lib/Command/TransferOwnership.php | 5 +- lib/Db/AclMapper.php | 3 +- lib/Db/AssignmentMapper.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\Input\InputInterface; 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 @@ final class TransferOwnership extends Command { $output->writeln("Transfer deck entities from $owner to $newOwner completed"); } - } diff --git a/lib/Db/AclMapper.php b/lib/Db/AclMapper.php index 721ff1b6e..48237ce03 100644 --- a/lib/Db/AclMapper.php +++ b/lib/Db/AclMapper.php @@ -63,8 +63,7 @@ class AclMapper extends DeckMapper implements IPermissionMapper { * @param $newOwnerId * @return void */ - public function transferOwnership($ownerId, $newOwnerId) - { + public function transferOwnership($ownerId, $newOwnerId) { $params = [ 'owner' => $ownerId, 'newOwner' => $newOwnerId, diff --git a/lib/Db/AssignmentMapper.php b/lib/Db/AssignmentMapper.php index 5ed3ce115..7a7f368ae 100644 --- a/lib/Db/AssignmentMapper.php +++ b/lib/Db/AssignmentMapper.php @@ -152,8 +152,7 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { * @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 4e0af34fe..fc5455ef4 100644 --- a/lib/Db/BoardMapper.php +++ b/lib/Db/BoardMapper.php @@ -476,19 +476,18 @@ class BoardMapper extends QBMapper implements IPermissionMapper { }); } - /** - * @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 3bf88da4b..0d935ff26 100644 --- a/lib/Db/CardMapper.php +++ b/lib/Db/CardMapper.php @@ -592,8 +592,7 @@ class CardMapper extends QBMapper implements IPermissionMapper { * @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 9f81cc4c7..1a9969e4f 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -683,8 +683,7 @@ class BoardService { * @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 @@ class AssignedUsersMapperTest extends \Test\TestCase { $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 e3750a709ddb08d882e28e248fee7f9fca100112 Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Mon, 8 Jun 2020 14:06:50 +0200 Subject: [PATCH 05/29] 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 @@ use OCA\Deck\Db\Board; * @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 @@ class AssignedUsersMapperTest extends \Test\TestCase { $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 8b454952146d49ebf5648ab757d30565acaa51eb Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Tue, 9 Jun 2020 05:21:24 +0200 Subject: [PATCH 06/29] Check type before transfer card participants ownership Signed-off-by: Sergey Shliakhov temp --- Makefile | 3 +-- lib/Db/AssignmentMapper.php | 5 +++-- tests/integration/config/behat.yml | 2 +- .../database/TransferOwnershipTest.php | 21 ++++++++++++++++++- tests/integration/run.sh | 2 +- tests/unit/Service/BoardServiceTest.php | 5 +++++ 6 files changed, 31 insertions(+), 7 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/AssignmentMapper.php b/lib/Db/AssignmentMapper.php index 7a7f368ae..5bc49e40d 100644 --- a/lib/Db/AssignmentMapper.php +++ b/lib/Db/AssignmentMapper.php @@ -155,9 +155,10 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { 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 b8673a677..507138122 100644 --- a/tests/integration/config/behat.yml +++ b/tests/integration/config/behat.yml @@ -5,7 +5,7 @@ default: - '%paths.base%/../features/' contexts: - ServerContext: - baseUrl: http://localhost:8080/ + baseUrl: http://localhost:9090/ - RequestContext - BoardContext - CommentContext 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 @@ use OCA\Deck\Db\Board; 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 @@ class TransferOwnershipTest extends \Test\TestCase { \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 @@ class TransferOwnershipTest extends \Test\TestCase { $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 @@ class TransferOwnershipTest extends \Test\TestCase { */ 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 ab9790b26..de8b32563 100755 --- a/tests/integration/run.sh +++ b/tests/integration/run.sh @@ -26,7 +26,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 e59662489..c64422ff5 100644 --- a/tests/unit/Service/BoardServiceTest.php +++ b/tests/unit/Service/BoardServiceTest.php @@ -31,6 +31,7 @@ use OCA\Deck\Db\Assignment; use OCA\Deck\Db\AssignmentMapper; 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; @@ -58,6 +59,8 @@ class BoardServiceTest extends TestCase { private $boardMapper; /** @var StackMapper */ private $stackMapper; + /** @var CardMapper */ + private $cardMapper; /** @var PermissionService */ private $permissionService; /** @var NotificationHelper */ @@ -85,6 +88,7 @@ class BoardServiceTest extends TestCase { $this->boardMapper = $this->createMock(BoardMapper::class); $this->stackMapper = $this->createMock(StackMapper::class); $this->config = $this->createMock(IConfig::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); @@ -106,6 +110,7 @@ class BoardServiceTest extends TestCase { $this->permissionService, $this->notificationHelper, $this->assignedUsersMapper, + $this->cardMapper, $this->userManager, $this->groupManager, $this->activityManager, From 7df4b7c4bf790c6bf2861fc461af946b3c926c53 Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Sat, 18 Jul 2020 07:40:47 +0300 Subject: [PATCH 07/29] 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/AssignmentMapper.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 48237ce03..42aec5c4a 100644 --- a/lib/Db/AclMapper.php +++ b/lib/Db/AclMapper.php @@ -64,13 +64,33 @@ class AclMapper extends DeckMapper implements IPermissionMapper { * @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/AssignmentMapper.php b/lib/Db/AssignmentMapper.php index 5bc49e40d..d783cdedb 100644 --- a/lib/Db/AssignmentMapper.php +++ b/lib/Db/AssignmentMapper.php @@ -154,10 +154,17 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { */ 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 1a9969e4f..bbcc351e8 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -685,9 +685,9 @@ class BoardService { */ 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 @@ use OCA\Deck\Db\Board; * @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 @@ class TransferOwnershipTest extends \Test\TestCase { $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 @@ class TransferOwnershipTest extends \Test\TestCase { 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 @@ class TransferOwnershipTest extends \Test\TestCase { * @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 @@ class TransferOwnershipTest extends \Test\TestCase { * @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 @@ class TransferOwnershipTest extends \Test\TestCase { * @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 6106066460bc4cf79d685a6349de737d93a03393 Mon Sep 17 00:00:00 2001 From: Sergey Shliakhov Date: Sat, 18 Jul 2020 09:02:28 +0300 Subject: [PATCH 08/29] Fix coding styles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl Signed-off-by: Max --- lib/Db/AclMapper.php | 32 ++--- lib/Db/AssignmentMapper.php | 18 +-- lib/Service/BoardService.php | 6 +- .../database/TransferOwnershipTest.php | 118 +++++++++--------- tests/unit/Service/BoardServiceTest.php | 4 +- 5 files changed, 89 insertions(+), 89 deletions(-) diff --git a/lib/Db/AclMapper.php b/lib/Db/AclMapper.php index 42aec5c4a..d34738d82 100644 --- a/lib/Db/AclMapper.php +++ b/lib/Db/AclMapper.php @@ -64,33 +64,33 @@ class AclMapper extends DeckMapper implements IPermissionMapper { * @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/AssignmentMapper.php b/lib/Db/AssignmentMapper.php index d783cdedb..1c5e43819 100644 --- a/lib/Db/AssignmentMapper.php +++ b/lib/Db/AssignmentMapper.php @@ -155,16 +155,16 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { 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 bbcc351e8..89652f286 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -685,9 +685,9 @@ class BoardService { */ 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 @@ class TransferOwnershipTest extends \Test\TestCase { $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 @@ class TransferOwnershipTest extends \Test\TestCase { $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 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 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 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 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 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 c64422ff5..600f6a37c 100644 --- a/tests/unit/Service/BoardServiceTest.php +++ b/tests/unit/Service/BoardServiceTest.php @@ -59,8 +59,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 */ From ba7cadf9d5cdf65b1a7742773b275dbe92c485b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 3 Nov 2020 15:06:47 +0100 Subject: [PATCH 09/29] Fix card mapper query for transfer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Db/CardMapper.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Db/CardMapper.php b/lib/Db/CardMapper.php index 0d935ff26..973a1c736 100644 --- a/lib/Db/CardMapper.php +++ b/lib/Db/CardMapper.php @@ -597,8 +597,8 @@ class CardMapper extends QBMapper implements IPermissionMapper { 'owner' => $ownerId, 'newOwner' => $newOwnerId ]; - $sql = "UPDATE `{$this->tableName}` SET `owner` = :newOwner WHERE `owner` = :owner"; - $stmt = $this->execute($sql, $params); + $sql = "UPDATE `*PREFIX*{$this->tableName}` SET `owner` = :newOwner WHERE `owner` = :owner"; + $stmt = $this->db->executeQuery($sql, $params); $stmt->closeCursor(); } } From 3e7d0d3d72a86c96c36ac04b3a2544b2686e08a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 10 Nov 2020 12:53:47 +0100 Subject: [PATCH 10/29] Use proper description of what gets transferred MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Command/TransferOwnership.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/Command/TransferOwnership.php b/lib/Command/TransferOwnership.php index c3175477e..2b06c4e1c 100644 --- a/lib/Command/TransferOwnership.php +++ b/lib/Command/TransferOwnership.php @@ -20,7 +20,7 @@ final class TransferOwnership extends Command { protected function configure() { $this ->setName('deck:transfer-ownership') - ->setDescription('Change owner of deck entities') + ->setDescription('Change owner of deck boards') ->addArgument( 'owner', InputArgument::REQUIRED, @@ -37,10 +37,10 @@ final class TransferOwnership extends Command { $owner = $input->getArgument('owner'); $newOwner = $input->getArgument('newOwner'); - $output->writeln("Transfer deck entities from $owner to $newOwner"); + $output->writeln("Transfer deck boards from $owner to $newOwner"); $this->boardService->transferOwnership($owner, $newOwner); - $output->writeln("Transfer deck entities from $owner to $newOwner completed"); + $output->writeln("Transfer deck boards from $owner to $newOwner completed"); } } From fa7fcef470df08888b6dd386209bdd65b3c51ed5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 10 Nov 2020 12:54:17 +0100 Subject: [PATCH 11/29] Just cleanup old ACL rules, there are none for the board owner so nothing to cleanup or persist there MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Db/AclMapper.php | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/lib/Db/AclMapper.php b/lib/Db/AclMapper.php index d34738d82..327d8aa34 100644 --- a/lib/Db/AclMapper.php +++ b/lib/Db/AclMapper.php @@ -69,28 +69,14 @@ class AclMapper extends DeckMapper implements IPermissionMapper { '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 + + // Drop existing ACL rules for the new owner $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)"; + WHERE `participant` = :newOwner 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(); } } From e8ada52c37f7271c87155fb884f94078465f79a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 10 Nov 2020 12:54:36 +0100 Subject: [PATCH 12/29] Make queries work with the new base mapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl fix: conflicts --- lib/Db/AclMapper.php | 9 ++++----- lib/Db/AssignmentMapper.php | 9 +++++---- lib/Service/BoardService.php | 16 +++++++++------- .../database/TransferOwnershipTest.php | 9 +++++---- 4 files changed, 23 insertions(+), 20 deletions(-) diff --git a/lib/Db/AclMapper.php b/lib/Db/AclMapper.php index 327d8aa34..c45a2d421 100644 --- a/lib/Db/AclMapper.php +++ b/lib/Db/AclMapper.php @@ -63,19 +63,18 @@ class AclMapper extends DeckMapper implements IPermissionMapper { * @param $newOwnerId * @return void */ - public function transferOwnership($ownerId, $newOwnerId) { + public function transferOwnership($boardId, $ownerId, $newOwnerId) { $params = [ - 'owner' => $ownerId, 'newOwner' => $newOwnerId, - 'type' => Acl::PERMISSION_TYPE_USER + 'type' => Acl::PERMISSION_TYPE_USER, + 'boardId' => $boardId ]; // Drop existing ACL rules for the new owner $sql = "DELETE FROM `{$this->tableName}` WHERE `participant` = :newOwner AND `type` = :type - AND EXISTS (SELECT `id` FROM (SELECT `id` FROM `{$this->tableName}` - WHERE `participant` = :newOwner AND `type` = :type) as tmp)"; + AND `board_id` = :boardId"; $stmt = $this->execute($sql, $params); $stmt->closeCursor(); } diff --git a/lib/Db/AssignmentMapper.php b/lib/Db/AssignmentMapper.php index 1c5e43819..a5000184d 100644 --- a/lib/Db/AssignmentMapper.php +++ b/lib/Db/AssignmentMapper.php @@ -157,16 +157,17 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { 'newOwner' => $newOwnerId, 'type' => AssignedUsers::TYPE_USER ]; - $sql = "DELETE FROM `{$this->tableName}` WHERE `participant` = :newOwner AND `type`= :type"; - $stmt = $this->execute($sql, $params); + $qb = $this->db->getQueryBuilder(); + $sql = "DELETE FROM `*PREFIX*{$this->tableName}` WHERE `participant` = :newOwner AND `type`= :type"; + $stmt = $this->db->executeQuery($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); + $sql = "UPDATE `*PREFIX*{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner AND `type`= :type"; + $stmt = $this->db->executeQuery($sql, $params); $stmt->closeCursor(); } } diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index 89652f286..671354905 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -678,14 +678,12 @@ class BoardService { return $newBoard; } - /** - * @param $ownerId - * @param $newOwnerId - * @return void - */ - public function transferOwnership($owner, $newOwner) { + public function transferOwnership(string $owner, string $newOwner): void { + $boards = $this->boardMapper->findAllByUser($owner); + foreach ($boards as $board) { + $this->aclMapper->transferOwnership($board->getId(), $owner, $newOwner); + } $this->boardMapper->transferOwnership($owner, $newOwner); - $this->aclMapper->transferOwnership($owner, $newOwner); $this->assignedUsersMapper->transferOwnership($owner, $newOwner); $this->cardMapper->transferOwnership($owner, $newOwner); } @@ -721,4 +719,8 @@ class BoardService { public function getBoardUrl($endpoint) { return $this->urlGenerator->linkToRouteAbsolute('deck.page.index') . '#' . $endpoint; } + + private function clearBoardsCache() { + $this->boardsCache = null; + } } diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php index 582fca85d..fbfab856f 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -82,6 +82,7 @@ class TransferOwnershipTest extends \Test\TestCase { */ public function testTransferBoardOwnership() { $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); + $this->invokePrivate($this->boardService, 'clearBoardsCache'); $board = $this->boardService->find($this->board->getId()); $boardOwner = $board->getOwner(); $this->assertEquals(self::TEST_USER_2, $boardOwner); @@ -94,10 +95,10 @@ class TransferOwnershipTest extends \Test\TestCase { $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_USER_2 && $item->getType() === Acl::PERMISSION_TYPE_USER; - }); - $this->assertTrue($isTargetInAcl); + // Check if old owner is no longer in ACL + $this->assertTrue((bool)array_filter($acl, function ($item) { + return $item->getParticipant() === self::TEST_USER_1 && $item->getType() === Acl::PERMISSION_TYPE_USER; + })); } /** From 4d3dabb94e153304f73f95ae17dcebe9eda66505 Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 9 Feb 2022 14:45:00 +0100 Subject: [PATCH 13/29] fix: Assignment is the new AssignedUsers Signed-off-by: Max --- lib/Db/AssignmentMapper.php | 4 ++-- .../database/TransferOwnershipTest.php | 22 +++++++++---------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/Db/AssignmentMapper.php b/lib/Db/AssignmentMapper.php index a5000184d..8d193fad0 100644 --- a/lib/Db/AssignmentMapper.php +++ b/lib/Db/AssignmentMapper.php @@ -155,7 +155,7 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { public function transferOwnership($ownerId, $newOwnerId) { $params = [ 'newOwner' => $newOwnerId, - 'type' => AssignedUsers::TYPE_USER + 'type' => Assignment::TYPE_USER ]; $qb = $this->db->getQueryBuilder(); $sql = "DELETE FROM `*PREFIX*{$this->tableName}` WHERE `participant` = :newOwner AND `type`= :type"; @@ -164,7 +164,7 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { $params = [ 'owner' => $ownerId, 'newOwner' => $newOwnerId, - 'type' => AssignedUsers::TYPE_USER + 'type' => Assignment::TYPE_USER ]; $sql = "UPDATE `*PREFIX*{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner AND `type`= :type"; $stmt = $this->db->executeQuery($sql, $params); diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php index fbfab856f..2b1e30403 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -3,8 +3,8 @@ namespace OCA\Deck\Service; use OCA\Deck\Db\Acl; -use OCA\Deck\Db\AssignedUsers; -use OCA\Deck\Db\AssignedUsersMapper; +use OCA\Deck\Db\Assignment; +use OCA\Deck\Db\AssignmentMapper; use OCA\Deck\Db\Board; /** @@ -23,8 +23,8 @@ class TransferOwnershipTest extends \Test\TestCase { protected $cardService; /** @var StackService */ protected $stackService; - /** @var AssignedUsersMapper */ - protected $assignedUsersMapper; + /** @var AssignmentMapper */ + protected $assignmentMapper; /** @var AssignmentService */ private $assignmentService; /** @var Board */ @@ -55,7 +55,7 @@ class TransferOwnershipTest extends \Test\TestCase { $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->assignmentMapper = \OC::$server->query(AssignmentMapper::class); $this->createBoardWithExampleData(); } @@ -128,9 +128,9 @@ class TransferOwnershipTest extends \Test\TestCase { */ public function testReassignCardToNewOwner() { $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); - $assignedUsers = $this->assignedUsersMapper->find($this->cards[0]->getId()); + $users = $this->assignmentMapper->findAll($this->cards[0]->getId()); $participantsUIDs = []; - foreach ($assignedUsers as $user) { + foreach ($users as $user) { $participantsUIDs[] = $user->getParticipant(); } $this->assertContains(self::TEST_USER_2, $participantsUIDs); @@ -141,11 +141,11 @@ class TransferOwnershipTest extends \Test\TestCase { * @covers ::transferOwnership */ public function testReassignCardToNewParticipantOnlyIfParticipantHasUserType() { - $this->assignmentService->assignUser($this->cards[1]->getId(), self::TEST_USER_1, AssignedUsers::TYPE_GROUP); + $this->assignmentService->assignUser($this->cards[1]->getId(), self::TEST_USER_1, Assignment::TYPE_GROUP); $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); - $assignedUsers = $this->assignedUsersMapper->find($this->cards[1]->getId()); + $users = $this->assignmentMapper->findAll($this->cards[1]->getId()); $participantsUIDs = []; - foreach ($assignedUsers as $user) { + foreach ($users as $user) { $participantsUIDs[] = $user->getParticipant(); } $this->assertContains(self::TEST_USER_1, $participantsUIDs); @@ -196,7 +196,7 @@ class TransferOwnershipTest extends \Test\TestCase { */ public function testTargetAlreadyParticipantOfCard() { $this->expectNotToPerformAssertions(); - $this->assignmentService->assignUser($this->cards[0]->getId(), self::TEST_USER_3, AssignedUsers::TYPE_USER); + $this->assignmentService->assignUser($this->cards[0]->getId(), self::TEST_USER_3, Assignment::TYPE_USER); $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3); } From b6340e54c308dc8571a93e900035214f5067d2b9 Mon Sep 17 00:00:00 2001 From: Max Date: Wed, 9 Feb 2022 15:38:33 +0100 Subject: [PATCH 14/29] fix: queries with the new base mapper in BoardMapper Signed-off-by: Max --- lib/Db/BoardMapper.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Db/BoardMapper.php b/lib/Db/BoardMapper.php index fc5455ef4..aea517e3b 100644 --- a/lib/Db/BoardMapper.php +++ b/lib/Db/BoardMapper.php @@ -486,8 +486,8 @@ class BoardMapper extends QBMapper implements IPermissionMapper { 'owner' => $ownerId, 'newOwner' => $newOwnerId ]; - $sql = "UPDATE `{$this->tableName}` SET `owner` = :newOwner WHERE `owner` = :owner"; - $stmt = $this->execute($sql, $params); + $sql = "UPDATE `*PREFIX*{$this->tableName}` SET `owner` = :newOwner WHERE `owner` = :owner"; + $stmt = $this->db->executeQuery($sql, $params); $stmt->closeCursor(); } } From afbbdf0c1be99dce3e67531d6eedc78860c237a3 Mon Sep 17 00:00:00 2001 From: Luka Trovic Date: Fri, 4 Mar 2022 18:06:08 +0100 Subject: [PATCH 15/29] fix: unit test & psalm static code analysis issues Signed-off-by: Luka Trovic --- lib/Command/TransferOwnership.php | 4 +- lib/Db/AssignmentMapper.php | 3 +- tests/unit/Activity/ActivityManagerTest.php | 16 +- tests/unit/Cron/DeleteCronTest.php | 18 +-- tests/unit/Cron/ScheduledNoificationsTest.php | 5 +- .../Notification/NotificationHelperTest.php | 149 +++++++----------- tests/unit/Service/AttachmentServiceTest.php | 92 +++++++++-- tests/unit/Service/BoardServiceTest.php | 39 +++-- tests/unit/Service/PermissionServiceTest.php | 29 ++-- 9 files changed, 200 insertions(+), 155 deletions(-) diff --git a/lib/Command/TransferOwnership.php b/lib/Command/TransferOwnership.php index 2b06c4e1c..9d3ae1b48 100644 --- a/lib/Command/TransferOwnership.php +++ b/lib/Command/TransferOwnership.php @@ -33,7 +33,7 @@ final class TransferOwnership extends Command { ); } - protected function execute(InputInterface $input, OutputInterface $output) { + protected function execute(InputInterface $input, OutputInterface $output): int { $owner = $input->getArgument('owner'); $newOwner = $input->getArgument('newOwner'); @@ -42,5 +42,7 @@ final class TransferOwnership extends Command { $this->boardService->transferOwnership($owner, $newOwner); $output->writeln("Transfer deck boards from $owner to $newOwner completed"); + + return 0; } } diff --git a/lib/Db/AssignmentMapper.php b/lib/Db/AssignmentMapper.php index 8d193fad0..836add105 100644 --- a/lib/Db/AssignmentMapper.php +++ b/lib/Db/AssignmentMapper.php @@ -148,11 +148,12 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { } /** + * @psalm-suppress InvalidScalarArgument * @param $ownerId * @param $newOwnerId * @return void */ - public function transferOwnership($ownerId, $newOwnerId) { + public function transferOwnership(string $ownerId, string $newOwnerId) { $params = [ 'newOwner' => $newOwnerId, 'type' => Assignment::TYPE_USER diff --git a/tests/unit/Activity/ActivityManagerTest.php b/tests/unit/Activity/ActivityManagerTest.php index 5e486731e..d292d9023 100644 --- a/tests/unit/Activity/ActivityManagerTest.php +++ b/tests/unit/Activity/ActivityManagerTest.php @@ -169,18 +169,15 @@ class ActivityManagerTest extends TestCase { $this->mockUser('user2'), ]; $event = $this->createMock(IEvent::class); - $event->expects($this->at(0)) + $event->expects($this->once()) ->method('getObjectType') ->willReturn($objectType); - $event->expects($this->at(0)) + $event->expects($this->once()) ->method('getObjectId') ->willReturn(1); - $event->expects($this->at(2)) + $event->expects($this->exactly(2)) ->method('setAffectedUser') - ->with('user1'); - $event->expects($this->at(3)) - ->method('setAffectedUser') - ->with('user2'); + ->withConsecutive(['user1'], ['user2']); $mapper = null; switch ($objectType) { case ActivityManager::DECK_OBJECT_BOARD: @@ -196,10 +193,7 @@ class ActivityManagerTest extends TestCase { $this->permissionService->expects($this->once()) ->method('findUsers') ->willReturn($users); - $this->manager->expects($this->at(0)) - ->method('publish') - ->with($event); - $this->manager->expects($this->at(1)) + $this->manager->expects($this->exactly(2)) ->method('publish') ->with($event); $this->invokePrivate($this->activityManager, 'sendToUsers', [$event]); diff --git a/tests/unit/Cron/DeleteCronTest.php b/tests/unit/Cron/DeleteCronTest.php index e7ba1c6b9..e411a5d31 100644 --- a/tests/unit/Cron/DeleteCronTest.php +++ b/tests/unit/Cron/DeleteCronTest.php @@ -66,18 +66,14 @@ class DeleteCronTest extends \Test\TestCase { $this->boardMapper->expects($this->once()) ->method('findToDelete') ->willReturn($boards); - $this->boardMapper->expects($this->at(1)) + $this->boardMapper->expects($this->exactly(count($boards))) ->method('delete') - ->with($boards[0]); - $this->boardMapper->expects($this->at(2)) - ->method('delete') - ->with($boards[1]); - $this->boardMapper->expects($this->at(3)) - ->method('delete') - ->with($boards[2]); - $this->boardMapper->expects($this->at(4)) - ->method('delete') - ->with($boards[3]); + ->withConsecutive( + [$boards[0]], + [$boards[1]], + [$boards[2]], + [$boards[3]] + ); $attachment = new Attachment(); $attachment->setType('deck_file'); diff --git a/tests/unit/Cron/ScheduledNoificationsTest.php b/tests/unit/Cron/ScheduledNoificationsTest.php index ff7aad6a1..f44b91fa1 100644 --- a/tests/unit/Cron/ScheduledNoificationsTest.php +++ b/tests/unit/Cron/ScheduledNoificationsTest.php @@ -54,10 +54,7 @@ class ScheduledNoificationsTest extends \Test\TestCase { $this->cardMapper->expects($this->once()) ->method('findOverdue') ->willReturn($cards); - $this->notificationHelper->expects($this->at(0)) - ->method('sendCardDuedate') - ->with($c1); - $this->notificationHelper->expects($this->at(1)) + $this->notificationHelper->expects($this->exactly(2)) ->method('sendCardDuedate') ->with($c1); $this->scheduledNotifications->run(null); diff --git a/tests/unit/Notification/NotificationHelperTest.php b/tests/unit/Notification/NotificationHelperTest.php index 0d5b2527c..c4f0fd9cd 100644 --- a/tests/unit/Notification/NotificationHelperTest.php +++ b/tests/unit/Notification/NotificationHelperTest.php @@ -114,17 +114,18 @@ class NotificationHelperTest extends \Test\TestCase { } public function testSendCardDuedate() { - $this->config->expects($this->at(0)) + $param1 = ['foo', 'bar', 'asd']; + $param2 = 'deck'; + $param3 = 'board:234:notify-due'; + $DUE_ASSIGNED = ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED; + + $this->config->expects($this->exactly(3)) ->method('getUserValue') - ->with('foo', 'deck', 'board:234:notify-due', ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED) - ->willReturn(ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ALL); - $this->config->expects($this->at(1)) - ->method('getUserValue') - ->with('bar', 'deck', 'board:234:notify-due', ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED) - ->willReturn(ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ALL); - $this->config->expects($this->at(2)) - ->method('getUserValue') - ->with('asd', 'deck', 'board:234:notify-due', ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED) + ->withConsecutive( + [$param1[0], $param2, $param3, $DUE_ASSIGNED], + [$param1[1], $param2, $param3, $DUE_ASSIGNED], + [$param1[2], $param2, $param3, $DUE_ASSIGNED], + ) ->willReturn(ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ALL); $card = Card::fromParams([ @@ -180,24 +181,12 @@ class NotificationHelperTest extends \Test\TestCase { $n3->expects($this->once())->method('setSubject')->with('card-overdue', ['MyCardTitle', 'MyBoardTitle'])->willReturn($n3); $n3->expects($this->once())->method('setDateTime')->willReturn($n3); - $this->notificationManager->expects($this->at(0)) + $this->notificationManager->expects($this->exactly(3)) ->method('createNotification') - ->willReturn($n1); - $this->notificationManager->expects($this->at(1)) + ->willReturnOnConsecutiveCalls($n1, $n2, $n3); + $this->notificationManager->expects($this->exactly(3)) ->method('notify') - ->with($n1); - $this->notificationManager->expects($this->at(2)) - ->method('createNotification') - ->willReturn($n2); - $this->notificationManager->expects($this->at(3)) - ->method('notify') - ->with($n2); - $this->notificationManager->expects($this->at(4)) - ->method('createNotification') - ->willReturn($n3); - $this->notificationManager->expects($this->at(5)) - ->method('notify') - ->with($n3); + ->withConsecutive([$n1], [$n2], [$n3]); $this->cardMapper->expects($this->once()) ->method('markNotified') @@ -207,18 +196,19 @@ class NotificationHelperTest extends \Test\TestCase { } public function testSendCardDuedateAssigned() { - $this->config->expects($this->at(0)) + $param1 = ['foo', 'bar', 'asd']; + $param2 = 'deck'; + $param3 = 'board:234:notify-due'; + $DUE_ASSIGNED = ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED; + + $this->config->expects($this->exactly(3)) ->method('getUserValue') - ->with('foo', 'deck', 'board:234:notify-due', ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED) - ->willReturn(ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED); - $this->config->expects($this->at(1)) - ->method('getUserValue') - ->with('bar', 'deck', 'board:234:notify-due', ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED) - ->willReturn(ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED); - $this->config->expects($this->at(2)) - ->method('getUserValue') - ->with('asd', 'deck', 'board:234:notify-due', ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED) - ->willReturn(ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED); + ->withConsecutive( + [$param1[0], $param2, $param3, $DUE_ASSIGNED], + [$param1[1], $param2, $param3, $DUE_ASSIGNED], + [$param1[2], $param2, $param3, $DUE_ASSIGNED] + ) + ->willReturn($DUE_ASSIGNED); $users = [ new DummyUser('foo'), new DummyUser('bar'), new DummyUser('asd') @@ -278,24 +268,12 @@ class NotificationHelperTest extends \Test\TestCase { $n3->expects($this->once())->method('setSubject')->with('card-overdue', ['MyCardTitle', 'MyBoardTitle'])->willReturn($n3); $n3->expects($this->once())->method('setDateTime')->willReturn($n3); - $this->notificationManager->expects($this->at(0)) + $this->notificationManager->expects($this->exactly(3)) ->method('createNotification') - ->willReturn($n1); - $this->notificationManager->expects($this->at(1)) + ->willReturnOnConsecutiveCalls($n1, $n2, $n3); + $this->notificationManager->expects($this->exactly(3)) ->method('notify') - ->with($n1); - $this->notificationManager->expects($this->at(2)) - ->method('createNotification') - ->willReturn($n2); - $this->notificationManager->expects($this->at(3)) - ->method('notify') - ->with($n2); - $this->notificationManager->expects($this->at(4)) - ->method('createNotification') - ->willReturn($n3); - $this->notificationManager->expects($this->at(5)) - ->method('notify') - ->with($n3); + ->withConsecutive([$n1], [$n2], [$n3]); $this->cardMapper->expects($this->once()) ->method('markNotified') @@ -306,18 +284,20 @@ class NotificationHelperTest extends \Test\TestCase { public function testSendCardDuedateNever() { - $this->config->expects($this->at(0)) + $param1 = ['foo', 'bar', 'asd']; + $param2 = 'deck'; + $param3 = 'board:234:notify-due'; + $DUE_ASSIGNED = ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED; + $DUE_OFF = ConfigService::SETTING_BOARD_NOTIFICATION_DUE_OFF; + + $this->config->expects($this->exactly(3)) ->method('getUserValue') - ->with('foo', 'deck', 'board:234:notify-due', ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED) - ->willReturn(ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED); - $this->config->expects($this->at(1)) - ->method('getUserValue') - ->with('bar', 'deck', 'board:234:notify-due', ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED) - ->willReturn(ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED); - $this->config->expects($this->at(2)) - ->method('getUserValue') - ->with('asd', 'deck', 'board:234:notify-due', ConfigService::SETTING_BOARD_NOTIFICATION_DUE_ASSIGNED) - ->willReturn(ConfigService::SETTING_BOARD_NOTIFICATION_DUE_OFF); + ->withConsecutive( + [$param1[0], $param2, $param3, $DUE_ASSIGNED], + [$param1[1], $param2, $param3, $DUE_ASSIGNED], + [$param1[2], $param2, $param3, $DUE_ASSIGNED] + ) + ->willReturnOnConsecutiveCalls($DUE_ASSIGNED, $DUE_ASSIGNED, $DUE_OFF); $users = [ new DummyUser('foo'), new DummyUser('bar'), new DummyUser('asd') @@ -370,18 +350,12 @@ class NotificationHelperTest extends \Test\TestCase { $n2->expects($this->once())->method('setSubject')->with('card-overdue', ['MyCardTitle', 'MyBoardTitle'])->willReturn($n2); $n2->expects($this->once())->method('setDateTime')->willReturn($n2); - $this->notificationManager->expects($this->at(0)) + $this->notificationManager->expects($this->exactly(2)) ->method('createNotification') - ->willReturn($n1); - $this->notificationManager->expects($this->at(1)) + ->willReturnOnConsecutiveCalls($n1, $n2); + $this->notificationManager->expects($this->exactly(2)) ->method('notify') - ->with($n1); - $this->notificationManager->expects($this->at(2)) - ->method('createNotification') - ->willReturn($n2); - $this->notificationManager->expects($this->at(3)) - ->method('notify') - ->with($n2); + ->withConsecutive([$n1], [$n2]); $this->cardMapper->expects($this->once()) ->method('markNotified') @@ -423,10 +397,10 @@ class NotificationHelperTest extends \Test\TestCase { $notification->expects($this->once())->method('setSubject')->with('card-assigned', ['MyCardTitle', 'MyBoardTitle', 'admin'])->willReturn($notification); $notification->expects($this->once())->method('setDateTime')->willReturn($notification); - $this->notificationManager->expects($this->at(0)) + $this->notificationManager->expects($this->once()) ->method('createNotification') ->willReturn($notification); - $this->notificationManager->expects($this->at(1)) + $this->notificationManager->expects($this->once()) ->method('notify') ->with($notification); @@ -451,10 +425,10 @@ class NotificationHelperTest extends \Test\TestCase { $notification->expects($this->once())->method('setSubject')->with('board-shared', ['MyBoardTitle', 'admin'])->willReturn($notification); $notification->expects($this->once())->method('setDateTime')->willReturn($notification); - $this->notificationManager->expects($this->at(0)) + $this->notificationManager->expects($this->once()) ->method('createNotification') ->willReturn($notification); - $this->notificationManager->expects($this->at(1)) + $this->notificationManager->expects($this->once()) ->method('notify') ->with($notification); @@ -490,10 +464,10 @@ class NotificationHelperTest extends \Test\TestCase { $notification->expects($this->once())->method('setSubject')->with('board-shared', ['MyBoardTitle', 'admin'])->willReturn($notification); $notification->expects($this->once())->method('setDateTime')->willReturn($notification); - $this->notificationManager->expects($this->at(0)) + $this->notificationManager->expects($this->once()) ->method('createNotification') ->willReturn($notification); - $this->notificationManager->expects($this->at(1)) + $this->notificationManager->expects($this->once()) ->method('notify') ->with($notification); @@ -540,19 +514,12 @@ class NotificationHelperTest extends \Test\TestCase { $notification2->expects($this->once())->method('setSubject')->with('card-comment-mentioned', ['MyCard', 1, 'admin'])->willReturn($notification2); $notification2->expects($this->once())->method('setDateTime')->willReturn($notification2); - $this->notificationManager->expects($this->at(0)) + $this->notificationManager->expects($this->exactly(2)) ->method('createNotification') - ->willReturn($notification1); - $this->notificationManager->expects($this->at(1)) + ->willReturnOnConsecutiveCalls($notification1, $notification2); + $this->notificationManager->expects($this->exactly(2)) ->method('notify') - ->with($notification1); - - $this->notificationManager->expects($this->at(2)) - ->method('createNotification') - ->willReturn($notification2); - $this->notificationManager->expects($this->at(3)) - ->method('notify') - ->with($notification2); + ->withConsecutive([$notification1], [$notification2]); $this->notificationHelper->sendMention($comment); } diff --git a/tests/unit/Service/AttachmentServiceTest.php b/tests/unit/Service/AttachmentServiceTest.php index 5fa5d73cc..b236c55d2 100644 --- a/tests/unit/Service/AttachmentServiceTest.php +++ b/tests/unit/Service/AttachmentServiceTest.php @@ -110,8 +110,22 @@ class AttachmentServiceTest extends TestCase { $this->cache = $this->createMock(ICache::class); $this->cacheFactory->expects($this->any())->method('createDistributed')->willReturn($this->cache); - $this->appContainer->expects($this->at(0))->method('query')->with(FileService::class)->willReturn($this->attachmentServiceImpl); - $this->appContainer->expects($this->at(1))->method('query')->with(FilesAppService::class)->willReturn($this->filesAppServiceImpl); + $this->appContainer->expects($this->exactly(2)) + ->method('query') + ->withConsecutive( + [FileService::class], + [FilesAppService::class] + ) + ->willReturnOnConsecutiveCalls($this->attachmentServiceImpl, $this->filesAppServiceImpl); + + /* $this->appContainer->expects($this->at(0)) + ->method('query') + ->with(FileService::class) + ->willReturn($this->attachmentServiceImpl); + $this->appContainer->expects($this->at(1)) + ->method('query') + ->with(FilesAppService::class) + ->willReturn($this->filesAppServiceImpl); */ $this->application->expects($this->any()) ->method('getContainer') @@ -129,9 +143,27 @@ class AttachmentServiceTest extends TestCase { $fileServiceMock = $this->createMock(FileService::class); $fileAppServiceMock = $this->createMock(FilesAppService::class); - $appContainer->expects($this->at(0))->method('query')->with(FileService::class)->willReturn($fileServiceMock); - $appContainer->expects($this->at(1))->method('query')->with(FilesAppService::class)->willReturn($fileAppServiceMock); - $appContainer->expects($this->at(2))->method('query')->with(MyAttachmentService::class)->willReturn(new MyAttachmentService()); + $appContainer->expects($this->exactly(3)) + ->method('query') + ->withConsecutive( + [FileService::class], + [FilesAppService::class], + [MyAttachmentService::class] + ) + ->willReturnOnConsecutiveCalls($fileServiceMock, $fileAppServiceMock, new MyAttachmentService()); + + /* $appContainer->expects($this->at(0)) + ->method('query') + ->with(FileService::class) + ->willReturn($fileServiceMock); + $appContainer->expects($this->at(1)) + ->method('query') + ->with(FilesAppService::class) + ->willReturn($fileAppServiceMock); + $appContainer->expects($this->at(2)) + ->method('query') + ->with(MyAttachmentService::class) + ->willReturn(new MyAttachmentService()); */ $application->expects($this->any()) ->method('getContainer') @@ -148,12 +180,32 @@ class AttachmentServiceTest extends TestCase { $appContainer = $this->createMock(IAppContainer::class); $fileServiceMock = $this->createMock(FileService::class); $fileAppServiceMock = $this->createMock(FilesAppService::class); - $appContainer->expects($this->at(0))->method('query')->with(FileService::class)->willReturn($fileServiceMock); - $appContainer->expects($this->at(1))->method('query')->with(FilesAppService::class)->willReturn($fileAppServiceMock); - $appContainer->expects($this->at(2))->method('query')->with(MyAttachmentService::class)->willReturn(new MyAttachmentService()); + + $appContainer->expects($this->exactly(3)) + ->method('query') + ->withConsecutive( + [FileService::class], + [FilesAppService::class], + [MyAttachmentService::class] + ) + ->willReturnOnConsecutiveCalls($fileServiceMock, $fileAppServiceMock, new MyAttachmentService()); + + /* $appContainer->expects($this->at(0)) + ->method('query') + ->with(FileService::class) + ->willReturn($fileServiceMock); + $appContainer->expects($this->at(1)) + ->method('query') + ->with(FilesAppService::class) + ->willReturn($fileAppServiceMock); + $appContainer->expects($this->at(2)) + ->method('query') + ->with(MyAttachmentService::class) + ->willReturn(new MyAttachmentService()); */ $application->expects($this->any()) ->method('getContainer') ->willReturn($appContainer); + $attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->changeHelper, $this->permissionService, $application, $this->cacheFactory, $this->userId, $this->l10n, $this->activityManager); $attachmentService->registerAttachmentService('custom', MyAttachmentService::class); $attachmentService->getService('deck_file_invalid'); @@ -185,12 +237,19 @@ class AttachmentServiceTest extends TestCase { ->with(123) ->willReturn($attachments); - $this->attachmentServiceImpl->expects($this->at(0)) + $this->attachmentServiceImpl->expects($this->exactly(2)) + ->method('extendData') + ->withConsecutive( + [$attachments[0]], + [$attachments[1]] + ); + + /* $this->attachmentServiceImpl->expects($this->at(0)) ->method('extendData') ->with($attachments[0]); $this->attachmentServiceImpl->expects($this->at(1)) ->method('extendData') - ->with($attachments[1]); + ->with($attachments[1]); */ $this->assertEquals($attachments, $this->attachmentService->findAll(123, false)); } @@ -215,12 +274,21 @@ class AttachmentServiceTest extends TestCase { ->with(123, false) ->willReturn($attachmentsDeleted); - $this->attachmentServiceImpl->expects($this->at(0)) + $this->attachmentServiceImpl->expects($this->exactly(4)) + ->method('extendData') + ->withConsecutive( + [$attachments[0]], + [$attachments[1]], + [$attachmentsDeleted[0]], + [$attachmentsDeleted[1]] + ); + + /* $this->attachmentServiceImpl->expects($this->at(0)) ->method('extendData') ->with($attachments[0]); $this->attachmentServiceImpl->expects($this->at(1)) ->method('extendData') - ->with($attachments[1]); + ->with($attachments[1]); */ $this->assertEquals(array_merge($attachments, $attachmentsDeleted), $this->attachmentService->findAll(123, true)); } diff --git a/tests/unit/Service/BoardServiceTest.php b/tests/unit/Service/BoardServiceTest.php index 600f6a37c..333b85116 100644 --- a/tests/unit/Service/BoardServiceTest.php +++ b/tests/unit/Service/BoardServiceTest.php @@ -300,22 +300,41 @@ class BoardServiceTest extends TestCase { $existingAcl->setPermissionEdit($currentUserAcl[0]); $existingAcl->setPermissionShare($currentUserAcl[1]); $existingAcl->setPermissionManage($currentUserAcl[2]); - $this->permissionService->expects($this->at(0)) - ->method('checkPermission') - ->with($this->boardMapper, 123, Acl::PERMISSION_SHARE, null); + if ($currentUserAcl[2]) { - $this->permissionService->expects($this->at(1)) + $this->permissionService->expects($this->exactly(2)) ->method('checkPermission') - ->with($this->boardMapper, 123, Acl::PERMISSION_MANAGE, null); + ->withConsecutive( + [$this->boardMapper, 123, Acl::PERMISSION_SHARE, null], + [$this->boardMapper, 123, Acl::PERMISSION_MANAGE, null] + ); } else { $this->aclMapper->expects($this->once()) ->method('findAll') ->willReturn([$existingAcl]); - $this->permissionService->expects($this->at(1)) + + $this->permissionService->expects($this->exactly(2)) ->method('checkPermission') - ->with($this->boardMapper, 123, Acl::PERMISSION_MANAGE, null) - ->willThrowException(new NoPermissionException('No permission')); - $this->permissionService->expects($this->at(2)) + ->withConsecutive( + [$this->boardMapper, 123, Acl::PERMISSION_SHARE, null], + [$this->boardMapper, 123, Acl::PERMISSION_MANAGE, null] + ) + ->will( + $this->onConsecutiveCalls( + true, + $this->throwException(new NoPermissionException('No permission')) + ) + ); + + $this->permissionService->expects($this->exactly(3)) + ->method('userCan') + ->willReturnOnConsecutiveCalls( + $currentUserAcl[0], + $currentUserAcl[1], + $currentUserAcl[2] + ); + + /* $this->permissionService->expects($this->at(2)) ->method('userCan') ->willReturn($currentUserAcl[0]); $this->permissionService->expects($this->at(3)) @@ -323,7 +342,7 @@ class BoardServiceTest extends TestCase { ->willReturn($currentUserAcl[1]); $this->permissionService->expects($this->at(4)) ->method('userCan') - ->willReturn($currentUserAcl[2]); + ->willReturn($currentUserAcl[2]); */ } $user = $this->createMock(IUser::class); diff --git a/tests/unit/Service/PermissionServiceTest.php b/tests/unit/Service/PermissionServiceTest.php index f92adc059..0994a778e 100644 --- a/tests/unit/Service/PermissionServiceTest.php +++ b/tests/unit/Service/PermissionServiceTest.php @@ -139,13 +139,17 @@ class PermissionServiceTest extends \Test\TestCase { } public function testUserIsBoardOwner() { - $board = new Board(); - $board->setOwner('admin'); - $this->boardMapper->expects($this->at(0))->method('find')->with(123)->willReturn($board); + $adminBoard = new Board(); + $adminBoard->setOwner('admin'); + $userBoard = new Board(); + $userBoard->setOwner('user1'); + + $this->boardMapper->expects($this->exactly(2)) + ->method('find') + ->withConsecutive([123], [234]) + ->willReturnOnConsecutiveCalls($adminBoard, $userBoard); + $this->assertEquals(true, $this->service->userIsBoardOwner(123)); - $board = new Board(); - $board->setOwner('user1'); - $this->boardMapper->expects($this->at(0))->method('find')->with(234)->willReturn($board); $this->assertEquals(false, $this->service->userIsBoardOwner(234)); } @@ -336,7 +340,7 @@ class PermissionServiceTest extends \Test\TestCase { $aclGroup->setParticipant('group1'); $board = $this->createMock(Board::class); - $board->expects($this->at(0)) + $board->expects($this->once()) ->method('__call') ->with('getOwner', []) ->willReturn('user1'); @@ -348,14 +352,11 @@ class PermissionServiceTest extends \Test\TestCase { ->method('find') ->with(123) ->willReturn($board); - $this->userManager->expects($this->at(0)) + $this->userManager->expects($this->exactly(2)) ->method('get') - ->with('user1') - ->willReturn($user1); - $this->userManager->expects($this->at(1)) - ->method('get') - ->with('user2') - ->willReturn($user2); + ->withConsecutive(['user1'], ['user2']) + ->willReturnOnConsecutiveCalls($user1, $user2); + $group = $this->createMock(IGroup::class); $group->expects($this->once()) ->method('getUsers') From 4615926e3b278a091961b329270684961c514564 Mon Sep 17 00:00:00 2001 From: Luka Trovic Date: Tue, 8 Mar 2022 10:27:06 +0100 Subject: [PATCH 16/29] fix: integration tests Signed-off-by: Luka Trovic --- lib/Db/AssignmentMapper.php | 14 +++++------ lib/Db/BoardMapper.php | 23 ++++++++++++++++++- lib/Service/BoardService.php | 18 +++++++++++++++ .../database/TransferOwnershipTest.php | 2 +- 4 files changed, 47 insertions(+), 10 deletions(-) diff --git a/lib/Db/AssignmentMapper.php b/lib/Db/AssignmentMapper.php index 836add105..222e556cc 100644 --- a/lib/Db/AssignmentMapper.php +++ b/lib/Db/AssignmentMapper.php @@ -154,19 +154,17 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { * @return void */ public function transferOwnership(string $ownerId, string $newOwnerId) { - $params = [ - 'newOwner' => $newOwnerId, - 'type' => Assignment::TYPE_USER - ]; - $qb = $this->db->getQueryBuilder(); - $sql = "DELETE FROM `*PREFIX*{$this->tableName}` WHERE `participant` = :newOwner AND `type`= :type"; - $stmt = $this->db->executeQuery($sql, $params); - $stmt->closeCursor(); $params = [ 'owner' => $ownerId, 'newOwner' => $newOwnerId, 'type' => Assignment::TYPE_USER ]; + $qb = $this->db->getQueryBuilder(); + $sql = "DELETE FROM `*PREFIX*{$this->tableName}` WHERE `participant` = :newOwner AND `type`= :type AND id IN + (SELECT id FROM `*PREFIX*{$this->tableName}` WHERE `participant` = :owner)"; + $stmt = $this->db->executeQuery($sql, $params); + $stmt->closeCursor(); + $sql = "UPDATE `*PREFIX*{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner AND `type`= :type"; $stmt = $this->db->executeQuery($sql, $params); $stmt->closeCursor(); diff --git a/lib/Db/BoardMapper.php b/lib/Db/BoardMapper.php index aea517e3b..4889cc56c 100644 --- a/lib/Db/BoardMapper.php +++ b/lib/Db/BoardMapper.php @@ -182,7 +182,7 @@ class BoardMapper extends QBMapper implements IPermissionMapper { // shared with user $qb->resetQueryParts(); - $qb->select('b.id', 'title', 'owner', 'color', 'archived', 'deleted_at', 'last_modified') + $qb->selectDistinct('b.id', 'title', 'owner', 'color', 'archived', 'deleted_at', 'last_modified') //->selectAlias('1', 'shared') ->from('deck_boards', 'b') ->innerJoin('b', 'deck_board_acl', 'acl', $qb->expr()->eq('b.id', 'acl.board_id')) @@ -490,4 +490,25 @@ class BoardMapper extends QBMapper implements IPermissionMapper { $stmt = $this->db->executeQuery($sql, $params); $stmt->closeCursor(); } + + /** + * Reset Cache for a + * given board or a given user + * + * @param int|null $boardId + * @param int|null $userId + */ + public function flushCache(?int $boardId = null, ?string $userId = null) + { + if ($boardId) { + unset($this->boardCache[$boardId]); + } else { + $this->boardCache = null; + } + if ($userId) { + unset($this->userBoardCache[$userId]); + } else { + $this->userBoardCache = null; + } + } } diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index 671354905..d7c6deb9a 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -528,6 +528,9 @@ class BoardService { $this->boardMapper->mapAcl($newAcl); $this->changeHelper->boardChanged($boardId); + $board = $this->find($boardId); + $this->clearBoardFromCache($board); + // TODO: use the dispatched event for this try { $resourceProvider = \OC::$server->query(\OCA\Deck\Collaboration\Resources\ResourceProvider::class); @@ -681,6 +684,7 @@ class BoardService { public function transferOwnership(string $owner, string $newOwner): void { $boards = $this->boardMapper->findAllByUser($owner); foreach ($boards as $board) { + $this->clearBoardFromCache($board); $this->aclMapper->transferOwnership($board->getId(), $owner, $newOwner); } $this->boardMapper->transferOwnership($owner, $newOwner); @@ -723,4 +727,18 @@ class BoardService { private function clearBoardsCache() { $this->boardsCache = null; } + + /** + * Clean a given board data + * from the Cache + * + * @param OCA\Deck\Db\Board $board + */ + private function clearBoardFromCache(Board $board) { + $boardId = $board->getId(); + $boardOwnerId = $board->getOwner(); + + $this->boardMapper->flushCache($boardId, $boardOwnerId); + unset($this->boardsCache[$boardId]); + } } diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php index 2b1e30403..2c549e9c0 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -82,7 +82,7 @@ class TransferOwnershipTest extends \Test\TestCase { */ public function testTransferBoardOwnership() { $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); - $this->invokePrivate($this->boardService, 'clearBoardsCache'); + /* $this->invokePrivate($this->boardService, 'clearBoardsCache'); */ $board = $this->boardService->find($this->board->getId()); $boardOwner = $board->getOwner(); $this->assertEquals(self::TEST_USER_2, $boardOwner); From e4551bde156a02f58c76fbe63db05366ec6834fe Mon Sep 17 00:00:00 2001 From: Luka Trovic Date: Thu, 10 Mar 2022 09:50:40 +0100 Subject: [PATCH 17/29] feat: add integration test for transferring board ownership with data Signed-off-by: Luka Trovic --- .../database/TransferOwnershipTest.php | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php index 2c549e9c0..7ffb4d135 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -6,6 +6,7 @@ use OCA\Deck\Db\Acl; use OCA\Deck\Db\Assignment; use OCA\Deck\Db\AssignmentMapper; use OCA\Deck\Db\Board; +use OCA\Deck\Db\Card; /** * @group DB @@ -88,6 +89,25 @@ class TransferOwnershipTest extends \Test\TestCase { $this->assertEquals(self::TEST_USER_2, $boardOwner); } + /** + * @covers ::transferOwnership + */ + public function testTransferBoardOwnershipWithData() + { + $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_USER_2, $boardOwner); + + $cards = $this->cards; + $newOwnerOwnsTheCards = (bool)array_product(array_filter($cards, function (Card $card) { + $cardUpdated = $this->cardService->find($card->getId()); + return $cardUpdated->getOwner() === self::TEST_USER_2; + })); + $this->assertTrue($newOwnerOwnsTheCards); + } + /** * @covers ::transferOwnership */ From 72134e6e95fdda2613834f4bfaea6acc9a52282d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 14 Mar 2022 08:34:02 +0100 Subject: [PATCH 18/29] fix: unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Db/BoardMapper.php | 6 +++--- lib/Service/BoardService.php | 4 ++-- lib/Service/PermissionService.php | 1 + tests/unit/Service/BoardServiceTest.php | 28 ++++++++++++------------- 4 files changed, 20 insertions(+), 19 deletions(-) diff --git a/lib/Db/BoardMapper.php b/lib/Db/BoardMapper.php index 4889cc56c..5cc5cbc00 100644 --- a/lib/Db/BoardMapper.php +++ b/lib/Db/BoardMapper.php @@ -182,7 +182,7 @@ class BoardMapper extends QBMapper implements IPermissionMapper { // shared with user $qb->resetQueryParts(); - $qb->selectDistinct('b.id', 'title', 'owner', 'color', 'archived', 'deleted_at', 'last_modified') + $qb->select('b.id', 'title', 'owner', 'color', 'archived', 'deleted_at', 'last_modified') //->selectAlias('1', 'shared') ->from('deck_boards', 'b') ->innerJoin('b', 'deck_board_acl', 'acl', $qb->expr()->eq('b.id', 'acl.board_id')) @@ -492,9 +492,9 @@ class BoardMapper extends QBMapper implements IPermissionMapper { } /** - * Reset Cache for a + * Reset Cache for a * given board or a given user - * + * * @param int|null $boardId * @param int|null $userId */ diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index d7c6deb9a..d09007390 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -528,7 +528,7 @@ class BoardService { $this->boardMapper->mapAcl($newAcl); $this->changeHelper->boardChanged($boardId); - $board = $this->find($boardId); + $board = $this->boardMapper->find($boardId); $this->clearBoardFromCache($board); // TODO: use the dispatched event for this @@ -731,7 +731,7 @@ class BoardService { /** * Clean a given board data * from the Cache - * + * * @param OCA\Deck\Db\Board $board */ private function clearBoardFromCache(Board $board) { diff --git a/lib/Service/PermissionService.php b/lib/Service/PermissionService.php index 498e8904c..e94d073df 100644 --- a/lib/Service/PermissionService.php +++ b/lib/Service/PermissionService.php @@ -241,6 +241,7 @@ class PermissionService { if (array_key_exists((string) $boardId, $this->users) && !$refresh) { return $this->users[(string) $boardId]; } + try { $board = $this->boardMapper->find($boardId); } catch (DoesNotExistException $e) { diff --git a/tests/unit/Service/BoardServiceTest.php b/tests/unit/Service/BoardServiceTest.php index 333b85116..0e8bfeb15 100644 --- a/tests/unit/Service/BoardServiceTest.php +++ b/tests/unit/Service/BoardServiceTest.php @@ -154,7 +154,7 @@ class BoardServiceTest extends TestCase { ->method('find') ->with(1) ->willReturn($b1); - $this->permissionService->expects($this->once()) + $this->permissionService->expects($this->any()) ->method('findUsers') ->willReturn([ 'admin' => 'admin', @@ -258,6 +258,11 @@ class BoardServiceTest extends TestCase { ->method('insert') ->with($acl) ->willReturn($acl); + $this->permissionService->expects($this->any()) + ->method('findUsers') + ->willReturn([ + 'admin' => 'admin', + ]); $this->assertEquals($acl, $this->service->addAcl( 123, 'user', 'admin', true, true, true )); @@ -306,7 +311,7 @@ class BoardServiceTest extends TestCase { ->method('checkPermission') ->withConsecutive( [$this->boardMapper, 123, Acl::PERMISSION_SHARE, null], - [$this->boardMapper, 123, Acl::PERMISSION_MANAGE, null] + [$this->boardMapper, 123, Acl::PERMISSION_MANAGE, null] ); } else { $this->aclMapper->expects($this->once()) @@ -329,20 +334,10 @@ class BoardServiceTest extends TestCase { $this->permissionService->expects($this->exactly(3)) ->method('userCan') ->willReturnOnConsecutiveCalls( - $currentUserAcl[0], - $currentUserAcl[1], + $currentUserAcl[0], + $currentUserAcl[1], $currentUserAcl[2] ); - - /* $this->permissionService->expects($this->at(2)) - ->method('userCan') - ->willReturn($currentUserAcl[0]); - $this->permissionService->expects($this->at(3)) - ->method('userCan') - ->willReturn($currentUserAcl[1]); - $this->permissionService->expects($this->at(4)) - ->method('userCan') - ->willReturn($currentUserAcl[2]); */ } $user = $this->createMock(IUser::class); @@ -357,6 +352,11 @@ class BoardServiceTest extends TestCase { $acl->resolveRelation('participant', function ($participant) use (&$user) { return null; }); + $this->permissionService->expects($this->any()) + ->method('findUsers') + ->willReturn([ + 'admin' => 'admin', + ]); $this->notificationHelper->expects($this->once()) ->method('sendBoardShared'); $expected = clone $acl; From b77409003262519904787bf29a7f533344a060d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 14 Mar 2022 08:37:22 +0100 Subject: [PATCH 19/29] fix: Psalm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- composer.json | 2 +- composer.lock | 72 +++++++++++++++++++++++++++++++----- lib/Db/BoardMapper.php | 6 +-- lib/Service/BoardService.php | 5 +-- 4 files changed, 66 insertions(+), 19 deletions(-) diff --git a/composer.json b/composer.json index d87af90d3..b3c9ee616 100644 --- a/composer.json +++ b/composer.json @@ -14,7 +14,7 @@ }, "require-dev": { "roave/security-advisories": "dev-master", - "christophwurst/nextcloud": "^21@dev", + "christophwurst/nextcloud": "dev-master", "phpunit/phpunit": "^9", "nextcloud/coding-standard": "^1.0.0", "symfony/event-dispatcher": "^4.0", diff --git a/composer.lock b/composer.lock index 45a248a27..e34fc1098 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "c3c765fb5379719b5ab8824e9fbb03f9", + "content-hash": "33c2fe0cccf29841e021001c6430310a", "packages": [ { "name": "cogpowered/finediff", @@ -301,25 +301,29 @@ }, { "name": "christophwurst/nextcloud", - "version": "v21.0.0", + "version": "dev-master", "source": { "type": "git", "url": "https://github.com/ChristophWurst/nextcloud_composer.git", - "reference": "41e1476b4aed5bce7371895054049eca353729c5" + "reference": "cd35b7f4519d9b1c836443ec5dcd973d7f0f9cdd" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/ChristophWurst/nextcloud_composer/zipball/41e1476b4aed5bce7371895054049eca353729c5", - "reference": "41e1476b4aed5bce7371895054049eca353729c5", + "url": "https://api.github.com/repos/ChristophWurst/nextcloud_composer/zipball/cd35b7f4519d9b1c836443ec5dcd973d7f0f9cdd", + "reference": "cd35b7f4519d9b1c836443ec5dcd973d7f0f9cdd", "shasum": "" }, "require": { - "php": "^7.3 || ~8.0.0" + "php": "^7.4 || ~8.0 || ~8.1", + "psr/container": "^1.0", + "psr/event-dispatcher": "^1.0", + "psr/log": "^1.1" }, + "default-branch": true, "type": "library", "extra": { "branch-alias": { - "dev-master": "21.0.0-dev" + "dev-master": "24.0.0-dev" } }, "notification-url": "https://packagist.org/downloads/", @@ -335,9 +339,9 @@ "description": "Composer package containing Nextcloud's public API (classes, interfaces)", "support": { "issues": "https://github.com/ChristophWurst/nextcloud_composer/issues", - "source": "https://github.com/ChristophWurst/nextcloud_composer/tree/v21.0.0" + "source": "https://github.com/ChristophWurst/nextcloud_composer/tree/master" }, - "time": "2021-03-01T08:42:25+00:00" + "time": "2022-03-11T01:33:55+00:00" }, { "name": "composer/package-versions-deprecated", @@ -2304,6 +2308,56 @@ }, "time": "2021-11-05T16:50:12+00:00" }, + { + "name": "psr/event-dispatcher", + "version": "1.0.0", + "source": { + "type": "git", + "url": "https://github.com/php-fig/event-dispatcher.git", + "reference": "dbefd12671e8a14ec7f180cab83036ed26714bb0" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/php-fig/event-dispatcher/zipball/dbefd12671e8a14ec7f180cab83036ed26714bb0", + "reference": "dbefd12671e8a14ec7f180cab83036ed26714bb0", + "shasum": "" + }, + "require": { + "php": ">=7.2.0" + }, + "type": "library", + "extra": { + "branch-alias": { + "dev-master": "1.0.x-dev" + } + }, + "autoload": { + "psr-4": { + "Psr\\EventDispatcher\\": "src/" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "PHP-FIG", + "homepage": "http://www.php-fig.org/" + } + ], + "description": "Standard interfaces for event handling.", + "keywords": [ + "events", + "psr", + "psr-14" + ], + "support": { + "issues": "https://github.com/php-fig/event-dispatcher/issues", + "source": "https://github.com/php-fig/event-dispatcher/tree/1.0.0" + }, + "time": "2019-01-08T18:20:26+00:00" + }, { "name": "psr/log", "version": "1.1.4", diff --git a/lib/Db/BoardMapper.php b/lib/Db/BoardMapper.php index 5cc5cbc00..81dc648bf 100644 --- a/lib/Db/BoardMapper.php +++ b/lib/Db/BoardMapper.php @@ -492,11 +492,7 @@ class BoardMapper extends QBMapper implements IPermissionMapper { } /** - * Reset Cache for a - * given board or a given user - * - * @param int|null $boardId - * @param int|null $userId + * Reset cache for a given board or a given user */ public function flushCache(?int $boardId = null, ?string $userId = null) { diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index d09007390..ceea63519 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -729,10 +729,7 @@ class BoardService { } /** - * Clean a given board data - * from the Cache - * - * @param OCA\Deck\Db\Board $board + * Clean a given board data from the Cache */ private function clearBoardFromCache(Board $board) { $boardId = $board->getId(); From a45e46f80a459a1d66fd7885ff6aa59fcd310521 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 14 Mar 2022 09:47:46 +0100 Subject: [PATCH 20/29] Allow transfer of single boards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Command/TransferOwnership.php | 51 +++++++++++++++++-- lib/Db/AclMapper.php | 2 +- lib/Db/AssignmentMapper.php | 4 +- lib/Db/BoardMapper.php | 24 ++++----- lib/Db/CardMapper.php | 7 +-- lib/Service/BoardService.php | 26 ++++++++-- .../database/TransferOwnershipTest.php | 35 ++++++++++--- tests/unit/Service/AttachmentServiceTest.php | 4 +- 8 files changed, 114 insertions(+), 39 deletions(-) diff --git a/lib/Command/TransferOwnership.php b/lib/Command/TransferOwnership.php index 9d3ae1b48..3b42132ca 100644 --- a/lib/Command/TransferOwnership.php +++ b/lib/Command/TransferOwnership.php @@ -4,17 +4,22 @@ namespace OCA\Deck\Command; use OCA\Deck\Service\BoardService; use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Helper\QuestionHelper; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; +use Symfony\Component\Console\Question\ConfirmationQuestion; final class TransferOwnership extends Command { protected $boardService; + protected $questionHelper; - public function __construct(BoardService $boardService) { + public function __construct(BoardService $boardService, QuestionHelper $questionHelper) { parent::__construct(); $this->boardService = $boardService; + $this->questionHelper = $questionHelper; } protected function configure() { @@ -30,18 +35,54 @@ final class TransferOwnership extends Command { 'newOwner', InputArgument::REQUIRED, 'New owner uid' - ); + ) + ->addArgument( + 'boardId', + InputArgument::OPTIONAL, + 'Single board ID' + ) + ->addOption( + 'remap', + 'r', + InputOption::VALUE_NONE, + 'Reassign card details of the old owner to the new one' + ) + ; } protected function execute(InputInterface $input, OutputInterface $output): int { $owner = $input->getArgument('owner'); $newOwner = $input->getArgument('newOwner'); + $boardId = $input->getArgument('boardId'); - $output->writeln("Transfer deck boards from $owner to $newOwner"); + $remapAssignment = $input->getOption('remap'); - $this->boardService->transferOwnership($owner, $newOwner); + $board = $boardId ? $this->boardService->find($boardId) : null; - $output->writeln("Transfer deck boards from $owner to $newOwner completed"); + if ($boardId !== null && $board->getOwner() !== $owner) { + $output->writeln("$owner is not the owner of the board $boardId (" . $board->getTitle() . ")"); + return 1; + } + + if ($boardId) { + $output->writeln("Transfer board " . $board->getTitle() . " from ". $board->getOwner() ." to $newOwner"); + } else { + $output->writeln("Transfer all boards from $owner to $newOwner"); + } + + $question = new ConfirmationQuestion('Do you really want to continue? (y/n) ', false); + if (!$this->questionHelper->ask($input, $output, $question)) { + return 1; + } + + if ($boardId) { + $this->boardService->transferBoardOwnership($boardId, $newOwner, $remapAssignment); + $output->writeln("Board " . $board->getTitle() . " from ". $board->getOwner() ." transferred to $newOwner completed"); + return 0; + } + + $this->boardService->transferOwnership($owner, $newOwner, $remapAssignment); + $output->writeln("All boards from $owner to $newOwner transferred"); return 0; } diff --git a/lib/Db/AclMapper.php b/lib/Db/AclMapper.php index c45a2d421..bc7784bdc 100644 --- a/lib/Db/AclMapper.php +++ b/lib/Db/AclMapper.php @@ -63,7 +63,7 @@ class AclMapper extends DeckMapper implements IPermissionMapper { * @param $newOwnerId * @return void */ - public function transferOwnership($boardId, $ownerId, $newOwnerId) { + public function transferOwnership($boardId, $newOwnerId) { $params = [ 'newOwner' => $newOwnerId, 'type' => Acl::PERMISSION_TYPE_USER, diff --git a/lib/Db/AssignmentMapper.php b/lib/Db/AssignmentMapper.php index 222e556cc..06c37e24d 100644 --- a/lib/Db/AssignmentMapper.php +++ b/lib/Db/AssignmentMapper.php @@ -153,7 +153,7 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { * @param $newOwnerId * @return void */ - public function transferOwnership(string $ownerId, string $newOwnerId) { + public function transferOwnership(string $ownerId, string $newOwnerId, int $boardId = null) { $params = [ 'owner' => $ownerId, 'newOwner' => $newOwnerId, @@ -164,7 +164,7 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { (SELECT id FROM `*PREFIX*{$this->tableName}` WHERE `participant` = :owner)"; $stmt = $this->db->executeQuery($sql, $params); $stmt->closeCursor(); - + $sql = "UPDATE `*PREFIX*{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner AND `type`= :type"; $stmt = $this->db->executeQuery($sql, $params); $stmt->closeCursor(); diff --git a/lib/Db/BoardMapper.php b/lib/Db/BoardMapper.php index 81dc648bf..93be76d7e 100644 --- a/lib/Db/BoardMapper.php +++ b/lib/Db/BoardMapper.php @@ -477,25 +477,23 @@ class BoardMapper extends QBMapper implements IPermissionMapper { } /** - * @param $ownerId - * @param $newOwnerId - * @return void + * @throws \OCP\DB\Exception */ - public function transferOwnership($ownerId, $newOwnerId) { - $params = [ - 'owner' => $ownerId, - 'newOwner' => $newOwnerId - ]; - $sql = "UPDATE `*PREFIX*{$this->tableName}` SET `owner` = :newOwner WHERE `owner` = :owner"; - $stmt = $this->db->executeQuery($sql, $params); - $stmt->closeCursor(); + public function transferOwnership(string $ownerId, string $newOwnerId, $boardId = null): void { + $qb = $this->db->getQueryBuilder(); + $qb->update('deck_boards') + ->set('owner', $qb->createNamedParameter($newOwnerId, IQueryBuilder::PARAM_STR)) + ->where($qb->expr()->eq('owner', $qb->createNamedParameter($ownerId, IQueryBuilder::PARAM_STR))); + if ($boardId !== null) { + $qb->andWhere($qb->expr()->eq('id', $qb->createNamedParameter($boardId, IQueryBuilder::PARAM_INT))); + } + $qb->executeStatement(); } /** * Reset cache for a given board or a given user */ - public function flushCache(?int $boardId = null, ?string $userId = null) - { + public function flushCache(?int $boardId = null, ?string $userId = null) { if ($boardId) { unset($this->boardCache[$boardId]); } else { diff --git a/lib/Db/CardMapper.php b/lib/Db/CardMapper.php index 973a1c736..471e2f9d7 100644 --- a/lib/Db/CardMapper.php +++ b/lib/Db/CardMapper.php @@ -587,12 +587,7 @@ class CardMapper extends QBMapper implements IPermissionMapper { }); } - /** - * @param $ownerId - * @param $newOwnerId - * @return void - */ - public function transferOwnership($ownerId, $newOwnerId) { + public function transferOwnership(string $ownerId, string $newOwnerId, int $boardId = null): void { $params = [ 'owner' => $ownerId, 'newOwner' => $newOwnerId diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index ceea63519..7ac663db7 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -681,15 +681,33 @@ class BoardService { return $newBoard; } - public function transferOwnership(string $owner, string $newOwner): void { + public function transferBoardOwnership(int $boardId, string $newOwner, $changeContent = false): void { + $board = $this->boardMapper->find($boardId); + $previousOwner = $board->getOwner(); + $this->clearBoardFromCache($board); + $this->aclMapper->transferOwnership($boardId, $newOwner); + $this->boardMapper->transferOwnership($previousOwner, $newOwner, $boardId); + + // Optionally also change user assignments and card owner information + if ($changeContent) { + $this->assignedUsersMapper->transferOwnership($previousOwner, $newOwner, $boardId); + $this->cardMapper->transferOwnership($previousOwner, $newOwner, $boardId); + } + } + + public function transferOwnership(string $owner, string $newOwner, $changeContent = false): void { $boards = $this->boardMapper->findAllByUser($owner); foreach ($boards as $board) { $this->clearBoardFromCache($board); - $this->aclMapper->transferOwnership($board->getId(), $owner, $newOwner); + $this->aclMapper->transferOwnership($board->getId(), $newOwner); } $this->boardMapper->transferOwnership($owner, $newOwner); - $this->assignedUsersMapper->transferOwnership($owner, $newOwner); - $this->cardMapper->transferOwnership($owner, $newOwner); + + // Optionally also change user assignments and card owner information + if ($changeContent) { + $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 7ffb4d135..8b55642d3 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -10,7 +10,7 @@ use OCA\Deck\Db\Card; /** * @group DB - * @coversDefaultClass OCA\Deck\Service\BoardService + * @coversDefaultClass \OCA\Deck\Service\BoardService */ class TransferOwnershipTest extends \Test\TestCase { private const TEST_USER_1 = 'test-share-user1'; @@ -92,8 +92,7 @@ class TransferOwnershipTest extends \Test\TestCase { /** * @covers ::transferOwnership */ - public function testTransferBoardOwnershipWithData() - { + public function testTransferBoardOwnershipWithData() { $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); $board = $this->boardService->find($this->board->getId()); @@ -105,7 +104,7 @@ class TransferOwnershipTest extends \Test\TestCase { $cardUpdated = $this->cardService->find($card->getId()); return $cardUpdated->getOwner() === self::TEST_USER_2; })); - $this->assertTrue($newOwnerOwnsTheCards); + $this->assertTrue($newOwnerOwnsTheCards); } /** @@ -137,17 +136,27 @@ class TransferOwnershipTest extends \Test\TestCase { * @covers ::transferOwnership */ public function testTransferCardOwnership() { - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true); $card = $this->cardService->find($this->cards[0]->getId()); $cardOwner = $card->getOwner(); $this->assertEquals(self::TEST_USER_2, $cardOwner); } + /** + * @covers ::transferOwnership + */ + public function testTransferPreserveCardOwnership() { + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, false); + $card = $this->cardService->find($this->cards[0]->getId()); + $cardOwner = $card->getOwner(); + $this->assertEquals(self::TEST_USER_1, $cardOwner); + } + /** * @covers ::transferOwnership */ public function testReassignCardToNewOwner() { - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true); $users = $this->assignmentMapper->findAll($this->cards[0]->getId()); $participantsUIDs = []; foreach ($users as $user) { @@ -157,6 +166,20 @@ class TransferOwnershipTest extends \Test\TestCase { $this->assertNotContains(self::TEST_USER_1, $participantsUIDs); } + /** + * @covers ::transferOwnership + */ + public function testNoReassignCardToNewOwner() { + $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, false); + $users = $this->assignmentMapper->findAll($this->cards[0]->getId()); + $participantsUIDs = []; + foreach ($users as $user) { + $participantsUIDs[] = $user->getParticipant(); + } + $this->assertContains(self::TEST_USER_1, $participantsUIDs); + $this->assertNotContains(self::TEST_USER_2, $participantsUIDs); + } + /** * @covers ::transferOwnership */ diff --git a/tests/unit/Service/AttachmentServiceTest.php b/tests/unit/Service/AttachmentServiceTest.php index b236c55d2..9d12d1ab3 100644 --- a/tests/unit/Service/AttachmentServiceTest.php +++ b/tests/unit/Service/AttachmentServiceTest.php @@ -240,7 +240,7 @@ class AttachmentServiceTest extends TestCase { $this->attachmentServiceImpl->expects($this->exactly(2)) ->method('extendData') ->withConsecutive( - [$attachments[0]], + [$attachments[0]], [$attachments[1]] ); @@ -277,7 +277,7 @@ class AttachmentServiceTest extends TestCase { $this->attachmentServiceImpl->expects($this->exactly(4)) ->method('extendData') ->withConsecutive( - [$attachments[0]], + [$attachments[0]], [$attachments[1]], [$attachmentsDeleted[0]], [$attachmentsDeleted[1]] From a032287cb529ccc196f1b4b6fae209003aaf5c08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 14 Mar 2022 11:21:45 +0100 Subject: [PATCH 21/29] cleanup test cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- docs/User_documentation_en.md | 18 ++++- tests/unit/Service/AttachmentServiceTest.php | 70 ++++++-------------- 2 files changed, 36 insertions(+), 52 deletions(-) diff --git a/docs/User_documentation_en.md b/docs/User_documentation_en.md index 9aacbd82a..f68d87fe6 100644 --- a/docs/User_documentation_en.md +++ b/docs/User_documentation_en.md @@ -159,8 +159,22 @@ For example the search `project tag:ToDo assigned:alice assigned:bob` will retur Other text tokens will be used to perform a case-insensitive search on the card title and description -In addition wuotes can be used to pass a query with spaces, e.g. `"Exact match with spaces"` or `title:"My card"`. +In addition, quotes can be used to pass a query with spaces, e.g. `"Exact match with spaces"` or `title:"My card"`. ### 8. 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` + +```bash +php occ deck:transfer-ownership owner newOwner +``` + +The transfer will preserve card details linked to the old owner, which can also be remapped by using the `--remap` option on the occ command. +```bash +php occ deck:transfer-ownership --remap owner newOwner +``` + +Individual boards can be transferred by adding the id of the board to the command: + +```bash +php occ deck:transfer-ownership owner newOwner 123 +``` diff --git a/tests/unit/Service/AttachmentServiceTest.php b/tests/unit/Service/AttachmentServiceTest.php index 9d12d1ab3..3638cf7ea 100644 --- a/tests/unit/Service/AttachmentServiceTest.php +++ b/tests/unit/Service/AttachmentServiceTest.php @@ -116,16 +116,10 @@ class AttachmentServiceTest extends TestCase { [FileService::class], [FilesAppService::class] ) - ->willReturnOnConsecutiveCalls($this->attachmentServiceImpl, $this->filesAppServiceImpl); - - /* $this->appContainer->expects($this->at(0)) - ->method('query') - ->with(FileService::class) - ->willReturn($this->attachmentServiceImpl); - $this->appContainer->expects($this->at(1)) - ->method('query') - ->with(FilesAppService::class) - ->willReturn($this->filesAppServiceImpl); */ + ->willReturnOnConsecutiveCalls( + $this->attachmentServiceImpl, + $this->filesAppServiceImpl + ); $this->application->expects($this->any()) ->method('getContainer') @@ -150,20 +144,11 @@ class AttachmentServiceTest extends TestCase { [FilesAppService::class], [MyAttachmentService::class] ) - ->willReturnOnConsecutiveCalls($fileServiceMock, $fileAppServiceMock, new MyAttachmentService()); - - /* $appContainer->expects($this->at(0)) - ->method('query') - ->with(FileService::class) - ->willReturn($fileServiceMock); - $appContainer->expects($this->at(1)) - ->method('query') - ->with(FilesAppService::class) - ->willReturn($fileAppServiceMock); - $appContainer->expects($this->at(2)) - ->method('query') - ->with(MyAttachmentService::class) - ->willReturn(new MyAttachmentService()); */ + ->willReturnOnConsecutiveCalls( + $fileServiceMock, + $fileAppServiceMock, + new MyAttachmentService() + ); $application->expects($this->any()) ->method('getContainer') @@ -188,20 +173,12 @@ class AttachmentServiceTest extends TestCase { [FilesAppService::class], [MyAttachmentService::class] ) - ->willReturnOnConsecutiveCalls($fileServiceMock, $fileAppServiceMock, new MyAttachmentService()); + ->willReturnOnConsecutiveCalls( + $fileServiceMock, + $fileAppServiceMock, + new MyAttachmentService() + ); - /* $appContainer->expects($this->at(0)) - ->method('query') - ->with(FileService::class) - ->willReturn($fileServiceMock); - $appContainer->expects($this->at(1)) - ->method('query') - ->with(FilesAppService::class) - ->willReturn($fileAppServiceMock); - $appContainer->expects($this->at(2)) - ->method('query') - ->with(MyAttachmentService::class) - ->willReturn(new MyAttachmentService()); */ $application->expects($this->any()) ->method('getContainer') ->willReturn($appContainer); @@ -241,15 +218,13 @@ class AttachmentServiceTest extends TestCase { ->method('extendData') ->withConsecutive( [$attachments[0]], - [$attachments[1]] + [$attachments[1]], + ) + ->willReturnOnConsecutiveCalls( + $attachments[0], + $attachments[1], ); - /* $this->attachmentServiceImpl->expects($this->at(0)) - ->method('extendData') - ->with($attachments[0]); - $this->attachmentServiceImpl->expects($this->at(1)) - ->method('extendData') - ->with($attachments[1]); */ $this->assertEquals($attachments, $this->attachmentService->findAll(123, false)); } @@ -283,12 +258,6 @@ class AttachmentServiceTest extends TestCase { [$attachmentsDeleted[1]] ); - /* $this->attachmentServiceImpl->expects($this->at(0)) - ->method('extendData') - ->with($attachments[0]); - $this->attachmentServiceImpl->expects($this->at(1)) - ->method('extendData') - ->with($attachments[1]); */ $this->assertEquals(array_merge($attachments, $attachmentsDeleted), $this->attachmentService->findAll(123, true)); } @@ -464,5 +433,6 @@ class AttachmentServiceTest extends TestCase { ->method('allowUndo') ->willReturn(false); $actual = $this->attachmentService->restore(1, 1); + $this->assertEquals($expected, $actual); } } From c2144373d9bc252708d0756adb2000d19e34d94f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 14 Mar 2022 17:25:06 +0100 Subject: [PATCH 22/29] fix: Properly handle limited scope for remapping users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Db/AclMapper.php | 26 +++----- lib/Db/AssignmentMapper.php | 53 +++++++++------ lib/Db/CardMapper.php | 33 ++++++++++ lib/Service/BoardService.php | 36 ++++++---- tests/integration/config/behat.yml | 2 +- .../database/TransferOwnershipTest.php | 65 ++++++++++++++----- 6 files changed, 149 insertions(+), 66 deletions(-) diff --git a/lib/Db/AclMapper.php b/lib/Db/AclMapper.php index bc7784bdc..5cf188aa0 100644 --- a/lib/Db/AclMapper.php +++ b/lib/Db/AclMapper.php @@ -25,6 +25,7 @@ namespace OCA\Deck\Db; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; +use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; class AclMapper extends DeckMapper implements IPermissionMapper { @@ -59,23 +60,14 @@ class AclMapper extends DeckMapper implements IPermissionMapper { } /** - * @param $ownerId - * @param $newOwnerId - * @return void + * @throws \OCP\DB\Exception */ - public function transferOwnership($boardId, $newOwnerId) { - $params = [ - 'newOwner' => $newOwnerId, - 'type' => Acl::PERMISSION_TYPE_USER, - 'boardId' => $boardId - ]; - - // Drop existing ACL rules for the new owner - $sql = "DELETE FROM `{$this->tableName}` - WHERE `participant` = :newOwner - AND `type` = :type - AND `board_id` = :boardId"; - $stmt = $this->execute($sql, $params); - $stmt->closeCursor(); + public function deleteParticipantFromBoard(int $boardId, int $type, string $participant): void { + $qb = $this->db->getQueryBuilder(); + $qb->delete('deck_board_acl') + ->where($qb->expr()->eq('type', $qb->createNamedParameter($type, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->eq('participant', $qb->createNamedParameter($participant, IQueryBuilder::PARAM_STR))) + ->andWhere($qb->expr()->eq('board_id', $qb->createNamedParameter($boardId, IQueryBuilder::PARAM_INT))); + $qb->executeStatement(); } } diff --git a/lib/Db/AssignmentMapper.php b/lib/Db/AssignmentMapper.php index 06c37e24d..fdbf80d85 100644 --- a/lib/Db/AssignmentMapper.php +++ b/lib/Db/AssignmentMapper.php @@ -29,6 +29,7 @@ use OCA\Deck\NotFoundException; use OCA\Deck\Service\CirclesService; use OCP\AppFramework\Db\Entity; use OCP\AppFramework\Db\QBMapper; +use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; use OCP\IGroupManager; use OCP\IUserManager; @@ -147,26 +148,38 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper { return null; } - /** - * @psalm-suppress InvalidScalarArgument - * @param $ownerId - * @param $newOwnerId - * @return void - */ - public function transferOwnership(string $ownerId, string $newOwnerId, int $boardId = null) { - $params = [ - 'owner' => $ownerId, - 'newOwner' => $newOwnerId, - 'type' => Assignment::TYPE_USER - ]; - $qb = $this->db->getQueryBuilder(); - $sql = "DELETE FROM `*PREFIX*{$this->tableName}` WHERE `participant` = :newOwner AND `type`= :type AND id IN - (SELECT id FROM `*PREFIX*{$this->tableName}` WHERE `participant` = :owner)"; - $stmt = $this->db->executeQuery($sql, $params); - $stmt->closeCursor(); + public function remapAssignedUser(int $boardId, string $userId, string $newUserId): void { + $subQuery = $this->db->getQueryBuilder(); + $subQuery->selectAlias('a.id', 'id') + ->from('deck_assigned_users', 'a') + ->innerJoin('a', 'deck_cards', 'c', 'c.id = a.card_id') + ->innerJoin('c', 'deck_stacks', 's', 's.id = c.stack_id') + ->where($subQuery->expr()->eq('a.type', $subQuery->createNamedParameter(Assignment::TYPE_USER, IQueryBuilder::PARAM_INT))) + ->andWhere($subQuery->expr()->eq('a.participant', $subQuery->createNamedParameter($userId, IQueryBuilder::PARAM_STR))) + ->andWhere($subQuery->expr()->eq('s.board_id', $subQuery->createNamedParameter($boardId, IQueryBuilder::PARAM_INT))) + ->setMaxResults(1000); - $sql = "UPDATE `*PREFIX*{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner AND `type`= :type"; - $stmt = $this->db->executeQuery($sql, $params); - $stmt->closeCursor(); + $qb = $this->db->getQueryBuilder(); + $qb->update('deck_assigned_users') + ->set('participant', $qb->createParameter('participant')) + ->where($qb->expr()->in('id', $qb->createParameter('ids'))); + + $moreResults = true; + do { + $result = $subQuery->executeQuery(); + $ids = array_map(function ($item) { + return $item['id']; + }, $result->fetchAll()); + + if (count($ids) === 0 || $result->rowCount() === 0) { + $moreResults = false; + } + + $qb->setParameter('participant', $newUserId, IQueryBuilder::PARAM_STR); + $qb->setParameter('ids', $ids, IQueryBuilder::PARAM_INT_ARRAY); + $qb->executeStatement(); + } while ($moreResults === true); + + $result->closeCursor(); } } diff --git a/lib/Db/CardMapper.php b/lib/Db/CardMapper.php index 471e2f9d7..973a544f1 100644 --- a/lib/Db/CardMapper.php +++ b/lib/Db/CardMapper.php @@ -596,4 +596,37 @@ class CardMapper extends QBMapper implements IPermissionMapper { $stmt = $this->db->executeQuery($sql, $params); $stmt->closeCursor(); } + + public function remapCardOwner(int $boardId, string $userId, string $newUserId): void { + $subQuery = $this->db->getQueryBuilder(); + $subQuery->selectAlias('c.id', 'id') + ->from('deck_cards', 'c') + ->innerJoin('c', 'deck_stacks', 's', 's.id = c.stack_id') + ->where($subQuery->expr()->eq('c.owner', $subQuery->createNamedParameter($userId, IQueryBuilder::PARAM_STR))) + ->andWhere($subQuery->expr()->eq('s.board_id', $subQuery->createNamedParameter($boardId, IQueryBuilder::PARAM_INT))) + ->setMaxResults(1000); + + $qb = $this->db->getQueryBuilder(); + $qb->update('deck_cards') + ->set('owner', $qb->createParameter('owner')) + ->where($qb->expr()->in('id', $qb->createParameter('ids'))); + + $moreResults = true; + do { + $result = $subQuery->executeQuery(); + $ids = array_map(function ($item) { + return $item['id']; + }, $result->fetchAll()); + + if (count($ids) === 0 || $result->rowCount() === 0) { + $moreResults = false; + } + + $qb->setParameter('owner', $newUserId, IQueryBuilder::PARAM_STR); + $qb->setParameter('ids', $ids, IQueryBuilder::PARAM_INT_ARRAY); + $qb->executeStatement(); + } while ($moreResults === true); + + $result->closeCursor(); + } } diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index 7ac663db7..a8678f488 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -685,28 +685,38 @@ class BoardService { $board = $this->boardMapper->find($boardId); $previousOwner = $board->getOwner(); $this->clearBoardFromCache($board); - $this->aclMapper->transferOwnership($boardId, $newOwner); + $this->aclMapper->deleteParticipantFromBoard($boardId, Acl::PERMISSION_TYPE_USER, $newOwner); $this->boardMapper->transferOwnership($previousOwner, $newOwner, $boardId); // Optionally also change user assignments and card owner information if ($changeContent) { - $this->assignedUsersMapper->transferOwnership($previousOwner, $newOwner, $boardId); - $this->cardMapper->transferOwnership($previousOwner, $newOwner, $boardId); + $this->assignedUsersMapper->remapAssignedUser($boardId, $previousOwner, $newOwner); + $this->cardMapper->remapCardOwner($boardId, $previousOwner, $newOwner); } } public function transferOwnership(string $owner, string $newOwner, $changeContent = false): void { - $boards = $this->boardMapper->findAllByUser($owner); - foreach ($boards as $board) { - $this->clearBoardFromCache($board); - $this->aclMapper->transferOwnership($board->getId(), $newOwner); - } - $this->boardMapper->transferOwnership($owner, $newOwner); + \OC::$server->getDatabaseConnection()->beginTransaction(); + try { + $boards = $this->boardMapper->findAllByUser($owner); + foreach ($boards as $board) { + $this->clearBoardFromCache($board); + } - // Optionally also change user assignments and card owner information - if ($changeContent) { - $this->assignedUsersMapper->transferOwnership($owner, $newOwner); - $this->cardMapper->transferOwnership($owner, $newOwner); + $this->boardMapper->transferOwnership($owner, $newOwner); + // Optionally also change user assignments and card owner information + if ($changeContent) { + foreach ($boards as $board) { + $this->clearBoardFromCache($board); + $this->aclMapper->deleteParticipantFromBoard($board->getId(), Acl::PERMISSION_TYPE_USER, $newOwner); + $this->assignedUsersMapper->remapAssignedUser($board->getId(), $owner, $newOwner); + $this->cardMapper->remapCardOwner($board->getId(), $owner, $newOwner); + } + } + + \OC::$server->getDatabaseConnection()->commit(); + } catch (\Throwable $e) { + \OC::$server->getDatabaseConnection()->rollBack(); } } diff --git a/tests/integration/config/behat.yml b/tests/integration/config/behat.yml index 507138122..b8673a677 100644 --- a/tests/integration/config/behat.yml +++ b/tests/integration/config/behat.yml @@ -5,7 +5,7 @@ default: - '%paths.base%/../features/' contexts: - ServerContext: - baseUrl: http://localhost:9090/ + baseUrl: http://localhost:8080/ - RequestContext - BoardContext - CommentContext diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php index 8b55642d3..a7e4fbf9e 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -157,11 +157,9 @@ class TransferOwnershipTest extends \Test\TestCase { */ public function testReassignCardToNewOwner() { $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true); - $users = $this->assignmentMapper->findAll($this->cards[0]->getId()); - $participantsUIDs = []; - foreach ($users as $user) { - $participantsUIDs[] = $user->getParticipant(); - } + $participantsUIDs = array_map(function ($user) { + return $user->getParticipant(); + }, $this->assignmentMapper->findAll($this->cards[0]->getId())); $this->assertContains(self::TEST_USER_2, $participantsUIDs); $this->assertNotContains(self::TEST_USER_1, $participantsUIDs); } @@ -171,11 +169,9 @@ class TransferOwnershipTest extends \Test\TestCase { */ public function testNoReassignCardToNewOwner() { $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, false); - $users = $this->assignmentMapper->findAll($this->cards[0]->getId()); - $participantsUIDs = []; - foreach ($users as $user) { - $participantsUIDs[] = $user->getParticipant(); - } + $participantsUIDs = array_map(function ($user) { + return $user->getParticipant(); + }, $this->assignmentMapper->findAll($this->cards[0]->getId())); $this->assertContains(self::TEST_USER_1, $participantsUIDs); $this->assertNotContains(self::TEST_USER_2, $participantsUIDs); } @@ -186,11 +182,9 @@ class TransferOwnershipTest extends \Test\TestCase { public function testReassignCardToNewParticipantOnlyIfParticipantHasUserType() { $this->assignmentService->assignUser($this->cards[1]->getId(), self::TEST_USER_1, Assignment::TYPE_GROUP); $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); - $users = $this->assignmentMapper->findAll($this->cards[1]->getId()); - $participantsUIDs = []; - foreach ($users as $user) { - $participantsUIDs[] = $user->getParticipant(); - } + $participantsUIDs = array_map(function ($user) { + return $user->getParticipant(); + }, $this->assignmentMapper->findAll($this->cards[1]->getId())); $this->assertContains(self::TEST_USER_1, $participantsUIDs); $this->assertNotContains(self::TEST_USER_2, $participantsUIDs); } @@ -243,6 +237,47 @@ class TransferOwnershipTest extends \Test\TestCase { $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3); } + /** + * @covers ::transferOwnership + */ + public function testTransferSingleBoardAssignment() { + // Arrange separate board next to the one being transferred + $board = $this->boardService->create('Test 2', self::TEST_USER_1, '000000'); + $id = $board->getId(); + $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 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); + $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); + + // Act + $this->boardService->transferBoardOwnership($this->board->getId(), self::TEST_USER_2, true); + + // Assert that the selected board was transferred + $card = $this->cardService->find($this->cards[0]->getId()); + $this->assertEquals(self::TEST_USER_2, $card->getOwner()); + + $participantsUIDs = array_map(function ($assignment) { + return $assignment->getParticipant(); + }, $this->assignmentMapper->findAll($this->cards[0]->getId())); + $this->assertContains(self::TEST_USER_2, $participantsUIDs); + $this->assertNotContains(self::TEST_USER_1, $participantsUIDs); + + // Assert that other board remained unchanged + $card = $this->cardService->find($cards[0]->getId()); + $this->assertEquals(self::TEST_USER_1, $card->getOwner()); + + $participantsUIDs = array_map(function ($assignment) { + return $assignment->getParticipant(); + }, $this->assignmentMapper->findAll($cards[0]->getId())); + $this->assertContains(self::TEST_USER_1, $participantsUIDs); + $this->assertNotContains(self::TEST_USER_2, $participantsUIDs); + } + public function tearDown(): void { $this->boardService->deleteForce($this->board->getId()); parent::tearDown(); From 4f13977851f63cbe6f893c9cf3fa8a4f8cf82627 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 14 Mar 2022 17:56:07 +0100 Subject: [PATCH 23/29] Reuse single board transfer for all user boards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Command/TransferOwnership.php | 8 +++-- lib/Service/BoardService.php | 54 +++++++++++++------------------ 2 files changed, 27 insertions(+), 35 deletions(-) diff --git a/lib/Command/TransferOwnership.php b/lib/Command/TransferOwnership.php index 3b42132ca..715200f75 100644 --- a/lib/Command/TransferOwnership.php +++ b/lib/Command/TransferOwnership.php @@ -77,12 +77,14 @@ final class TransferOwnership extends Command { if ($boardId) { $this->boardService->transferBoardOwnership($boardId, $newOwner, $remapAssignment); - $output->writeln("Board " . $board->getTitle() . " from ". $board->getOwner() ." transferred to $newOwner completed"); + $output->writeln("Board " . $board->getTitle() . " from ". $board->getOwner() ." transferred to $newOwner completed"); return 0; } - $this->boardService->transferOwnership($owner, $newOwner, $remapAssignment); - $output->writeln("All boards from $owner to $newOwner transferred"); + foreach ($this->boardService->transferOwnership($owner, $newOwner, $remapAssignment) as $board) { + $output->writeln(" - " . $board->getTitle() . " transferred"); + } + $output->writeln("All boards from $owner to $newOwner transferred"); return 0; } diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index a8678f488..556408551 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -681,42 +681,32 @@ class BoardService { return $newBoard; } - public function transferBoardOwnership(int $boardId, string $newOwner, $changeContent = false): void { - $board = $this->boardMapper->find($boardId); - $previousOwner = $board->getOwner(); - $this->clearBoardFromCache($board); - $this->aclMapper->deleteParticipantFromBoard($boardId, Acl::PERMISSION_TYPE_USER, $newOwner); - $this->boardMapper->transferOwnership($previousOwner, $newOwner, $boardId); + public function transferBoardOwnership(int $boardId, string $newOwner, bool $changeContent = false): Board { + \OC::$server->getDatabaseConnection()->beginTransaction(); + try { + $board = $this->boardMapper->find($boardId); + $previousOwner = $board->getOwner(); + $this->clearBoardFromCache($board); + $this->aclMapper->deleteParticipantFromBoard($boardId, Acl::PERMISSION_TYPE_USER, $newOwner); + $this->boardMapper->transferOwnership($previousOwner, $newOwner, $boardId); - // Optionally also change user assignments and card owner information - if ($changeContent) { - $this->assignedUsersMapper->remapAssignedUser($boardId, $previousOwner, $newOwner); - $this->cardMapper->remapCardOwner($boardId, $previousOwner, $newOwner); + // Optionally also change user assignments and card owner information + if ($changeContent) { + $this->assignedUsersMapper->remapAssignedUser($boardId, $previousOwner, $newOwner); + $this->cardMapper->remapCardOwner($boardId, $previousOwner, $newOwner); + } + \OC::$server->getDatabaseConnection()->commit(); + return $this->boardMapper->find($boardId); + } catch (\Throwable $e) { + \OC::$server->getDatabaseConnection()->rollBack(); + throw $e; } } - public function transferOwnership(string $owner, string $newOwner, $changeContent = false): void { - \OC::$server->getDatabaseConnection()->beginTransaction(); - try { - $boards = $this->boardMapper->findAllByUser($owner); - foreach ($boards as $board) { - $this->clearBoardFromCache($board); - } - - $this->boardMapper->transferOwnership($owner, $newOwner); - // Optionally also change user assignments and card owner information - if ($changeContent) { - foreach ($boards as $board) { - $this->clearBoardFromCache($board); - $this->aclMapper->deleteParticipantFromBoard($board->getId(), Acl::PERMISSION_TYPE_USER, $newOwner); - $this->assignedUsersMapper->remapAssignedUser($board->getId(), $owner, $newOwner); - $this->cardMapper->remapCardOwner($board->getId(), $owner, $newOwner); - } - } - - \OC::$server->getDatabaseConnection()->commit(); - } catch (\Throwable $e) { - \OC::$server->getDatabaseConnection()->rollBack(); + public function transferOwnership(string $owner, string $newOwner, bool $changeContent = false): \Generator { + $boards = $this->boardMapper->findAllByUser($owner); + foreach ($boards as $board) { + yield $this->transferBoardOwnership($board->getId(), $newOwner, $changeContent); } } From 3a4ec07103ac67e1c27dbf6ef90a648bc49b9fa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 14 Mar 2022 19:12:04 +0100 Subject: [PATCH 24/29] fix: test cases using generator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- .../database/TransferOwnershipTest.php | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php index a7e4fbf9e..c7e9662d5 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -82,8 +82,7 @@ class TransferOwnershipTest extends \Test\TestCase { * @covers ::transferOwnership */ public function testTransferBoardOwnership() { - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); - /* $this->invokePrivate($this->boardService, 'clearBoardsCache'); */ + iterator_to_array($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_USER_2, $boardOwner); @@ -93,7 +92,7 @@ class TransferOwnershipTest extends \Test\TestCase { * @covers ::transferOwnership */ public function testTransferBoardOwnershipWithData() { - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2)); $board = $this->boardService->find($this->board->getId()); $boardOwner = $board->getOwner(); @@ -111,7 +110,7 @@ class TransferOwnershipTest extends \Test\TestCase { * @covers ::transferOwnership */ public function testTransferACLOwnership() { - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2)); $board = $this->boardService->find($this->board->getId()); $acl = $board->getAcl(); // Check if old owner is no longer in ACL @@ -124,7 +123,7 @@ class TransferOwnershipTest extends \Test\TestCase { * @covers ::transferOwnership */ public function testNoTransferAclOwnershipIfGroupType() { - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); + iterator_to_array($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) { @@ -136,7 +135,7 @@ class TransferOwnershipTest extends \Test\TestCase { * @covers ::transferOwnership */ public function testTransferCardOwnership() { - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true); + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true)); $card = $this->cardService->find($this->cards[0]->getId()); $cardOwner = $card->getOwner(); $this->assertEquals(self::TEST_USER_2, $cardOwner); @@ -146,7 +145,7 @@ class TransferOwnershipTest extends \Test\TestCase { * @covers ::transferOwnership */ public function testTransferPreserveCardOwnership() { - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, false); + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, false)); $card = $this->cardService->find($this->cards[0]->getId()); $cardOwner = $card->getOwner(); $this->assertEquals(self::TEST_USER_1, $cardOwner); @@ -156,7 +155,7 @@ class TransferOwnershipTest extends \Test\TestCase { * @covers ::transferOwnership */ public function testReassignCardToNewOwner() { - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true); + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true)); $participantsUIDs = array_map(function ($user) { return $user->getParticipant(); }, $this->assignmentMapper->findAll($this->cards[0]->getId())); @@ -168,7 +167,7 @@ class TransferOwnershipTest extends \Test\TestCase { * @covers ::transferOwnership */ public function testNoReassignCardToNewOwner() { - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, false); + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, false)); $participantsUIDs = array_map(function ($user) { return $user->getParticipant(); }, $this->assignmentMapper->findAll($this->cards[0]->getId())); @@ -181,7 +180,7 @@ class TransferOwnershipTest extends \Test\TestCase { */ public function testReassignCardToNewParticipantOnlyIfParticipantHasUserType() { $this->assignmentService->assignUser($this->cards[1]->getId(), self::TEST_USER_1, Assignment::TYPE_GROUP); - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2); + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2)); $participantsUIDs = array_map(function ($user) { return $user->getParticipant(); }, $this->assignmentMapper->findAll($this->cards[1]->getId())); @@ -194,14 +193,14 @@ class TransferOwnershipTest extends \Test\TestCase { */ public function testTargetAlreadyParticipantOfBoard() { $this->expectNotToPerformAssertions(); - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3); + iterator_to_array($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); + iterator_to_array($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) { @@ -215,7 +214,7 @@ class TransferOwnershipTest extends \Test\TestCase { */ 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); + iterator_to_array($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) { @@ -234,7 +233,7 @@ class TransferOwnershipTest extends \Test\TestCase { public function testTargetAlreadyParticipantOfCard() { $this->expectNotToPerformAssertions(); $this->assignmentService->assignUser($this->cards[0]->getId(), self::TEST_USER_3, Assignment::TYPE_USER); - $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3); + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3)); } /** From bf9a51d16746789614bcf9b7dabf9b2227d8e06f Mon Sep 17 00:00:00 2001 From: Luka Trovic Date: Mon, 14 Mar 2022 22:42:34 +0100 Subject: [PATCH 25/29] feat: add api endpoint and UI to transfer a board to a different user Signed-off-by: Luka Trovic --- appinfo/routes.php | 1 + lib/Controller/BoardController.php | 11 ++++++ lib/Service/BoardService.php | 6 ++++ src/components/board/SharingTabSidebar.vue | 39 +++++++++++++++++++++- src/store/main.js | 8 ++++- 5 files changed, 63 insertions(+), 2 deletions(-) diff --git a/appinfo/routes.php b/appinfo/routes.php index 8862eb015..b641e1073 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -39,6 +39,7 @@ return [ ['name' => 'board#updateAcl', 'url' => '/boards/{boardId}/acl/{aclId}', 'verb' => 'PUT'], ['name' => 'board#deleteAcl', 'url' => '/boards/{boardId}/acl/{aclId}', 'verb' => 'DELETE'], ['name' => 'board#clone', 'url' => '/boards/{boardId}/clone', 'verb' => 'POST'], + ['name' => 'board#transferOwner', 'url' => '/boards/{boardId}/transferOwner', 'verb' => 'PUT'], // stacks ['name' => 'stack#index', 'url' => '/stacks/{boardId}', 'verb' => 'GET'], diff --git a/lib/Controller/BoardController.php b/lib/Controller/BoardController.php index 9d0b9fcaf..086cf8a59 100644 --- a/lib/Controller/BoardController.php +++ b/lib/Controller/BoardController.php @@ -155,4 +155,15 @@ class BoardController extends ApiController { public function clone($boardId) { return $this->boardService->clone($boardId, $this->userId); } + + /** + * @NoAdminRequired + * @param $boardId + * @param $owner + * @param $newOwner + * * @return null|void + */ + public function transferOwner($boardId, $owner, $newOwner) { + return $this->boardService->transferOwnership($owner, $newOwner); + } } diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index 556408551..9f47afba7 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -746,6 +746,12 @@ class BoardService { $this->boardsCache = null; } + private function getBoardOwner($boardId) { + $board = $this->boardMapper->find($boardId); + + return $board->getOwner(); + } + /** * Clean a given board data from the Cache */ diff --git a/src/components/board/SharingTabSidebar.vue b/src/components/board/SharingTabSidebar.vue index b1f84bf91..d09c99afd 100644 --- a/src/components/board/SharingTabSidebar.vue +++ b/src/components/board/SharingTabSidebar.vue @@ -53,6 +53,9 @@ {{ t('deck', 'Can manage') }} + + {{ t('deck', 'Owner') }} + {{ t('deck', 'Delete') }} @@ -72,7 +75,7 @@ import { Avatar, Multiselect, Actions, ActionButton, ActionCheckbox } from '@nex import { CollectionList } from 'nextcloud-vue-collections' import { mapGetters, mapState } from 'vuex' import { getCurrentUser } from '@nextcloud/auth' -import { showError } from '@nextcloud/dialogs' +import { showError, showSuccess } from '@nextcloud/dialogs' import debounce from 'lodash/debounce' export default { @@ -97,6 +100,7 @@ export default { isSearching: false, addAcl: null, addAclForAPI: null, + newOwner: null, } }, computed: { @@ -194,6 +198,39 @@ export default { clickDeleteAcl(acl) { this.$store.dispatch('deleteAclFromCurrentBoard', acl) }, + clickTranserOwner(newOwner) { + OC.dialogs.confirmDestructive( + t('deck', 'Are you sure you want to transfer the board {title} for {user} ?', { title: this.board.title, user: newOwner }), + t('deck', 'Transfer the board.'), + { + type: OC.dialogs.YES_NO_BUTTONS, + confirm: t('deck', 'Transfer'), + confirmClasses: 'error', + cancel: t('deck', 'Cancel'), + }, + async (result) => { + if (result) { + try { + this.isLoading = true + await this.$store.dispatch('transferOwnership', { + boardId: this.board.id, + newOwner, + owner: this.board.owner.uid, + }) + const successMessage = t('deck', 'Transfer the board for {user} successfully', { user: newOwner }) + showSuccess(successMessage) + this.$router.push({ name: 'main' }) + } catch (e) { + const errorMessage = t('deck', 'Failed to transfer the board for {user}', { user: newOwner.user }) + showError(errorMessage) + } finally { + this.isLoading = false + } + } + }, + true + ) + }, }, } diff --git a/src/store/main.js b/src/store/main.js index bf37284f9..f7c3a16ee 100644 --- a/src/store/main.js +++ b/src/store/main.js @@ -26,7 +26,7 @@ import { loadState } from '@nextcloud/initial-state' import Vue from 'vue' import Vuex from 'vuex' import axios from '@nextcloud/axios' -import { generateOcsUrl } from '@nextcloud/router' +import { generateOcsUrl, generateUrl } from '@nextcloud/router' import { BoardApi } from '../services/BoardApi' import actions from './actions' import stack from './stack' @@ -486,5 +486,11 @@ export default new Vuex.Store({ dispatch('loadBoardById', acl.boardId) }) }, + async transferOwnership({ commit }, { boardId, owner, newOwner }) { + await axios.put(generateUrl(`apps/deck/boards/${boardId}/transferOwner`), { + owner, + newOwner, + }) + }, }, }) From 9f1dbd137cd0b1149532bfb36dd99e4b742612a7 Mon Sep 17 00:00:00 2001 From: Luka Trovic Date: Wed, 16 Mar 2022 17:13:29 +0100 Subject: [PATCH 26/29] fix: feedback Signed-off-by: Luka Trovic --- lib/Controller/BoardController.php | 10 ++++++++-- lib/Service/BoardService.php | 6 ------ src/components/board/SharingTabSidebar.vue | 7 +++---- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/lib/Controller/BoardController.php b/lib/Controller/BoardController.php index 086cf8a59..b82629723 100644 --- a/lib/Controller/BoardController.php +++ b/lib/Controller/BoardController.php @@ -27,6 +27,8 @@ use OCA\Deck\Db\Acl; use OCA\Deck\Service\BoardService; use OCA\Deck\Service\PermissionService; use OCP\AppFramework\ApiController; +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\DataResponse; use OCP\IRequest; class BoardController extends ApiController { @@ -163,7 +165,11 @@ class BoardController extends ApiController { * @param $newOwner * * @return null|void */ - public function transferOwner($boardId, $owner, $newOwner) { - return $this->boardService->transferOwnership($owner, $newOwner); + public function transferOwner(int $boardId, string $newOwner): DataResponse { + if ($this->permissionService->userIsBoardOwner($boardId, $this->userId)) { + return new DataResponse($this->boardService->transferBoardOwnership($boardId, $newOwner), HTTP::STATUS_OK); + } + + return new DataResponse([], HTTP::STATUS_UNAUTHORIZED); } } diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index 9f47afba7..556408551 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -746,12 +746,6 @@ class BoardService { $this->boardsCache = null; } - private function getBoardOwner($boardId) { - $board = $this->boardMapper->find($boardId); - - return $board->getOwner(); - } - /** * Clean a given board data from the Cache */ diff --git a/src/components/board/SharingTabSidebar.vue b/src/components/board/SharingTabSidebar.vue index d09c99afd..b51089eb6 100644 --- a/src/components/board/SharingTabSidebar.vue +++ b/src/components/board/SharingTabSidebar.vue @@ -53,7 +53,7 @@ {{ t('deck', 'Can manage') }} - + {{ t('deck', 'Owner') }} @@ -198,7 +198,7 @@ export default { clickDeleteAcl(acl) { this.$store.dispatch('deleteAclFromCurrentBoard', acl) }, - clickTranserOwner(newOwner) { + clickTransferOwner(newOwner) { OC.dialogs.confirmDestructive( t('deck', 'Are you sure you want to transfer the board {title} for {user} ?', { title: this.board.title, user: newOwner }), t('deck', 'Transfer the board.'), @@ -214,8 +214,7 @@ export default { this.isLoading = true await this.$store.dispatch('transferOwnership', { boardId: this.board.id, - newOwner, - owner: this.board.owner.uid, + newOwner }) const successMessage = t('deck', 'Transfer the board for {user} successfully', { user: newOwner }) showSuccess(successMessage) From 3f29cd97dbb450b453a797d7468829084cd70782 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 21 Mar 2022 13:42:15 +0100 Subject: [PATCH 27/29] Cover case where the owner is preserved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Controller/BoardController.php | 3 +- lib/Service/BoardService.php | 10 +++- src/components/board/SharingTabSidebar.vue | 2 +- .../database/TransferOwnershipTest.php | 58 ++++++++++++++----- 4 files changed, 56 insertions(+), 17 deletions(-) diff --git a/lib/Controller/BoardController.php b/lib/Controller/BoardController.php index b82629723..c5e2c62a0 100644 --- a/lib/Controller/BoardController.php +++ b/lib/Controller/BoardController.php @@ -24,6 +24,7 @@ namespace OCA\Deck\Controller; use OCA\Deck\Db\Acl; +use OCA\Deck\Db\Board; use OCA\Deck\Service\BoardService; use OCA\Deck\Service\PermissionService; use OCP\AppFramework\ApiController; @@ -152,7 +153,7 @@ class BoardController extends ApiController { /** * @NoAdminRequired * @param $boardId - * @return \OCP\Deck\DB\Board + * @return Board */ public function clone($boardId) { return $this->boardService->clone($boardId, $this->userId); diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index 556408551..ee0da6720 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -24,6 +24,7 @@ namespace OCA\Deck\Service; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use OCA\Deck\Activity\ActivityManager; use OCA\Deck\Activity\ChangeSet; use OCA\Deck\AppInfo\Application; @@ -694,6 +695,11 @@ class BoardService { if ($changeContent) { $this->assignedUsersMapper->remapAssignedUser($boardId, $previousOwner, $newOwner); $this->cardMapper->remapCardOwner($boardId, $previousOwner, $newOwner); + } else { + try { + $this->addAcl($boardId, Acl::PERMISSION_TYPE_USER, $previousOwner, true, true, true); + } catch (UniqueConstraintViolationException $e) { + } } \OC::$server->getDatabaseConnection()->commit(); return $this->boardMapper->find($boardId); @@ -706,7 +712,9 @@ class BoardService { public function transferOwnership(string $owner, string $newOwner, bool $changeContent = false): \Generator { $boards = $this->boardMapper->findAllByUser($owner); foreach ($boards as $board) { - yield $this->transferBoardOwnership($board->getId(), $newOwner, $changeContent); + if ($board->getOwner() === $owner) { + yield $this->transferBoardOwnership($board->getId(), $newOwner, $changeContent); + } } } diff --git a/src/components/board/SharingTabSidebar.vue b/src/components/board/SharingTabSidebar.vue index b51089eb6..0aa2136d2 100644 --- a/src/components/board/SharingTabSidebar.vue +++ b/src/components/board/SharingTabSidebar.vue @@ -53,7 +53,7 @@ {{ t('deck', 'Can manage') }} - + {{ t('deck', 'Owner') }} diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php index c7e9662d5..a88178c06 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -64,7 +64,6 @@ class TransferOwnershipTest extends \Test\TestCase { $stacks = []; $board = $this->boardService->create('Test', self::TEST_USER_1, '000000'); $id = $board->getId(); - $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); @@ -110,13 +109,20 @@ class TransferOwnershipTest extends \Test\TestCase { * @covers ::transferOwnership */ public function testTransferACLOwnership() { - iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2)); + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true)); $board = $this->boardService->find($this->board->getId()); $acl = $board->getAcl(); - // Check if old owner is no longer in ACL - $this->assertTrue((bool)array_filter($acl, function ($item) { - return $item->getParticipant() === self::TEST_USER_1 && $item->getType() === Acl::PERMISSION_TYPE_USER; - })); + $this->assertBoardDoesNotHaveAclUser($board, self::TEST_USER_1); + } + + /** + * @covers ::transferOwnership + */ + public function testTransferACLOwnershipPreserveOwner() { + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, false)); + $board = $this->boardService->find($this->board->getId()); + $acl = $board->getAcl(); + $this->assertBoardHasAclUser($board, self::TEST_USER_1); } /** @@ -196,17 +202,41 @@ class TransferOwnershipTest extends \Test\TestCase { iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3)); } + private function assertBoardHasAclUser($board, $userId) { + $hasUser = (bool)array_filter($board->getAcl(), function ($item) use ($userId) { + return $item->getParticipant() === $userId && $item->getType() === Acl::PERMISSION_TYPE_USER; + }); + self::assertTrue($hasUser, 'user ' . $userId . ' should be in the board acl list'); + } + + private function assertBoardDoesNotHaveAclUser($board, $userId) { + $hasUser = (bool)array_filter($board->getAcl(), function ($item) use ($userId) { + return $item->getParticipant() === $userId && $item->getType() === Acl::PERMISSION_TYPE_USER; + }); + self::assertFalse($hasUser, 'user ' . $userId . ' should not be in the board acl list'); + } + /** * @covers ::transferOwnership */ - public function testDontRemoveTargetFromAcl() { - iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_2, self::TEST_USER_3)); + public function testDontRemoveOldOwnerFromAcl() { + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2)); $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); + + $this->assertBoardDoesNotHaveAclUser($board, self::TEST_USER_2); + $this->assertBoardHasAclUser($board, self::TEST_USER_3); + $this->assertBoardHasAclUser($board, self::TEST_USER_1); + } + + /** + * @covers ::transferOwnership + */ + public function testRemoveOldOwnerFromAclForChange() { + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true)); + $board = $this->boardService->find($this->board->getId()); + $this->assertBoardDoesNotHaveAclUser($board, self::TEST_USER_2); + $this->assertBoardHasAclUser($board, self::TEST_USER_3); + $this->assertBoardDoesNotHaveAclUser($board, self::TEST_USER_1); } /** @@ -214,7 +244,7 @@ class TransferOwnershipTest extends \Test\TestCase { */ public function testMergePermissions() { $this->boardService->addAcl($this->board->getId(), Acl::PERMISSION_TYPE_USER, self::TEST_USER_2, true, false, true); - iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_2, self::TEST_USER_3)); + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3)); $board = $this->boardService->find($this->board->getId()); $acl = $board->getAcl(); $isMerged = (bool)array_filter($acl, function ($item) { From 23f0b16a5aca738699bfaa396ff4d505f975b4cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 22 Mar 2022 08:21:18 +0100 Subject: [PATCH 28/29] Handle board exceptions more gracefully MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Command/TransferOwnership.php | 18 ++++++++++++++++-- lib/Controller/BoardController.php | 4 ---- lib/Service/BoardService.php | 13 +++++++------ lib/Service/PermissionService.php | 9 +++++++++ 4 files changed, 32 insertions(+), 12 deletions(-) diff --git a/lib/Command/TransferOwnership.php b/lib/Command/TransferOwnership.php index 715200f75..95a4f65bb 100644 --- a/lib/Command/TransferOwnership.php +++ b/lib/Command/TransferOwnership.php @@ -2,7 +2,9 @@ namespace OCA\Deck\Command; +use OCA\Deck\Db\BoardMapper; use OCA\Deck\Service\BoardService; +use OCA\Deck\Service\PermissionService; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Helper\QuestionHelper; use Symfony\Component\Console\Input\InputArgument; @@ -13,12 +15,16 @@ use Symfony\Component\Console\Question\ConfirmationQuestion; final class TransferOwnership extends Command { protected $boardService; + protected $boardMapper; + protected $permissionService; protected $questionHelper; - public function __construct(BoardService $boardService, QuestionHelper $questionHelper) { + public function __construct(BoardService $boardService, BoardMapper $boardMapper, PermissionService $permissionService, QuestionHelper $questionHelper) { parent::__construct(); $this->boardService = $boardService; + $this->boardMapper = $boardMapper; + $this->permissionService = $permissionService; $this->questionHelper = $questionHelper; } @@ -57,7 +63,15 @@ final class TransferOwnership extends Command { $remapAssignment = $input->getOption('remap'); - $board = $boardId ? $this->boardService->find($boardId) : null; + $this->boardService->setUserId($owner); + $this->permissionService->setUserId($owner); + + try { + $board = $boardId ? $this->boardMapper->find($boardId) : null; + } catch (\Exception $e) { + $output->writeln("Could not find a board for the provided id."); + return 1; + } if ($boardId !== null && $board->getOwner() !== $owner) { $output->writeln("$owner is not the owner of the board $boardId (" . $board->getTitle() . ")"); diff --git a/lib/Controller/BoardController.php b/lib/Controller/BoardController.php index c5e2c62a0..0e909c305 100644 --- a/lib/Controller/BoardController.php +++ b/lib/Controller/BoardController.php @@ -161,10 +161,6 @@ class BoardController extends ApiController { /** * @NoAdminRequired - * @param $boardId - * @param $owner - * @param $newOwner - * * @return null|void */ public function transferOwner(int $boardId, string $newOwner): DataResponse { if ($this->permissionService->userIsBoardOwner($boardId, $this->userId)) { diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index ee0da6720..e1cfc8023 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -524,7 +524,7 @@ class BoardService { $acl->setPermissionManage($manage); $newAcl = $this->aclMapper->insert($acl); - $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_BOARD, $newAcl, ActivityManager::SUBJECT_BOARD_SHARE); + $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_BOARD, $newAcl, ActivityManager::SUBJECT_BOARD_SHARE, [], $this->userId); $this->notificationHelper->sendBoardShared((int)$boardId, $acl); $this->boardMapper->mapAcl($newAcl); $this->changeHelper->boardChanged($boardId); @@ -689,17 +689,18 @@ class BoardService { $previousOwner = $board->getOwner(); $this->clearBoardFromCache($board); $this->aclMapper->deleteParticipantFromBoard($boardId, Acl::PERMISSION_TYPE_USER, $newOwner); + if (!$changeContent) { + try { + $this->addAcl($boardId, Acl::PERMISSION_TYPE_USER, $previousOwner, true, true, true); + } catch (UniqueConstraintViolationException $e) { + } + } $this->boardMapper->transferOwnership($previousOwner, $newOwner, $boardId); // Optionally also change user assignments and card owner information if ($changeContent) { $this->assignedUsersMapper->remapAssignedUser($boardId, $previousOwner, $newOwner); $this->cardMapper->remapCardOwner($boardId, $previousOwner, $newOwner); - } else { - try { - $this->addAcl($boardId, Acl::PERMISSION_TYPE_USER, $previousOwner, true, true, true); - } catch (UniqueConstraintViolationException $e) { - } } \OC::$server->getDatabaseConnection()->commit(); return $this->boardMapper->find($boardId); diff --git a/lib/Service/PermissionService.php b/lib/Service/PermissionService.php index e94d073df..6777eb910 100644 --- a/lib/Service/PermissionService.php +++ b/lib/Service/PermissionService.php @@ -333,4 +333,13 @@ class PermissionService { } return $groups; } + + /** + * Set a different user than the current one, e.g. when no user is available in occ + * + * @param string $userId + */ + public function setUserId(string $userId): void { + $this->userId = $userId; + } } From c6aef45d8cfcabf3066bcf757d563c4ae5a39151 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 22 Mar 2022 08:22:29 +0100 Subject: [PATCH 29/29] Adjust documentaion wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- docs/User_documentation_en.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/User_documentation_en.md b/docs/User_documentation_en.md index f68d87fe6..876b95f02 100644 --- a/docs/User_documentation_en.md +++ b/docs/User_documentation_en.md @@ -162,19 +162,19 @@ Other text tokens will be used to perform a case-insensitive search on the card In addition, quotes can be used to pass a query with spaces, e.g. `"Exact match with spaces"` or `title:"My card"`. ### 8. New owner for the deck entities -You can transfer ownership of boards, cards, etc to a new user, using `occ` command `deck:transfer-ownership` +You can transfer ownership of boards, cards, etc to a new user, using `occ` command `deck:transfer-ownership` ```bash -php occ deck:transfer-ownership owner newOwner +php occ deck:transfer-ownership previousOwner newOwner ``` The transfer will preserve card details linked to the old owner, which can also be remapped by using the `--remap` option on the occ command. ```bash -php occ deck:transfer-ownership --remap owner newOwner +php occ deck:transfer-ownership --remap previousOwner newOwner ``` Individual boards can be transferred by adding the id of the board to the command: ```bash -php occ deck:transfer-ownership owner newOwner 123 +php occ deck:transfer-ownership previousOwner newOwner 123 ```