cal/paper-dynasty-discord
1
0
Fork 0
Code Issues 14 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

fix: daily check-in interaction migration + paperdex dupe detection #107

Merged
Claude merged 2 commits from fix/checkin-and-paperdex-fixes into main 2026-03-23 04:43:05 +00:00
Conversation 0 Commits 2 Files Changed 2 +16 -9
cal commented 2026-03-23 04:35:16 +00:00
Owner
Copy Link

Fixes #19, Fixes #23

Summary

  • Fix #19: Complete the daily_checkin migration to discord.Interaction. Removed two greeting = assignments and replaced await greeting.edit(...) with await interaction.edit_original_response(...). Also removed the # TODO: complete the migration to an interaction comment.
  • Fix #23: Implement paperdex dupe detection in get_card_embeds() (helpers/main.py). The commented-out TODO block is replaced with working code that queries the cards API by player_id + team_id, then adds a "Dupes" field to the embed showing how many duplicate copies the team owns.

Files changed

  • cogs/economy.py — 4 lines changed in daily_checkin
  • helpers/main.py — 5 lines replaced in get_card_embeds

Test plan

  • Trigger /comeonmanineedthis on a 2nd, 4th, or 5th check-in and confirm the bot responds via interaction (no greeting AttributeError)
  • Trigger /comeonmanineedthis when roll_for_cards returns nothing and confirm the failure message appears correctly
  • View a card embed for a team that owns multiple copies of a player — confirm a "Dupes: N dupes" field appears
  • View a card embed for a Paper Dynasty team — confirm no "Dupes" field is added

🤖 Generated with Claude Code

Fixes #19, Fixes #23 ## Summary - **Fix #19**: Complete the `daily_checkin` migration to `discord.Interaction`. Removed two `greeting =` assignments and replaced `await greeting.edit(...)` with `await interaction.edit_original_response(...)`. Also removed the `# TODO: complete the migration to an interaction` comment. - **Fix #23**: Implement paperdex dupe detection in `get_card_embeds()` (`helpers/main.py`). The commented-out TODO block is replaced with working code that queries the cards API by `player_id` + `team_id`, then adds a "Dupes" field to the embed showing how many duplicate copies the team owns. ## Files changed - `cogs/economy.py` — 4 lines changed in `daily_checkin` - `helpers/main.py` — 5 lines replaced in `get_card_embeds` ## Test plan - [ ] Trigger `/comeonmanineedthis` on a 2nd, 4th, or 5th check-in and confirm the bot responds via interaction (no `greeting` AttributeError) - [ ] Trigger `/comeonmanineedthis` when `roll_for_cards` returns nothing and confirm the failure message appears correctly - [ ] View a card embed for a team that owns multiple copies of a player — confirm a "Dupes: N dupes" field appears - [ ] View a card embed for a Paper Dynasty team — confirm no "Dupes" field is added 🤖 Generated with [Claude Code](https://claude.com/claude-code)
cal added 1 commit 2026-03-23 04:35:17 +00:00
fix: daily check-in interaction migration + paperdex dupe detection
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m23s
Details
9d71c5a2d1 
Fix 1 (closes #19): Complete the migration of daily_checkin to
discord.Interaction. Remove greeting = assignments and TODO comment;
replace await greeting.edit(...) with await interaction.edit_original_response(...).

Fix 2 (closes #23): Implement paperdex dupe detection in get_card_embeds().
Query cards API by player_id + team_id and display a 'Dupes' field on the
embed showing how many duplicate copies the team owns.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cal reviewed 2026-03-23 04:36:58 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review

Files Reviewed

  • cogs/economy.py (modified)
  • helpers/main.py (modified)

Findings

Correctness

cogs/economy.py — daily_checkin migration (Fix #19): LGTM

All three references to greeting are correctly eliminated. The two greeting = await interaction.edit_original_response(...) assignments are stripped to bare await interaction.edit_original_response(...) calls (lines 562 and 577 in the working file), and the final await greeting.edit(...) on the failure path is replaced with await interaction.edit_original_response(...) (line 602). The TODO comment is removed. The method is fully migrated and should behave correctly with discord.Interaction.

helpers/main.py — paperdex dupe detection (Fix #23): BUG

db_get can return None when the API responds with a non-200 status (see api_calls.py lines 103–106 — the default none_okay=True means failures silently return None). The new code does not guard against this:

team_dex = await db_get('cards', params=[...])
copy_count = team_dex['count'] if team_dex['count'] else 1  # TypeError if team_dex is None

If the cards API call fails, team_dex will be None and team_dex['count'] raises a TypeError, crashing embed generation for any non-Paper-Dynasty card whenever the API is degraded. A guard is required:

team_dex = await db_get('cards', params=[("player_id", card["player"]["player_id"]), ('team_id', card['team']['id'])])
if team_dex is not None:
    copy_count = team_dex['count'] if team_dex['count'] else 1
    dupe_count = copy_count - 1
    embed.add_field(name='Dupes', value=f'{dupe_count} dupe{"s" if dupe_count != 1 else ""}')

There is also a secondary logic issue: copy_count = team_dex['count'] if team_dex['count'] else 1 — if the API returns count: 0, copy_count is set to 1, producing -1 dupes. The original commented-out code had the same flaw. Consider dupe_count = max(0, (team_dex['count'] or 1) - 1) or skip the field entirely when count is 0.

Security

No issues found.

Style & Conventions

No issues found. The new code follows the existing patterns in helpers/main.py — same quoting style, same db_get call pattern, same embed field API. The field name Dupes (vs the old commented # Dupes) is correct.

Suggestions

  • The copy_count default-to-1 fallback when count is 0 is semantically wrong — -1 dupes is nonsensical. Use max(0, ...) or skip the field when count is 0.

Verdict: REQUEST_CHANGES

The economy.py migration is clean and correct. The helpers/main.py change has a real crash bug: db_get returns None on API errors (the default none_okay=True behavior), and the new code accesses team_dex['count'] without a None check. On any API degradation, embed generation for all non-Paper-Dynasty cards will throw a TypeError. Please add an if team_dex is not None: guard around the dupe block before merging.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `cogs/economy.py` (modified) - `helpers/main.py` (modified) ### Findings #### Correctness **`cogs/economy.py` — daily_checkin migration (Fix #19): LGTM** All three references to `greeting` are correctly eliminated. The two `greeting = await interaction.edit_original_response(...)` assignments are stripped to bare `await interaction.edit_original_response(...)` calls (lines 562 and 577 in the working file), and the final `await greeting.edit(...)` on the failure path is replaced with `await interaction.edit_original_response(...)` (line 602). The TODO comment is removed. The method is fully migrated and should behave correctly with `discord.Interaction`. **`helpers/main.py` — paperdex dupe detection (Fix #23): BUG** `db_get` can return `None` when the API responds with a non-200 status (see `api_calls.py` lines 103–106 — the default `none_okay=True` means failures silently return `None`). The new code does not guard against this: ```python team_dex = await db_get('cards', params=[...]) copy_count = team_dex['count'] if team_dex['count'] else 1 # TypeError if team_dex is None ``` If the cards API call fails, `team_dex` will be `None` and `team_dex['count']` raises a `TypeError`, crashing embed generation for any non-Paper-Dynasty card whenever the API is degraded. A guard is required: ```python team_dex = await db_get('cards', params=[("player_id", card["player"]["player_id"]), ('team_id', card['team']['id'])]) if team_dex is not None: copy_count = team_dex['count'] if team_dex['count'] else 1 dupe_count = copy_count - 1 embed.add_field(name='Dupes', value=f'{dupe_count} dupe{"s" if dupe_count != 1 else ""}') ``` There is also a secondary logic issue: `copy_count = team_dex['count'] if team_dex['count'] else 1` — if the API returns `count: 0`, `copy_count` is set to `1`, producing `-1 dupes`. The original commented-out code had the same flaw. Consider `dupe_count = max(0, (team_dex['count'] or 1) - 1)` or skip the field entirely when count is 0. #### Security No issues found. #### Style & Conventions No issues found. The new code follows the existing patterns in `helpers/main.py` — same quoting style, same `db_get` call pattern, same embed field API. The field name `Dupes` (vs the old commented `# Dupes`) is correct. #### Suggestions - The `copy_count` default-to-1 fallback when count is 0 is semantically wrong — -1 dupes is nonsensical. Use `max(0, ...)` or skip the field when count is 0. ### Verdict: REQUEST_CHANGES The `economy.py` migration is clean and correct. The `helpers/main.py` change has a real crash bug: `db_get` returns `None` on API errors (the default `none_okay=True` behavior), and the new code accesses `team_dex['count']` without a None check. On any API degradation, embed generation for all non-Paper-Dynasty cards will throw a `TypeError`. Please add an `if team_dex is not None:` guard around the dupe block before merging. --- *Automated review by Claude PR Reviewer*
helpers/main.py Outdated
@ -187,2 +184,2 @@
# count = 1 if not team_dex['count'] else team_dex['count']
# embed.add_field(name='# Dupes', value=f'{count - 1} dupe{"s" if count - 1 != 1 else ""}')
if card['team']['lname'] != 'Paper Dynasty':
team_dex = await db_get('cards', params=[("player_id", card["player"]["player_id"]), ('team_id', card['team']['id'])])
cal commented 2026-03-23 04:36:57 +00:00
Author
Owner
Copy Link

db_get defaults to none_okay=True, meaning it returns None on any non-200 API response. team_dex['count'] on the next line will raise a TypeError if the cards API is unavailable or returns an error. Wrap this block in if team_dex is not None: to prevent crashing embed generation for all non-Paper-Dynasty cards.

`db_get` defaults to `none_okay=True`, meaning it returns `None` on any non-200 API response. `team_dex['count']` on the next line will raise a `TypeError` if the cards API is unavailable or returns an error. Wrap this block in `if team_dex is not None:` to prevent crashing embed generation for all non-Paper-Dynasty cards.
cal added 1 commit 2026-03-23 04:38:20 +00:00
fix: guard paperdex dupe detection against None API response
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m30s
Details
ee01fce0b8 
db_get returns None on API errors. Added None guard and fixed
dupe count math to use max(0, count - 1) instead of ternary
that produced -1 dupes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cal reviewed 2026-03-23 04:41:27 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review (Re-review after fix)

Verdict: APPROVED — Gitea blocks self-approval, so this is posted as a COMMENT, but the verdict is a clean approve.

Files Reviewed

  • helpers/main.py (modified — dupe detection block)
  • cogs/economy.py (modified — daily check-in migration)

Findings

Correctness

None guard (helpers/main.py): The previous crash path is correctly closed. db_get returning None now skips the entire dupe block — if team_dex is not None: wraps both the dupe_count calculation and the embed.add_field call. No crash can occur.

Dupe count math (helpers/main.py): max(0, team_dex["count"] - 1) is correct for all boundary cases:

  • count = 0 → max(0, -1) = 0 dupes (handled safely)
  • count = 1 → max(0, 0) = 0 dupes (you own the only copy, correct)
  • count = 2 → 1 dupe (correct)
  • The old ternary could produce -1; this cannot.

daily_checkin fix (cogs/economy.py): Verified on the branch directly. Both greeting = assignments are removed, the dangling await greeting.edit(...) is replaced with await interaction.edit_original_response(...), and the TODO comment is gone. No reference to greeting remains in this function.

Security

No issues found.

Style & Conventions

No issues found. The new block uses consistent double-quote strings and matches surrounding code style.

Suggestions

  • The if team_dex is not None: guard silently omits the "Dupes" field when db_get returns None. This is reasonable behavior — a missing field beats a crash — but a log line here would help diagnose unexpected None returns in production. Minor; does not block merging.

Summary

Both reported issues are correctly fixed. The None guard closes the crash path cleanly, the dupe count math is correct at all boundary values, and the economy.py interaction migration is fully applied with no remaining greeting references. No new issues introduced.

Automated review by Claude PR Reviewer

## AI Code Review (Re-review after fix) **Verdict: APPROVED** — Gitea blocks self-approval, so this is posted as a COMMENT, but the verdict is a clean approve. ### Files Reviewed - `helpers/main.py` (modified — dupe detection block) - `cogs/economy.py` (modified — daily check-in migration) ### Findings #### Correctness **None guard (helpers/main.py):** The previous crash path is correctly closed. `db_get` returning `None` now skips the entire dupe block — `if team_dex is not None:` wraps both the `dupe_count` calculation and the `embed.add_field` call. No crash can occur. **Dupe count math (helpers/main.py):** `max(0, team_dex["count"] - 1)` is correct for all boundary cases: - `count = 0` → `max(0, -1)` = 0 dupes (handled safely) - `count = 1` → `max(0, 0)` = 0 dupes (you own the only copy, correct) - `count = 2` → 1 dupe (correct) - The old ternary could produce -1; this cannot. **daily_checkin fix (cogs/economy.py):** Verified on the branch directly. Both `greeting =` assignments are removed, the dangling `await greeting.edit(...)` is replaced with `await interaction.edit_original_response(...)`, and the TODO comment is gone. No reference to `greeting` remains in this function. #### Security No issues found. #### Style & Conventions No issues found. The new block uses consistent double-quote strings and matches surrounding code style. #### Suggestions - The `if team_dex is not None:` guard silently omits the "Dupes" field when `db_get` returns `None`. This is reasonable behavior — a missing field beats a crash — but a log line here would help diagnose unexpected `None` returns in production. Minor; does not block merging. ### Summary Both reported issues are correctly fixed. The None guard closes the crash path cleanly, the dupe count math is correct at all boundary values, and the economy.py interaction migration is fully applied with no remaining `greeting` references. No new issues introduced. --- *Automated review by Claude PR Reviewer*
cal force-pushed fix/checkin-and-paperdex-fixes from ee01fce0b8 to 1c03d91478 2026-03-23 04:42:55 +00:00 Compare
Claude merged commit 9167bc2f1c into main 2026-03-23 04:43:05 +00:00
Claude deleted branch fix/checkin-and-paperdex-fixes 2026-03-23 04:43:06 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working

   
ai-working

   
bug

   
enhancement

   
feature

New feature proposal

  
in-queue

Approved for development

  
performance

   
security

   
tech-debt

   
todo
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
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 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-discord#107
No description provided.
Delete Branch "fix/checkin-and-paperdex-fixes"

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?