cal/major-domo-v2
1
0
Fork 0
Code Issues 6 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

feat: add is_admin() helper to utils/permissions.py (#55) #64

Merged
cal merged 1 commits from ai/major-domo-v2#55 into next-release 2026-03-09 14:38:08 +00:00
Conversation 0 Commits 1 Files Changed 3 +66 -28
cal commented 2026-03-05 04:33:43 +00:00
Owner
Copy Link

Summary

  • Added is_admin(interaction: discord.Interaction) -> bool helper to utils/permissions.py that centralizes the admin check with the correct isinstance(interaction.user, discord.Member) guard
  • Updated can_edit_player_image() in commands/profile/images.py to use is_admin() instead of directly accessing interaction.user.guild_permissions.administrator — the direct access would raise AttributeError in DM contexts where user is a discord.User not a discord.Member
  • Updated the corresponding test to mock interaction.user with spec=discord.Member so the isinstance guard passes

Files Changed

  • utils/permissions.py — added is_admin() helper after is_league_server()
  • commands/profile/images.py — import and use is_admin() in can_edit_player_image()
  • tests/test_commands_profile_images.py — fix admin test mock to use spec=discord.Member

Test Results

930 passed, 3 skipped

Other observations

Several other places in the codebase access guild_permissions.administrator directly without the isinstance guard (e.g. commands/admin/management.py, commands/admin/users.py, commands/help/main.py, commands/utilities/charts.py). These are out of scope for this issue but could be migrated to is_admin() as follow-up work (issue #54 proposes a broader refactor).

## Summary - Added `is_admin(interaction: discord.Interaction) -> bool` helper to `utils/permissions.py` that centralizes the admin check with the correct `isinstance(interaction.user, discord.Member)` guard - Updated `can_edit_player_image()` in `commands/profile/images.py` to use `is_admin()` instead of directly accessing `interaction.user.guild_permissions.administrator` — the direct access would raise `AttributeError` in DM contexts where `user` is a `discord.User` not a `discord.Member` - Updated the corresponding test to mock `interaction.user` with `spec=discord.Member` so the `isinstance` guard passes ## Files Changed - `utils/permissions.py` — added `is_admin()` helper after `is_league_server()` - `commands/profile/images.py` — import and use `is_admin()` in `can_edit_player_image()` - `tests/test_commands_profile_images.py` — fix admin test mock to use `spec=discord.Member` ## Test Results 930 passed, 3 skipped ## Other observations Several other places in the codebase access `guild_permissions.administrator` directly without the `isinstance` guard (e.g. `commands/admin/management.py`, `commands/admin/users.py`, `commands/help/main.py`, `commands/utilities/charts.py`). These are out of scope for this issue but could be migrated to `is_admin()` as follow-up work (issue #54 proposes a broader refactor).
cal added 1 commit 2026-03-05 04:33:44 +00:00
feat: add is_admin() helper to utils/permissions.py (#55)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m9s
Details
ed40b532b5 
Add centralized `is_admin(interaction)` helper that includes the
`isinstance(interaction.user, discord.Member)` guard, preventing
AttributeError in DM contexts.

Use it in `can_edit_player_image()` which previously accessed
`guild_permissions.administrator` directly without the isinstance
guard. Update the corresponding test to mock the user with
`spec=discord.Member` so the isinstance check passes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cal added the
ai-reviewing
 label 2026-03-05 04:45:31 +00:00
cal reviewed 2026-03-05 04:47:17 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review

Files Reviewed

  • utils/permissions.py (modified — is_admin() helper added)
  • commands/profile/images.py (modified — can_edit_player_image() updated)
  • tests/test_commands_profile_images.py (modified — admin mock fixed)

Findings

Correctness

  • is_admin() implementation is correct. The short-circuit isinstance(interaction.user, discord.Member) and interaction.user.guild_permissions.administrator safely guards against DM contexts where user is discord.User and has no guild_permissions attribute.
  • can_edit_player_image() correctly delegates to is_admin(), eliminating the bare interaction.user.guild_permissions.administrator access that would AttributeError in DMs.
  • The MagicMock(spec=discord.Member) fix in the test is correct: spec causes Python to set __class__ to discord.Member on the mock, so isinstance(mock, discord.Member) returns True as the new is_admin() guard requires.
  • The import discord added to the test file is a required dependency for MagicMock(spec=discord.Member) — not an unused import.

Security

  • No issues. Admin check logic is consistent with the existing admin_only() decorator pattern in the same file.

Style & Conventions

  • Bulk of the diff is cosmetic reformatting (single → double quotes, trailing commas, Black-style line wrapping). All consistent with the project's formatter output.
  • is_admin() is placed after is_league_server() alongside other non-decorator helpers — correct location.
  • from utils.permissions import is_admin import added correctly alongside other utils.* imports in images.py.

Suggestions

  • The admin_only() decorator in permissions.py duplicates the same isinstance + guild_permissions.administrator logic inline. It could eventually delegate to is_admin(), but the decorator has a different flow (early-return with user-facing error messages after a guild-presence check), so keeping them separate is reasonable for now. Issue #54 tracks the broader refactor.

Verdict: APPROVED ✅

Clean, well-scoped fix. The is_admin() helper correctly centralizes the DM-safe admin check, the call site in can_edit_player_image() is properly updated, and the test mock is correctly patched to exercise the new code path. All 930 tests passing. Ready to merge.

Note: Gitea prevented posting as APPROVED (self-review restriction). Verdict is APPROVED — no changes required.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `utils/permissions.py` (modified — `is_admin()` helper added) - `commands/profile/images.py` (modified — `can_edit_player_image()` updated) - `tests/test_commands_profile_images.py` (modified — admin mock fixed) ### Findings #### Correctness - `is_admin()` implementation is correct. The short-circuit `isinstance(interaction.user, discord.Member) and interaction.user.guild_permissions.administrator` safely guards against DM contexts where `user` is `discord.User` and has no `guild_permissions` attribute. - `can_edit_player_image()` correctly delegates to `is_admin()`, eliminating the bare `interaction.user.guild_permissions.administrator` access that would `AttributeError` in DMs. - The `MagicMock(spec=discord.Member)` fix in the test is correct: `spec` causes Python to set `__class__` to `discord.Member` on the mock, so `isinstance(mock, discord.Member)` returns `True` as the new `is_admin()` guard requires. - The `import discord` added to the test file is a required dependency for `MagicMock(spec=discord.Member)` — not an unused import. #### Security - No issues. Admin check logic is consistent with the existing `admin_only()` decorator pattern in the same file. #### Style & Conventions - Bulk of the diff is cosmetic reformatting (single → double quotes, trailing commas, Black-style line wrapping). All consistent with the project's formatter output. - `is_admin()` is placed after `is_league_server()` alongside other non-decorator helpers — correct location. - `from utils.permissions import is_admin` import added correctly alongside other `utils.*` imports in `images.py`. #### Suggestions - The `admin_only()` decorator in `permissions.py` duplicates the same `isinstance` + `guild_permissions.administrator` logic inline. It could eventually delegate to `is_admin()`, but the decorator has a different flow (early-return with user-facing error messages after a guild-presence check), so keeping them separate is reasonable for now. Issue #54 tracks the broader refactor. ### Verdict: APPROVED ✅ Clean, well-scoped fix. The `is_admin()` helper correctly centralizes the DM-safe admin check, the call site in `can_edit_player_image()` is properly updated, and the test mock is correctly patched to exercise the new code path. All 930 tests passing. Ready to merge. > Note: Gitea prevented posting as APPROVED (self-review restriction). Verdict is APPROVED — no changes required. --- *Automated review by Claude PR Reviewer*
cal added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-05 04:47:41 +00:00
cal changed target branch from main to next-release 2026-03-07 03:27:30 +00:00
cal force-pushed ai/major-domo-v2#55 from ed40b532b5 to c41166d53c 2026-03-09 14:37:41 +00:00 Compare
cal merged commit 4314b67537 into next-release 2026-03-09 14:38:08 +00:00
cal deleted branch ai/major-domo-v2#55 2026-03-09 14:38:08 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working

   
in-next-release

Fix included in the next-release branch, pending merge to main

  
status/in-progress

Work is actively in progress

  
status/pr-open

A pull request has been opened for this issue

No Label
ai-changes-requested
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
in-next-release
status/in-progress
status/pr-open
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/major-domo-v2#64
No description provided.
Delete Branch "ai/major-domo-v2#55"

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?