-
-
Notifications
You must be signed in to change notification settings - Fork 41
Add snippet banning and unbanning #443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…re executing the command
Reviewer's Guide by SourceryThis pull request implements snippet banning and unbanning functionality for a Discord bot. It adds two new moderation commands, File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Atmois - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 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.
| ) | ||
|
|
||
|
|
||
| class SnippetBanFlags(commands.FlagConverter, delimiter=" ", prefix="-"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider creating a base class for flag converters to reduce duplication
The SnippetBanFlags, SnippetUnbanFlags, and WarnFlags classes share similar structure. Consider creating a base class to reduce code duplication and improve maintainability.
class BaseFlags(commands.FlagConverter, delimiter=" ", prefix="-"):
pass
class SnippetBanFlags(BaseFlags):
| await self.send_embed(ctx, embed, log_type="mod") | ||
| await ctx.send(embed=embed, delete_after=30, ephemeral=True) | ||
|
|
||
| async def is_snippetbanned(self, guild_id: int, user_id: int) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Move is_snippetbanned method to a common location to avoid duplication
This method is duplicated in both snippetban.py and snippetunban.py. Consider moving it to a shared utility module or base class to ensure consistency and reduce duplication.
from tux.utils.moderation import is_snippetbanned
class SnippetBan(commands.Cog):
# ... existing code ...
@commands.command()
async def snippetban(self, ctx, user: discord.Member):
if await is_snippetbanned(self.case_controller, ctx.guild.id, user.id):
# ... rest of the method
| await self.send_dm(ctx, flags.silent, target, flags.reason, "Snippet Banned") | ||
| await self.handle_case_response(ctx, case, "created", flags.reason, target) | ||
|
|
||
| async def handle_case_response( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Refactor shared functionality into a base class
The handle_case_response method and overall command structure are very similar in both snippetban.py and snippetunban.py. Consider creating a base class or utility functions to reduce code duplication and improve maintainability.
class SnippetModBase:
async def handle_case_response(
self,
ctx: commands.Context[commands.Bot],
case: Case,
action: str,
reason: str,
target: discord.Member
) -> None:
# Implementation here
| await ctx.send("This command cannot be used in direct messages.") | ||
| return | ||
|
|
||
| if await self.is_snippetbanned(ctx.guild.id, ctx.author.id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (performance): Consider optimizing the is_snippetbanned check
The current implementation of is_snippetbanned fetches all cases and then counts them. Consider optimizing this by using a more efficient database query to directly get the count or the latest status.
snippet_ban_status = await self.get_snippet_ban_status(ctx.guild.id, ctx.author.id)
if snippet_ban_status:
await ctx.send("You are banned from using snippets.")
return
| self.config = DatabaseController().guild_config | ||
| self.case_controller = CaseController() | ||
|
|
||
| async def is_snippetbanned(self, guild_id: int, user_id: int) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider separating the user ban check into a utility function or service.
The new code introduces additional complexity due to the added dependencies on CaseController and CaseType, as well as the new is_snippetbanned method. This increases coupling and makes the code harder to maintain. To improve, consider separating the concern of checking if a user is banned into a utility function or service, which would keep the Snippets class focused on its primary responsibility. Additionally, if possible, simplify the logic for determining if a user is banned, perhaps by leveraging a method in CaseController that directly checks this status. This refactoring would enhance readability and maintainability.
| unban_cases = await self.case_controller.get_all_cases_by_type(guild_id, CaseType.SNIPPETUNBAN) | ||
|
|
||
| ban_count = sum(1 for case in ban_cases if case.case_target_id == user_id) | ||
| unban_count = sum(1 for case in unban_cases if case.case_target_id == user_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Simplify constant sum() call (simplify-constant-sum)
| unban_count = sum(1 for case in unban_cases if case.case_target_id == user_id) | |
| unban_count = sum(bool(case.case_target_id == user_id) |
Explanation
Assum add the values it treats True as 1, and False as 0. We make useof this fact to simplify the generator expression inside the
sum call.
| ban_cases = await self.case_controller.get_all_cases_by_type(guild_id, CaseType.SNIPPETBAN) | ||
| unban_cases = await self.case_controller.get_all_cases_by_type(guild_id, CaseType.SNIPPETUNBAN) | ||
|
|
||
| ban_count = sum(1 for case in ban_cases if case.case_target_id == user_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Simplify constant sum() call (simplify-constant-sum)
| ban_count = sum(1 for case in ban_cases if case.case_target_id == user_id) | |
| ban_count = sum(bool(case.case_target_id == user_id) |
Explanation
Assum add the values it treats True as 1, and False as 0. We make useof this fact to simplify the generator expression inside the
sum call.
| unban_cases = await self.case_controller.get_all_cases_by_type(guild_id, CaseType.SNIPPETUNBAN) | ||
|
|
||
| ban_count = sum(1 for case in ban_cases if case.case_target_id == user_id) | ||
| unban_count = sum(1 for case in unban_cases if case.case_target_id == user_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Simplify constant sum() call (simplify-constant-sum)
| unban_count = sum(1 for case in unban_cases if case.case_target_id == user_id) | |
| unban_count = sum(bool(case.case_target_id == user_id) |
Explanation
Assum add the values it treats True as 1, and False as 0. We make useof this fact to simplify the generator expression inside the
sum call.
| ban_cases = await self.case_controller.get_all_cases_by_type(guild_id, CaseType.SNIPPETBAN) | ||
| unban_cases = await self.case_controller.get_all_cases_by_type(guild_id, CaseType.SNIPPETUNBAN) | ||
|
|
||
| ban_count = sum(1 for case in ban_cases if case.case_target_id == user_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Simplify constant sum() call (simplify-constant-sum)
| ban_count = sum(1 for case in ban_cases if case.case_target_id == user_id) | |
| ban_count = sum(bool(case.case_target_id == user_id) |
Explanation
Assum add the values it treats True as 1, and False as 0. We make useof this fact to simplify the generator expression inside the
sum call.
| unban_cases = await self.case_controller.get_all_cases_by_type(guild_id, CaseType.SNIPPETUNBAN) | ||
|
|
||
| ban_count = sum(1 for case in ban_cases if case.case_target_id == user_id) | ||
| unban_count = sum(1 for case in unban_cases if case.case_target_id == user_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Simplify constant sum() call (simplify-constant-sum)
| unban_count = sum(1 for case in unban_cases if case.case_target_id == user_id) | |
| unban_count = sum(bool(case.case_target_id == user_id) |
Explanation
Assum add the values it treats True as 1, and False as 0. We make useof this fact to simplify the generator expression inside the
sum call.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Added two hybrid moderation commands, snippet ban
{prefix}snippetbanand snippet unban{prefix}snippetunbanto prevent users from creating snippets. Both are registered as cases and have checks to ensure a snippet banned user isn't snippet banned again and vice versa. Requires Mod (Level 3) permissions to use.Closes #439