From fe88e748a31a33491fb932a628732dce78d534e4 Mon Sep 17 00:00:00 2001 From: fs Date: Mon, 6 Apr 2026 12:38:47 +0200 Subject: [PATCH] fix(notifications): use createFromMessage directly for CLI-safe scheduler notifications createForTenantAdminUsersFromMessage relies on listActiveUserIdsByTenantId which returns empty results in CLI context (scheduler). Instead, iterate admin users directly and call createFromMessage per user per tenant. Verified end-to-end: notification created, dedup blocks duplicates. Co-Authored-By: Claude Opus 4.6 (1M context) --- ...SchedulerJobFailedNotificationListener.php | 37 ++++-------- ...dulerJobFailedNotificationListenerTest.php | 60 +++++++++++++------ 2 files changed, 52 insertions(+), 45 deletions(-) diff --git a/modules/notifications/lib/Module/Notifications/Listeners/SchedulerJobFailedNotificationListener.php b/modules/notifications/lib/Module/Notifications/Listeners/SchedulerJobFailedNotificationListener.php index 1c693e5..09c15f8 100644 --- a/modules/notifications/lib/Module/Notifications/Listeners/SchedulerJobFailedNotificationListener.php +++ b/modules/notifications/lib/Module/Notifications/Listeners/SchedulerJobFailedNotificationListener.php @@ -57,33 +57,18 @@ final class SchedulerJobFailedNotificationListener implements EventListener ] ); - $tenantIds = $this->resolveAdminTenantIds($adminUserIds); - - foreach ($tenantIds as $tenantId) { - $this->notificationService->createForTenantAdminUsersFromMessage( - $tenantId, - $message, - null, - $adminSet - ); - } - } - - /** - * Collect all unique tenant IDs across admin users. - * - * @param list $userIds - * @return list - */ - private function resolveAdminTenantIds(array $userIds): array - { - $tenantIds = []; - foreach ($userIds as $userId) { - foreach ($this->userTenantRepository->listTenantIdsByUserId($userId) as $tenantId) { - $tenantIds[$tenantId] = true; + // Create notifications directly per admin user + tenant. + // This avoids listActiveUserIdsByTenantId which requires a web-context + // DB session — the scheduler runs in CLI where that lookup may fail. + foreach ($adminUserIds as $userId) { + $tenantIds = $this->userTenantRepository->listTenantIdsByUserId($userId); + if ($tenantIds === []) { + $this->notificationService->createFromMessage($userId, null, $message); + continue; + } + foreach ($tenantIds as $tenantId) { + $this->notificationService->createFromMessage($userId, $tenantId, $message); } } - - return array_keys($tenantIds); } } diff --git a/modules/notifications/tests/Module/Notifications/Listeners/SchedulerJobFailedNotificationListenerTest.php b/modules/notifications/tests/Module/Notifications/Listeners/SchedulerJobFailedNotificationListenerTest.php index 55a194d..9f25259 100644 --- a/modules/notifications/tests/Module/Notifications/Listeners/SchedulerJobFailedNotificationListenerTest.php +++ b/modules/notifications/tests/Module/Notifications/Listeners/SchedulerJobFailedNotificationListenerTest.php @@ -26,7 +26,7 @@ class SchedulerJobFailedNotificationListenerTest extends TestCase $this->listener = new SchedulerJobFailedNotificationListener($this->notifService, $this->userRepo, $this->utRepo); } - public function testHandleCreatesNotificationsForAdminsAcrossTenants(): void + public function testHandleCreatesNotificationsPerAdminPerTenant(): void { $this->userRepo->expects($this->any())->method('listPrivilegedUserIdsByPermissionKeys') ->with([PermissionService::JOBS_MANAGE]) @@ -39,31 +39,53 @@ class SchedulerJobFailedNotificationListenerTest extends TestCase [30, [2, 3]], ]); - $this->notifService->expects($this->exactly(3)) - ->method('createForTenantAdminUsersFromMessage') - ->willReturnCallback(function (int $tenantId, NotificationMessage $message, ?int $excludeUserId, array $adminSet): int { - $this->assertContains($tenantId, [1, 2, 3]); + $calls = []; + $this->notifService->expects($this->exactly(4)) + ->method('createFromMessage') + ->willReturnCallback(function (int $userId, ?int $tenantId, NotificationMessage $message) use (&$calls): int { + $calls[] = ['user' => $userId, 'tenant' => $tenantId]; $this->assertSame('scheduler.job_failed', $message->type()); - $this->assertStringContainsString('System audit purge', $message->title()); + $this->assertStringContainsString('Test Job', $message->title()); $this->assertSame('admin/scheduled-jobs', $message->link()); - $this->assertNull($excludeUserId); - $this->assertArrayHasKey(20, $adminSet); - $this->assertArrayHasKey(30, $adminSet); $this->assertSame('Scheduled job failed: %s', $message->titleKey()); $data = $message->data(); - $this->assertSame('job:system_audit_purge', $data['dedupe_target']); - $this->assertSame('job_not_registered', $data['error_code']); + $this->assertSame('job:test_fail_job', $data['dedupe_target']); return 1; }); $this->listener->handle('scheduler.job_failed', [ 'job_id' => 5, - 'job_key' => 'system_audit_purge', - 'label' => 'System audit purge', + 'job_key' => 'test_fail_job', + 'label' => 'Test Job', 'error_code' => 'job_not_registered', 'error_message' => null, 'trigger_type' => 'scheduler', ]); + + // User 20: tenant 1, tenant 2. User 30: tenant 2, tenant 3. = 4 calls total. + $this->assertCount(4, $calls); + $this->assertSame(['user' => 20, 'tenant' => 1], $calls[0]); + $this->assertSame(['user' => 20, 'tenant' => 2], $calls[1]); + $this->assertSame(['user' => 30, 'tenant' => 2], $calls[2]); + $this->assertSame(['user' => 30, 'tenant' => 3], $calls[3]); + } + + public function testHandleCreatesGlobalNotificationWhenNoTenants(): void + { + $this->userRepo->expects($this->any())->method('listPrivilegedUserIdsByPermissionKeys')->willReturn([10]); + $this->utRepo->expects($this->any())->method('listTenantIdsByUserId')->willReturn([]); + + $this->notifService->expects($this->once()) + ->method('createFromMessage') + ->with(10, null, $this->anything()) + ->willReturn(1); + + $this->listener->handle('scheduler.job_failed', [ + 'job_id' => 1, + 'job_key' => 'some_job', + 'label' => 'Some Job', + 'error_code' => 'unexpected_error', + ]); } public function testHandleUsesJobKeyAsLabelWhenLabelEmpty(): void @@ -72,8 +94,8 @@ class SchedulerJobFailedNotificationListenerTest extends TestCase $this->utRepo->expects($this->any())->method('listTenantIdsByUserId')->willReturn([1]); $this->notifService->expects($this->once()) - ->method('createForTenantAdminUsersFromMessage') - ->willReturnCallback(function (int $tenantId, NotificationMessage $message): int { + ->method('createFromMessage') + ->willReturnCallback(function (int $userId, ?int $tenantId, NotificationMessage $message): int { $this->assertStringContainsString('my_job_key', $message->title()); return 1; }); @@ -88,7 +110,7 @@ class SchedulerJobFailedNotificationListenerTest extends TestCase public function testHandleSkipsEmptyPayload(): void { - $this->notifService->expects($this->never())->method('createForTenantAdminUsersFromMessage'); + $this->notifService->expects($this->never())->method('createFromMessage'); $this->listener->handle('scheduler.job_failed', []); $this->listener->handle('scheduler.job_failed', ['job_key' => '', 'job_id' => 0]); @@ -98,7 +120,7 @@ class SchedulerJobFailedNotificationListenerTest extends TestCase public function testHandleSkipsWhenNoAdmins(): void { $this->userRepo->expects($this->any())->method('listPrivilegedUserIdsByPermissionKeys')->willReturn([]); - $this->notifService->expects($this->never())->method('createForTenantAdminUsersFromMessage'); + $this->notifService->expects($this->never())->method('createFromMessage'); $this->listener->handle('scheduler.job_failed', [ 'job_id' => 5, @@ -114,8 +136,8 @@ class SchedulerJobFailedNotificationListenerTest extends TestCase $this->utRepo->expects($this->any())->method('listTenantIdsByUserId')->willReturn([1]); $this->notifService->expects($this->once()) - ->method('createForTenantAdminUsersFromMessage') - ->willReturnCallback(function (int $tenantId, NotificationMessage $message): int { + ->method('createFromMessage') + ->willReturnCallback(function (int $userId, ?int $tenantId, NotificationMessage $message): int { $this->assertNull($message->body()); return 1; });