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();