fix: replace broken interaction_check decorator with MaintenanceAwareTree subclass #83
40
bot.py
40
bot.py
@ -64,6 +64,44 @@ def setup_logging():
|
||||
return logger
|
||||
|
||||
|
||||
class MaintenanceAwareTree(discord.app_commands.CommandTree):
|
||||
"""
|
||||
CommandTree subclass that gates all interactions behind a maintenance mode check.
|
||||
|
||||
When bot.maintenance_mode is True, non-administrator users receive an ephemeral
|
||||
error message and the interaction is blocked. Administrators are always allowed
|
||||
through. When maintenance_mode is False the check is a no-op and every
|
||||
interaction proceeds normally.
|
||||
|
||||
This is the correct way to register a global interaction_check for app commands
|
||||
in discord.py — overriding the method on a CommandTree subclass passed via
|
||||
tree_cls rather than attempting to assign a decorator to self.tree inside
|
||||
setup_hook.
|
||||
"""
|
||||
|
||||
async def interaction_check(self, interaction: discord.Interaction) -> bool:
|
||||
"""Allow admins through; block everyone else when maintenance mode is active."""
|
||||
bot = interaction.client # type: ignore[assignment]
|
||||
|
||||
# If maintenance mode is off, always allow.
|
||||
if not getattr(bot, "maintenance_mode", False):
|
||||
return True
|
||||
|
||||
# Maintenance mode is on — let administrators through unconditionally.
|
||||
if (
|
||||
isinstance(interaction.user, discord.Member)
|
||||
and interaction.user.guild_permissions.administrator
|
||||
):
|
||||
return True
|
||||
|
||||
# Block non-admin users with an ephemeral notice.
|
||||
await interaction.response.send_message(
|
||||
"The bot is currently in maintenance mode. Please try again later.",
|
||||
ephemeral=True,
|
||||
)
|
||||
return False
|
||||
|
||||
|
||||
class SBABot(commands.Bot):
|
||||
"""Custom bot class for SBA league management."""
|
||||
|
||||
@ -77,8 +115,10 @@ class SBABot(commands.Bot):
|
||||
command_prefix="!", # Legacy prefix, primarily using slash commands
|
||||
intents=intents,
|
||||
description="Major Domo v2.0",
|
||||
tree_cls=MaintenanceAwareTree,
|
||||
)
|
||||
|
||||
self.maintenance_mode: bool = False
|
||||
self.logger = logging.getLogger("discord_bot_v2")
|
||||
self.maintenance_mode: bool = False
|
||||
|
||||
|
||||
@ -490,7 +490,7 @@ class AdminCommands(commands.Cog):
|
||||
await interaction.response.defer()
|
||||
|
||||
is_enabling = mode.lower() == "on"
|
||||
self.bot.maintenance_mode = is_enabling
|
||||
self.bot.maintenance_mode = is_enabling # type: ignore[attr-defined]
|
||||
self.logger.info(
|
||||
f"Maintenance mode {'enabled' if is_enabling else 'disabled'} by {interaction.user} (id={interaction.user.id})"
|
||||
)
|
||||
|
||||
282
tests/test_bot_maintenance_tree.py
Normal file
282
tests/test_bot_maintenance_tree.py
Normal file
@ -0,0 +1,282 @@
|
||||
"""
|
||||
Tests for MaintenanceAwareTree and the maintenance_mode attribute on SBABot.
|
||||
|
||||
What:
|
||||
Verifies that the CommandTree subclass correctly gates interactions behind
|
||||
bot.maintenance_mode. When maintenance mode is off every interaction is
|
||||
allowed through unconditionally. When maintenance mode is on, non-admin
|
||||
users receive an ephemeral error message and the check returns False, while
|
||||
administrators are always allowed through.
|
||||
|
||||
Why:
|
||||
The original code attempted to register an interaction_check via a decorator
|
||||
on self.tree inside setup_hook. That is not a valid pattern in discord.py —
|
||||
interaction_check is an overridable async method on CommandTree, not a
|
||||
decorator. The broken assignment caused a RuntimeWarning (unawaited
|
||||
coroutine) and silently made maintenance mode a no-op. These tests confirm
|
||||
the correct subclass-based implementation behaves as specified.
|
||||
"""
|
||||
|
||||
import pytest
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import discord
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Helpers / fixtures
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _make_bot(maintenance_mode: bool = False) -> MagicMock:
|
||||
"""Return a minimal mock bot with a maintenance_mode attribute."""
|
||||
bot = MagicMock()
|
||||
bot.maintenance_mode = maintenance_mode
|
||||
return bot
|
||||
|
||||
|
||||
def _make_interaction(is_admin: bool, bot: MagicMock) -> AsyncMock:
|
||||
"""
|
||||
Build a mock discord.Interaction.
|
||||
|
||||
The interaction's .client is set to *bot* so that MaintenanceAwareTree
|
||||
can read bot.maintenance_mode via interaction.client, mirroring how
|
||||
discord.py wires things at runtime.
|
||||
"""
|
||||
interaction = AsyncMock(spec=discord.Interaction)
|
||||
interaction.client = bot
|
||||
|
||||
# Mock the user as a guild Member so that guild_permissions is accessible.
|
||||
user = MagicMock(spec=discord.Member)
|
||||
user.guild_permissions = MagicMock()
|
||||
user.guild_permissions.administrator = is_admin
|
||||
interaction.user = user
|
||||
|
||||
# response.send_message must be awaitable.
|
||||
interaction.response = AsyncMock()
|
||||
interaction.response.send_message = AsyncMock()
|
||||
|
||||
return interaction
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Import the class under test after mocks are available.
|
||||
# We import here (not at module level) so that the conftest env-vars are set
|
||||
# before any discord_bot_v2 modules are touched.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _patch_discord_app_commands(monkeypatch):
|
||||
"""
|
||||
Prevent MaintenanceAwareTree.__init__ from calling discord internals that
|
||||
need a real event loop / Discord connection. We test only the logic of
|
||||
interaction_check, so we stub out the parent __init__.
|
||||
"""
|
||||
# Nothing extra to patch for the interaction_check itself; the parent
|
||||
# CommandTree.__init__ is only called when constructing SBABot, which we
|
||||
# don't do in these unit tests.
|
||||
yield
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Tests for MaintenanceAwareTree.interaction_check
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestMaintenanceAwareTree:
|
||||
"""Unit tests for MaintenanceAwareTree.interaction_check."""
|
||||
|
||||
@pytest.fixture
|
||||
def tree_cls(self):
|
||||
"""Import and return the MaintenanceAwareTree class."""
|
||||
from bot import MaintenanceAwareTree
|
||||
|
||||
return MaintenanceAwareTree
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# Maintenance OFF
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_maintenance_off_allows_non_admin(self, tree_cls):
|
||||
"""
|
||||
When maintenance_mode is False, non-admin users are always allowed.
|
||||
The check must return True without sending any message.
|
||||
"""
|
||||
bot = _make_bot(maintenance_mode=False)
|
||||
interaction = _make_interaction(is_admin=False, bot=bot)
|
||||
|
||||
# Instantiate tree without calling parent __init__ by testing the method
|
||||
# directly on an unbound basis.
|
||||
result = await tree_cls.interaction_check(
|
||||
MagicMock(), # placeholder 'self' for the tree instance
|
||||
interaction,
|
||||
)
|
||||
|
||||
assert result is True
|
||||
interaction.response.send_message.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_maintenance_off_allows_admin(self, tree_cls):
|
||||
"""
|
||||
When maintenance_mode is False, admin users are also always allowed.
|
||||
"""
|
||||
bot = _make_bot(maintenance_mode=False)
|
||||
interaction = _make_interaction(is_admin=True, bot=bot)
|
||||
|
||||
result = await tree_cls.interaction_check(MagicMock(), interaction)
|
||||
|
||||
assert result is True
|
||||
interaction.response.send_message.assert_not_called()
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# Maintenance ON — non-admin
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_maintenance_on_blocks_non_admin(self, tree_cls):
|
||||
"""
|
||||
When maintenance_mode is True, non-admin users must be blocked.
|
||||
The check must return False and send an ephemeral message.
|
||||
"""
|
||||
bot = _make_bot(maintenance_mode=True)
|
||||
interaction = _make_interaction(is_admin=False, bot=bot)
|
||||
|
||||
result = await tree_cls.interaction_check(MagicMock(), interaction)
|
||||
|
||||
assert result is False
|
||||
interaction.response.send_message.assert_called_once()
|
||||
|
||||
# Confirm the call used ephemeral=True
|
||||
_, kwargs = interaction.response.send_message.call_args
|
||||
assert kwargs.get("ephemeral") is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_maintenance_on_message_has_no_emoji(self, tree_cls):
|
||||
"""
|
||||
The maintenance block message must not contain emoji characters.
|
||||
The project style deliberately strips emoji from user-facing strings.
|
||||
"""
|
||||
import unicodedata
|
||||
|
||||
bot = _make_bot(maintenance_mode=True)
|
||||
interaction = _make_interaction(is_admin=False, bot=bot)
|
||||
|
||||
await tree_cls.interaction_check(MagicMock(), interaction)
|
||||
|
||||
args, _ = interaction.response.send_message.call_args
|
||||
message_text = args[0] if args else ""
|
||||
|
||||
for ch in message_text:
|
||||
category = unicodedata.category(ch)
|
||||
assert category != "So", (
|
||||
f"Unexpected emoji/symbol character {ch!r} (category {category!r}) "
|
||||
f"found in maintenance message: {message_text!r}"
|
||||
)
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# Maintenance ON — admin
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_maintenance_on_allows_admin(self, tree_cls):
|
||||
"""
|
||||
When maintenance_mode is True, administrator users must still be
|
||||
allowed through. Admins should never be locked out of bot commands.
|
||||
"""
|
||||
bot = _make_bot(maintenance_mode=True)
|
||||
interaction = _make_interaction(is_admin=True, bot=bot)
|
||||
|
||||
result = await tree_cls.interaction_check(MagicMock(), interaction)
|
||||
|
||||
assert result is True
|
||||
interaction.response.send_message.assert_not_called()
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# Edge case: non-Member user during maintenance
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_maintenance_on_blocks_non_member_user(self, tree_cls):
|
||||
"""
|
||||
When maintenance_mode is True and the user is not a guild Member
|
||||
(e.g. interaction from a DM context), the check must still block them
|
||||
because we cannot verify administrator status.
|
||||
"""
|
||||
bot = _make_bot(maintenance_mode=True)
|
||||
interaction = AsyncMock(spec=discord.Interaction)
|
||||
interaction.client = bot
|
||||
|
||||
# Simulate a non-Member user (e.g. discord.User from DM context)
|
||||
user = MagicMock(spec=discord.User)
|
||||
# discord.User has no guild_permissions attribute
|
||||
interaction.user = user
|
||||
interaction.response = AsyncMock()
|
||||
interaction.response.send_message = AsyncMock()
|
||||
|
||||
result = await tree_cls.interaction_check(MagicMock(), interaction)
|
||||
|
||||
assert result is False
|
||||
interaction.response.send_message.assert_called_once()
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# Missing attribute safety: bot without maintenance_mode attr
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_missing_maintenance_mode_attr_defaults_to_allowed(self, tree_cls):
|
||||
"""
|
||||
If the bot object does not have a maintenance_mode attribute (e.g.
|
||||
during testing or unusual startup), getattr fallback must treat it as
|
||||
False and allow the interaction.
|
||||
"""
|
||||
bot = MagicMock()
|
||||
# Deliberately do NOT set bot.maintenance_mode
|
||||
del bot.maintenance_mode
|
||||
|
||||
interaction = _make_interaction(is_admin=False, bot=bot)
|
||||
|
||||
result = await tree_cls.interaction_check(MagicMock(), interaction)
|
||||
|
||||
assert result is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Tests for SBABot.maintenance_mode attribute
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestSBABotMaintenanceModeAttribute:
|
||||
"""
|
||||
Confirms that SBABot.__init__ always sets maintenance_mode = False.
|
||||
|
||||
We avoid constructing a real SBABot (which requires a Discord event loop
|
||||
and valid token infrastructure) by patching the entire commands.Bot.__init__
|
||||
and then calling SBABot.__init__ directly on a bare instance so that only
|
||||
the SBABot-specific attribute assignments execute.
|
||||
"""
|
||||
|
||||
def test_maintenance_mode_default_is_false(self, monkeypatch):
|
||||
"""
|
||||
SBABot.__init__ must set self.maintenance_mode = False so that the
|
||||
MaintenanceAwareTree has the attribute available from the very first
|
||||
interaction, even before /admin-maintenance is ever called.
|
||||
|
||||
Strategy: patch commands.Bot.__init__ to be a no-op so super().__init__
|
||||
succeeds without a real Discord connection, then call SBABot.__init__
|
||||
and assert the attribute is present with the correct default value.
|
||||
"""
|
||||
from unittest.mock import patch
|
||||
from discord.ext import commands
|
||||
from bot import SBABot
|
||||
|
||||
with patch.object(commands.Bot, "__init__", return_value=None):
|
||||
bot = SBABot.__new__(SBABot)
|
||||
SBABot.__init__(bot)
|
||||
|
||||
assert hasattr(
|
||||
bot, "maintenance_mode"
|
||||
), "SBABot must define self.maintenance_mode in __init__"
|
||||
assert (
|
||||
bot.maintenance_mode is False
|
||||
), "SBABot.maintenance_mode must default to False"
|
||||
Loading…
Reference in New Issue
Block a user