feat: Refractor tier badge prefix in card embed title (WP-12) (#77) #88
No reviewers
cal
Labels
No Label
ai-changes-requested
ai-failed
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
ai-working
bug
enhancement
feature
in-queue
performance
security
tech-debt
todo
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: cal/paper-dynasty-discord#88
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "ai/paper-dynasty-database#77"
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?
Closes cal/paper-dynasty-database#77
Summary
Adds Refractor tier badge to
get_card_embeds()inhelpers/main.py.evolution/cards/{card_id}before building the embed[BC](Base Chrome)[R](Refractor)[GR](Gold Refractor)[SF](Superfractor)try/except Exception— API failure never breaks card displayFiles changed
helpers/main.py—TIER_BADGESmodule-level constant + badge logic inget_card_embeds(); also fixed broken{e}log format strings (missing f-prefix + restoredas e)tests/test_card_embed_evolution.py— 11 unit tests covering all tier badge scenariosruff.toml— restored to match main (branch had stripped global config)Test results
All 11 tests pass. Scenarios covered:
[BC], T2 →[R], T3 →[GR], T4 →[SF]None(404) → no badgeAdd evolution tier badge to get_card_embeds() title. Fetches evolution/cards/{id} endpoint; prepends [T1]/[T2]/[T3]/[EVO] when current_tier > 0. API failure is silently swallowed so card display is never broken. Also add ruff.toml to suppress legacy star-import rules (F403/F405) and bare-except/type-comparison rules (E722/E721) for helpers/main.py, which predates the pre-commit hook installation. Closes #77 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>AI Code Review
Files Reviewed
helpers/main.py(modified)tests/test_card_embed_evolution.py(added)ruff.toml(added)Findings
Correctness
No issues found.
The tier badge logic is correct end-to-end:
tier_badge = ""is initialized before the try block, so adb_getexception or a None/T0 response always falls through to an empty badge — the embed title is unaffected.['EVO' if tier >= 4 else f'T{tier}']correctly maps T1/T2/T3 as text prefixes and T4+ as[EVO]. Using>= 4rather than== 4is a safe future-proof choice if tier 5+ is ever added.evo_state.get("current_tier", 0) > 0correctly short-circuits on missing key, eliminating a potentialKeyError.card['id']is an integer from the API response. The f-string endpointf"evolution/cards/{card['id']}"follows the established pattern used incogs/admins.py(e.g.f"cards/{card_id}") and inhelpers/main.py:1396.db_getdefaults tonone_okay=True, so a 404 from the evolution endpoint returnsNonecleanly — consistent with spec ("missing → no badge").evo_statein the new block does not shadow anything used downstream. The existing Pokemon evolution code usesevo_mon, so there is no conflict.One very minor observation: the
except Exception: passat lines 113–116 silently swallows all errors from the evolution fetch. This is intentional per the PR spec ("API failure never breaks card display"), and consistent with the spirit of graceful degradation here. However, unlike every other exception block in this function (which all calllogging.error), this one leaves no trace in the log. Alogger.debug(...)call inside the except would make it significantly easier to diagnose if the evolution API degrades without anyone noticing. This is non-blocking — the behavior is correct as written.Security
No issues found.
card['id']is typed as an integer from a trusted API response; no user-controlled data flows into the endpoint string.Style & Conventions
No issues found. The ruff auto-fixes (f-string literals without interpolation → plain strings) are correct and conventional cleanup. The changes follow existing project patterns:
db_getusage,try/except Exception,discord.Embedconstruction.The two pre-existing
except Exception as e:→except Exception:changes at lines ~213 and ~224 also introduce a latent bug: thelogging.errorcall on the following line uses"could not pull evolution: {e}"(a regular string, not an f-string), so{e}is never interpolated — the log message has always been useless. This is pre-existing and out of scope for this PR, but worth noting.Test Coverage
Coverage is thorough and well-structured. All specified scenarios are addressed:
[Tx]prefix (exact assertions)[EVO]prefix (both positive and negative assertions — confirms[T4]is absent)The
_patch_db_gethelper cleanly routespaperdexcalls to a valid stub so the rest ofget_card_embedscan execute without raising onall_dex["count"]. The test design correctly handles the function's full execution path, not just the badge logic.The
_call_get_card_embedshelper handles bothlistand non-list returns defensively, which is correct sinceget_card_embedsis typed as-> list.ruff.toml
The file is new (no pre-existing
ruff.toml,pyproject.toml, or.ruff.tomlin the repo). The suppressions are scoped tohelpers/main.pyonly, which is the right approach. The four codes suppressed:F403/F405: star-import related (pre-existing, well documented in the comment)E722: bareexcept:(pre-existing pattern in this file)E721: type comparison with==instead ofisinstance(pre-existing)The scope is precise — no repo-wide blanket suppression. The inline comment explaining the rationale is adequate.
Verdict: APPROVED
The implementation is correct, well-tested, and follows project conventions. The graceful-degradation pattern (try/except with empty-badge fallback) is the right design for a non-critical display enhancement. The
ruff.tomlsuppression is appropriately scoped. One non-blocking suggestion: add alogger.debug(...)inside theexcept Exception: passblock to make silent API failures visible in debug logs.Automated review by Claude PR Reviewer
Hold — reconsider approach before merging.
This adds an API call to
get_card_embeds()for every card display. Before proceeding, we need to evaluate:get_card_embeds()is called per typical user session — if it's frequent (browsing collection, pack opens, game displays), the cumulative latency of an extra API round-trip per card could noticeably degrade the experienceDon't scrap the work — the badge UX itself is good. The implementation just needs to be more mindful of the cost per embed.
AI Code Review
Files Reviewed
helpers/main.py(modified)ruff.toml(added — conflicts with existing file onmain)tests/test_card_embed_evolution.py(added)Findings
Correctness
[BLOCKER] Badge labels are stale — Refractor rename not applied.
The Refractor rename decision was recorded on 2026-03-22 (one day before this PR's last update). All user-facing tier labels must use the new names:
[T1][Refractor](or abbreviated)[T2][Gold Refractor](or abbreviated)[T3][Superfractor](or abbreviated)[EVO][Superfractor](fully evolved = T3/T4 per the naming doc)The internal
evolution/cards/{card_id}endpoint path is acceptable per the rename doc ("internal API endpoints may keepevolutionnaming for now"). But the Discord-visible strings athelpers/main.pyline 118 ship[T1]/[T2]/[T3]/[EVO]which are not the approved player-facing names.The test suite hardcodes these same stale labels, so both files need updating together.
[BLOCKER]
ruff.tomlconflicts with existing file onmain— PR is not mergeable.mainalready has aruff.tomlwith global lint ignores:This PR adds a narrower file with
[lint.per-file-ignores]forhelpers/main.pyonly. Git cannot auto-merge these — hencemergeable: false. The resolution should merge both sets of rules into a single file rather than replacing the existing global configuration. Specifically,E722andE721added by this PR are not covered by the existing global ignores and should be added (either globally or as per-file-ignores), while preserving all existing rules.[MINOR] Broken log format string at
helpers/main.pylines ~215 and ~226.This is a pre-existing bug, but this PR changed
except Exception as e:toexcept Exception:at those two sites while leaving the log message as"could not pull evolution: {e}"— a plain string, not an f-string. The{e}is now a literal. Theexc_info=Truewill still capture the traceback, but the message itself will never interpolate the exception. This PR introduced the regression by removing theas ebinding. If a fix is out of scope for this PR, the lines should at minimum revert toexcept Exception as e:to restore the original (admittedly imperfect) behavior.Security
No issues found. The evolution API call is fire-and-forget with a broad
try/except; no user input is interpolated into the endpoint path beyondcard['id']which originates from a trusted API response.Style & Conventions
f"..."where no interpolation occurs) throughout the file is correct and follows ruff's F541 rule already configured in the existingruff.toml. No issues._make_card()factory pattern is consistent with the project's preference for factory data over mocks.Suggestions
tier >= 4threshold for[EVO]uses>=rather than== 4. If the API can return tier values above 4, this is correct defensive coding. If the max is exactly 4,== 4would be clearer. Worth confirming with the database schema._tier_badge(tier: int) -> strhelper extracted fromget_card_embedsonce the Refractor names are applied — the ternary is readable now but will be harder to read with longer badge strings like"[Gold Refractor]".Verdict: REQUEST_CHANGES
Two blockers must be addressed before merge:
helpers/main.pyandtests/test_card_embed_evolution.pyneed updating.ruff.tomlmerge conflict must be resolved by combining this PR'sper-file-ignoresadditions with the existing global[lint].ignorerules onmainrather than replacing the file.The core implementation — API call placement, error handling, color-from-rarity behavior, test coverage — is solid and correct.
Automated review by Claude PR Reviewer
- Badge labels: [R] Refractor, [GR] Gold Refractor, [SF] Superfractor, [SF★] fully evolved - Fix broken {e} log format strings (restore `as e` + add f-string prefix) - Restore ruff.toml from main (branch had stripped global config) - Update all test assertions for new badge names (11/11 pass) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>AI Code Review
Files Reviewed
helpers/main.py(modified)tests/test_card_embed_evolution.py(added)ruff.toml(added in PR, identical to main)Findings
Correctness
The core feature implementation is correct.
get_card_embeds()fetchesevolution/cards/{card_id}before building the embed, checkscurrent_tier > 0, and prepends the appropriate badge. The Refractor tier names match the project's naming convention per MEMORY.md ([R],[GR],[SF],[SF★]).One minor observation: the
TIER_BADGESdict{1: "R", 2: "GR", 3: "SF"}is instantiated insideget_card_embeds()on every call. This is inconsequential at this scale but could be a module-level constant. Not a blocking issue.The two
logging.errorcalls that previously used"could not pull evolution: {e}"(broken format strings —{e}never interpolated) are correctly fixed tof"could not pull evolution: {e}". This is a genuine bug fix.PR description mismatch: The PR body still references the old tier names (
[T1]/[T2]/[T3],[EVO]) from before the Refractor rename commit. The actual implementation uses[R]/[GR]/[SF]/[SF★]. The description is stale but the code and tests are consistent with each other — not a code defect.Security
No issues found. The evolution API response is accessed defensively with
.get("current_tier", 0)and the entire block is wrapped intry/except Exception. No user-controlled data flows into unsafe contexts.Style & Conventions
The cleanup of unnecessary f-strings (e.g.
f"Only you"→"Only you",f"Close Pack"→"Close Pack") is consistent with ruff's F541 rule and improves code quality throughout the file. These are correct autofixes, not style preferences.The
except ValueError as e:→except ValueError:andexcept Exception as e:→except Exception:changes are correct whereewas unused.The new
ruff.tomlin the PR diff is byte-for-byte identical to theruff.tomlalready onmain(added via PR #109). Gitea shows it as new in this PR because the branch predates #109 — it will merge cleanly with no conflict.Test Coverage
11 tests covering all specified scenarios:
None→ no badgeThe
_call_get_card_embedshelper is defined after the test classes that use it — this works correctly in Python since method bodies are resolved at call time. No issue.The
_patch_db_getrouting approach is clean: it handles theevolution/cards/prefix, thepaperdexendpoint, and falls back toNonefor everything else. This correctly isolates the unit under test without requiring a full mock of the entirehelpers.mainmodule.Suggestions
TIER_BADGES = {1: "R", 2: "GR", 3: "SF"}could be a module-level constant to avoid re-instantiating on every card embed call. Low priority.[R]/[GR]/[SF]/[SF★]instead of[T1]/[T2]/[T3]/[EVO]) for accuracy in the merge history. This is cosmetic.Verdict: APPROVED
The implementation is correct, the Refractor tier names match the project's established naming convention, the API fallback is robust, and all 11 tests cover the specified scenarios. The f-string fixes and ruff cleanup are welcome improvements. No blocking issues.
Automated review by Claude PR Reviewer
AI Code Review
Files Reviewed
helpers/main.py(modified)tests/test_card_embed_evolution.py(added)ruff.toml(added)Findings
Correctness
[BLOCKER] PR body describes the wrong badge labels.
The PR description still documents the original spec:
[T1]/[T2]/[T3]for tiers 1–3 and[EVO]for tier 4. The actual code in the diff correctly implements the updated tier spec:[BC],[R],[GR],[SF]. The description is misleading for anyone reviewing or reverting this PR. The PR body should be updated to match the implementation.[MINOR]
TIER_BADGESdict is defined inside the hot path.TIER_BADGES = {1: "BC", 2: "R", 3: "GR", 4: "SF"}is constructed on every call toget_card_embeds(), even whenevo_stateexists andcurrent_tier > 0. This is a constant; it should be module-level or at minimum defined outside theifblock. In a high-traffic bot this is unnecessary per-call allocation.Badge fallback for unknown tiers (
TIER_BADGES.get(tier, 'SF')).If the database ever returns a
current_tiervalue above 4, the fallback silently renders[SF], which may be incorrect rather than no badge. This is a minor risk given current API contracts but worth noting.Badge for
current_tier == 0is correctly suppressed — the> 0guard is sound.evo_stateNone check is correct —if evo_state and evo_state.get(...)properly handles 404 responses.f-string fixes in existing logging calls are correct — the two
logging.error(f"could not pull evolution: {e}")lines previously used a plain string (nof), so{e}was never interpolated. This is a genuine bug fix included in the diff.Security
No issues found. The evolution API endpoint is constructed with
card['id'], which is an integer from a trusted upstream API response — not a user-supplied string. No injection surface.Style & Conventions
The large volume of
f""→""string literal cleanups (removing redundant f-prefixes) is correct and consistent with ruff F541. These are all pre-existing style issues thatruff --fixresolved. They are in-scope given the PR body's explanation.ruff.tomlis well-structured with clear per-file suppression rationale in comments. SuppressingE722(bareexcept) globally forhelpers/**is broad, but the PR body explains this correctly — the new bareexcept Exception:inget_card_embeds()follows the pattern established by existing code in the file, and the fallback-on-failure behavior is intentional.Test Coverage
Test file is thorough. All 5 tier states (T0–T4), API 404 (None), API exception, color immutability, and badge format/position are covered. Docstrings on each test case explain what and why — matches project convention.
One observation:
_call_get_card_embedsuses a late import (from helpers.main import get_card_embeds) inside the helper. This works but means the import happens on every test invocation. Prefer a module-level import with a conftest or fixture if this test file grows. Not a blocker.The test file does not exist in the working tree at
tests/test_card_embed_evolution.py— it exists only in the PR branch. Confirmed from the diff.Suggestions
TIER_BADGESto module level (e.g., adjacent to other constants at the top ofhelpers/main.py) to avoid recreating it on each call.[BC],[R],[GR],[SF]) and the "Refractor" tier naming — the description currently documents the superseded[T1]/[T2]/[T3]/[EVO]spec, which creates confusion in the commit history.Verdict: REQUEST_CHANGES
The implementation and tests are correct against the current tier spec. The PR body documents the wrong badge scheme (
[T1]/[T2]/[T3]/[EVO]) — it predates the Refractor naming update. This is the one required change before merging: the description needs to accurately reflect what is shipping ([BC],[R],[GR],[SF]). TheTIER_BADGESconstant placement is a minor quality issue that should also be addressed.Automated review by Claude PR Reviewer
@ -125,0 +114,4 @@try:evo_state = await db_get(f"evolution/cards/{card['id']}")if evo_state and evo_state.get("current_tier", 0) > 0:tier = evo_state["current_tier"]TIER_BADGES is a constant defined inside the conditional on every call. Move it to module level to avoid per-invocation allocation.
@ -125,0 +115,4 @@evo_state = await db_get(f"evolution/cards/{card['id']}")if evo_state and evo_state.get("current_tier", 0) > 0:tier = evo_state["current_tier"]TIER_BADGES = {1: "BC", 2: "R", 3: "GR", 4: "SF"}TIER_BADGES.get(tier, 'SF') silently renders [SF] for any unrecognized tier value > 4. Consider returning no badge (empty string) as the fallback instead of defaulting to the highest tier.
feat: tier badge prefix in card embed title (WP-12) (#77)to feat: Refractor tier badge prefix in card embed title (WP-12) (#77)AI Code Review
Files Reviewed
helpers/main.py(modified)tests/test_card_embed_evolution.py(added)ruff.toml(added)Findings
Correctness
TIER_BADGES dict is correct.
{1: "BC", 2: "R", 3: "GR", 4: "SF"}matches the spec exactly. T0 has no entry, soTIER_BADGES.get(0)returnsNoneand no badge is prepended. The guardevo_state.get("current_tier", 0) > 0also prevents any badge on T0. Both layers are consistent and correct.Badge format is correct. The template
f"[{badge}] "produces[BC],[R],[GR],[SF]with a trailing space before the player name. This matches the required format.API call placement is correct. The
evolution/cards/{card_id}fetch happens before thediscord.Embedis constructed, so the title is composed correctly in one step rather than mutated after the fact.f-string bug fix is correct. The two
logging.error("could not pull evolution: {e}", ...)calls on lines 226 and 237 ofmainwere broken (missing f-prefix). The PR adds the f-prefix, fixing silent variable interpolation failures. Good catch.Dead import removal is correct. The imports removed by the PR (
os,traceback,pygsheets,get_close_matches,dataclass,Literal,log_exception, and severalutilssymbols) are confirmed unused in the file body. These are safe removals.Unnecessary f-string literals cleaned up. The many
f"Only you",f"Close Pack",f"Cancel", etc. changes are cosmetically correct and would eventually be caught by ruff F541.Security
No issues found. The
card['id']value interpolated into the API endpoint path is sourced from the application's own database response, not from user input. No injection risk.Style & Conventions
Test file naming is inconsistent with the Refractor rename. This PR is part of the Evolution-to-Refractor rename, but the new test file is named
tests/test_card_embed_evolution.py. It should betests/test_card_embed_refractor.pyto match the rename initiative. This is the primary blocking issue.The module-level docstring in the test file also reads "Tests for WP-12: Tier Badge on Card Embed" and refers to "evolution progress" in the description — these should say "Refractor" to be consistent.
_call_get_card_embedsis a module-level helper, not a fixture. The pattern of defining_call_get_card_embedsas a plain async function at the bottom of the test file is slightly unconventional — most pytest projects would make this a@pytest.fixtureor inline the call. This is not a blocker but worth noting.TIER_BADGEScomment is accurate. The inline comment# Refractor tier badge prefixes for card embed title (T0 = no badge)correctly uses Refractor terminology. No issue.Suggestions
_patch_db_gethelper docstring says "so other get_card_embeds internals still behave" — this is accurate but could note that thepaperdexandplays/endpoints are stubbed out. Minor.current_tier=5or a future unknown tier to confirm the safe-fallback path produces no badge. Atest_unknown_tier_no_badgecase would round out coverage, though all defined tiers are tested.Verdict: REQUEST_CHANGES
One change is required before merge: rename
tests/test_card_embed_evolution.pytotests/test_card_embed_refractor.pyand update the module docstring to use "Refractor" terminology. This is a Refractor rename PR — shipping a new test file with "evolution" in the name is directly contrary to the rename goal and will require a follow-up cleanup commit.The core implementation (badge logic, fallback behavior, API call, f-string fix, dead import removal, ruff config) is correct and well-tested.
Automated review by Claude PR Reviewer
AI Code Review
Files Reviewed
helpers/main.py(modified)tests/test_card_embed_refractor.py(added)ruff.toml(added)Findings
Correctness
TIER_BADGES = {1: "BC", 2: "R", 3: "GR", 4: "SF"}at module level is the right pattern — no magic strings scattered through the function. The conditionalif evo_state and evo_state.get("current_tier", 0) > 0correctly suppresses the badge for T0 (base card) and for aNoneAPI response.evolution/cards/{card_id}is correct. Despite the task context mentioning a/api/v2/refractor/*rename, the database API router is still registered at/api/v2/evolutionin its currentmainbranch. The rename has not shipped. Using the old endpoint is correct behavior right now.logging.error("could not pull evolution: {e}", ...)bug fixes (missing f-prefix) are valid and were pre-existing bugs.N+1 API Call Pattern
The prior review flagged this and it is still present:
display_cardscallsget_card_embedsin a list comprehension, and eachget_card_embedscall now issues an individualdb_get("evolution/cards/{card_id}"). For a standard 5-card pack this means 5 serial HTTP requests added to the existing embed-build latency.This is not a blocker for this PR. The database API has no batch endpoint for evolution card states (
GET /evolution/cardsdoes not exist — onlyGET /evolution/cards/{card_id}), so a batch approach would require a database-side change first. Thetry/exceptwrapper ensures failures are silent and non-blocking. This should be tracked as a follow-up: add a batch state endpoint on the database side, then refactordisplay_cardsto pre-fetch all states in one call before building embeds.Security
No issues found. No new user inputs reach the API call —
card['id']is an integer from the database response, not user-supplied input.Style & Conventions
test_card_embed_refractor.py— the blocker from the prior review is resolved.tests/test_card_embed_evolution.pyin the "Files changed" section — cosmetic inconsistency in the PR body only, does not affect code.ruff.tomlrestoration is clean and matches the documented pattern for legacy per-file suppressions.Suggestions
GET /api/v2/evolution/cards?card_ids=1,2,3batch endpoint on the database side, then refactordisplay_cardsto pre-fetch all Refractor states before the embed-build loop.test_card_embed_evolution.py) — worth updating for accuracy if this PR is referenced later.Verdict: APPROVED
The previous blocker (test file naming) is resolved. The implementation is correct against the current live API, the fallback is robust, and test coverage is thorough. The N+1 pattern is a known architectural limitation with no available batch endpoint today — it should be addressed in a follow-up database PR, not here.
Automated review by Claude PR Reviewer
Approved — reviewer sign-off confirmed. Merging.