feat: add evaluated_only filter to GET /api/v2/refractor/cards (#174) #175

Claude · 2026-03-25T22:33:16Z

Claude commented

2026-03-25 22:33:16 +00:00

Closes #174

Summary

Adds evaluated_only: bool = Query(default=True) parameter to list_card_states() in routers_v2/refractor.py
When True (default), applies WHERE last_evaluated_at IS NOT NULL — excludes placeholder rows created at pack-open time that have never been through the evaluator
When False, returns all rows including zero-value unevaluated cards (admin/pipeline use case)
Uses last_evaluated_at IS NOT NULL rather than current_value > 0 to correctly include cards with legitimately zero stats after evaluation

Files changed

app/routers_v2/refractor.py — new param + filter predicate + updated docstring
tests/test_refractor_state_api.py — new test test_list_cards_evaluated_only_false_includes_unevaluated (T3-8)

Test

New SQLite-backed test creates one unevaluated card (last_evaluated_at=None) and one evaluated card (last_evaluated_at=timestamp) for the same team, then asserts:

Default call returns only the evaluated card (count=1)
evaluated_only=false returns both cards (count=2)

No migration required — pure query-layer change.

Closes #174 ## Summary - Adds `evaluated_only: bool = Query(default=True)` parameter to `list_card_states()` in `routers_v2/refractor.py` - When `True` (default), applies `WHERE last_evaluated_at IS NOT NULL` — excludes placeholder rows created at pack-open time that have never been through the evaluator - When `False`, returns all rows including zero-value unevaluated cards (admin/pipeline use case) - Uses `last_evaluated_at IS NOT NULL` rather than `current_value > 0` to correctly include cards with legitimately zero stats after evaluation ## Files changed - `app/routers_v2/refractor.py` — new param + filter predicate + updated docstring - `tests/test_refractor_state_api.py` — new test `test_list_cards_evaluated_only_false_includes_unevaluated` (T3-8) ## Test New SQLite-backed test creates one unevaluated card (`last_evaluated_at=None`) and one evaluated card (`last_evaluated_at=timestamp`) for the same team, then asserts: - Default call returns only the evaluated card (count=1) - `evaluated_only=false` returns both cards (count=2) No migration required — pure query-layer change.

Claude added 1 commit 2026-03-25 22:33:17 +00:00

feat: add evaluated_only filter to GET /api/v2/refractor/cards (#174 ) 537eabcc4d

Closes #174

Adds `evaluated_only: bool = Query(default=True)` to `list_card_states()`.
When True (the default), cards with `last_evaluated_at IS NULL` are excluded —
these are placeholder rows created at pack-open time but never run through the
evaluator. At team scale this eliminates ~2739 zero-value rows from the
default response, making the Discord /refractor status command efficient
without any bot-side changes.

Set `evaluated_only=false` to include all rows (admin/pipeline use case).

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

Claude referenced this pull request

2026-03-25 22:33:25 +00:00

GET /api/v2/refractor/cards: add `evaluated_only` filter to exclude zero-value cards #174

Claude added the

ai-reviewing

label 2026-03-25 22:45:53 +00:00

Claude reviewed 2026-03-25 22:47:17 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/routers_v2/refractor.py (modified)
tests/test_refractor_state_api.py (modified)

Findings

Correctness

evaluated_only: bool = Query(default=True) is added to list_card_states() at the correct position in the parameter list (before limit/offset).
Peewee idiom .is_null(False) is the correct way to express IS NOT NULL. ✅
Filter is applied before total = query.count(), so the response count correctly reflects total matching evaluated rows before pagination. ✅
Default True is a behaviour change for existing callers (they now get only evaluated cards by default), but the PR body clearly documents this as intentional and motivates it with the performance context ("2753 rows, ~14 evaluated"). No issues.

Security

No new user-controlled query construction. evaluated_only is a plain boolean — no injection risk.
Auth guard unchanged (token validated before query execution). ✅

Style & Conventions

Docstring updated with aligned-column formatting consistent with the rest of the function's docstring. ✅
bool (not Optional[bool]) is consistent with limit: int and offset: int in the same signature. ✅
No unnecessary abstractions; the filter is a single inline .where() call. ✅

Test Coverage

Test creates one unevaluated row (last_evaluated_at=None) and one evaluated row (last_evaluated_at=timestamp) for the same team. ✅
Calls the endpoint twice — once with default (no evaluated_only param) and once with evaluated_only=false — and asserts count and exact player names in both cases. ✅
Docstring explains "what" and "why" (including the 2753-row / 14-evaluated performance motivation). ✅

Suggestions

Minor / non-blocking: The comment block labels this test T3-8, but based on prior PRs T3-8 was already used for test_get_card_state_last_evaluated_at_null. The numeric label is comment-only and doesn't affect test execution, but consider renaming to T3-9 to avoid confusion if the T3-x numbering is referenced elsewhere.

Verdict: APPROVED

Clean, focused PR. Correct Peewee idiom, filter placed correctly before the count, both the default and opt-out paths are tested with specific assertions. No migration needed. The T3-8 label collision is a trivial nit.

Note: Posted as COMMENT — Gitea self-review restriction prevents APPROVED on own PR.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/refractor.py` (modified) - `tests/test_refractor_state_api.py` (modified) ### Findings #### Correctness - `evaluated_only: bool = Query(default=True)` is added to `list_card_states()` at the correct position in the parameter list (before `limit`/`offset`). - Peewee idiom `.is_null(False)` is the correct way to express `IS NOT NULL`. ✅ - Filter is applied **before** `total = query.count()`, so the response `count` correctly reflects total matching evaluated rows before pagination. ✅ - Default `True` is a behaviour change for existing callers (they now get only evaluated cards by default), but the PR body clearly documents this as intentional and motivates it with the performance context ("2753 rows, ~14 evaluated"). No issues. #### Security - No new user-controlled query construction. `evaluated_only` is a plain boolean — no injection risk. - Auth guard unchanged (token validated before query execution). ✅ #### Style & Conventions - Docstring updated with aligned-column formatting consistent with the rest of the function's docstring. ✅ - `bool` (not `Optional[bool]`) is consistent with `limit: int` and `offset: int` in the same signature. ✅ - No unnecessary abstractions; the filter is a single inline `.where()` call. ✅ #### Test Coverage - Test creates one unevaluated row (`last_evaluated_at=None`) and one evaluated row (`last_evaluated_at=timestamp`) for the same team. ✅ - Calls the endpoint twice — once with default (no `evaluated_only` param) and once with `evaluated_only=false` — and asserts count and exact player names in both cases. ✅ - Docstring explains "what" and "why" (including the 2753-row / 14-evaluated performance motivation). ✅ #### Suggestions - **Minor / non-blocking**: The comment block labels this test `T3-8`, but based on prior PRs `T3-8` was already used for `test_get_card_state_last_evaluated_at_null`. The numeric label is comment-only and doesn't affect test execution, but consider renaming to `T3-9` to avoid confusion if the T3-x numbering is referenced elsewhere. ### Verdict: APPROVED Clean, focused PR. Correct Peewee idiom, filter placed correctly before the count, both the default and opt-out paths are tested with specific assertions. No migration needed. The T3-8 label collision is a trivial nit. > Note: Posted as COMMENT — Gitea self-review restriction prevents APPROVED on own PR. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-25 22:47:36 +00:00

cal approved these changes 2026-03-25 22:52:50 +00:00

Dismissed

cal left a comment

AI Code Review

Files Reviewed

app/routers_v2/refractor.py (modified)
tests/test_refractor_state_api.py (modified)

Findings

Correctness

The filter placement is correct. query = query.where(RefractorCardState.last_evaluated_at.is_null(False)) is applied before total = query.count(), so the count reflects the filtered result rather than the full table — this is the right behavior.
is_null(False) is the correct Peewee idiom for IS NOT NULL. Using is_null(True) would invert the logic, so the boolean argument is load-bearing and correctly set.
The filter is positioned after all other optional filters (card_type, tier, season, progress), which is consistent with the existing pattern and avoids any query ordering issues.
The default=True semantics are accurate: new callers get evaluated-only results without needing to pass the parameter, which matches the stated use case (bot performance at 2753 rows, ~14 evaluated).

Breaking Change Risk: default=True

This is the most important thing to verify before merging. Any existing caller of GET /api/v2/refractor/cards that expects unevaluated placeholder rows in its results will now silently receive a filtered subset.
The PR body documents the real-world data shape (2753 rows, ~14 evaluated), which makes the impact of the default immediately clear: if existing callers are the Discord bot fetching a team's card list for display, default=True is the correct default and any result set that previously included unevaluated rows was arguably wrong. If any admin/pipeline caller depends on the full unfiltered set, it must now pass evaluated_only=false.
The fix is not reversible without another API change, so the default choice is final. Given the PR justification and data shape, default=True is the right call.

Security

No issues found. The parameter is a boolean with no user-supplied string interpolation into SQL. Peewee's is_null() generates parameterized SQL.

Style & Conventions

Docstring alignment was updated to match the new parameter name width — this is cosmetic but correct.
The new parameter follows the existing Query() declaration style exactly.
No unused imports introduced.

Test Coverage

T3-8 covers the two critical paths: default call returns only the evaluated card; evaluated_only=false returns both.
The test verifies exact count and checks player names in the result set, not just the count — this catches a filter inversion bug that a count-only assertion would miss.
The test docstring clearly explains both the what and the why, including the production data motivation (2753 rows / ~14 evaluated), consistent with project testing conventions.
One gap worth noting: there is no test asserting that evaluated_only=true (explicitly passed, not defaulted) behaves the same as the default. This is a minor omission since the parameter plumbing is trivially correct, but noted for completeness.

Suggestions

Not blocking: Consider adding a ?evaluated_only=true explicit-true assertion alongside the default call in T3-8 to ensure the parameter round-trips correctly through FastAPI's bool parsing (FastAPI accepts true/false as strings for bool query params — worth one extra assert).

Verdict: APPROVED

The implementation is correct, the filter logic is sound Peewee ORM, the default is well-justified by the production data shape, and the test coverage is solid. No issues warrant blocking this merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/refractor.py` (modified) - `tests/test_refractor_state_api.py` (modified) ### Findings #### Correctness - The filter placement is correct. `query = query.where(RefractorCardState.last_evaluated_at.is_null(False))` is applied before `total = query.count()`, so the count reflects the filtered result rather than the full table — this is the right behavior. - `is_null(False)` is the correct Peewee idiom for `IS NOT NULL`. Using `is_null(True)` would invert the logic, so the boolean argument is load-bearing and correctly set. - The filter is positioned after all other optional filters (card_type, tier, season, progress), which is consistent with the existing pattern and avoids any query ordering issues. - The `default=True` semantics are accurate: new callers get evaluated-only results without needing to pass the parameter, which matches the stated use case (bot performance at 2753 rows, ~14 evaluated). #### Breaking Change Risk: default=True - This is the most important thing to verify before merging. Any existing caller of `GET /api/v2/refractor/cards` that expects unevaluated placeholder rows in its results will now silently receive a filtered subset. - The PR body documents the real-world data shape (2753 rows, ~14 evaluated), which makes the impact of the default immediately clear: if existing callers are the Discord bot fetching a team's card list for display, `default=True` is the correct default and any result set that previously included unevaluated rows was arguably wrong. If any admin/pipeline caller depends on the full unfiltered set, it must now pass `evaluated_only=false`. - The fix is not reversible without another API change, so the default choice is final. Given the PR justification and data shape, `default=True` is the right call. #### Security - No issues found. The parameter is a boolean with no user-supplied string interpolation into SQL. Peewee's `is_null()` generates parameterized SQL. #### Style & Conventions - Docstring alignment was updated to match the new parameter name width — this is cosmetic but correct. - The new parameter follows the existing Query() declaration style exactly. - No unused imports introduced. #### Test Coverage - T3-8 covers the two critical paths: default call returns only the evaluated card; `evaluated_only=false` returns both. - The test verifies exact count and checks player names in the result set, not just the count — this catches a filter inversion bug that a count-only assertion would miss. - The test docstring clearly explains both the what and the why, including the production data motivation (2753 rows / ~14 evaluated), consistent with project testing conventions. - One gap worth noting: there is no test asserting that `evaluated_only=true` (explicitly passed, not defaulted) behaves the same as the default. This is a minor omission since the parameter plumbing is trivially correct, but noted for completeness. #### Suggestions - Not blocking: Consider adding a `?evaluated_only=true` explicit-true assertion alongside the default call in T3-8 to ensure the parameter round-trips correctly through FastAPI's bool parsing (FastAPI accepts `true`/`false` as strings for bool query params — worth one extra assert). ### Verdict: APPROVED The implementation is correct, the filter logic is sound Peewee ORM, the default is well-justified by the production data shape, and the test coverage is solid. No issues warrant blocking this merge. --- *Automated review by Claude PR Reviewer*

cal approved these changes 2026-03-25 22:53:03 +00:00

cal left a comment

Approved via pd-pr

cal merged commit c2c978ac47 into main

2026-03-25 22:53:08 +00:00

cal deleted branch issue/174-get-api-v2-refractor-cards-add-evaluated-only-filt

2026-03-25 22:53:09 +00:00

cal referenced this issue from a commit

2026-03-25 22:53:10 +00:00

Merge pull request 'feat: add evaluated_only filter to GET /api/v2/refractor/cards (#174)' (#175) from issue/174-get-api-v2-refractor-cards-add-evaluated-only-filt into main

Sign in to join this conversation.

No reviewers

No Milestone

No project

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#175