feat: rewrite ctf events detection, notification and ctf channel creation#16
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request significantly refactors the CTF event tracking system, transitioning from JSON-based storage to a SQLite database with SQLAlchemy ORM, and introduces an interactive GUI for channel management through Discord's UI components.
Key Changes
- Replaces JSON file storage with SQLite database using SQLAlchemy and async support via aiosqlite
- Implements event change detection (create, update, delete) with automated notifications to both announcement and private channels
- Introduces interactive Discord UI with modals and buttons for creating/joining CTF channels
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 30 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Adds new dependencies: aiosqlite, pydantic-settings, sqlalchemy, greenlet, and supporting packages |
| pyproject.toml | Updates project dependencies to include sqlalchemy, aiosqlite, and pydantic-settings |
| src/config.py | Replaces environment variable loading with Pydantic settings management |
| src/database/model.py | Defines SQLAlchemy ORM models for Event and CustomChannel tables |
| src/database/database.py | Implements async database engine, session management, and initialization |
| src/crud/crud_event.py | Provides CRUD operations for Event model with async database access |
| src/crud/crud_custom_channel.py | Provides CRUD operations for CustomChannel model |
| src/crud/init.py | Exports CRUD functions for easy import |
| src/utils/join_channel.py | Implements channel joining logic for both CTFTime events and custom channels |
| src/utils/embed_creator.py | Updates embed creation to accept customizable title parameter |
| src/utils/ctf_api.py | Enhances API functions to support fetching individual events by ID |
| src/utils/country_flags.py | Adds country code to emoji mapping for event organizer display |
| src/cogs/ctf.py | Implements slash commands and UI views for CTF channel management |
| src/cogs/bgtask_interactions.py | Implements background task for event polling and interaction handling |
| src/data_manager.py | Removes obsolete JSON-based event storage |
| ctfeed.py | Refactors main entry point with cog loading and database initialization |
| setup_env.sh | Updates environment setup script for new configuration format |
| .env.example | Updates example environment file with new variable names |
Comments suppressed due to low confidence (3)
src/utils/ctf_api.py:11
- The function now accepts an optional
event_idparameter but lacks documentation explaining its purpose and behavior. Consider adding a docstring to document what happens whenevent_idis provided versus when it'sNone, and what the function returns in each case.
src/utils/embed_creator.py:11 - The function signature for
create_event_embedwas changed to require atitleparameter, but there's no documentation explaining what this parameter represents or what format it should be in. Consider adding a docstring to document the function parameters and their expected values.
src/utils/ctf_api.py:19 - The check
not(event_id is None)uses a non-Pythonic pattern. The more idiomatic way to check if a value is not None isevent_id is not None.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
| except Exception as e: | ||
| logger.error(f"Failed to create channel: {e}") | ||
| await interaction.followup.send(content=f"Failed to create channel: {e}", ephemeral=True) |
There was a problem hiding this comment.
The error message exposes the full exception details to the user. This could potentially leak sensitive information or implementation details. Consider providing a more generic error message to the user while logging the full exception details server-side.
| logger.error(f"Failed to join channel: {e}") | ||
| await interaction.followup.send(content=f"Failed to join channel: {e}", ephemeral=True) |
There was a problem hiding this comment.
The error message exposes the full exception details to the user. This could potentially leak sensitive information or implementation details. Consider providing a more generic error message to the user while logging the full exception details server-side.
| logger.error(f"Failed to join channel: {e}") | |
| await interaction.followup.send(content=f"Failed to join channel: {e}", ephemeral=True) | |
| logger.exception("Failed to join channel") | |
| await interaction.followup.send( | |
| content="Failed to join channel due to an internal error. Please try again later.", | |
| ephemeral=True, | |
| ) |
| async def get_announcement_channel(bot:commands.Bot) -> discord.TextChannel: | ||
| channel_name = settings.ANNOUNCEMENT_CHANNEL_NAME | ||
|
|
||
| channel = None | ||
| for guild in bot.guilds: | ||
| for text_channel in guild.text_channels: | ||
| if text_channel.name.lower() == channel_name.lower(): | ||
| channel = text_channel | ||
| break | ||
| if channel: | ||
| break | ||
|
|
||
| if not channel: | ||
| logger.error(f"Can't find channel named '{channel_name}'") | ||
| logger.error(f"Please check:") | ||
| logger.error(f"1. Channel name is correct: {channel_name}") | ||
| logger.error(f"2. Bot has permission to view the channel") | ||
| logger.error(f"3. The channel exists in the server where the Bot is located") | ||
| await bot.close() | ||
| return | ||
|
|
||
| return channel |
There was a problem hiding this comment.
The function iterates through all guilds and text channels to find a channel by name. This approach has O(n*m) complexity where n is the number of guilds and m is the average number of text channels per guild. If the bot is in multiple guilds with many channels, this could be a performance bottleneck. Consider caching the channel or storing its ID directly in the configuration.
| for event in known_events: | ||
| events_api = await fetch_ctf_events(event.event_id) | ||
| if len(events_api) != 1: | ||
| # event removed | ||
| logger.info(f"Detected: {event.title} (event_id={event.event_id}) was removed") | ||
|
|
||
| await crud.delete_event(session, event_id=[event.event_id]) | ||
|
|
||
| embed = discord.Embed( | ||
| color=discord.Color.red(), | ||
| title=f"{event.title} was removed", | ||
| footer=discord.EmbedFooter(text=f"Event ID: {event.event_id} | CTFtime.org") | ||
| ) | ||
| # send notification to announcement channel | ||
| await channel.send(embed=embed) | ||
| # send notification to private channel | ||
| if not(event.channel_id is None) and not(self.bot.get_channel(event.channel_id) is None): | ||
| await self.bot.get_channel(event.channel_id).send(embed=embed) | ||
| else: | ||
| # check update | ||
| event_api = events_api[0] | ||
| ntitle = event_api["title"] | ||
| nstart = datetime.fromisoformat(event_api["start"]).timestamp() | ||
| nfinish = datetime.fromisoformat(event_api["finish"]).timestamp() | ||
|
|
||
| if event.title != ntitle or \ | ||
| event.start != nstart or event.finish != nfinish: | ||
| # update detected | ||
| logger.info(f"Detected: {ntitle} (old: {event.title}) (event_id={event.event_id}) was updated") | ||
|
|
||
| await crud.update_event(session, event_id=event.event_id, | ||
| title=ntitle, | ||
| start=nstart, | ||
| finish=nfinish) | ||
|
|
||
| embed = await create_event_embed(event_api, title="Update detected") | ||
| # send notification to announcement channel | ||
| await channel.send(embed=embed) | ||
| # send notification to private channel | ||
| if not(event.channel_id is None) and not(self.bot.get_channel(event.channel_id) is None): | ||
| await self.bot.get_channel(event.channel_id).send(embed=embed) |
There was a problem hiding this comment.
The background task fetches individual event details for every known event on each iteration to check for updates. This results in N+1 API calls where N is the number of known events. This pattern can be inefficient and may hit rate limits if there are many events. Consider batching these requests or implementing a more efficient polling strategy.
| if not(title is None): | ||
| event.title = title | ||
|
|
||
| if not(start is None): | ||
| event.start = start | ||
|
|
||
| if not(finish is None): | ||
| event.finish = finish | ||
|
|
||
| if not(channel_id is None): | ||
| event.channel_id = channel_id |
There was a problem hiding this comment.
Multiple checks throughout the code use not(condition) or not(value is None) patterns which are non-Pythonic. For example, lines 76, 79, 85 use not(title is None), not(start is None), and not(channel_id is None). These should be written as title is not None, start is not None, and channel_id is not None respectively.
| @@ -0,0 +1,25 @@ | |||
| from sqlalchemy import Column, String, Integer, ForeignKey, CheckConstraint | |||
There was a problem hiding this comment.
Import of 'ForeignKey' is not used.
Import of 'CheckConstraint' is not used.
| from sqlalchemy import Column, String, Integer, ForeignKey, CheckConstraint | |
| from sqlalchemy import Column, String, Integer |
| try: | ||
| _ = custom_id.split(":") | ||
| event_id:int = int(_[2]) | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| try: | ||
| _ = custom_id.split(":") | ||
| channel_id:int = int(_[2]) | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| async def callback(self, interaction: discord.Interaction): | ||
| try: | ||
| event_id = int(self.children[0].value) | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| except: | |
| except (ValueError, TypeError): |
| async def callback(self, interaction:discord.Interaction): | ||
| try: | ||
| channel_id = int(self.children[0].value) | ||
| except: |
There was a problem hiding this comment.
Except block directly handles BaseException.
| except: | |
| except (ValueError, TypeError): |
This pull request rewrites CTF events detection, notification and CTF channel creation.
ctf_api.pyandembed_creator.py.