Skip to content

Commit d52ca5c

Browse files
mejo-mejo-
authored
Merge pull request #8243 from nextcloud/backport/8242/stable32
[stable32] fix(trash): allow to configure the trash retention in hours
2 parents 37c5ef6 + 986ef7f commit d52ca5c

12 files changed

Lines changed: 53 additions & 22 deletions

‎lib/Cron/DeleteCron.php‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,15 @@
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;
2021
use OCP\BackgroundJob\TimedJob;
2122

2223
class DeleteCron extends TimedJob {
2324

25+
private $configService;
2426
/** @var BoardMapper */
2527
private $boardMapper;
2628
/** @var CardMapper */
@@ -36,6 +38,7 @@ class DeleteCron extends TimedJob {
3638

3739
public function __construct(
3840
ITimeFactory $time,
41+
ConfigService $configService,
3942
BoardMapper $boardMapper,
4043
CardMapper $cardMapper,
4144
AttachmentService $attachmentService,
@@ -44,14 +47,15 @@ public function __construct(
4447
DeckShareProvider $deckShareProvider,
4548
) {
4649
parent::__construct($time);
50+
$this->configService = $configService;
4751
$this->boardMapper = $boardMapper;
4852
$this->cardMapper = $cardMapper;
4953
$this->attachmentService = $attachmentService;
5054
$this->attachmentMapper = $attachmentMapper;
5155
$this->stackMapper = $stackMapper;
5256
$this->deckShareProvider = $deckShareProvider;
5357

54-
$this->setInterval(60 * 60 * 24);
58+
$this->setInterval(60 * 60); // Run once every hour
5559
$this->setTimeSensitivity(IJob::TIME_INSENSITIVE);
5660
}
5761

@@ -60,18 +64,19 @@ public function __construct(
6064
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
6165
*/
6266
protected function run($argument) {
63-
$boards = $this->boardMapper->findToDelete();
67+
$timeLimit = time() - $this->configService->getTrashRetention();
68+
69+
$boards = $this->boardMapper->findToDelete($timeLimit);
6470
foreach ($boards as $board) {
6571
$this->boardMapper->delete($board);
6672
}
6773

68-
$timeLimit = time() - (60 * 5); // 5 min buffer
6974
$cards = $this->cardMapper->findToDelete($timeLimit, 500);
7075
foreach ($cards as $card) {
7176
$this->cardMapper->delete($card);
7277
}
7378

74-
$attachments = $this->attachmentMapper->findToDelete();
79+
$attachments = $this->attachmentMapper->findToDelete($timeLimit);
7580
foreach ($attachments as $attachment) {
7681
try {
7782
$service = $this->attachmentService->getService($attachment->getType());
@@ -88,7 +93,7 @@ protected function run($argument) {
8893
$this->deckShareProvider->delete($share);
8994
}
9095

91-
$stacks = $this->stackMapper->findToDelete();
96+
$stacks = $this->stackMapper->findToDelete($timeLimit);
9297
foreach ($stacks as $stack) {
9398
$this->stackMapper->delete($stack);
9499
}

‎lib/Db/AttachmentMapper.php‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,7 @@ public function findAll($cardId) {
9090
* @param bool $withOffset
9191
* @return array
9292
*/
93-
public function findToDelete($cardId = null, $withOffset = true) {
94-
// add buffer of 5 min
95-
$timeLimit = time() - (60 * 5);
93+
public function findToDelete(int $timeLimit, $cardId = null, $withOffset = true) {
9694
$qb = $this->db->getQueryBuilder();
9795
$qb->select('*')
9896
->from($this->getTableName())

‎lib/Db/BoardMapper.php‎

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

443-
public function findToDelete() {
444-
// add buffer of 5 min
445-
$timeLimit = time() - (60 * 5);
443+
public function findToDelete(int $timeLimit) {
446444
$qb = $this->db->getQueryBuilder();
447445
$qb->select('id', 'title', 'owner', 'color', 'archived', 'deleted_at', 'last_modified')
448446
->from('deck_boards')

‎lib/Db/StackMapper.php‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -165,9 +165,7 @@ public function findBoardId($id): ?int {
165165
return $result !== false ? $result : null;
166166
}
167167

168-
public function findToDelete(): array {
169-
// add buffer of 5 min
170-
$timeLimit = time() - (60 * 5);
168+
public function findToDelete(int $timeLimit): array {
171169
$qb = $this->db->getQueryBuilder();
172170
$qb->select('*')
173171
->from($this->getTableName())

‎lib/Service/AttachmentService.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
use OCP\IUserManager;
2828

2929
class AttachmentService {
30+
private $configService;
3031
private $attachmentMapper;
3132
private $cardMapper;
3233
private $permissionService;
@@ -49,6 +50,7 @@ class AttachmentService {
4950
private AttachmentServiceValidator $attachmentServiceValidator;
5051

5152
public function __construct(
53+
ConfigService $configService,
5254
AttachmentMapper $attachmentMapper,
5355
CardMapper $cardMapper,
5456
IUserManager $userManager,
@@ -61,6 +63,7 @@ public function __construct(
6163
ActivityManager $activityManager,
6264
AttachmentServiceValidator $attachmentServiceValidator,
6365
) {
66+
$this->configService = $configService;
6467
$this->attachmentMapper = $attachmentMapper;
6568
$this->cardMapper = $cardMapper;
6669
$this->permissionService = $permissionService;
@@ -115,7 +118,8 @@ public function findAll($cardId, $withDeleted = false) {
115118

116119
$attachments = $this->attachmentMapper->findAll($cardId);
117120
if ($withDeleted) {
118-
$attachments = array_merge($attachments, $this->attachmentMapper->findToDelete($cardId, false));
121+
$timeLimit = time() - $this->configService->getTrashRetention();
122+
$attachments = array_merge($attachments, $this->attachmentMapper->findToDelete($timeLimit, $cardId, false));
119123
}
120124

121125
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
@@ -227,4 +227,10 @@ public function setAttachmentFolder(?string $userId, string $path): void {
227227

228228
$this->config->setUserValue($userId ?? $this->getUserId(), 'deck', 'attachment_folder', $path);
229229
}
230+
231+
public function getTrashRetention(): int {
232+
$value = $this->config->getAppValue(Application::APP_ID, 'trashRetentionHours', '5');
233+
$hours = (int)$value > 0 ? (int)$value : 5;
234+
return 60 * 60 * $hours;
235+
}
230236
}

‎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
@@ -152,7 +152,8 @@ public function testFindAllToDelete() {
152152
$this->boards[0]->setDeletedAt(1);
153153
$this->boards[0] = $this->boardMapper->update($this->boards[0]);
154154

155-
$actual = $this->boardMapper->findToDelete();
155+
$timeLimit = time() - (60 * 60 * 5);
156+
$actual = $this->boardMapper->findToDelete($timeLimit);
156157
$this->boards[0]->resetUpdatedFields();
157158
$this->assertEquals([$this->boards[0]], $actual);
158159

‎tests/unit/Service/AttachmentServiceTest.php‎

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

6666
/** @var IUserManager|MockObject */
6767
private $userManager;
68+
/** @var ConfigService */
69+
private $configService;
6870
/** @var AttachmentMapper|MockObject */
6971
private $attachmentMapper;
7072
/** @var CardMapper|MockObject */
@@ -107,6 +109,8 @@ public function setUp(): void {
107109

108110
$this->appContainer = $this->createMock(IAppContainer::class);
109111

112+
$this->configService = $this->createMock(ConfigService::class);
113+
110114
$this->userManager = $this->createMock(IUserManager::class);
111115
$this->attachmentMapper = $this->createMock(AttachmentMapper::class);
112116
$this->cardMapper = $this->createMock(CardMapper::class);
@@ -135,6 +139,7 @@ public function setUp(): void {
135139
$this->attachmentServiceValidator = $this->createMock(AttachmentServiceValidator::class);
136140

137141
$this->attachmentService = new AttachmentService(
142+
$this->configService,
138143
$this->attachmentMapper,
139144
$this->cardMapper,
140145
$this->userManager,
@@ -171,7 +176,7 @@ public function testRegisterAttachmentService() {
171176
$application->expects($this->any())
172177
->method('getContainer')
173178
->willReturn($appContainer);
174-
$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator);
179+
$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);
175180
$attachmentService->registerAttachmentService('custom', MyAttachmentService::class);
176181
$this->assertEquals($fileServiceMock, $attachmentService->getService('deck_file'));
177182
$this->assertEquals(MyAttachmentService::class, get_class($attachmentService->getService('custom')));
@@ -201,7 +206,7 @@ public function testRegisterAttachmentServiceNotExisting() {
201206
->method('getContainer')
202207
->willReturn($appContainer);
203208

204-
$attachmentService = new AttachmentService($this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator);
209+
$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);
205210
$attachmentService->registerAttachmentService('custom', MyAttachmentService::class);
206211
$attachmentService->getService('deck_file_invalid');
207212
}
@@ -262,9 +267,12 @@ public function testFindAllWithDeleted() {
262267
->method('findAll')
263268
->with(123)
264269
->willReturn($attachments);
270+
$this->configService->expects($this->once())
271+
->method('getTrashRetention')
272+
->willReturn(3600);
265273
$this->attachmentMapper->expects($this->once())
266274
->method('findToDelete')
267-
->with(123, false)
275+
->with($this->anything(), 123, false)
268276
->willReturn($attachmentsDeleted);
269277

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

0 commit comments

Comments
 (0)