pal-e-docs

Review: Schedule data model + API (practices, events)

review-627-2026-03-28 Doc

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — Standalone, scoped during admin interface review
  • [x] Repo — forgejo_admin/basketball-api
  • [x] User Story — story:WS-S13, well-written admin schedule management story
  • [x] Context — Thorough: explains hardcoded schedule in westside-app, architecture decision (two tables), full data model with column specs
  • [x] File Targets — 5 files to modify/create, 2 files explicitly excluded
  • [x] Acceptance Criteria — 7 criteria
  • [x] Test Expectations — 4 test categories with run command
  • [x] Constraints — 5 constraints listed
  • [x] Checklist — Present with migration testing added
  • [x] Related — Lists project, arch doc, and follow-up tickets
All required sections for the Feature template are present. Template is complete.

Traceability

  • [x] story:WS-S13 — "As an admin, I want to view and manage the program schedule" (label present on board item)
  • [x] arch:basketball-api — Correctly targets the basketball-api architecture component (label present on board item)
  • [x] Forgejo issue — forgejo_admin/basketball-api#230, open
Traceability triangle is complete.

File Targets

  • [x] src/basketball_api/models.py — Verified: exists (489 lines). Contains existing Division enum, Team model, Tenant model. EventType enum and new models would be added here. Pattern confirmed: uses Mapped types, tenant FK, server_default=func.now().
  • [x] src/basketball_api/routes/admin.py — Verified: exists (1048 lines). Contains existing admin CRUD patterns with require_admin dependency, tenant scoping, Pydantic response models.
  • [ ] src/basketball_api/schemas.py — ISSUE: This file does NOT exist. The codebase has NO centralized schemas.py. All Pydantic schemas are defined inline in route files (e.g., admin.py line 51 defines GenerateTokensResponse, public.py defines InterestLeadRequest/InterestLeadResponse). The agent should follow existing convention and define schedule schemas inline in the route file, not create a new schemas.py.
  • [x] alembic/versions/xxx_add_schedule_tables.py — Verified: alembic/versions/ directory exists with 21 existing migrations (001 through 021). Next migration would be 022.
  • [x] scripts/seed_schedule.py — Verified: scripts/ directory exists with 10 existing scripts (seed.py, backfill_stripe.py, create_groupme_groups.py, etc.). New seed script fits this pattern.
  • [x] src/basketball_api/routes/public.py (excluded) — Verified: correctly excluded, public endpoints are a separate concern.

Repo Placement

Correct. Issue is filed on forgejo_admin/basketball-api and all file targets are within basketball-api. The ticket explicitly states westside-app frontend is a follow-up ticket. No cross-repo work needed.

Dependencies

  • [x] No blocking dependencies — all prerequisites are satisfied.
Board item #130 "Phase 13: Practice Schedule" exists in backlog with label blocked-by:marcus-input. This is a legacy plan-era phase item. Item #627 appears to be the properly scoped replacement. No active blocker — the marcus-input block on #130 was about schedule details, which are now captured in the seed data section of this ticket.
Board item #410 "Update Schedule: Kings/Queens toggle" is done — frontend schedule page with hardcoded data. No conflict. Board item #299 "Practice schedule page" is done — earlier frontend iteration. No conflict.
No items currently in in_progress that block this work. Follow-up dependencies (admin frontend view, public page refactor) are documented in the issue body and would be separate tickets.

Acceptance Criteria

7 acceptance criteria. All are verifiable by an agent:
  • "Alembic migration creates tables" — testable via alembic upgrade head
  • "Models follow existing patterns" — verifiable by code review against existing models
  • "GET /admin/schedule returns combined" — testable via pytest
  • "CRUD endpoints work" — testable via pytest
  • "Seed script populates data" — testable by running script
  • "Division filter works" — testable via pytest
  • "Existing tests still pass" — testable via pytest tests/ -v
Criteria are individually testable and specific. However, combined volume (7 AC + 9 endpoints + 2 models + migration + seed) is substantial. See Decomposition Assessment.

Blast Radius

  • Division enum mismatch (CRITICAL): The ticket states division: Enum(Division), nullable with values "kings/queens" in both table specs. However, the actual Division enum in models.py (line 52-54) has values boys = "boys" and girls = "girls", NOT kings/queens. The InterestLead model (line 458-460) uses a freetext String(20) field called program for "kings"/"queens" values. The public.py route (line 186) validates _VALID_PROGRAMS = {"kings", "queens"} separately. The ticket must clarify: use the existing Division(boys/girls) enum, or use a freetext program string field like InterestLead, or extend the enum. This is a design decision that an agent cannot make.
  • Tenant model relationships: Adding new tables with tenant_id FK should also add relationship lists to the Tenant model (lines 156-159 show existing patterns: parents, registrations, coaches, teams). Not mentioned in file targets.
  • admin.py is already 1048 lines: Adding 9 new endpoints (~200-300 lines) to an already large file. Existing codebase has precedent for separate route files (jersey.py, checkout.py, coaches_api.py, password_reset.py). A new routes/schedule.py would be more maintainable.
  • Rollback is straightforward: alembic downgrade -1 removes tables, route removal is clean.
  • No similar bug patterns found in sibling services.

Decomposition Assessment

Apply the three-thing limit and five-minute rule:
  • Discrete changes: (1) new enum + 2 models in models.py, (2) alembic migration, (3) 9 CRUD endpoints with Pydantic schemas, (4) seed script, (5) tests for all of the above. That is 5 discrete changes — exceeds the 3-thing limit.
  • Estimated agent time: 10-15 minutes minimum (2 models, 9 endpoints, migration, seed script, integration tests). Exceeds 5-minute rule significantly.
  • Independent subtasks: Yes. Data model + migration is independent from endpoint implementation. Seed script is independent from both. These could be parallelized after the model ticket merges.
NEEDS DECOMPOSITION. Recommend splitting via template-board into 3 sub-tickets:
  • Data model + migration: EventType enum, PracticeSchedule model, Event model, alembic migration, model unit tests. ~3 files, 2 AC, ~3 min.
  • Schedule API endpoints: 9 CRUD endpoints with Pydantic schemas in a new routes/schedule.py, endpoint integration tests. ~2 files, 3 AC, ~4 min.
  • Seed script: Populate current hardcoded schedule data, verify with assertions. ~1 file, 2 AC, ~2 min.

Recommendation

  • [BODY] Fix Division enum reference: the ticket says "kings/queens" but the codebase Division enum (models.py:52-54) uses boys/girls. The InterestLead model uses a freetext String(20) "program" field for kings/queens. Clarify which approach to use for schedule tables.
  • [BODY] Fix file target: src/basketball_api/schemas.py does not exist. The codebase defines all Pydantic schemas inline in route files. Either remove this target or replace with a new routes/schedule.py route file.
  • [SCOPE] Clarify: should schedule endpoints go in existing admin.py (already 1048 lines) or a new routes/schedule.py? Existing codebase precedent supports separate route files (jersey.py at 200 lines, checkout.py, coaches_api.py).
  • [DECOMPOSE] 7 AC, 9 endpoints, 2 models, migration, seed script, tests across 5 discrete changes. Exceeds both the 3-thing limit and 5-minute rule. Recommend decomposition via template-board into 3 sub-tickets (data model, API endpoints, seed script).