Normalize event statuses at the API layer

[Aloento]

Summary

Normalize non-canonical event status values at the API output layer and move description rows out of updates[] into the event description field.

Status normalization

normalizeStatus(raw, endDate, eventType) maps legacy values stored in the DB to their canonical form, applied to every API event output (list, detail, updates[], extract):

raw	normalized
analyzing	analysing
in progress	in_progress
scheduled	planned
SYSTEM	resolved (incident) / completed (maintenance, info) when end_date is set; passed through when end_date is nil
changed / impact changed	not a status change: collapsed to the previous normalized status in the updates sequence, seeded from the event status so a leading row is never empty

impact stays numeric (0-3). The DB layer is untouched: legacy values remain in the database and are normalized only at the API boundary.

Description contract exception (intentional)

Rows with status == description are no longer emitted in updates[]. The latest such row's text becomes the event's description field, falling back to the stored description when no such row exists. This is the single deliberate contract change in this PR: it moves the frontend description workaround (StatusEnum.Description in Status.Trans.V2.ts) into the backend so it can be removed in a follow-up frontend PR. Removing the row also resequences updates[] IDs.

Existing visibility filtering (isInternalStatus, isCancelledWithoutPublicStatus) is unchanged; description rows are now captured only after the visibility filter.

Verification

• go build ./...
• go vet ./...
• go test -count=1 ./internal/...
• go test -c -o <tmp> ./tests/ (integration suite compiles; not run locally)

Unit tests in internal/api/v2/v2_visibility_test.go cover normalizeStatus (SYSTEM with/without end date, incident vs maintenance/info, analyzing/in progress/scheduled, changed/impact changed) and mapEventUpdates (description removal and latest-text extraction, ID resequencing, leading changed seeded from the event status).

Contract notes

• updates[].id is resequenced when a description row is removed. Any client that caches those ids as list keys must re-key; no such consumer is known.
• SYSTEM without an end date is emitted verbatim. It has no canonical terminal status, and the frontend already maps it (StatusEnum.System returns the previous status). Pinned by a test.

Review follow-up

• Event-level status now collapses changed/impact changed to the previous status from the update history (eventStatus), so it no longer disagrees with updates[]; a test pins updates[last].status == event.status for that case.
• lastStatus for a leading changed/impact changed row is seeded from the event status.
• description rows are captured only after the visibility filter.
• SYSTEM completion status is picked per event type.
• Left as-is with intent: analyzing is mapped for every event type (the value is incident-only in practice), SYSTEM without an end date is passed through, and a description row with empty text does not overwrite the stored description.

Deferred (pre-existing, not introduced here): isCancelledWithoutPublicStatus still inspects raw, pre-normalization statuses; normalizing there would change which events are hidden and is left out of this PR. eventStatus intentionally walks the full history rather than reusing the updates mapping, so the event-level status stays view-independent.

[ecosquad-autoreview]

Review: Normalize event statuses at the API layer

Verdict: approve — the change is well-scoped, tested, and the normalization is applied consistently at the API boundary. A few fragility notes below.

Summary

mapEventUpdates now drops description rows (latest one becomes the event description), and normalizeStatus maps legacy values (analyzing→analysing, in progress→in_progress, scheduled→planned, SYSTEM→resolved/completed when an end date exists). changed/impact changed are collapsed to the previous normalized status in the updates sequence. New unit tests cover all branches of both functions.

Findings

[suggestion] lastStatus empty on a leading changed/impact changed row — internal/api/v2/v2.go:1863-1875
If the first visible status row of an event is changed or impact changed (e.g., a stored event whose history starts with a date change), lastStatus is still the zero value "", so the emitted update gets an empty status. Consider initializing lastStatus from the incident's own normalizeStatus(inc.Status, ...) (passed in) or keeping the raw value as a fallback when lastStatus == "".

[suggestion] Visibility check runs on raw DB statuses, normalization happens after — internal/api/v2/v2.go:1863-1875
isInternalStatus and the GetIncidentHandler hiding checks (v2.go:306-321) operate on the raw Statuses/Status, while clients see normalized values. This is coherent today only because the internal set (pending_review, reviewed) contains no legacy aliases. If a legacy row were ever classified as internal (or an internal alias stored, e.g. scheduled for a pre-review maintenance), an event could be hidden from or shown to unauthenticated users inconsistently with what authenticated users see. Worth a comment documenting that the two vocabularies must stay disjoint, or normalizing before the visibility check.

[suggestion] latestDescription is captured regardless of auth / internal filtering — internal/api/v2/v2.go:1860-1868
Description rows are extracted before the isInternalStatus filter. If a description row were ever listed as internal, its text would still leak into description for unauthenticated callers. Not reachable with the current internal set, but the ordering makes the description extraction implicitly public; consider extracting only rows that also pass the visibility filter.

[suggestion] Type-mixed mappings — internal/api/v2/v2.go:1892-1905
analyzing→IncidentAnalysing is applied to any event type (an info/maintenance row with that legacy value would get an incident status), and SYSTEM for a non-incident type maps to MaintenanceCompleted including for info events, which have their own InfoCompleted constant. Fine if the DB only contains these legacy values for incident/maintenance events, but a per-type switch (or at least a //nolint-style comment) would make the intent explicit and prevent a silent wrong status for a future legacy value.

Minor

• toAPIEvent prefers latestDescription even when it's an empty string over a non-empty stored Description (v2.go:335-339). The latestDescription != "" guard handles the common case, but a deliberately empty description row still suppresses the stored description — likely intended, just noting.
• CI (build/go-test) was still queued at review time; the PR description claims go build/go test pass locally, which is consistent with what I read.

No critical issues found; the description-contract change and ID resequencing are explicitly documented in the PR description, and the new tests (v2_visibility_test.go) pin the behavior.

[ecosquad-autoreview]

Summary

This PR normalizes legacy event status values at the API output boundary (normalizeStatus) and moves description rows out of updates[] into the event description field. The core logic in internal/api/v2/v2.go is sensible and the change is scoped to the API layer, leaving the DB untouched. Unit tests are added and the integration test was updated to expect the normalized resolved value. I could not run the build or tests (CI is still queued), so verification rests on the PR description and CI.

Findings

Warning — eventStatus can return an empty status for events whose raw status is changed/impact changed (internal/api/v2/v2.go, ~lines 1898–1920)

eventStatus first checks the normalized own status; if it is changed/impact changed it walks the status history to find the previous real status. Two gaps:

1. The history walk has no fallback: if every row is a changed/impact changed/description row, the function returns whatever status was seeded with, which for a leading changed event is "". An event stored with status = changed (or impact changed) and a matching updates history would then be serialized with status: "" — an unparseable value for the frontend StatusEnum.
2. The walk is not visibility-aware, unlike mapEventUpdates (which drops internal rows for unauthenticated callers). An unauthenticated user can therefore see status derived from an internal row (e.g. pending_review) while updates[] shows none of it, breaking the invariant the new test TestEventStatus_MatchesLastUpdate asserts for the authenticated case.

Fix: return a documented fallback (e.g. keep the raw normalizeStatus result, or skip the collapse when no previous public status exists) and filter history rows with the same isInternalStatus rule used in mapEventUpdates — which means passing isAuth into eventStatus. Add a unit test for a leading changed with only changed rows in the history.

Suggestion — status filter semantics changed (internal/api/v2/v2.go, validateAndSetStatus + GetEventsHandler)

Filters are applied in the DB against raw stored values, but the response now returns normalized ones. Filtering by status=analyzing (or scheduled, SYSTEM) can still match rows, yet the response shows analysing/planned/resolved/completed instead of the value the caller asked for — and status=SYSTEM will never appear in a response. Clients that compare filter input to output values (or re-filter client-side) will see a mismatch. Consider either documenting this in docs/events.md or normalizing the query parameter to its canonical form before the DB filter.

Suggestion — unauthenticated description leak edge case (internal/api/v2/v2.go, ~lines 1872–1880)

latestDescription is now captured only for rows that survive the isInternalStatus filter, so a description row whose text is non-empty is safe. But if a public description row exists with empty text and the stored inc.Description is non-empty, unauthenticated callers get "" while authenticated ones fall back to the stored description (only if an internal description row is filtered out — verify the ordering intent). This is likely acceptable (an empty description row deliberately means "clear the description", per the PR description), just worth a comment in toAPIEvent so the latestDescription != "" check isn't mistaken for a bug later.

Suggestion — mapEventUpdates/eventStatus duplicate the collapse logic

Both functions re-derive "what is the previous real status" independently. Keep them in sync deliberately (the collapse rules are already subtly different, see above) or factor out a helper so a future change to normalization cannot silently desync the status field from the updates[] tail.

Notes

• tests/v2_events_test.go: the OutDatedSystem → IncidentResolved assertion change is the correct consequence of normalizing SYSTEM with an end date at the output boundary; good that the integration test was updated.
• The test additions in v2_visibility_test.go cover the main branches (SYSTEM with/without end date per event type, description extraction, leading changed seeding) — the gaps above are the only uncovered paths I found.
• CI checks were still queued at review time; the correctness of the eventStatus history walk (lines were only partially visible) is based on the patch and surrounding tests rather than a full read.

[ecosquad-autoreview]

Review — PR #48: status normalization + description contract change

The change is well-structured: normalizeStatus is applied at the API boundary, mapEventUpdates is covered by new unit tests, and the DB layer is untouched. CI hasn't run yet (queued), so findings below are from reading only.

Suggestion

1. isCancelledWithoutPublicStatus operates on raw (pre-normalization) status — internal/api/v2/v2.go (~line 230 in GetIncidentHandler, and wherever else it's applied). It compares r.Status / raw update statuses against canonical constants. Since legacy values like "in progress" or "SYSTEM" remain in the DB and are only normalized in toAPIEvent, an unauthenticated detail request on a cancelled event whose updates contain only legacy-spelled public statuses may not be classified as "cancelled without public status" (or vice versa). This is pre-existing behavior, but normalization makes the two code paths (list/detail 404 gates vs. rendered output) diverge more. Consider normalizing inside isCancelledWithoutPublicStatus as well, or at least add a test pinning the interaction.

2. updates[last].status == event.status invariant only holds for the authenticated view — the new test TestEventStatus_MatchesLastUpdate calls mapEventUpdates(..., true, ...). For isAuth == false, trailing internal rows (e.g. pending_review/reviewed for maintenance) are filtered from updates[], so the last visible update can differ from eventStatus(inc), which is computed from the full history. That's probably fine, but it's worth a comment or a second test documenting that the invariant is auth-dependent, so a future change doesn't accidentally assume it universally.

3. Description overwrites the stored description when the latest row's text is empty — toAPIEvent (internal/api/v2/v2.go:~334): if latestDescription != "" guards this — wait, re-reading: the guard is latestDescription != "", so an empty description row does not overwrite the stored one. The PR body says "an empty description row yields an empty description" — that contradicts the code as written (the != "" check falls through to the stored description). Please reconcile the PR description with the code/test; the code behavior is the safer one and I'd keep it.

4. eventStatus re-runs the history walk that mapEventUpdates already does — toAPIEvent calls both mapEventUpdates and eventStatus(inc), each walking inc.Statuses and calling normalizeStatus per row. Negligible perf, but eventStatus could be derived from the mapping result (last emitted update, seeded from normalized event status) instead of re-implementing the collapse logic, which removes the risk of the two implementations drifting apart.

What I could not verify

• go build / go test — CI is queued at review time; the diff looks self-consistent and tests compile against the new signature.
• Whether any client relies on updates[].id stability (PR notes resequencing; no known consumer).
• Exact contents of isCancelledWithoutPublicStatus (file truncated before it) — finding 1 is flagged accordingly.

Verdict

No correctness, security or data-loss bug found in what I read; the flagged items are contract/documentation consistency issues.

[ecosquad-autoreview]

Summary

This PR normalizes non-canonical status values at the API output boundary (normalizeStatus), collapses changed/impact changed rows to the previous status (eventStatus), and moves description rows out of updates[] into the event description field (mapEventUpdates). The logic is reasonable and well-tested in v2_visibility_test.go, and the DB layer is deliberately left untouched.

I could not run the build or tests: CI (go-test, go-test-acc, build, ent-codegen, golangci-lint) is all queued, not yet run. Please confirm those pass before merging — I'm flagging a few contract/consistency concerns below that a test suite would not catch on its own.

Findings

Warning — event-level status is now normalized everywhere, a broad contract change

internal/api/v2/v2.go toAPIEvent (~line 348): Status: eventStatus(inc) replaces the previous Status: inc.Status. This means every event output (list, detail, extract) now emits canonical values instead of the stored raw values — e.g. an incident stored as analyzing returns analysing, and a SYSTEM event with an end date returns resolved/completed.

• The PR description calls out the updates[] and description changes, but the event-level status change is the widest blast radius here. Confirm no API consumer (frontend Status.Trans.V2.ts, monitoring, caches) does a switch/match on the raw stored values. If the frontend still maps analyzing/SYSTEM itself, those branches now become dead or could double-map.
• The comment at v2.go:64 (// Status does not take into account OutDatedSystem status.) is now stale/incorrect — eventStatus does map SYSTEM (with end date) to a terminal status. Update or remove it.

Warning — leading changed/impact changed row collapses to changed/impact changed

In mapEventUpdates (~line 1872), lastStatus is seeded from the raw eventStatus argument, which toAPIEvent passes as inc.Status. So when the event's own stored status is changed or impact changed, a leading such row collapses to lastStatus == "changed"/"impact changed" — a non-canonical value — rather than a real state. Meanwhile the event-level Status field uses eventStatus(inc) which does collapse. This is a minor inconsistency: updates[0].status can be a non-canonical changed while event.status is collapsed. Fix by seeding lastStatus from eventStatus(inc) (the collapsed value) instead of the raw inc.Status, or by passing the collapsed status into mapEventUpdates.

Suggestion — description extraction only after the visibility filter

mapEventUpdates captures latestDescription only for rows that pass the isInternalStatus filter (description rows aren't internal, so in practice this is fine). Worth a one-line comment confirming the intent, and a test covering the case where a description row is the only row — currently the description fallback to inc.Description is untested.

Suggestion — resequencing note

The PR contract note about updates[].id resequencing is accurate but low-risk: IDs were already sequential idx values (not DB IDs) before this change, so removing a description row doesn't change the meaning of existing IDs — it only shifts subsequent ones. No action needed; just confirming I checked.

Verdict

No blocking correctness/security/data-loss bugs found in the diff, so I'm approving with the above follow-ups. Please (1) confirm the queued CI green, and (2) address the two warnings — the broad event-level status contract change and the leading-changed seeding inconsistency — before this ships to the frontend.