-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add flagging functionality for moderation #556
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
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 - here's some feedback:
Overall Comments:
- Consider refactoring the common logic in flag.py and unflag.py to reduce code duplication.
- The is_flagged method might have performance issues for large guilds. Consider optimizing this or using a different approach to track flag status.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -309,3 +309,21 @@ class SnippetUnbanFlags(commands.FlagConverter, case_insensitive=True, delimiter | |||
aliases=["s", "quiet"], | |||
default=False, | |||
) | |||
|
|||
|
|||
class FlagFlags(commands.FlagConverter, case_insensitive=True, 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 combining FlagFlags and UnFlagFlags classes
The FlagFlags and UnFlagFlags classes are very similar. Consider if they can be combined into a single class to reduce code duplication, or if there's a compelling reason to keep them separate.
class SharedFlags(commands.FlagConverter, case_insensitive=True, delimiter=" "):
reason: str = commands.flag(name="reason", default="")
duration: str = commands.flag(name="duration", default="")
class FlagFlags(SharedFlags, prefix="-"):
pass
class UnFlagFlags(SharedFlags, prefix="+"):
pass
silent_action=True, | ||
) | ||
|
||
async def is_flagged(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: Consider moving is_flagged method to a common location
The is_flagged method is duplicated in both flag.py and unflag.py. Consider moving this to a common location, such as a base class or utility module, to avoid code repetition and improve maintainability.
from tux.utils.moderation import is_flagged
class Flag(commands.Cog):
# ... other methods ...
@staticmethod
async def is_flagged(guild_id: int, user_id: int) -> bool:
return await is_flagged(guild_id, user_id)
from . import ModerationCogBase | ||
|
||
|
||
class Unflag(ModerationCogBase): |
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 consolidating flag-related functionality into a single cog and moving shared methods to a utility module.
The current implementation introduces unnecessary complexity through code duplication and inconsistent naming. To simplify and improve maintainability:
- Combine the flag and unflag functionality into a single cog:
class FlagManagement(ModerationCogBase):
def __init__(self, bot: Tux) -> None:
super().__init__(bot)
self.case_controller = CaseController()
self.flag.usage = generate_usage(self.flag, FlagFlags)
self.unflag.usage = generate_usage(self.unflag, FlagFlags)
@commands.hybrid_command(name="flag", aliases=["fl"])
@commands.guild_only()
@checks.has_pl(2)
async def flag(self, ctx: commands.Context[Tux], member: discord.Member, *, flags: FlagFlags) -> None:
# ... (existing flag implementation)
@commands.hybrid_command(name="unflag", aliases=["ufl"])
@commands.guild_only()
@checks.has_pl(2)
async def unflag(self, ctx: commands.Context[Tux], member: discord.Member, *, flags: FlagFlags) -> None:
# ... (unflag implementation)
# ... (other methods)
- Move the
is_flagged
method to a shared utility module:
# In tux/utils/moderation.py
from tux.database.controllers.case import CaseController
from prisma.enums import CaseType
async def is_flagged(guild_id: int, user_id: int) -> bool:
case_controller = CaseController()
flag_cases = await case_controller.get_all_cases_by_type(guild_id, CaseType.FLAG)
unflag_cases = await case_controller.get_all_cases_by_type(guild_id, CaseType.UNFLAG)
flag_count = sum(case.case_user_id == user_id for case in flag_cases)
unflag_count = sum(case.case_user_id == user_id for case in unflag_cases)
return flag_count > unflag_count
Then, in the FlagManagement cog:
from tux.utils.moderation import is_flagged
class FlagManagement(ModerationCogBase):
# ...
async def flag(self, ctx: commands.Context[Tux], member: discord.Member, *, flags: FlagFlags) -> None:
assert ctx.guild
if await is_flagged(ctx.guild.id, member.id):
await ctx.send("User is already flagged.", delete_after=30, ephemeral=True)
return
# ... (rest of the method)
# ...
These changes will reduce code duplication, improve maintainability, and resolve the naming inconsistency while keeping all functionality intact.
from . import ModerationCogBase | ||
|
||
|
||
class Flag(ModerationCogBase): |
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 consolidating flag-related functionality into a single cog and moving shared methods to a utility module.
The current implementation introduces unnecessary complexity through code duplication and separation of closely related functionality. Consider the following improvements:
- Combine flag and unflag functionality into a single cog:
class FlagManagement(ModerationCogBase):
def __init__(self, bot: Tux) -> None:
super().__init__(bot)
self.case_controller = CaseController()
self.flag.usage = generate_usage(self.flag, FlagFlags)
self.unflag.usage = generate_usage(self.unflag, UnFlagFlags)
@commands.hybrid_command(name="flag", aliases=["fl"])
async def flag(self, ctx: commands.Context[Tux], member: discord.Member, *, flags: FlagFlags) -> None:
# Implement flag logic here
@commands.hybrid_command(name="unflag", aliases=["ufl"])
async def unflag(self, ctx: commands.Context[Tux], member: discord.Member, *, flags: UnFlagFlags) -> None:
# Implement unflag logic here (current implementation)
# Other methods...
- Move the
is_flagged
method to a common utility module to avoid duplication:
# In tux/utils/moderation.py
async def is_flagged(case_controller: CaseController, guild_id: int, user_id: int) -> bool:
flag_cases = await case_controller.get_all_cases_by_type(guild_id, CaseType.FLAG)
unflag_cases = await case_controller.get_all_cases_by_type(guild_id, CaseType.UNFLAG)
flag_count = sum(case.case_user_id == user_id for case in flag_cases)
unflag_count = sum(case.case_user_id == user_id for case in unflag_cases)
return flag_count > unflag_count
# In FlagManagement cog
from tux.utils.moderation import is_flagged
class FlagManagement(ModerationCogBase):
# ...
async def unflag(self, ctx: commands.Context[Tux], member: discord.Member, *, flags: UnFlagFlags) -> None:
# ...
if not await is_flagged(self.case_controller, ctx.guild.id, member.id):
await ctx.send("User is not flagged.", delete_after=30, ephemeral=True)
return
# ...
# Remove the is_flagged method from this class
These changes will reduce code duplication, improve maintainability, and provide a more logical structure for flag-related operations while maintaining all existing functionality.
Description
Allows moderators to flag users who may be of concern, adding checks for if user is already flagged or not for each command.
To be improved upon with better features for flagging in the future see #555 for more info.
Emotes for flag and unflag need to be added aswell as they have just been given tempoary emojis.
PR also adds the functionality for cases to not be sent to a logging channel, improves when the handle_case_response is called for maintainability and improving the codebase and updates case embeds to properly convey information about a DM not being sent or a silent action.
Guidelines
My code follows the style guidelines of this project (formatted with Ruff)
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation if needed
My changes generate no new warnings
I have tested this change
Any dependent changes have been merged and published in downstream modules
I have added all appropriate labels to this PR
I have followed all of these guidelines.
How Has This Been Tested? (if applicable)
tested in tux dev
Screenshots (if applicable)
n/a
Additional Information
n/a