feat: add parent_phone/parent_email to admin CRM response models #287

Merged

forgejo_admin merged 1 commit from 281-admin-crm-parent-fields into main

2026-04-03 22:25:26 +00:00

Conversation 1 Commits 1 Files changed 3 +55

forgejo_admin commented

2026-04-03 17:50:49 +00:00

Owner

Copy link

Summary

Add missing parent contact fields to IncompletePlayerItem and TeamPlayerItem response models so the frontend can display complete parent information in admin CRM views.

Changes

src/basketball_api/routes/admin.py — Added parent_phone: str | None to IncompletePlayerItem model and populated from p.parent.phone. Added parent_phone: str | None and parent_email: str | None to TeamPlayerItem model and populated from parent relationship.
tests/test_admin_email.py — Added test for parent_phone present in incomplete players response, and test for null when parent has no phone.
tests/test_admin_teams.py — Added assertions for parent_phone and parent_email in existing player shape test. Added new test verifying fields are populated when parent has contact info.

Test Plan

pytest tests/test_admin_email.py tests/test_admin_teams.py — 43 tests pass
Verified parent_phone returns value when parent has phone, null when missing
Verified parent_email returns value in TeamPlayerItem response

Review Checklist

ruff format and ruff check clean
All existing tests pass
New tests cover both populated and null cases
No duplicate of AdminPlayerItem.parent_phone (#276)

Related Notes

None

Closes #281
Spike: #278 (Mismatches 5, 7)
Prior fix: #276 (AdminPlayerItem.parent_phone)

## Summary Add missing parent contact fields to `IncompletePlayerItem` and `TeamPlayerItem` response models so the frontend can display complete parent information in admin CRM views. ## Changes - `src/basketball_api/routes/admin.py` — Added `parent_phone: str | None` to `IncompletePlayerItem` model and populated from `p.parent.phone`. Added `parent_phone: str | None` and `parent_email: str | None` to `TeamPlayerItem` model and populated from parent relationship. - `tests/test_admin_email.py` — Added test for `parent_phone` present in incomplete players response, and test for null when parent has no phone. - `tests/test_admin_teams.py` — Added assertions for `parent_phone` and `parent_email` in existing player shape test. Added new test verifying fields are populated when parent has contact info. ## Test Plan - `pytest tests/test_admin_email.py tests/test_admin_teams.py` — 43 tests pass - Verified `parent_phone` returns value when parent has phone, null when missing - Verified `parent_email` returns value in TeamPlayerItem response ## Review Checklist - [x] `ruff format` and `ruff check` clean - [x] All existing tests pass - [x] New tests cover both populated and null cases - [x] No duplicate of AdminPlayerItem.parent_phone (#276) ## Related Notes - None ## Related - Closes #281 - Spike: #278 (Mismatches 5, 7) - Prior fix: #276 (AdminPlayerItem.parent_phone)

forgejo_admin added 1 commit

2026-04-03 17:50:49 +00:00

feat: add parent_phone/parent_email to IncompletePlayerItem and TeamPlayerItem

Some checks failed

ci/woodpecker/pr/woodpecker Pipeline failed

Details

2dd2dd4b67

IncompletePlayerItem now includes parent_phone. TeamPlayerItem now includes
both parent_phone and parent_email. Fields are populated from the parent
relationship and default to null when the parent record lacks them.

Closes #281

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

forgejo_admin commented

2026-04-03 17:54:19 +00:00

Author

Owner

Copy link

PR #287 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / Pydantic

This PR adds parent_phone to IncompletePlayerItem and parent_phone + parent_email to TeamPlayerItem -- two admin CRM response models. The implementation is correct:

Model fields match the ORM: Parent.phone is Mapped[str | None], and the Pydantic fields use str | None = None. Parent.email is Mapped[str] (non-nullable).
No N+1 queries: Both _query_incomplete_players() and admin_teams() already use joinedload(Player.parent), so p.parent.phone and p.parent.email are accessed from eagerly-loaded data.
No duplication with AdminPlayerItem: The PR checklist references #276 -- confirmed that AdminPlayerItem uses a separate model and these fields are added to distinct response models (IncompletePlayerItem, TeamPlayerItem).
PEP 484 / Pydantic: Type annotations are correct and consistent with existing patterns in the file.
No migration needed: These are response-model-only changes; no new DB columns.

BLOCKERS

None.

NITS

TeamPlayerItem.parent_email typed as str | None = None but Parent.email is non-nullable (Mapped[str]). This won't break anything (the field will always be populated), but it is slightly misleading -- it suggests the value can be null when it cannot. For consistency with IncompletePlayerItem.parent_email: str, consider typing it as str instead. Non-blocking.

SOP COMPLIANCE

Branch named after issue: 281-admin-crm-parent-fields follows {issue-number}-{kebab-case-purpose}
PR body follows template: Summary, Changes, Test Plan, Related sections present
Related references spike #278 and prior fix #276
No secrets committed
No scope creep -- changes are tightly scoped to the two response models
Tests cover both populated and null cases for all new fields
Related section does not reference a plan slug (acceptable -- this is a standalone ticket from spike #278, not plan-driven work)

PROCESS OBSERVATIONS

Clean decomposition from spike #278: each mismatch gets its own ticket and PR. Good for deployment frequency and change failure isolation.
4 new tests across 2 test files cover happy path (populated values) and edge case (null/missing phone). Test coverage is solid.
The 55-line addition with zero deletions signals pure additive change -- low change failure risk.

VERDICT: APPROVED

## PR #287 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / Pydantic This PR adds `parent_phone` to `IncompletePlayerItem` and `parent_phone` + `parent_email` to `TeamPlayerItem` -- two admin CRM response models. The implementation is correct: - **Model fields match the ORM**: `Parent.phone` is `Mapped[str | None]`, and the Pydantic fields use `str | None = None`. `Parent.email` is `Mapped[str]` (non-nullable). - **No N+1 queries**: Both `_query_incomplete_players()` and `admin_teams()` already use `joinedload(Player.parent)`, so `p.parent.phone` and `p.parent.email` are accessed from eagerly-loaded data. - **No duplication with AdminPlayerItem**: The PR checklist references #276 -- confirmed that `AdminPlayerItem` uses a separate model and these fields are added to distinct response models (`IncompletePlayerItem`, `TeamPlayerItem`). - **PEP 484 / Pydantic**: Type annotations are correct and consistent with existing patterns in the file. - **No migration needed**: These are response-model-only changes; no new DB columns. ### BLOCKERS None. ### NITS 1. **`TeamPlayerItem.parent_email` typed as `str | None = None`** but `Parent.email` is non-nullable (`Mapped[str]`). This won't break anything (the field will always be populated), but it is slightly misleading -- it suggests the value can be null when it cannot. For consistency with `IncompletePlayerItem.parent_email: str`, consider typing it as `str` instead. Non-blocking. ### SOP COMPLIANCE - [x] Branch named after issue: `281-admin-crm-parent-fields` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body follows template: Summary, Changes, Test Plan, Related sections present - [x] Related references spike #278 and prior fix #276 - [x] No secrets committed - [x] No scope creep -- changes are tightly scoped to the two response models - [x] Tests cover both populated and null cases for all new fields - [ ] Related section does not reference a plan slug (acceptable -- this is a standalone ticket from spike #278, not plan-driven work) ### PROCESS OBSERVATIONS - Clean decomposition from spike #278: each mismatch gets its own ticket and PR. Good for deployment frequency and change failure isolation. - 4 new tests across 2 test files cover happy path (populated values) and edge case (null/missing phone). Test coverage is solid. - The 55-line addition with zero deletions signals pure additive change -- low change failure risk. ### VERDICT: APPROVED

forgejo_admin merged commit ba8bffb4d1 into main

2026-04-03 22:25:26 +00:00

forgejo_admin deleted branch 281-admin-crm-parent-fields

2026-04-03 22:25:26 +00:00

forgejo_admin referenced this pull request from a commit

2026-04-03 22:25:27 +00:00

feat: add parent_phone/parent_email to admin CRM response models (#287)

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

Reference in a new issue

Repository

forgejo_admin/basketball-api

Title

Body

No description provided.

Delete branch "281-admin-crm-parent-fields"

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