From a1dc6159589e371fc96b28b2edaa8e5bb4b36507 Mon Sep 17 00:00:00 2001 From: Eric Peters Date: Fri, 24 Apr 2026 18:13:50 +0200 Subject: [PATCH] Implement generic webhook delivery --- .../DeliverNotificationAction.php | 263 ++++++++++++++- ...ketStatusChangedNotificationDeliveries.php | 26 +- .../DeliverNotificationActionTest.php | 300 ++++++++++++++++++ ...tStatusChangedNotificationDispatchTest.php | 82 +++++ 4 files changed, 667 insertions(+), 4 deletions(-) create mode 100644 tests/Feature/Notifications/DeliverNotificationActionTest.php diff --git a/app/Actions/Notifications/DeliverNotificationAction.php b/app/Actions/Notifications/DeliverNotificationAction.php index 55606a4..47ebf90 100644 --- a/app/Actions/Notifications/DeliverNotificationAction.php +++ b/app/Actions/Notifications/DeliverNotificationAction.php @@ -2,17 +2,278 @@ namespace App\Actions\Notifications; +use App\Enums\NotificationChannel; +use App\Enums\NotificationDeliveryStatus; +use App\Models\NotificationAction; +use App\Models\NotificationDeliveryLog; +use Carbon\CarbonImmutable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Queue\Queueable; +use Illuminate\Http\Client\ConnectionException; +use Illuminate\Http\Client\Response; +use Illuminate\Support\Facades\Http; +use Illuminate\Support\Str; +use Throwable; class DeliverNotificationAction implements ShouldQueue { use Queueable; + public int $tries = 3; + + public int $timeout = 15; + public function __construct( public int $notificationActionId, public TicketStatusChangedPayload $payload, + public ?int $notificationDeliveryLogId, ) {} - public function handle(): void {} + /** + * @return list + */ + public function backoff(): array + { + return [15, 60, 300]; + } + + public function handle(): void + { + $notificationAction = NotificationAction::query()->find($this->notificationActionId); + + if ($notificationAction === null) { + return; + } + + if ($notificationAction->channel !== NotificationChannel::Webhook) { + return; + } + + $deliveryLog = $this->resolveDeliveryLog($notificationAction); + $attemptCount = $this->attempts(); + $attemptedAt = CarbonImmutable::now(); + + try { + $response = Http::acceptJson() + ->asJson() + ->connectTimeout(5) + ->timeout(10) + ->post($notificationAction->delivery_url, $this->payload->toArray()); + } catch (ConnectionException $exception) { + $this->handleConnectionFailure($deliveryLog, $attemptCount, $attemptedAt, $exception); + + return; + } + + if ($response->successful()) { + $this->markSucceeded($deliveryLog, $response, $attemptCount, $attemptedAt); + + return; + } + + if ($this->shouldRetry($response) && $this->hasAttemptsRemaining($attemptCount)) { + $this->markRetryable($deliveryLog, $response, $attemptCount, $attemptedAt); + $this->release($this->backoffDelay($attemptCount)); + + return; + } + + $this->markFailed($deliveryLog, $response, $attemptCount, $attemptedAt); + $this->fail(new \RuntimeException($this->responseFailureDescription($response))); + } + + public function failed(?Throwable $exception): void + { + $deliveryLog = $this->loadDeliveryLog(); + + if ($deliveryLog === null || $deliveryLog->delivery_status === NotificationDeliveryStatus::Succeeded) { + return; + } + + $deliveryLog->update([ + 'attempt_count' => max($deliveryLog->attempt_count, $this->attempts()), + 'delivery_status' => NotificationDeliveryStatus::Failed, + 'delivered_at' => null, + 'error_excerpt' => $deliveryLog->error_excerpt ?? $this->excerpt($exception?->getMessage()), + 'last_attempted_at' => $deliveryLog->last_attempted_at ?? CarbonImmutable::now(), + ]); + } + + private function handleConnectionFailure( + NotificationDeliveryLog $deliveryLog, + int $attemptCount, + CarbonImmutable $attemptedAt, + ConnectionException $exception, + ): void { + $errorExcerpt = $this->excerpt($exception->getMessage()) ?? 'Webhook request failed.'; + + if ($this->hasAttemptsRemaining($attemptCount)) { + $deliveryLog->update([ + 'attempt_count' => $attemptCount, + 'delivery_status' => NotificationDeliveryStatus::Retryable, + 'delivered_at' => null, + 'error_excerpt' => $errorExcerpt, + 'http_status' => null, + 'last_attempted_at' => $attemptedAt, + 'response_excerpt' => null, + ]); + + $this->release($this->backoffDelay($attemptCount)); + + return; + } + + $deliveryLog->update([ + 'attempt_count' => $attemptCount, + 'delivery_status' => NotificationDeliveryStatus::Failed, + 'delivered_at' => null, + 'error_excerpt' => $errorExcerpt, + 'http_status' => null, + 'last_attempted_at' => $attemptedAt, + 'response_excerpt' => null, + ]); + + $this->fail($exception); + } + + private function markSucceeded( + NotificationDeliveryLog $deliveryLog, + Response $response, + int $attemptCount, + CarbonImmutable $attemptedAt, + ): void { + $deliveryLog->update([ + 'attempt_count' => $attemptCount, + 'delivery_status' => NotificationDeliveryStatus::Succeeded, + 'delivered_at' => $attemptedAt, + 'error_excerpt' => null, + 'http_status' => $response->status(), + 'last_attempted_at' => $attemptedAt, + 'response_excerpt' => $this->responseExcerpt($response), + ]); + } + + private function markRetryable( + NotificationDeliveryLog $deliveryLog, + Response $response, + int $attemptCount, + CarbonImmutable $attemptedAt, + ): void { + $deliveryLog->update([ + 'attempt_count' => $attemptCount, + 'delivery_status' => NotificationDeliveryStatus::Retryable, + 'delivered_at' => null, + 'error_excerpt' => $this->responseFailureDescription($response), + 'http_status' => $response->status(), + 'last_attempted_at' => $attemptedAt, + 'response_excerpt' => $this->responseExcerpt($response), + ]); + } + + private function markFailed( + NotificationDeliveryLog $deliveryLog, + Response $response, + int $attemptCount, + CarbonImmutable $attemptedAt, + ): void { + $deliveryLog->update([ + 'attempt_count' => $attemptCount, + 'delivery_status' => NotificationDeliveryStatus::Failed, + 'delivered_at' => null, + 'error_excerpt' => $this->responseFailureDescription($response), + 'http_status' => $response->status(), + 'last_attempted_at' => $attemptedAt, + 'response_excerpt' => $this->responseExcerpt($response), + ]); + } + + private function resolveDeliveryLog(NotificationAction $notificationAction): NotificationDeliveryLog + { + $deliveryLog = $this->loadDeliveryLog(); + + if ($deliveryLog !== null) { + return $deliveryLog; + } + + $deliveryLog = NotificationDeliveryLog::query()->create([ + 'attempt_count' => 0, + 'channel' => $notificationAction->channel, + 'delivery_status' => NotificationDeliveryStatus::Pending, + 'event_type' => $this->payload->eventType, + 'notification_action_id' => $notificationAction->id, + 'payload_metadata' => $this->payloadMetadata(), + ]); + + $this->notificationDeliveryLogId = $deliveryLog->id; + + return $deliveryLog; + } + + private function loadDeliveryLog(): ?NotificationDeliveryLog + { + if ($this->notificationDeliveryLogId === null) { + return null; + } + + return NotificationDeliveryLog::query() + ->whereKey($this->notificationDeliveryLogId) + ->where('notification_action_id', $this->notificationActionId) + ->first(); + } + + private function shouldRetry(Response $response): bool + { + return $response->serverError() || in_array($response->status(), [408, 429], true); + } + + private function hasAttemptsRemaining(int $attemptCount): bool + { + return $attemptCount < $this->tries; + } + + private function backoffDelay(int $attemptCount): int + { + $backoff = $this->backoff(); + $index = min($attemptCount - 1, count($backoff) - 1); + + return $backoff[$index]; + } + + private function responseExcerpt(Response $response): ?string + { + return $this->excerpt($response->body()) ?? $this->excerpt($response->reason()); + } + + private function responseFailureDescription(Response $response): string + { + $reason = $this->excerpt($response->reason()); + + if ($reason === null) { + return "Webhook returned HTTP {$response->status()}."; + } + + return "Webhook returned HTTP {$response->status()} {$reason}."; + } + + /** + * @return array{project_key: string, ticket_key: string} + */ + private function payloadMetadata(): array + { + return [ + 'project_key' => $this->payload->projectKey, + 'ticket_key' => $this->payload->ticketKey, + ]; + } + + private function excerpt(?string $value): ?string + { + if ($value === null) { + return null; + } + + $excerpt = (string) Str::of($value)->trim()->squish()->limit(500); + + return $excerpt === '' ? null : $excerpt; + } } diff --git a/app/Actions/Notifications/QueueTicketStatusChangedNotificationDeliveries.php b/app/Actions/Notifications/QueueTicketStatusChangedNotificationDeliveries.php index 516d6fe..98a6d35 100644 --- a/app/Actions/Notifications/QueueTicketStatusChangedNotificationDeliveries.php +++ b/app/Actions/Notifications/QueueTicketStatusChangedNotificationDeliveries.php @@ -2,7 +2,10 @@ namespace App\Actions\Notifications; +use App\Enums\NotificationChannel; +use App\Enums\NotificationDeliveryStatus; use App\Models\NotificationAction; +use App\Models\NotificationDeliveryLog; class QueueTicketStatusChangedNotificationDeliveries { @@ -12,11 +15,28 @@ class QueueTicketStatusChangedNotificationDeliveries ->enabled() ->where('project_id', $event->payload->projectId) ->where('event_type', $event->payload->eventType->value) - ->get(['id']) + ->get(['channel', 'id']) ->each(function (NotificationAction $notificationAction) use ($event): void { + $notificationDeliveryLogId = null; + + if ($notificationAction->channel === NotificationChannel::Webhook) { + $notificationDeliveryLogId = NotificationDeliveryLog::query()->create([ + 'attempt_count' => 0, + 'channel' => $notificationAction->channel, + 'delivery_status' => NotificationDeliveryStatus::Pending, + 'event_type' => $event->payload->eventType, + 'notification_action_id' => $notificationAction->id, + 'payload_metadata' => [ + 'project_key' => $event->payload->projectKey, + 'ticket_key' => $event->payload->ticketKey, + ], + ])->id; + } + DeliverNotificationAction::dispatch( - $notificationAction->id, - $event->payload, + notificationActionId: $notificationAction->id, + payload: $event->payload, + notificationDeliveryLogId: $notificationDeliveryLogId, ); }); } diff --git a/tests/Feature/Notifications/DeliverNotificationActionTest.php b/tests/Feature/Notifications/DeliverNotificationActionTest.php new file mode 100644 index 0000000..6a6503a --- /dev/null +++ b/tests/Feature/Notifications/DeliverNotificationActionTest.php @@ -0,0 +1,300 @@ + Http::response('', 204), + ]); + + $notificationAction = NotificationAction::factory()->webhook()->create([ + 'delivery_url' => 'https://hooks.example.test/kanbangit/ticket-status', + ]); + $deliveryLog = $this->createPendingDeliveryLog($notificationAction); + + (new DeliverNotificationAction( + notificationActionId: $notificationAction->id, + payload: $this->payload(), + notificationDeliveryLogId: $deliveryLog->id, + ))->handle(); + + Http::assertSentCount(1); + Http::assertSent(function (Request $request): bool { + return $request->method() === 'POST' + && $request->url() === 'https://hooks.example.test/kanbangit/ticket-status' + && $request->hasHeader('Accept', 'application/json') + && $request->hasHeader('Content-Type', 'application/json') + && $request->data() === $this->expectedPayload(); + }); + + $deliveryLog = $deliveryLog->fresh(); + + $this->assertSame(NotificationDeliveryStatus::Succeeded, $deliveryLog->delivery_status); + $this->assertSame(1, $deliveryLog->attempt_count); + $this->assertSame(204, $deliveryLog->http_status); + $this->assertSame('No Content', $deliveryLog->response_excerpt); + $this->assertNull($deliveryLog->error_excerpt); + $this->assertNotNull($deliveryLog->last_attempted_at); + $this->assertNotNull($deliveryLog->delivered_at); + } + + /** + * Ensure retryable webhook responses release the job and update the delivery log. + */ + public function test_retryable_webhook_responses_mark_the_log_retryable_and_release_the_job(): void + { + Http::preventStrayRequests(); + Http::fake([ + 'https://hooks.example.test/kanbangit/ticket-status' => Http::response([ + 'message' => 'Temporary outage', + ], 503), + ]); + + $notificationAction = NotificationAction::factory()->webhook()->create([ + 'delivery_url' => 'https://hooks.example.test/kanbangit/ticket-status', + ]); + $deliveryLog = $this->createPendingDeliveryLog($notificationAction); + $job = (new DeliverNotificationAction( + notificationActionId: $notificationAction->id, + payload: $this->payload(), + notificationDeliveryLogId: $deliveryLog->id, + ))->withFakeQueueInteractions(); + + $job->handle(); + + $job->assertReleased($job->backoff()[0]); + $job->assertNotFailed(); + + $deliveryLog = $deliveryLog->fresh(); + + $this->assertSame(NotificationDeliveryStatus::Retryable, $deliveryLog->delivery_status); + $this->assertSame(1, $deliveryLog->attempt_count); + $this->assertSame(503, $deliveryLog->http_status); + $this->assertStringContainsString('Temporary outage', (string) $deliveryLog->response_excerpt); + $this->assertSame('Webhook returned HTTP 503 Service Unavailable.', $deliveryLog->error_excerpt); + $this->assertNull($deliveryLog->delivered_at); + } + + /** + * Ensure permanent webhook errors fail immediately without scheduling another retry. + */ + public function test_permanent_webhook_responses_mark_the_log_as_failed(): void + { + Http::preventStrayRequests(); + Http::fake([ + 'https://hooks.example.test/kanbangit/ticket-status' => Http::response([ + 'message' => 'The ticket payload was invalid.', + ], 422), + ]); + + $notificationAction = NotificationAction::factory()->webhook()->create([ + 'delivery_url' => 'https://hooks.example.test/kanbangit/ticket-status', + ]); + $deliveryLog = $this->createPendingDeliveryLog($notificationAction); + $job = (new DeliverNotificationAction( + notificationActionId: $notificationAction->id, + payload: $this->payload(), + notificationDeliveryLogId: $deliveryLog->id, + ))->withFakeQueueInteractions(); + + $job->handle(); + + $job->assertFailed(); + $job->assertNotReleased(); + + $deliveryLog = $deliveryLog->fresh(); + + $this->assertSame(NotificationDeliveryStatus::Failed, $deliveryLog->delivery_status); + $this->assertSame(1, $deliveryLog->attempt_count); + $this->assertSame(422, $deliveryLog->http_status); + $this->assertStringContainsString('invalid', (string) $deliveryLog->response_excerpt); + $this->assertStringContainsString('Webhook returned HTTP 422', (string) $deliveryLog->error_excerpt); + } + + /** + * Ensure connection failures are marked retryable while attempts remain. + */ + public function test_connection_failures_mark_the_log_retryable_and_release_the_job(): void + { + Http::preventStrayRequests(); + Http::fake([ + 'https://hooks.example.test/kanbangit/ticket-status' => Http::failedConnection( + 'Connection timed out while contacting the remote endpoint.', + ), + ]); + + $notificationAction = NotificationAction::factory()->webhook()->create([ + 'delivery_url' => 'https://hooks.example.test/kanbangit/ticket-status', + ]); + $deliveryLog = $this->createPendingDeliveryLog($notificationAction); + $job = (new DeliverNotificationAction( + notificationActionId: $notificationAction->id, + payload: $this->payload(), + notificationDeliveryLogId: $deliveryLog->id, + ))->withFakeQueueInteractions(); + + $job->handle(); + + $job->assertReleased($job->backoff()[0]); + $job->assertNotFailed(); + + $deliveryLog = $deliveryLog->fresh(); + + $this->assertSame(NotificationDeliveryStatus::Retryable, $deliveryLog->delivery_status); + $this->assertSame(1, $deliveryLog->attempt_count); + $this->assertNull($deliveryLog->http_status); + $this->assertNull($deliveryLog->response_excerpt); + $this->assertSame('Connection timed out while contacting the remote endpoint.', $deliveryLog->error_excerpt); + } + + /** + * Ensure the last retryable attempt is marked failed once the job exhausts its retries. + */ + public function test_final_retryable_failures_mark_the_log_as_failed(): void + { + Http::preventStrayRequests(); + Http::fake([ + 'https://hooks.example.test/kanbangit/ticket-status' => Http::response([ + 'message' => 'Webhook service unavailable', + ], 503), + ]); + + $notificationAction = NotificationAction::factory()->webhook()->create([ + 'delivery_url' => 'https://hooks.example.test/kanbangit/ticket-status', + ]); + $deliveryLog = $this->createPendingDeliveryLog($notificationAction); + $job = (new DeliverNotificationAction( + notificationActionId: $notificationAction->id, + payload: $this->payload(), + notificationDeliveryLogId: $deliveryLog->id, + ))->withFakeQueueInteractions(); + + $job->job->attempts = $job->tries; + + $job->handle(); + + $job->assertFailed(); + $job->assertNotReleased(); + + $deliveryLog = $deliveryLog->fresh(); + + $this->assertSame(NotificationDeliveryStatus::Failed, $deliveryLog->delivery_status); + $this->assertSame($job->tries, $deliveryLog->attempt_count); + $this->assertSame(503, $deliveryLog->http_status); + $this->assertSame('Webhook returned HTTP 503 Service Unavailable.', $deliveryLog->error_excerpt); + } + + /** + * Ensure webhook delivery jobs expose the retry and timeout configuration required by the ticket. + */ + public function test_webhook_delivery_jobs_use_the_expected_retry_and_backoff_configuration(): void + { + $job = new DeliverNotificationAction( + notificationActionId: 1, + payload: $this->payload(), + notificationDeliveryLogId: 1, + ); + + $this->assertSame(3, $job->tries); + $this->assertSame(15, $job->timeout); + $this->assertSame([15, 60, 300], $job->backoff()); + } + + private function createPendingDeliveryLog(NotificationAction $notificationAction): NotificationDeliveryLog + { + return NotificationDeliveryLog::factory() + ->pending() + ->forNotificationAction($notificationAction) + ->create([ + 'payload_metadata' => [ + 'project_key' => 'CORE', + 'ticket_key' => 'CORE-42', + ], + ]); + } + + private function payload(): TicketStatusChangedPayload + { + return new TicketStatusChangedPayload( + eventType: NotificationEventType::TicketStatusChanged, + projectId: 10, + projectKey: 'CORE', + projectName: 'Core Platform', + ticketId: 42, + ticketKey: 'CORE-42', + ticketTitle: 'Fix webhook delivery', + ticketUrl: 'https://kanbangit.test/projects/core/tickets/42', + actorId: 7, + actorName: 'Erica Example', + fromStatusId: 1, + fromStatusLabel: 'Backlog', + toStatusId: 2, + toStatusLabel: 'Doing', + occurredAt: CarbonImmutable::parse('2026-04-24T09:15:30+00:00'), + ); + } + + /** + * @return array{ + * event_type: string, + * project: array{id: int, key: string, name: string}, + * ticket: array{id: int, key: string, title: string}, + * actor: array{id: int, name: string}, + * from: array{id: int, label: string}, + * to: array{id: int, label: string}, + * ticket_url: string, + * occurred_at: string + * } + */ + private function expectedPayload(): array + { + return [ + 'event_type' => 'ticket_status_changed', + 'project' => [ + 'id' => 10, + 'key' => 'CORE', + 'name' => 'Core Platform', + ], + 'ticket' => [ + 'id' => 42, + 'key' => 'CORE-42', + 'title' => 'Fix webhook delivery', + ], + 'actor' => [ + 'id' => 7, + 'name' => 'Erica Example', + ], + 'from' => [ + 'id' => 1, + 'label' => 'Backlog', + ], + 'to' => [ + 'id' => 2, + 'label' => 'Doing', + ], + 'ticket_url' => 'https://kanbangit.test/projects/core/tickets/42', + 'occurred_at' => '2026-04-24T09:15:30.000000Z', + ]; + } +} diff --git a/tests/Feature/Notifications/TicketStatusChangedNotificationDispatchTest.php b/tests/Feature/Notifications/TicketStatusChangedNotificationDispatchTest.php index ca73341..a3f146a 100644 --- a/tests/Feature/Notifications/TicketStatusChangedNotificationDispatchTest.php +++ b/tests/Feature/Notifications/TicketStatusChangedNotificationDispatchTest.php @@ -5,6 +5,7 @@ namespace Tests\Feature\Notifications; use App\Actions\Notifications\DeliverNotificationAction; use App\Actions\ProjectTicketMetadata\InitializeProjectTicketMetadata; use App\Enums\NotificationChannel; +use App\Enums\NotificationDeliveryStatus; use App\Enums\NotificationEventType; use App\Models\BoardColumn; use App\Models\NotificationAction; @@ -16,6 +17,7 @@ use App\Models\User; use Carbon\CarbonImmutable; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Queue; use Tests\TestCase; @@ -166,6 +168,86 @@ class TicketStatusChangedNotificationDispatchTest extends TestCase $this->assertDatabaseCount('ticket_worklog_entries', 0); } + /** + * Ensure ticket moves stay successful even when webhook delivery fails later on a queued attempt. + */ + public function test_ticket_moves_still_succeed_when_webhook_delivery_fails_later(): void + { + Queue::fake(); + Http::preventStrayRequests(); + + $user = User::factory()->create([ + 'name' => 'Erica Example', + ]); + $project = Project::factory()->create([ + 'key' => 'CORE', + ]); + + app(InitializeProjectTicketMetadata::class)->handle($project); + + $doingColumn = BoardColumn::factory()->for($project)->create([ + 'name' => 'Doing', + 'position' => 1, + ]); + $doingStatus = $this->createColumnStatus($doingColumn); + $backlogStatus = $this->backlogStatus($project); + $this->allowMove($backlogStatus, $doingStatus); + + $ticket = $this->createTicket($project, $backlogStatus, 'Retry failing webhook'); + $webhookAction = NotificationAction::factory() + ->webhook() + ->for($project) + ->create([ + 'delivery_url' => 'https://hooks.example.test/kanbangit/retry-failing-webhook', + ]); + + $this->syncProjectPermissions($project, $user, ['edit', 'view']); + + $this->actingAs($user) + ->patch(route('projects.tickets.status.update', [$project, $ticket]), [ + 'redirect_to' => 'backlog', + 'ticket_status_id' => $doingStatus->id, + ]) + ->assertRedirect(route('projects.backlog.index', $project)) + ->assertSessionHas('status', 'CORE-1 moved to Doing.'); + + $queuedJob = null; + + Queue::assertPushed(DeliverNotificationAction::class, function (DeliverNotificationAction $job) use ($webhookAction, &$queuedJob): bool { + if ($job->notificationActionId !== $webhookAction->id) { + return false; + } + + $queuedJob = $job; + + return true; + }); + + $this->assertInstanceOf(DeliverNotificationAction::class, $queuedJob); + + Http::fake([ + 'https://hooks.example.test/kanbangit/retry-failing-webhook' => Http::failedConnection( + 'Connection timed out while contacting the remote endpoint.', + ), + ]); + + $queuedJob->withFakeQueueInteractions()->handle(); + + $queuedJob->assertReleased($queuedJob->backoff()[0]); + + $this->assertDatabaseHas('tickets', [ + 'id' => $ticket->id, + 'ticket_status_id' => $doingStatus->id, + ]); + + $this->assertDatabaseHas('notification_delivery_logs', [ + 'notification_action_id' => $webhookAction->id, + 'delivery_status' => NotificationDeliveryStatus::Retryable->value, + 'attempt_count' => 1, + 'http_status' => null, + ]); + } + private function backlogStatus(Project $project): TicketStatus { return $project->ticketStatuses()