From 4ecbe6fa10ad014bc3f2aafcc99ca7e452f55208 Mon Sep 17 00:00:00 2001 From: Jonas Date: Tue, 4 Aug 2026 17:36:55 +0200 Subject: [PATCH 1/2] fix(trash): allow to configure the trash retention in hours Also change default from 5 minutes to 5 hours. The cronjob was configured to run only once a day anyway, so the deletion took place sometime between 5 minutes and 1 day after the item was trashed. Signed-off-by: Jonas Assisted-by: OpenCode:claude-fable-5 --- lib/Cron/DeleteCron.php | 13 ++++++++----- lib/Db/AttachmentMapper.php | 4 +--- lib/Db/BoardMapper.php | 4 +--- lib/Db/StackMapper.php | 4 +--- lib/Service/AttachmentService.php | 5 ++++- lib/Service/ConfigService.php | 6 ++++++ tests/unit/Cron/DeleteCronTest.php | 5 +++++ tests/unit/Db/AttachmentMapperTest.php | 5 +++-- tests/unit/Db/BoardMapperTest.php | 3 ++- tests/unit/Service/AttachmentServiceTest.php | 14 +++++++++++--- 10 files changed, 42 insertions(+), 21 deletions(-) diff --git a/lib/Cron/DeleteCron.php b/lib/Cron/DeleteCron.php index eff098caea..cdee3b6ea1 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; @@ -36,6 +37,7 @@ class DeleteCron extends TimedJob { public function __construct( ITimeFactory $time, + private readonly ConfigService $configService, BoardMapper $boardMapper, CardMapper $cardMapper, AttachmentService $attachmentService, @@ -51,7 +53,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 +62,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 +91,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 36d947c61a..83c3327d15 100644 --- a/lib/Db/AttachmentMapper.php +++ b/lib/Db/AttachmentMapper.php @@ -82,9 +82,7 @@ public function findAll(int $cardId): array { /** * @return Attachment[] */ - public function findToDelete(?int $cardId = null, bool $withOffset = true): array { - // add buffer of 5 min - $timeLimit = time() - (60 * 5); + public function findToDelete(int $timeLimit, ?int $cardId = null, bool $withOffset = true): array { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from($this->getTableName()) diff --git a/lib/Db/BoardMapper.php b/lib/Db/BoardMapper.php index 56d4b80d03..23486dafc5 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 ad27680d1a..a034e7cff7 100644 --- a/lib/Db/StackMapper.php +++ b/lib/Db/StackMapper.php @@ -153,9 +153,7 @@ public function findBoardId(int $id): ?int { * @return array * @throws \OCP\DB\Exception */ - 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 0c2463be00..272169fb12 100644 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -28,6 +28,7 @@ use Psr\Container\ContainerExceptionInterface; class AttachmentService { + private $configService; private $attachmentMapper; private $cardMapper; private $permissionService; @@ -50,6 +51,7 @@ class AttachmentService { private AttachmentServiceValidator $attachmentServiceValidator; public function __construct( + ConfigService $configService, AttachmentMapper $attachmentMapper, CardMapper $cardMapper, IUserManager $userManager, @@ -107,7 +109,8 @@ public function findAll(int $cardId, bool $withDeleted = false): array { $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 7e381255e8..0602f0a9ab 100644 --- a/lib/Service/ConfigService.php +++ b/lib/Service/ConfigService.php @@ -232,4 +232,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 017aefb5d3..a7d50e30b4 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 5ceac7d43f..8b25db6ff3 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(); $filteredActual = array_values(array_filter($actual, function ($board) { diff --git a/tests/unit/Service/AttachmentServiceTest.php b/tests/unit/Service/AttachmentServiceTest.php index 0a4b4e3445..caa8d70fac 100644 --- a/tests/unit/Service/AttachmentServiceTest.php +++ b/tests/unit/Service/AttachmentServiceTest.php @@ -67,6 +67,8 @@ class AttachmentServiceTest extends TestCase { /** @var IUserManager|MockObject */ private $userManager; + /** @var ConfigService */ + private $configService; /** @var AttachmentMapper|MockObject */ private $attachmentMapper; /** @var CardMapper|MockObject */ @@ -109,6 +111,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); @@ -137,6 +141,7 @@ public function setUp(): void { $this->attachmentServiceValidator = $this->createMock(AttachmentServiceValidator::class); $this->attachmentService = new AttachmentService( + $this->configService, $this->attachmentMapper, $this->cardMapper, $this->userManager, @@ -173,7 +178,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'))); @@ -203,7 +208,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'); } @@ -264,9 +269,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)) From 967a522deb47886bc073d11cb7b5c518eb6294a1 Mon Sep 17 00:00:00 2001 From: Jonas Date: Wed, 5 Aug 2026 13:50:55 +0200 Subject: [PATCH 2/2] test(phpunit): fix failing tests Signed-off-by: Jonas --- lib/Service/AttachmentService.php | 1 + tests/unit/Service/Importer/BoardImportServiceTest.php | 7 +++++++ tests/unit/Service/PermissionServiceTest.php | 2 +- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index 272169fb12..ac47a1f5e1 100644 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -64,6 +64,7 @@ public function __construct( ActivityManager $activityManager, AttachmentServiceValidator $attachmentServiceValidator, ) { + $this->configService = $configService; $this->attachmentMapper = $attachmentMapper; $this->cardMapper = $cardMapper; $this->permissionService = $permissionService; diff --git a/tests/unit/Service/Importer/BoardImportServiceTest.php b/tests/unit/Service/Importer/BoardImportServiceTest.php index c1cc7b2f94..485907eae4 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())