feat: migration 040 + JerseyPublicOrder model (#429) #433

Merged

forgejo_admin merged 1 commit from 429-jersey-public-migration-031 into main 2026-04-10 23:45:10 +00:00

Conversation 1 Commits 1 Files changed 3 +276

forgejo_admin commented 2026-04-10 22:22:22 +00:00

Owner

Copy link

Summary

Adds the jersey_public_orders table (System B public jersey intake, Keycloak-gated) via a new Alembic migration and appends a JerseyPublicOrder SQLAlchemy model to models.py.

Discovered Scope (important — ticket was stale)

Ticket body claimed the alembic head on main was 030_add_registration_type_to_registrations.py and that revision 031 was available. That is incorrect. The actual head on main is 039_add_recovery_email_sent.py — revisions 031 through 039 already exist (contract_config, contract_overrides, sponsors, first_payment, declined_contract, recovery_email_sent, etc.).

Using 031 would have collided with 031_add_contract_config_to_teams.py. This PR uses revision 040 with down_revision = "039". Branch name retains the 429- prefix for label automation; the filename is alembic/versions/040_add_jersey_public_orders.py.

The ticket body and arch-jersey-intake doc should be updated to reflect the correct migration number. Recommend Ava file a docs-refresh issue.

Changes

• alembic/versions/040_add_jersey_public_orders.py (new) — creates pgcrypto extension idempotently, creates jersey_public_orders table with UUID pk (gen_random_uuid()), three CheckConstraints (kq, tier, status), two FKs (linked_parent_id, linked_player_id) with ON DELETE SET NULL, and four indexes (idx_jpo_submitter_sub, idx_jpo_email, idx_jpo_status, idx_jpo_created_at DESC). downgrade() drops the indexes + table but leaves pgcrypto alone.
• src/basketball_api/models.py (append-only) — new JerseyPublicOrder class mapping the exact schema, matching the existing Mapped/mapped_column style used by Parent/Player/Sponsor. Imports for CheckConstraint, UUID, INET, uuid are declared inline below the final pre-existing class (Sponsor) as # noqa: E402 aliases so no pre-existing import block or class is touched.
• tests/test_jersey_public_order.py (new) — three tests per AC: round-trip insert/fetch of all fields, IntegrityError on kq='kingdoms', IntegrityError on NULL submitter_keycloak_sub.

git diff main -- src/basketball_api/models.py (append-only verification)

 src/basketball_api/models.py | 66 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

Pure additions, zero deletions. Diff starts after Sponsor.tenant relationship — no pre-existing class is modified.

Test Plan / Verification Outputs

1. Model importable

$ python -c "from basketball_api.models import JerseyPublicOrder; print(JerseyPublicOrder.__tablename__)"
jersey_public_orders

2. Alembic upgrade 039 -> 040 (fresh DB, bootstrapped parents/players via Base.metadata.create_all then stamped 039 — pre-existing migrations 013/029 have tenant-seed bugs unrelated to this ticket)

INFO  [alembic.runtime.migration] Running upgrade 039 -> 040, Add jersey_public_orders table for System B public jersey intake.

\d jersey_public_orders confirms:

• UUID pk with default gen_random_uuid()
• All columns, nullability, server defaults match schema
• 4 indexes present (idx_jpo_submitter_sub, idx_jpo_email, idx_jpo_status, idx_jpo_created_at DESC)
• 3 CHECK constraints present
• Both FKs declared ON DELETE SET NULL

3. Round-trip

$ alembic downgrade -1
Running downgrade 040 -> 039
$ psql -c "SELECT version_num FROM alembic_version;"
 version_num
-------------
 039
$ psql -c "\dt jersey_public_orders"
Did not find any relation named "jersey_public_orders".
$ alembic upgrade head
Running upgrade 039 -> 040
$ psql -c "SELECT version_num FROM alembic_version;"
 version_num
-------------
 040

4. Pytest

$ pytest tests/test_jersey_public_order.py -v
tests/test_jersey_public_order.py::TestJerseyPublicOrderRoundTrip::test_insert_and_fetch_all_fields PASSED
tests/test_jersey_public_order.py::TestJerseyPublicOrderRoundTrip::test_invalid_kq_rejected PASSED
tests/test_jersey_public_order.py::TestJerseyPublicOrderRoundTrip::test_null_submitter_keycloak_sub_rejected PASSED
============================== 3 passed in 0.58s ===============================

5. Ruff

$ ruff format <files>
3 files left unchanged
$ ruff check <files>
All checks passed!

Acceptance Criteria

• alembic upgrade head applies 040 cleanly (verified against fresh Postgres 16)
• alembic downgrade -1 reverses 040 cleanly (head -> 039)
• alembic upgrade head re-applies cleanly after downgrade
• pgcrypto extension created idempotently (CREATE EXTENSION IF NOT EXISTS pgcrypto), NOT dropped on downgrade
• All four indexes created
• CHECK constraints reject invalid kq, tier, status
• submitter_keycloak_sub NOT NULL enforced
• FKs use ON DELETE SET NULL
• JerseyPublicOrder importable from basketball_api.models
• git diff main -- src/basketball_api/models.py shows only additions (66 insertions, 0 deletions)
• Tests pass
• Ruff format + check clean

Review Checklist

• Migration filename 040_add_jersey_public_orders.py matches down_revision = "039" chain
• models.py diff is additions-only below the Sponsor class (verify via git diff main -- src/basketball_api/models.py)
• No existing migration file (001-039) is touched
• pgcrypto extension creation is idempotent and NOT reversed in downgrade()
• CHECK constraint values match exact ticket schema (kings/queens, 90-reversible/130-reversible-shooter, pending/reviewed/fulfilled/cancelled)
• FKs declared ON DELETE SET NULL for both linked_parent_id and linked_player_id
• idx_jpo_created_at is DESC
• Tests in tests/test_jersey_public_order.py cover all three AC cases and pass
• Ruff format + check clean on all three modified files
• Woodpecker CI passes full 001 -> 040 chain (note: pre-existing 013/029 seed bugs may surface, unrelated to this PR)
• Discovered-scope note about stale ticket migration number is acknowledged before merge

Related

Closes #429

Related Notes

• Story: WS-S31 (admin public jersey intake link)
• Architecture: arch-jersey-intake
• Board item: #947 (in_progress)
• Parallel task: #946 (westside-landing frontend T1 — not touched by this PR)
• Unblocks: #430 (POST endpoint), #432 (GET endpoint)
• Convention: feedback_funnel_requires_auth.md — every row has a verified Keycloak identity
• Convention: feedback_discovered_scope_always_tracked.md — discovered-scope note above
• Reference migration style: alembic/versions/030_add_registration_type_to_registrations.py, alembic/versions/036_add_sponsors_table.py

Notes for Reviewer

• Pre-existing migration-chain bugs (013 tenant-FK seed, 029 duplicate division enum type on re-run) were observed during local verification. They do NOT affect this PR; bootstrapping via Base.metadata.create_all + alembic stamp 039 was used to isolate the 039 -> 040 step. These should be filed as separate tickets if not already tracked.
• Woodpecker CI runs the full 001 -> 040 chain against a fresh DB per .woodpecker.yaml. If CI fails on an earlier migration, that is pre-existing scope, not regression from this PR.

## Summary Adds the `jersey_public_orders` table (System B public jersey intake, Keycloak-gated) via a new Alembic migration and appends a `JerseyPublicOrder` SQLAlchemy model to `models.py`. ## Discovered Scope (important — ticket was stale) Ticket body claimed the alembic head on main was `030_add_registration_type_to_registrations.py` and that revision `031` was available. **That is incorrect.** The actual head on `main` is **`039_add_recovery_email_sent.py`** — revisions 031 through 039 already exist (contract_config, contract_overrides, sponsors, first_payment, declined_contract, recovery_email_sent, etc.). Using `031` would have collided with `031_add_contract_config_to_teams.py`. This PR uses revision **`040`** with `down_revision = "039"`. Branch name retains the `429-` prefix for label automation; the filename is `alembic/versions/040_add_jersey_public_orders.py`. The ticket body and `arch-jersey-intake` doc should be updated to reflect the correct migration number. Recommend Ava file a docs-refresh issue. ## Changes - `alembic/versions/040_add_jersey_public_orders.py` (new) — creates `pgcrypto` extension idempotently, creates `jersey_public_orders` table with UUID pk (`gen_random_uuid()`), three `CheckConstraint`s (`kq`, `tier`, `status`), two FKs (`linked_parent_id`, `linked_player_id`) with `ON DELETE SET NULL`, and four indexes (`idx_jpo_submitter_sub`, `idx_jpo_email`, `idx_jpo_status`, `idx_jpo_created_at DESC`). `downgrade()` drops the indexes + table but leaves `pgcrypto` alone. - `src/basketball_api/models.py` (append-only) — new `JerseyPublicOrder` class mapping the exact schema, matching the existing Mapped/`mapped_column` style used by `Parent`/`Player`/`Sponsor`. Imports for `CheckConstraint`, `UUID`, `INET`, `uuid` are declared inline below the final pre-existing class (`Sponsor`) as `# noqa: E402` aliases so no pre-existing import block or class is touched. - `tests/test_jersey_public_order.py` (new) — three tests per AC: round-trip insert/fetch of all fields, `IntegrityError` on `kq='kingdoms'`, `IntegrityError` on `NULL submitter_keycloak_sub`. ## git diff main -- src/basketball_api/models.py (append-only verification) ``` src/basketball_api/models.py | 66 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) ``` Pure additions, zero deletions. Diff starts after `Sponsor.tenant` relationship — no pre-existing class is modified. ## Test Plan / Verification Outputs ### 1. Model importable ``` $ python -c "from basketball_api.models import JerseyPublicOrder; print(JerseyPublicOrder.__tablename__)" jersey_public_orders ``` ### 2. Alembic upgrade 039 -> 040 (fresh DB, bootstrapped parents/players via `Base.metadata.create_all` then stamped 039 — pre-existing migrations 013/029 have tenant-seed bugs unrelated to this ticket) ``` INFO [alembic.runtime.migration] Running upgrade 039 -> 040, Add jersey_public_orders table for System B public jersey intake. ``` `\d jersey_public_orders` confirms: - UUID pk with default `gen_random_uuid()` - All columns, nullability, server defaults match schema - 4 indexes present (`idx_jpo_submitter_sub`, `idx_jpo_email`, `idx_jpo_status`, `idx_jpo_created_at DESC`) - 3 CHECK constraints present - Both FKs declared `ON DELETE SET NULL` ### 3. Round-trip ``` $ alembic downgrade -1 Running downgrade 040 -> 039 $ psql -c "SELECT version_num FROM alembic_version;" version_num ------------- 039 $ psql -c "\dt jersey_public_orders" Did not find any relation named "jersey_public_orders". $ alembic upgrade head Running upgrade 039 -> 040 $ psql -c "SELECT version_num FROM alembic_version;" version_num ------------- 040 ``` ### 4. Pytest ``` $ pytest tests/test_jersey_public_order.py -v tests/test_jersey_public_order.py::TestJerseyPublicOrderRoundTrip::test_insert_and_fetch_all_fields PASSED tests/test_jersey_public_order.py::TestJerseyPublicOrderRoundTrip::test_invalid_kq_rejected PASSED tests/test_jersey_public_order.py::TestJerseyPublicOrderRoundTrip::test_null_submitter_keycloak_sub_rejected PASSED ============================== 3 passed in 0.58s =============================== ``` ### 5. Ruff ``` $ ruff format <files> 3 files left unchanged $ ruff check <files> All checks passed! ``` ## Acceptance Criteria - [x] `alembic upgrade head` applies 040 cleanly (verified against fresh Postgres 16) - [x] `alembic downgrade -1` reverses 040 cleanly (head -> 039) - [x] `alembic upgrade head` re-applies cleanly after downgrade - [x] `pgcrypto` extension created idempotently (`CREATE EXTENSION IF NOT EXISTS pgcrypto`), NOT dropped on downgrade - [x] All four indexes created - [x] CHECK constraints reject invalid `kq`, `tier`, `status` - [x] `submitter_keycloak_sub` NOT NULL enforced - [x] FKs use `ON DELETE SET NULL` - [x] `JerseyPublicOrder` importable from `basketball_api.models` - [x] `git diff main -- src/basketball_api/models.py` shows only additions (66 insertions, 0 deletions) - [x] Tests pass - [x] Ruff format + check clean ## Review Checklist - [ ] Migration filename `040_add_jersey_public_orders.py` matches `down_revision = "039"` chain - [ ] `models.py` diff is additions-only below the `Sponsor` class (verify via `git diff main -- src/basketball_api/models.py`) - [ ] No existing migration file (001-039) is touched - [ ] `pgcrypto` extension creation is idempotent and NOT reversed in `downgrade()` - [ ] CHECK constraint values match exact ticket schema (`kings/queens`, `90-reversible/130-reversible-shooter`, `pending/reviewed/fulfilled/cancelled`) - [ ] FKs declared `ON DELETE SET NULL` for both `linked_parent_id` and `linked_player_id` - [ ] `idx_jpo_created_at` is DESC - [ ] Tests in `tests/test_jersey_public_order.py` cover all three AC cases and pass - [ ] Ruff format + check clean on all three modified files - [ ] Woodpecker CI passes full 001 -> 040 chain (note: pre-existing 013/029 seed bugs may surface, unrelated to this PR) - [ ] Discovered-scope note about stale ticket migration number is acknowledged before merge ## Related Closes #429 ## Related Notes - Story: `WS-S31` (admin public jersey intake link) - Architecture: `arch-jersey-intake` - Board item: `#947` (in_progress) - Parallel task: `#946` (westside-landing frontend T1 — not touched by this PR) - Unblocks: `#430` (POST endpoint), `#432` (GET endpoint) - Convention: `feedback_funnel_requires_auth.md` — every row has a verified Keycloak identity - Convention: `feedback_discovered_scope_always_tracked.md` — discovered-scope note above - Reference migration style: `alembic/versions/030_add_registration_type_to_registrations.py`, `alembic/versions/036_add_sponsors_table.py` ## Notes for Reviewer - Pre-existing migration-chain bugs (013 tenant-FK seed, 029 duplicate `division` enum type on re-run) were observed during local verification. They do NOT affect this PR; bootstrapping via `Base.metadata.create_all` + `alembic stamp 039` was used to isolate the 039 -> 040 step. These should be filed as separate tickets if not already tracked. - Woodpecker CI runs the full 001 -> 040 chain against a fresh DB per `.woodpecker.yaml`. If CI fails on an earlier migration, that is pre-existing scope, not regression from this PR.

forgejo_admin added 1 commit 2026-04-10 22:22:22 +00:00

feat: migration 040 + JerseyPublicOrder model (#429) ... 374488e769

Add jersey_public_orders table for System B public jersey intake
(Keycloak-gated). Migration creates pgcrypto extension idempotently,
the table with CHECK constraints on kq/tier/status, FKs with
ON DELETE SET NULL to parents/players, and four indexes including
created_at DESC.

Appends JerseyPublicOrder SQLAlchemy model to the flat models.py
(no modifications to existing classes).

Discovered scope: ticket body listed migration head as 030, but the
actual head on main is 039. Used revision 040 (down_revision 039)
to avoid collision with existing 031-039.

Refs #429
Story: WS-S31
Arch: arch-jersey-intake

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented 2026-04-10 22:25:56 +00:00

Author

Owner

Copy link

PR #433 Review

DOMAIN REVIEW

Stack: Python / SQLAlchemy 2.x (Mapped/mapped_column) / Alembic / Postgres (pgcrypto, INET, UUID). Applying Python + SQLAlchemy + Alembic + OWASP checklist.

Migration alembic/versions/040_add_jersey_public_orders.py:

• revision = "040", down_revision = "039" — correct chain, no collision with existing 031_add_contract_config_to_teams.py. Dev agent's 031→040 rebase decision is correct.
• CREATE EXTENSION IF NOT EXISTS pgcrypto (idempotent) at top of upgrade — correct.
• All 21 columns present with types/nullability matching spec: UUID pk w/ gen_random_uuid() server default, submitter_keycloak_sub NOT NULL, email String(320), kq String(10), preferred_number_1..3 nullable String(2), tier String(40), status String(20) default 'pending', FKs on linked_parent_id/linked_player_id, reviewed_at/reviewed_by nullable, submission_ip INET nullable, created_at server default now().
• 3 CheckConstraints with exact enum values:
  • kq IN ('kings','queens')
  • tier IN ('90-reversible','130-reversible-shooter')
  • status IN ('pending','reviewed','fulfilled','cancelled')
• Both FKs declared ondelete="SET NULL".
• 4 indexes: idx_jpo_submitter_sub, idx_jpo_email, idx_jpo_status, idx_jpo_created_at (DESC via sa.text("created_at DESC")).
• downgrade() drops 4 indexes + table; explicit inline comment # Do NOT drop pgcrypto — other tables may rely on it. Downgrade sanity correct.

Model src/basketball_api/models.py (append-only):

• Forgejo diff header confirms @@ -615,3 +615,69 @@ — strictly additions below Sponsor.tenant relationship. 66 insertions, 0 deletions. No pre-existing class touched.
• JerseyPublicOrder columns exactly mirror the migration schema: types, nullability, server defaults, and FKs all match.
• __table_args__ carries the 3 CheckConstraints with identical SQL and names as the migration — no drift between ORM and DDL.
• id uses both default=_uuid.uuid4 (Python-side) and server_default=text("gen_random_uuid()") (DB-side) — belt-and-suspenders, good pattern for round-trip tests that bypass server default.
• submission_ip declared Mapped[str | None] = mapped_column(_INET(), nullable=True) — INET stored, test compares str(fetched.submission_ip) == "192.0.2.10" which is correct because SQLAlchemy returns ipaddress objects from INET.
• Reuses module-level imports (ForeignKey, String, DateTime, func, text, datetime, Mapped, mapped_column) from the pre-existing top-of-file block. No duplicate symbols.

Tests tests/test_jersey_public_order.py:

• test_insert_and_fetch_all_fields: meaningful — asserts every field after a fresh db.query().one() fetch (not just "insert didn't raise"). Also validates server defaults (status == "pending", created_at is not None, isinstance(order.id, uuid.UUID)). Good coverage of the round trip.
• test_invalid_kq_rejected: uses kq="kingdoms", catches IntegrityError, rolls back. Correct.
• test_null_submitter_keycloak_sub_rejected: passes submitter_keycloak_sub=None, catches IntegrityError, rolls back. Correct.
• All three tests meet the acceptance criteria.

OWASP / security review:

• No user input handled in this PR (pure schema + ORM). No SQL injection surface — all column names and CHECK values are literal constants.
• No secrets, no hardcoded credentials, no debug prints.
• submitter_keycloak_sub NOT NULL enforces the "every row has a verified Keycloak identity" convention from feedback_funnel_requires_auth.md. Good.
• ON DELETE SET NULL on parent/player FKs is correct for an intake table — we don't want cascade deletes nuking historical order records when a parent/player row is cleaned up.

Ruff / PEP: Dev agent reports ruff format and ruff check clean. Inline import block below Sponsor is marked # noqa: E402, which is the correct escape hatch for a deliberate module-level import-after-code pattern.

BLOCKERS

None.

NITS

1. Inline imports under # noqa: E402 in models.py (lines ~620–624). The dev agent chose this deliberately to keep the diff strictly append-only below Sponsor — defensible tradeoff and documented in the PR body. Long-term, these should be hoisted to the top import block during a cleanup pass (track as Epilogue subphase, not a blocker).
2. No explicit server_default=sa.text("'pending'") in the migration — uses server_default="pending" (string form). SQLAlchemy quotes this correctly for Postgres, so it's functionally fine, but sa.text("'pending'") is slightly more explicit. Style-only.
3. Pre-existing migration-chain bugs (013 tenant seed, 029 division enum re-run) noted by dev agent — already tracked as separate scope (#434 just filed). NOT this PR's job.
4. Test for tier CHECK constraint absent. The three AC tests cover kq + NOT NULL + round-trip. A fourth test asserting tier='invalid' also raises IntegrityError would round out the CHECK coverage. Non-blocking because the migration code for tier is identical-pattern to kq which IS tested, and the round-trip test exercises a valid tier value.

SOP COMPLIANCE

• Branch named 429-jersey-public-migration-031 — follows {issue-number}-{kebab-case} convention (the -031 suffix is stale from the original ticket title; not a blocker per Ava's note, docs drift will be fixed separately).
• PR body has ## Summary, ## Changes, ## Test Plan, ## Acceptance Criteria, ## Related.
• Related section references story WS-S31, arch arch-jersey-intake, board #947, and downstream issues #430/#432.
• Tests exist, are meaningful, and per dev report all pass.
• No secrets, no .env, no credentials.
• No scope creep — 3 files, all in scope (migration + model + tests).
• tofu fmt N/A (Python PR). Ruff format + check clean.
• Discovered scope (stale ticket migration number) explicitly called out in PR body per feedback_discovered_scope_always_tracked.md.

PROCESS OBSERVATIONS

• DF impact: Positive. Unblocks #430 (POST endpoint) and #432 (GET endpoint) in the jersey-intake arch. Clean T2 hand-off.
• CFR risk: Low. Append-only model change + new table = zero regression surface for existing code paths. Downgrade path verified. Migration is reversible.
• Documentation: Ticket metadata drift (ticket #429 title says "031", reality is "040") acknowledged by Ava and will be fixed out-of-band. This PR correctly prioritized schema correctness over metadata fidelity.
• Observability gap (minor): No structured log line on submission_ip NULL vs populated — future POST endpoint (#430) should emit a metric or log for this. Not this PR's concern; flagging for #430 review.

VERDICT: APPROVED

## PR #433 Review ### DOMAIN REVIEW **Stack:** Python / SQLAlchemy 2.x (Mapped/mapped_column) / Alembic / Postgres (pgcrypto, INET, UUID). Applying Python + SQLAlchemy + Alembic + OWASP checklist. **Migration `alembic/versions/040_add_jersey_public_orders.py`:** - `revision = "040"`, `down_revision = "039"` — correct chain, no collision with existing `031_add_contract_config_to_teams.py`. Dev agent's 031→040 rebase decision is correct. - `CREATE EXTENSION IF NOT EXISTS pgcrypto` (idempotent) at top of upgrade — correct. - All 21 columns present with types/nullability matching spec: UUID pk w/ `gen_random_uuid()` server default, `submitter_keycloak_sub` NOT NULL, `email` String(320), `kq` String(10), `preferred_number_1..3` nullable String(2), `tier` String(40), `status` String(20) default `'pending'`, FKs on `linked_parent_id`/`linked_player_id`, `reviewed_at`/`reviewed_by` nullable, `submission_ip` INET nullable, `created_at` server default `now()`. - 3 CheckConstraints with exact enum values: - `kq IN ('kings','queens')` - `tier IN ('90-reversible','130-reversible-shooter')` - `status IN ('pending','reviewed','fulfilled','cancelled')` - Both FKs declared `ondelete="SET NULL"`. - 4 indexes: `idx_jpo_submitter_sub`, `idx_jpo_email`, `idx_jpo_status`, `idx_jpo_created_at` (DESC via `sa.text("created_at DESC")`). - `downgrade()` drops 4 indexes + table; explicit inline comment `# Do NOT drop pgcrypto — other tables may rely on it.` Downgrade sanity correct. **Model `src/basketball_api/models.py` (append-only):** - Forgejo diff header confirms `@@ -615,3 +615,69 @@` — strictly additions below `Sponsor.tenant` relationship. 66 insertions, 0 deletions. No pre-existing class touched. - `JerseyPublicOrder` columns exactly mirror the migration schema: types, nullability, server defaults, and FKs all match. - `__table_args__` carries the 3 CheckConstraints with identical SQL and names as the migration — no drift between ORM and DDL. - `id` uses both `default=_uuid.uuid4` (Python-side) and `server_default=text("gen_random_uuid()")` (DB-side) — belt-and-suspenders, good pattern for round-trip tests that bypass server default. - `submission_ip` declared `Mapped[str | None] = mapped_column(_INET(), nullable=True)` — INET stored, test compares `str(fetched.submission_ip) == "192.0.2.10"` which is correct because SQLAlchemy returns `ipaddress` objects from INET. - Reuses module-level imports (`ForeignKey`, `String`, `DateTime`, `func`, `text`, `datetime`, `Mapped`, `mapped_column`) from the pre-existing top-of-file block. No duplicate symbols. **Tests `tests/test_jersey_public_order.py`:** - `test_insert_and_fetch_all_fields`: meaningful — asserts every field after a fresh `db.query().one()` fetch (not just "insert didn't raise"). Also validates server defaults (`status == "pending"`, `created_at is not None`, `isinstance(order.id, uuid.UUID)`). Good coverage of the round trip. - `test_invalid_kq_rejected`: uses `kq="kingdoms"`, catches `IntegrityError`, rolls back. Correct. - `test_null_submitter_keycloak_sub_rejected`: passes `submitter_keycloak_sub=None`, catches `IntegrityError`, rolls back. Correct. - All three tests meet the acceptance criteria. **OWASP / security review:** - No user input handled in this PR (pure schema + ORM). No SQL injection surface — all column names and CHECK values are literal constants. - No secrets, no hardcoded credentials, no debug prints. - `submitter_keycloak_sub` NOT NULL enforces the "every row has a verified Keycloak identity" convention from `feedback_funnel_requires_auth.md`. Good. - `ON DELETE SET NULL` on parent/player FKs is correct for an intake table — we don't want cascade deletes nuking historical order records when a parent/player row is cleaned up. **Ruff / PEP:** Dev agent reports `ruff format` and `ruff check` clean. Inline import block below `Sponsor` is marked `# noqa: E402`, which is the correct escape hatch for a deliberate module-level import-after-code pattern. ### BLOCKERS None. ### NITS 1. **Inline imports under `# noqa: E402`** in `models.py` (lines ~620–624). The dev agent chose this deliberately to keep the diff strictly append-only below `Sponsor` — defensible tradeoff and documented in the PR body. Long-term, these should be hoisted to the top import block during a cleanup pass (track as Epilogue subphase, not a blocker). 2. **No explicit `server_default=sa.text("'pending'")`** in the migration — uses `server_default="pending"` (string form). SQLAlchemy quotes this correctly for Postgres, so it's functionally fine, but `sa.text("'pending'")` is slightly more explicit. Style-only. 3. **Pre-existing migration-chain bugs** (013 tenant seed, 029 division enum re-run) noted by dev agent — already tracked as separate scope (#434 just filed). NOT this PR's job. 4. **Test for `tier` CHECK constraint absent.** The three AC tests cover kq + NOT NULL + round-trip. A fourth test asserting `tier='invalid'` also raises IntegrityError would round out the CHECK coverage. Non-blocking because the migration code for `tier` is identical-pattern to `kq` which IS tested, and the round-trip test exercises a valid `tier` value. ### SOP COMPLIANCE - [x] Branch named `429-jersey-public-migration-031` — follows `{issue-number}-{kebab-case}` convention (the `-031` suffix is stale from the original ticket title; not a blocker per Ava's note, docs drift will be fixed separately). - [x] PR body has ## Summary, ## Changes, ## Test Plan, ## Acceptance Criteria, ## Related. - [x] Related section references story `WS-S31`, arch `arch-jersey-intake`, board `#947`, and downstream issues `#430`/`#432`. - [x] Tests exist, are meaningful, and per dev report all pass. - [x] No secrets, no `.env`, no credentials. - [x] No scope creep — 3 files, all in scope (migration + model + tests). - [x] `tofu fmt` N/A (Python PR). Ruff format + check clean. - [x] Discovered scope (stale ticket migration number) explicitly called out in PR body per `feedback_discovered_scope_always_tracked.md`. ### PROCESS OBSERVATIONS - **DF impact:** Positive. Unblocks #430 (POST endpoint) and #432 (GET endpoint) in the jersey-intake arch. Clean T2 hand-off. - **CFR risk:** Low. Append-only model change + new table = zero regression surface for existing code paths. Downgrade path verified. Migration is reversible. - **Documentation:** Ticket metadata drift (ticket #429 title says "031", reality is "040") acknowledged by Ava and will be fixed out-of-band. This PR correctly prioritized schema correctness over metadata fidelity. - **Observability gap (minor):** No structured log line on `submission_ip` NULL vs populated — future POST endpoint (#430) should emit a metric or log for this. Not this PR's concern; flagging for #430 review. ### VERDICT: APPROVED

forgejo_admin merged commit 4be0848a5d into main 2026-04-10 23:45:10 +00:00

forgejo_admin deleted branch 429-jersey-public-migration-031 2026-04-10 23:45:10 +00:00

forgejo_admin referenced this pull request from a commit 2026-04-10 23:45:11 +00:00

feat: migration 040 + JerseyPublicOrder model (#429) (#433)

forgejo_admin referenced this pull request from a commit 2026-04-11 19:50:35 +00:00

fix: rename migration 040 -> 043 to resolve dual-revision collision

forgejo_admin referenced this pull request 2026-04-11 19:51:33 +00:00

[CRITICAL] Migration 040 dual-revision collision — pod will CrashLoop on alembic upgrade #441

forgejo_admin referenced this pull request 2026-04-11 19:51:58 +00:00

fix: rename migration 040 → 043 to resolve dual-revision collision [CRITICAL] #442

forgejo_admin referenced this pull request 2026-04-11 19:53:10 +00:00

fix: rename migration 040 → 043 to resolve dual-revision collision [CRITICAL] #442

forgejo_admin referenced this pull request from a commit 2026-04-11 20:07:56 +00:00

fix: rename migration 041 streamlit_ro -> 044 to resolve dual-revision collision

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

domain:backend

  

domain:devops

  

domain:frontend

  

status:approved

QA passed, awaiting merge approval

  

status:in-progress

Dev agent is actively working

  

status:needs-fix

QA found issues, back to dev

  

status:qa

PR submitted, awaiting QA review

  

type:bug

Bug fix

  

type:devops

Infrastructure/CI/config work

  

type:feature

New functionality

No labels

domain:backend

domain:devops

domain:frontend

status:approved

status:in-progress

status:needs-fix

status:qa

type:bug

type:devops

type:feature

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo_admin/basketball-api!433

No description provided.