Temp Bans (not finished) #448
Conversation
Reviewer's Guide by SourceryThis pull request implements a temporary ban feature for a Discord bot. The changes include adding a new TempBan cog, modifying the TempBanFlags class, and updating the snippet editing functionality. The implementation focuses on creating a command to temporarily ban users with customizable duration and reason, along with proper logging and case management. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @GabberBalls - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Fix logic error in untempban method (link)
Overall Comments:
- The
untempbanmethod needs revision. It's not clear how it's triggered when a ban expires, and the unbanning condition seems incorrect. - Consider improving error handling in the
tempbancommand. Currently, it proceeds to create a case and schedule an unban even if the initial ban action fails. - The code could benefit from better organization. Consider using constants or enums for magic numbers and strings, and potentially refactoring the duration parsing logic for improved robustness.
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 1 other issue
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
tux/cogs/moderation/tempban.py
Outdated
| ) -> None: | ||
| if ctx.guild is None: | ||
| logger.warning("Temp ban command used outside of a guild context.") | ||
| return |
There was a problem hiding this comment.
issue (bug_risk): Improve error handling in tempban command to ensure consistency
If the ban fails, the function should return early to prevent creating a case or scheduling an unban for a user who wasn't actually banned.
tux/cogs/moderation/tempban.py
Outdated
| async def untempban(self, ctx: commands.Context[commands.Bot], expires_at, target: discord.Member) -> None: | ||
| if expires_at is None: | ||
| return | ||
| now = datetime.datetime.now(tz=datetime.UTC) | ||
|
|
||
| if now <= self.db.case.get_all_cases_by_type(discord.guild_id, case_type=CaseType.TEMPBAN): | ||
| unban.Unban.unban(self, target, "Temporary Ban Expired") | ||
| return |
There was a problem hiding this comment.
issue (bug_risk): Fix logic error in untempban method
The condition for unbanning is incorrect. It should compare the current time with expires_at, not with the result of get_all_cases_by_type. This needs to be fixed to ensure temporary bans are properly lifted.
| aliases=["r"], | ||
| default=MISSING, | ||
| ) | ||
| expires_at: int = commands.flag( |
There was a problem hiding this comment.
issue (complexity): Consider keeping expires_at as an int to simplify its usage.
The recent changes to the TempBanFlags class have increased complexity. Changing expires_at from int to str introduces additional parsing and validation needs, which can complicate the codebase. Consider keeping expires_at as an int to represent the duration in days, simplifying its usage. The addition of the silent flag is a useful feature, but remember to document it thoroughly to ensure clarity for future developers. Here's a suggestion to maintain simplicity:
class TempBanFlags(commands.FlagConverter, delimiter=" ", prefix="-"):
reason: str = commands.flag(
name="reason",
description="The reason for the member temp ban.",
aliases=["r"],
default=MISSING,
)
expires_at: int = commands.flag(
name="expires_at",
description="The time in days the ban will last for.",
aliases=["t", "d", "e"],
)
purge_days: int = commands.flag(
name="purge_days",
description="The number of days (< 7) to purge in messages.",
aliases=["p"],
default=0,
)
silent: bool = commands.flag(
name="silent",
description="Do not send a DM to the target.",
aliases=["s", "quiet"],
default=False,
)This approach keeps the complexity manageable and ensures consistency across the codebase.
2 participants
@kzndotsh requested i create a pull request so she can look at the code
Summary by Sourcery
Add a temporary ban feature to the moderation cog, allowing users to be banned for a specified duration with additional options. Improve snippet editing feedback by showing a content preview.
New Features:
tempban) that allows moderators to ban a user for a specified duration with options for silent bans and message purging.Enhancements: