From 87458e4b8b3ab9c78eeb079e015290f4d0d6fb57 Mon Sep 17 00:00:00 2001 From: adrunkhuman <16039109+adrunkhuman@users.noreply.github.com> Date: Sat, 21 Mar 2026 23:59:21 +0100 Subject: [PATCH] refactor: extract AdminDMHandler base to eliminate _verify_admin duplication (#117) `FixtureCreationHandler` and `ResultsEntryHandler` had identical ~40-line `_verify_admin` implementations and repeated `has_session` boilerplate. Introduces `AdminDMHandler[S]` in `handlers/base_dm_handler.py` with: - abstract `get_session` / `clear_session` for subclasses to bind to the correct store methods - concrete `has_session` delegating to `get_session` - concrete `_verify_admin` using `self.clear_session` instead of direct store calls Both admin handlers inherit from the base; `DMPredictionHandler` is left standalone (no admin check, different session model). Co-Authored-By: Claude Sonnet 4.6 --- typer_bot/handlers/__init__.py | 3 +- typer_bot/handlers/base_dm_handler.py | 97 +++++++++++++++++++++++++++ typer_bot/handlers/fixture_handler.py | 80 +++------------------- typer_bot/handlers/results_handler.py | 78 +++------------------ 4 files changed, 120 insertions(+), 138 deletions(-) create mode 100644 typer_bot/handlers/base_dm_handler.py diff --git a/typer_bot/handlers/__init__.py b/typer_bot/handlers/__init__.py index 29acd8f..42e3639 100644 --- a/typer_bot/handlers/__init__.py +++ b/typer_bot/handlers/__init__.py @@ -1,7 +1,8 @@ """Discord workflow handlers.""" +from .base_dm_handler import AdminDMHandler from .dm_prediction_handler import DMPredictionHandler from .fixture_handler import FixtureCreationHandler from .results_handler import ResultsEntryHandler -__all__ = ["DMPredictionHandler", "FixtureCreationHandler", "ResultsEntryHandler"] +__all__ = ["AdminDMHandler", "DMPredictionHandler", "FixtureCreationHandler", "ResultsEntryHandler"] diff --git a/typer_bot/handlers/base_dm_handler.py b/typer_bot/handlers/base_dm_handler.py new file mode 100644 index 0000000..0088b81 --- /dev/null +++ b/typer_bot/handlers/base_dm_handler.py @@ -0,0 +1,97 @@ +"""Base class for admin DM workflow handlers.""" + +from __future__ import annotations + +import logging +from abc import ABC, abstractmethod +from collections.abc import Callable + +import discord + +from typer_bot.database import Database +from typer_bot.services.workflow_state import WorkflowStateStore + +logger = logging.getLogger(__name__) + + +class AdminDMHandler[S](ABC): + """Base for DM handlers that require admin verification before processing.""" + + def __init__( + self, bot: discord.Client, db: Database, workflow_state: WorkflowStateStore + ) -> None: + self.bot = bot + self.db = db + self.workflow_state = workflow_state + + @abstractmethod + def get_session(self, user_id: str) -> S | None: + """Return the active session for a user, or None. + + Implementations must evict expired sessions before returning so that + `has_session` (which delegates here) never returns True for stale entries. + """ + + def has_session(self, user_id: str) -> bool: + """Check if user has an active session.""" + return self.get_session(user_id) is not None + + @abstractmethod + def clear_session(self, user_id: str) -> None: + """Remove the active session for a user.""" + + async def _verify_admin( + self, + message: discord.Message, + user_id: str, + guild_id: int | None, + is_admin_fn: Callable[[discord.Member | None], bool], + ) -> bool: + """Verify the user still holds admin permissions. + + Clears the session and sends a denial message on any failure path. + """ + if not guild_id: + logger.warning(f"No guild_id in session for user {user_id}") + self.clear_session(user_id) + await message.author.send("Permission denied or session expired.") + return False + + guild = self.bot.get_guild(guild_id) + if not guild: + logger.warning(f"Guild not found for ID: {guild_id}") + self.clear_session(user_id) + await message.author.send("Permission denied or session expired.") + return False + + member = guild.get_member(int(user_id)) + if not member: + try: + member = await guild.fetch_member(int(user_id)) + except discord.NotFound: + logger.warning( + f"Member {user_id} not found in guild {guild_id} (cache miss + fetch miss)" + ) + self.clear_session(user_id) + await message.author.send("Permission denied or session expired.") + return False + except discord.Forbidden as e: + # Bot lacks GUILD_MEMBERS privileged intent or member is inaccessible + logger.error( + f"fetch_member forbidden for user {user_id} — check GUILD_MEMBERS intent: {e}" + ) + self.clear_session(user_id) + await message.author.send("Permission denied or session expired.") + return False + except discord.HTTPException as e: + logger.warning(f"fetch_member transient failure for user {user_id}: {e}") + await message.author.send("Could not verify permissions, please try again.") + return False + + if not is_admin_fn(member): + logger.warning(f"Permission denied for user {user_id}") + self.clear_session(user_id) + await message.author.send("Permission denied or session expired.") + return False + + return True diff --git a/typer_bot/handlers/fixture_handler.py b/typer_bot/handlers/fixture_handler.py index 19d8b7a..50d08a9 100644 --- a/typer_bot/handlers/fixture_handler.py +++ b/typer_bot/handlers/fixture_handler.py @@ -7,8 +7,8 @@ import discord from discord import ui -from typer_bot.database import Database -from typer_bot.services.workflow_state import FixtureSession, WorkflowStateStore +from typer_bot.handlers.base_dm_handler import AdminDMHandler +from typer_bot.services.workflow_state import FixtureSession from typer_bot.utils import APP_TZ, format_for_discord, now from typer_bot.utils.logger import log_event @@ -18,18 +18,17 @@ MAX_GAMES = 100 -class FixtureCreationHandler: +class FixtureCreationHandler(AdminDMHandler[FixtureSession]): """Handles the DM workflow for creating fixtures.""" - def __init__(self, bot: discord.Client, db: Database, workflow_state: WorkflowStateStore): - self.bot = bot - self.db = db - self.workflow_state = workflow_state - def get_session(self, user_id: str) -> FixtureSession | None: """Return the active fixture session for a user.""" return self.workflow_state.get_fixture_session(user_id) + def clear_session(self, user_id: str) -> None: + """Remove the active fixture creation session.""" + self.workflow_state.clear_fixture_session(user_id) + def start_session(self, user_id: str, channel_id: int, guild_id: int) -> None: """Initialize a new fixture creation session.""" self.workflow_state.start_fixture_session(user_id, channel_id, guild_id) @@ -43,10 +42,6 @@ def start_session(self, user_id: str, channel_id: int, guild_id: int) -> None: step="games", ) - def has_session(self, user_id: str) -> bool: - """Check if user has an active fixture creation session.""" - return self.workflow_state.has_fixture_session(user_id) - async def handle_dm( self, message: discord.Message, @@ -88,59 +83,6 @@ async def handle_dm( return True - async def _verify_admin( - self, - message: discord.Message, - user_id: str, - guild_id: int | None, - is_admin_fn: Callable[[discord.Member | None], bool], - ) -> bool: - """Verify user is still an admin.""" - if not guild_id: - logger.warning(f"No guild_id in fixture state for user {user_id}") - self.workflow_state.clear_fixture_session(user_id) - await message.author.send("Permission denied or session expired.") - return False - - guild = self.bot.get_guild(guild_id) - if not guild: - logger.warning(f"Guild not found for ID: {guild_id}") - self.workflow_state.clear_fixture_session(user_id) - await message.author.send("Permission denied or session expired.") - return False - - member = guild.get_member(int(user_id)) - if not member: - try: - member = await guild.fetch_member(int(user_id)) - except discord.NotFound: - logger.warning( - f"Member {user_id} not found in guild {guild_id} (cache miss + fetch miss)" - ) - self.workflow_state.clear_fixture_session(user_id) - await message.author.send("Permission denied or session expired.") - return False - except discord.Forbidden as e: - # Bot lacks GUILD_MEMBERS privileged intent or member is inaccessible - logger.error( - f"fetch_member forbidden for user {user_id} — check GUILD_MEMBERS intent: {e}" - ) - self.workflow_state.clear_fixture_session(user_id) - await message.author.send("Permission denied or session expired.") - return False - except discord.HTTPException as e: - logger.warning(f"fetch_member transient failure for user {user_id}: {e}") - await message.author.send("Could not verify permissions, please try again.") - return False - - if not is_admin_fn(member): - logger.warning(f"Permission denied for user {user_id}") - self.workflow_state.clear_fixture_session(user_id) - await message.author.send("Permission denied or session expired.") - return False - - return True - async def _handle_games_step(self, message: discord.Message, user_id: str) -> None: """Handle games list input.""" state = self.get_session(user_id) @@ -235,12 +177,12 @@ async def _show_preview(self, user: discord.User | discord.Member, user_id: str) if deadline is None: await user.send("Session expired. Please start over with `/admin fixture create`.") - self.workflow_state.clear_fixture_session(user_id) + self.clear_session(user_id) return if not channel or not isinstance(channel, discord.TextChannel): await user.send("Error: Could not find the original channel.") - self.workflow_state.clear_fixture_session(user_id) + self.clear_session(user_id) return max_week = await self.db.get_max_week_number() @@ -277,7 +219,7 @@ async def create_fixture( ) -> tuple[int, int]: """Create the fixture in the database and return ID + allocated week.""" fixture_id, allocated_week = await self.db.create_next_fixture(games, deadline) - self.workflow_state.clear_fixture_session(user_id) + self.clear_session(user_id) log_event( logger, event_type="fixture.created", @@ -291,7 +233,7 @@ async def create_fixture( def cancel_session(self, user_id: str, reason: str = "cancelled") -> None: """Cancel the fixture creation session.""" - self.workflow_state.clear_fixture_session(user_id) + self.clear_session(user_id) log_event( logger, event_type="session.fixture.completed", diff --git a/typer_bot/handlers/results_handler.py b/typer_bot/handlers/results_handler.py index b138cab..f3d482e 100644 --- a/typer_bot/handlers/results_handler.py +++ b/typer_bot/handlers/results_handler.py @@ -6,8 +6,8 @@ import discord from discord import ui -from typer_bot.database import Database -from typer_bot.services.workflow_state import ResultsSession, WorkflowStateStore +from typer_bot.handlers.base_dm_handler import AdminDMHandler +from typer_bot.services.workflow_state import ResultsSession from typer_bot.utils import parse_line_predictions from typer_bot.utils.logger import log_event @@ -16,18 +16,17 @@ MAX_MESSAGE_LENGTH = 5000 -class ResultsEntryHandler: +class ResultsEntryHandler(AdminDMHandler[ResultsSession]): """Handles the DM workflow for entering results.""" - def __init__(self, bot: discord.Client, db: Database, workflow_state: WorkflowStateStore): - self.bot = bot - self.db = db - self.workflow_state = workflow_state - def get_session(self, user_id: str) -> ResultsSession | None: """Return the active results session for a user.""" return self.workflow_state.get_results_session(user_id) + def clear_session(self, user_id: str) -> None: + """Remove the active results entry session.""" + self.workflow_state.clear_results_session(user_id) + def start_session(self, user_id: str, fixture_id: int, guild_id: int, week_number: int) -> None: """Initialize a new results entry session.""" self.workflow_state.start_results_session(user_id, fixture_id, guild_id) @@ -42,10 +41,6 @@ def start_session(self, user_id: str, fixture_id: int, guild_id: int, week_numbe guild_id=guild_id, ) - def has_session(self, user_id: str) -> bool: - """Check if user has an active results entry session.""" - return self.workflow_state.has_results_session(user_id) - async def handle_dm( self, message: discord.Message, @@ -82,7 +77,7 @@ async def handle_dm( if not fixture: await message.author.send("Error: Fixture no longer exists.") - self.workflow_state.clear_results_session(user_id) + self.clear_session(user_id) return True processing_msg = await message.author.send("Processing your results...") @@ -121,59 +116,6 @@ async def handle_dm( return True - async def _verify_admin( - self, - message: discord.Message, - user_id: str, - guild_id: int | None, - is_admin_fn: Callable[[discord.Member | None], bool], - ) -> bool: - """Verify user is still an admin.""" - if not guild_id: - logger.warning(f"No guild_id in result data for user {user_id}") - self.workflow_state.clear_results_session(user_id) - await message.author.send("Permission denied or session expired.") - return False - - guild = self.bot.get_guild(guild_id) - if not guild: - logger.warning(f"Guild not found for ID: {guild_id}") - self.workflow_state.clear_results_session(user_id) - await message.author.send("Permission denied or session expired.") - return False - - member = guild.get_member(int(user_id)) - if not member: - try: - member = await guild.fetch_member(int(user_id)) - except discord.NotFound: - logger.warning( - f"Member {user_id} not found in guild {guild_id} (cache miss + fetch miss)" - ) - self.workflow_state.clear_results_session(user_id) - await message.author.send("Permission denied or session expired.") - return False - except discord.Forbidden as e: - # Bot lacks GUILD_MEMBERS privileged intent or member is inaccessible - logger.error( - f"fetch_member forbidden for user {user_id} — check GUILD_MEMBERS intent: {e}" - ) - self.workflow_state.clear_results_session(user_id) - await message.author.send("Permission denied or session expired.") - return False - except discord.HTTPException as e: - logger.warning(f"fetch_member transient failure for user {user_id}: {e}") - await message.author.send("Could not verify permissions, please try again.") - return False - - if not is_admin_fn(member): - logger.warning(f"Permission denied for user {user_id}") - self.workflow_state.clear_results_session(user_id) - await message.author.send("Permission denied or session expired.") - return False - - return True - async def save_results( self, user_id: str, fixture_id: int, week_number: int, results: list[str] ) -> None: @@ -190,7 +132,7 @@ async def save_results( "This fixture has already been scored. Use `/admin panel` → correct results to make changes." ) await self.db.save_results(fixture_id, results) - self.workflow_state.clear_results_session(user_id) + self.clear_session(user_id) log_event( logger, event_type="results.entered", @@ -203,7 +145,7 @@ async def save_results( def cancel_session(self, user_id: str, reason: str = "cancelled") -> None: """Cancel the results entry session.""" - self.workflow_state.clear_results_session(user_id) + self.clear_session(user_id) logger.debug( f"Results session {reason}", extra={