From b5166c02db2e38e18c1986afda7855e6c538cb3d Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Mon, 23 Feb 2026 21:21:41 +0100 Subject: [PATCH 1/4] spec(110): Ops-UX Enforcement & Cleanup (Enterprise Standard Rollout) --- .../checklists/requirements.md | 37 +++ specs/110-ops-ux-enforcement/spec.md | 266 ++++++++++++++++++ 2 files changed, 303 insertions(+) create mode 100644 specs/110-ops-ux-enforcement/checklists/requirements.md create mode 100644 specs/110-ops-ux-enforcement/spec.md diff --git a/specs/110-ops-ux-enforcement/checklists/requirements.md b/specs/110-ops-ux-enforcement/checklists/requirements.md new file mode 100644 index 0000000..73ecbd0 --- /dev/null +++ b/specs/110-ops-ux-enforcement/checklists/requirements.md @@ -0,0 +1,37 @@ +# Specification Quality Checklist: Ops-UX Enforcement & Cleanup (Enterprise Standard Rollout) + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-02-23 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- All items pass. Spec is ready for `/speckit.plan`. +- Scope violation tables provide concrete remediation targets (11 files across 4 violation categories). +- Guard tests (FR-012) are explicitly specced as static analysis (grep-based Pest), not runtime — no application booting required. +- P2 tasks (T110-040/041) are optional and explicitly gated; they do not block P0/P1 delivery. diff --git a/specs/110-ops-ux-enforcement/spec.md b/specs/110-ops-ux-enforcement/spec.md new file mode 100644 index 0000000..fbc6e9e --- /dev/null +++ b/specs/110-ops-ux-enforcement/spec.md @@ -0,0 +1,266 @@ +# Feature Specification: Ops-UX Enforcement & Cleanup (Enterprise Standard Rollout) + +**Feature Branch**: `110-ops-ux-enforcement` +**Created**: 2026-02-23 +**Status**: Draft +**Depends On**: `specs/055-ops-ux-rollout/spec.md` + +## Spec Scope Fields *(mandatory)* + +- **Scope**: tenant (all operation-run-producing flows within a tenant) +- **Primary Routes**: No new routes. Affected internal flows: Inventory Sync, Backup Schedule Retention, Backup Schedule Run, Bulk Policy Export, Bulk Restore Run Force Delete, Add/Remove Policies to Backup Set, Restore Run execution. +- **Data Ownership**: `operation_runs` (tenant-scoped), `notifications` (tenant user-scoped). No schema changes required. +- **RBAC**: No new RBAC surfaces. Existing capability gates on triggering operations remain unchanged. Notification delivery is limited to the initiator user. System/scheduled runs with no initiator receive no DB notification. + +*Canonical-view fields not applicable — no new views introduced.* + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 — No Silent Completions (Priority: P1) + +As a tenant admin, when I trigger any tracked operation (Inventory Sync, Retention, Backup Schedule Run), I always receive exactly one terminal DB notification upon completion, so I can audit the outcome without checking the Monitoring hub manually. + +**Why this priority**: Silent completions break auditability — the core promise of the Ops-UX system. Missing terminal notifications mean admins have no persistent outcome record outside the Monitoring hub. + +**Independent Test**: Trigger (or simulate) an inventory sync run to terminal state and assert exactly one `OperationRunCompleted` DB notification exists for the initiator. + +**Acceptance Scenarios**: + +1. **Given** an `inventory_sync` OperationRun with an initiator, **When** `InventorySyncService` transitions it to a terminal outcome, **Then** exactly one `OperationRunCompleted` DB notification is persisted for the initiator user. +2. **Given** an `apply_backup_retention` OperationRun with an initiator, **When** `ApplyBackupScheduleRetentionJob` completes (success or failure), **Then** exactly one `OperationRunCompleted` DB notification is persisted for the initiator user. +3. **Given** an OperationRun with **no initiator** (system/scheduled run), **When** the run transitions to terminal, **Then** zero DB notifications are emitted. + +--- + +### User Story 2 — No Notification Spam (Priority: P1) + +As a tenant admin, I never receive duplicate completion DB notifications for a single run, and I never receive queued/running state DB notifications for any operation. + +**Why this priority**: Notification spam erodes trust in the notification surface. Even one duplicate makes admins ignore notifications — defeating the entire audit layer. + +**Independent Test**: Simulate a `RunBackupScheduleJob` to completion and assert zero queued/running DB notifications exist, and exactly one terminal DB notification. + +**Acceptance Scenarios**: + +1. **Given** a backup schedule run job enqueued and completed, **When** all job code paths execute, **Then** zero queued/running DB notifications are persisted, and exactly one terminal `OperationRunCompleted` exists for the initiator. +2. **Given** a `BulkPolicyExportJob` that reaches terminal state via any path (success / abort / circuit-break), **Then** exactly one terminal `OperationRunCompleted` DB notification exists and zero notifications from job-level `sendToDatabase()` calls. +3. **Given** any bulk job that previously sent custom completion DB notifications (`BulkRestoreRunForceDeleteJob`, `AddPoliciesToBackupSetJob`, `RemovePoliciesFromBackupSetJob`), **When** the job completes, **Then** no job-level DB notifications are emitted. + +--- + +### User Story 3 — Legacy Notification Removed (Priority: P1) + +As a tenant admin running a restore operation, my completion feedback comes exclusively from the canonical `OperationRunCompleted` notification, not from a legacy `RunStatusChangedNotification` with inconsistent copy or link behavior. + +**Why this priority**: The legacy class is an out-of-system notification that bypasses canonical delivery, creating inconsistent UX copy and a second notification channel that cannot be centrally controlled. + +**Independent Test**: Confirm `RunStatusChangedNotification` class does not exist in `app/` and `ExecuteRestoreRunJob` no longer references it. Restore run completion produces exactly one `OperationRunCompleted`. + +**Acceptance Scenarios**: + +1. **Given** `ExecuteRestoreRunJob` completes a restore run to terminal state, **Then** no `RunStatusChangedNotification` is dispatched, and exactly one `OperationRunCompleted` DB notification is persisted for the initiator. +2. **Given** a developer searches `app/` and `tests/` for `RunStatusChangedNotification`, **Then** no results are found (class deleted, all references removed). + +--- + +### User Story 4 — Regression Guards Enforce the Constitution (Priority: P1) + +As a developer working on the repo, if I accidentally introduce a direct `$operationRun->update(['status' => ...])` outside `OperationRunService`, a CI guard test immediately fails with a clear file + snippet report so I know exactly what to fix. + +**Why this priority**: Without automated guards, the enforcement degrades over time as new features are added. Guards are the only scalable way to maintain the constitution without manual code review on every PR. + +**Independent Test**: Introduce a synthetic violation in a temp file, run the guard test, confirm it fails with actionable output. Remove the violation, confirm the test passes. + +**Acceptance Scenarios**: + +1. **Given** the codebase contains a direct `$operationRun->update(['status' => ...])` outside `OperationRunService`, **When** the guard test runs, **Then** it fails and outputs the violating file path and snippet. +2. **Given** a job file contains both an OperationRun reference and a `sendToDatabase()` call (not on the allowlist), **When** the DB-notification guard test runs, **Then** it fails with the file path. +3. **Given** any file in `app/` or `tests/` references `RunStatusChangedNotification`, **When** the legacy-class guard test runs, **Then** it fails. +4. **Given** the codebase has no violations, **When** all guard tests run, **Then** all pass green. + +--- + +### User Story 5 — Canonical "Already Queued" Toast (Priority: P2) + +As a tenant admin triggering an operation that is already queued, I receive a consistent, canonical "already queued" toast message rather than ad-hoc copy from individual feature components, so the experience is uniform across all operation types. + +**Why this priority**: P2 polish — non-blocking, but addresses the last remaining ad-hoc UX copy point identified in the audit. + +**Independent Test**: Trigger the "already queued" dedup path in `BackupSetPolicyPickerTable` and assert the toast uses the canonical presenter output. + +**Acceptance Scenarios**: + +1. **Given** an operation is already queued, **When** a user attempts to queue it again via the policy picker, **Then** a toast is shown using `OperationUxPresenter::alreadyQueuedToast(...)` with canonical copy. + +--- + +### Edge Cases + +- What happens when a job fails with an unhandled exception — does the OperationRun get stuck in a non-terminal state? *(Assumption: existing job `failed()` callback or `finally` block already transitions via service; confirm during implementation per-file.)* +- What happens when `OperationRunService` itself throws during terminal transition? *(Assumption: run stays non-terminal; pre-existing behavior outside scope of this spec.)* +- What if an OperationRun has no initiator and a job previously sent a DB notification — will removing that path cause silence? *(Confirmed acceptable: system runs are auditable via Monitoring hub only.)* + +## Requirements *(mandatory)* + +**Constitution alignment — OPS-UX-001**: This spec enforces the Ops-UX 3-surface contract. All status/outcome transitions must be routed through `OperationRunService`. Terminal DB notifications are sent exclusively via `OperationRunCompleted` from within the service. No job or feature code may send its own completion DB notification. + +**Constitution alignment — No new Filament screens**: This spec makes no changes to Filament Resources, RelationManagers, or Pages (beyond the optional P2 presenter helper). The UI Action Matrix is not required. + +**Constitution alignment — No new Graph calls / OperationRun types**: This spec modifies existing operation flows only; it does not introduce new operation types or Graph calls. + +**Constitution alignment — BADGE-001**: No new status badge values introduced. + +### Functional Requirements + +- **FR-001**: All OperationRun `status` and `outcome` field transitions MUST go through `OperationRunService` canonical transition methods. Direct `$operationRun->update(['status' => ...])`, `$operationRun->status = ...`, `$operationRun->outcome = ...`, or bulk query updates on status/outcome are forbidden outside `OperationRunService`. +- **FR-002**: `OperationRunService` MUST emit exactly one `OperationRunCompleted` DB notification to the initiator when transitioning a run to a terminal outcome (when an initiator exists). +- **FR-003**: No job or service code outside `OperationRunService` MAY emit a DB notification representing operation completion, abort, or terminal state. +- **FR-004**: No code anywhere MAY emit a DB notification for queued or running operation states. +- **FR-005**: `RunStatusChangedNotification` MUST be deleted. No references to it may remain in `app/` or `tests/`. +- **FR-006**: `InventorySyncService` MUST transition to terminal state exclusively via `OperationRunService`, not via direct model updates. +- **FR-007**: `ApplyBackupScheduleRetentionJob` MUST transition to terminal state exclusively via `OperationRunService`. +- **FR-008**: `TenantpilotBackfillWorkspaceIds` console command MUST use the canonical transition if it transitions OperationRun status (initiator may be null; no DB notification emitted in that case). +- **FR-009**: `RunBackupScheduleJob` MUST NOT emit any queued or completion DB notifications; outcome notification is handled by `OperationRunService` terminal transition. +- **FR-010**: `BulkPolicyExportJob`, `BulkRestoreRunForceDeleteJob`, `AddPoliciesToBackupSetJob`, and `RemovePoliciesFromBackupSetJob` MUST NOT call `sendToDatabase()` for operation completion or abort feedback. +- **FR-011**: Context-only updates (e.g., updating `context`, `message`, `reason_code` fields without touching `status` or `outcome`) are permitted directly on the model outside `OperationRunService`. +- **FR-012**: Three Pest guard tests MUST exist and pass in CI: + - Guard A: Detects direct status/outcome transitions outside `OperationRunService`; reports file + snippet. + - Guard B: Detects `sendToDatabase()` calls in operation-flow jobs that also reference OperationRun; reports file path. + - Guard C: Detects any reference to `RunStatusChangedNotification` in `app/` or `tests/`. +- **FR-013** *(P2)*: `OperationUxPresenter` MUST expose an `alreadyQueuedToast(...)` static helper returning canonical copy + duration (+ optional "View run" action). +- **FR-014** *(P2)*: `BackupSetPolicyPickerTable` dedup toast MUST use `OperationUxPresenter::alreadyQueuedToast(...)`. + +## Scope (Known Violations — Remediation Targets) + +### Status transition bypass (direct model update — silent completion) + +| File | Violation | Priority | +|------|-----------|----------| +| `app/Services/Inventory/InventorySyncService.php` | Direct `update([status/outcome])` — silent completion | P0 | +| `app/Jobs/ApplyBackupScheduleRetentionJob.php` | Direct `update([...])` — silent completion | P0 | +| `app/Console/Commands/TenantpilotBackfillWorkspaceIds.php` | Direct status update | P1 | + +### Job-level DB notifications (duplicates / queued spam) + +| File | Violation | Priority | +|------|-----------|----------| +| `app/Jobs/RunBackupScheduleJob.php` | Queued DB notification + custom finished notification | P0 | +| `app/Jobs/BulkPolicyExportJob.php` | Multiple `sendToDatabase()` paths | P1 | +| `app/Jobs/BulkRestoreRunForceDeleteJob.php` | Multiple `sendToDatabase()` paths | P1 | +| `app/Jobs/AddPoliciesToBackupSetJob.php` | Custom completion DB notifications | P1 | +| `app/Jobs/RemovePoliciesFromBackupSetJob.php` | Custom completion DB notifications | P1 | + +### Legacy notification outside system + +| File | Violation | Priority | +|------|-----------|----------| +| `app/Jobs/ExecuteRestoreRunJob.php` | References `RunStatusChangedNotification` | P0 | +| `app/Notifications/RunStatusChangedNotification.php` | Class to delete | P0 | + +### Optional polish (P2) + +| File | Violation | Priority | +|------|-----------|----------| +| `app/Livewire/BackupSetPolicyPickerTable.php` | Ad-hoc "already queued" toast (non-canonical copy) | P2 | + +--- + +## Tasks + +### Phase 1 — P0: Fix silent completions + +- [ ] T110-001 (P0) `InventorySyncService` — replace direct terminal `update([status/outcome])` with `OperationRunService` canonical transition method +- [ ] T110-002 (P0) `ApplyBackupScheduleRetentionJob` — replace direct terminal `update([...])` with canonical transition method +- [ ] T110-003 (P1) `TenantpilotBackfillWorkspaceIds` — replace direct status update with canonical transition method (initiator may be null) + +### Phase 2 — P0: Remove legacy notification + +- [ ] T110-010 (P0) `ExecuteRestoreRunJob` — remove `RunStatusChangedNotification` invocation; rely on service terminal notification +- [ ] T110-011 (P0) Delete `RunStatusChangedNotification` class; assert zero references remain + +### Phase 3 — P0/P1: Remove job-level DB notifications + +- [ ] T110-020 (P0) `RunBackupScheduleJob` — remove queued DB notification producer +- [ ] T110-021 (P0) `RunBackupScheduleJob` — remove custom finished DB notification producer +- [ ] T110-022 (P1) `BulkPolicyExportJob` — remove all `sendToDatabase()` branches for completion/abort +- [ ] T110-023 (P1) `BulkRestoreRunForceDeleteJob` — remove all completion/abort `sendToDatabase()` branches +- [ ] T110-024 (P1) `AddPoliciesToBackupSetJob` — remove custom completion/failure DB notifications +- [ ] T110-025 (P1) `RemovePoliciesFromBackupSetJob` — remove custom completion/failure DB notifications + +### Phase 4 — Guards (mandatory) + +- [ ] T110-030 (P0) Pest guard: No direct OperationRun status/outcome transitions outside OperationRunService (file + snippet report; context-only updates allowed) +- [ ] T110-031 (P0) Pest guard: Jobs must not emit `sendToDatabase()` in operation flows (allowlist escape hatch if needed, minimal) +- [ ] T110-032 (P0) Pest guard: No references to `RunStatusChangedNotification` in `app/` or `tests/` + +### Phase 5 — Optional polish (P2) + +- [ ] T110-040 (P2) Add `OperationUxPresenter::alreadyQueuedToast(...)` canonical helper +- [ ] T110-041 (P2) Migrate `BackupSetPolicyPickerTable` dedup toast to `alreadyQueuedToast` + +--- + +## Testing Plan (Pest) + +### Guard tests (mandatory — CI enforcement layer) +- `tests/Feature/OpsUx/Constitution/DirectStatusTransitionGuardTest.php` +- `tests/Feature/OpsUx/Constitution/JobDbNotificationGuardTest.php` +- `tests/Feature/OpsUx/Constitution/LegacyNotificationGuardTest.php` + +### Regression tests (mandatory for P0 fixes) +- `tests/Feature/OpsUx/Regression/InventorySyncTerminalNotificationTest.php` — exactly one `OperationRunCompleted` for initiator; none for system run +- `tests/Feature/OpsUx/Regression/BackupRetentionTerminalNotificationTest.php` — exactly one `OperationRunCompleted`; no direct model transitions +- `tests/Feature/OpsUx/Regression/BackupScheduleRunNotificationTest.php` — zero queued DB notifications; exactly one terminal notification +- `tests/Feature/OpsUx/Regression/BulkJobCircuitBreakerTest.php` — circuit-breaker / abort path yields exactly one terminal `OperationRunCompleted` and zero job-level DB notifications (representative bulk job) + +### Recommended test locations +- Guards: `tests/Feature/OpsUx/Constitution/` +- Regressions: `tests/Feature/OpsUx/Regression/` + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: Every tracked operation with an initiator produces exactly one persistent DB notification upon terminal completion — zero operations complete silently. +- **SC-002**: Zero queued/running DB notifications are emitted across all operation flows. +- **SC-003**: Zero duplicate completion DB notifications for any single operation run across all exit paths (success, failure, partial, circuit-break). +- **SC-004**: `RunStatusChangedNotification` class is fully deleted — zero references remain in the codebase. +- **SC-005**: All three CI guard tests pass green on a clean codebase and fail with actionable output when a synthetic violation is introduced. +- **SC-006**: All existing OpsUx test suites and related test files pass without regression after changes. +- **SC-007** *(P2)*: All "already queued" dedup feedback paths use identical canonical copy, verifiable by a single source-of-truth presenter call. + +--- + +## Definition of Done (DoD) + +- [ ] All in-scope files no longer directly update status/outcome outside `OperationRunService` +- [ ] All in-scope jobs no longer emit completion/abort/queued DB notifications +- [ ] `RunStatusChangedNotification` deleted; zero references in `app/` and `tests/` +- [ ] Three guard tests exist and pass in CI (and fail with actionable output on synthetic violations) +- [ ] All OpsUx regression tests pass +- [ ] Full test suite green (no regressions) +- [ ] Pint formatting clean for all touched files +- [ ] `AGENTS.md` / constitution updated to reference the non-negotiable Ops-UX rule if not already present from 055 follow-up + +--- + +## Assumptions + +1. `OperationRunService` already exposes a canonical terminal transition method (from Spec 055); this spec calls it, not redeclares it. +2. The `failed()` job lifecycle callback or try/finally blocks in affected jobs already exist (or will be added) to ensure terminal transitions even on unhandled exceptions — confirmed during implementation per-file. +3. Guard tests are static analysis (filesystem grep-based) Pest tests, not runtime tests. They do not require a running application. +4. The allowlist for Guard B (job DB notifications) is intentionally minimal. Any new entry requires justification in a spec comment. +5. P2 tasks (T110-040/041) are optional and do not gate release of P0/P1 work. + +--- + +## Rollout / PR Slicing + +| PR | Tasks | Priority | +|----|-------|----------| +| PR-A | T110-001, T110-002 + regression tests (inventory sync, retention) | P0 | +| PR-B | T110-010, T110-011 + restore run tests | P0 | +| PR-C | T110-020, T110-021 + backup schedule tests | P0 | +| PR-D | T110-022–T110-025 + bulk job tests | P1 | +| PR-E | T110-030, T110-031, T110-032 (guards — can land early) | P0 | +| PR-F | T110-040, T110-041 (optional polish) | P2 | -- 2.45.2 From 3c5f4c209b1deb4cadae87cdd6903690bcca7ec7 Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Mon, 23 Feb 2026 22:48:30 +0100 Subject: [PATCH 2/4] spec(110): resolve scope, UI matrix, guard targets --- .github/agents/copilot-instructions.md | 2 +- .../checklists/requirements.md | 8 +- .../contracts/no-api-changes.md | 9 + specs/110-ops-ux-enforcement/data-model.md | 39 ++++ specs/110-ops-ux-enforcement/plan.md | 157 +++++++++++++ specs/110-ops-ux-enforcement/quickstart.md | 23 ++ specs/110-ops-ux-enforcement/research.md | 89 ++++++++ specs/110-ops-ux-enforcement/spec.md | 80 +++---- specs/110-ops-ux-enforcement/tasks.md | 207 ++++++++++++++++++ 9 files changed, 570 insertions(+), 44 deletions(-) create mode 100644 specs/110-ops-ux-enforcement/contracts/no-api-changes.md create mode 100644 specs/110-ops-ux-enforcement/data-model.md create mode 100644 specs/110-ops-ux-enforcement/plan.md create mode 100644 specs/110-ops-ux-enforcement/quickstart.md create mode 100644 specs/110-ops-ux-enforcement/research.md create mode 100644 specs/110-ops-ux-enforcement/tasks.md diff --git a/.github/agents/copilot-instructions.md b/.github/agents/copilot-instructions.md index 27632db..2b2c6c4 100644 --- a/.github/agents/copilot-instructions.md +++ b/.github/agents/copilot-instructions.md @@ -58,8 +58,8 @@ ## Code Style PHP 8.4.15: Follow standard conventions ## Recent Changes +- 110-ops-ux-enforcement: Added PHP 8.4.x + Laravel 12, Filament v5, Livewire v4 - 109-review-pack-export: Added PHP 8.4 (Laravel 12) + Filament v5, Livewire v4, Laravel Framework v12 - 109-review-pack-export: Added [if applicable, e.g., PostgreSQL, CoreData, files or N/A] -- 109-review-pack-export: Added [if applicable, e.g., PostgreSQL, CoreData, files or N/A] diff --git a/specs/110-ops-ux-enforcement/checklists/requirements.md b/specs/110-ops-ux-enforcement/checklists/requirements.md index 73ecbd0..217eb47 100644 --- a/specs/110-ops-ux-enforcement/checklists/requirements.md +++ b/specs/110-ops-ux-enforcement/checklists/requirements.md @@ -31,7 +31,7 @@ ## Feature Readiness ## Notes -- All items pass. Spec is ready for `/speckit.plan`. -- Scope violation tables provide concrete remediation targets (11 files across 4 violation categories). -- Guard tests (FR-012) are explicitly specced as static analysis (grep-based Pest), not runtime — no application booting required. -- P2 tasks (T110-040/041) are optional and explicitly gated; they do not block P0/P1 delivery. +- All items pass. +- Spec includes the mandatory UI Action Matrix and explicitly states “no new screens” while allowing targeted start-surface cleanup. +- Remediation targets are enumerated in the spec’s “Known Violations” tables; the executable task list is the single source of truth in `tasks.md`. +- Guard tests (FR-012) are specced as static analysis (filesystem scan) with explicit allowlist, so they fail fast with actionable output. diff --git a/specs/110-ops-ux-enforcement/contracts/no-api-changes.md b/specs/110-ops-ux-enforcement/contracts/no-api-changes.md new file mode 100644 index 0000000..c7044a8 --- /dev/null +++ b/specs/110-ops-ux-enforcement/contracts/no-api-changes.md @@ -0,0 +1,9 @@ +# Contracts: No API Changes + +This feature introduces **no new HTTP endpoints**, no new API resources, and no changes to existing request/response contracts. + +Scope is limited to internal operation flow behavior: + +- OperationRun status/outcome transitions must go through `OperationRunService`. +- Queued/running DB notifications are banned. +- Terminal completion notification is initiator-only and emitted exactly once. diff --git a/specs/110-ops-ux-enforcement/data-model.md b/specs/110-ops-ux-enforcement/data-model.md new file mode 100644 index 0000000..3980cb2 --- /dev/null +++ b/specs/110-ops-ux-enforcement/data-model.md @@ -0,0 +1,39 @@ +# Phase 1 Design: Data Model (No Schema Changes) + +This feature does not introduce schema changes. It enforces consistent usage of existing entities. + +## Entity: OperationRun (`operation_runs`) + +**Ownership/scoping**: + +- Tenant-scoped operational artifact. +- Initiator user is optional (system/scheduled runs). + +**Key fields (existing)**: + +- `id` +- `workspace_id` / `tenant_id` (scoping) +- `user_id` (initiator; nullable) +- `type` (operation type string) +- `status` (`queued`/`running`/`completed`) +- `outcome` (terminal outcome; nullable until completed) +- `started_at`, `completed_at` +- `summary_counts` (JSON/array of numeric-only whitelisted keys) +- `failure_summary` (sanitized bounded array) +- `context` (additional metadata; mutable) + +**Invariants enforced by this feature**: + +- All transitions of `status` and `outcome` happen through `OperationRunService::updateRun()`. +- The only operation-related DB notification is the terminal `OperationRunCompleted`, emitted when transitioning into `completed` and only when `user_id` exists. + +## Entity: Database Notifications (`notifications`) + +**Ownership/scoping**: + +- User-scoped records (`notifiable_type=User`), used for persistent notification audit. + +**Invariants enforced by this feature**: + +- No queued/running state notifications are persisted. +- Exactly one terminal operation completion notification is persisted per OperationRun + initiator. diff --git a/specs/110-ops-ux-enforcement/plan.md b/specs/110-ops-ux-enforcement/plan.md new file mode 100644 index 0000000..e407cbf --- /dev/null +++ b/specs/110-ops-ux-enforcement/plan.md @@ -0,0 +1,157 @@ +# Implementation Plan: Ops-UX Enforcement & Cleanup (Enterprise Standard Rollout) + +**Branch**: `110-ops-ux-enforcement` | **Date**: 2026-02-23 | **Spec**: [specs/110-ops-ux-enforcement/spec.md](specs/110-ops-ux-enforcement/spec.md) +**Input**: Feature specification from `/specs/110-ops-ux-enforcement/spec.md` + +**Note**: This template is filled in by the `/speckit.plan` command. See `.specify/scripts/` for helper scripts. + +## Summary + +Enforce the Ops-UX “3-surface contract” by removing non-canonical operation-run status/outcome transitions and banning queued/running DB notifications across in-scope operation flows. + +Implementation is split into: + +- Cleanup: move all terminal transitions to `OperationRunService`, remove job-level `sendToDatabase()` completion/queued notifications, and delete the legacy `RunStatusChangedNotification`. +- Enforcement: add Pest guard tests (filesystem scans) that fail CI when these patterns reappear. +- Verification: add focused regression tests asserting “exactly one terminal `OperationRunCompleted` notification for initiator; none for system runs; and zero queued/running notifications”. + +## Technical Context + + + +**Language/Version**: PHP 8.4.x +**Primary Dependencies**: Laravel 12, Filament v5, Livewire v4 +**Storage**: PostgreSQL (Sail) +**Testing**: Pest v4 (`vendor/bin/sail artisan test --compact`) +**Target Platform**: Docker via Laravel Sail (local); Dokploy (staging/prod) +**Project Type**: Laravel monolith (Filament admin panel) +**Performance Goals**: Guard tests run in < 1s locally; overall targeted test pack < 30s +**Constraints**: + +- No schema changes and no new routes. +- Terminal DB notification is initiator-only and emitted exactly once. +- No queued/running DB notifications anywhere (including `OperationRunQueued`). +- Existing RBAC/capability gates for starting operations remain unchanged. + +**Scale/Scope**: Repo-wide enforcement via guard tests; remediation limited to enumerated violating flows in the spec. + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +This spec is a cleanup/enforcement layer and does not introduce new screens, Graph calls, or operation types. + +- Inventory-first / Read-write separation: PASS (no new inventory/backups semantics; no new write UX) +- Graph contract path: PASS (no Graph call additions) +- Deterministic capabilities: PASS (no capability model changes) +- RBAC-UX / isolation: PASS (no new routes or cross-plane access) +- Run observability: PASS (this spec strengthens dedupe + terminal notifications by removing ad-hoc notifications) +- Automation: PASS (no scheduling behavior changes; only notification/transition handling) +- Data minimization: PASS (removes notification spam; no payload storage changes) +- Badge semantics: PASS (no new badges) +- Filament UI contracts: PASS (no new screens; targeted updates to existing start surfaces only). The spec includes a mandatory UI Action Matrix. +## Project Structure + +### Documentation (this feature) + +```text +specs/110-ops-ux-enforcement/ +├── plan.md # This file (/speckit.plan command output) +├── research.md # Phase 0 output (/speckit.plan command) +├── data-model.md # Phase 1 output (/speckit.plan command) +├── quickstart.md # Phase 1 output (/speckit.plan command) +├── contracts/ # Phase 1 output (/speckit.plan command) +└── tasks.md # Phase 2 output (/speckit.tasks command - NOT created by /speckit.plan) +``` + +### Source Code (repository root) + + +```text +app/ +├── Jobs/ +├── Notifications/ +├── Services/ +└── Support/ + +config/ + +database/ +├── migrations/ +└── factories/ + +resources/ +├── views/ +└── css/ + +routes/ + +tests/ +└── Feature/ +``` + +**Structure Decision**: Laravel monolith. Enforcement is implemented as: + +- Targeted production code edits under `app/` (jobs/services/notifications). +- Guard + regression tests under `tests/Feature/OpsUx/**`. + +## Complexity Tracking + +> **Fill ONLY if Constitution Check has violations that must be justified** + +| Violation | Why Needed | Simpler Alternative Rejected Because | +|-----------|------------|-------------------------------------| +| [e.g., 4th project] | [current need] | [why 3 projects insufficient] | +| [e.g., Repository pattern] | [specific problem] | [why direct DB access insufficient] | + +## Phase 0 — Outline & Research + +**Inputs**: [specs/110-ops-ux-enforcement/spec.md](specs/110-ops-ux-enforcement/spec.md), [constitution](.specify/memory/constitution.md) + +Research is captured in [specs/110-ops-ux-enforcement/research.md](specs/110-ops-ux-enforcement/research.md) and resolves: + +- Guard-test heuristics (regex/scan approach) to keep false positives low. +- Where/how queued DB notifications are emitted today, and how to disable them by default. +- Preferred test strategy for “exactly once terminal notification”. + +## Phase 1 — Design & Contracts + +Design artifacts are captured in: + +- [specs/110-ops-ux-enforcement/data-model.md](specs/110-ops-ux-enforcement/data-model.md) (no schema changes; key entity behavior) +- [specs/110-ops-ux-enforcement/contracts/](specs/110-ops-ux-enforcement/contracts/) (no new API endpoints; documented as “no contract changes”) +- [specs/110-ops-ux-enforcement/quickstart.md](specs/110-ops-ux-enforcement/quickstart.md) (how to run the focused tests) + +**Design highlights**: + +- The only terminal DB notification is `OperationRunCompleted`, emitted from `OperationRunService` when an initiator exists. +- Guard A scans `app/**/*.php` for `->update([...])` arrays that include `status` and/or `outcome` keys, excluding `OperationRunService`. +- Guard B scans `app/**/*.php` for files that both reference an OperationRun signal and emit DB notifications (`sendToDatabase(` or `->notify(`), with a strict allowlist. +- Guard C ensures the legacy notification class is fully removed. + +## Phase 2 — Execution Plan (Implementation) + +Work is implemented in small, reviewable steps: + +1. **P0 fixes (silent completions)**: replace direct terminal updates with `OperationRunService` transitions in the enumerated services/jobs. +2. **P0 fixes (notification spam)**: remove queued/running and custom completion DB notifications from the enumerated jobs. +3. **P0 fixes (legacy removal)**: delete `RunStatusChangedNotification` and remove any invocation. +4. **Enforcement**: add Pest guard tests (A/B/C) + minimal allowlist. +5. **Verification**: add focused regression tests for the key flows (inventory sync, retention, backup schedule run) proving the “exactly once terminal notification” contract. + +**Out of scope**: new UI pages, schema changes, new operation types, Graph contract changes. + +## Constitution Check (Post-Phase 1 Re-check) + +- PASS: No new routes, no Graph calls, no new Filament screens. +- PASS: Strengthens Run Observability by centralizing terminal notification emission. +- PASS: RBAC-UX/isolation unaffected (no new access paths). diff --git a/specs/110-ops-ux-enforcement/quickstart.md b/specs/110-ops-ux-enforcement/quickstart.md new file mode 100644 index 0000000..766eb29 --- /dev/null +++ b/specs/110-ops-ux-enforcement/quickstart.md @@ -0,0 +1,23 @@ +# Quickstart: Ops-UX Enforcement & Cleanup + +## Prereqs + +- Sail running: `vendor/bin/sail up -d` + +## Run the focused guard tests + +Once implemented, run the guard tests only: + +- `vendor/bin/sail artisan test --compact --filter=OpsUx` + +If the tests are split by directory as in the spec: + +- `vendor/bin/sail artisan test --compact tests/Feature/OpsUx/Constitution` + +## Run the focused regression tests + +- `vendor/bin/sail artisan test --compact tests/Feature/OpsUx/Regression` + +## Format touched files + +- `vendor/bin/sail bin pint --dirty --format agent` diff --git a/specs/110-ops-ux-enforcement/research.md b/specs/110-ops-ux-enforcement/research.md new file mode 100644 index 0000000..ea1c0cb --- /dev/null +++ b/specs/110-ops-ux-enforcement/research.md @@ -0,0 +1,89 @@ +# Phase 0 Research: Ops-UX Enforcement & Cleanup + +This research document resolves implementation uncertainties for the enforcement spec and records key decisions. + +## Decision 1 — Implement guards as Pest “architecture tests” (filesystem scan) + +**Decision**: Implement Guard A/B/C as Pest tests that scan PHP source files in `app/` (and for Guard C also `tests/`) and fail CI with actionable file + snippet output. + +**Rationale**: + +- Runs in the same CI pipeline as the rest of the test suite (no new tooling step). +- Enforcement is repo-wide and does not rely on developer discipline or code review memory. +- Can produce highly actionable failure output (file path + snippet) without external dependencies. + +**Alternatives considered**: + +- PHPStan custom rules: higher precision, but adds configuration + toolchain scope. +- `nikic/php-parser` AST scanning: high precision but adds a dependency (out of bounds per repo constraints). + +## Decision 2 — Guard A detection uses tokenizer-assisted scanning (not pure regex) + +**Decision**: Implement Guard A using PHP’s built-in tokenizer (`token_get_all()`) to detect `->update([...])` call sites and then inspect the array literal for `status` / `outcome` keys. + +**Rationale**: + +- Pure multi-line regex is brittle with nested arrays, comments, and strings. +- Tokenization gives a dependency-free way to avoid common false positives and to compute an approximate line number. +- The spec’s intended heuristic (“`->update([...])` block contains status/outcome keys”) maps naturally to tokens. + +**Alternatives considered**: + +- Regex-only `DOTALL` matching with string/comment skipping (`(*SKIP)(*F)`): simpler, but can still drift across the wrong closing bracket and become noisy. +- Manual char-by-char scanner after a simple “find start” regex: workable, but token-based is typically easier to maintain in PHP. + +**Repo evidence**: + +- Canonical status/outcome transitions live in `OperationRunService::updateRun()`. +- Violations exist in jobs/services that call `$operationRun->update([... 'status' => ..., 'outcome' => ...])` directly (enumerated in the spec). + +## Decision 3 — Guard B flags “OperationRun in scope” + “DB notification emission” + +**Decision**: Implement Guard B as a two-signal scan: + +- **OperationRun signal**: file contains any of: `use App\\Models\\OperationRun;`, `OperationRun`, `$this->operationRun`, `$operationRun`. +- **DB emission signal**: file contains either `sendToDatabase(` or `->notify(`. +- **Allowlist**: only `app/Services/OperationRunService.php` and `app/Notifications/OperationRunCompleted.php`. + +**Rationale**: + +- Enforces the constitution: completion notifications are centralized, queued/running DB notifications are banned. +- Intentionally errs on the side of flagging new ad-hoc notification producers. + +**Alternatives considered**: + +- “DB emission only” = `sendToDatabase(` and ignore `->notify(`: lower noise, but would miss notify-based DB notifications. +- Restrict scan to `app/Jobs/**` and `app/Services/**`: lower noise, but spec requires `app/**/*.php` for defense-in-depth. + +**Repo evidence (high-level)**: + +- `OperationRunService` currently emits both `OperationRunCompleted` and `OperationRunQueued` via `->notify(...)`. +- There are multiple job files with `->sendToDatabase(...)` in code paths that also handle an OperationRun. +- There are Filament resource actions that both dispatch jobs with `$operationRun` and call `->sendToDatabase(...)` for queued feedback. These are queued DB notifications and conflict with FR-004. + +## Decision 4 — Ban queued DB notifications by changing service defaults + +**Decision**: Change `OperationRunService::dispatchOrFail(..., bool $emitQueuedNotification = true)` (and any upstream helper that takes `emitQueuedNotification`) so the default is `false`, and ensure no call site opts back into queued DB notifications. + +**Rationale**: + +- Matches FR-004 / FR-012b: queued/running DB notifications are forbidden repo-wide. +- Prevents new call sites from accidentally enabling queued DB notifications by omission. + +**Alternatives considered**: + +- Delete `OperationRunQueued` entirely: higher churn; leaving the class unused is acceptable as long as it’s never emitted. +- Keep default `true` but require opt-out everywhere: violates FR-012b and is easy to regress. + +## Decision 5 — “Exactly once terminal notification” relies on `previousStatus` guard + +**Decision**: Rely on `OperationRunService::updateRun()` behavior that emits `OperationRunCompleted` only when transitioning into `Completed` from a non-completed status. + +**Rationale**: + +- Centralizes “exactly once” semantics in a single place. +- Keeps jobs/services free of notification-specific logic. + +**Alternatives considered**: + +- Add additional DB-level dedupe: unnecessary if the service is the only producer and transitions are canonical. diff --git a/specs/110-ops-ux-enforcement/spec.md b/specs/110-ops-ux-enforcement/spec.md index fbc6e9e..999d14f 100644 --- a/specs/110-ops-ux-enforcement/spec.md +++ b/specs/110-ops-ux-enforcement/spec.md @@ -5,10 +5,18 @@ # Feature Specification: Ops-UX Enforcement & Cleanup (Enterprise Standard Rollo **Status**: Draft **Depends On**: `specs/055-ops-ux-rollout/spec.md` +## Clarifications + +### Session 2026-02-23 + +- Q: For Guard A, what detection strategy should we use to catch forbidden status/outcome transitions while allowing context-only updates? → A: Scan `app/` for `->update([...])` blocks that include `status` or `outcome` keys (same call block, multi-line), excluding `app/Services/OperationRunService.php`. +- Q: Should queued/running DB notifications be allowed (e.g., `OperationRunQueued`) or is the DB notification surface terminal-only? → A: Ban queued/running DB notifications entirely (including `OperationRunQueued`); the only DB notification for operations is the terminal `OperationRunCompleted` notification. +- Q: For Guard B (DB-notification guard), what scanning scope/heuristic should we use? → A: Scan `app/**/*.php` and fail when a file both references an OperationRun (import or variable/property usage) and emits any database notification (`sendToDatabase(` or `->notify(`), except for an explicit allowlist. + ## Spec Scope Fields *(mandatory)* - **Scope**: tenant (all operation-run-producing flows within a tenant) -- **Primary Routes**: No new routes. Affected internal flows: Inventory Sync, Backup Schedule Retention, Backup Schedule Run, Bulk Policy Export, Bulk Restore Run Force Delete, Add/Remove Policies to Backup Set, Restore Run execution. +- **Primary Routes**: No new routes. Affected internal flows: Inventory Sync, Backup Schedule Retention, Backup Schedule Run, Bulk Policy Export, Bulk Restore Run Restore, Bulk Restore Run Force Delete, Bulk Policy Unignore, Entra Group manual sync, Add/Remove Policies to Backup Set, Restore Run execution. - **Data Ownership**: `operation_runs` (tenant-scoped), `notifications` (tenant user-scoped). No schema changes required. - **RBAC**: No new RBAC surfaces. Existing capability gates on triggering operations remain unchanged. Notification delivery is limited to the initiator user. System/scheduled runs with no initiator receive no DB notification. @@ -104,7 +112,18 @@ ## Requirements *(mandatory)* **Constitution alignment — OPS-UX-001**: This spec enforces the Ops-UX 3-surface contract. All status/outcome transitions must be routed through `OperationRunService`. Terminal DB notifications are sent exclusively via `OperationRunCompleted` from within the service. No job or feature code may send its own completion DB notification. -**Constitution alignment — No new Filament screens**: This spec makes no changes to Filament Resources, RelationManagers, or Pages (beyond the optional P2 presenter helper). The UI Action Matrix is not required. +**Constitution alignment — Filament scope**: This spec introduces **no new Filament screens**. It DOES include targeted updates to existing Filament start surfaces (Resources/Pages) to remove queued/running DB notifications and ensure start-surface feedback is toast-only. + +**UI Action Matrix (mandatory)** + +| Surface | User action | Authorization | Start surface feedback | Progress surface | Terminal surface | +|---------|-------------|---------------|------------------------|------------------|-----------------| +| `PolicyResource` (tenant context) | Trigger an operation that creates/reuses an OperationRun | Existing capability checks (unchanged) | Toast-only (no DB notification) + optional “View run” link | Monitoring → Operations + run detail | Exactly one `OperationRunCompleted` DB notification to initiator (service-owned) | +| `PolicyVersionResource` (tenant context) | Trigger an operation that creates/reuses an OperationRun | Existing capability checks (unchanged) | Toast-only (no DB notification) + optional “View run” link | Monitoring → Operations + run detail | Exactly one `OperationRunCompleted` DB notification to initiator (service-owned) | +| `BackupScheduleResource` (tenant context) | Run / queue backup schedule operation | Existing capability checks (unchanged) | Toast-only (no DB notification) + optional “View run” link | Monitoring → Operations + run detail | Exactly one `OperationRunCompleted` DB notification to initiator (service-owned) | +| `TenantResource` (tenant context) | Trigger an operation that creates/reuses an OperationRun | Existing capability checks (unchanged) | Toast-only (no DB notification) + optional “View run” link | Monitoring → Operations + run detail | Exactly one `OperationRunCompleted` DB notification to initiator (service-owned) | +| `EntraGroupResource` → `ListEntraGroups` (tenant context) | Start manual sync operation | Existing capability checks (unchanged) | Toast-only (no DB notification) + optional “View run” link | Monitoring → Operations + run detail | Exactly one `OperationRunCompleted` DB notification to initiator (service-owned) | +| Scheduled/system operations | Jobs run without an initiator | Existing schedule/lock semantics (unchanged) | N/A | Monitoring → Operations + run detail | No DB notification (initiator is null); Monitoring remains canonical | **Constitution alignment — No new Graph calls / OperationRun types**: This spec modifies existing operation flows only; it does not introduce new operation types or Graph calls. @@ -115,18 +134,20 @@ ### Functional Requirements - **FR-001**: All OperationRun `status` and `outcome` field transitions MUST go through `OperationRunService` canonical transition methods. Direct `$operationRun->update(['status' => ...])`, `$operationRun->status = ...`, `$operationRun->outcome = ...`, or bulk query updates on status/outcome are forbidden outside `OperationRunService`. - **FR-002**: `OperationRunService` MUST emit exactly one `OperationRunCompleted` DB notification to the initiator when transitioning a run to a terminal outcome (when an initiator exists). - **FR-003**: No job or service code outside `OperationRunService` MAY emit a DB notification representing operation completion, abort, or terminal state. -- **FR-004**: No code anywhere MAY emit a DB notification for queued or running operation states. +- **FR-004**: No code anywhere MAY emit a DB notification for queued or running operation states. This includes `OperationRunQueued` and any other “queued” / “started” / “running” database notifications. - **FR-005**: `RunStatusChangedNotification` MUST be deleted. No references to it may remain in `app/` or `tests/`. - **FR-006**: `InventorySyncService` MUST transition to terminal state exclusively via `OperationRunService`, not via direct model updates. - **FR-007**: `ApplyBackupScheduleRetentionJob` MUST transition to terminal state exclusively via `OperationRunService`. - **FR-008**: `TenantpilotBackfillWorkspaceIds` console command MUST use the canonical transition if it transitions OperationRun status (initiator may be null; no DB notification emitted in that case). - **FR-009**: `RunBackupScheduleJob` MUST NOT emit any queued or completion DB notifications; outcome notification is handled by `OperationRunService` terminal transition. -- **FR-010**: `BulkPolicyExportJob`, `BulkRestoreRunForceDeleteJob`, `AddPoliciesToBackupSetJob`, and `RemovePoliciesFromBackupSetJob` MUST NOT call `sendToDatabase()` for operation completion or abort feedback. +- **FR-010**: `BulkPolicyExportJob`, `BulkRestoreRunForceDeleteJob`, `BulkRestoreRunRestoreJob`, `BulkPolicyUnignoreJob`, `AddPoliciesToBackupSetJob`, and `RemovePoliciesFromBackupSetJob` MUST NOT call `sendToDatabase()` for operation completion, abort, or queued/running feedback. +- **FR-010b**: Filament start surfaces that initiate operation-run-producing flows MUST NOT persist queued/running DB notifications (including any `sendToDatabase()` “queued” notifications). Start feedback is toast-only. - **FR-011**: Context-only updates (e.g., updating `context`, `message`, `reason_code` fields without touching `status` or `outcome`) are permitted directly on the model outside `OperationRunService`. - **FR-012**: Three Pest guard tests MUST exist and pass in CI: - - Guard A: Detects direct status/outcome transitions outside `OperationRunService`; reports file + snippet. - - Guard B: Detects `sendToDatabase()` calls in operation-flow jobs that also reference OperationRun; reports file path. + - Guard A: Detects direct status/outcome transitions outside `OperationRunService`; reports file + snippet. Implementation MUST scan `app/**/*.php` for `->update(` calls whose update array includes a `status` and/or `outcome` key (multi-line block match allowed), excluding `app/Services/OperationRunService.php`. Context-only updates without `status`/`outcome` MUST NOT fail this guard. + - Guard B: Detects DB-notification emissions in operation-flow code; reports file path. Implementation MUST scan `app/**/*.php` and fail when a file contains BOTH (a) an OperationRun signal (`use App\\Models\\OperationRun;` OR `OperationRun` token OR `$this->operationRun` OR `$operationRun`) AND (b) a DB-notification emission (`sendToDatabase(` OR `->notify(`). Allowed exceptions (explicit allowlist): `app/Services/OperationRunService.php`, `app/Notifications/OperationRunCompleted.php`. - Guard C: Detects any reference to `RunStatusChangedNotification` in `app/` or `tests/`. +- **FR-012b**: Operation enqueue helpers MUST NOT emit queued DB notifications by default. Any helper param like `emitQueuedNotification` MUST default to `false`, and all current call sites MUST comply. - **FR-013** *(P2)*: `OperationUxPresenter` MUST expose an `alreadyQueuedToast(...)` static helper returning canonical copy + duration (+ optional "View run" action). - **FR-014** *(P2)*: `BackupSetPolicyPickerTable` dedup toast MUST use `OperationUxPresenter::alreadyQueuedToast(...)`. @@ -147,9 +168,21 @@ ### Job-level DB notifications (duplicates / queued spam) | `app/Jobs/RunBackupScheduleJob.php` | Queued DB notification + custom finished notification | P0 | | `app/Jobs/BulkPolicyExportJob.php` | Multiple `sendToDatabase()` paths | P1 | | `app/Jobs/BulkRestoreRunForceDeleteJob.php` | Multiple `sendToDatabase()` paths | P1 | +| `app/Jobs/BulkRestoreRunRestoreJob.php` | Multiple `sendToDatabase()` paths | P1 | +| `app/Jobs/BulkPolicyUnignoreJob.php` | Multiple `sendToDatabase()` paths | P1 | | `app/Jobs/AddPoliciesToBackupSetJob.php` | Custom completion DB notifications | P1 | | `app/Jobs/RemovePoliciesFromBackupSetJob.php` | Custom completion DB notifications | P1 | +### Start-surface queued DB notifications (forbidden) + +| File | Violation | Priority | +|------|-----------|----------| +| `app/Filament/Resources/PolicyResource.php` | Queued/running DB notification emitted from a start surface | P1 | +| `app/Filament/Resources/PolicyVersionResource.php` | Queued/running DB notification emitted from a start surface | P1 | +| `app/Filament/Resources/BackupScheduleResource.php` | Queued/running DB notification emitted from a start surface | P1 | +| `app/Filament/Resources/TenantResource.php` | Queued/running DB notification emitted from a start surface | P1 | +| `app/Filament/Resources/EntraGroupResource/Pages/ListEntraGroups.php` | Queued/running DB notification emitted from a start surface | P1 | + ### Legacy notification outside system | File | Violation | Priority | @@ -165,40 +198,9 @@ ### Optional polish (P2) --- -## Tasks +## Execution Plan Reference -### Phase 1 — P0: Fix silent completions - -- [ ] T110-001 (P0) `InventorySyncService` — replace direct terminal `update([status/outcome])` with `OperationRunService` canonical transition method -- [ ] T110-002 (P0) `ApplyBackupScheduleRetentionJob` — replace direct terminal `update([...])` with canonical transition method -- [ ] T110-003 (P1) `TenantpilotBackfillWorkspaceIds` — replace direct status update with canonical transition method (initiator may be null) - -### Phase 2 — P0: Remove legacy notification - -- [ ] T110-010 (P0) `ExecuteRestoreRunJob` — remove `RunStatusChangedNotification` invocation; rely on service terminal notification -- [ ] T110-011 (P0) Delete `RunStatusChangedNotification` class; assert zero references remain - -### Phase 3 — P0/P1: Remove job-level DB notifications - -- [ ] T110-020 (P0) `RunBackupScheduleJob` — remove queued DB notification producer -- [ ] T110-021 (P0) `RunBackupScheduleJob` — remove custom finished DB notification producer -- [ ] T110-022 (P1) `BulkPolicyExportJob` — remove all `sendToDatabase()` branches for completion/abort -- [ ] T110-023 (P1) `BulkRestoreRunForceDeleteJob` — remove all completion/abort `sendToDatabase()` branches -- [ ] T110-024 (P1) `AddPoliciesToBackupSetJob` — remove custom completion/failure DB notifications -- [ ] T110-025 (P1) `RemovePoliciesFromBackupSetJob` — remove custom completion/failure DB notifications - -### Phase 4 — Guards (mandatory) - -- [ ] T110-030 (P0) Pest guard: No direct OperationRun status/outcome transitions outside OperationRunService (file + snippet report; context-only updates allowed) -- [ ] T110-031 (P0) Pest guard: Jobs must not emit `sendToDatabase()` in operation flows (allowlist escape hatch if needed, minimal) -- [ ] T110-032 (P0) Pest guard: No references to `RunStatusChangedNotification` in `app/` or `tests/` - -### Phase 5 — Optional polish (P2) - -- [ ] T110-040 (P2) Add `OperationUxPresenter::alreadyQueuedToast(...)` canonical helper -- [ ] T110-041 (P2) Migrate `BackupSetPolicyPickerTable` dedup toast to `alreadyQueuedToast` - ---- +The executable work breakdown for this spec lives in [specs/110-ops-ux-enforcement/tasks.md](specs/110-ops-ux-enforcement/tasks.md). The spec intentionally avoids duplicating a task list to prevent drift. ## Testing Plan (Pest) diff --git a/specs/110-ops-ux-enforcement/tasks.md b/specs/110-ops-ux-enforcement/tasks.md new file mode 100644 index 0000000..5dcc267 --- /dev/null +++ b/specs/110-ops-ux-enforcement/tasks.md @@ -0,0 +1,207 @@ +--- + +description: "Executable task list for Ops-UX Enforcement & Cleanup" + +--- + +# Tasks: Ops-UX Enforcement & Cleanup (Enterprise Standard Rollout) + +**Input**: Design documents from `/specs/110-ops-ux-enforcement/` + +- plan.md: [specs/110-ops-ux-enforcement/plan.md](specs/110-ops-ux-enforcement/plan.md) +- spec.md: [specs/110-ops-ux-enforcement/spec.md](specs/110-ops-ux-enforcement/spec.md) +- research.md: [specs/110-ops-ux-enforcement/research.md](specs/110-ops-ux-enforcement/research.md) +- data-model.md: [specs/110-ops-ux-enforcement/data-model.md](specs/110-ops-ux-enforcement/data-model.md) +- contracts/: [specs/110-ops-ux-enforcement/contracts/](specs/110-ops-ux-enforcement/contracts/) + +**Tests**: REQUIRED (Pest) — both guard tests and focused regressions. + +## Phase 1: Setup (Shared Test Infrastructure) + +**Purpose**: Create minimal shared helpers for guard tests and keep failure output consistent. + +- [ ] T001 [P] Add source scanning helper in tests/Support/OpsUx/SourceFileScanner.php + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: Cross-cutting invariants that must be true before user story work can be considered “done”. + +- [ ] T002 Update queued notification defaults in app/Services/OperationRunService.php (dispatchOrFail + enqueue helpers default emitQueuedNotification=false) +- [ ] T003 Confirm no call sites opt into queued DB notifications in app/Services/OperationRunService.php (remove/forbid emitQueuedNotification:true usage) + +**Checkpoint**: No queued/running DB notifications can be emitted by default. + +--- + +## Phase 3: User Story 1 — No Silent Completions (Priority: P1) 🎯 MVP + +**Goal**: Terminal transitions always go through `OperationRunService`, producing exactly one terminal `OperationRunCompleted` notification for initiators. + +**Independent Test**: Inventory sync + retention flow transitions to terminal and persists exactly one `OperationRunCompleted` notification for the initiator; system runs persist none. + +### Tests (write first) + +- [ ] T004 [P] [US1] Add inventory sync terminal notification regression test in tests/Feature/OpsUx/Regression/InventorySyncTerminalNotificationTest.php +- [ ] T005 [P] [US1] Add retention terminal notification regression test in tests/Feature/OpsUx/Regression/BackupRetentionTerminalNotificationTest.php + +### Implementation + +- [ ] T006 [US1] Refactor terminal transition in app/Services/Inventory/InventorySyncService.php to use OperationRunService::updateRun() +- [ ] T007 [US1] Refactor terminal transition in app/Jobs/ApplyBackupScheduleRetentionJob.php to use OperationRunService::updateRun() +- [ ] T008 [US1] Refactor OperationRun status/outcome update in app/Console/Commands/TenantpilotBackfillWorkspaceIds.php to use OperationRunService::updateRun() (initiator may be null) + +**Checkpoint**: US1 regressions pass, with no silent completions. + +--- + +## Phase 4: User Story 2 — No Notification Spam (Priority: P1) + +**Goal**: Remove job-level queued/completion DB notifications and eliminate queued DB notifications from start surfaces. + +**Independent Test**: Backup schedule run + representative bulk flow complete with exactly one terminal `OperationRunCompleted` and zero queued/running DB notifications. + +### Tests (write first) + +- [ ] T009 [P] [US2] Add backup schedule run notification regression test in tests/Feature/OpsUx/Regression/BackupScheduleRunNotificationTest.php +- [ ] T010 [P] [US2] Add bulk job “abort/circuit-break” regression test in tests/Feature/OpsUx/Regression/BulkJobCircuitBreakerTest.php + +### Implementation (Jobs) + +- [ ] T011 [P] [US2] Remove queued + custom finished DB notifications in app/Jobs/RunBackupScheduleJob.php +- [ ] T012 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkPolicyExportJob.php +- [ ] T013 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkRestoreRunForceDeleteJob.php +- [ ] T029 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkRestoreRunRestoreJob.php +- [ ] T030 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkPolicyUnignoreJob.php +- [ ] T014 [P] [US2] Remove custom completion/failure DB notifications in app/Jobs/AddPoliciesToBackupSetJob.php +- [ ] T015 [P] [US2] Remove custom completion/failure DB notifications in app/Jobs/RemovePoliciesFromBackupSetJob.php + +### Implementation (Start surfaces / Filament) + +- [ ] T016 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/PolicyResource.php (remove sendToDatabase for queued ops) +- [ ] T017 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/BackupScheduleResource.php (remove sendToDatabase for queued ops) +- [ ] T018 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/TenantResource.php (remove sendToDatabase for queued ops) +- [ ] T019 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/PolicyVersionResource.php (remove sendToDatabase for queued ops) +- [ ] T031 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/EntraGroupResource/Pages/ListEntraGroups.php (remove sendToDatabase for queued ops) + +**Checkpoint**: US2 regressions pass and notifications remain terminal-only. + +--- + +## Phase 5: User Story 3 — Legacy Notification Removed (Priority: P1) + +**Goal**: Remove the out-of-system `RunStatusChangedNotification` and rely exclusively on canonical terminal `OperationRunCompleted`. + +**Independent Test**: Restore run completion produces exactly one `OperationRunCompleted` notification and there are zero references to `RunStatusChangedNotification` in `app/` and `tests/`. + +### Tests (write first) + +- [ ] T020 [P] [US3] Add restore run terminal notification regression test in tests/Feature/OpsUx/Regression/RestoreRunTerminalNotificationTest.php + +### Implementation + +- [ ] T021 [US3] Remove legacy notification invocation in app/Jobs/ExecuteRestoreRunJob.php +- [ ] T022 [US3] Delete legacy notification class app/Notifications/RunStatusChangedNotification.php + +**Checkpoint**: US3 regression passes; no legacy notification remains. + +--- + +## Phase 6: User Story 4 — Regression Guards Enforce the Constitution (Priority: P1) + +**Goal**: CI guard tests fail fast when forbidden patterns reappear. + +**Independent Test**: Guards fail with actionable output on a synthetic violation and pass on a clean codebase. + +### Guard tests + +- [ ] T023 [P] [US4] Implement Guard A in tests/Feature/OpsUx/Constitution/DirectStatusTransitionGuardTest.php (scan app/** for ->update([...]) containing status/outcome; exclude app/Services/OperationRunService.php; print snippet) +- [ ] T024 [P] [US4] Implement Guard B in tests/Feature/OpsUx/Constitution/JobDbNotificationGuardTest.php (scan app/** for OperationRun signal + DB notify emission; allowlist app/Services/OperationRunService.php and app/Notifications/OperationRunCompleted.php) +- [ ] T025 [P] [US4] Implement Guard C in tests/Feature/OpsUx/Constitution/LegacyNotificationGuardTest.php (scan app/** and tests/** for RunStatusChangedNotification) + +**Checkpoint**: Guard tests pass green and provide clear failure output. + +--- + +## Phase 7: User Story 5 — Canonical "Already Queued" Toast (Priority: P2) + +**Goal**: Dedup “already queued” messaging is canonical and consistent. + +**Independent Test**: Trigger dedup path and confirm toast uses `OperationUxPresenter::alreadyQueuedToast(...)`. + +- [ ] T026 [P] [US5] Add OperationUxPresenter::alreadyQueuedToast(...) helper in app/Support/OpsUx/OperationUxPresenter.php +- [ ] T027 [US5] Migrate dedup toast to canonical helper in app/Livewire/BackupSetPolicyPickerTable.php + +--- + +## Phase 8: Polish & Cross-Cutting Concerns + +**Purpose**: Keep docs and developer workflow aligned with final test locations. + +- [ ] T028 [P] Update quickstart commands/paths if needed in specs/110-ops-ux-enforcement/quickstart.md + +--- + +## Dependencies & Execution Order + +### User Story Completion Order + +- US1 → US2 → US3 → US4 → US5 + +Rationale: + +- US1–US3 remove known violations so the guard suite (US4) can pass on a clean codebase. +- US5 is optional polish and can land after enforcement is stable. + +### Dependency Graph + +```text +Phase 1 (Setup) ─┬─> Phase 2 (Foundational) ─┬─> US1 ─┬─> US2 ─┬─> US3 ─┬─> US4 ─┬─> US5 + │ │ │ │ │ └─> Polish + └───────────────────────────┴────────┴────────┴────────┴─────────────── +``` + +## Parallel Execution Examples + +### US1 (tests + implementation) + +```bash +T004: tests/Feature/OpsUx/Regression/InventorySyncTerminalNotificationTest.php +T005: tests/Feature/OpsUx/Regression/BackupRetentionTerminalNotificationTest.php +``` + +### US2 (jobs + start surfaces) + +```bash +T011: app/Jobs/RunBackupScheduleJob.php +T012: app/Jobs/BulkPolicyExportJob.php +T013: app/Jobs/BulkRestoreRunForceDeleteJob.php +T014: app/Jobs/AddPoliciesToBackupSetJob.php +T015: app/Jobs/RemovePoliciesFromBackupSetJob.php +T016–T019: app/Filament/Resources/*Resource.php +``` + +### US4 (guards) + +```bash +T023: tests/Feature/OpsUx/Constitution/DirectStatusTransitionGuardTest.php +T024: tests/Feature/OpsUx/Constitution/JobDbNotificationGuardTest.php +T025: tests/Feature/OpsUx/Constitution/LegacyNotificationGuardTest.php +``` + +## Implementation Strategy + +### MVP Scope (recommended) + +- Foundational (Phase 2) + US1 only. + +This yields the highest-value guarantee quickly: “no silent completions” and exactly-once terminal notifications for the most critical flows. + +### Incremental Delivery + +1. Foundational → US1 → validate +2. US2 → validate +3. US3 → validate +4. US4 guards → enforce +5. US5 polish -- 2.45.2 From 29112225b678c9abebf1a802b93dfd924143d6af Mon Sep 17 00:00:00 2001 From: Ahmed Darrazi Date: Mon, 23 Feb 2026 22:51:46 +0100 Subject: [PATCH 3/4] docs(constitution): codify Ops-UX non-negotiables (3-surface + lifecycle + guards) --- .specify/memory/constitution.md | 78 ++++++++++++++++++++++++++-- .specify/templates/plan-template.md | 5 ++ .specify/templates/spec-template.md | 7 +++ .specify/templates/tasks-template.md | 7 +++ 4 files changed, 93 insertions(+), 4 deletions(-) diff --git a/.specify/memory/constitution.md b/.specify/memory/constitution.md index 3b66104..b8d5ecb 100644 --- a/.specify/memory/constitution.md +++ b/.specify/memory/constitution.md @@ -1,11 +1,15 @@ - **Language/Version**: PHP 8.4.x **Primary Dependencies**: Laravel 12, Filament v5, Livewire v4 **Storage**: PostgreSQL (Sail) @@ -37,7 +31,7 @@ ## Technical Context - No queued/running DB notifications anywhere (including `OperationRunQueued`). - Existing RBAC/capability gates for starting operations remain unchanged. -**Scale/Scope**: Repo-wide enforcement via guard tests; remediation limited to enumerated violating flows in the spec. +**Scale/Scope**: Tenant-runtime remediation for enumerated violating flows + repo-wide enforcement via guard tests. ## Constitution Check @@ -69,13 +63,6 @@ ### Documentation (this feature) ``` ### Source Code (repository root) - - ```text app/ ├── Jobs/ @@ -110,8 +97,7 @@ ## Complexity Tracking | Violation | Why Needed | Simpler Alternative Rejected Because | |-----------|------------|-------------------------------------| -| [e.g., 4th project] | [current need] | [why 3 projects insufficient] | -| [e.g., Repository pattern] | [specific problem] | [why direct DB access insufficient] | +| None | N/A | N/A | ## Phase 0 — Outline & Research diff --git a/specs/110-ops-ux-enforcement/quickstart.md b/specs/110-ops-ux-enforcement/quickstart.md index 766eb29..bccb50a 100644 --- a/specs/110-ops-ux-enforcement/quickstart.md +++ b/specs/110-ops-ux-enforcement/quickstart.md @@ -3,21 +3,22 @@ # Quickstart: Ops-UX Enforcement & Cleanup ## Prereqs - Sail running: `vendor/bin/sail up -d` +- Sail running: `./vendor/bin/sail up -d` ## Run the focused guard tests Once implemented, run the guard tests only: -- `vendor/bin/sail artisan test --compact --filter=OpsUx` - -If the tests are split by directory as in the spec: - -- `vendor/bin/sail artisan test --compact tests/Feature/OpsUx/Constitution` +- `./vendor/bin/sail artisan test --compact tests/Feature/OpsUx/Constitution` ## Run the focused regression tests -- `vendor/bin/sail artisan test --compact tests/Feature/OpsUx/Regression` +- `./vendor/bin/sail artisan test --compact tests/Feature/OpsUx/Regression` + +## Run the combined Ops-UX pack + +- `./vendor/bin/sail artisan test --compact --group=ops-ux` ## Format touched files -- `vendor/bin/sail bin pint --dirty --format agent` +- `./vendor/bin/sail bin pint --dirty --format agent` diff --git a/specs/110-ops-ux-enforcement/spec.md b/specs/110-ops-ux-enforcement/spec.md index 999d14f..0835978 100644 --- a/specs/110-ops-ux-enforcement/spec.md +++ b/specs/110-ops-ux-enforcement/spec.md @@ -15,7 +15,7 @@ ### Session 2026-02-23 ## Spec Scope Fields *(mandatory)* -- **Scope**: tenant (all operation-run-producing flows within a tenant) +- **Scope**: tenant runtime remediation + repo-wide enforcement guards (CI/static scan) - **Primary Routes**: No new routes. Affected internal flows: Inventory Sync, Backup Schedule Retention, Backup Schedule Run, Bulk Policy Export, Bulk Restore Run Restore, Bulk Restore Run Force Delete, Bulk Policy Unignore, Entra Group manual sync, Add/Remove Policies to Backup Set, Restore Run execution. - **Data Ownership**: `operation_runs` (tenant-scoped), `notifications` (tenant user-scoped). No schema changes required. - **RBAC**: No new RBAC surfaces. Existing capability gates on triggering operations remain unchanged. Notification delivery is limited to the initiator user. System/scheduled runs with no initiator receive no DB notification. @@ -144,7 +144,11 @@ ### Functional Requirements - **FR-010b**: Filament start surfaces that initiate operation-run-producing flows MUST NOT persist queued/running DB notifications (including any `sendToDatabase()` “queued” notifications). Start feedback is toast-only. - **FR-011**: Context-only updates (e.g., updating `context`, `message`, `reason_code` fields without touching `status` or `outcome`) are permitted directly on the model outside `OperationRunService`. - **FR-012**: Three Pest guard tests MUST exist and pass in CI: - - Guard A: Detects direct status/outcome transitions outside `OperationRunService`; reports file + snippet. Implementation MUST scan `app/**/*.php` for `->update(` calls whose update array includes a `status` and/or `outcome` key (multi-line block match allowed), excluding `app/Services/OperationRunService.php`. Context-only updates without `status`/`outcome` MUST NOT fail this guard. + - Guard A: Detects direct status/outcome transitions outside `OperationRunService`; reports file + snippet. Implementation MUST scan `app/**/*.php` for forbidden transition patterns, excluding `app/Services/OperationRunService.php`, including: + - `->update(` calls whose update array includes a `status` and/or `outcome` key (multi-line block match allowed) + - direct assignments to `->status` / `->outcome` + - query/builder/bulk `update([...])` calls that set `status` and/or `outcome` + Context-only updates without `status`/`outcome` MUST NOT fail this guard. - Guard B: Detects DB-notification emissions in operation-flow code; reports file path. Implementation MUST scan `app/**/*.php` and fail when a file contains BOTH (a) an OperationRun signal (`use App\\Models\\OperationRun;` OR `OperationRun` token OR `$this->operationRun` OR `$operationRun`) AND (b) a DB-notification emission (`sendToDatabase(` OR `->notify(`). Allowed exceptions (explicit allowlist): `app/Services/OperationRunService.php`, `app/Notifications/OperationRunCompleted.php`. - Guard C: Detects any reference to `RunStatusChangedNotification` in `app/` or `tests/`. - **FR-012b**: Operation enqueue helpers MUST NOT emit queued DB notifications by default. Any helper param like `emitQueuedNotification` MUST default to `false`, and all current call sites MUST comply. @@ -252,17 +256,19 @@ ## Assumptions 2. The `failed()` job lifecycle callback or try/finally blocks in affected jobs already exist (or will be added) to ensure terminal transitions even on unhandled exceptions — confirmed during implementation per-file. 3. Guard tests are static analysis (filesystem grep-based) Pest tests, not runtime tests. They do not require a running application. 4. The allowlist for Guard B (job DB notifications) is intentionally minimal. Any new entry requires justification in a spec comment. -5. P2 tasks (T110-040/041) are optional and do not gate release of P0/P1 work. +5. P2 tasks (`T026`/`T027`) are optional and do not gate release of P0/P1 work. --- ## Rollout / PR Slicing -| PR | Tasks | Priority | -|----|-------|----------| -| PR-A | T110-001, T110-002 + regression tests (inventory sync, retention) | P0 | -| PR-B | T110-010, T110-011 + restore run tests | P0 | -| PR-C | T110-020, T110-021 + backup schedule tests | P0 | -| PR-D | T110-022–T110-025 + bulk job tests | P1 | -| PR-E | T110-030, T110-031, T110-032 (guards — can land early) | P0 | -| PR-F | T110-040, T110-041 (optional polish) | P2 | +PR slicing is phase/story-based. `tasks.md` remains the source of truth for exact task IDs and sequencing. + +| PR | Scope Slice | Priority | +|----|-------------|----------| +| PR-A | Phase 1–3 (Setup + Foundational + US1 “No Silent Completions”) | P0 | +| PR-B | Phase 4 (US2 “No Notification Spam” cleanup: jobs + start surfaces) | P0/P1 | +| PR-C | Phase 5 (US3 legacy notification removal) | P0 | +| PR-D | Phase 6 (US4 constitution guard tests A/B/C) | P0 | +| PR-E | Phase 7 (US5 canonical “already queued” toast polish) | P2 | +| PR-F | Phase 8 (docs alignment + validation execution) | P1/P2 | diff --git a/specs/110-ops-ux-enforcement/tasks.md b/specs/110-ops-ux-enforcement/tasks.md index 5dcc267..f45630c 100644 --- a/specs/110-ops-ux-enforcement/tasks.md +++ b/specs/110-ops-ux-enforcement/tasks.md @@ -20,7 +20,7 @@ ## Phase 1: Setup (Shared Test Infrastructure) **Purpose**: Create minimal shared helpers for guard tests and keep failure output consistent. -- [ ] T001 [P] Add source scanning helper in tests/Support/OpsUx/SourceFileScanner.php +- [X] T001 [P] Add source scanning helper in tests/Support/OpsUx/SourceFileScanner.php --- @@ -28,8 +28,8 @@ ## Phase 2: Foundational (Blocking Prerequisites) **Purpose**: Cross-cutting invariants that must be true before user story work can be considered “done”. -- [ ] T002 Update queued notification defaults in app/Services/OperationRunService.php (dispatchOrFail + enqueue helpers default emitQueuedNotification=false) -- [ ] T003 Confirm no call sites opt into queued DB notifications in app/Services/OperationRunService.php (remove/forbid emitQueuedNotification:true usage) +- [X] T002 Update queued notification defaults in app/Services/OperationRunService.php (dispatchOrFail + enqueue helpers default emitQueuedNotification=false) +- [X] T003 Confirm repo-wide call sites do not opt into queued DB notifications (remove/forbid `emitQueuedNotification: true` usages in `app/**`) **Checkpoint**: No queued/running DB notifications can be emitted by default. @@ -43,14 +43,14 @@ ## Phase 3: User Story 1 — No Silent Completions (Priority: P1) 🎯 MVP ### Tests (write first) -- [ ] T004 [P] [US1] Add inventory sync terminal notification regression test in tests/Feature/OpsUx/Regression/InventorySyncTerminalNotificationTest.php -- [ ] T005 [P] [US1] Add retention terminal notification regression test in tests/Feature/OpsUx/Regression/BackupRetentionTerminalNotificationTest.php +- [X] T004 [P] [US1] Add inventory sync terminal notification regression test in tests/Feature/OpsUx/Regression/InventorySyncTerminalNotificationTest.php +- [X] T005 [P] [US1] Add retention terminal notification regression test in tests/Feature/OpsUx/Regression/BackupRetentionTerminalNotificationTest.php ### Implementation -- [ ] T006 [US1] Refactor terminal transition in app/Services/Inventory/InventorySyncService.php to use OperationRunService::updateRun() -- [ ] T007 [US1] Refactor terminal transition in app/Jobs/ApplyBackupScheduleRetentionJob.php to use OperationRunService::updateRun() -- [ ] T008 [US1] Refactor OperationRun status/outcome update in app/Console/Commands/TenantpilotBackfillWorkspaceIds.php to use OperationRunService::updateRun() (initiator may be null) +- [X] T006 [US1] Refactor terminal transition in app/Services/Inventory/InventorySyncService.php to use OperationRunService::updateRun() +- [X] T007 [US1] Refactor terminal transition in app/Jobs/ApplyBackupScheduleRetentionJob.php to use OperationRunService::updateRun() +- [X] T008 [US1] Refactor OperationRun status/outcome update in app/Console/Commands/TenantpilotBackfillWorkspaceIds.php to use OperationRunService::updateRun() (initiator may be null) **Checkpoint**: US1 regressions pass, with no silent completions. @@ -64,26 +64,26 @@ ## Phase 4: User Story 2 — No Notification Spam (Priority: P1) ### Tests (write first) -- [ ] T009 [P] [US2] Add backup schedule run notification regression test in tests/Feature/OpsUx/Regression/BackupScheduleRunNotificationTest.php -- [ ] T010 [P] [US2] Add bulk job “abort/circuit-break” regression test in tests/Feature/OpsUx/Regression/BulkJobCircuitBreakerTest.php +- [X] T009 [P] [US2] Add backup schedule run notification regression test in tests/Feature/OpsUx/Regression/BackupScheduleRunNotificationTest.php +- [X] T010 [P] [US2] Add bulk job “abort/circuit-break” regression test in tests/Feature/OpsUx/Regression/BulkJobCircuitBreakerTest.php ### Implementation (Jobs) -- [ ] T011 [P] [US2] Remove queued + custom finished DB notifications in app/Jobs/RunBackupScheduleJob.php -- [ ] T012 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkPolicyExportJob.php -- [ ] T013 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkRestoreRunForceDeleteJob.php -- [ ] T029 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkRestoreRunRestoreJob.php -- [ ] T030 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkPolicyUnignoreJob.php -- [ ] T014 [P] [US2] Remove custom completion/failure DB notifications in app/Jobs/AddPoliciesToBackupSetJob.php -- [ ] T015 [P] [US2] Remove custom completion/failure DB notifications in app/Jobs/RemovePoliciesFromBackupSetJob.php +- [X] T011 [P] [US2] Remove queued + custom finished DB notifications in app/Jobs/RunBackupScheduleJob.php +- [X] T012 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkPolicyExportJob.php +- [X] T013 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkRestoreRunForceDeleteJob.php +- [X] T029 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkRestoreRunRestoreJob.php +- [X] T030 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkPolicyUnignoreJob.php +- [X] T014 [P] [US2] Remove custom completion/failure DB notifications in app/Jobs/AddPoliciesToBackupSetJob.php +- [X] T015 [P] [US2] Remove custom completion/failure DB notifications in app/Jobs/RemovePoliciesFromBackupSetJob.php ### Implementation (Start surfaces / Filament) -- [ ] T016 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/PolicyResource.php (remove sendToDatabase for queued ops) -- [ ] T017 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/BackupScheduleResource.php (remove sendToDatabase for queued ops) -- [ ] T018 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/TenantResource.php (remove sendToDatabase for queued ops) -- [ ] T019 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/PolicyVersionResource.php (remove sendToDatabase for queued ops) -- [ ] T031 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/EntraGroupResource/Pages/ListEntraGroups.php (remove sendToDatabase for queued ops) +- [X] T016 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/PolicyResource.php (remove sendToDatabase for queued ops) +- [X] T017 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/BackupScheduleResource.php (remove sendToDatabase for queued ops) +- [X] T018 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/TenantResource.php (remove sendToDatabase for queued ops) +- [X] T019 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/PolicyVersionResource.php (remove sendToDatabase for queued ops) +- [X] T031 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/EntraGroupResource/Pages/ListEntraGroups.php (remove sendToDatabase for queued ops) **Checkpoint**: US2 regressions pass and notifications remain terminal-only. @@ -97,12 +97,12 @@ ## Phase 5: User Story 3 — Legacy Notification Removed (Priority: P1) ### Tests (write first) -- [ ] T020 [P] [US3] Add restore run terminal notification regression test in tests/Feature/OpsUx/Regression/RestoreRunTerminalNotificationTest.php +- [X] T020 [P] [US3] Add restore run terminal notification regression test in tests/Feature/OpsUx/Regression/RestoreRunTerminalNotificationTest.php ### Implementation -- [ ] T021 [US3] Remove legacy notification invocation in app/Jobs/ExecuteRestoreRunJob.php -- [ ] T022 [US3] Delete legacy notification class app/Notifications/RunStatusChangedNotification.php +- [X] T021 [US3] Remove legacy notification invocation in app/Jobs/ExecuteRestoreRunJob.php +- [X] T022 [US3] Delete legacy notification class app/Notifications/RunStatusChangedNotification.php **Checkpoint**: US3 regression passes; no legacy notification remains. @@ -116,9 +116,9 @@ ## Phase 6: User Story 4 — Regression Guards Enforce the Constitution (Priorit ### Guard tests -- [ ] T023 [P] [US4] Implement Guard A in tests/Feature/OpsUx/Constitution/DirectStatusTransitionGuardTest.php (scan app/** for ->update([...]) containing status/outcome; exclude app/Services/OperationRunService.php; print snippet) -- [ ] T024 [P] [US4] Implement Guard B in tests/Feature/OpsUx/Constitution/JobDbNotificationGuardTest.php (scan app/** for OperationRun signal + DB notify emission; allowlist app/Services/OperationRunService.php and app/Notifications/OperationRunCompleted.php) -- [ ] T025 [P] [US4] Implement Guard C in tests/Feature/OpsUx/Constitution/LegacyNotificationGuardTest.php (scan app/** and tests/** for RunStatusChangedNotification) +- [X] T023 [P] [US4] Implement Guard A in tests/Feature/OpsUx/Constitution/DirectStatusTransitionGuardTest.php (scan app/** for forbidden status/outcome transitions: ->update([...]) arrays, direct ->status/->outcome assignments, and query/bulk updates; exclude app/Services/OperationRunService.php; print snippet) +- [X] T024 [P] [US4] Implement Guard B in tests/Feature/OpsUx/Constitution/JobDbNotificationGuardTest.php (scan app/** for OperationRun signal + DB notify emission; allowlist app/Services/OperationRunService.php and app/Notifications/OperationRunCompleted.php) +- [X] T025 [P] [US4] Implement Guard C in tests/Feature/OpsUx/Constitution/LegacyNotificationGuardTest.php (scan app/** and tests/** for RunStatusChangedNotification) **Checkpoint**: Guard tests pass green and provide clear failure output. @@ -130,16 +130,34 @@ ## Phase 7: User Story 5 — Canonical "Already Queued" Toast (Priority: P2) **Independent Test**: Trigger dedup path and confirm toast uses `OperationUxPresenter::alreadyQueuedToast(...)`. -- [ ] T026 [P] [US5] Add OperationUxPresenter::alreadyQueuedToast(...) helper in app/Support/OpsUx/OperationUxPresenter.php -- [ ] T027 [US5] Migrate dedup toast to canonical helper in app/Livewire/BackupSetPolicyPickerTable.php +- [X] T026 [P] [US5] Add OperationUxPresenter::alreadyQueuedToast(...) helper in app/Support/OpsUx/OperationUxPresenter.php +- [X] T027 [US5] Migrate dedup toast to canonical helper in app/Livewire/BackupSetPolicyPickerTable.php --- ## Phase 8: Polish & Cross-Cutting Concerns -**Purpose**: Keep docs and developer workflow aligned with final test locations. +**Purpose**: Final documentation alignment plus execution validation (guards, regressions, full suite, formatting). -- [ ] T028 [P] Update quickstart commands/paths if needed in specs/110-ops-ux-enforcement/quickstart.md +- [X] T028 [P] Update quickstart commands/paths if needed in specs/110-ops-ux-enforcement/quickstart.md +- [X] T032 Run focused Ops-UX regression pack (including `tests/Feature/OpsUx/Regression/*`) and confirm green (SC-006 / DoD) +- [X] T033 Run constitution guard tests (`tests/Feature/OpsUx/Constitution/*`) and verify actionable failure output on synthetic violation + green on clean codebase (SC-005) +- [X] T034 Run full test suite (or CI-equivalent command used by this repo) and confirm green (DoD) +- [X] T035 [P] Run Pint for touched files via Sail (`./vendor/bin/sail bin pint --dirty`) and confirm clean (DoD) + +--- + +## Phase 9: Follow-up — Repo-wide Start/Dedup Toast Standardization + +**Purpose**: Ensure all remaining Filament start/dedup surfaces use canonical Ops-UX toasts and trigger immediate progress refresh. + +- [X] T036 Standardize tenant verification start/dedup toasts + progress refresh (Tenant list row, tenant view header, verification widget) +- [X] T037 Standardize review pack generation widget to canonical queued/already queued toasts + view-run action +- [X] T038 Ensure admin roles scan creates/dedupes OperationRun at enqueue time + canonical toasts + progress refresh +- [X] T039 Standardize backup set removal dedupe notifications to canonical already queued toast + progress refresh +- [X] T040 Standardize restore run idempotency “already queued” to canonical already queued toast (with view-run when available) +- [X] T041 Standardize policy bulk delete queued/dedup toast to canonical queued/already queued + progress refresh +- [X] T042 Standardize onboarding wizard verification + bootstrap start/dedup toasts + progress refresh --- diff --git a/tests/Feature/BackupScheduling/RunNowRetryActionsTest.php b/tests/Feature/BackupScheduling/RunNowRetryActionsTest.php index 7969121..927a327 100644 --- a/tests/Feature/BackupScheduling/RunNowRetryActionsTest.php +++ b/tests/Feature/BackupScheduling/RunNowRetryActionsTest.php @@ -4,11 +4,8 @@ use App\Jobs\RunBackupScheduleJob; use App\Models\BackupSchedule; use App\Models\OperationRun; -use App\Models\User; -use App\Notifications\OperationRunQueued; use App\Services\Graph\GraphClientInterface; use App\Services\OperationRunService; -use App\Support\OperationRunLinks; use Carbon\CarbonImmutable; use Filament\Facades\Filament; use Illuminate\Support\Facades\Queue; @@ -25,7 +22,7 @@ }); }); -test('operator can run now and it persists a database notification', function () { +test('operator can run now without persisting a database notification', function () { Queue::fake([RunBackupScheduleJob::class]); [$user, $tenant] = createUserWithTenant(role: 'operator'); @@ -67,19 +64,7 @@ && $job->operationRun->is($operationRun); }); - $this->assertDatabaseCount('notifications', 1); - $this->assertDatabaseHas('notifications', [ - 'notifiable_id' => $user->id, - 'notifiable_type' => User::class, - 'type' => OperationRunQueued::class, - 'data->format' => 'filament', - 'data->title' => 'Backup schedule run queued', - ]); - - $notification = $user->notifications()->latest('id')->first(); - expect($notification)->not->toBeNull(); - expect($notification->data['actions'][0]['url'] ?? null) - ->toBe(OperationRunLinks::view($operationRun, $tenant)); + $this->assertDatabaseCount('notifications', 0); }); test('run now is unique per click (no dedupe)', function () { @@ -119,10 +104,10 @@ expect($runs[0])->not->toBe($runs[1]); Queue::assertPushed(RunBackupScheduleJob::class, 2); - $this->assertDatabaseCount('notifications', 2); + $this->assertDatabaseCount('notifications', 0); }); -test('operator can retry and it persists a database notification', function () { +test('operator can retry without persisting a database notification', function () { Queue::fake([RunBackupScheduleJob::class]); [$user, $tenant] = createUserWithTenant(role: 'operator'); @@ -163,19 +148,7 @@ && $job->operationRun instanceof OperationRun && $job->operationRun->is($operationRun); }); - $this->assertDatabaseCount('notifications', 1); - $this->assertDatabaseHas('notifications', [ - 'notifiable_id' => $user->id, - 'notifiable_type' => User::class, - 'type' => OperationRunQueued::class, - 'data->format' => 'filament', - 'data->title' => 'Backup schedule run queued', - ]); - - $notification = $user->notifications()->latest('id')->first(); - expect($notification)->not->toBeNull(); - expect($notification->data['actions'][0]['url'] ?? null) - ->toBe(OperationRunLinks::view($operationRun, $tenant)); + $this->assertDatabaseCount('notifications', 0); }); test('retry is unique per click (no dedupe)', function () { @@ -215,7 +188,7 @@ expect($runs[0])->not->toBe($runs[1]); Queue::assertPushed(RunBackupScheduleJob::class, 2); - $this->assertDatabaseCount('notifications', 2); + $this->assertDatabaseCount('notifications', 0); }); test('readonly cannot dispatch run now or retry', function () { @@ -258,7 +231,7 @@ ->toBe(0); }); -test('operator can bulk run now and it persists a database notification', function () { +test('operator can bulk run now without persisting a database notification', function () { Queue::fake([RunBackupScheduleJob::class]); [$user, $tenant] = createUserWithTenant(role: 'operator'); @@ -311,20 +284,10 @@ ->toBe([$user->id]); Queue::assertPushed(RunBackupScheduleJob::class, 2); - $this->assertDatabaseCount('notifications', 1); - $this->assertDatabaseHas('notifications', [ - 'notifiable_id' => $user->id, - 'data->format' => 'filament', - 'data->title' => 'Runs dispatched', - ]); - - $notification = $user->notifications()->latest('id')->first(); - expect($notification)->not->toBeNull(); - expect($notification->data['actions'][0]['url'] ?? null) - ->toBe(OperationRunLinks::index($tenant)); + $this->assertDatabaseCount('notifications', 0); }); -test('operator can bulk retry and it persists a database notification', function () { +test('operator can bulk retry without persisting a database notification', function () { Queue::fake([RunBackupScheduleJob::class]); [$user, $tenant] = createUserWithTenant(role: 'operator'); @@ -377,17 +340,7 @@ ->toBe([$user->id]); Queue::assertPushed(RunBackupScheduleJob::class, 2); - $this->assertDatabaseCount('notifications', 1); - $this->assertDatabaseHas('notifications', [ - 'notifiable_id' => $user->id, - 'data->format' => 'filament', - 'data->title' => 'Retries dispatched', - ]); - - $notification = $user->notifications()->latest('id')->first(); - expect($notification)->not->toBeNull(); - expect($notification->data['actions'][0]['url'] ?? null) - ->toBe(OperationRunLinks::index($tenant)); + $this->assertDatabaseCount('notifications', 0); }); test('operator can bulk retry even if a previous canonical run exists', function () { diff --git a/tests/Feature/BackupSets/RemovePoliciesJobNotificationTest.php b/tests/Feature/BackupSets/RemovePoliciesJobNotificationTest.php index 542ee83..e6321ea 100644 --- a/tests/Feature/BackupSets/RemovePoliciesJobNotificationTest.php +++ b/tests/Feature/BackupSets/RemovePoliciesJobNotificationTest.php @@ -4,11 +4,11 @@ use App\Models\BackupItem; use App\Models\BackupSet; use App\Models\OperationRun; +use App\Notifications\OperationRunCompleted; use App\Services\Intune\AuditLogger; use App\Support\OperationRunLinks; -use Filament\Notifications\DatabaseNotification; -it('remove policies job sends completion notification with view link', function () { +it('remove policies job sends canonical terminal notification with view link', function () { [$user, $tenant] = createUserWithTenant(role: 'owner'); $backupSet = BackupSet::factory()->create([ @@ -54,7 +54,7 @@ $this->assertDatabaseHas('notifications', [ 'notifiable_id' => $user->getKey(), 'notifiable_type' => $user->getMorphClass(), - 'type' => DatabaseNotification::class, + 'type' => OperationRunCompleted::class, ]); $notification = $user->notifications()->latest('id')->first(); diff --git a/tests/Feature/Filament/BackupSetPolicyPickerTableTest.php b/tests/Feature/Filament/BackupSetPolicyPickerTableTest.php index 3e4554b..fb663aa 100644 --- a/tests/Feature/Filament/BackupSetPolicyPickerTableTest.php +++ b/tests/Feature/Filament/BackupSetPolicyPickerTableTest.php @@ -9,6 +9,7 @@ use App\Models\Tenant; use App\Models\User; use App\Services\Intune\BackupService; +use App\Support\OpsUx\OperationUxPresenter; use Filament\Facades\Filament; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Queue; @@ -125,6 +126,13 @@ ->count())->toBe(1); Queue::assertPushed(AddPoliciesToBackupSetJob::class, 1); + + $notifications = session('filament.notifications', []); + $expectedToast = OperationUxPresenter::alreadyQueuedToast('backup_set.add_policies'); + + expect($notifications)->not->toBeEmpty(); + expect(collect($notifications)->last()['title'] ?? null)->toBe($expectedToast->getTitle()); + expect(collect($notifications)->last()['body'] ?? null)->toBe($expectedToast->getBody()); }); test('policy picker table forbids readonly users from starting add policies (403)', function () { diff --git a/tests/Feature/Filament/BaselineCompareLandingStartSurfaceTest.php b/tests/Feature/Filament/BaselineCompareLandingStartSurfaceTest.php new file mode 100644 index 0000000..6777cde --- /dev/null +++ b/tests/Feature/Filament/BaselineCompareLandingStartSurfaceTest.php @@ -0,0 +1,55 @@ +actingAs($user); + + $tenant->makeCurrent(); + Filament::setTenant($tenant, true); + + $profile = BaselineProfile::factory()->active()->create([ + 'workspace_id' => (int) $tenant->workspace_id, + ]); + + $snapshot = BaselineSnapshot::factory()->create([ + 'workspace_id' => (int) $tenant->workspace_id, + 'baseline_profile_id' => (int) $profile->getKey(), + ]); + + $profile->update(['active_snapshot_id' => (int) $snapshot->getKey()]); + + BaselineTenantAssignment::factory()->create([ + 'workspace_id' => (int) $tenant->workspace_id, + 'tenant_id' => (int) $tenant->getKey(), + 'baseline_profile_id' => (int) $profile->getKey(), + ]); + + Livewire::test(BaselineCompareLanding::class) + ->callAction('compareNow') + ->assertDispatchedTo(BulkOperationProgress::class, OpsUxBrowserEvents::RunEnqueued, tenantId: (int) $tenant->getKey()); + + Queue::assertPushed(CompareBaselineToTenantJob::class); + + $run = OperationRun::query() + ->where('tenant_id', (int) $tenant->getKey()) + ->where('type', 'baseline_compare') + ->latest('id') + ->first(); + + expect($run)->not->toBeNull(); + expect($run?->status)->toBe('queued'); +}); diff --git a/tests/Feature/Filament/CreateCtaPlacementTest.php b/tests/Feature/Filament/CreateCtaPlacementTest.php index dfff483..128127f 100644 --- a/tests/Feature/Filament/CreateCtaPlacementTest.php +++ b/tests/Feature/Filament/CreateCtaPlacementTest.php @@ -222,9 +222,9 @@ function getHeaderAction(Testable $component, string $name): ?Action ->withSession([WorkspaceContext::SESSION_KEY => (int) $workspace->getKey()]); $component = Livewire::test(ListTenants::class) - ->assertTableEmptyStateActionsExistInOrder(['create']); + ->assertTableEmptyStateActionsExistInOrder(['add_tenant']); - $headerCreate = getHeaderAction($component, 'create'); + $headerCreate = getHeaderAction($component, 'add_tenant'); expect($headerCreate)->not->toBeNull(); expect($headerCreate?->isVisible())->toBeFalse(); }); @@ -237,7 +237,7 @@ function getHeaderAction(Testable $component, string $name): ?Action $component = Livewire::test(ListTenants::class) ->assertCountTableRecords(1); - $headerCreate = getHeaderAction($component, 'create'); + $headerCreate = getHeaderAction($component, 'add_tenant'); expect($headerCreate)->not->toBeNull(); expect($headerCreate?->isVisible())->toBeTrue(); }); diff --git a/tests/Feature/Notifications/OperationRunNotificationTest.php b/tests/Feature/Notifications/OperationRunNotificationTest.php index 07f05ad..5a74120 100644 --- a/tests/Feature/Notifications/OperationRunNotificationTest.php +++ b/tests/Feature/Notifications/OperationRunNotificationTest.php @@ -24,7 +24,7 @@ $service->dispatchOrFail($run, function (): void { // no-op (dispatch succeeded) - }); + }, emitQueuedNotification: true); $this->assertDatabaseHas('notifications', [ 'notifiable_id' => $user->getKey(), @@ -58,7 +58,7 @@ $service->dispatchOrFail($run, function (): void { // no-op - }); + }, emitQueuedNotification: true); expect($user->notifications()->count())->toBe(0); }); diff --git a/tests/Feature/OpsUx/Constitution/DirectStatusTransitionGuardTest.php b/tests/Feature/OpsUx/Constitution/DirectStatusTransitionGuardTest.php new file mode 100644 index 0000000..e1625a8 --- /dev/null +++ b/tests/Feature/OpsUx/Constitution/DirectStatusTransitionGuardTest.php @@ -0,0 +1,66 @@ + '/(?:\$this->operationRun|\$operationRun|\$opRun)\s*->\s*update\s*\(\s*\[(?:(?!\)\s*;).)*?(?:[\'"]status[\'"]|[\'"]outcome[\'"])\s*=>/s', + 'direct fill()/forceFill() with status/outcome' => '/(?:\$this->operationRun|\$operationRun|\$opRun)\s*->\s*(?:fill|forceFill)\s*\(\s*\[(?:(?!\)\s*;).)*?(?:[\'"]status[\'"]|[\'"]outcome[\'"])\s*=>/s', + 'direct property assignment' => '/(?:\$this->operationRun|\$operationRun|\$opRun)\s*->\s*(?:status|outcome)\s*=(?!=)/', + 'OperationRun query/bulk update with status/outcome' => '/OperationRun::(?:(?!;).){0,800}?->\s*update\s*\(\s*\[(?:(?!\)\s*;).)*?(?:[\'"]status[\'"]|[\'"]outcome[\'"])\s*=>/s', + ]; + + $violations = []; + + foreach ($files as $file) { + $source = SourceFileScanner::read($file); + + foreach ($patterns as $label => $pattern) { + if (! preg_match_all($pattern, $source, $matches, PREG_OFFSET_CAPTURE)) { + continue; + } + + foreach ($matches[0] as [$snippetMatch, $offset]) { + if (! is_int($offset)) { + continue; + } + + $line = substr_count(substr($source, 0, $offset), "\n") + 1; + + $violations[] = [ + 'file' => SourceFileScanner::relativePath($file), + 'line' => $line, + 'label' => $label, + 'snippet' => SourceFileScanner::snippet($source, $line), + 'match' => trim((string) $snippetMatch), + ]; + } + } + } + + if ($violations !== []) { + $messages = array_map(static function (array $violation): string { + return sprintf( + "%s:%d [%s]\n%s", + $violation['file'], + $violation['line'], + $violation['label'], + $violation['snippet'], + ); + }, $violations); + + $this->fail( + "Forbidden direct OperationRun status/outcome transition(s) found outside OperationRunService:\n\n" + .implode("\n\n", $messages) + ); + } + + expect($violations)->toBe([]); +})->group('ops-ux'); diff --git a/tests/Feature/OpsUx/Constitution/JobDbNotificationGuardTest.php b/tests/Feature/OpsUx/Constitution/JobDbNotificationGuardTest.php new file mode 100644 index 0000000..463b552 --- /dev/null +++ b/tests/Feature/OpsUx/Constitution/JobDbNotificationGuardTest.php @@ -0,0 +1,71 @@ + SourceFileScanner::relativePath($file), + 'line' => $line, + 'snippet' => SourceFileScanner::snippet($source, $line), + ]; + } + } + + if ($violations !== []) { + $messages = array_map(static function (array $violation): string { + return sprintf( + "%s:%d\n%s", + $violation['file'], + $violation['line'], + $violation['snippet'], + ); + }, $violations); + + $this->fail( + "Forbidden OperationRun-related database notification emission found (use canonical OperationRunService terminal notification / toast-only start feedback):\n\n" + .implode("\n\n", $messages) + ); + } + + expect($violations)->toBe([]); +})->group('ops-ux'); diff --git a/tests/Feature/OpsUx/Constitution/LegacyNotificationGuardTest.php b/tests/Feature/OpsUx/Constitution/LegacyNotificationGuardTest.php new file mode 100644 index 0000000..0ce378f --- /dev/null +++ b/tests/Feature/OpsUx/Constitution/LegacyNotificationGuardTest.php @@ -0,0 +1,54 @@ + SourceFileScanner::relativePath($file), + 'line' => $line, + 'snippet' => SourceFileScanner::snippet($source, $line), + ]; + + $offset = $position + strlen($needle); + } + } + + if ($violations !== []) { + $messages = array_map(static function (array $violation): string { + return sprintf( + "%s:%d\n%s", + $violation['file'], + $violation['line'], + $violation['snippet'], + ); + }, $violations); + + $this->fail( + "Legacy notification reference(s) found:\n\n".implode("\n\n", $messages) + ); + } + + expect($violations)->toBe([]); +})->group('ops-ux'); diff --git a/tests/Feature/OpsUx/NoQueuedDbNotificationsTest.php b/tests/Feature/OpsUx/NoQueuedDbNotificationsTest.php index d387753..734a5cb 100644 --- a/tests/Feature/OpsUx/NoQueuedDbNotificationsTest.php +++ b/tests/Feature/OpsUx/NoQueuedDbNotificationsTest.php @@ -6,7 +6,7 @@ use App\Services\OperationRunService; use Filament\Facades\Filament; -it('emits at most one queued database notification per newly created run', function (): void { +it('emits at most one queued database notification per newly created run when explicitly enabled', function (): void { [$user, $tenant] = createUserWithTenant(role: 'owner'); $this->actingAs($user); @@ -25,7 +25,7 @@ $service->dispatchOrFail($run, function (): void { // no-op (dispatch succeeded) - }); + }, emitQueuedNotification: true); expect($user->notifications()->count())->toBe(1); $this->assertDatabaseHas('notifications', [ diff --git a/tests/Feature/OpsUx/NotificationViewRunLinkTest.php b/tests/Feature/OpsUx/NotificationViewRunLinkTest.php index f520178..7553c4d 100644 --- a/tests/Feature/OpsUx/NotificationViewRunLinkTest.php +++ b/tests/Feature/OpsUx/NotificationViewRunLinkTest.php @@ -3,7 +3,6 @@ declare(strict_types=1); use App\Models\OperationRun; -use App\Notifications\RunStatusChangedNotification; use App\Services\OperationRunService; use App\Support\OperationRunLinks; use Filament\Facades\Filament; @@ -42,8 +41,11 @@ ->toBe(OperationRunLinks::view($run, $tenant)); })->group('ops-ux'); -it('does not link to legacy bulk run resources in status-change notifications', function (): void { +it('does not link to legacy bulk run resources in canonical terminal notifications', function (): void { [$user, $tenant] = createUserWithTenant(role: 'owner'); + $this->actingAs($user); + + Filament::setTenant($tenant, true); $run = OperationRun::factory()->create([ 'tenant_id' => $tenant->getKey(), @@ -55,12 +57,16 @@ 'context' => ['operation' => ['type' => 'policy.delete']], ]); - $user->notify(new RunStatusChangedNotification([ - 'tenant_id' => (int) $tenant->getKey(), - 'run_type' => 'bulk_operation', - 'run_id' => (int) $run->getKey(), - 'status' => 'completed', - ])); + /** @var OperationRunService $service */ + $service = app(OperationRunService::class); + + $service->updateRun( + $run, + status: 'completed', + outcome: 'succeeded', + summaryCounts: ['total' => 1], + failures: [], + ); $notification = $user->notifications()->latest('id')->first(); expect($notification)->not->toBeNull(); diff --git a/tests/Feature/OpsUx/QueuedToastCopyTest.php b/tests/Feature/OpsUx/QueuedToastCopyTest.php index 2247e6c..53d2aea 100644 --- a/tests/Feature/OpsUx/QueuedToastCopyTest.php +++ b/tests/Feature/OpsUx/QueuedToastCopyTest.php @@ -20,3 +20,10 @@ expect($duration)->toBeGreaterThanOrEqual(3000); expect($duration)->toBeLessThanOrEqual(5000); })->group('ops-ux'); + +it('builds canonical already-queued toast copy', function (): void { + $toast = OperationUxPresenter::alreadyQueuedToast('backup_set.add_policies'); + + expect($toast->getTitle())->toBe('Backup set update already queued'); + expect($toast->getBody())->toBe('A matching run is already queued or running.'); +})->group('ops-ux'); diff --git a/tests/Feature/OpsUx/Regression/BackupRetentionTerminalNotificationTest.php b/tests/Feature/OpsUx/Regression/BackupRetentionTerminalNotificationTest.php new file mode 100644 index 0000000..4053158 --- /dev/null +++ b/tests/Feature/OpsUx/Regression/BackupRetentionTerminalNotificationTest.php @@ -0,0 +1,75 @@ +create([ + 'tenant_id' => (int) $tenant->getKey(), + 'name' => 'Retention Regression', + 'is_enabled' => true, + 'timezone' => 'UTC', + 'frequency' => 'daily', + 'time_of_day' => '01:00:00', + 'days_of_week' => null, + 'policy_types' => ['deviceConfiguration'], + 'include_foundations' => true, + 'retention_keep_last' => 2, + ]); + + $sets = collect(range(1, 4))->map(function (int $index) use ($tenant): BackupSet { + return BackupSet::query()->create([ + 'tenant_id' => (int) $tenant->getKey(), + 'name' => 'Retention Set '.$index, + 'status' => 'completed', + 'item_count' => 0, + 'completed_at' => now()->subMinutes(10 - $index), + ]); + }); + + $completedAt = now('UTC')->startOfMinute()->subMinutes(8); + + foreach ($sets as $set) { + OperationRun::query()->create([ + 'workspace_id' => (int) $tenant->workspace_id, + 'tenant_id' => (int) $tenant->getKey(), + 'user_id' => null, + 'initiator_name' => 'System', + 'type' => 'backup_schedule_run', + 'status' => 'completed', + 'outcome' => 'succeeded', + 'run_identity_hash' => hash('sha256', 'ops-ux-retention-regression:'.$schedule->id.':'.$set->id), + 'summary_counts' => [], + 'failure_summary' => [], + 'context' => [ + 'backup_schedule_id' => (int) $schedule->id, + 'backup_set_id' => (int) $set->id, + ], + 'started_at' => $completedAt, + 'completed_at' => $completedAt, + ]); + + $completedAt = $completedAt->addMinute(); + } + + ApplyBackupScheduleRetentionJob::dispatchSync((int) $schedule->getKey()); + + $retentionRun = OperationRun::query() + ->where('tenant_id', (int) $tenant->getKey()) + ->where('type', 'backup_schedule_retention') + ->latest('id') + ->first(); + + expect($retentionRun)->not->toBeNull(); + expect($retentionRun?->status)->toBe('completed'); + expect($retentionRun?->outcome)->toBe('succeeded'); + expect((int) ($retentionRun?->summary_counts['processed'] ?? 0))->toBe(2); + expect($user->notifications()->count())->toBe(0); + $this->assertDatabaseCount('notifications', 0); +})->group('ops-ux'); diff --git a/tests/Feature/OpsUx/Regression/BackupScheduleRunNotificationTest.php b/tests/Feature/OpsUx/Regression/BackupScheduleRunNotificationTest.php new file mode 100644 index 0000000..a917900 --- /dev/null +++ b/tests/Feature/OpsUx/Regression/BackupScheduleRunNotificationTest.php @@ -0,0 +1,113 @@ +actingAs($user); + + $tenant->makeCurrent(); + Filament::setTenant($tenant, true); + + $schedule = BackupSchedule::query()->create([ + 'tenant_id' => (int) $tenant->getKey(), + 'name' => 'OpsUx Regression Schedule', + 'is_enabled' => true, + 'timezone' => 'UTC', + 'frequency' => 'daily', + 'time_of_day' => '10:00:00', + 'days_of_week' => null, + 'policy_types' => ['deviceConfiguration'], + 'include_foundations' => true, + 'retention_keep_last' => 30, + 'next_run_at' => null, + ]); + + /** @var OperationRunService $operationRuns */ + $operationRuns = app(OperationRunService::class); + $run = $operationRuns->ensureRun( + tenant: $tenant, + type: 'backup_schedule_run', + inputs: ['backup_schedule_id' => (int) $schedule->getKey()], + initiator: $user, + ); + + app()->bind(PolicySyncService::class, fn (): PolicySyncService => new class extends PolicySyncService + { + public function __construct() {} + + public function syncPoliciesWithReport($tenant, ?array $supportedTypes = null): array + { + return ['synced' => [], 'failures' => []]; + } + }); + + $backupSet = BackupSet::factory()->create([ + 'tenant_id' => (int) $tenant->getKey(), + 'status' => 'completed', + 'item_count' => 0, + ]); + + app()->bind(BackupService::class, fn (): BackupService => new class($backupSet) extends BackupService + { + public function __construct(private readonly BackupSet $backupSet) {} + + public function createBackupSet($tenant, $policyIds, ?string $actorEmail = null, ?string $actorName = null, ?string $name = null, bool $includeAssignments = false, bool $includeScopeTags = false, bool $includeFoundations = false): BackupSet + { + return $this->backupSet; + } + }); + + Cache::flush(); + + (new RunBackupScheduleJob(operationRun: $run, backupScheduleId: (int) $schedule->getKey()))->handle( + app(PolicySyncService::class), + app(BackupService::class), + app(PolicyTypeResolver::class), + app(ScheduleTimeService::class), + app(AuditLogger::class), + app(RunErrorMapper::class), + ); + + $run->refresh(); + + expect($run->status)->toBe('completed'); + expect($run->outcome)->toBe('succeeded'); + expect($user->notifications()->count())->toBe(1); + + $this->assertDatabaseHas('notifications', [ + 'notifiable_id' => $user->getKey(), + 'notifiable_type' => $user->getMorphClass(), + 'type' => OperationRunCompleted::class, + ]); + + $this->assertDatabaseMissing('notifications', [ + 'notifiable_id' => $user->getKey(), + 'notifiable_type' => $user->getMorphClass(), + 'type' => DatabaseNotification::class, + ]); + + Bus::assertDispatched(ApplyBackupScheduleRetentionJob::class); +})->group('ops-ux'); diff --git a/tests/Feature/OpsUx/Regression/BulkJobCircuitBreakerTest.php b/tests/Feature/OpsUx/Regression/BulkJobCircuitBreakerTest.php new file mode 100644 index 0000000..b9f9073 --- /dev/null +++ b/tests/Feature/OpsUx/Regression/BulkJobCircuitBreakerTest.php @@ -0,0 +1,55 @@ +actingAs($user); + + $tenant->makeCurrent(); + Filament::setTenant($tenant, true); + + /** @var OperationRunService $operationRuns */ + $operationRuns = app(OperationRunService::class); + $run = $operationRuns->ensureRun( + tenant: $tenant, + type: 'policy.export', + inputs: ['scope' => 'subset', 'policy_ids' => [999_999_991]], + initiator: $user, + ); + + $job = new BulkPolicyExportJob( + tenantId: (int) $tenant->getKey(), + userId: (int) $user->getKey(), + policyIds: [999_999_991], + backupName: 'OpsUx Circuit Breaker Regression', + operationRun: $run, + ); + + $job->handle($operationRuns); + + $run->refresh(); + + expect($run->status)->toBe('completed'); + expect($run->outcome)->toBe('failed'); + expect((int) ($run->summary_counts['failed'] ?? 0))->toBeGreaterThan(0); + expect($user->notifications()->count())->toBe(1); + + $this->assertDatabaseHas('notifications', [ + 'notifiable_id' => $user->getKey(), + 'notifiable_type' => $user->getMorphClass(), + 'type' => OperationRunCompleted::class, + ]); + + $this->assertDatabaseMissing('notifications', [ + 'notifiable_id' => $user->getKey(), + 'notifiable_type' => $user->getMorphClass(), + 'type' => DatabaseNotification::class, + ]); +})->group('ops-ux'); diff --git a/tests/Feature/OpsUx/Regression/InventorySyncTerminalNotificationTest.php b/tests/Feature/OpsUx/Regression/InventorySyncTerminalNotificationTest.php new file mode 100644 index 0000000..242fbfa --- /dev/null +++ b/tests/Feature/OpsUx/Regression/InventorySyncTerminalNotificationTest.php @@ -0,0 +1,77 @@ +defaultSelectionPayload(); + $computed = $sync->normalizeAndHashSelection($selectionPayload); + + $mockSync = \Mockery::mock(InventorySyncService::class); + $mockSync + ->shouldReceive('executeSelection') + ->once() + ->andReturn([ + 'status' => 'success', + 'had_errors' => false, + 'error_codes' => [], + 'error_context' => [], + 'errors_count' => 0, + 'items_observed_count' => 0, + 'items_upserted_count' => 0, + 'skipped_policy_types' => [], + 'processed_policy_types' => $computed['selection']['policy_types'], + 'failed_policy_types' => [], + 'selection_hash' => $computed['selection_hash'], + ]); + + /** @var OperationRunService $operationRuns */ + $operationRuns = app(OperationRunService::class); + $run = $operationRuns->ensureRun( + tenant: $tenant, + type: 'inventory_sync', + inputs: $computed['selection'], + initiator: $user, + ); + + $job = new RunInventorySyncJob( + tenantId: (int) $tenant->getKey(), + userId: (int) $user->getKey(), + operationRun: $run, + ); + + expect($user->notifications()->count())->toBe(0); + + $job->handle($mockSync, app(AuditLogger::class), $operationRuns); + + $run->refresh(); + + expect($run->status)->toBe('completed'); + expect($run->outcome)->toBe('succeeded'); + expect($user->notifications()->count())->toBe(1); + + $this->assertDatabaseHas('notifications', [ + 'notifiable_id' => $user->getKey(), + 'notifiable_type' => $user->getMorphClass(), + 'type' => OperationRunCompleted::class, + ]); +})->group('ops-ux'); + +it('does not persist terminal notifications for system-run inventory syncs without initiator', function (): void { + [, $tenant] = createUserWithTenant(role: 'owner'); + + $sync = app(InventorySyncService::class); + $run = $sync->syncNow($tenant, $sync->defaultSelectionPayload()); + + expect($run->status)->toBe('completed'); + expect($tenant->users()->firstOrFail()->notifications()->count())->toBe(0); + $this->assertDatabaseCount('notifications', 0); +})->group('ops-ux'); diff --git a/tests/Feature/OpsUx/Regression/RestoreRunTerminalNotificationTest.php b/tests/Feature/OpsUx/Regression/RestoreRunTerminalNotificationTest.php new file mode 100644 index 0000000..1efd1bb --- /dev/null +++ b/tests/Feature/OpsUx/Regression/RestoreRunTerminalNotificationTest.php @@ -0,0 +1,87 @@ +actingAs($user); + + $tenant->makeCurrent(); + Filament::setTenant($tenant, true); + + $backupSet = BackupSet::factory()->create([ + 'tenant_id' => (int) $tenant->getKey(), + ]); + + $restoreRun = RestoreRun::factory()->create([ + 'tenant_id' => (int) $tenant->getKey(), + 'backup_set_id' => (int) $backupSet->getKey(), + 'requested_by' => $user->email, + 'status' => 'queued', + 'started_at' => null, + 'completed_at' => null, + ]); + + /** @var OperationRunService $operationRuns */ + $operationRuns = app(OperationRunService::class); + $operationRun = $operationRuns->ensureRun( + tenant: $tenant, + type: 'restore.execute', + inputs: [ + 'restore_run_id' => (int) $restoreRun->getKey(), + 'backup_set_id' => (int) $backupSet->getKey(), + 'is_dry_run' => (bool) ($restoreRun->is_dry_run ?? false), + ], + initiator: $user, + ); + + $operationRun->forceFill([ + 'user_id' => (int) $user->getKey(), + 'initiator_name' => $user->name, + ])->save(); + + $this->mock(RestoreService::class, function ($mock) use ($restoreRun): void { + $mock->shouldReceive('executeForRun') + ->once() + ->andReturnUsing(function () use ($restoreRun): RestoreRun { + RestoreRun::query()->whereKey($restoreRun->getKey())->update([ + 'status' => 'completed', + 'completed_at' => now(), + ]); + + return RestoreRun::query()->findOrFail($restoreRun->getKey()); + }); + }); + + $job = new ExecuteRestoreRunJob( + restoreRunId: (int) $restoreRun->getKey(), + actorEmail: $user->email, + actorName: $user->name, + operationRun: $operationRun, + ); + + expect($user->notifications()->count())->toBe(0); + + $job->handle(app(RestoreService::class), app(AuditLogger::class)); + + $operationRun->refresh(); + + expect($operationRun->status)->toBe('completed'); + expect($operationRun->outcome)->toBe('succeeded'); + expect($user->notifications()->count())->toBe(1); + + $this->assertDatabaseHas('notifications', [ + 'notifiable_id' => $user->getKey(), + 'notifiable_type' => $user->getMorphClass(), + 'type' => OperationRunCompleted::class, + ]); +})->group('ops-ux'); diff --git a/tests/Feature/ReviewPack/ReviewPackGenerationTest.php b/tests/Feature/ReviewPack/ReviewPackGenerationTest.php index e6f829c..59ec017 100644 --- a/tests/Feature/ReviewPack/ReviewPackGenerationTest.php +++ b/tests/Feature/ReviewPack/ReviewPackGenerationTest.php @@ -303,7 +303,7 @@ function seedTenantWithData(Tenant $tenant): void }); }); -it('sends queued database notification when review pack generation is requested', function (): void { +it('does not send queued database notification when review pack generation is requested', function (): void { Queue::fake(); Notification::fake(); @@ -313,7 +313,7 @@ function seedTenantWithData(Tenant $tenant): void $service = app(ReviewPackService::class); $service->generate($tenant, $user); - Notification::assertSentTo($user, OperationRunQueued::class); + Notification::assertNotSentTo($user, OperationRunQueued::class); }); // ─── OperationRun Type ────────────────────────────────────────── diff --git a/tests/Support/OpsUx/SourceFileScanner.php b/tests/Support/OpsUx/SourceFileScanner.php new file mode 100644 index 0000000..ffa076f --- /dev/null +++ b/tests/Support/OpsUx/SourceFileScanner.php @@ -0,0 +1,103 @@ + $roots + * @param list $excludedAbsolutePaths + * @return list + */ + public static function phpFiles(array $roots, array $excludedAbsolutePaths = []): array + { + $files = []; + $excluded = array_fill_keys(array_map(self::normalizePath(...), $excludedAbsolutePaths), true); + + foreach ($roots as $root) { + $root = self::normalizePath($root); + + if (! is_dir($root)) { + continue; + } + + $iterator = new RecursiveIteratorIterator( + new RecursiveDirectoryIterator($root, RecursiveDirectoryIterator::SKIP_DOTS) + ); + + /** @var SplFileInfo $file */ + foreach ($iterator as $file) { + if (! $file->isFile()) { + continue; + } + + $path = self::normalizePath($file->getPathname()); + + if (pathinfo($path, PATHINFO_EXTENSION) !== 'php') { + continue; + } + + if (isset($excluded[$path])) { + continue; + } + + $files[] = $path; + } + } + + sort($files); + + return array_values(array_unique($files)); + } + + public static function projectRoot(): string + { + return self::normalizePath(dirname(__DIR__, 3)); + } + + public static function relativePath(string $absolutePath): string + { + $absolutePath = self::normalizePath($absolutePath); + $root = self::projectRoot(); + + if (str_starts_with($absolutePath, $root.'/')) { + return substr($absolutePath, strlen($root) + 1); + } + + return $absolutePath; + } + + public static function read(string $path): string + { + return (string) file_get_contents($path); + } + + public static function snippet(string $source, int $line, int $contextLines = 2): string + { + $allLines = preg_split('/\R/', $source) ?: []; + $line = max(1, $line); + + $start = max(1, $line - $contextLines); + $end = min(count($allLines), $line + $contextLines); + + $snippet = []; + + for ($index = $start; $index <= $end; $index++) { + $prefix = $index === $line ? '>' : ' '; + $snippet[] = sprintf('%s%4d | %s', $prefix, $index, $allLines[$index - 1] ?? ''); + } + + return implode("\n", $snippet); + } + + private static function normalizePath(string $path): string + { + return str_replace('\\', '/', $path); + } +} -- 2.45.2