ldraney/basketball-api
1
0
Fork 0
Code Issues 90 Pull requests 11 Projects Releases Packages Wiki Activity Actions

feat: add nickname column to players table #414

Open
forgejo_admin wants to merge 2 commits from 408-player-nickname into main
pull from: 408-player-nickname
merge into: ldraney:main
Conversation 1 Commits 2 Files changed 5 +285
forgejo_admin commented 2026-04-09 13:41:05 +00:00
Contributor
Copy link

Summary

Add a nullable nickname field (String(100)) to the Player model so admins can assign display nicknames to players. The field is admin-set only — non-admin PATCH requests silently ignore it.

Changes

  • src/basketball_api/models.py — Added nickname: Mapped[str | None] column (String(100), nullable)
  • alembic/versions/031_add_nickname_to_players.py — Migration to add the column
  • src/basketball_api/routes/players.py — Added nickname to PlayerProfileResponse, PlayerProfileUpdate, and _player_profile_response helper. Added admin-only guard that strips nickname from non-admin PATCH requests.
  • src/basketball_api/routes/admin.py — Added nickname to AdminPlayerItem schema and serialization in admin players list
  • tests/test_nickname.py — 9 tests covering model behavior, API round-trip (admin set/clear/persist), non-admin rejection, and admin list inclusion

Test Plan

  • pytest tests/test_nickname.py — 9/9 pass
  • pytest tests/ — 907 passed, 8 errors (pre-existing enum collision in test_account.py, unrelated)
  • ruff format + ruff check — clean

Review Checklist

  • Model field matches existing nullable string patterns
  • Migration is sequential (031, revises 030)
  • Nickname included in player profile response and admin list response
  • Admin-only guard prevents non-admin users from setting nickname
  • Registration flow untouched
  • Contract endpoints untouched
  • Tests cover model, API round-trip, permission guard, and admin list

Related Notes

None.

Related

## Summary Add a nullable `nickname` field (String(100)) to the Player model so admins can assign display nicknames to players. The field is admin-set only — non-admin PATCH requests silently ignore it. ## Changes - `src/basketball_api/models.py` — Added `nickname: Mapped[str | None]` column (String(100), nullable) - `alembic/versions/031_add_nickname_to_players.py` — Migration to add the column - `src/basketball_api/routes/players.py` — Added nickname to `PlayerProfileResponse`, `PlayerProfileUpdate`, and `_player_profile_response` helper. Added admin-only guard that strips nickname from non-admin PATCH requests. - `src/basketball_api/routes/admin.py` — Added nickname to `AdminPlayerItem` schema and serialization in admin players list - `tests/test_nickname.py` — 9 tests covering model behavior, API round-trip (admin set/clear/persist), non-admin rejection, and admin list inclusion ## Test Plan - `pytest tests/test_nickname.py` — 9/9 pass - `pytest tests/` — 907 passed, 8 errors (pre-existing enum collision in test_account.py, unrelated) - `ruff format` + `ruff check` — clean ## Review Checklist - [x] Model field matches existing nullable string patterns - [x] Migration is sequential (031, revises 030) - [x] Nickname included in player profile response and admin list response - [x] Admin-only guard prevents non-admin users from setting nickname - [x] Registration flow untouched - [x] Contract endpoints untouched - [x] Tests cover model, API round-trip, permission guard, and admin list ## Related Notes None. ## Related - Forgejo issue: #408 - Closes #408
feat: add nickname column to players table
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
c69027a072 
Add nullable String(100) nickname field to Player model, migration 031,
and include it in player profile and admin list responses. Nickname is
admin-set only — non-admin PATCH requests silently ignore the field.

Closes #408

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-04-09 13:41:37 +00:00
Author
Contributor
Copy link

QA Review — PR #414

Summary

Adds nullable nickname (String(100)) to Player model with migration 031, serialization in player profile and admin list responses, and admin-only write guard.

Findings

Model & Migration

  • Column definition matches existing nullable string patterns (e.g., hometown, custom_notes)
  • Migration 031 correctly chains from 030, clean upgrade/downgrade
  • No data backfill needed (nullable, no default) -- correct

Serialization

  • PlayerProfileResponse includes nickname -- correct
  • AdminPlayerItem includes nickname -- correct
  • _player_profile_response helper passes player.nickname -- correct
  • Public routes (public.py) do NOT expose nickname -- correct (no leakage)

Permission Guard

  • Non-admin PATCH strips nickname from provided dict before applying -- correct pattern
  • Admin PATCH can set/clear nickname -- tested
  • Registration flow untouched -- verified (no changes to register.py)
  • Contract endpoints untouched -- verified

Tests

  • 9 tests covering: model default, set, clear, GET null, admin set, admin clear, persistence round-trip, non-admin rejection, admin list inclusion
  • All tests follow existing fixture patterns from test_players.py
  • Admin list test correctly overrides require_admin dependency (matches test_admin_spa.py pattern)

Nits

  • None identified. Clean, minimal, follows all existing patterns.

VERDICT: APPROVED

## QA Review — PR #414 ### Summary Adds nullable `nickname` (String(100)) to Player model with migration 031, serialization in player profile and admin list responses, and admin-only write guard. ### Findings **Model & Migration** - Column definition matches existing nullable string patterns (e.g., `hometown`, `custom_notes`) - Migration 031 correctly chains from 030, clean upgrade/downgrade - No data backfill needed (nullable, no default) -- correct **Serialization** - `PlayerProfileResponse` includes nickname -- correct - `AdminPlayerItem` includes nickname -- correct - `_player_profile_response` helper passes `player.nickname` -- correct - Public routes (`public.py`) do NOT expose nickname -- correct (no leakage) **Permission Guard** - Non-admin PATCH strips `nickname` from `provided` dict before applying -- correct pattern - Admin PATCH can set/clear nickname -- tested - Registration flow untouched -- verified (no changes to `register.py`) - Contract endpoints untouched -- verified **Tests** - 9 tests covering: model default, set, clear, GET null, admin set, admin clear, persistence round-trip, non-admin rejection, admin list inclusion - All tests follow existing fixture patterns from `test_players.py` - Admin list test correctly overrides `require_admin` dependency (matches `test_admin_spa.py` pattern) **Nits** - None identified. Clean, minimal, follows all existing patterns. ### VERDICT: APPROVED
forgejo_admin force-pushed 408-player-nickname from c69027a072
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
to 49e2b9ccaf
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
Details
2026-04-09 13:49:30 +00:00 Compare
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
Details
This pull request has changes conflicting with the target branch.
  • src/basketball_api/routes/players.py
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 408-player-nickname:408-player-nickname
git switch 408-player-nickname

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff 408-player-nickname
git switch 408-player-nickname
git rebase main
git switch main
git merge --ff-only 408-player-nickname
git switch 408-player-nickname
git rebase main
git switch main
git merge --no-ff 408-player-nickname
git switch main
git merge --squash 408-player-nickname
git switch main
git merge --ff-only 408-player-nickname
git switch main
git merge 408-player-nickname
git push origin main
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
ldraney/basketball-api!414
No description provided.