Harden notification release regressions #62
@@ -78,26 +78,26 @@ class DeliverNotificationAction implements ShouldQueue
|
||||
NotificationChannel::Webhook => $this->sendWebhookRequest($notificationAction),
|
||||
};
|
||||
} catch (ConnectionException $exception) {
|
||||
$this->handleConnectionFailure($deliveryLog, $notificationAction->channel, $attemptCount, $attemptedAt, $exception);
|
||||
$this->handleConnectionFailure($deliveryLog, $notificationAction, $attemptCount, $attemptedAt, $exception);
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
if ($response->successful()) {
|
||||
$this->markSucceeded($deliveryLog, $response, $attemptCount, $attemptedAt);
|
||||
$this->markSucceeded($deliveryLog, $notificationAction, $response, $attemptCount, $attemptedAt);
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
if ($this->shouldRetry($response) && $this->hasAttemptsRemaining($attemptCount)) {
|
||||
$this->markRetryable($deliveryLog, $notificationAction->channel, $response, $attemptCount, $attemptedAt);
|
||||
$this->markRetryable($deliveryLog, $notificationAction, $response, $attemptCount, $attemptedAt);
|
||||
$this->release($this->backoffDelay($attemptCount));
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
$this->markFailed($deliveryLog, $notificationAction->channel, $response, $attemptCount, $attemptedAt);
|
||||
$this->fail(new \RuntimeException($this->responseFailureDescription($notificationAction->channel, $response)));
|
||||
$this->markFailed($deliveryLog, $notificationAction, $response, $attemptCount, $attemptedAt);
|
||||
$this->fail(new \RuntimeException($this->responseFailureDescription($notificationAction, $response)));
|
||||
}
|
||||
|
||||
public function failed(?Throwable $exception): void
|
||||
@@ -108,23 +108,29 @@ class DeliverNotificationAction implements ShouldQueue
|
||||
return;
|
||||
}
|
||||
|
||||
$notificationAction = NotificationAction::query()->find($this->notificationActionId);
|
||||
|
||||
$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()),
|
||||
'error_excerpt' => $deliveryLog->error_excerpt ?? $this->sanitizedExcerpt(
|
||||
$exception?->getMessage(),
|
||||
$notificationAction,
|
||||
),
|
||||
'last_attempted_at' => $deliveryLog->last_attempted_at ?? CarbonImmutable::now(),
|
||||
]);
|
||||
}
|
||||
|
||||
private function handleConnectionFailure(
|
||||
NotificationDeliveryLog $deliveryLog,
|
||||
NotificationChannel $channel,
|
||||
NotificationAction $notificationAction,
|
||||
int $attemptCount,
|
||||
CarbonImmutable $attemptedAt,
|
||||
ConnectionException $exception,
|
||||
): void {
|
||||
$errorExcerpt = $this->excerpt($exception->getMessage()) ?? $this->requestFailureDescription($channel);
|
||||
$errorExcerpt = $this->sanitizedExcerpt($exception->getMessage(), $notificationAction)
|
||||
?? $this->requestFailureDescription($notificationAction->channel);
|
||||
|
||||
if ($this->hasAttemptsRemaining($attemptCount)) {
|
||||
$deliveryLog->update([
|
||||
@@ -157,6 +163,7 @@ class DeliverNotificationAction implements ShouldQueue
|
||||
|
||||
private function markSucceeded(
|
||||
NotificationDeliveryLog $deliveryLog,
|
||||
NotificationAction $notificationAction,
|
||||
Response $response,
|
||||
int $attemptCount,
|
||||
CarbonImmutable $attemptedAt,
|
||||
@@ -168,13 +175,13 @@ class DeliverNotificationAction implements ShouldQueue
|
||||
'error_excerpt' => null,
|
||||
'http_status' => $response->status(),
|
||||
'last_attempted_at' => $attemptedAt,
|
||||
'response_excerpt' => $this->responseExcerpt($response),
|
||||
'response_excerpt' => $this->responseExcerpt($response, $notificationAction),
|
||||
]);
|
||||
}
|
||||
|
||||
private function markRetryable(
|
||||
NotificationDeliveryLog $deliveryLog,
|
||||
NotificationChannel $channel,
|
||||
NotificationAction $notificationAction,
|
||||
Response $response,
|
||||
int $attemptCount,
|
||||
CarbonImmutable $attemptedAt,
|
||||
@@ -183,16 +190,16 @@ class DeliverNotificationAction implements ShouldQueue
|
||||
'attempt_count' => $attemptCount,
|
||||
'delivery_status' => NotificationDeliveryStatus::Retryable,
|
||||
'delivered_at' => null,
|
||||
'error_excerpt' => $this->responseFailureDescription($channel, $response),
|
||||
'error_excerpt' => $this->responseFailureDescription($notificationAction, $response),
|
||||
'http_status' => $response->status(),
|
||||
'last_attempted_at' => $attemptedAt,
|
||||
'response_excerpt' => $this->responseExcerpt($response),
|
||||
'response_excerpt' => $this->responseExcerpt($response, $notificationAction),
|
||||
]);
|
||||
}
|
||||
|
||||
private function markFailed(
|
||||
NotificationDeliveryLog $deliveryLog,
|
||||
NotificationChannel $channel,
|
||||
NotificationAction $notificationAction,
|
||||
Response $response,
|
||||
int $attemptCount,
|
||||
CarbonImmutable $attemptedAt,
|
||||
@@ -201,10 +208,10 @@ class DeliverNotificationAction implements ShouldQueue
|
||||
'attempt_count' => $attemptCount,
|
||||
'delivery_status' => NotificationDeliveryStatus::Failed,
|
||||
'delivered_at' => null,
|
||||
'error_excerpt' => $this->responseFailureDescription($channel, $response),
|
||||
'error_excerpt' => $this->responseFailureDescription($notificationAction, $response),
|
||||
'http_status' => $response->status(),
|
||||
'last_attempted_at' => $attemptedAt,
|
||||
'response_excerpt' => $this->responseExcerpt($response),
|
||||
'response_excerpt' => $this->responseExcerpt($response, $notificationAction),
|
||||
]);
|
||||
}
|
||||
|
||||
@@ -300,15 +307,16 @@ class DeliverNotificationAction implements ShouldQueue
|
||||
return $backoff[$index];
|
||||
}
|
||||
|
||||
private function responseExcerpt(Response $response): ?string
|
||||
private function responseExcerpt(Response $response, NotificationAction $notificationAction): ?string
|
||||
{
|
||||
return $this->excerpt($response->body()) ?? $this->excerpt($response->reason());
|
||||
return $this->sanitizedExcerpt($response->body(), $notificationAction)
|
||||
?? $this->sanitizedExcerpt($response->reason(), $notificationAction);
|
||||
}
|
||||
|
||||
private function responseFailureDescription(NotificationChannel $channel, Response $response): string
|
||||
private function responseFailureDescription(NotificationAction $notificationAction, Response $response): string
|
||||
{
|
||||
$reason = $this->excerpt($response->reason());
|
||||
$subject = $this->deliverySubject($channel);
|
||||
$reason = $this->sanitizedExcerpt($response->reason(), $notificationAction);
|
||||
$subject = $this->deliverySubject($notificationAction->channel);
|
||||
|
||||
if ($reason === null) {
|
||||
return "{$subject} returned HTTP {$response->status()}.";
|
||||
@@ -367,4 +375,24 @@ class DeliverNotificationAction implements ShouldQueue
|
||||
|
||||
return $excerpt === '' ? null : $excerpt;
|
||||
}
|
||||
|
||||
private function sanitizedExcerpt(?string $value, ?NotificationAction $notificationAction): ?string
|
||||
{
|
||||
if (
|
||||
$value !== null
|
||||
&& $notificationAction !== null
|
||||
&& filled($notificationAction->delivery_url)
|
||||
) {
|
||||
$value = str_replace(
|
||||
[
|
||||
$notificationAction->delivery_url,
|
||||
str_replace('/', '\/', $notificationAction->delivery_url),
|
||||
],
|
||||
'[redacted]',
|
||||
$value,
|
||||
);
|
||||
}
|
||||
|
||||
return $this->excerpt($value);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3,8 +3,11 @@
|
||||
namespace Tests\Feature\Api;
|
||||
|
||||
use App\Actions\Git\GitProviderKey;
|
||||
use App\Actions\Notifications\DeliverNotificationAction;
|
||||
use App\Actions\ProjectTicketMetadata\InitializeProjectTicketMetadata;
|
||||
use App\Enums\NotificationDeliveryStatus;
|
||||
use App\Models\BoardColumn;
|
||||
use App\Models\NotificationAction;
|
||||
use App\Models\Project;
|
||||
use App\Models\RepositoryConnection;
|
||||
use App\Models\Ticket;
|
||||
@@ -14,6 +17,7 @@ use App\Models\User;
|
||||
use App\Models\UserGitConnection;
|
||||
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||
use Illuminate\Support\Facades\Http;
|
||||
use Illuminate\Support\Facades\Queue;
|
||||
use Tests\TestCase;
|
||||
|
||||
class TicketApiTest extends TestCase
|
||||
@@ -193,6 +197,80 @@ class TicketApiTest extends TestCase
|
||||
]);
|
||||
}
|
||||
|
||||
public function test_status_moves_still_queue_notification_deliveries_over_the_api(): void
|
||||
{
|
||||
Queue::fake();
|
||||
|
||||
[$user, $project] = $this->projectContext();
|
||||
|
||||
$backlogStatus = $project->ticketStatuses()->firstOrFail();
|
||||
$boardColumn = BoardColumn::factory()->for($project)->create([
|
||||
'name' => 'Doing',
|
||||
'position' => 1,
|
||||
]);
|
||||
$doingStatus = TicketStatus::factory()
|
||||
->for($project)
|
||||
->for($boardColumn)
|
||||
->create([
|
||||
'name' => 'Doing',
|
||||
'position' => 1,
|
||||
]);
|
||||
$backlogStatus->allowedTargetStatuses()->sync([$doingStatus->id]);
|
||||
|
||||
$ticket = $this->ticket($project, $backlogStatus);
|
||||
$notificationAction = NotificationAction::factory()
|
||||
->webhook()
|
||||
->for($project)
|
||||
->create();
|
||||
|
||||
$this->withApplicationKey($user)
|
||||
->patchJson(route('api.projects.tickets.status.update', [$project, $ticket]), [
|
||||
'ticket_status_id' => $doingStatus->id,
|
||||
])
|
||||
->assertOk()
|
||||
->assertJsonPath('data.ticket_status_id', $doingStatus->id)
|
||||
->assertJsonPath('meta.message', 'CORE-1 moved to Doing.');
|
||||
|
||||
Queue::assertPushed(DeliverNotificationAction::class, function (DeliverNotificationAction $job) use ($notificationAction): bool {
|
||||
return $job->notificationActionId === $notificationAction->id
|
||||
&& $job->payload->ticketKey === 'CORE-1'
|
||||
&& $job->payload->toStatusLabel === 'Doing'
|
||||
&& $job->notificationDeliveryLogId !== null;
|
||||
});
|
||||
|
||||
$this->assertDatabaseHas('notification_delivery_logs', [
|
||||
'notification_action_id' => $notificationAction->id,
|
||||
'delivery_status' => NotificationDeliveryStatus::Pending->value,
|
||||
'attempt_count' => 0,
|
||||
]);
|
||||
}
|
||||
|
||||
public function test_no_op_status_moves_keep_the_existing_api_response_and_do_not_queue_notifications(): void
|
||||
{
|
||||
Queue::fake();
|
||||
|
||||
[$user, $project] = $this->projectContext();
|
||||
|
||||
$backlogStatus = $project->ticketStatuses()->firstOrFail();
|
||||
$ticket = $this->ticket($project, $backlogStatus);
|
||||
|
||||
NotificationAction::factory()
|
||||
->webhook()
|
||||
->for($project)
|
||||
->create();
|
||||
|
||||
$this->withApplicationKey($user)
|
||||
->patchJson(route('api.projects.tickets.status.update', [$project, $ticket]), [
|
||||
'ticket_status_id' => $backlogStatus->id,
|
||||
])
|
||||
->assertOk()
|
||||
->assertJsonPath('data.ticket_status_id', $backlogStatus->id)
|
||||
->assertJsonPath('meta.message', 'CORE-1 already belongs to Backlog.');
|
||||
|
||||
Queue::assertNothingPushed();
|
||||
$this->assertDatabaseCount('ticket_worklog_entries', 0);
|
||||
}
|
||||
|
||||
public function test_ticket_links_and_branch_endpoints_are_available(): void
|
||||
{
|
||||
Http::preventStrayRequests();
|
||||
|
||||
@@ -2,7 +2,9 @@
|
||||
|
||||
namespace Tests\Feature\Mcp;
|
||||
|
||||
use App\Actions\Notifications\DeliverNotificationAction;
|
||||
use App\Actions\ProjectTicketMetadata\InitializeProjectTicketMetadata;
|
||||
use App\Enums\NotificationDeliveryStatus;
|
||||
use App\Mcp\Servers\KanbanGitServer;
|
||||
use App\Mcp\Tools\GetTicket;
|
||||
use App\Mcp\Tools\ListMyAssignedTickets;
|
||||
@@ -11,11 +13,13 @@ use App\Mcp\Tools\MoveTicketStatus;
|
||||
use App\Mcp\Tools\UpdateTicketAssignee;
|
||||
use App\Mcp\Tools\UpdateTicketDescription;
|
||||
use App\Models\BoardColumn;
|
||||
use App\Models\NotificationAction;
|
||||
use App\Models\Project;
|
||||
use App\Models\Ticket;
|
||||
use App\Models\TicketStatus;
|
||||
use App\Models\User;
|
||||
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||
use Illuminate\Support\Facades\Queue;
|
||||
use Illuminate\Testing\Fluent\AssertableJson;
|
||||
use Tests\TestCase;
|
||||
|
||||
@@ -319,6 +323,90 @@ class KanbanGitMcpTest extends TestCase
|
||||
->etc());
|
||||
}
|
||||
|
||||
public function test_mcp_status_moves_still_queue_notification_deliveries_when_actions_are_enabled(): void
|
||||
{
|
||||
Queue::fake();
|
||||
|
||||
$user = User::factory()->create();
|
||||
$project = $this->project('CORE', 'Core project');
|
||||
$backlogStatus = $project->ticketStatuses()->firstOrFail();
|
||||
$doingStatus = $this->ticketStatus($project, 'Doing');
|
||||
$ticket = $this->ticket($project, $backlogStatus, [
|
||||
'ticket_key' => 'CORE-1',
|
||||
'title' => 'Move ticket',
|
||||
]);
|
||||
|
||||
$backlogStatus->allowedTargetStatuses()->sync([$doingStatus->id]);
|
||||
$this->syncProjectPermissions($project, $user, ['write']);
|
||||
|
||||
$notificationAction = NotificationAction::factory()
|
||||
->webhook()
|
||||
->for($project)
|
||||
->create();
|
||||
|
||||
KanbanGitServer::actingAs($user)
|
||||
->tool(MoveTicketStatus::class, [
|
||||
'ticket_id' => $ticket->id,
|
||||
'ticket_status_id' => $doingStatus->id,
|
||||
])
|
||||
->assertOk()
|
||||
->assertStructuredContent(fn (AssertableJson $json) => $json
|
||||
->where('data.ticket.ticket_status_id', $doingStatus->id)
|
||||
->where('meta.destination_label', 'Doing')
|
||||
->where('meta.message', 'CORE-1 moved to Doing.')
|
||||
->where('meta.moved', true)
|
||||
->etc());
|
||||
|
||||
Queue::assertPushed(DeliverNotificationAction::class, function (DeliverNotificationAction $job) use ($notificationAction): bool {
|
||||
return $job->notificationActionId === $notificationAction->id
|
||||
&& $job->payload->ticketKey === 'CORE-1'
|
||||
&& $job->payload->toStatusLabel === 'Doing'
|
||||
&& $job->notificationDeliveryLogId !== null;
|
||||
});
|
||||
|
||||
$this->assertDatabaseHas('notification_delivery_logs', [
|
||||
'notification_action_id' => $notificationAction->id,
|
||||
'delivery_status' => NotificationDeliveryStatus::Pending->value,
|
||||
'attempt_count' => 0,
|
||||
]);
|
||||
}
|
||||
|
||||
public function test_mcp_no_op_status_moves_do_not_queue_notification_deliveries(): void
|
||||
{
|
||||
Queue::fake();
|
||||
|
||||
$user = User::factory()->create();
|
||||
$project = $this->project('CORE', 'Core project');
|
||||
$backlogStatus = $project->ticketStatuses()->firstOrFail();
|
||||
$ticket = $this->ticket($project, $backlogStatus, [
|
||||
'ticket_key' => 'CORE-1',
|
||||
'title' => 'Move ticket',
|
||||
]);
|
||||
|
||||
$this->syncProjectPermissions($project, $user, ['write']);
|
||||
|
||||
NotificationAction::factory()
|
||||
->webhook()
|
||||
->for($project)
|
||||
->create();
|
||||
|
||||
KanbanGitServer::actingAs($user)
|
||||
->tool(MoveTicketStatus::class, [
|
||||
'ticket_id' => $ticket->id,
|
||||
'ticket_status_id' => $backlogStatus->id,
|
||||
])
|
||||
->assertOk()
|
||||
->assertStructuredContent(fn (AssertableJson $json) => $json
|
||||
->where('data.ticket.ticket_status_id', $backlogStatus->id)
|
||||
->where('meta.destination_label', 'Backlog')
|
||||
->where('meta.message', 'CORE-1 already belongs to Backlog.')
|
||||
->where('meta.moved', false)
|
||||
->etc());
|
||||
|
||||
Queue::assertNothingPushed();
|
||||
$this->assertDatabaseCount('ticket_worklog_entries', 0);
|
||||
}
|
||||
|
||||
public function test_list_tickets_by_status_requires_a_status_selector(): void
|
||||
{
|
||||
$user = User::factory()->create();
|
||||
|
||||
@@ -290,6 +290,78 @@ class DeliverNotificationActionTest extends TestCase
|
||||
$this->assertSame('Connection timed out while contacting the remote endpoint.', $deliveryLog->error_excerpt);
|
||||
}
|
||||
|
||||
/**
|
||||
* Ensure retryable delivery logs redact stored secret URLs from response excerpts.
|
||||
*/
|
||||
public function test_retryable_response_excerpts_redact_delivery_urls(): void
|
||||
{
|
||||
Http::preventStrayRequests();
|
||||
|
||||
$deliveryUrl = 'https://hooks.example.test/kanbangit/secret-token';
|
||||
|
||||
Http::fake([
|
||||
$deliveryUrl => Http::response([
|
||||
'message' => "Remote service could not deliver to {$deliveryUrl}.",
|
||||
], 503),
|
||||
]);
|
||||
|
||||
$notificationAction = NotificationAction::factory()->webhook()->create([
|
||||
'delivery_url' => $deliveryUrl,
|
||||
]);
|
||||
$deliveryLog = $this->createPendingDeliveryLog($notificationAction);
|
||||
$job = (new DeliverNotificationAction(
|
||||
notificationActionId: $notificationAction->id,
|
||||
payload: $this->payload(),
|
||||
notificationDeliveryLogId: $deliveryLog->id,
|
||||
))->withFakeQueueInteractions();
|
||||
|
||||
$job->handle();
|
||||
|
||||
$job->assertReleased($job->backoff()[0]);
|
||||
|
||||
$deliveryLog = $deliveryLog->fresh();
|
||||
|
||||
$this->assertSame(NotificationDeliveryStatus::Retryable, $deliveryLog->delivery_status);
|
||||
$this->assertStringContainsString('[redacted]', (string) $deliveryLog->response_excerpt);
|
||||
$this->assertStringNotContainsString($deliveryUrl, (string) $deliveryLog->response_excerpt);
|
||||
}
|
||||
|
||||
/**
|
||||
* Ensure connection failure logs redact stored secret URLs from error excerpts.
|
||||
*/
|
||||
public function test_connection_failure_excerpts_redact_delivery_urls(): void
|
||||
{
|
||||
Http::preventStrayRequests();
|
||||
|
||||
$deliveryUrl = 'https://hooks.example.test/kanbangit/secret-token';
|
||||
|
||||
Http::fake([
|
||||
$deliveryUrl => Http::failedConnection(
|
||||
"Connection timed out while contacting {$deliveryUrl}.",
|
||||
),
|
||||
]);
|
||||
|
||||
$notificationAction = NotificationAction::factory()->webhook()->create([
|
||||
'delivery_url' => $deliveryUrl,
|
||||
]);
|
||||
$deliveryLog = $this->createPendingDeliveryLog($notificationAction);
|
||||
$job = (new DeliverNotificationAction(
|
||||
notificationActionId: $notificationAction->id,
|
||||
payload: $this->payload(),
|
||||
notificationDeliveryLogId: $deliveryLog->id,
|
||||
))->withFakeQueueInteractions();
|
||||
|
||||
$job->handle();
|
||||
|
||||
$job->assertReleased($job->backoff()[0]);
|
||||
|
||||
$deliveryLog = $deliveryLog->fresh();
|
||||
|
||||
$this->assertSame(NotificationDeliveryStatus::Retryable, $deliveryLog->delivery_status);
|
||||
$this->assertStringContainsString('[redacted]', (string) $deliveryLog->error_excerpt);
|
||||
$this->assertStringNotContainsString($deliveryUrl, (string) $deliveryLog->error_excerpt);
|
||||
}
|
||||
|
||||
/**
|
||||
* Ensure Discord deliveries reject empty rendered messages before sending the request.
|
||||
*/
|
||||
|
||||
Reference in New Issue
Block a user