Skip to content

Commit 0dc30ec

Browse files
mejo-mejo-
committed
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 <jonas@freesources.org> Assisted-by: OpenCode:claude-fable-5
1 parent 341c618 commit 0dc30ec

11 files changed

Lines changed: 42 additions & 21 deletions

‎lib/Cron/DeleteCron.php‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use OCA\Deck\Db\StackMapper;
1414
use OCA\Deck\InvalidAttachmentType;
1515
use OCA\Deck\Service\AttachmentService;
16+
use OCA\Deck\Service\ConfigService;
1617
use OCA\Deck\Sharing\DeckShareProvider;
1718
use OCP\AppFramework\Utility\ITimeFactory;
1819
use OCP\BackgroundJob\IJob;
@@ -35,6 +36,7 @@ class DeleteCron extends TimedJob {
3536

3637
public function __construct(
3738
ITimeFactory $time,
39+
private readonly ConfigService $configService,
3840
BoardMapper $boardMapper,
3941
CardMapper $cardMapper,
4042
AttachmentService $attachmentService,
@@ -50,7 +52,7 @@ public function __construct(
5052
$this->stackMapper = $stackMapper;
5153
$this->deckShareProvider = $deckShareProvider;
5254

53-
$this->setInterval(60 * 60 * 24);
55+
$this->setInterval(60 * 60); // Run once every hour
5456
$this->setTimeSensitivity(IJob::TIME_INSENSITIVE);
5557
}
5658

@@ -59,18 +61,19 @@ public function __construct(
5961
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
6062
*/
6163
protected function run($argument) {
62-
$boards = $this->boardMapper->findToDelete();
64+
$timeLimit = time() - $this->configService->getTrashRetention();
65+
66+
$boards = $this->boardMapper->findToDelete($timeLimit);
6367
foreach ($boards as $board) {
6468
$this->boardMapper->delete($board);
6569
}
6670

67-
$timeLimit = time() - (60 * 5); // 5 min buffer
6871
$cards = $this->cardMapper->findToDelete($timeLimit, 500);
6972
foreach ($cards as $card) {
7073
$this->cardMapper->delete($card);
7174
}
7275

73-
$attachments = $this->attachmentMapper->findToDelete();
76+
$attachments = $this->attachmentMapper->findToDelete($timeLimit);
7477
foreach ($attachments as $attachment) {
7578
try {
7679
$service = $this->attachmentService->getService($attachment->getType());
@@ -87,7 +90,7 @@ protected function run($argument) {
8790
$this->deckShareProvider->delete($share);
8891
}
8992

90-
$stacks = $this->stackMapper->findToDelete();
93+
$stacks = $this->stackMapper->findToDelete($timeLimit);
9194
foreach ($stacks as $stack) {
9295
$this->stackMapper->delete($stack);
9396
}

‎lib/Db/AttachmentMapper.php‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,9 +108,7 @@ public function findAll(int $cardId): array {
108108
/**
109109
* @return Attachment[]
110110
*/
111-
public function findToDelete(?int $cardId = null, bool $withOffset = true): array {
112-
// add buffer of 5 min
113-
$timeLimit = time() - (60 * 5);
111+
public function findToDelete(int $timeLimit, ?int $cardId = null, bool $withOffset = true): array {
114112
$qb = $this->db->getQueryBuilder();
115113
$qb->select('*')
116114
->from($this->getTableName())

‎lib/Db/BoardMapper.php‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -455,9 +455,7 @@ public function findAll(): array {
455455
return $this->findEntities($qb);
456456
}
457457

458-
public function findToDelete() {
459-
// add buffer of 5 min
460-
$timeLimit = time() - (60 * 5);
458+
public function findToDelete(int $timeLimit) {
461459
$qb = $this->db->getQueryBuilder();
462460
$qb->select('id', 'title', 'owner', 'color', 'archived', 'deleted_at', 'last_modified', 'external_id', 'share_token')
463461
->from('deck_boards')

‎lib/Db/StackMapper.php‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -219,9 +219,7 @@ public function findBoardId(int $id): ?int {
219219
* @return array<Stack>
220220
* @throws \OCP\DB\Exception
221221
*/
222-
public function findToDelete(): array {
223-
// add buffer of 5 min
224-
$timeLimit = time() - (60 * 5);
222+
public function findToDelete(int $timeLimit): array {
225223
$qb = $this->db->getQueryBuilder();
226224
$qb->select('*')
227225
->from($this->getTableName())

‎lib/Service/AttachmentService.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ class AttachmentService {
3232
private array $services = [];
3333

3434
public function __construct(
35+
private readonly ConfigService $configService,
3536
private readonly AttachmentMapper $attachmentMapper,
3637
private readonly CardMapper $cardMapper,
3738
private readonly IUserManager $userManager,
@@ -77,7 +78,8 @@ public function findAll(int $cardId, bool $withDeleted = false): array {
7778

7879
$attachments = $this->attachmentMapper->findAll($cardId);
7980
if ($withDeleted) {
80-
$attachments = array_merge($attachments, $this->attachmentMapper->findToDelete($cardId, false));
81+
$timeLimit = time() - $this->configService->getTrashRetention();
82+
$attachments = array_merge($attachments, $this->attachmentMapper->findToDelete($timeLimit, $cardId, false));
8183
}
8284

8385
foreach (array_keys($this->services) as $attachmentType) {

‎lib/Service/ConfigService.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,4 +251,10 @@ public function setAttachmentFolder(?string $userId, string $path): void {
251251

252252
$this->config->setUserValue($userId ?? $this->getUserId(), 'deck', 'attachment_folder', $path);
253253
}
254+
255+
public function getTrashRetention(): int {
256+
$value = $this->config->getAppValue(Application::APP_ID, 'trashRetentionHours', '5');
257+
$hours = (int)$value > 0 ? (int)$value : 5;
258+
return 60 * 60 * $hours;
259+
}
254260
}

‎tests/unit/Cron/DeleteCronTest.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
use OCA\Deck\Db\StackMapper;
3535
use OCA\Deck\InvalidAttachmentType;
3636
use OCA\Deck\Service\AttachmentService;
37+
use OCA\Deck\Service\ConfigService;
3738
use OCA\Deck\Service\IAttachmentService;
3839
use OCA\Deck\Sharing\DeckShareProvider;
3940
use OCP\AppFramework\Utility\ITimeFactory;
@@ -44,6 +45,8 @@ class DeleteCronTest extends TestCase {
4445

4546
/** @var ITimeFactory|MockObject */
4647
private $timeFactory;
48+
/** @var ConfigService|MockObject */
49+
private $configService;
4750
/** @var BoardMapper|MockObject */
4851
protected $boardMapper;
4952
/** @var CardMapper|\PHPUnit\Framework\MockObject\MockObject */
@@ -62,6 +65,7 @@ class DeleteCronTest extends TestCase {
6265
public function setUp(): void {
6366
parent::setUp();
6467
$this->timeFactory = $this->createMock(ITimeFactory::class);
68+
$this->configService = $this->createMock(ConfigService::class);
6569
$this->boardMapper = $this->createMock(BoardMapper::class);
6670
$this->cardMapper = $this->createMock(CardMapper::class);
6771
$this->attachmentService = $this->createMock(AttachmentService::class);
@@ -70,6 +74,7 @@ public function setUp(): void {
7074
$this->deckShareProvider = $this->createMock(DeckShareProvider::class);
7175
$this->deleteCron = new DeleteCron(
7276
$this->timeFactory,
77+
$this->configService,
7378
$this->boardMapper,
7479
$this->cardMapper,
7580
$this->attachmentService,

‎tests/unit/Db/AttachmentMapperTest.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,9 @@ public function testFindToDelete() {
110110
$attachment->resetUpdatedFields();
111111
}
112112

113-
$this->assertEquals([$attachmentsToDelete[0]], $this->attachmentMapper->findToDelete(1));
114-
$this->assertEquals([$attachmentsToDelete[2]], $this->attachmentMapper->findToDelete(2));
113+
$timeLimit = time() - (60 * 60 * 5);
114+
$this->assertEquals([$attachmentsToDelete[0]], $this->attachmentMapper->findToDelete($timeLimit, 1));
115+
$this->assertEquals([$attachmentsToDelete[2]], $this->attachmentMapper->findToDelete($timeLimit, 2));
115116
}
116117

117118
public function testIsOwner() {

‎tests/unit/Db/BoardMapperTest.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,8 @@ public function testFindAllToDelete() {
154154
$this->boards[0]->setDeletedAt(1);
155155
$this->boards[0] = $this->boardMapper->update($this->boards[0]);
156156

157-
$actual = $this->boardMapper->findToDelete();
157+
$timeLimit = time() - (60 * 60 * 5);
158+
$actual = $this->boardMapper->findToDelete($timeLimit);
158159
$this->boards[0]->resetUpdatedFields();
159160

160161
$filteredActual = array_values(array_filter($actual, function ($board) {

‎tests/unit/Service/AttachmentServiceTest.php‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@ class AttachmentServiceTest extends TestCase {
6767

6868
/** @var IUserManager|MockObject */
6969
private $userManager;
70+
/** @var ConfigService */
71+
private $configService;
7072
/** @var AttachmentMapper|MockObject */
7173
private $attachmentMapper;
7274
/** @var CardMapper|MockObject */
@@ -106,6 +108,8 @@ public function setUp(): void {
106108

107109
$this->appContainer = $this->createMock(ContainerInterface::class);
108110

111+
$this->configService = $this->createMock(ConfigService::class);
112+
109113
$this->userManager = $this->createMock(IUserManager::class);
110114
$this->attachmentMapper = $this->createMock(AttachmentMapper::class);
111115
$this->cardMapper = $this->createMock(CardMapper::class);
@@ -130,6 +134,7 @@ public function setUp(): void {
130134
$this->attachmentServiceValidator = $this->createMock(AttachmentServiceValidator::class);
131135

132136
$this->attachmentService = new AttachmentService(
137+
$this->configService,
133138
$this->attachmentMapper,
134139
$this->cardMapper,
135140
$this->userManager,
@@ -166,7 +171,7 @@ public function testRegisterAttachmentService() {
166171
$application->expects($this->any())
167172
->method('getContainer')
168173
->willReturn($appContainer);
169-
$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator);
174+
$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);
170175
$attachmentService->registerAttachmentService('custom', MyAttachmentService::class);
171176
$this->assertEquals($fileServiceMock, $attachmentService->getService('deck_file'));
172177
$this->assertEquals(MyAttachmentService::class, get_class($attachmentService->getService('custom')));
@@ -191,7 +196,7 @@ public function testRegisterAttachmentServiceNotExisting() {
191196
->method('getContainer')
192197
->willReturn($appContainer);
193198

194-
$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator);
199+
$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);
195200
$attachmentService->registerAttachmentService('custom', MyAttachmentService::class);
196201
$attachmentService->getService('deck_file_invalid');
197202
}
@@ -252,9 +257,12 @@ public function testFindAllWithDeleted() {
252257
->method('findAll')
253258
->with(123)
254259
->willReturn($attachments);
260+
$this->configService->expects($this->once())
261+
->method('getTrashRetention')
262+
->willReturn(3600);
255263
$this->attachmentMapper->expects($this->once())
256264
->method('findToDelete')
257-
->with(123, false)
265+
->with($this->anything(), 123, false)
258266
->willReturn($attachmentsDeleted);
259267

260268
$this->attachmentServiceImpl->expects($this->exactly(4))

0 commit comments

Comments
 (0)