Skip to content

Commit 43d5ad1

Browse files
mejo-backportbot[bot]
mejo-
authored and
backportbot[bot]
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 09c5cd5 commit 43d5ad1

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
@@ -14,6 +14,7 @@
1414
use OCA\Deck\Db\StackMapper;
1515
use OCA\Deck\InvalidAttachmentType;
1616
use OCA\Deck\Service\AttachmentService;
17+
use OCA\Deck\Service\ConfigService;
1718
use OCA\Deck\Sharing\DeckShareProvider;
1819
use OCP\AppFramework\Utility\ITimeFactory;
1920
use OCP\BackgroundJob\IJob;
@@ -36,6 +37,7 @@ class DeleteCron extends TimedJob {
3637

3738
public function __construct(
3839
ITimeFactory $time,
40+
private readonly ConfigService $configService,
3941
BoardMapper $boardMapper,
4042
CardMapper $cardMapper,
4143
AttachmentService $attachmentService,
@@ -51,7 +53,7 @@ public function __construct(
5153
$this->stackMapper = $stackMapper;
5254
$this->deckShareProvider = $deckShareProvider;
5355

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

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

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

74-
$attachments = $this->attachmentMapper->findToDelete();
77+
$attachments = $this->attachmentMapper->findToDelete($timeLimit);
7578
foreach ($attachments as $attachment) {
7679
try {
7780
$service = $this->attachmentService->getService($attachment->getType());
@@ -88,7 +91,7 @@ protected function run($argument) {
8891
$this->deckShareProvider->delete($share);
8992
}
9093

91-
$stacks = $this->stackMapper->findToDelete();
94+
$stacks = $this->stackMapper->findToDelete($timeLimit);
9295
foreach ($stacks as $stack) {
9396
$this->stackMapper->delete($stack);
9497
}

‎lib/Db/AttachmentMapper.php‎

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

‎lib/Db/BoardMapper.php‎

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

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

‎lib/Db/StackMapper.php‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -220,9 +220,7 @@ public function findBoardId(int $id): ?int {
220220
* @return array<Stack>
221221
* @throws \OCP\DB\Exception
222222
*/
223-
public function findToDelete(): array {
224-
// add buffer of 5 min
225-
$timeLimit = time() - (60 * 5);
223+
public function findToDelete(int $timeLimit): array {
226224
$qb = $this->db->getQueryBuilder();
227225
$qb->select('*')
228226
->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
@@ -252,4 +252,10 @@ public function setAttachmentFolder(?string $userId, string $path): void {
252252

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

‎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 */
@@ -109,6 +111,8 @@ public function setUp(): void {
109111

110112
$this->appContainer = $this->createMock(IAppContainer::class);
111113

114+
$this->configService = $this->createMock(ConfigService::class);
115+
112116
$this->userManager = $this->createMock(IUserManager::class);
113117
$this->attachmentMapper = $this->createMock(AttachmentMapper::class);
114118
$this->cardMapper = $this->createMock(CardMapper::class);
@@ -137,6 +141,7 @@ public function setUp(): void {
137141
$this->attachmentServiceValidator = $this->createMock(AttachmentServiceValidator::class);
138142

139143
$this->attachmentService = new AttachmentService(
144+
$this->configService,
140145
$this->attachmentMapper,
141146
$this->cardMapper,
142147
$this->userManager,
@@ -173,7 +178,7 @@ public function testRegisterAttachmentService() {
173178
$application->expects($this->any())
174179
->method('getContainer')
175180
->willReturn($appContainer);
176-
$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator);
181+
$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);
177182
$attachmentService->registerAttachmentService('custom', MyAttachmentService::class);
178183
$this->assertEquals($fileServiceMock, $attachmentService->getService('deck_file'));
179184
$this->assertEquals(MyAttachmentService::class, get_class($attachmentService->getService('custom')));
@@ -203,7 +208,7 @@ public function testRegisterAttachmentServiceNotExisting() {
203208
->method('getContainer')
204209
->willReturn($appContainer);
205210

206-
$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator);
211+
$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);
207212
$attachmentService->registerAttachmentService('custom', MyAttachmentService::class);
208213
$attachmentService->getService('deck_file_invalid');
209214
}
@@ -264,9 +269,12 @@ public function testFindAllWithDeleted() {
264269
->method('findAll')
265270
->with(123)
266271
->willReturn($attachments);
272+
$this->configService->expects($this->once())
273+
->method('getTrashRetention')
274+
->willReturn(3600);
267275
$this->attachmentMapper->expects($this->once())
268276
->method('findToDelete')
269-
->with(123, false)
277+
->with($this->anything(), 123, false)
270278
->willReturn($attachmentsDeleted);
271279

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

0 commit comments

Comments
 (0)