forgejo_admin/basketball-api
1
0
Fork 1
Code Issues 88 Pull requests 9 Projects Releases Packages Wiki Activity Actions

feat: add contract_config JSONB to teams table (#319) #335

Merged
forgejo_admin merged 1 commit from 319-contract-config-teams into main 2026-04-04 22:01:38 +00:00
Conversation 2 Commits 1 Files changed 2 +28
forgejo_admin commented 2026-04-04 21:48:32 +00:00
Owner
Copy link

Summary

Adds a nullable contract_config JSONB column to the teams table, enabling per-team contract configuration storage.

Changes

  • alembic/versions/031_add_contract_config_to_teams.py -- New migration adding contract_config JSONB DEFAULT NULL to teams table (reversible)
  • src/basketball_api/models.py -- Added contract_config: Mapped[dict | None] field to Team model using JSONB type

Test Plan

  • Model imports verified: Team.__table__.columns includes contract_config
  • Ruff format and check pass clean
  • Existing test suite has pre-existing DB fixture failures unrelated to this change (confirmed same failures on main)
  • Migration is reversible: downgrade() drops the column

Review Checklist

  • Ruff format and check pass
  • Model column uses correct JSONB type from sqlalchemy.dialects.postgresql
  • Migration is reversible (downgrade drops column)
  • Column is nullable with no default (DEFAULT NULL)
  • No existing tests broken by this change

Related Notes

None.

Related

Closes #319

## Summary Adds a nullable `contract_config` JSONB column to the `teams` table, enabling per-team contract configuration storage. ## Changes - `alembic/versions/031_add_contract_config_to_teams.py` -- New migration adding `contract_config JSONB DEFAULT NULL` to `teams` table (reversible) - `src/basketball_api/models.py` -- Added `contract_config: Mapped[dict | None]` field to `Team` model using `JSONB` type ## Test Plan - Model imports verified: `Team.__table__.columns` includes `contract_config` - Ruff format and check pass clean - Existing test suite has pre-existing DB fixture failures unrelated to this change (confirmed same failures on main) - Migration is reversible: `downgrade()` drops the column ## Review Checklist - [x] Ruff format and check pass - [x] Model column uses correct JSONB type from `sqlalchemy.dialects.postgresql` - [x] Migration is reversible (downgrade drops column) - [x] Column is nullable with no default (DEFAULT NULL) - [x] No existing tests broken by this change ## Related Notes None. ## Related Closes #319
feat: include is_public in admin players list response
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
3c167eae94 
The AdminPlayerItem model and GET /admin/players response now include
the is_public boolean field, enabling the westside-app admin UI to
display and toggle player visibility state.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
feat: add contract_config JSONB column to teams table
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
Details
ac76415ee6 
Adds a nullable JSONB column for per-team contract configuration,
enabling teams to store contract templates and signing settings.

Closes #319

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-04-04 21:50:18 +00:00
Author
Owner
Copy link

QA Review -- PR #335

Scope Check

  • Migration (031_add_contract_config_to_teams.py): Correct. Adds nullable JSONB column, reversible downgrade drops it. Follows the established pattern from migration 030.
  • Model (models.py): Correct. contract_config: Mapped[dict | None] = mapped_column(JSONB, nullable=True) -- uses the already-imported JSONB type, placed logically before created_at.

Observations

  1. Stale commit in branch: The diff includes an unrelated commit (3c167ea feat: include is_public in admin players list response) that modifies admin.py and test_admin_spa.py. This commit exists in the local clone but not on remote main. Recommend rebasing onto origin/main to produce a clean single-commit diff. This is a blocking nit -- the PR should only contain changes for issue #319.

Checklist

  • JSONB import already exists (line 19) -- no new imports needed in model
  • Migration revision chain correct (030 -> 031)
  • Column is nullable with no server_default (DEFAULT NULL implicit)
  • Downgrade drops column (reversible)
  • Ruff format and check pass
  • No test regressions from this change (pre-existing DB fixture issue confirmed on main)

VERDICT: APPROVE with nit

The two files in scope (migration + model) are correct and clean. The stale is_public commit should be rebased out before merge, but that is a branch hygiene issue, not a code quality issue. The contract_config implementation itself is approved.

## QA Review -- PR #335 ### Scope Check - **Migration** (`031_add_contract_config_to_teams.py`): Correct. Adds nullable JSONB column, reversible downgrade drops it. Follows the established pattern from migration 030. - **Model** (`models.py`): Correct. `contract_config: Mapped[dict | None] = mapped_column(JSONB, nullable=True)` -- uses the already-imported JSONB type, placed logically before `created_at`. ### Observations 1. **Stale commit in branch**: The diff includes an unrelated commit (`3c167ea feat: include is_public in admin players list response`) that modifies `admin.py` and `test_admin_spa.py`. This commit exists in the local clone but not on remote main. Recommend rebasing onto `origin/main` to produce a clean single-commit diff. **This is a blocking nit** -- the PR should only contain changes for issue #319. ### Checklist - [x] JSONB import already exists (line 19) -- no new imports needed in model - [x] Migration revision chain correct (030 -> 031) - [x] Column is nullable with no server_default (DEFAULT NULL implicit) - [x] Downgrade drops column (reversible) - [x] Ruff format and check pass - [x] No test regressions from this change (pre-existing DB fixture issue confirmed on main) ### VERDICT: APPROVE with nit The two files in scope (migration + model) are correct and clean. The stale `is_public` commit should be rebased out before merge, but that is a branch hygiene issue, not a code quality issue. The contract_config implementation itself is approved.
forgejo_admin force-pushed 319-contract-config-teams from ac76415ee6
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
Details
to 1c9a948b6d
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
2026-04-04 21:50:46 +00:00 Compare
forgejo_admin commented 2026-04-04 21:50:49 +00:00
Author
Owner
Copy link

Rebased onto origin/main and skipped the stale is_public commit. Branch now contains only the single contract_config commit. QA nit resolved.

Rebased onto `origin/main` and skipped the stale `is_public` commit. Branch now contains only the single contract_config commit. QA nit resolved.
forgejo_admin deleted branch 319-contract-config-teams 2026-04-04 22:01:38 +00:00
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
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!335
No description provided.