diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 6af8cf630d..559b404630 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -55,6 +55,7 @@ use OCA\Mail\Listener\SpamReportListener; use OCA\Mail\Listener\TaskProcessingListener; use OCA\Mail\Listener\UserDeletedListener; +use OCA\Mail\Listener\UserEnabledDisabledListener; use OCA\Mail\Notification\Notifier; use OCA\Mail\Provider\MailProvider; use OCA\Mail\Search\FilteringProvider; @@ -85,6 +86,7 @@ use OCP\User\Events\OutOfOfficeEndedEvent; use OCP\User\Events\OutOfOfficeScheduledEvent; use OCP\User\Events\OutOfOfficeStartedEvent; +use OCP\User\Events\UserChangedEvent; use OCP\User\Events\UserDeletedEvent; use OCP\Util; use Psr\Container\ContainerInterface; @@ -146,6 +148,7 @@ public function register(IRegistrationContext $context): void { $context->registerEventListener(NewMessagesSynchronized::class, NewMessagesNotifier::class); $context->registerEventListener(NewMessagesSynchronized::class, NewMessagesSummarizeListener::class); $context->registerEventListener(SynchronizationEvent::class, AccountSynchronizedThreadUpdaterListener::class); + $context->registerEventListener(UserChangedEvent::class, UserEnabledDisabledListener::class); $context->registerEventListener(UserDeletedEvent::class, UserDeletedListener::class); $context->registerEventListener(NewMessagesSynchronized::class, FollowUpClassifierListener::class); $context->registerEventListener(OutOfOfficeStartedEvent::class, OutOfOfficeListener::class); diff --git a/lib/Listener/UserEnabledDisabledListener.php b/lib/Listener/UserEnabledDisabledListener.php new file mode 100644 index 0000000000..d18fe26847 --- /dev/null +++ b/lib/Listener/UserEnabledDisabledListener.php @@ -0,0 +1,52 @@ + + */ +class UserEnabledDisabledListener implements IEventListener { + public function __construct( + private AccountService $accountService, + private LoggerInterface $logger, + ) { + } + + #[\Override] + public function handle(Event $event): void { + if (!($event instanceof UserChangedEvent) || $event->getFeature() !== 'enabled') { + return; + } + + $enabled = (bool)$event->getValue(); + $uid = $event->getUser()->getUID(); + try { + foreach ($this->accountService->findByUserId($uid) as $account) { + if ($enabled) { + $this->accountService->scheduleBackgroundJobs($account->getId()); + } else { + $this->accountService->removeBackgroundJobs($account->getId()); + } + } + } catch (Throwable $e) { + $this->logger->error('Could not update Mail background jobs after user {uid} was ' . ($enabled ? 'enabled' : 'disabled'), [ + 'uid' => $uid, + 'exception' => $e, + ]); + } + } +} diff --git a/lib/Migration/FixBackgroundJobs.php b/lib/Migration/FixBackgroundJobs.php index 081e74347b..e8b0fd09c1 100644 --- a/lib/Migration/FixBackgroundJobs.php +++ b/lib/Migration/FixBackgroundJobs.php @@ -10,6 +10,7 @@ use OCA\Mail\Db\MailAccountMapper; use OCA\Mail\Service\AccountService; +use OCP\IUserManager; use OCP\Migration\IOutput; use OCP\Migration\IRepairStep; use function method_exists; @@ -21,12 +22,13 @@ class FixBackgroundJobs implements IRepairStep { public function __construct( private MailAccountMapper $mapper, private AccountService $accountService, + private IUserManager $userManager, ) { } #[\Override] public function getName(): string { - return 'Insert background jobs for all accounts'; + return 'Reconcile background jobs for all accounts'; } /** @@ -34,8 +36,9 @@ public function getName(): string { */ #[\Override] public function run(IOutput $output) { - // Skip if method does not exist yet during upgrade - if (!method_exists($this->accountService, 'scheduleBackgroundJobs')) { + // Skip if methods do not exist yet during upgrade + if (!method_exists($this->accountService, 'scheduleBackgroundJobs') + || !method_exists($this->accountService, 'removeBackgroundJobs')) { return; } @@ -43,7 +46,12 @@ public function run(IOutput $output) { $output->startProgress(count($accounts)); foreach ($accounts as $account) { - $this->accountService->scheduleBackgroundJobs($account->getId()); + $user = $this->userManager->get($account->getUserId()); + if ($user === null || !$user->isEnabled()) { + $this->accountService->removeBackgroundJobs($account->getId()); + } else { + $this->accountService->scheduleBackgroundJobs($account->getId()); + } $output->advance(); } diff --git a/lib/Service/AccountService.php b/lib/Service/AccountService.php index 256b546e98..d4ba50542d 100644 --- a/lib/Service/AccountService.php +++ b/lib/Service/AccountService.php @@ -31,6 +31,18 @@ use function array_map; class AccountService { + /** + * @var list> + */ + private const PER_ACCOUNT_JOBS = [ + SyncJob::class, + TrainImportanceClassifierJob::class, + PreviewEnhancementProcessingJob::class, + QuotaJob::class, + ScheduleJob::class, + RepairSyncJob::class, + ]; + /** * Cache accounts for multiple calls to 'findByUserId' * @@ -246,14 +258,12 @@ 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); - $this->scheduleBackgroundJob(ScheduleJob::class, $now, $arguments); - $inThreeDays = $now + (3 * 86400); - $this->scheduleBackgroundJob(RepairSyncJob::class, $inThreeDays, $arguments); + foreach (self::PER_ACCOUNT_JOBS as $job) { + // Defer the expensive full repair sync so it doesn't compete with the initial sync + $runAfter = $job === RepairSyncJob::class ? $inThreeDays : $now; + $this->scheduleBackgroundJob($job, $runAfter, $arguments); + } } /** @@ -268,4 +278,11 @@ private function scheduleBackgroundJob(string $job, int $runAfter, mixed $argume $this->jobList->scheduleAfter($job, $runAfter, $argument); } } + + public function removeBackgroundJobs(int $accountId): void { + $arguments = ['accountId' => $accountId]; + foreach (self::PER_ACCOUNT_JOBS as $job) { + $this->jobList->remove($job, $arguments); + } + } } diff --git a/tests/Unit/Listener/UserEnabledDisabledListenerTest.php b/tests/Unit/Listener/UserEnabledDisabledListenerTest.php new file mode 100644 index 0000000000..ec1d204583 --- /dev/null +++ b/tests/Unit/Listener/UserEnabledDisabledListenerTest.php @@ -0,0 +1,124 @@ +accountService = $this->createMock(AccountService::class); + $this->logger = $this->createMock(LoggerInterface::class); + + $this->listener = new UserEnabledDisabledListener( + $this->accountService, + $this->logger, + ); + } + + private function createUserMock(string $userId): IUser&MockObject { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($userId); + return $user; + } + + private function createAccountMock(int $id): Account { + $mailAccount = new MailAccount(); + $mailAccount->setId($id); + return new Account($mailAccount); + } + + public function testHandleUnrelatedEvent(): void { + $this->accountService->expects($this->never()) + ->method('findByUserId'); + + $this->listener->handle(new Event()); + } + + public function testHandleUnrelatedFeature(): void { + $event = new UserChangedEvent($this->createUserMock('user'), 'displayName', 'New name'); + + $this->accountService->expects($this->never()) + ->method('findByUserId'); + + $this->listener->handle($event); + } + + public function testHandleDisableRemovesJobs(): void { + $user = $this->createUserMock('test-user'); + $event = new UserChangedEvent($user, 'enabled', false, true); + + $this->accountService->expects($this->once()) + ->method('findByUserId') + ->with('test-user') + ->willReturn([$this->createAccountMock(1), $this->createAccountMock(2)]); + + $this->accountService->expects($this->never()) + ->method('scheduleBackgroundJobs'); + $this->accountService->expects($this->exactly(2)) + ->method('removeBackgroundJobs') + ->willReturnCallback(function (int $accountId): void { + $this->assertContains($accountId, [1, 2]); + }); + + $this->listener->handle($event); + } + + public function testHandleEnableSchedulesJobs(): void { + $user = $this->createUserMock('test-user'); + $event = new UserChangedEvent($user, 'enabled', true, false); + + $this->accountService->expects($this->once()) + ->method('findByUserId') + ->with('test-user') + ->willReturn([$this->createAccountMock(42)]); + + $this->accountService->expects($this->once()) + ->method('scheduleBackgroundJobs') + ->with(42); + $this->accountService->expects($this->never()) + ->method('removeBackgroundJobs'); + + $this->listener->handle($event); + } + + public function testHandleSwallowsExceptions(): void { + $user = $this->createUserMock('test-user'); + $event = new UserChangedEvent($user, 'enabled', false, true); + $exception = new \RuntimeException('boom'); + + $this->accountService->expects($this->once()) + ->method('findByUserId') + ->with('test-user') + ->willReturn([$this->createAccountMock(1)]); + $this->accountService->method('removeBackgroundJobs') + ->willThrowException($exception); + + $this->logger->expects($this->once()) + ->method('error') + ->with($this->anything(), ['uid' => 'test-user', 'exception' => $exception]); + + $this->listener->handle($event); + } +} diff --git a/tests/Unit/Migration/FixBackgroundJobsTest.php b/tests/Unit/Migration/FixBackgroundJobsTest.php new file mode 100644 index 0000000000..9914b315d5 --- /dev/null +++ b/tests/Unit/Migration/FixBackgroundJobsTest.php @@ -0,0 +1,85 @@ +mapper = $this->createMock(MailAccountMapper::class); + $this->accountService = $this->createMock(AccountService::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->output = $this->createMock(IOutput::class); + + $this->step = new FixBackgroundJobs( + $this->mapper, + $this->accountService, + $this->userManager, + ); + } + + private function createAccount(int $id, string $uid): MailAccount { + $account = new MailAccount(); + $account->setId($id); + $account->setUserId($uid); + return $account; + } + + public function testReconcilesEnabledAndDisabledUsers(): void { + $enabledAccount = $this->createAccount(1, 'enabled-user'); + $disabledAccount = $this->createAccount(2, 'disabled-user'); + $orphanAccount = $this->createAccount(3, 'missing-user'); + + $this->mapper->method('getAllAccounts') + ->willReturn([$enabledAccount, $disabledAccount, $orphanAccount]); + + $enabledUser = $this->createMock(IUser::class); + $enabledUser->method('isEnabled')->willReturn(true); + $disabledUser = $this->createMock(IUser::class); + $disabledUser->method('isEnabled')->willReturn(false); + + $this->userManager->method('get') + ->willReturnMap([ + ['enabled-user', $enabledUser], + ['disabled-user', $disabledUser], + ['missing-user', null], + ]); + + $this->accountService->expects($this->once()) + ->method('scheduleBackgroundJobs') + ->with(1); + $removed = []; + $this->accountService->expects($this->exactly(2)) + ->method('removeBackgroundJobs') + ->willReturnCallback(function (int $id) use (&$removed): void { + $removed[] = $id; + }); + + $this->step->run($this->output); + + $this->assertEqualsCanonicalizing([2, 3], $removed); + } +} diff --git a/tests/Unit/Service/AccountServiceTest.php b/tests/Unit/Service/AccountServiceTest.php index 1461b14ac8..43307157ec 100644 --- a/tests/Unit/Service/AccountServiceTest.php +++ b/tests/Unit/Service/AccountServiceTest.php @@ -11,8 +11,12 @@ use ChristophWurst\Nextcloud\Testing\TestCase; use Horde_Imap_Client_Socket; use OCA\Mail\Account; +use OCA\Mail\BackgroundJob\ContextChat\ScheduleJob; +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\DelegationMapper; use OCA\Mail\Db\MailAccount; use OCA\Mail\Db\MailAccountMapper; @@ -343,4 +347,26 @@ public function testScheduleBackgroundJobs(): void { $this->accountService->scheduleBackgroundJobs($mailAccountId); } + + public function testRemoveBackgroundJobs(): void { + $mailAccountId = 1000; + $removed = []; + $this->jobList->expects($this->exactly(6)) + ->method('remove') + ->willReturnCallback(function (string $job, $argument) use (&$removed, $mailAccountId): void { + $this->assertSame(['accountId' => $mailAccountId], $argument); + $removed[] = $job; + }); + + $this->accountService->removeBackgroundJobs($mailAccountId); + + $this->assertEqualsCanonicalizing([ + SyncJob::class, + TrainImportanceClassifierJob::class, + PreviewEnhancementProcessingJob::class, + QuotaJob::class, + ScheduleJob::class, + RepairSyncJob::class, + ], $removed); + } }