diff --git a/app/app.py b/app/app.py index 709e48f..72085d1 100644 --- a/app/app.py +++ b/app/app.py @@ -147,6 +147,11 @@ def create_app(config=None): """ app = Flask(__name__) + # Cache-busting stamps for static files, filled lazily by the url_defaults + # hook below. Per application instance, so the test suite does not carry + # one app's mtimes into the next. + _static_stamps = {} + # --- defaults from the environment ------------------------------------ app.config['SECRET_KEY'] = os.getenv('SECRET_KEY') app.config['SQLALCHEMY_DATABASE_URI'] = os.getenv('DATABASE_URL') @@ -244,6 +249,35 @@ def create_app(config=None): # unconditionally so templates can carry nonce="" beforehand. g.csp_nonce = secrets.token_urlsafe(16) + @app.url_defaults + def version_static_urls(endpoint, values): + """Stamp every static URL with the file's modification time. + + Without this, nginx cannot be allowed to cache style.css and main.js: + their URLs never change, so a 30-day expiry means a 30-day-old stylesheet + with no way to invalidate it short of telling people to hard-refresh. + With it, a deployed file gets a new URL and the old entry simply stops + being asked for — which is what makes the `immutable` in nginx.conf + true rather than merely fast (PERF-006). + + The stamp is computed once per file per process. The process restarts + on deploy, which is exactly when a file can have changed. + """ + if endpoint != 'static' or 'filename' not in values: + return + filename = values['filename'] + stamp = _static_stamps.get(filename) + if stamp is None: + try: + stamp = str(int(os.stat(os.path.join(app.static_folder, filename)).st_mtime)) + except OSError: + # A missing file is the template's problem, not this hook's: + # let the URL build and let the 404 say so. + stamp = '' + _static_stamps[filename] = stamp + if stamp: + values['v'] = stamp + @app.context_processor def inject_csp_nonce(): return { diff --git a/app/discord_bot.py b/app/discord_bot.py index 12604a0..6c7a784 100644 --- a/app/discord_bot.py +++ b/app/discord_bot.py @@ -20,7 +20,7 @@ from zoneinfo import ZoneInfo from apscheduler.schedulers.asyncio import AsyncIOScheduler from apscheduler.triggers.cron import CronTrigger -from discord import Intents +from discord import Forbidden, HTTPException, Intents, NotFound from discord.ext import commands from dotenv import load_dotenv @@ -44,6 +44,23 @@ PENDING_MAX_AGE_DAYS = 30 #: restart or a slow start-up, short of the next day's occurrence. REMINDER_GRACE_SECONDS = 3600 +#: How many Discord user objects to keep resolved (PERF-005). +#: +#: A direct message costs two sequential API calls: resolve the snowflake to +#: a user, then send. The first is identical every time for the same person, +#: and the bot writes to the same few dozen people over and over — a season +#: of matches, then a reminder every evening at 18:00. +#: +#: What this does not fix: the first notification of a twenty-player match +#: still resolves twenty distinct users. The saving is across notifications, +#: not within one. Cutting the second call would need Discord's bulk DM +#: endpoints, which do not exist. +#: +#: Bounded because the process is long-lived. Eviction is oldest-first on the +#: insertion order of the dict, close enough to least-recently-used for a +#: roster that fits several times over. +USER_CACHE_MAX = 512 + CHECK_EMOJI = '✅' # Green checkmark CROSS_EMOJI = '❌' # Red X @@ -75,6 +92,7 @@ class TeamTryoutsBot(commands.Bot): self.message_queue = Queue() # Thread-safe queue for messages from Flask self.scheduler = AsyncIOScheduler() self.timezone = ZoneInfo('America/Toronto') # EDT timezone + self._user_cache = {} # Discord snowflake -> User, bounded (PERF-005) def _load_pending(self): """Load pending requests from the JSON file. @@ -217,6 +235,88 @@ class TeamTryoutsBot(commands.Bot): except Exception as e: logger.error(f'Error starting scheduler: {e}') + async def _resolve_user(self, discord_uid, *, context=''): + """Return the Discord user behind a snowflake, or None. + + Tries three sources in order of cost: the library's own cache, which + is free but mostly empty since the members intent was dropped + (OPS-014); ours, which survives across notifications; then the API. + + A failure here is logged once, saying which of the two reasons it + was — a snowflake nobody owns is a data problem to fix in the + account, an HTTP error is Discord being Discord (PERF-005). + """ + try: + uid = int(discord_uid) + except (TypeError, ValueError): + logger.warning('Invalid Discord user id %r%s', discord_uid, context) + return None + + user = self.get_user(uid) or self._user_cache.get(uid) + if user is not None: + return user + + try: + user = await self.fetch_user(uid) + except NotFound: + logger.warning( + 'Discord user %s does not exist%s. The id stored on the account is ' + 'wrong or the account was deleted; nothing will ever be delivered ' + 'to it.', + uid, + context, + ) + return None + except HTTPException as exc: + logger.warning('Could not resolve Discord user %s%s: %s', uid, context, exc) + return None + + if user is None: + return None + + self._user_cache[uid] = user + while len(self._user_cache) > USER_CACHE_MAX: + self._user_cache.pop(next(iter(self._user_cache))) + return user + + async def _send_dm(self, discord_uid, message, *, purpose, recipient=''): + """Send one direct message and record what became of it. + + Returns the sent Message, or None. Every path logs exactly once, with + the recipient and the purpose, so that nineteen deliveries out of + twenty read as nineteen successes and one named failure instead of + looking like twenty (PERF-005). + + The three failures are not the same problem and must not read alike: + `blocked` is permanent until the person reopens their DMs and no + retry will change it, `failed` is transient, `unreachable` means the + account itself could not be resolved. + """ + context = f' ({purpose}{", " + recipient if recipient else ""})' + user = await self._resolve_user(discord_uid, context=context) + if user is None: + logger.warning('Notification not delivered — %s: recipient unreachable', purpose) + return None + + who = recipient or getattr(user, 'name', str(discord_uid)) + try: + sent = await user.send(message) + except Forbidden: + logger.warning( + 'Notification not delivered — %s to %s: their direct messages are ' + 'closed to this bot. Retrying will not help; they have to allow ' + 'DMs from server members.', + purpose, + who, + ) + return None + except HTTPException as exc: + logger.error('Notification not delivered — %s to %s: %s', purpose, who, exc) + return None + + logger.info('Delivered %s to %s (message_id=%s)', purpose, who, sent.id) + return sent + async def process_queue(self): """Process messages from the queue (runs continuously).""" while True: @@ -266,11 +366,7 @@ class TeamTryoutsBot(commands.Bot): return # Fetch the user who reacted - try: - user = await self.fetch_user(payload.user_id) - except Exception: - return - + user = await self._resolve_user(payload.user_id, context=' (reaction handler)') if user is None: return @@ -336,47 +432,50 @@ class TeamTryoutsBot(commands.Bot): request_id: int, ) -> int: """Send a One on One request DM to a coach with reactions.""" - try: - user_id = int(coach_discord_id) - except (ValueError, TypeError): - logger.warning(f"Invalid coach_discord_id '{coach_discord_id}'") + message = ( + "📅 **One on One Request**\n\n" + f"**Player:** {player_name}\n" + f"**Team:** {team_name or 'Unknown Team'}\n" + f"**Date:** {date_str}\n" + f"**Time:** {start_time} - {end_time}\n" + f"**Discussion Points:** {points or 'No specific points provided'}\n\n" + "Please respond by clicking a reaction below:\n" + f"{CHECK_EMOJI} - Confirm the meeting\n" + f"{CROSS_EMOJI} - Decline (you can add a reason by replying before clicking)" + ) + + msg = await self._send_dm( + coach_discord_id, + message, + purpose='one-on-one request', + recipient=coach_name, + ) + if msg is None: return None try: - user = await self.fetch_user(user_id) - if not user: - return None - - message = ( - "📅 **One on One Request**\n\n" - f"**Player:** {player_name}\n" - f"**Team:** {team_name or 'Unknown Team'}\n" - f"**Date:** {date_str}\n" - f"**Time:** {start_time} - {end_time}\n" - f"**Discussion Points:** {points or 'No specific points provided'}\n\n" - "Please respond by clicking a reaction below:\n" - f"{CHECK_EMOJI} - Confirm the meeting\n" - f"{CROSS_EMOJI} - Decline (you can add a reason by replying before clicking)" - ) - - msg = await user.send(message) await msg.add_reaction(CHECK_EMOJI) await msg.add_reaction(CROSS_EMOJI) + except HTTPException as exc: + # The message is out; without reactions the coach cannot answer + # from Discord, but the web interface still works. + logger.error( + 'One on One request %s reached %s without its reactions (%s). It can ' + 'only be answered from the web interface.', + request_id, + coach_name, + exc, + ) + return None - # Track this pending request - self.pending_requests[msg.id] = { - 'type': 'one_on_one', - 'id': request_id, - 'created_at': time.time(), - } - self._save_pending() - - logger.info(f"Sent One on One DM with reactions, message_id={msg.id}") - return msg.id - - except Exception as e: - logger.error(f"Error sending One on One DM: {e}") - return None + # Track this pending request + self.pending_requests[msg.id] = { + 'type': 'one_on_one', + 'id': request_id, + 'created_at': time.time(), + } + self._save_pending() + return msg.id async def _send_schedule_notification( self, @@ -397,59 +496,63 @@ class TeamTryoutsBot(commands.Bot): event_time: Time string. reference_id: ID of the MatchParticipant or TryoutRegistration record. """ + # Look up the DB user to get their Discord user ID + from app.models import User as DBUser + + db_user = DBUser.query.get(user_id) + if not db_user: + logger.warning(f"DB user {user_id} not found for schedule notification") + return None + + if not db_user.discord_user_id: + logger.warning(f"User {db_user.username} has no Discord user ID, cannot send DM") + return None + + event_name = "Match" if event_type == 'match' else "Tryout" + + message = ( + f"📅 **{event_name} Scheduled**\n\n" + f"You have been added to the following {event_type}:\n" + f"**{event_title}**\n" + f"**Date:** {event_date}\n" + f"**Time:** {event_time}\n\n" + "Please confirm your attendance:\n" + f"{CHECK_EMOJI} - Confirm attendance\n" + f"{CROSS_EMOJI} - Decline" + ) + + msg = await self._send_dm( + db_user.discord_user_id, + message, + purpose=f'{event_type} schedule notification', + recipient=db_user.username, + ) + if msg is None: + return None + try: - # Look up the DB user to get their Discord user ID - from app.models import User as DBUser - - db_user = DBUser.query.get(user_id) - if not db_user: - logger.warning(f"DB user {user_id} not found for schedule notification") - return None - - if not db_user.discord_user_id: - logger.warning(f"User {db_user.username} has no Discord user ID, cannot send DM") - return None - - discord_uid = int(db_user.discord_user_id) - user = await self.fetch_user(discord_uid) - if not user: - logger.warning(f"Could not fetch Discord user {discord_uid}") - return None - - event_name = "Match" if event_type == 'match' else "Tryout" - - message = ( - f"📅 **{event_name} Scheduled**\n\n" - f"You have been added to the following {event_type}:\n" - f"**{event_title}**\n" - f"**Date:** {event_date}\n" - f"**Time:** {event_time}\n\n" - "Please confirm your attendance:\n" - f"{CHECK_EMOJI} - Confirm attendance\n" - f"{CROSS_EMOJI} - Decline" - ) - - msg = await user.send(message) await msg.add_reaction(CHECK_EMOJI) await msg.add_reaction(CROSS_EMOJI) - - # Track this pending request - self.pending_requests[msg.id] = { - 'type': 'schedule_addition', - 'id': reference_id, - 'event_type': event_type, - 'created_at': time.time(), - } - self._save_pending() - - logger.info( - f"Sent {event_type} schedule notification to {db_user.username}, message_id={msg.id}" + except HTTPException as exc: + # Without the reactions the player has no way to answer: the + # message asks them to click something that is not there. + logger.error( + 'Schedule notification reached %s without its reactions (%s). They ' + 'cannot confirm attendance from Discord.', + db_user.username, + exc, ) - return msg.id + return None - except Exception as e: - logger.error(f"Error sending schedule notification: {e}") - return None + # Track this pending request + self.pending_requests[msg.id] = { + 'type': 'schedule_addition', + 'id': reference_id, + 'event_type': event_type, + 'created_at': time.time(), + } + self._save_pending() + return msg.id async def handle_one_on_one_approve(self, coach, message_id, request_id, channel): """Handle coach approving a One on One request.""" @@ -693,11 +796,6 @@ class TeamTryoutsBot(commands.Bot): logger.warning(f"Player has no Discord user ID for request {request.id}") return - player_user = await self.fetch_user(int(player_discord_id)) - if not player_user: - logger.warning(f"Could not fetch Discord user {player_discord_id}") - return - if approved: message = ( "🎉 **One on One Session Confirmed!**\n\n" @@ -722,9 +820,11 @@ class TeamTryoutsBot(commands.Bot): "Please try selecting a different time slot." ) - await player_user.send(message) - logger.info( - f"Sent One on One notification to player {player_full_name} (request {request.id})" + await self._send_dm( + player_discord_id, + message, + purpose=f'one-on-one response (request {request.id})', + recipient=player_full_name, ) except Exception as e: @@ -757,13 +857,20 @@ class TeamTryoutsBot(commands.Bot): now = datetime.now(self.timezone) tomorrow = now.date() + timedelta(days=1) + # Counted so that a partial run is visible as such. Per-recipient + # failures are logged by _send_dm; without this total, a batch in + # which half the reminders bounced looked exactly like one where + # they all went out (PERF-005). + attempted = delivered = 0 + # Find matches for tomorrow matches = Match.query.filter(Match.date == tomorrow).all() for match in matches: participants = MatchParticipant.query.filter_by(match_id=match.id).all() for participant in participants: if participant.player.discord_user_id: - await self.send_match_reminder(participant.player, match) + attempted += 1 + delivered += await self.send_match_reminder(participant.player, match) # Find tryouts for tomorrow tryouts = Tryout.query.filter(Tryout.date == tomorrow).all() @@ -771,7 +878,8 @@ class TeamTryoutsBot(commands.Bot): registrations = TryoutRegistration.query.filter_by(tryout_id=tryout.id).all() for reg in registrations: if reg.player.discord_user_id: - await self.send_tryout_reminder(reg.player, tryout) + attempted += 1 + delivered += await self.send_tryout_reminder(reg.player, tryout) # Find One on One sessions for tomorrow (only approved ones) one_on_ones = ( @@ -783,42 +891,59 @@ class TeamTryoutsBot(commands.Bot): ) for session in one_on_ones: if session.player and session.player.discord_user_id: - await self.send_one_on_one_reminder(session.player, session) + attempted += 1 + delivered += await self.send_one_on_one_reminder(session.player, session) + + if attempted and delivered < attempted: + logger.warning( + 'Daily reminders for %s: %d of %d delivered, %d failed. See the ' + 'lines above for who and why.', + tomorrow, + delivered, + attempted, + attempted - delivered, + ) + else: + logger.info('Daily reminders for %s: %d delivered', tomorrow, delivered) except Exception as e: logger.error(f"Error sending daily reminders: {e}") async def send_match_reminder(self, player, match): - """Send match reminder to player.""" - try: - player_user = await self.fetch_user(int(player.discord_user_id)) - message = ( - "🔔 **Match Reminder**\n\n" - f"Your match **{match.title}** is scheduled for tomorrow:\n" - f"**Date:** {match.date.strftime('%A, %B %d, %Y')}\n" - f"**Time:** {match.start_time.strftime('%I:%M %p') if match.start_time else 'TBD'} - " - f"{match.end_time.strftime('%I:%M %p') if match.end_time else 'TBD'}\n" - f"**Location:** {match.location or 'TBD'}\n\n" - "Please confirm your attendance in the app." - ) - await player_user.send(message) - except Exception as e: - logger.error(f"Error sending match reminder: {e}") + """Send match reminder to player. True if it was delivered.""" + message = ( + "🔔 **Match Reminder**\n\n" + f"Your match **{match.title}** is scheduled for tomorrow:\n" + f"**Date:** {match.date.strftime('%A, %B %d, %Y')}\n" + f"**Time:** {match.start_time.strftime('%I:%M %p') if match.start_time else 'TBD'} - " + f"{match.end_time.strftime('%I:%M %p') if match.end_time else 'TBD'}\n" + f"**Location:** {match.location or 'TBD'}\n\n" + "Please confirm your attendance in the app." + ) + sent = await self._send_dm( + player.discord_user_id, + message, + purpose='match reminder', + recipient=player.username, + ) + return sent is not None async def send_tryout_reminder(self, player, tryout): - """Send tryout reminder to player.""" - try: - player_user = await self.fetch_user(int(player.discord_user_id)) - message = ( - "🔔 **Tryout Reminder**\n\n" - f"Your tryout **{tryout.title}** is scheduled for tomorrow:\n" - f"**Date:** {tryout.date.strftime('%A, %B %d, %Y')}\n" - f"**Location:** {tryout.location or 'TBD'}\n\n" - "Please confirm your attendance in the app." - ) - await player_user.send(message) - except Exception as e: - logger.error(f"Error sending tryout reminder: {e}") + """Send tryout reminder to player. True if it was delivered.""" + message = ( + "🔔 **Tryout Reminder**\n\n" + f"Your tryout **{tryout.title}** is scheduled for tomorrow:\n" + f"**Date:** {tryout.date.strftime('%A, %B %d, %Y')}\n" + f"**Location:** {tryout.location or 'TBD'}\n\n" + "Please confirm your attendance in the app." + ) + sent = await self._send_dm( + player.discord_user_id, + message, + purpose='tryout reminder', + recipient=player.username, + ) + return sent is not None async def _send_one_on_one_response_dm( self, @@ -841,11 +966,6 @@ class TeamTryoutsBot(commands.Bot): logger.warning("Cannot send response DM: no player_discord_id") return False - player_user = await self.fetch_user(int(player_discord_id)) - if not player_user: - logger.warning(f"Could not fetch Discord user {player_discord_id}") - return False - if approved: message = ( "🎉 **One on One Session Confirmed!**\n\n" @@ -870,31 +990,35 @@ class TeamTryoutsBot(commands.Bot): "Please try selecting a different time slot." ) - await player_user.send(message) - logger.info( - f"Sent One on One response DM to player {player_full_name} (approved={approved})" + sent = await self._send_dm( + player_discord_id, + message, + purpose=f'one-on-one response ({"approved" if approved else "declined"})', + recipient=player_full_name, ) - return True + return sent is not None except Exception as e: logger.error(f"Error sending One on One response DM: {e}") return False async def send_one_on_one_reminder(self, player, session): - """Send One on One reminder to player.""" - try: - player_user = await self.fetch_user(int(player.discord_user_id)) - message = ( - "🔔 **One on One Reminder**\n\n" - f"Your One on One session with **{session.coach.full_name}** is scheduled for tomorrow:\n" - f"**Date:** {session.date.strftime('%A, %B %d, %Y')}\n" - f"**Time:** {session.start_time.strftime('%I:%M %p')} - {session.end_time.strftime('%I:%M %p')}\n" - f"**Discussion Points:** {session.points or 'No specific points provided'}\n\n" - "Please prepare for your session!" - ) - await player_user.send(message) - except Exception as e: - logger.error(f"Error sending One on One reminder: {e}") + """Send One on One reminder to player. True if it was delivered.""" + message = ( + "🔔 **One on One Reminder**\n\n" + f"Your One on One session with **{session.coach.full_name}** is scheduled for tomorrow:\n" + f"**Date:** {session.date.strftime('%A, %B %d, %Y')}\n" + f"**Time:** {session.start_time.strftime('%I:%M %p')} - {session.end_time.strftime('%I:%M %p')}\n" + f"**Discussion Points:** {session.points or 'No specific points provided'}\n\n" + "Please prepare for your session!" + ) + sent = await self._send_dm( + player.discord_user_id, + message, + purpose='one-on-one reminder', + recipient=player.username, + ) + return sent is not None # Global bot instance diff --git a/app/nginx.conf b/app/nginx.conf index 71c10ff..bae6fa1 100644 --- a/app/nginx.conf +++ b/app/nginx.conf @@ -143,15 +143,41 @@ http { } # --------------------------------------------------------------------- - # Static Files (served directly by Nginx for performance) - # Uncomment and adjust path if you want Nginx to serve static files + # Static Files (PERF-006) + # + # 59 KB of CSS and JS on every page load, previously proxied through + # Waitress. Nginx serves them from disk instead. + # + # ADJUST THIS ONE PATH to the deployment's checkout, absolute, forward + # slashes even on Windows. Nginx resolves a relative path against its + # own install prefix, not against this file. The trailing slash on both + # the location and the alias is required: without it /static/css/x.css + # resolves one directory too high. + # + # `immutable` is safe here and only here: url_for('static', …) appends + # ?v= (see version_static_urls in app/app.py), so a deployed file + # is requested under a new URL and the cached copy of the old one is + # never asked for again. Removing that stamp and leaving this block + # gives every visitor a month-old stylesheet. # --------------------------------------------------------------------- - # location /static/ { - # alias C:/path/to/team-tryouts/static/; - # expires 30d; - # add_header Cache-Control "public, immutable"; - # access_log off; - # } + location /static/ { + alias C:/team-tryouts/app/static/; + expires 30d; + access_log off; + + # These three are repeated on purpose. In nginx, add_header is + # inherited from the enclosing block ONLY when the current block + # declares none of its own — one add_header here silently drops + # every security header set at server level. Dropping nosniff on + # the JavaScript is the one that matters. + add_header Cache-Control "public, immutable"; + add_header X-Content-Type-Options "nosniff" always; + add_header Strict-Transport-Security "max-age=31536000; includeSubDomains; preload" always; + + # A missing static file must 404, not fall through to Flask: the + # fallthrough would hide a broken deploy behind a working page. + try_files $uri =404; + } # --------------------------------------------------------------------- # Rate Limiting diff --git a/docs/deployment.md b/docs/deployment.md index baafc41..a1e6f6e 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -64,11 +64,30 @@ BACKUP_RETENTION_DAYS=30 ## Step 3: Configure Nginx -1. Copy `nginx.conf` to your Nginx installation directory (e.g., `C:\nginx\conf\`) -2. Place SSL certificate files: +1. Copy `app/nginx.conf` to your Nginx installation directory (e.g., `C:\nginx\conf\`) +2. **Edit the `alias` in the `location /static/` block** to point at this + checkout's `app/static/` directory — absolute path, forward slashes, keep + the trailing slash. It ships as `C:/team-tryouts/app/static/`, which is a + guess about your machine. Nginx resolves a relative path against its own + install prefix, not against `nginx.conf`. +3. Place SSL certificate files: - `C:\nginx\certs\fullchain.pem` - `C:\nginx\certs\privkey.pem` -3. Start Nginx: `C:\nginx\nginx.exe` +4. Check the configuration parses before restarting: `C:\nginx\nginx.exe -t` +5. Start Nginx: `C:\nginx\nginx.exe` + +Nginx serves `/static/` from disk with a 30-day `immutable` cache. That is +only safe because `url_for('static', …)` appends `?v=` to every static +URL (`version_static_urls` in `app/app.py`), so a redeployed file is requested +under a new URL. If that stamp is ever removed, remove the cache headers with +it or visitors keep a month-old stylesheet. + +After a deploy, confirm the stamp changed rather than trusting it: + +```powershell +# The v= value must differ from the one served before the deploy. +(Invoke-WebRequest https://your-domain/auth/login).Content -match 'style\.css\?v=(\d+)' +``` ### Obtaining SSL Certificates diff --git a/tests/test_discord_delivery.py b/tests/test_discord_delivery.py new file mode 100644 index 0000000..f0fea05 --- /dev/null +++ b/tests/test_discord_delivery.py @@ -0,0 +1,272 @@ +"""Resolving Discord users, and saying what became of each message. + +PERF-005. A direct message costs two sequential API calls — resolve the +snowflake, then send — and the first one is identical every time for the +same person. The bot writes to the same roster over and over, so resolution +is cached across notifications. + +The other half of the constat was visibility: a batch of twenty reminders in +which three bounced produced no line saying three had bounced. Failures are +now named, per recipient, and the daily batch reports its own shortfall. + +Nothing here talks to Discord. The coroutines are driven with asyncio.run +rather than pytest-asyncio, which the project does not depend on. +""" + +import asyncio +import logging + +import pytest +from discord import Forbidden, HTTPException, NotFound + +from app.discord_bot import USER_CACHE_MAX, TeamTryoutsBot + + +class FakeUser: + """Stands in for a discord.User. Records what it was asked to send.""" + + def __init__(self, uid, name=None, raises=None): + self.id = uid + self.name = name or f'user{uid}' + self.raises = raises + self.sent = [] + + async def send(self, message): + if self.raises is not None: + raise self.raises + self.sent.append(message) + return FakeMessage(1000 + len(self.sent)) + + +class FakeMessage: + def __init__(self, mid): + self.id = mid + self.reactions = [] + + async def add_reaction(self, emoji): + self.reactions.append(emoji) + + +def _response(status): + """A minimal object with the attributes discord's exceptions read.""" + + class _R: + def __init__(self): + self.status = status + self.reason = 'test' + + return _R() + + +def forbidden(): + return Forbidden(_response(403), 'Cannot send messages to this user') + + +def http_error(): + return HTTPException(_response(500), 'Internal Server Error') + + +def not_found(): + return NotFound(_response(404), 'Unknown User') + + +@pytest.fixture +def logs(caplog): + """Let the bot's records reach caplog. + + configure_logging sets propagate = False on the 'app' logger so records + are not written twice, and caplog's handler sits on the root logger. So + these tests passed on their own and failed in the full suite, where some + earlier test had already built an application — the assertions were + reading an empty log, not a silent bot. + + Restored afterwards. The production setting is right; it is only in the + way here. + """ + package_logger = logging.getLogger('app') + previous = package_logger.propagate + package_logger.propagate = True + caplog.set_level(logging.INFO, logger='app.discord_bot') + yield caplog + package_logger.propagate = previous + + +@pytest.fixture +def bot(): + """A bot object with nothing but the state the delivery path needs. + + __new__ rather than the constructor: TeamTryoutsBot.__init__ builds a + real discord.py client, which wants an event loop and a token. + """ + instance = TeamTryoutsBot.__new__(TeamTryoutsBot) + instance._user_cache = {} + instance.pending_requests = {} + instance.fetch_calls = [] + + # The library's own cache. Empty by default: the members intent was + # dropped in OPS-014, so in production it almost always misses. + instance.library_cache = {} + instance.get_user = instance.library_cache.get + + async def fetch_user(uid): + instance.fetch_calls.append(uid) + if uid in instance.fetch_failures: + raise instance.fetch_failures[uid] + return FakeUser(uid) + + instance.fetch_failures = {} + instance.fetch_user = fetch_user + return instance + + +class TestResolution: + def test_the_same_recipient_is_fetched_once(self, bot): + first = asyncio.run(bot._resolve_user(42)) + second = asyncio.run(bot._resolve_user(42)) + + assert first is second + assert bot.fetch_calls == [42], 'the second lookup should have come from the cache' + + def test_distinct_recipients_are_each_fetched(self, bot): + """The cache pays off across notifications, not within one. + + Twenty players in one match are still twenty lookups. Claiming + otherwise in the commit message would have been the easy lie. + """ + for uid in range(10, 20): + asyncio.run(bot._resolve_user(uid)) + + assert bot.fetch_calls == list(range(10, 20)) + + def test_the_library_cache_is_consulted_before_the_api(self, bot): + known = FakeUser(7) + bot.library_cache[7] = known + + assert asyncio.run(bot._resolve_user(7)) is known + assert bot.fetch_calls == [] + + def test_a_string_snowflake_resolves(self, bot): + """Every caller reads discord_user_id off a model, where it is text.""" + asyncio.run(bot._resolve_user('42')) + asyncio.run(bot._resolve_user(42)) + + assert bot.fetch_calls == [42], 'the string and the int must be the same cache entry' + + def test_a_malformed_id_is_rejected_without_a_call(self, bot): + assert asyncio.run(bot._resolve_user('not-a-snowflake')) is None + assert asyncio.run(bot._resolve_user(None)) is None + assert bot.fetch_calls == [] + + def test_the_cache_is_bounded(self, bot): + for uid in range(USER_CACHE_MAX + 25): + asyncio.run(bot._resolve_user(uid)) + + assert len(bot._user_cache) <= USER_CACHE_MAX + + def test_an_unknown_account_is_not_cached(self, bot): + """A snowflake nobody owns must stay retryable. + + Caching the miss would mean that fixing the id on the account has no + effect until the process restarts. + """ + bot.fetch_failures[99] = not_found() + + assert asyncio.run(bot._resolve_user(99)) is None + assert asyncio.run(bot._resolve_user(99)) is None + assert bot.fetch_calls == [99, 99] + + def test_a_transport_failure_says_which_kind_it_was(self, bot, logs): + bot.fetch_failures[99] = not_found() + bot.fetch_failures[98] = http_error() + + with logs.at_level(logging.WARNING, logger='app.discord_bot'): + asyncio.run(bot._resolve_user(99)) + asyncio.run(bot._resolve_user(98)) + + text = logs.text + assert 'does not exist' in text, 'a bad id is an account to fix, and must read that way' + assert 'Could not resolve' in text + + +class TestDelivery: + def test_a_delivered_message_is_logged_with_its_recipient(self, bot, logs): + with logs.at_level(logging.INFO, logger='app.discord_bot'): + sent = asyncio.run(bot._send_dm(5, 'hello', purpose='match reminder', recipient='ana')) + + assert sent is not None + assert 'match reminder' in logs.text + assert 'ana' in logs.text + + def test_closed_dms_are_named_and_not_confused_with_an_outage(self, bot, logs): + bot.library_cache[5] = FakeUser(5, raises=forbidden()) + + with logs.at_level(logging.WARNING, logger='app.discord_bot'): + sent = asyncio.run(bot._send_dm(5, 'hello', purpose='match reminder', recipient='ana')) + + assert sent is None + assert 'ana' in logs.text + assert 'Retrying will not help' in logs.text, ( + 'a closed inbox is permanent; reporting it like a transient error ' + 'sends someone chasing an outage that is not there' + ) + + def test_a_transient_failure_is_an_error_not_a_warning(self, bot, logs): + bot.library_cache[5] = FakeUser(5, raises=http_error()) + + with logs.at_level(logging.WARNING, logger='app.discord_bot'): + sent = asyncio.run(bot._send_dm(5, 'x', purpose='match reminder', recipient='ana')) + + assert sent is None + levels = {record.levelno for record in logs.records} + assert logging.ERROR in levels + + def test_an_unreachable_recipient_still_produces_a_line(self, bot, logs): + bot.fetch_failures[5] = not_found() + + with logs.at_level(logging.WARNING, logger='app.discord_bot'): + sent = asyncio.run(bot._send_dm(5, 'x', purpose='tryout reminder', recipient='ana')) + + assert sent is None + assert 'not delivered' in logs.text + assert 'tryout reminder' in logs.text + + +class TestReminderOutcomes: + """The daily batch counts what it delivered, so it needs a real answer. + + These three return values feed `delivered += await …` in + _send_daily_reminders_impl. A reminder that returned None on both paths + would make a wholly failed batch report as a wholly successful one. + """ + + class FakePlayer: + username = 'ana' + discord_user_id = '5' + + class FakeMatch: + title = 'Finals' + location = 'Arena' + + from datetime import date, time + + date = date(2026, 8, 12) + start_time = time(18, 0) + end_time = time(20, 0) + + class FakeTryout: + title = 'Open tryout' + location = 'Arena' + + from datetime import date + + date = date(2026, 8, 12) + + def test_a_delivered_reminder_reports_true(self, bot): + assert asyncio.run(bot.send_match_reminder(self.FakePlayer(), self.FakeMatch())) is True + assert asyncio.run(bot.send_tryout_reminder(self.FakePlayer(), self.FakeTryout())) is True + + def test_a_bounced_reminder_reports_false(self, bot): + bot.library_cache[5] = FakeUser(5, raises=forbidden()) + + assert asyncio.run(bot.send_match_reminder(self.FakePlayer(), self.FakeMatch())) is False + assert asyncio.run(bot.send_tryout_reminder(self.FakePlayer(), self.FakeTryout())) is False diff --git a/tests/test_static_caching.py b/tests/test_static_caching.py new file mode 100644 index 0000000..67854a4 --- /dev/null +++ b/tests/test_static_caching.py @@ -0,0 +1,104 @@ +"""Static URLs carry a version stamp, so nginx may cache them (PERF-006). + +The audit asked for three lines of nginx: serve /static/ from disk with a +30-day expiry. Enabling that alone would have been a regression. The CSS and +the JS are referenced by a fixed URL, so a month-long cache means a +month-old stylesheet after every deploy, with no way to invalidate it short +of asking people to hard-refresh. + +The stamp is what makes the caching safe: a changed file gets a new URL, and +the cached copy of the old one is simply never requested again. Delete the +stamp and the nginx block becomes a bug — which is the only reason these +tests exist. +""" + +import os +import re + +import pytest +from flask import url_for + + +@pytest.fixture +def nginx_conf(): + root = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + with open(os.path.join(root, 'app', 'nginx.conf'), encoding='utf-8') as handle: + return handle.read() + + +class TestVersionStamp: + def test_static_urls_carry_a_stamp(self, app): + with app.test_request_context(): + url = url_for('static', filename='css/style.css') + + assert re.search(r'\?v=\d+$', url), f'no cache-busting stamp in {url}' + + def test_a_touched_file_gets_a_new_url(self, app): + """The whole point: redeploying a file must change its URL. + + The stamp is memoised per process — the process restarts on deploy, + which is exactly when a file can have changed — so this drives a + fresh application rather than touching the file under a live one. + """ + from app.app import create_app + + path = os.path.join(app.static_folder, 'css/style.css') + original = os.stat(path) + + with app.test_request_context(): + before = url_for('static', filename='css/style.css') + + os.utime(path, (original.st_atime, original.st_mtime + 60)) + try: + second = create_app(dict(app.config)) + with second.test_request_context(): + after = url_for('static', filename='css/style.css') + finally: + os.utime(path, (original.st_atime, original.st_mtime)) + + assert before != after + + def test_a_missing_file_still_builds_a_url(self, app): + """A template naming a file that is not there must 404, not 500.""" + with app.test_request_context(): + url = url_for('static', filename='css/does-not-exist.css') + + assert url.endswith('does-not-exist.css'), 'no stamp, and no exception either' + + def test_other_endpoints_are_untouched(self, app): + with app.test_request_context(): + assert '?v=' not in url_for('main.index') + + def test_the_stylesheet_and_the_script_are_versioned_in_the_page(self, client, app): + """The stamp is worthless if the layout bypasses url_for.""" + page = client.get('/auth/login').get_data(as_text=True) + + assert re.search(r'style\.css\?v=\d+', page) + assert re.search(r'main\.js\?v=\d+', page) + + +class TestNginx: + def test_the_static_block_is_live(self, nginx_conf): + block = re.search(r'^\s*location /static/ \{', nginx_conf, re.MULTILINE) + assert block, 'the /static/ block is commented out again — 59 KB per page load' + + def test_caching_headers_are_present(self, nginx_conf): + static_block = nginx_conf.split('location /static/')[1].split('\n }')[0] + + assert 'expires 30d' in static_block + assert 'immutable' in static_block + + def test_security_headers_survive_the_block(self, nginx_conf): + """One add_header in a location drops every inherited one. + + nginx only inherits add_header from the enclosing block when the + current block declares none of its own. Setting Cache-Control here + therefore removes nosniff from the JavaScript unless it is repeated. + """ + static_block = nginx_conf.split('location /static/')[1].split('\n }')[0] + + assert 'X-Content-Type-Options' in static_block, ( + 'add_header here cancels the inherited security headers; nosniff ' + 'has to be repeated inside the block' + ) + assert 'Strict-Transport-Security' in static_block