# PR #304 Code Review — `fix(competitions): correct status state machine + expose dates on MyCompetitionItem` **Reviewer:** tele (coordinator side) **Date:** 2026-04-21 **PR:** https://github.com/NanoStreetApp/backend/pull/304 **HEAD:** `6f6f9d46` • Branch `fix/competition-status-state-machine` **Diff:** 5 files, +469/-24 (single commit, well-scoped) --- ## TL;DR — APPROVE. Ship it. Clean, well-scoped, well-tested fix for the exact bug mobile hit (status='upcoming' past start_date). Adds `_derive_status()` computed-on-read helper, applied at all 4 read sites AND 4 mutation guards — no stale-status leaks. Also extends `MyCompetitionItem` with `start_date` + `end_date` per user's explicit request, unblocking mobile from rendering upcoming/active/ended nuance without a second fetch. Cron retained for the one-shot end-of-competition lifecycle. Test coverage is strong (376-line new test file + 2 existing tests updated). No blockers. Ready to flip from draft to ready-for-review. --- ## What the PR does 1. **New helper:** `_derive_status(start_date, end_date, today=None)` in `simtrade_competition_service.py` — pure function, documented state machine: - `upcoming` — `start_date > today` - `active` — `start_date <= today < end_date` - `ended` — `today >= end_date` 2. **Read-site application (4 sites)** — always returns fresh status to clients: - `list_competitions` (both `my_competitions` and `available_competitions` paths) - `get_competition` (single) - `resolve_invite_code` (invite flow returns current status) - `leave_competition` (pre-check derives fresh) 3. **Mutation-guard application (4 sites)** — no stale-DB-status loophole lets bad transitions slip through: - `update_competition` — can't change dates on an active (per-derived) comp - `delete_competition` — only-upcoming guard uses derived status - `join_competition` — blocks active/ended (derived) - `leave_competition` — blocks active/ended (derived) 4. **DTO extension:** `MyCompetitionItem` (`app/models/simtrade.py`) gains required `start_date: date` + `end_date: date` fields. Parity with `AvailableCompetitionItem`. Mobile can now render proper state differentiation. 5. **Cron retained:** `run_status_transitions` daily task still handles the one-shot end-of-competition lifecycle (snapshot, reconcile, liquidate, rank). Only the client-surfaced status + guard checks are now computed-on-read. Good architecture — cron keeps DB consistent in the background, read path is always fresh. 6. **Tests:** - New `tests/unit/test_competition_status_derivation.py` (376 lines): `TestDeriveStatus` with 8 boundary/edge cases (upcoming/active/ended + `start==today`, `end==today`, `start==end==today`) + test classes for each read site + mutation guard - Updated `tests/unit/test_competition_join_leave_sp2.py`: existing mocks now include `start_date`/`end_date` so the derived-status path runs through correctly - Updated `tests/unit/test_simtrade_competition_funding.py`: mock factory now generates consistent dates per `status` arg (nice touch — reduces test fragility) --- ## What I checked (positive findings) - **Purity** — helper has no side effects, takes `today` as a parameter for deterministic testing - **Docstring quality** — clear state-machine inline in the helper's docstring, explains the retention of the cron in the module-level notes - **Boundary correctness** — `start == today` falls into `active` (documented + tested); `today == end` falls into `ended` (documented + tested) - **Coverage completeness** — every place that previously read `comp["status"]` has been audited. No stale leaks. - **Mutation-guard symmetry** — all 4 guards derive, so user can't join/leave/update/delete via a stale-status loophole - **Cron not broken** — `run_status_transitions` retained for the lifecycle-action side of things (snapshot, reconcile, liquidate, rank). Good architectural call. - **PR body quality** — Why / What / How (with a table of changed sites) / Test plan — human-team review-friendly, cites the mobile PR #166 band-aid for context, explicitly calls out what's NOT changed (cron behavior). --- ## Minor observations (non-blocking) 1. **`date.today()` timezone semantics** — uses Python's local-system-time default. If the backend's deployment container runs in UTC but "today" intent is ET (market calendar), there's a sub-day edge around midnight UTC. However, the **existing** cron + `_ends_in_days()` use the same `date.today()` semantics, so this PR matches the existing convention. Not a new bug. If team ever wants to fix: migrate all `date.today()` call sites to an ET-aware helper as a separate cleanup PR. 2. **Helper scope** — `_derive_status` is private to `simtrade_competition_service.py` (underscore prefix). If it's ever needed by another service or a new endpoint, promote to a shared module. Fine for now. 3. **`_to_date` coercion helper** — wraps `date.fromisoformat`. Small cleanup, replaces 3 prior inline `isinstance(..., str) and date.fromisoformat(...)` patterns. Nice dedup. 4. **DTO fields are `required`** — `start_date` / `end_date` are declared as non-optional. Backend rollout needs to land before mobile updates its DTO consumer to avoid a mid-flight runtime mismatch (clients expecting new fields from stale server or vice versa). Standard staged-rollout caution; PR description mentions mobile PR #166 as band-aid which already ships. None of these are blockers. --- ## Test plan validation - ✅ New `test_competition_status_derivation.py` — 376 LOC covers state machine + every read site + every mutation guard with mocked responses - ✅ Existing tests updated with consistent `start_date`/`end_date` mocks (prevents false pass where mocked `status` doesn't match derived) - ✅ Fixture factory in `test_simtrade_competition_funding.py` derives dates from requested status — avoids the per-test "set up valid dates" boilerplate - ✅ Cron test (if any for `run_status_transitions`) unaffected — cron logic untouched - PR description mentions the test plan checklist visually — matches what's in the code Haven't run the tests myself (read-only review), but test structure + coverage patterns look correct per backend conventions. Agent reported `make test-unit` + local curl passing. --- ## Recommendation **APPROVE. Flip to ready-for-review.** No further code changes needed. Backend team can review + merge at their pace. Once merged, mobile follows up with a small DTO-consumption PR to use the new `start_date`/`end_date` fields for a proper client-side `startDate < today` check (removing the `endsInDays`-only fallback from PR #166's Option C).