From eb41283573f85cbef11bcfe40059b7930bea9802 Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Mon, 8 Sep 2025 14:28:37 +0200 Subject: [PATCH 1/2] fix: schedule repair sync job when adding an account Signed-off-by: Daniel Kesselberg --- lib/Migration/FixBackgroundJobs.php | 26 +++------- lib/Service/AccountService.php | 37 +++++++++++--- tests/Unit/Service/AccountServiceTest.php | 36 ++++++++++--- .../Unit/Service/Provisioning/ManagerTest.php | 51 ++++++++++--------- 4 files changed, 96 insertions(+), 54 deletions(-) diff --git a/lib/Migration/FixBackgroundJobs.php b/lib/Migration/FixBackgroundJobs.php index a0c769acb1..19dc0d6c49 100644 --- a/lib/Migration/FixBackgroundJobs.php +++ b/lib/Migration/FixBackgroundJobs.php @@ -8,25 +8,16 @@ namespace OCA\Mail\Migration; -use OCA\Mail\BackgroundJob\PreviewEnhancementProcessingJob; -use OCA\Mail\BackgroundJob\QuotaJob; -use OCA\Mail\BackgroundJob\RepairSyncJob; -use OCA\Mail\BackgroundJob\SyncJob; -use OCA\Mail\BackgroundJob\TrainImportanceClassifierJob; use OCA\Mail\Db\MailAccountMapper; -use OCP\BackgroundJob\IJobList; +use OCA\Mail\Service\AccountService; use OCP\Migration\IOutput; use OCP\Migration\IRepairStep; class FixBackgroundJobs implements IRepairStep { - /** @var IJobList */ - private $jobList; - /** @var MailAccountMapper */ - private $mapper; - - public function __construct(IJobList $jobList, MailAccountMapper $mapper) { - $this->jobList = $jobList; - $this->mapper = $mapper; + public function __construct( + private MailAccountMapper $mapper, + private AccountService $accountService, + ) { } #[\Override] @@ -43,13 +34,10 @@ public function run(IOutput $output) { $output->startProgress(count($accounts)); foreach ($accounts as $account) { - $this->jobList->add(SyncJob::class, ['accountId' => $account->getId()]); - $this->jobList->add(RepairSyncJob::class, ['accountId' => $account->getId()]); - $this->jobList->add(TrainImportanceClassifierJob::class, ['accountId' => $account->getId()]); - $this->jobList->add(PreviewEnhancementProcessingJob::class, ['accountId' => $account->getId()]); - $this->jobList->add(QuotaJob::class, ['accountId' => $account->getId()]); + $this->accountService->scheduleBackgroundJobs($account->getId()); $output->advance(); } + $output->finishProgress(); } } diff --git a/lib/Service/AccountService.php b/lib/Service/AccountService.php index 55c3bda87d..b717d1c2d0 100644 --- a/lib/Service/AccountService.php +++ b/lib/Service/AccountService.php @@ -14,6 +14,7 @@ use OCA\Mail\AppInfo\Application; use OCA\Mail\BackgroundJob\PreviewEnhancementProcessingJob; use OCA\Mail\BackgroundJob\QuotaJob; +use OCA\Mail\BackgroundJob\RepairSyncJob; use OCA\Mail\BackgroundJob\SyncJob; use OCA\Mail\BackgroundJob\TrainImportanceClassifierJob; use OCA\Mail\Db\MailAccount; @@ -23,6 +24,7 @@ use OCA\Mail\IMAP\IMAPClientFactory; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\BackgroundJob\IJob; use OCP\BackgroundJob\IJobList; use OCP\IConfig; use function array_map; @@ -53,7 +55,7 @@ public function __construct( IJobList $jobList, IMAPClientFactory $imapClientFactory, private readonly IConfig $config, - private readonly ITimeFactory $time, + private readonly ITimeFactory $timeFactory, ) { $this->mapper = $mapper; $this->aliasesService = $aliasesService; @@ -176,17 +178,14 @@ public function save(MailAccount $newAccount): MailAccount { $newAccount = $this->mapper->save($newAccount); // Insert background jobs for this account - $this->jobList->add(SyncJob::class, ['accountId' => $newAccount->getId()]); - $this->jobList->add(TrainImportanceClassifierJob::class, ['accountId' => $newAccount->getId()]); - $this->jobList->add(PreviewEnhancementProcessingJob::class, ['accountId' => $newAccount->getId()]); - $this->jobList->add(QuotaJob::class, ['accountId' => $newAccount->getId()]); + $this->scheduleBackgroundJobs($newAccount->getId()); // Set initial heartbeat $this->config->setUserValue( $newAccount->getUserId(), Application::APP_ID, 'ui-heartbeat', - (string)$this->time->getTime(), + (string)$this->timeFactory->getTime(), ); return $newAccount; @@ -235,4 +234,30 @@ public function testAccountConnection(string $currentUserId, int $accountId) :bo return false; } } + + public function scheduleBackgroundJobs(int $accountId): void { + $arguments = ['accountId' => $accountId]; + + $now = $this->timeFactory->getTime(); + $this->scheduleBackgroundJob(SyncJob::class, $now, $arguments); + $this->scheduleBackgroundJob(TrainImportanceClassifierJob::class, $now, $arguments); + $this->scheduleBackgroundJob(PreviewEnhancementProcessingJob::class, $now, $arguments); + $this->scheduleBackgroundJob(QuotaJob::class, $now, $arguments); + + $inThreeDays = $now + (3 * 86400); + $this->scheduleBackgroundJob(RepairSyncJob::class, $inThreeDays, $arguments); + } + + /** + * IJobList::add() / IJobList::scheduleAfter() resets last_run, last_check, and reserved_at if the job exists. + * To avoid unwanted resets (e.g. when enabling debug mode), we check first if the job is already present. + * + * @param class-string $job + * @param mixed $argument The serializable argument to be passed to $job->run() when the job is executed + */ + private function scheduleBackgroundJob(string $job, int $runAfter, mixed $argument = null): void { + if (!$this->jobList->has($job, $argument)) { + $this->jobList->scheduleAfter($job, $runAfter, $argument); + } + } } diff --git a/tests/Unit/Service/AccountServiceTest.php b/tests/Unit/Service/AccountServiceTest.php index 46824285a2..d9c11bdeb5 100644 --- a/tests/Unit/Service/AccountServiceTest.php +++ b/tests/Unit/Service/AccountServiceTest.php @@ -11,6 +11,8 @@ use ChristophWurst\Nextcloud\Testing\TestCase; use Horde_Imap_Client_Socket; use OCA\Mail\Account; +use OCA\Mail\BackgroundJob\QuotaJob; +use OCA\Mail\BackgroundJob\SyncJob; use OCA\Mail\Db\MailAccount; use OCA\Mail\Db\MailAccountMapper; use OCA\Mail\Exception\ClientException; @@ -163,24 +165,31 @@ public function testDeleteByAccountId() { } public function testSave() { - $account = new MailAccount(); - $account->setUserId('user1'); + $mailAccount = new MailAccount(); + $mailAccount->setId(1000); + $mailAccount->setUserId('user1'); $this->mapper->expects($this->once()) ->method('save') - ->with($account) + ->with($mailAccount) ->will($this->returnArgument(0)); - $this->time->expects(self::once()) + $this->time->expects(self::exactly(2)) ->method('getTime') ->willReturn(1755850409); + + $this->jobList->method('has') + ->willReturn(false); + $this->jobList->expects($this->exactly(5)) + ->method('scheduleAfter'); + $this->config->expects(self::once()) ->method('setUserValue') ->with('user1', 'mail', 'ui-heartbeat', 1755850409); - $actual = $this->accountService->save($account); + $actual = $this->accountService->save($mailAccount); - $this->assertEquals($account, $actual); + $this->assertEquals($mailAccount, $actual); } public function testUpdateSignature() { @@ -228,4 +237,19 @@ public function testAccountsSuccesfulConnection() { $connected = $this->accountService->testAccountConnection($this->user, $accountId); $this->assertTrue($connected); } + + public function testScheduleBackgroundJobs(): void { + $mailAccountId = 1000; + $this->time->expects(self::once()) + ->method('getTime') + ->willReturn(1755850409); + $this->jobList->method('has') + ->willReturnCallback(function ($job) { + return $job === SyncJob::class || $job === QuotaJob::class; + }); + $this->jobList->expects($this->exactly(3)) + ->method('scheduleAfter'); + + $this->accountService->scheduleBackgroundJobs($mailAccountId); + } } diff --git a/tests/Unit/Service/Provisioning/ManagerTest.php b/tests/Unit/Service/Provisioning/ManagerTest.php index f9f1f9181f..f37e3fe4aa 100644 --- a/tests/Unit/Service/Provisioning/ManagerTest.php +++ b/tests/Unit/Service/Provisioning/ManagerTest.php @@ -72,15 +72,16 @@ public function testUpdateProvisionSingleUser() { $config->setProvisioningDomain('batman.com'); $config->setEmailTemplate('%USER%@batman.com'); $configs = [$config]; - $account = new MailAccount(); + $mailAccount = new MailAccount(); + $mailAccount->setId(1000); $this->mock->getParameter('mailAccountMapper') ->expects($this->once()) ->method('findProvisionedAccount') - ->willReturn($account); + ->willReturn($mailAccount); $this->mock->getParameter('mailAccountMapper') ->expects($this->once()) ->method('update') - ->with($account); + ->with($mailAccount); $result = $this->manager->provisionSingleUser($configs, $user); $this->assertTrue($result); @@ -92,7 +93,8 @@ public function testProvisionSingleUser() { 'getEmailAddress' => 'bruce.wayne@batman.com', 'getUID' => 'bruce' ]); - $account = new MailAccount(); + $mailAccount = new MailAccount(); + $mailAccount->setId(1000); $config = new Provisioning(); $config->setId(1); $config->setProvisioningDomain('batman.com'); @@ -105,11 +107,11 @@ public function testProvisionSingleUser() { $this->mock->getParameter('mailAccountMapper') ->expects($this->once()) ->method('insert') - ->willReturn($account); + ->willReturn($mailAccount); $this->mock->getParameter('tagMapper') ->expects($this->once()) ->method('createDefaultTags') - ->with($account); + ->with($mailAccount); $result = $this->manager->provisionSingleUser($configs, $user); $this->assertTrue($result); @@ -121,21 +123,21 @@ public function testUpdateProvisionSingleUserWithWildcard() { 'getEmailAddress' => 'bruce.wayne@batman.com', 'getUID' => 'bruce.wayne' ]); - $account = new MailAccount(); $config = new Provisioning(); $config->setId(1); $config->setProvisioningDomain('*'); $config->setEmailTemplate('%USER%@batman.com'); $configs = [$config]; - $account = new MailAccount(); + $mailAccount = new MailAccount(); + $mailAccount->setId(1000); $this->mock->getParameter('mailAccountMapper') ->expects($this->once()) ->method('findProvisionedAccount') - ->willReturn($account); + ->willReturn($mailAccount); $this->mock->getParameter('mailAccountMapper') ->expects($this->once()) ->method('update') - ->with($account); + ->with($mailAccount); $result = $this->manager->provisionSingleUser($configs, $user); $this->assertTrue($result); @@ -147,7 +149,8 @@ public function testProvisionSingleUserWithWildcard() { 'getEmailAddress' => 'bruce.wayne@batman.com', 'getUID' => 'bruce' ]); - $account = new MailAccount(); + $mailAccount = new MailAccount(); + $mailAccount->setId(1000); $config = new Provisioning(); $config->setId(1); $config->setProvisioningDomain('*'); @@ -160,11 +163,11 @@ public function testProvisionSingleUserWithWildcard() { $this->mock->getParameter('mailAccountMapper') ->expects($this->once()) ->method('insert') - ->willReturn($account); + ->willReturn($mailAccount); $this->mock->getParameter('tagMapper') ->expects($this->once()) ->method('createDefaultTags') - ->with($account); + ->with($mailAccount); $result = $this->manager->provisionSingleUser($configs, $user); $this->assertTrue($result); @@ -175,7 +178,6 @@ public function testProvisionSingleUserNoDomainMatch() { $user = $this->createConfiguredMock(IUser::class, [ 'getEmailAddress' => 'bruce.wayne@batman.com' ]); - $account = new MailAccount(); $config = new Provisioning(); $config->setId(1); $config->setProvisioningDomain('arkham-asylum.com'); @@ -224,18 +226,19 @@ public function testUpdatePasswordNotProvisioned(): void { public function testUpdateLoginPassword(): void { /** @var IUser|MockObject $user */ $user = $this->createMock(IUser::class); - $account = new MailAccount(); + $mailAccount = new MailAccount(); + $mailAccount->setId(1000); $this->mock->getParameter('mailAccountMapper') ->expects($this->once()) ->method('findProvisionedAccount') - ->willReturn($account); + ->willReturn($mailAccount); $config = new Provisioning(); $config->setProvisioningDomain(Provisioning::WILDCARD); $config->setMasterPasswordEnabled(false); $this->mock->getParameter('mailAccountMapper') ->expects($this->once()) ->method('update') - ->with($account); + ->with($mailAccount); $this->manager->updatePassword($user, '123456', [$config]); } @@ -243,11 +246,12 @@ public function testUpdateLoginPassword(): void { public function testUpdateMasterPasswordWithExistingLoginPassword(): void { /** @var IUser|MockObject $user */ $user = $this->createMock(IUser::class); - $account = new MailAccount(); + $mailAccount = new MailAccount(); + $mailAccount->setId(1000); $this->mock->getParameter('mailAccountMapper') ->expects($this->once()) ->method('findProvisionedAccount') - ->willReturn($account); + ->willReturn($mailAccount); $config = new Provisioning(); $config->setProvisioningDomain(Provisioning::WILDCARD); $config->setMasterPasswordEnabled(true); @@ -260,7 +264,7 @@ public function testUpdateMasterPasswordWithExistingLoginPassword(): void { $this->mock->getParameter('mailAccountMapper') ->expects($this->once()) ->method('update') - ->with($account); + ->with($mailAccount); $this->manager->updatePassword($user, '123456', [$config]); } @@ -268,11 +272,12 @@ public function testUpdateMasterPasswordWithExistingLoginPassword(): void { public function testUpdateMasterPasswordWithoutLoginPassword(): void { /** @var IUser|MockObject $user */ $user = $this->createMock(IUser::class); - $account = new MailAccount(); + $mailAccount = new MailAccount(); + $mailAccount->setId(1000); $this->mock->getParameter('mailAccountMapper') ->expects($this->once()) ->method('findProvisionedAccount') - ->willReturn($account); + ->willReturn($mailAccount); $config = new Provisioning(); $config->setProvisioningDomain(Provisioning::WILDCARD); $config->setMasterPasswordEnabled(true); @@ -285,7 +290,7 @@ public function testUpdateMasterPasswordWithoutLoginPassword(): void { $this->mock->getParameter('mailAccountMapper') ->expects($this->once()) ->method('update') - ->with($account); + ->with($mailAccount); $this->manager->updatePassword($user, null, [$config]); } From 5f9a3131ac6f31680255c616bc26e7938d1752a5 Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Tue, 9 Sep 2025 13:19:37 +0200 Subject: [PATCH 2/2] fix: schedule jobs on account provisioning Signed-off-by: Daniel Kesselberg --- lib/Service/Provisioning/Manager.php | 9 +++++++-- tests/Unit/Service/Provisioning/ManagerTest.php | 3 +++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/Service/Provisioning/Manager.php b/lib/Service/Provisioning/Manager.php index 42c0abd5e1..ce117cc423 100644 --- a/lib/Service/Provisioning/Manager.php +++ b/lib/Service/Provisioning/Manager.php @@ -18,6 +18,7 @@ use OCA\Mail\Db\ProvisioningMapper; use OCA\Mail\Db\TagMapper; use OCA\Mail\Exception\ValidationException; +use OCA\Mail\Service\AccountService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\ICacheFactory; @@ -56,7 +57,8 @@ class Manager { /** @var ICacheFactory */ private $cacheFactory; - public function __construct(IUserManager $userManager, + public function __construct( + IUserManager $userManager, ProvisioningMapper $provisioningMapper, MailAccountMapper $mailAccountMapper, ICrypto $crypto, @@ -64,7 +66,9 @@ public function __construct(IUserManager $userManager, AliasMapper $aliasMapper, LoggerInterface $logger, TagMapper $tagMapper, - ICacheFactory $cacheFactory) { + ICacheFactory $cacheFactory, + private AccountService $accountService, + ) { $this->userManager = $userManager; $this->provisioningMapper = $provisioningMapper; $this->mailAccountMapper = $mailAccountMapper; @@ -211,6 +215,7 @@ public function provisionSingleUser(array $provisionings, IUser $user): bool { $this->updateAccount($user, $mailAccount, $provisioning) ); + $this->accountService->scheduleBackgroundJobs($mailAccount->getId()); $this->tagMapper->createDefaultTags($mailAccount); } diff --git a/tests/Unit/Service/Provisioning/ManagerTest.php b/tests/Unit/Service/Provisioning/ManagerTest.php index f37e3fe4aa..3b29412b45 100644 --- a/tests/Unit/Service/Provisioning/ManagerTest.php +++ b/tests/Unit/Service/Provisioning/ManagerTest.php @@ -112,6 +112,9 @@ public function testProvisionSingleUser() { ->expects($this->once()) ->method('createDefaultTags') ->with($mailAccount); + $this->mock->getParameter('accountService') + ->expects($this->once()) + ->method('scheduleBackgroundJobs'); $result = $this->manager->provisionSingleUser($configs, $user); $this->assertTrue($result);