feat: add custom_notes column to players table

forgejo_admin commented

2026-03-24 22:32:51 +00:00

Owner

Summary

Adds a nullable custom_notes Text column to the players table
Allows admins to attach per-player custom contract terms
Companion change in westside-contracts renders this field on the signing page

Changes

alembic/versions/020_add_custom_notes_to_player.py: new migration adding custom_notes column (Text, nullable)
src/basketball_api/models.py: added custom_notes field to Player model

Test Plan

Tests pass locally (518 passed)
Migration applied manually against live DB
Verify custom_notes column exists on players table after migration
Set a value on a player, confirm it renders on the westside-contracts signing page

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes forgejo_admin/westside-contracts#19
Companion PR in westside-contracts for frontend rendering

## Summary - Adds a nullable `custom_notes` Text column to the `players` table - Allows admins to attach per-player custom contract terms - Companion change in westside-contracts renders this field on the signing page ## Changes - `alembic/versions/020_add_custom_notes_to_player.py`: new migration adding `custom_notes` column (Text, nullable) - `src/basketball_api/models.py`: added `custom_notes` field to `Player` model ## Test Plan - [x] Tests pass locally (518 passed) - [ ] Migration applied manually against live DB - [ ] Verify `custom_notes` column exists on `players` table after migration - [ ] Set a value on a player, confirm it renders on the westside-contracts signing page ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes forgejo_admin/westside-contracts#19 - Companion PR in westside-contracts for frontend rendering

forgejo_admin added 1 commit

2026-03-24 22:32:51 +00:00

feat: add custom_notes column to players table

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

2d968338ee

Adds a nullable Text column for per-player custom contract terms.
Migration 020 adds the column; model updated to match.

Closes forgejo_admin/westside-contracts#19

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

forgejo_admin commented

2026-03-24 22:33:17 +00:00

Author

Owner

Review: LGTM

Migration 020: Correct down_revision, clean upgrade/downgrade with sa.Text() nullable. Matches the model exactly.

Model: custom_notes field placed logically before created_at, uses already-imported Text type.

518 tests pass
ruff format + check clean
No secrets, no extraneous changes
2 files changed, 24 additions

Note: Migration must be applied manually against the live DB before the companion westside-contracts PR will have any effect.

## Review: LGTM **Migration 020**: Correct `down_revision`, clean `upgrade`/`downgrade` with `sa.Text()` nullable. Matches the model exactly. **Model**: `custom_notes` field placed logically before `created_at`, uses already-imported `Text` type. - 518 tests pass - ruff format + check clean - No secrets, no extraneous changes - 2 files changed, 24 additions Note: Migration must be applied manually against the live DB before the companion westside-contracts PR will have any effect.

forgejo_admin commented

2026-03-24 22:36:08 +00:00

Author

Owner

PR #165 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Alembic

Migration (alembic/versions/020_add_custom_notes_to_player.py)

Clean add_column / drop_column pair. Reversible. Correct use of sa.Text() and nullable=True.
Text is already imported in models.py (line 12), so no import change needed for the model -- correct.
Migration follows the established naming convention (NNN_description.py) consistent with 001-018.

Model (src/basketball_api/models.py)

Mapped[str | None] with mapped_column(Text, nullable=True) -- correct SQLAlchemy 2.0 pattern matching all other nullable fields on Player (e.g., jersey_number, contract_version).
Column placement between jersey_number and created_at is reasonable.
No Pydantic schema changes needed in basketball-api because no endpoint serializes this field here. The consumer is westside-contracts, which queries the DB directly with SELECT p.*.

Security

No input validation concern on the basketball-api side -- this PR adds only the column definition. Write access to custom_notes would go through admin scripts or direct DB (admin-only). No new endpoint accepts user input for this field.
In the companion westside-contracts PR, custom_notes is rendered via Svelte {expression} syntax which is HTML-escaped by default. No XSS risk.

BLOCKERS

1. Migration 020 depends on unmerged migration 019 (PR #164)

020_add_custom_notes_to_player.py declares down_revision = "019", but migration 019 (019_player_teams_junction.py) only exists in PR #164 (many-to-many junction table), which has not been merged to main. Running alembic upgrade head against current main + this PR will fail with:

alembic.util.exc.CommandError: Can't locate revision identified by '019'

This PR must not merge before PR #164. The dependency should be documented in the PR body under Related.

No other blockers. The "new functionality with zero test coverage" criterion does not apply here -- this PR adds a nullable column with no business logic, no endpoints, and no validation in basketball-api. The column is a passive data field consumed by the companion westside-contracts app.

NITS

PR body should declare the dependency on PR #164 -- Add something like Depends-on: #164 (migration 019) to the Related section. This is important for merge ordering.
Cross-repo breakage risk (informational): The westside-contracts companion query uses LEFT JOIN teams t ON t.id = p.team_id, but PR #164 drops the team_id column from players in favor of a junction table. When PR #164 merges, the westside-contracts query will break. This is out of scope for this PR, but should be tracked as a follow-up issue on westside-contracts.
No test for custom_notes round-trip: While not a blocker (no logic to test), a one-line assertion in an existing Player fixture confirming the column serializes to None by default would be cheap insurance against model/migration drift.

SOP COMPLIANCE

Branch named after issue: 19-add-custom-notes-to-player matches westside-contracts#19
PR body has Summary, Changes, Test Plan, Related sections
Related section references plan slug -- no plan slug referenced (acceptable if this is standalone board work, not plan-driven)
No secrets committed
No unnecessary file changes (2 files, both directly relevant)
Commit messages are descriptive

PROCESS OBSERVATIONS

Merge ordering risk: Three open PRs (#163, #164, #165) all add sequential migrations. PR #164 (migration 019) must merge before this PR (migration 020). If #163 also introduces a migration, the chain needs coordination. Document dependencies explicitly to prevent alembic head conflicts.
Cross-repo coordination: This PR + westside-contracts PR #20 form a coupled deployment pair. The westside-contracts query (LEFT JOIN teams t ON t.id = p.team_id) will break once PR #164 (junction table) merges. A westside-contracts issue should track updating that query to use the junction table.

VERDICT: NOT APPROVED

Single reason: migration 020 references down_revision = "019" which does not exist on main. This will cause alembic upgrade head to fail. Document the dependency on PR #164 in the Related section, and ensure PR #164 merges first. Once that dependency is acknowledged and the merge order is locked in, this PR is clean and ready.

## PR #165 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Alembic **Migration (`alembic/versions/020_add_custom_notes_to_player.py`)** - Clean `add_column` / `drop_column` pair. Reversible. Correct use of `sa.Text()` and `nullable=True`. - `Text` is already imported in `models.py` (line 12), so no import change needed for the model -- correct. - Migration follows the established naming convention (`NNN_description.py`) consistent with 001-018. **Model (`src/basketball_api/models.py`)** - `Mapped[str | None]` with `mapped_column(Text, nullable=True)` -- correct SQLAlchemy 2.0 pattern matching all other nullable fields on Player (e.g., `jersey_number`, `contract_version`). - Column placement between `jersey_number` and `created_at` is reasonable. - No Pydantic schema changes needed in basketball-api because no endpoint serializes this field here. The consumer is westside-contracts, which queries the DB directly with `SELECT p.*`. **Security** - No input validation concern on the basketball-api side -- this PR adds only the column definition. Write access to `custom_notes` would go through admin scripts or direct DB (admin-only). No new endpoint accepts user input for this field. - In the companion westside-contracts PR, `custom_notes` is rendered via Svelte `{expression}` syntax which is HTML-escaped by default. No XSS risk. ### BLOCKERS **1. Migration 020 depends on unmerged migration 019 (PR #164)** `020_add_custom_notes_to_player.py` declares `down_revision = "019"`, but migration 019 (`019_player_teams_junction.py`) only exists in PR #164 (many-to-many junction table), which has not been merged to `main`. Running `alembic upgrade head` against current `main` + this PR will fail with: ``` alembic.util.exc.CommandError: Can't locate revision identified by '019' ``` This PR **must not merge before PR #164**. The dependency should be documented in the PR body under Related. No other blockers. The "new functionality with zero test coverage" criterion does not apply here -- this PR adds a nullable column with no business logic, no endpoints, and no validation in basketball-api. The column is a passive data field consumed by the companion westside-contracts app. ### NITS 1. **PR body should declare the dependency on PR #164** -- Add something like `Depends-on: #164 (migration 019)` to the Related section. This is important for merge ordering. 2. **Cross-repo breakage risk (informational)**: The westside-contracts companion query uses `LEFT JOIN teams t ON t.id = p.team_id`, but PR #164 drops the `team_id` column from `players` in favor of a junction table. When PR #164 merges, the westside-contracts query will break. This is out of scope for this PR, but should be tracked as a follow-up issue on westside-contracts. 3. **No test for custom_notes round-trip**: While not a blocker (no logic to test), a one-line assertion in an existing Player fixture confirming the column serializes to `None` by default would be cheap insurance against model/migration drift. ### SOP COMPLIANCE - [x] Branch named after issue: `19-add-custom-notes-to-player` matches westside-contracts#19 - [x] PR body has Summary, Changes, Test Plan, Related sections - [ ] Related section references plan slug -- no plan slug referenced (acceptable if this is standalone board work, not plan-driven) - [x] No secrets committed - [x] No unnecessary file changes (2 files, both directly relevant) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Merge ordering risk**: Three open PRs (#163, #164, #165) all add sequential migrations. PR #164 (migration 019) must merge before this PR (migration 020). If #163 also introduces a migration, the chain needs coordination. Document dependencies explicitly to prevent alembic head conflicts. - **Cross-repo coordination**: This PR + westside-contracts PR #20 form a coupled deployment pair. The westside-contracts query (`LEFT JOIN teams t ON t.id = p.team_id`) will break once PR #164 (junction table) merges. A westside-contracts issue should track updating that query to use the junction table. ### VERDICT: NOT APPROVED Single reason: migration 020 references `down_revision = "019"` which does not exist on `main`. This will cause `alembic upgrade head` to fail. Document the dependency on PR #164 in the Related section, and ensure PR #164 merges first. Once that dependency is acknowledged and the merge order is locked in, this PR is clean and ready.

forgejo_admin added 1 commit

2026-03-24 22:36:59 +00:00

fix: correct migration down_revision from 019 to 018

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

fe3546ad4e

Migration 019 (player-teams junction) is unmerged. The latest
migration on main is 018. QA caught the broken chain.

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