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

fix: sort /teams/{id}/evolutions by current_tier desc, current_value desc (#116) #120

Merged
cal merged 1 commits from ai/paper-dynasty-database-116 into next-release 2026-03-19 18:24:49 +00:00
Conversation 0 Commits 1 Files Changed 1 +6 -3
cal commented 2026-03-19 18:14:36 +00:00
Owner
Copy Link

Closes #116

Summary

  • Changed .order_by(EvolutionCardState.player_id) to .order_by(current_tier DESC, current_value DESC) in list_team_evolutions
  • Updated the docstring to reflect the new ordering
  • Most-progressed cards now appear first in /evo status without needing the tier:1 filter

Files changed

  • app/routers_v2/teams.py — order_by clause in list_team_evolutions

Test results

117 passed, 15 skipped — all green.

Closes #116 ## Summary - Changed `.order_by(EvolutionCardState.player_id)` to `.order_by(current_tier DESC, current_value DESC)` in `list_team_evolutions` - Updated the docstring to reflect the new ordering - Most-progressed cards now appear first in `/evo status` without needing the `tier:1` filter ## Files changed - `app/routers_v2/teams.py` — order_by clause in `list_team_evolutions` ## Test results 117 passed, 15 skipped — all green.
cal added 2 commits 2026-03-19 18:14:37 +00:00 
JOIN the Player table in the evolutions query so p_name can be
included in each serialized item without N+1 queries.

Closes #115

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes #116

The endpoint was returning results in player_id insertion order, causing
/evo status in Discord to show a wall of T0/value-0 cards before any
progressed players. Sort by current_tier DESC, current_value DESC so
the most-evolved cards always appear first.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-03-19 18:15:48 +00:00
Claude approved these changes 2026-03-19 18:17:51 +00:00
Claude left a comment
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • app/routers_v2/teams.py (modified)

Findings

Correctness

  • Order-by change is correct: EvolutionCardState.current_tier and current_value are both confirmed fields on the model (IntegerField(default=0) and FloatField(default=0.0) respectively). .desc() is valid Peewee syntax.
  • Player join is correct: .switch(EvolutionCardState).join(Player) is the right Peewee pattern for multi-table joins after already joining EvolutionTrack. Player is added to .select() so state.player is eagerly loaded — no N+1.
  • state.player.p_name is safe: player FK on EvolutionCardState is non-nullable, and the inner join ensures only rows with valid player references are returned.
  • Count correctness preserved: Adding the Player join doesn't affect query.count() — Peewee wraps the full query as a subquery for count, all filters apply before .offset().limit().
  • Docstring updated: Old "ordered by player_id for stable ordering" correctly replaced with new description.

Security

  • No issues. Auth check (valid_token) is unchanged. No user input flows into the join or ordering logic.

Style & Conventions

  • Player is added to the lazy import block inside the function body, but Player is already imported at module level (line 18). The redundant lazy import is harmless — Python's module cache means it resolves to the same object — but it's unnecessary. Minor nit only.
  • Blank line removal at line 1051 (team_buy_players) is trivial whitespace; not a concern.

Suggestions

  • Tiebreaker stability: The new ordering (current_tier DESC, current_value DESC) is not fully deterministic when two cards share the same tier and value. Adding .order_by(..., EvolutionCardState.player_id) as a tiebreaker would restore stable pagination. Low priority for the /evo status use case, but worth considering if pagination is used.
  • Redundant lazy import: Player in from ..db_engine import EvolutionCardState, EvolutionTrack, Player is already in scope from the module-level import. Not harmful, just slightly noisy.
  • PR body scope: The diff includes the Player join and player_name changes (from PR #119's branch) in addition to the ordering change. The PR body only describes the ordering change. Not a code issue, but the combined scope could cause confusion on review.

Verdict: APPROVED

Clean fix. The ordering change is correct, the Player join follows the codebase's established Peewee multi-join pattern, and eager loading is properly handled. No correctness or security issues.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/teams.py` (modified) ### Findings #### Correctness - **Order-by change is correct**: `EvolutionCardState.current_tier` and `current_value` are both confirmed fields on the model (`IntegerField(default=0)` and `FloatField(default=0.0)` respectively). `.desc()` is valid Peewee syntax. - **Player join is correct**: `.switch(EvolutionCardState).join(Player)` is the right Peewee pattern for multi-table joins after already joining `EvolutionTrack`. `Player` is added to `.select()` so `state.player` is eagerly loaded — no N+1. - **`state.player.p_name` is safe**: `player` FK on `EvolutionCardState` is non-nullable, and the inner join ensures only rows with valid player references are returned. - **Count correctness preserved**: Adding the Player join doesn't affect `query.count()` — Peewee wraps the full query as a subquery for count, all filters apply before `.offset().limit()`. - **Docstring updated**: Old "ordered by player_id for stable ordering" correctly replaced with new description. #### Security - No issues. Auth check (`valid_token`) is unchanged. No user input flows into the join or ordering logic. #### Style & Conventions - `Player` is added to the lazy import block inside the function body, but `Player` is already imported at module level (line 18). The redundant lazy import is harmless — Python's module cache means it resolves to the same object — but it's unnecessary. Minor nit only. - Blank line removal at line 1051 (`team_buy_players`) is trivial whitespace; not a concern. #### Suggestions - **Tiebreaker stability**: The new ordering (`current_tier DESC, current_value DESC`) is not fully deterministic when two cards share the same tier and value. Adding `.order_by(..., EvolutionCardState.player_id)` as a tiebreaker would restore stable pagination. Low priority for the `/evo status` use case, but worth considering if pagination is used. - **Redundant lazy import**: `Player` in `from ..db_engine import EvolutionCardState, EvolutionTrack, Player` is already in scope from the module-level import. Not harmful, just slightly noisy. - **PR body scope**: The diff includes the Player join and `player_name` changes (from PR #119's branch) in addition to the ordering change. The PR body only describes the ordering change. Not a code issue, but the combined scope could cause confusion on review. ### Verdict: APPROVED Clean fix. The ordering change is correct, the Player join follows the codebase's established Peewee multi-join pattern, and eager loading is properly handled. No correctness or security issues. --- *Automated review by Claude PR Reviewer*
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-19 18:18:16 +00:00
cal force-pushed ai/paper-dynasty-database-116 from 2884c07aeb to 0953a45b9f 2026-03-19 18:24:22 +00:00 Compare
cal merged commit dc163e0ddd into next-release 2026-03-19 18:24:49 +00:00
cal deleted branch ai/paper-dynasty-database-116 2026-03-19 18:24:49 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Claude
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#120
No description provided.
Delete Branch "ai/paper-dynasty-database-116"

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?