From dc112664df8411a398ee01100ada84db1e64ba7f Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Mon, 23 Feb 2026 17:57:29 +0100 Subject: [PATCH] fix: review pack generation UX + notifications --- app/Filament/Resources/ReviewPackResource.php | 20 ++- app/Jobs/GenerateReviewPackJob.php | 75 ++++------- .../ReviewPackStatusNotification.php | 91 ------------- app/Services/ReviewPackService.php | 10 +- app/Support/OpsUx/OperationSummaryKeys.php | 3 + .../js/tenantpilot/livewire-intercept-shim.js | 121 +++++++++++------- specs/055-ops-ux-rollout/spec.md | 2 +- tests/Feature/LivewireInterceptShimTest.php | 16 +++ .../ReviewPack/ReviewPackGenerationTest.php | 37 ++++-- .../ReviewPack/ReviewPackResourceTest.php | 42 ++++++ 10 files changed, 209 insertions(+), 208 deletions(-) delete mode 100644 app/Notifications/ReviewPackStatusNotification.php create mode 100644 tests/Feature/LivewireInterceptShimTest.php diff --git a/app/Filament/Resources/ReviewPackResource.php b/app/Filament/Resources/ReviewPackResource.php index 69b9d77..3b9ca5b 100644 --- a/app/Filament/Resources/ReviewPackResource.php +++ b/app/Filament/Resources/ReviewPackResource.php @@ -10,6 +10,7 @@ use App\Support\Auth\Capabilities; use App\Support\Badges\BadgeDomain; use App\Support\Badges\BadgeRenderer; +use App\Support\OpsUx\OperationUxPresenter; use App\Support\Rbac\UiEnforcement; use App\Support\ReviewPackStatus; use App\Support\Ui\ActionSurface\ActionSurfaceDeclaration; @@ -329,8 +330,23 @@ public static function executeGeneration(array $data): void 'include_operations' => (bool) ($data['include_operations'] ?? true), ]; - $service->generate($tenant, $user, $options); + $reviewPack = $service->generate($tenant, $user, $options); - Notification::make()->success()->title('Review pack generation started.')->send(); + if (! $reviewPack->wasRecentlyCreated) { + Notification::make() + ->success() + ->title('Review pack already available') + ->body('A matching review pack is already ready. No new run was started.') + ->actions([ + Actions\Action::make('view_pack') + ->label('View pack') + ->url(static::getUrl('view', ['record' => $reviewPack], tenant: $tenant)), + ]) + ->send(); + + return; + } + + OperationUxPresenter::queuedToast('tenant.review_pack.generate')->send(); } } diff --git a/app/Jobs/GenerateReviewPackJob.php b/app/Jobs/GenerateReviewPackJob.php index db70a08..9760c89 100644 --- a/app/Jobs/GenerateReviewPackJob.php +++ b/app/Jobs/GenerateReviewPackJob.php @@ -9,7 +9,7 @@ use App\Models\ReviewPack; use App\Models\StoredReport; use App\Models\Tenant; -use App\Notifications\ReviewPackStatusNotification; +use App\Services\OperationRunService; use App\Services\ReviewPackService; use App\Support\OperationRunOutcome; use App\Support\OperationRunStatus; @@ -30,7 +30,7 @@ public function __construct( public int $operationRunId, ) {} - public function handle(): void + public function handle(OperationRunService $operationRunService): void { $reviewPack = ReviewPack::query()->find($this->reviewPackId); $operationRun = OperationRun::query()->find($this->operationRunId); @@ -47,28 +47,25 @@ public function handle(): void $tenant = $reviewPack->tenant; if (! $tenant instanceof Tenant) { - $this->markFailed($reviewPack, $operationRun, 'tenant_not_found', 'Tenant not found'); + $this->markFailed($reviewPack, $operationRun, $operationRunService, 'tenant_not_found', 'Tenant not found'); return; } - // Mark running - $operationRun->update([ - 'status' => OperationRunStatus::Running->value, - 'started_at' => now(), - ]); + // Mark running via OperationRunService (auto-sets started_at) + $operationRunService->updateRun($operationRun, OperationRunStatus::Running->value); $reviewPack->update(['status' => ReviewPackStatus::Generating->value]); try { - $this->executeGeneration($reviewPack, $operationRun, $tenant); + $this->executeGeneration($reviewPack, $operationRun, $tenant, $operationRunService); } catch (Throwable $e) { - $this->markFailed($reviewPack, $operationRun, 'generation_error', $e->getMessage()); + $this->markFailed($reviewPack, $operationRun, $operationRunService, 'generation_error', $e->getMessage()); throw $e; } } - private function executeGeneration(ReviewPack $reviewPack, OperationRun $operationRun, Tenant $tenant): void + private function executeGeneration(ReviewPack $reviewPack, OperationRun $operationRun, Tenant $tenant, OperationRunService $operationRunService): void { $options = $reviewPack->options ?? []; $includePii = (bool) ($options['include_pii'] ?? true); @@ -175,16 +172,13 @@ private function executeGeneration(ReviewPack $reviewPack, OperationRun $operati 'summary' => $summary, ]); - // 13. Mark OperationRun completed - $operationRun->update([ - 'status' => OperationRunStatus::Completed->value, - 'outcome' => OperationRunOutcome::Succeeded->value, - 'completed_at' => now(), - 'summary_counts' => $summary, - ]); - - // 14. Notify initiator - $this->notifyInitiator($reviewPack, 'ready'); + // 13. Mark OperationRun completed (auto-sends OperationRunCompleted notification) + $operationRunService->updateRun( + $operationRun, + status: OperationRunStatus::Completed->value, + outcome: OperationRunOutcome::Succeeded->value, + summaryCounts: $summary, + ); } /** @@ -382,38 +376,17 @@ private function assembleZip(string $tempFile, array $fileMap): void $zip->close(); } - private function markFailed(ReviewPack $reviewPack, OperationRun $operationRun, string $reasonCode, string $errorMessage): void + private function markFailed(ReviewPack $reviewPack, OperationRun $operationRun, OperationRunService $operationRunService, string $reasonCode, string $errorMessage): void { $reviewPack->update(['status' => ReviewPackStatus::Failed->value]); - $operationRun->update([ - 'status' => OperationRunStatus::Completed->value, - 'outcome' => OperationRunOutcome::Failed->value, - 'completed_at' => now(), - 'context' => array_merge($operationRun->context ?? [], [ - 'reason_code' => $reasonCode, - 'error_message' => mb_substr($errorMessage, 0, 500), - ]), - ]); - - $this->notifyInitiator($reviewPack, 'failed', $reasonCode); - } - - private function notifyInitiator(ReviewPack $reviewPack, string $status, ?string $reasonCode = null): void - { - $initiator = $reviewPack->initiator; - - if (! $initiator) { - return; - } - - try { - $initiator->notify(new ReviewPackStatusNotification($reviewPack, $status, $reasonCode)); - } catch (Throwable $e) { - Log::warning('Failed to send ReviewPack notification', [ - 'review_pack_id' => $reviewPack->getKey(), - 'error' => $e->getMessage(), - ]); - } + $operationRunService->updateRun( + $operationRun, + status: OperationRunStatus::Completed->value, + outcome: OperationRunOutcome::Failed->value, + failures: [ + ['code' => $reasonCode, 'message' => mb_substr($errorMessage, 0, 500)], + ], + ); } } diff --git a/app/Notifications/ReviewPackStatusNotification.php b/app/Notifications/ReviewPackStatusNotification.php deleted file mode 100644 index 9128de3..0000000 --- a/app/Notifications/ReviewPackStatusNotification.php +++ /dev/null @@ -1,91 +0,0 @@ - - */ - public function via(object $notifiable): array - { - return ['database']; - } - - /** - * @return array - */ - public function toDatabase(object $notifiable): array - { - $title = match ($this->status) { - 'ready' => 'Review Pack ready', - 'failed' => 'Review Pack generation failed', - default => 'Review Pack status updated', - }; - - $body = match ($this->status) { - 'ready' => 'Your tenant review pack has been generated and is ready for download.', - 'failed' => sprintf( - 'Review pack generation failed%s.', - $this->reasonCode ? ": {$this->reasonCode}" : '', - ), - default => 'Review pack status changed.', - }; - - $color = match ($this->status) { - 'ready' => 'success', - 'failed' => 'danger', - default => 'gray', - }; - - $icon = match ($this->status) { - 'ready' => 'heroicon-o-document-arrow-down', - 'failed' => 'heroicon-o-exclamation-triangle', - default => 'heroicon-o-document-text', - }; - - $actions = []; - $tenant = $this->reviewPack->tenant; - - if ($tenant instanceof Tenant && $this->status === 'ready') { - $actions[] = Action::make('view_pack') - ->label('View pack') - ->url(route('filament.admin.resources.review-packs.view', [ - 'tenant' => $tenant->external_id, - 'record' => $this->reviewPack->getKey(), - ])) - ->toArray(); - } - - return [ - 'format' => 'filament', - 'title' => $title, - 'body' => $body, - 'color' => $color, - 'duration' => 'persistent', - 'actions' => $actions, - 'icon' => $icon, - 'iconColor' => $color, - 'status' => null, - 'view' => null, - 'viewData' => [ - 'review_pack_id' => $this->reviewPack->getKey(), - 'status' => $this->status, - 'reason_code' => $this->reasonCode, - ], - ]; - } -} diff --git a/app/Services/ReviewPackService.php b/app/Services/ReviewPackService.php index caf5fbc..e05c4a7 100644 --- a/app/Services/ReviewPackService.php +++ b/app/Services/ReviewPackService.php @@ -56,10 +56,12 @@ public function generate(Tenant $tenant, User $user, array $options = []): Revie 'summary' => [], ]); - GenerateReviewPackJob::dispatch( - reviewPackId: (int) $reviewPack->getKey(), - operationRunId: (int) $operationRun->getKey(), - ); + $this->operationRunService->dispatchOrFail($operationRun, function () use ($reviewPack, $operationRun): void { + GenerateReviewPackJob::dispatch( + reviewPackId: (int) $reviewPack->getKey(), + operationRunId: (int) $operationRun->getKey(), + ); + }); return $reviewPack; } diff --git a/app/Support/OpsUx/OperationSummaryKeys.php b/app/Support/OpsUx/OperationSummaryKeys.php index d0c55d3..1359c0a 100644 --- a/app/Support/OpsUx/OperationSummaryKeys.php +++ b/app/Support/OpsUx/OperationSummaryKeys.php @@ -36,6 +36,9 @@ public static function all(): array 'report_created', 'report_deduped', 'alert_events_produced', + 'finding_count', + 'report_count', + 'operation_count', ]; } } diff --git a/public/js/tenantpilot/livewire-intercept-shim.js b/public/js/tenantpilot/livewire-intercept-shim.js index 8bac711..4356970 100644 --- a/public/js/tenantpilot/livewire-intercept-shim.js +++ b/public/js/tenantpilot/livewire-intercept-shim.js @@ -6,66 +6,93 @@ return; } - const Livewire = window.Livewire; + const applyShim = () => { + const Livewire = window.Livewire; - if (!Livewire || typeof Livewire.interceptMessage !== 'function') { - return; - } - - if (Livewire.__tenantpilotInterceptMessageShimApplied) { - return; - } - - const original = Livewire.interceptMessage.bind(Livewire); - - Livewire.interceptMessage = (handler) => { - if (typeof handler !== 'function') { - return original(handler); + if (!Livewire || typeof Livewire.interceptMessage !== 'function') { + return false; } - return original((context) => { - if (!context || typeof context !== 'object') { - return handler(context); + if (Livewire.__tenantpilotInterceptMessageShimApplied) { + return true; + } + + const original = Livewire.interceptMessage.bind(Livewire); + + Livewire.interceptMessage = (handler) => { + if (typeof handler !== 'function') { + return original(handler); } - const originalOnFinish = context.onFinish; - const originalOnSuccess = context.onSuccess; - - if (typeof originalOnFinish !== 'function' || typeof originalOnSuccess !== 'function') { - return handler(context); - } - - const finishCallbacks = []; - - const onFinish = (callback) => { - if (typeof callback === 'function') { - finishCallbacks.push(callback); + return original((context) => { + if (!context || typeof context !== 'object') { + return handler(context); } - return originalOnFinish(callback); - }; + const originalOnFinish = context.onFinish; + const originalOnSuccess = context.onSuccess; - const onSuccess = (callback) => { - return originalOnSuccess((...args) => { - // Ensure any registered finish callbacks are run before success callbacks. - // We don't swallow errors; we just stabilize ordering. - for (const finishCallback of finishCallbacks) { - finishCallback(...args); - } + if (typeof originalOnFinish !== 'function' || typeof originalOnSuccess !== 'function') { + return handler(context); + } + const finishCallbacks = []; + + const onFinish = (callback) => { if (typeof callback === 'function') { - return callback(...args); + finishCallbacks.push(callback); } - }); - }; - return handler({ - ...context, - onFinish, - onSuccess, + return originalOnFinish(callback); + }; + + const onSuccess = (callback) => { + return originalOnSuccess((...args) => { + // Ensure any registered finish callbacks are run before success callbacks. + // We don't swallow errors; we just stabilize ordering. + for (const finishCallback of finishCallbacks) { + finishCallback(...args); + } + + if (typeof callback === 'function') { + return callback(...args); + } + }); + }; + + return handler({ + ...context, + onFinish, + onSuccess, + }); }); - }); + }; + + Livewire.__tenantpilotInterceptMessageShimApplied = true; + + return true; }; - Livewire.__tenantpilotInterceptMessageShimApplied = true; + if (applyShim()) { + return; + } + + // Livewire may not be initialized yet when this script runs (depending on + // script tag order). Try again on `livewire:init` and with a short fallback poll. + const onInit = () => { + applyShim(); + }; + + window.addEventListener('livewire:init', onInit, { once: true }); + document.addEventListener('livewire:init', onInit, { once: true }); + + let tries = 0; + const maxTries = 50; + const timer = setInterval(() => { + tries += 1; + + if (applyShim() || tries >= maxTries) { + clearInterval(timer); + } + }, 100); })(); diff --git a/specs/055-ops-ux-rollout/spec.md b/specs/055-ops-ux-rollout/spec.md index 3762867..fbc48b1 100644 --- a/specs/055-ops-ux-rollout/spec.md +++ b/specs/055-ops-ux-rollout/spec.md @@ -184,7 +184,7 @@ ### Functional Requirements ### Canonical allowed summary keys (single source of truth) The following keys are the ONLY allowed summary keys for Ops-UX rendering: -`total, processed, succeeded, failed, skipped, compliant, noncompliant, unknown, created, updated, deleted, items, tenants, high, medium, low, findings_created, findings_resolved, findings_reopened, findings_unchanged, errors_recorded, posture_score, report_created, report_deduped, alert_events_produced` +`total, processed, succeeded, failed, skipped, compliant, noncompliant, unknown, created, updated, deleted, items, tenants, high, medium, low, findings_created, findings_resolved, findings_reopened, findings_unchanged, errors_recorded, posture_score, report_created, report_deduped, alert_events_produced, finding_count, report_count, operation_count` All normalizers/renderers MUST reference this canonical list (no duplicated lists in multiple places). diff --git a/tests/Feature/LivewireInterceptShimTest.php b/tests/Feature/LivewireInterceptShimTest.php new file mode 100644 index 0000000..ddc8eea --- /dev/null +++ b/tests/Feature/LivewireInterceptShimTest.php @@ -0,0 +1,16 @@ +get('/admin/login') + ->assertSuccessful() + ->assertSee('js/tenantpilot/livewire-intercept-shim.js', escape: false); +}); + +it('ships a shim that waits for Livewire initialization', function (): void { + $js = file_get_contents(public_path('js/tenantpilot/livewire-intercept-shim.js')); + + expect($js)->toBeString(); + expect($js)->toContain('livewire:init'); +}); diff --git a/tests/Feature/ReviewPack/ReviewPackGenerationTest.php b/tests/Feature/ReviewPack/ReviewPackGenerationTest.php index c2a5543..e6f829c 100644 --- a/tests/Feature/ReviewPack/ReviewPackGenerationTest.php +++ b/tests/Feature/ReviewPack/ReviewPackGenerationTest.php @@ -8,7 +8,8 @@ use App\Models\ReviewPack; use App\Models\StoredReport; use App\Models\Tenant; -use App\Notifications\ReviewPackStatusNotification; +use App\Notifications\OperationRunCompleted; +use App\Notifications\OperationRunQueued; use App\Services\ReviewPackService; use App\Support\OperationRunOutcome; use App\Support\OperationRunStatus; @@ -84,7 +85,7 @@ function seedTenantWithData(Tenant $tenant): void reviewPackId: (int) $pack->getKey(), operationRunId: (int) $pack->operation_run_id, ); - $job->handle(); + app()->call([$job, 'handle']); $pack->refresh(); @@ -108,8 +109,8 @@ function seedTenantWithData(Tenant $tenant): void expect($opRun->status)->toBe(OperationRunStatus::Completed->value); expect($opRun->outcome)->toBe(OperationRunOutcome::Succeeded->value); - // Notification sent - Notification::assertSentTo($user, ReviewPackStatusNotification::class); + // Notification sent (standard OperationRunCompleted via OperationRunService) + Notification::assertSentTo($user, OperationRunCompleted::class); }); // ─── Failure Path ────────────────────────────────────────────── @@ -138,7 +139,7 @@ function seedTenantWithData(Tenant $tenant): void ); try { - $job->handle(); + app()->call([$job, 'handle']); } catch (\RuntimeException) { // Expected — the job re-throws after marking failed } @@ -150,11 +151,10 @@ function seedTenantWithData(Tenant $tenant): void $opRun = OperationRun::query()->find($pack->operation_run_id); expect($opRun->status)->toBe(OperationRunStatus::Completed->value); expect($opRun->outcome)->toBe(OperationRunOutcome::Failed->value); - expect($opRun->context['reason_code'])->toBe('generation_error'); + expect($opRun->failure_summary)->toBeArray(); + expect($opRun->failure_summary[0]['code'])->toBe('generation_error'); - Notification::assertSentTo($user, ReviewPackStatusNotification::class, function ($notification) { - return $notification->status === 'failed'; - }); + Notification::assertSentTo($user, OperationRunCompleted::class); }); // ─── Empty Reports ────────────────────────────────────────────── @@ -172,7 +172,7 @@ function seedTenantWithData(Tenant $tenant): void reviewPackId: (int) $pack->getKey(), operationRunId: (int) $pack->operation_run_id, ); - $job->handle(); + app()->call([$job, 'handle']); $pack->refresh(); @@ -198,7 +198,7 @@ function seedTenantWithData(Tenant $tenant): void reviewPackId: (int) $pack->getKey(), operationRunId: (int) $pack->operation_run_id, ); - $job->handle(); + app()->call([$job, 'handle']); $pack->refresh(); expect($pack->status)->toBe(ReviewPackStatus::Ready->value); @@ -254,7 +254,7 @@ function seedTenantWithData(Tenant $tenant): void reviewPackId: (int) $pack->getKey(), operationRunId: (int) $pack->operation_run_id, ); - $job->handle(); + app()->call([$job, 'handle']); $pack->refresh(); @@ -303,6 +303,19 @@ function seedTenantWithData(Tenant $tenant): void }); }); +it('sends queued database notification when review pack generation is requested', function (): void { + Queue::fake(); + Notification::fake(); + + [$user, $tenant] = createUserWithTenant(); + + /** @var ReviewPackService $service */ + $service = app(ReviewPackService::class); + $service->generate($tenant, $user); + + Notification::assertSentTo($user, OperationRunQueued::class); +}); + // ─── OperationRun Type ────────────────────────────────────────── it('creates an OperationRun of type review_pack_generate', function (): void { diff --git a/tests/Feature/ReviewPack/ReviewPackResourceTest.php b/tests/Feature/ReviewPack/ReviewPackResourceTest.php index eb20509..60d2648 100644 --- a/tests/Feature/ReviewPack/ReviewPackResourceTest.php +++ b/tests/Feature/ReviewPack/ReviewPackResourceTest.php @@ -5,12 +5,15 @@ use App\Filament\Resources\ReviewPackResource; use App\Filament\Resources\ReviewPackResource\Pages\ListReviewPacks; use App\Filament\Resources\ReviewPackResource\Pages\ViewReviewPack; +use App\Models\OperationRun; use App\Models\ReviewPack; use App\Models\Tenant; +use App\Services\ReviewPackService; use App\Support\Auth\UiTooltips; use App\Support\ReviewPackStatus; use Filament\Facades\Filament; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\Queue; use Illuminate\Support\Facades\Storage; use Livewire\Livewire; @@ -95,6 +98,45 @@ ->assertActionExists('generate_pack', fn ($action): bool => $action->getTooltip() === UiTooltips::insufficientPermission()); }); +it('reuses an existing ready pack instead of starting a new run', function (): void { + Queue::fake(); + + $tenant = Tenant::factory()->create(); + [$user, $tenant] = createUserWithTenant(tenant: $tenant, role: 'owner'); + + $fingerprint = app(ReviewPackService::class)->computeFingerprint($tenant, [ + 'include_pii' => true, + 'include_operations' => true, + ]); + + ReviewPack::factory()->ready()->create([ + 'tenant_id' => (int) $tenant->getKey(), + 'workspace_id' => (int) $tenant->workspace_id, + 'initiated_by_user_id' => (int) $user->getKey(), + 'fingerprint' => $fingerprint, + 'expires_at' => now()->addDay(), + ]); + + $tenant->makeCurrent(); + Filament::setTenant($tenant, true); + + $operationRunsBefore = OperationRun::query()->count(); + $reviewPacksBefore = ReviewPack::query()->count(); + + Livewire::actingAs($user) + ->test(ListReviewPacks::class) + ->callAction('generate_pack', [ + 'include_pii' => true, + 'include_operations' => true, + ]) + ->assertNotified(); + + expect(OperationRun::query()->count())->toBe($operationRunsBefore); + expect(ReviewPack::query()->count())->toBe($reviewPacksBefore); + + Queue::assertNothingPushed(); +}); + // ─── Table Row Actions ─────────────────────────────────────── it('shows the download action for a ready pack', function (): void {