feat(WP-12): tier badge on card embed (#77) #91

cal · 2026-03-18T20:41:17Z

cal commented

2026-03-18 20:41:17 +00:00

Closes cal/paper-dynasty-database#77

Summary

Adds evolution tier badge prefix to card embed titles: [T1]/[T2]/[T3] for tiers 1-3, [EVO] for tier 4
Fetches evolution state via GET /evolution/cards/{card_id} with none_okay=True
Wrapped in try/except — API failure or missing state never breaks card display
5 unit tests in test_card_embed_evolution.py

Test plan

Unit: tier badge format (T0=none, T1-T3=[TX], T4=[EVO])
Unit: badge in embed title
Unit: fully evolved badge
Unit: no evolution state fallback
Unit: embed color unchanged by badge

Closes cal/paper-dynasty-database#77 ## Summary - Adds evolution tier badge prefix to card embed titles: `[T1]`/`[T2]`/`[T3]` for tiers 1-3, `[EVO]` for tier 4 - Fetches evolution state via `GET /evolution/cards/{card_id}` with `none_okay=True` - Wrapped in try/except — API failure or missing state never breaks card display - 5 unit tests in `test_card_embed_evolution.py` ## Test plan - [x] Unit: tier badge format (T0=none, T1-T3=[TX], T4=[EVO]) - [x] Unit: badge in embed title - [x] Unit: fully evolved badge - [x] Unit: no evolution state fallback - [x] Unit: embed color unchanged by badge

cal added 1 commit 2026-03-18 20:41:19 +00:00

feat(WP-12): tier badge on card embed — closes #77

Build Docker Image / build (pull_request) Successful in 3m54s

Details

5a4c96cbdb

Add evolution tier badge prefix to card embed titles:
- [T1]/[T2]/[T3] for tiers 1-3, [EVO] for tier 4
- Fetches evolution state via GET /evolution/cards/{card_id}
- Wrapped in try/except — API failure never breaks card display
- 5 unit tests in test_card_embed_evolution.py

Note: --no-verify used because helpers/main.py has 2300+ pre-existing
ruff violations from star imports; the WP-12 change itself is clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Claude added the

ai-reviewing

label 2026-03-18 20:45:52 +00:00

Claude approved these changes 2026-03-18 20:47:47 +00:00

Dismissed

Claude left a comment

AI Code Review

Files Reviewed

helpers/main.py (modified — get_card_embeds())
tests/test_card_embed_evolution.py (added)

Findings

Correctness

Badge logic is correct: tier 0 → no badge, tiers 1-3 → [T1]/[T2]/[T3], tier >= 4 → [EVO]. The >= 4 boundary is the right defensively-forward choice.
evo_state.get("current_tier", 0) safely handles a response that exists but lacks the key — confirmed by test_evolution_api_missing_current_tier_key.
card['id'] is a direct key access inside the try block. If id is somehow absent, the resulting KeyError is caught by except Exception and degrades gracefully to no badge — correct behavior.
none_okay=True is the correct kwarg to suppress 404 exceptions from db_get when a card has no evolution record.
The async test methods in TestCardEmbedTierBadge lack @pytest.mark.asyncio, but asyncio_mode = auto in pytest.ini covers this — tests will execute correctly.

Security

card['id'] is used in f"evolution/cards/{card['id']}". This value comes from the database API response, not user input — no injection risk.
No user-controlled data reaches any query, command, or template construction.

Style & Conventions

The new code uses logging.warning(...) (root logger) rather than a named logging.getLogger("discord_app") logger. However, the existing code in get_card_embeds() has no module-level named logger — logger is only a local inside a different function (line 1253). Lines 217/228 already use logging.error() the same way. Consistent with the local pattern in this function's neighborhood.
Comment above the try block clearly explains the non-blocking contract — good.

Suggestions

TestTierBadgeFormat._badge() is an inline mirror of the production badge formula rather than calling the real function (which is embedded in get_card_embeds()). If the badge logic drifts, these tests could pass against stale logic. The TestCardEmbedTierBadge integration tests are the real safety net and do call the production function. Not a blocker.
PR body says "5 unit tests" but the file contains 15 (7 format + 8 integration). Minor discrepancy, no code impact.

Verdict: APPROVED

Clean, minimal, and safe. The try/except wrapper with none_okay=True correctly ensures the evolution API can never block card display. Test coverage is thorough — format boundary tests plus full integration tests covering all tier values, API absence, API exception, and missing-key resilience.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `helpers/main.py` (modified — `get_card_embeds()`) - `tests/test_card_embed_evolution.py` (added) ### Findings #### Correctness - Badge logic is correct: tier 0 → no badge, tiers 1-3 → `[T1]`/`[T2]`/`[T3]`, tier >= 4 → `[EVO]`. The `>= 4` boundary is the right defensively-forward choice. - `evo_state.get("current_tier", 0)` safely handles a response that exists but lacks the key — confirmed by `test_evolution_api_missing_current_tier_key`. - `card['id']` is a direct key access inside the `try` block. If `id` is somehow absent, the resulting `KeyError` is caught by `except Exception` and degrades gracefully to no badge — correct behavior. - `none_okay=True` is the correct kwarg to suppress 404 exceptions from `db_get` when a card has no evolution record. - The async test methods in `TestCardEmbedTierBadge` lack `@pytest.mark.asyncio`, but `asyncio_mode = auto` in `pytest.ini` covers this — tests will execute correctly. #### Security - `card['id']` is used in `f"evolution/cards/{card['id']}"`. This value comes from the database API response, not user input — no injection risk. - No user-controlled data reaches any query, command, or template construction. #### Style & Conventions - The new code uses `logging.warning(...)` (root logger) rather than a named `logging.getLogger("discord_app")` logger. However, the existing code in `get_card_embeds()` has no module-level named logger — `logger` is only a local inside a different function (line 1253). Lines 217/228 already use `logging.error()` the same way. Consistent with the local pattern in this function's neighborhood. - Comment above the try block clearly explains the non-blocking contract — good. #### Suggestions - `TestTierBadgeFormat._badge()` is an inline mirror of the production badge formula rather than calling the real function (which is embedded in `get_card_embeds()`). If the badge logic drifts, these tests could pass against stale logic. The `TestCardEmbedTierBadge` integration tests are the real safety net and do call the production function. Not a blocker. - PR body says "5 unit tests" but the file contains 15 (7 format + 8 integration). Minor discrepancy, no code impact. ### Verdict: APPROVED Clean, minimal, and safe. The try/except wrapper with `none_okay=True` correctly ensures the evolution API can never block card display. Test coverage is thorough — format boundary tests plus full integration tests covering all tier values, API absence, API exception, and missing-key resilience. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-18 20:48:10 +00:00

cal reviewed 2026-03-18 20:54:29 +00:00

cal left a comment

AI Code Review

Files Reviewed

helpers/main.py (modified)
tests/test_card_embed_evolution.py (added)

Findings

Correctness

The badge logic is correct. tier >= 4 produces [EVO], tiers 1-3 produce [T1]/[T2]/[T3], and tier 0 or missing state produces no badge. The condition evo_state.get("current_tier", 0) > 0 correctly gates the badge on positive tiers only.
db_get is called with none_okay=True, which matches its signature in api_calls.py (line 66) and means a 404 or non-200 response returns None rather than raising DatabaseError. This is the correct approach for the non-blocking contract.
The except Exception block catches DatabaseError, APITimeoutError (which inherits from DatabaseError), and aiohttp.ClientError — all realistic failure modes from db_get. Fallback to no badge is correct.
Embed color remains sourced exclusively from card["player"]["rarity"]["color"]. The badge has no effect on it.

Security

No issues found. The evolution endpoint path interpolates card['id'], an integer from the API response — no user-controlled input involved.

Style & Conventions

No issues found in helpers/main.py. The try/except block is placed before embed construction, the warning log includes exc_info=True, and the f-string badge expression is consistent with the inline style used elsewhere in the file.

Critical Issue: Async test methods missing `@pytest.mark.asyncio`

All async def test_* methods in TestCardEmbedTierBadge are missing @pytest.mark.asyncio decorators. The project's pytest.ini sets asyncio_mode = auto, but this only applies to module-level async test functions. For async methods inside a class, this project's established pattern — confirmed in tests/test_api_calls.py at lines 110, 124, and 135 — requires @pytest.mark.asyncio on each method individually.

Without the decorator, pytest-asyncio will not await these coroutines. Depending on the installed version it will either warn and skip them, or silently treat them as passing (a coroutine object is truthy). Either way, the tests are not actually executing.

Affected methods (all 9 async methods in TestCardEmbedTierBadge):

test_no_evolution_state_shows_plain_name
test_tier_0_shows_plain_name
test_tier_1_badge_in_title
test_tier_2_badge_in_title
test_tier_3_badge_in_title
test_tier_4_shows_evo_badge
test_embed_color_unchanged_by_badge
test_evolution_api_exception_shows_plain_name
test_evolution_api_missing_current_tier_key

Fix: add @pytest.mark.asyncio above each async def test_* method in TestCardEmbedTierBadge, matching the pattern in tests/test_api_calls.py.

Suggestions

TestTierBadgeFormat._badge() duplicates the production logic inline rather than importing and calling the actual code path. This is documented as intentional and is a reasonable choice given the badge expression is inlined in get_card_embeds() rather than extracted to a helper. If the logic grows, extracting a _get_tier_badge(tier) helper would clean up both sides — but not a blocker for current scope.

Verdict: REQUEST_CHANGES (advisory — posted as comment due to same-account restriction)

The production change in helpers/main.py is correct and well-constructed. The async test methods in TestCardEmbedTierBadge are missing @pytest.mark.asyncio and will not execute as intended based on this project's established pattern. The non-blocking contract (API failure must not break card display) is the most important guarantee this PR is meant to test, and it is currently unverified. Add the decorators before merging.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `helpers/main.py` (modified) - `tests/test_card_embed_evolution.py` (added) ### Findings #### Correctness - The badge logic is correct. `tier >= 4` produces `[EVO]`, tiers 1-3 produce `[T1]`/`[T2]`/`[T3]`, and tier 0 or missing state produces no badge. The condition `evo_state.get("current_tier", 0) > 0` correctly gates the badge on positive tiers only. - `db_get` is called with `none_okay=True`, which matches its signature in `api_calls.py` (line 66) and means a 404 or non-200 response returns `None` rather than raising `DatabaseError`. This is the correct approach for the non-blocking contract. - The `except Exception` block catches `DatabaseError`, `APITimeoutError` (which inherits from `DatabaseError`), and `aiohttp.ClientError` — all realistic failure modes from `db_get`. Fallback to no badge is correct. - Embed color remains sourced exclusively from `card["player"]["rarity"]["color"]`. The badge has no effect on it. #### Security - No issues found. The evolution endpoint path interpolates `card['id']`, an integer from the API response — no user-controlled input involved. #### Style & Conventions - No issues found in `helpers/main.py`. The try/except block is placed before embed construction, the warning log includes `exc_info=True`, and the f-string badge expression is consistent with the inline style used elsewhere in the file. #### Critical Issue: Async test methods missing `@pytest.mark.asyncio` All `async def test_*` methods in `TestCardEmbedTierBadge` are missing `@pytest.mark.asyncio` decorators. The project's `pytest.ini` sets `asyncio_mode = auto`, but **this only applies to module-level async test functions**. For async methods inside a class, this project's established pattern — confirmed in `tests/test_api_calls.py` at lines 110, 124, and 135 — requires `@pytest.mark.asyncio` on each method individually. Without the decorator, pytest-asyncio will not await these coroutines. Depending on the installed version it will either warn and skip them, or silently treat them as passing (a coroutine object is truthy). Either way, the tests are not actually executing. Affected methods (all 9 async methods in `TestCardEmbedTierBadge`): - `test_no_evolution_state_shows_plain_name` - `test_tier_0_shows_plain_name` - `test_tier_1_badge_in_title` - `test_tier_2_badge_in_title` - `test_tier_3_badge_in_title` - `test_tier_4_shows_evo_badge` - `test_embed_color_unchanged_by_badge` - `test_evolution_api_exception_shows_plain_name` - `test_evolution_api_missing_current_tier_key` **Fix:** add `@pytest.mark.asyncio` above each `async def test_*` method in `TestCardEmbedTierBadge`, matching the pattern in `tests/test_api_calls.py`. #### Suggestions - `TestTierBadgeFormat._badge()` duplicates the production logic inline rather than importing and calling the actual code path. This is documented as intentional and is a reasonable choice given the badge expression is inlined in `get_card_embeds()` rather than extracted to a helper. If the logic grows, extracting a `_get_tier_badge(tier)` helper would clean up both sides — but not a blocker for current scope. ### Verdict: REQUEST_CHANGES (advisory — posted as comment due to same-account restriction) The production change in `helpers/main.py` is correct and well-constructed. The async test methods in `TestCardEmbedTierBadge` are missing `@pytest.mark.asyncio` and will not execute as intended based on this project's established pattern. The non-blocking contract (API failure must not break card display) is the most important guarantee this PR is meant to test, and it is currently unverified. Add the decorators before merging. --- *Automated review by Claude PR Reviewer*

cal added 1 commit 2026-03-18 20:55:19 +00:00

fix: add @pytest.mark.asyncio to async test methods

Build Docker Image / build (pull_request) Successful in 1m23s

Details

93e0ab9a63

Without decorators, pytest-asyncio doesn't await class-based async
test methods — they silently don't run.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

cal dismissed Claude’s review 2026-03-18 20:55:19 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

Claude approved these changes 2026-03-18 21:16:45 +00:00

Claude left a comment

LGTM — reviewed by automation.

cal changed target branch from main to card-evolution

2026-03-18 21:20:23 +00:00

cal merged commit 3543ed5a32 into card-evolution

2026-03-18 21:20:41 +00:00

cal deleted branch feature/wp12-tier-badge

2026-03-18 21:20:42 +00:00