ahmido
d49d33ac27
Implements feature 100 (Alert Targets): - US1: “Send test message” action (RBAC + confirmation + rate limit + audit + async job) - US2: Derived “Last test” status badge (Never/Sent/Failed/Pending) on view + edit surfaces - US3: “View last delivery” deep link + deliveries viewer filters (event_type, destination) incl. tenantless test deliveries Tests: - Full suite green (1348 passed, 7 skipped) - Added focused feature tests for send test, last test resolver/badges, and deep-link filters Co-authored-by: Ahmed Darrazi <ahmed.darrazi@live.de> Reviewed-on: #122
69 lines
4.0 KiB
Markdown
69 lines
4.0 KiB
Markdown
# Phase 0 — Research (099.1 Add-on)
|
||
|
||
This research resolves the open technical questions from the feature spec and anchors decisions to existing code in this repository.
|
||
|
||
## Decision 1 — Where “Alert Target View/Edit” lives in code
|
||
|
||
- **Decision**: Implement “Last test status” + “Send test message” on the Alert Target **Edit** page only.
|
||
- **Rationale**: The current Filament resource for alert targets is `AlertDestinationResource` and only defines `index/create/edit` pages (no View page).
|
||
- **Alternatives considered**:
|
||
- Add a new View page for `AlertDestinationResource` (rejected: expands UX surface and is not required for this add-on).
|
||
|
||
## Decision 2 — Canonical event type string
|
||
|
||
- **Decision**: Use a single canonical event type string for test sends: `alerts.test`.
|
||
- **Rationale**: The add-on spec requires deriving status from `alert_deliveries.event_type`. Keeping this string stable allows deterministic filtering and deep links.
|
||
- **Alternatives considered**:
|
||
- Use `test` or `destination_test` (rejected: deviates from spec and makes intent less explicit).
|
||
|
||
## Decision 3 — Timestamp mapping vs actual schema
|
||
|
||
- **Decision**: Map timestamps using existing columns:
|
||
- **Sent** → `sent_at`
|
||
- **Failed** → `updated_at` (no `failed_at` column exists)
|
||
- **Pending** → `send_after` if set, else `created_at`
|
||
- **Rationale**: The `alert_deliveries` schema only has `send_after`, `sent_at`, and `updated_at`. The spec’s `deliver_at` / `failed_at` naming is treated as conceptual.
|
||
- **Alternatives considered**:
|
||
- Add `failed_at` / `deliver_at` columns (rejected: violates “no new DB fields”).
|
||
|
||
## Decision 4 — How to represent test deliveries given current tenant enforcement
|
||
|
||
- **Decision**: Store test deliveries in `alert_deliveries` as **workspace-scoped, tenantless** records:
|
||
- `workspace_id` set
|
||
- `tenant_id` nullable
|
||
- `alert_rule_id` nullable
|
||
- `alert_destination_id` set
|
||
- **Rationale**:
|
||
- Alert targets (`AlertDestination`) are workspace-owned and accessible without a selected tenant.
|
||
- Current enforcement (`DerivesWorkspaceIdFromTenant`) requires a tenant and would make test deliveries invisible/uneven across users due to tenant entitlements.
|
||
- Deliver job execution (`DeliverAlertsJob`) does not require a tenant; it only needs the destination + payload.
|
||
- **Alternatives considered**:
|
||
- Pick an arbitrary tenant ID for test deliveries (rejected: would be invisible to some operators and breaks “single truth” per target).
|
||
- Create a synthetic “workspace tenant” record (rejected: adds data-model complexity).
|
||
|
||
## Decision 5 — Deep link mechanics for the Deliveries viewer
|
||
|
||
- **Decision**: Deep link into `AlertDeliveryResource` list view using Filament v5 filter query-string binding:
|
||
- `?filters[event_type][value]=alerts.test&filters[alert_destination_id][value]={DESTINATION_ID}`
|
||
- **Rationale**: Filament v5 `ListRecords` binds `tableFilters` to the URL under the `filters` key; `SelectFilter` uses `value`.
|
||
- **Alternatives considered**:
|
||
- Custom dedicated “Test deliveries” page (rejected: new surface beyond spec).
|
||
|
||
## Decision 6 — Badge semantics centralization (BADGE-001)
|
||
|
||
- **Decision**: Introduce centralized badge domains/mappers for:
|
||
- `AlertDeliveryStatus` (queued/deferred/sent/failed/suppressed/canceled)
|
||
- `AlertDestinationLastTestStatus` (never/pending/sent/failed)
|
||
- **Rationale**: Current `AlertDeliveryResource` implements ad-hoc status label/color helpers. This add-on must comply with BADGE-001 and should not add more local mappings.
|
||
- **Alternatives considered**:
|
||
- Keep ad-hoc mappings in the new code (rejected: violates BADGE-001).
|
||
|
||
## Decision 7 — OperationRun usage
|
||
|
||
- **Decision**: Do **not** create a new per-test `OperationRun`.
|
||
- **Rationale**:
|
||
- The user-triggered action is DB-only (authorization + rate limit + create delivery + audit), typically <2s.
|
||
- The external work runs via the existing `alerts.deliver` operation run (created by `DeliverAlertsJob`).
|
||
- **Alternatives considered**:
|
||
- Create a dedicated operation run type (rejected: higher complexity for small UX add-on).
|