fix: add parent_phone to /admin/players list endpoint #277

Merged

forgejo_admin merged 1 commit from 276-parent-phone-admin-list into main

2026-04-03 16:21:38 +00:00

Conversation 1 Commits 1 Files changed 2 +34

forgejo_admin commented

2026-04-03 16:10:29 +00:00

Owner

Copy link

Summary

The GET /admin/players endpoint returned parent_name and parent_email but omitted parent_phone, despite the data existing in the parents table. This adds the missing field following the same pattern already used by the CSV export endpoint.

Changes

src/basketball_api/routes/admin.py -- Added parent_phone: str | None = None to AdminPlayerItem model and passed parent.phone in the constructor
tests/test_admin_spa.py -- Added parent_phone to field presence assertion, added null check for parent without phone, added new test verifying populated phone values

Test Plan

pytest tests/test_admin_spa.py -- 9/9 passing
Verify GET /admin/players response includes parent_phone for each player
Verify parent_phone is null when parent has no phone set
Verify parent_phone matches parents.phone from database when set

Review Checklist

ruff format and ruff check pass
All tests pass (9/9)
No unrelated changes

Related Notes

None

Closes #276
Companion frontend ticket: forgejo_admin/westside-landing#203

## Summary The `GET /admin/players` endpoint returned `parent_name` and `parent_email` but omitted `parent_phone`, despite the data existing in the `parents` table. This adds the missing field following the same pattern already used by the CSV export endpoint. ## Changes - `src/basketball_api/routes/admin.py` -- Added `parent_phone: str | None = None` to `AdminPlayerItem` model and passed `parent.phone` in the constructor - `tests/test_admin_spa.py` -- Added `parent_phone` to field presence assertion, added null check for parent without phone, added new test verifying populated phone values ## Test Plan - `pytest tests/test_admin_spa.py` -- 9/9 passing - Verify `GET /admin/players` response includes `parent_phone` for each player - Verify `parent_phone` is `null` when parent has no phone set - Verify `parent_phone` matches `parents.phone` from database when set ## Review Checklist - [x] ruff format and ruff check pass - [x] All tests pass (9/9) - [x] No unrelated changes ## Related Notes - None ## Related - Closes #276 - Companion frontend ticket: `forgejo_admin/westside-landing#203`

forgejo_admin added 1 commit

2026-04-03 16:10:29 +00:00

fix: add parent_phone to /admin/players list endpoint

All checks were successful

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

26b3a5376d

The AdminPlayerItem response model was missing parent_phone despite the
data existing in the parents table. Follows the same pattern already used
by the CSV export endpoint.

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

forgejo_admin commented

2026-04-03 16:15:13 +00:00

Author

Owner

Copy link

PR #277 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy / Pydantic

This is a straightforward bug fix adding parent_phone to the AdminPlayerItem Pydantic model and its construction in the admin_list_players endpoint. The change follows the exact same pattern already established by the CSV export at line 526 (p.parent.phone).

Model alignment: Parent.phone is Mapped[str | None] (line 181 of models.py), and AdminPlayerItem.parent_phone is typed str | None = None. Types are consistent. The default of None means players whose parent has no phone will serialize as null -- correct behavior.

Relationship loading: Player.parent is already eager-loaded via joinedload(Player.parent) in the query (line 318), so accessing player.parent.phone will not trigger an N+1 query. No concerns.

Test coverage: Three test additions cover:

Null phone assertion on the existing fixture (alex["parent_phone"] is None) -- edge case
Field presence check in test_response_fields -- structural
New test_parent_phone_populated with a dedicated parent fixture having a phone number -- happy path

All three test axes (null, field presence, populated value) are covered. Well done.

BLOCKERS

None.

NITS

The CSV export at line 526 coerces None to empty string (p.parent.phone or ""), while the JSON endpoint returns null. This is fine -- CSV needs empty strings, JSON should use null -- but worth noting the intentional divergence for future maintainers.
test_response_fields does not include contract_status, country, or registration_type in expected_fields. This is pre-existing and out of scope for this PR, but the field list has drifted. Consider a follow-up to sync it.

SOP COMPLIANCE

Branch named after issue: 276-parent-phone-admin-list
PR body has Summary, Changes, Test Plan, Related
Related references plan slug -- N/A (bug fix, no plan)
No secrets committed
No scope creep -- exactly 2 files, both directly related
Commit messages are descriptive
Tests exist and pass (9/9 per PR body)

PROCESS OBSERVATIONS

Tight, well-scoped bug fix. Two files, 34 additions, zero deletions. The companion frontend ticket (westside-landing#203) is referenced, showing good cross-repo coordination. Low change failure risk.

VERDICT: APPROVED

## PR #277 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / SQLAlchemy / Pydantic This is a straightforward bug fix adding `parent_phone` to the `AdminPlayerItem` Pydantic model and its construction in the `admin_list_players` endpoint. The change follows the exact same pattern already established by the CSV export at line 526 (`p.parent.phone`). **Model alignment**: `Parent.phone` is `Mapped[str | None]` (line 181 of `models.py`), and `AdminPlayerItem.parent_phone` is typed `str | None = None`. Types are consistent. The default of `None` means players whose parent has no phone will serialize as `null` -- correct behavior. **Relationship loading**: `Player.parent` is already eager-loaded via `joinedload(Player.parent)` in the query (line 318), so accessing `player.parent.phone` will not trigger an N+1 query. No concerns. **Test coverage**: Three test additions cover: 1. Null phone assertion on the existing fixture (`alex["parent_phone"] is None`) -- edge case 2. Field presence check in `test_response_fields` -- structural 3. New `test_parent_phone_populated` with a dedicated parent fixture having a phone number -- happy path All three test axes (null, field presence, populated value) are covered. Well done. ### BLOCKERS None. ### NITS 1. The CSV export at line 526 coerces `None` to empty string (`p.parent.phone or ""`), while the JSON endpoint returns `null`. This is fine -- CSV needs empty strings, JSON should use `null` -- but worth noting the intentional divergence for future maintainers. 2. `test_response_fields` does not include `contract_status`, `country`, or `registration_type` in `expected_fields`. This is pre-existing and out of scope for this PR, but the field list has drifted. Consider a follow-up to sync it. ### SOP COMPLIANCE - [x] Branch named after issue: `276-parent-phone-admin-list` - [x] PR body has Summary, Changes, Test Plan, Related - [ ] Related references plan slug -- N/A (bug fix, no plan) - [x] No secrets committed - [x] No scope creep -- exactly 2 files, both directly related - [x] Commit messages are descriptive - [x] Tests exist and pass (9/9 per PR body) ### PROCESS OBSERVATIONS Tight, well-scoped bug fix. Two files, 34 additions, zero deletions. The companion frontend ticket (westside-landing#203) is referenced, showing good cross-repo coordination. Low change failure risk. ### VERDICT: APPROVED

forgejo_admin force-pushed 276-parent-phone-admin-list from 26b3a5376d

All checks were successful

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

to 6f2e32cc72

Some checks failed

ci/woodpecker/pr/woodpecker Pipeline failed

Details

2026-04-03 16:21:18 +00:00

Compare

forgejo_admin merged commit 67612c07a9 into main

2026-04-03 16:21:38 +00:00

forgejo_admin deleted branch 276-parent-phone-admin-list

2026-04-03 16:21:38 +00:00

forgejo_admin referenced this pull request from a commit

2026-04-03 16:21:39 +00:00

fix: add parent_phone to /admin/players list endpoint (#277)

Reviewers

No reviewers

Labels

Clear labels

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

No labels

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!277

Reference in a new issue

Repository

forgejo_admin/basketball-api

Title

Body

No description provided.

Delete branch "276-parent-phone-admin-list"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?

Rows
Columns