diff --git a/lib/Cron/DeleteCron.php b/lib/Cron/DeleteCron.php index eff098caea..561f8bdc28 100644 --- a/lib/Cron/DeleteCron.php +++ b/lib/Cron/DeleteCron.php @@ -14,6 +14,7 @@ use OCA\Deck\Db\StackMapper; use OCA\Deck\InvalidAttachmentType; use OCA\Deck\Service\AttachmentService; +use OCA\Deck\Service\ConfigService; use OCA\Deck\Sharing\DeckShareProvider; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\IJob; @@ -21,6 +22,7 @@ class DeleteCron extends TimedJob { + private $configService; /** @var BoardMapper */ private $boardMapper; /** @var CardMapper */ @@ -36,6 +38,7 @@ class DeleteCron extends TimedJob { public function __construct( ITimeFactory $time, + ConfigService $configService, BoardMapper $boardMapper, CardMapper $cardMapper, AttachmentService $attachmentService, @@ -44,6 +47,7 @@ public function __construct( DeckShareProvider $deckShareProvider, ) { parent::__construct($time); + $this->configService = $configService; $this->boardMapper = $boardMapper; $this->cardMapper = $cardMapper; $this->attachmentService = $attachmentService; @@ -51,7 +55,7 @@ public function __construct( $this->stackMapper = $stackMapper; $this->deckShareProvider = $deckShareProvider; - $this->setInterval(60 * 60 * 24); + $this->setInterval(60 * 60); // Run once every hour $this->setTimeSensitivity(IJob::TIME_INSENSITIVE); } @@ -60,18 +64,19 @@ public function __construct( * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ protected function run($argument) { - $boards = $this->boardMapper->findToDelete(); + $timeLimit = time() - $this->configService->getTrashRetention(); + + $boards = $this->boardMapper->findToDelete($timeLimit); foreach ($boards as $board) { $this->boardMapper->delete($board); } - $timeLimit = time() - (60 * 5); // 5 min buffer $cards = $this->cardMapper->findToDelete($timeLimit, 500); foreach ($cards as $card) { $this->cardMapper->delete($card); } - $attachments = $this->attachmentMapper->findToDelete(); + $attachments = $this->attachmentMapper->findToDelete($timeLimit); foreach ($attachments as $attachment) { try { $service = $this->attachmentService->getService($attachment->getType()); @@ -88,7 +93,7 @@ protected function run($argument) { $this->deckShareProvider->delete($share); } - $stacks = $this->stackMapper->findToDelete(); + $stacks = $this->stackMapper->findToDelete($timeLimit); foreach ($stacks as $stack) { $this->stackMapper->delete($stack); } diff --git a/lib/Db/AttachmentMapper.php b/lib/Db/AttachmentMapper.php index 52864a3c8b..ace7f0c968 100644 --- a/lib/Db/AttachmentMapper.php +++ b/lib/Db/AttachmentMapper.php @@ -90,9 +90,7 @@ public function findAll($cardId) { * @param bool $withOffset * @return array */ - public function findToDelete($cardId = null, $withOffset = true) { - // add buffer of 5 min - $timeLimit = time() - (60 * 5); + public function findToDelete(int $timeLimit, $cardId = null, $withOffset = true) { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from($this->getTableName()) diff --git a/lib/Db/BoardMapper.php b/lib/Db/BoardMapper.php index 239041bb0b..0574c9c0ad 100644 --- a/lib/Db/BoardMapper.php +++ b/lib/Db/BoardMapper.php @@ -440,9 +440,7 @@ public function findAll(): array { return $this->findEntities($qb); } - public function findToDelete() { - // add buffer of 5 min - $timeLimit = time() - (60 * 5); + public function findToDelete(int $timeLimit) { $qb = $this->db->getQueryBuilder(); $qb->select('id', 'title', 'owner', 'color', 'archived', 'deleted_at', 'last_modified') ->from('deck_boards') diff --git a/lib/Db/StackMapper.php b/lib/Db/StackMapper.php index 5e4a43842d..99bc2314bd 100644 --- a/lib/Db/StackMapper.php +++ b/lib/Db/StackMapper.php @@ -165,9 +165,7 @@ public function findBoardId($id): ?int { return $result !== false ? $result : null; } - public function findToDelete(): array { - // add buffer of 5 min - $timeLimit = time() - (60 * 5); + public function findToDelete(int $timeLimit): array { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from($this->getTableName()) diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index 06f0328e1c..e9df81f03f 100644 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -27,6 +27,7 @@ use OCP\IUserManager; class AttachmentService { + private $configService; private $attachmentMapper; private $cardMapper; private $permissionService; @@ -49,6 +50,7 @@ class AttachmentService { private AttachmentServiceValidator $attachmentServiceValidator; public function __construct( + ConfigService $configService, AttachmentMapper $attachmentMapper, CardMapper $cardMapper, IUserManager $userManager, @@ -61,6 +63,7 @@ public function __construct( ActivityManager $activityManager, AttachmentServiceValidator $attachmentServiceValidator, ) { + $this->configService = $configService; $this->attachmentMapper = $attachmentMapper; $this->cardMapper = $cardMapper; $this->permissionService = $permissionService; @@ -115,7 +118,8 @@ public function findAll($cardId, $withDeleted = false) { $attachments = $this->attachmentMapper->findAll($cardId); if ($withDeleted) { - $attachments = array_merge($attachments, $this->attachmentMapper->findToDelete($cardId, false)); + $timeLimit = time() - $this->configService->getTrashRetention(); + $attachments = array_merge($attachments, $this->attachmentMapper->findToDelete($timeLimit, $cardId, false)); } foreach (array_keys($this->services) as $attachmentType) { diff --git a/lib/Service/ConfigService.php b/lib/Service/ConfigService.php index 3211e1f3c4..59777d0740 100644 --- a/lib/Service/ConfigService.php +++ b/lib/Service/ConfigService.php @@ -227,4 +227,10 @@ public function setAttachmentFolder(?string $userId, string $path): void { $this->config->setUserValue($userId ?? $this->getUserId(), 'deck', 'attachment_folder', $path); } + + public function getTrashRetention(): int { + $value = $this->config->getAppValue(Application::APP_ID, 'trashRetentionHours', '5'); + $hours = (int)$value > 0 ? (int)$value : 5; + return 60 * 60 * $hours; + } } diff --git a/tests/unit/Cron/DeleteCronTest.php b/tests/unit/Cron/DeleteCronTest.php index 73ab3a416a..9834798fc6 100644 --- a/tests/unit/Cron/DeleteCronTest.php +++ b/tests/unit/Cron/DeleteCronTest.php @@ -34,6 +34,7 @@ use OCA\Deck\Db\StackMapper; use OCA\Deck\InvalidAttachmentType; use OCA\Deck\Service\AttachmentService; +use OCA\Deck\Service\ConfigService; use OCA\Deck\Service\IAttachmentService; use OCA\Deck\Sharing\DeckShareProvider; use OCP\AppFramework\Utility\ITimeFactory; @@ -44,6 +45,8 @@ class DeleteCronTest extends TestCase { /** @var ITimeFactory|MockObject */ private $timeFactory; + /** @var ConfigService|MockObject */ + private $configService; /** @var BoardMapper|MockObject */ protected $boardMapper; /** @var CardMapper|\PHPUnit\Framework\MockObject\MockObject */ @@ -62,6 +65,7 @@ class DeleteCronTest extends TestCase { public function setUp(): void { parent::setUp(); $this->timeFactory = $this->createMock(ITimeFactory::class); + $this->configService = $this->createMock(ConfigService::class); $this->boardMapper = $this->createMock(BoardMapper::class); $this->cardMapper = $this->createMock(CardMapper::class); $this->attachmentService = $this->createMock(AttachmentService::class); @@ -70,6 +74,7 @@ public function setUp(): void { $this->deckShareProvider = $this->createMock(DeckShareProvider::class); $this->deleteCron = new DeleteCron( $this->timeFactory, + $this->configService, $this->boardMapper, $this->cardMapper, $this->attachmentService, diff --git a/tests/unit/Db/AttachmentMapperTest.php b/tests/unit/Db/AttachmentMapperTest.php index 9489909f1c..c82e26531d 100644 --- a/tests/unit/Db/AttachmentMapperTest.php +++ b/tests/unit/Db/AttachmentMapperTest.php @@ -110,8 +110,9 @@ public function testFindToDelete() { $attachment->resetUpdatedFields(); } - $this->assertEquals([$attachmentsToDelete[0]], $this->attachmentMapper->findToDelete(1)); - $this->assertEquals([$attachmentsToDelete[2]], $this->attachmentMapper->findToDelete(2)); + $timeLimit = time() - (60 * 60 * 5); + $this->assertEquals([$attachmentsToDelete[0]], $this->attachmentMapper->findToDelete($timeLimit, 1)); + $this->assertEquals([$attachmentsToDelete[2]], $this->attachmentMapper->findToDelete($timeLimit, 2)); } public function testIsOwner() { diff --git a/tests/unit/Db/BoardMapperTest.php b/tests/unit/Db/BoardMapperTest.php index edbe8d49b9..64d2d00362 100644 --- a/tests/unit/Db/BoardMapperTest.php +++ b/tests/unit/Db/BoardMapperTest.php @@ -152,7 +152,8 @@ public function testFindAllToDelete() { $this->boards[0]->setDeletedAt(1); $this->boards[0] = $this->boardMapper->update($this->boards[0]); - $actual = $this->boardMapper->findToDelete(); + $timeLimit = time() - (60 * 60 * 5); + $actual = $this->boardMapper->findToDelete($timeLimit); $this->boards[0]->resetUpdatedFields(); $this->assertEquals([$this->boards[0]], $actual); diff --git a/tests/unit/Service/AttachmentServiceTest.php b/tests/unit/Service/AttachmentServiceTest.php index abdebb0b11..c539e71991 100644 --- a/tests/unit/Service/AttachmentServiceTest.php +++ b/tests/unit/Service/AttachmentServiceTest.php @@ -65,6 +65,8 @@ class AttachmentServiceTest extends TestCase { /** @var IUserManager|MockObject */ private $userManager; + /** @var ConfigService */ + private $configService; /** @var AttachmentMapper|MockObject */ private $attachmentMapper; /** @var CardMapper|MockObject */ @@ -107,6 +109,8 @@ public function setUp(): void { $this->appContainer = $this->createMock(IAppContainer::class); + $this->configService = $this->createMock(ConfigService::class); + $this->userManager = $this->createMock(IUserManager::class); $this->attachmentMapper = $this->createMock(AttachmentMapper::class); $this->cardMapper = $this->createMock(CardMapper::class); @@ -135,6 +139,7 @@ public function setUp(): void { $this->attachmentServiceValidator = $this->createMock(AttachmentServiceValidator::class); $this->attachmentService = new AttachmentService( + $this->configService, $this->attachmentMapper, $this->cardMapper, $this->userManager, @@ -171,7 +176,7 @@ public function testRegisterAttachmentService() { $application->expects($this->any()) ->method('getContainer') ->willReturn($appContainer); - $attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator); + $attachmentService = new AttachmentService($this->configService, $this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator); $attachmentService->registerAttachmentService('custom', MyAttachmentService::class); $this->assertEquals($fileServiceMock, $attachmentService->getService('deck_file')); $this->assertEquals(MyAttachmentService::class, get_class($attachmentService->getService('custom'))); @@ -201,7 +206,7 @@ public function testRegisterAttachmentServiceNotExisting() { ->method('getContainer') ->willReturn($appContainer); - $attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator); + $attachmentService = new AttachmentService($this->configService, $this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator); $attachmentService->registerAttachmentService('custom', MyAttachmentService::class); $attachmentService->getService('deck_file_invalid'); } @@ -262,9 +267,12 @@ public function testFindAllWithDeleted() { ->method('findAll') ->with(123) ->willReturn($attachments); + $this->configService->expects($this->once()) + ->method('getTrashRetention') + ->willReturn(3600); $this->attachmentMapper->expects($this->once()) ->method('findToDelete') - ->with(123, false) + ->with($this->anything(), 123, false) ->willReturn($attachmentsDeleted); $this->attachmentServiceImpl->expects($this->exactly(4)) diff --git a/tests/unit/Service/Importer/BoardImportServiceTest.php b/tests/unit/Service/Importer/BoardImportServiceTest.php index 7d8086092e..752d43078a 100644 --- a/tests/unit/Service/Importer/BoardImportServiceTest.php +++ b/tests/unit/Service/Importer/BoardImportServiceTest.php @@ -29,6 +29,7 @@ use OCA\Deck\Db\Assignment; use OCA\Deck\Db\AssignmentMapper; use OCA\Deck\Db\AttachmentMapper; +use OCA\Deck\Db\Board; use OCA\Deck\Db\BoardMapper; use OCA\Deck\Db\Card; use OCA\Deck\Db\CardMapper; @@ -151,6 +152,12 @@ public function testImportSuccess() { $this->userManager->method('userExists') ->willReturn(true); + $board = new Board(); + $board->setOwner('admin'); + $this->trelloJsonService + ->method('getBoard') + ->willReturn($board); + $this->boardMapper ->expects($this->once()) ->method('insert'); diff --git a/tests/unit/Service/PermissionServiceTest.php b/tests/unit/Service/PermissionServiceTest.php index a25a325d57..0e47d514c0 100644 --- a/tests/unit/Service/PermissionServiceTest.php +++ b/tests/unit/Service/PermissionServiceTest.php @@ -358,7 +358,7 @@ public function testFindUsers() { $this->userManager->expects($this->any()) ->method('userExists') ->withConsecutive(['user1'], ['user2']) - ->willReturnOnConsecutiveCalls($user1, $user2); + ->willReturnOnConsecutiveCalls(true, true); $group = $this->createMock(IGroup::class); $group->expects($this->once())