Skip to content

Add DB-only Player directory - #56

Merged
Mattsface merged 1 commit into
mainfrom
issue-30-player-directory
Sep 26, 2026
Merged

Mattsface merged 1 commit into
mainfrom
issue-30-player-directory

Conversation

@Mattsface

Copy link
Copy Markdown
Member

Summary

Continues GitHub issue #30 by adding the first real Player-domain destination.

  • adds a DB-only GET /players directory
  • derives available Player seasons from persisted player_seasons
  • defaults to the newest locally stored catalog season
  • adds case- and accent-insensitive Player name search
  • caps rendered search results at 50 without adding pagination infrastructure
  • validates player_id against the selected season's catalog membership
  • adds Players to the primary navigation now that it has a real destination
  • keeps Player selection shareable through season, q, and player_id
  • documents the Player-domain landing/directory behavior
  • adds focused repository and web coverage for Player discovery and selection

Player UI scope

The Player page is intentionally limited to discovery and selection.

It shows only persisted identity information needed to confirm the selected Player and does not expose Player analytics.

This PR does not add:

  • Player charts
  • Player statistics
  • Player comparison views
  • Player detail/metric routes
  • new Player ingestion
  • browser MLB/API calls
  • pagination or autocomplete infrastructure
  • generic entity/dashboard abstractions

Data semantics

Available Player seasons come from player_seasons, not hitting rows.

A selected Player must be a member of the selected season's persisted Player catalog. A globally known Player identity is not sufficient by itself.

Player name search reuses the existing case/accent folding used by Player catalog ordering.

Navigation

Primary navigation now contains:

  • Teams
  • Players

Team metric navigation and all existing Team URLs remain unchanged.

Preserved behavior

This PR preserves:

  • existing Team routes and query semantics
  • Team Offense / Pitching / Results navigation
  • Team selector behavior
  • MLB-wide completeness semantics
  • local Plotly behavior
  • existing error/recovery states
  • /health
  • the DB-only browser rendering guarantee

Testing

Adds coverage for:

  • catalog-season discovery and ordering
  • independence from Player hitting data
  • empty Player catalog behavior
  • newest stored-season default
  • explicit-season behavior
  • case- and accent-insensitive search
  • no-match states
  • season-scoped Player membership
  • shareable Player selection
  • result limiting
  • query encoding and HTML escaping
  • browser validation errors
  • missing-schema guidance
  • zero MLB/network/ingestion calls during Player browser requests
  • Team/Player primary navigation semantics

Part of #30.

Add GET /players as the first real Player-domain destination for #30.
Seasons come from persisted player_seasons (newest first by default);
search is a case- and accent-insensitive substring match over the
selected season's catalog, capped at 50 results; selection by player_id
requires membership in that season's catalog. Only identity fields are
shown: no Player statistics, charts, or MLB calls.

Primary navigation now exposes Teams and Players on every page, with
the current domain marked by aria-current="location". Team routes,
metric navigation, and query semantics are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Mattsface

Copy link
Copy Markdown
Member Author

@claude please review

@claude

claude Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Claude finished @Mattsface's task in 3m 25s —— View job


Review: Add DB-only Player directory

  • Gather context (diff, changed files, AGENTS.md)
  • Review repository/service/schema layer changes
  • Review web route + template changes
  • Review tests
  • Post final review feedback

Summary

This is a clean, well-scoped implementation that matches the PR description closely and follows the repo's layering rules (app/schemas/players.py → app/database/repositories.py → app/web/player_routes.py). I traced the route logic (app/web/player_routes.py:52-96) against tests/test_web_players.py case-by-case (default-season selection, invalid season, out-of-catalog player_id, empty catalog, missing-schema, search limiting, accent/case folding, shareable links, escaping) and didn't find a behavioral bug — the status/message precedence (status == 200 guard at app/web/player_routes.py:83) correctly avoids double/contradictory notices, and every edge case I traced has a corresponding assertion.

Things I specifically verified as correct (not just assumed):

  • list_player_catalog_seasons (app/database/repositories.py:512-528) correctly raises DatabaseSchemaMissingError only for the player_seasons table, re-raising other OperationalErrors.
  • Selection membership is checked against the full season catalog, independent of the search/result cap, so a selected player beyond the 50-result limit still resolves (app/web/player_routes.py:80-82, covered by test_result_limit_and_selection_beyond_limit).
  • normalize_player_name extraction (app/schemas/players.py:10-15) is a good de-duplication — catalog ordering and search now share one accent/case-fold implementation instead of drifting.
  • Jinja auto-escaping (default for .html templates) plus urlencode for generated links means the XSS test case is handled by existing infrastructure, not a new risk.
  • Primary nav (Teams/Players) is consistently added everywhere primary_navigation is overridden (base.html, team_base.html) or defaulted, including error/validation states.

Minor nit (non-blocking)

app/web/player_routes.py:20 imports MLB_LOGO_URL from app/web/routes.py. That constant is entity-neutral shell content (used only in base.html's brand logo), but it's owned by the Team-domain routes module. Now that a second domain module reaches into it, it might be worth hoisting MLB_LOGO_URL into a shared/neutral location (e.g. alongside other base-shell constants) so player_routes.py doesn't depend on the Team router module for unrelated branding data. Not a bug, just a small ownership smell worth a follow-up if more domains are added later.

Fix this →

Not verified

I was unable to execute ruff/pytest/poetry in this sandbox (commands required interactive approval that wasn't available), so I reviewed statically and by manually tracing test expectations against the implementation rather than confirming a green run. Please confirm CI passes before merging. Everything else in the "Definition of done" checklist (architecture, scope, migrations N/A here since no schema change, docs updated) looks satisfied from static review.

@Mattsface
Mattsface merged commit 8eb0e14 into main Sep 26, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant