cal/paper-dynasty-database
1
0
Fork 0
Code Issues 7 Pull Requests 4 Actions Packages Projects Releases Wiki Activity

feat: add GET /api/v2/refractor/cards list endpoint (#172) #173

Merged
cal merged 1 commits from issue/172-feat-add-get-api-v2-refractor-cards-list-endpoint into main 2026-03-25 14:52:25 +00:00
Conversation 0 Commits 1 Files Changed 3 +156 -7
Claude commented 2026-03-25 06:35:44 +00:00
Collaborator
Copy Link

Closes #172

Summary

Adds GET /api/v2/refractor/cards to the refractor router — the missing endpoint needed by the Discord bot's /refractor status command.

Endpoint behaviour

  • team_id (required): filters to a single team's refractor states
  • card_type filter: "batter", "sp", "rp" via RefractorTrack.card_type
  • tier filter: 0–4 via RefractorCardState.current_tier
  • season filter: EXISTS subquery against batting_season_stats OR pitching_season_stats — avoids row multiplication from players with both batting and pitching stats
  • progress=close filter: CASE expression maps current_tier → next threshold column; returns only cards where current_value >= next_threshold * 0.8 (and not fully evolved)
  • limit (1–100, default 10) / offset (≥0, default 0) pagination
  • count reflects total matching rows before limit/offset for pagination UI
  • Sorted current_tier DESC, current_value DESC
  • LEFT JOIN on Player: deleted players return player_name: null rather than erroring

Response shape additions

_build_card_state_response() extended with two new fields:

  • progress_pct — round(current_value / next_threshold * 100, 1), null when fully evolved
  • player_name — included when passed from list join; the single-card endpoint gains progress_pct automatically, player_name omitted until updated separately

Index

Non-unique (team_id) index added to refractor_card_state in db_engine.py (for SQLite dev) and a migration SQL file for production PostgreSQL.

Files changed

  • app/routers_v2/refractor.py — new list_card_states endpoint; extended _build_card_state_response(); removed pre-existing unused RefractorTrack import in evaluate_game (ruff required)
  • app/db_engine.py — non-unique (team_id) index on refractor_card_state
  • migrations/2026-03-25_add_refractor_card_state_team_index.sql — production migration

Other observations

  • GET /api/v2/teams/{team_id}/refractors in teams.py:1533 is the partial predecessor to this endpoint. The spec recommends deprecating it in a future release (Phase 2: add deprecation log; Phase 3: remove). Not changed in this PR.
  • The single-card GET /refractor/cards/{card_id} endpoint will now include progress_pct in responses (it calls _build_card_state_response() without player_name). This is a non-breaking addition.
Closes #172 ## Summary Adds `GET /api/v2/refractor/cards` to the refractor router — the missing endpoint needed by the Discord bot's `/refractor status` command. ### Endpoint behaviour - `team_id` (required): filters to a single team's refractor states - `card_type` filter: `"batter"`, `"sp"`, `"rp"` via `RefractorTrack.card_type` - `tier` filter: `0–4` via `RefractorCardState.current_tier` - `season` filter: EXISTS subquery against `batting_season_stats` OR `pitching_season_stats` — avoids row multiplication from players with both batting and pitching stats - `progress=close` filter: CASE expression maps `current_tier` → next threshold column; returns only cards where `current_value >= next_threshold * 0.8` (and not fully evolved) - `limit` (1–100, default 10) / `offset` (≥0, default 0) pagination - `count` reflects total matching rows **before** limit/offset for pagination UI - Sorted `current_tier DESC, current_value DESC` - LEFT JOIN on Player: deleted players return `player_name: null` rather than erroring ### Response shape additions `_build_card_state_response()` extended with two new fields: - `progress_pct` — `round(current_value / next_threshold * 100, 1)`, `null` when fully evolved - `player_name` — included when passed from list join; the single-card endpoint gains `progress_pct` automatically, `player_name` omitted until updated separately ### Index Non-unique `(team_id)` index added to `refractor_card_state` in `db_engine.py` (for SQLite dev) and a migration SQL file for production PostgreSQL. ## Files changed - `app/routers_v2/refractor.py` — new `list_card_states` endpoint; extended `_build_card_state_response()`; removed pre-existing unused `RefractorTrack` import in `evaluate_game` (ruff required) - `app/db_engine.py` — non-unique `(team_id)` index on `refractor_card_state` - `migrations/2026-03-25_add_refractor_card_state_team_index.sql` — production migration ## Other observations - `GET /api/v2/teams/{team_id}/refractors` in `teams.py:1533` is the partial predecessor to this endpoint. The spec recommends deprecating it in a future release (Phase 2: add deprecation log; Phase 3: remove). Not changed in this PR. - The single-card `GET /refractor/cards/{card_id}` endpoint will now include `progress_pct` in responses (it calls `_build_card_state_response()` without `player_name`). This is a non-breaking addition.
Claude added 1 commit 2026-03-25 06:35:45 +00:00 
Closes #172

- New GET /api/v2/refractor/cards endpoint in refractor router with
  team_id (required), card_type, tier, season, progress, limit, offset filters
- season filter uses EXISTS subquery against batting/pitching_season_stats
- progress=close filter uses CASE expression to compare current_value
  against next tier threshold (>= 80%)
- LEFT JOIN on Player so deleted players return player_name: null
- Sorting: current_tier DESC, current_value DESC
- count reflects total matching rows before pagination
- Extended _build_card_state_response() with progress_pct (computed) and
  optional player_name; single-card endpoint gains progress_pct automatically
- Added non-unique team_id index on refractor_card_state in db_engine.py
- Migration: 2026-03-25_add_refractor_card_state_team_index.sql
- Removed pre-existing unused RefractorTrack import in evaluate_game (ruff)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-03-25 06:45:45 +00:00
Claude reviewed 2026-03-25 06:48:28 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • app/routers_v2/refractor.py (modified — new list_card_states endpoint, extended _build_card_state_response, removed unused import)
  • app/db_engine.py (modified — new team index on refractor_card_state)
  • migrations/2026-03-25_add_refractor_card_state_team_index.sql (added)

Findings

Correctness

  • Pagination count field: Correctly computed from un-limited query before .offset().limit() — consistent with PR #169's total-count behavior. ✅
  • Route ordering: GET /cards is inserted before GET /cards/{card_id}. No ambiguity — exact path vs path-param route; FastAPI handles these correctly. ✅
  • progress=close CASE expression: For tiers 0–3, the CASE maps to the correct threshold column; tier-4 (fully evolved) returns NULL, but the fully_evolved == False guard already excludes those rows before the comparison. Logically sound. ✅
  • EXISTS subquery for season filter: Filters by both player and team — correct, since refractor state is keyed per (player, team) pair. Avoids row multiplication from two-way players. ✅
  • Removed RefractorTrack from evaluate_game lazy import: Correct — the class is never directly referenced by name in that function body; state.track is a Peewee FK accessor. ✅
  • fn, Case, JOIN availability: db_engine.py uses from peewee import *, so all three are in scope for re-import. ✅
  • BattingSeasonStats / PitchingSeasonStats: Both confirmed in db_engine.py (lines 1053, 1104). ✅
  • Migration file: Has BEGIN/COMMIT, CREATE INDEX IF NOT EXISTS (idempotent), and a rollback comment — follows the convention flagged in the PR #131 review. ✅

Security

  • Auth check (valid_token) applied before any DB access on the new endpoint — same pattern as all other endpoints. ✅
  • No SQL injection surface: all filter values go through Peewee's parameterised query builder. ✅

Style & Conventions

  • Lazy imports inside the function body — consistent with every other endpoint in this file. ✅
  • player_name retrieval via try/except Exception: pass — consistent with the same pattern in evaluate_game (line 209–213). ✅
  • # noqa: E712 comment on the fully_evolved == False comparison — correct, Peewee requires == for SQL expression building. ✅

Suggestions

  • progress parameter accepts arbitrary strings silently. Any value other than 'close' applies no filter and returns normally. Consistent with codebase pattern (no enum validation on string filters elsewhere), but an explicit 400 on unsupported values would make the contract clearer to callers.
  • progress_pct can exceed 100 if current_value > next_threshold (shouldn't happen in practice given how tier-ups work, but worth noting for UI consumers). A min(progress_pct, 100.0) cap or a docstring note would future-proof it.
  • No tests for the new endpoint. Consistent with the pagination PR pattern (tests added in separate follow-up PRs), but a test PR covering season, progress=close, and pagination filter combinations would be valuable given the query complexity.

Verdict: APPROVED ✅

Clean, correct implementation. The count-before-limit behavior, EXISTS-subquery season filter, CASE-expression progress filter, and LEFT JOIN player name resolution are all well-reasoned. The migration follows project conventions. Suggestions above are non-blocking.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/refractor.py` (modified — new `list_card_states` endpoint, extended `_build_card_state_response`, removed unused import) - `app/db_engine.py` (modified — new team index on `refractor_card_state`) - `migrations/2026-03-25_add_refractor_card_state_team_index.sql` (added) ### Findings #### Correctness - **Pagination `count` field**: Correctly computed from un-limited query before `.offset().limit()` — consistent with PR #169's total-count behavior. ✅ - **Route ordering**: `GET /cards` is inserted before `GET /cards/{card_id}`. No ambiguity — exact path vs path-param route; FastAPI handles these correctly. ✅ - **`progress=close` CASE expression**: For tiers 0–3, the CASE maps to the correct threshold column; tier-4 (fully evolved) returns NULL, but the `fully_evolved == False` guard already excludes those rows before the comparison. Logically sound. ✅ - **EXISTS subquery for `season` filter**: Filters by both `player` and `team` — correct, since refractor state is keyed per (player, team) pair. Avoids row multiplication from two-way players. ✅ - **Removed `RefractorTrack` from `evaluate_game` lazy import**: Correct — the class is never directly referenced by name in that function body; `state.track` is a Peewee FK accessor. ✅ - **`fn`, `Case`, `JOIN` availability**: `db_engine.py` uses `from peewee import *`, so all three are in scope for re-import. ✅ - **`BattingSeasonStats` / `PitchingSeasonStats`**: Both confirmed in `db_engine.py` (lines 1053, 1104). ✅ - **Migration file**: Has `BEGIN`/`COMMIT`, `CREATE INDEX IF NOT EXISTS` (idempotent), and a rollback comment — follows the convention flagged in the PR #131 review. ✅ #### Security - Auth check (`valid_token`) applied before any DB access on the new endpoint — same pattern as all other endpoints. ✅ - No SQL injection surface: all filter values go through Peewee's parameterised query builder. ✅ #### Style & Conventions - Lazy imports inside the function body — consistent with every other endpoint in this file. ✅ - `player_name` retrieval via `try/except Exception: pass` — consistent with the same pattern in `evaluate_game` (line 209–213). ✅ - `# noqa: E712` comment on the `fully_evolved == False` comparison — correct, Peewee requires `==` for SQL expression building. ✅ #### Suggestions - **`progress` parameter accepts arbitrary strings silently.** Any value other than `'close'` applies no filter and returns normally. Consistent with codebase pattern (no enum validation on string filters elsewhere), but an explicit `400` on unsupported values would make the contract clearer to callers. - **`progress_pct` can exceed 100** if `current_value > next_threshold` (shouldn't happen in practice given how tier-ups work, but worth noting for UI consumers). A `min(progress_pct, 100.0)` cap or a docstring note would future-proof it. - **No tests for the new endpoint.** Consistent with the pagination PR pattern (tests added in separate follow-up PRs), but a test PR covering `season`, `progress=close`, and pagination filter combinations would be valuable given the query complexity. ### Verdict: APPROVED ✅ Clean, correct implementation. The `count`-before-limit behavior, EXISTS-subquery season filter, CASE-expression progress filter, and LEFT JOIN player name resolution are all well-reasoned. The migration follows project conventions. Suggestions above are non-blocking. --- *Automated review by Claude PR Reviewer*
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-25 06:48:47 +00:00
cal approved these changes 2026-03-25 14:52:14 +00:00
cal left a comment
Owner
Copy Link

Approved as cal — reviewer verdict is APPROVED with no blocking concerns. Non-blocking suggestions noted for follow-up.

Approved as cal — reviewer verdict is APPROVED with no blocking concerns. Non-blocking suggestions noted for follow-up.
cal merged commit eefd4afa37 into main 2026-03-25 14:52:25 +00:00
cal deleted branch issue/172-feat-add-get-api-v2-refractor-cards-list-endpoint 2026-03-25 14:52:26 +00:00
cal added the
ai-merged
 label 2026-03-25 15:09:43 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
cal
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-merged

PR merged by AI ops agent

  
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-reviewing

   
ai-working

   
bug

   
enhancement

   
evolution

Card Evolution system

  
performance

   
phase-0

Render Pipeline Optimization

  
phase-1a

Schema & Data Foundation

  
phase-1b

API & Formula Engine

  
phase-1c

Bot Integration

  
phase-1d

Deployment

  
security

   
tech-debt

   
todo
No Label
ai-changes-requested
ai-failed
ai-merged
ai-pr-opened
ai-reviewed
ai-reviewing
ai-reviewing
ai-working
bug
enhancement
evolution
performance
phase-0
phase-1a
phase-1b
phase-1c
phase-1d
security
tech-debt
todo
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
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: cal/paper-dynasty-database#173
No description provided.
Delete Branch "issue/172-feat-add-get-api-v2-refractor-cards-list-endpoint"

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?