Merge pull request 'fix: replace broken interaction_check decorator with MaintenanceAwareTree subclass' (#83) from fix/maintenance-mode-interaction-check into main

Reviewed-on: #83

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
 

commands/admin/management.py

@@ -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})"
         )

tests/test_bot_maintenance_tree.py

@@ -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"