Automatically Remove Users from BigBrother Watch List on Perma Ban#821
Conversation
- Added apply_unwatch() and migrated the code from the unwatch command to it. This will give us more control regarding testing and also determining when unwatches trigger. - Added apply_watch() and migrated the code from the watch command to it. Again, this will assist with testing and could make it easier to automate adding to the watch list if need be. - Added unwatch call to apply_ban. User will only be removed from the watch list if they were permanently banned. They will not be removed if it was only temporary. Signed-off-by: Daniel Brown <browndj3@gmail.com>
MrHemlock
added
t: bug
- Added apply_unwatch() and migrated the code from the unwatch command to it. This will give us more control regarding testing and also determining when unwatches trigger. - Added apply_watch() and migrated the code from the watch command to it. Again, this will assist with testing and could make it easier to automate adding to the watch list if need be. - Added unwatch call to apply_ban. User will only be removed from the watch list if they were permanently banned. They will not be removed if it was only temporary. Signed-off-by: Daniel Brown <browndj3@gmail.com>
… into hemlock-perma-ban-watch-removal
Bugs fixed: - Previously, the code would check to see if `'expires_at'` was in the kwargs, which after testing I came to find out that it is regardless of the duration of the ban. It has sense been changed to use a `.get()` in order to do a proper comparison. - Code previously attempted to load from the `"BigBrother"` cog which is the incorrect spelling. Changed it to `"Big Brother"` to correct this. Logging Added: - Additional trace logs added to both the `infractions.py` file as well as `bigbrother.py` to assist with future debugging or testing. Signed-off-by: Daniel Brown <browndj3@gmail.com>
Kaizen: - Cut down on the size of the import line by changing the imports from bot.constants to instead just importing the constants. This will help clarify where certain constants are coming from. - The periodic checkpoint message will no longer ping `@everyone` or `@Admins` when the bot detects that it is being ran in a debug environment. Message is now a simple confirmation that the periodic ping method successfully ran. Signed-off-by: Daniel Brown <browndj3@gmail.com>
MarkKoz
left a comment
MarkKoz
left a comment
There was a problem hiding this comment.
I didn't know two spaces after full stops was a practice. I can live with it but it isn't consistent with any of the other text in this project as far as I know.
| action = ctx.guild.ban(user, reason=reason, delete_message_days=0) | ||
| await self.apply_infraction(ctx, infraction, user, action) | ||
|
|
||
| # Remove perma banned users from the watch list |
There was a problem hiding this comment.
apply_ban's docstring should mention that this happens.
There was a problem hiding this comment.
Addressed in most recent commit
| @with_role(*MODERATION_ROLES) | ||
| async def unwatch_command(self, ctx: Context, user: FetchedMember, *, reason: str) -> None: | ||
| """Stop relaying messages by the given `user`.""" | ||
| async def apply_unwatch(self, ctx: Context, user: FetchedMember, reason: str, banned: bool = False) -> None: |
There was a problem hiding this comment.
I think a more generic name like send_message would be better. Regardless, this parameter should be documented.
There was a problem hiding this comment.
Addressed in most recent commit
| if not banned: # Prevents a message being sent to the channel if part of a permanent ban | ||
| log.trace("User is not banned. Sending message to channel") | ||
| await ctx.send(f":white_check_mark: Messages sent by {user} will no longer be relayed.") | ||
|
|
||
| self._remove_user(user.id) | ||
| else: | ||
| await ctx.send(":x: The specified user is currently not being watched.") | ||
| log.trace("No active watches found for user.") | ||
| if not banned: # Prevents a message being sent to the channel if part of a permanent ban | ||
| log.trace("User is not perma banned. Send the error message.") | ||
| await ctx.send(":x: The specified user is currently not being watched.") |
There was a problem hiding this comment.
I think this will be cleaner if the message is assigned to a parameter and sent at the very end, similar to what the apply_watch function does.
There was a problem hiding this comment.
Addressed in most recent commit
Co-Authored-By: Mark <kozlovmark@gmail.com>
Co-Authored-By: Mark <kozlovmark@gmail.com>
Co-Authored-By: Mark <kozlovmark@gmail.com>
- Docstrings for `apply_ban()` have been edited to mention that the method also removes a banned user from the watch list. - Parameter `banned` in `apply_unwatch()` was changed to `send_message` in order to be more general. Boolean logic was swapped to coincide with that change. - `apply_unwatch()`'s sent message moved to the bottom of the method for clarity. Added `return`s to the method to exit early if no message needs to be sent. Signed-off-by: Daniel Brown <browndj3@gmail.com>
|
|
||
| bb_cog = self.bot.get_cog("Big Brother") | ||
| if not bb_cog: | ||
| log.trace(f"Big Brother cog not loaded; perma-banned user {user} won't be unwatched.") |
There was a problem hiding this comment.
While I understand that this is part of the trace logging flow, if this situation is encountered then this should be issued at the error level, not a trace.
There was a problem hiding this comment.
Corrected in the most recent commit
There was a problem hiding this comment.
Why wouldn't we want to be notified that the big brother cog isn't loaded?
There was a problem hiding this comment.
For the purpose of the trace flow it makes no difference if this is an error or a trace, but for the sake of knowing that something isn't quite right with one of our cogs, it makes total sense for it to be an error. I just didn't think about it when writing it.
There was a problem hiding this comment.
If that's the case then IMO a warning is more appropriate. If there's ever a legitimate reason to unload it, even temporarily, then it'd be strange to be greeted with an error when nothing is actually wrong with the code. If you don't want it to be unloaded, consider adding it to the blacklist in the extensions cog.
There was a problem hiding this comment.
Fair point, a warning makes sense to me. @sco1 that work for you?
- Changed the log for when the big brother cog doesn't load in the `apply_ban()` method doesn't properly load from a trace to an error.
sco1
left a comment
sco1
left a comment
There was a problem hiding this comment.
Looks cool, works cool, very cool
4 participants
Previously we had the bot set up so that when a user was permanently banned it would be automatically unwatched by our "Big Brother" system. This originally worked just fine, but the issue was that there was no embed being sent telling us about this. However, after the big rewrite, that automated functionality was lost in the process. This PR is intended to restore it.
The discussed solution was to move most of the current
unwatchcommand's functionality into a helper method and allow for a flag to be set when it's sent from a permanent ban. Thewatchcommand will also have its functionality moved to a helper method for consistency and ease of testing.Closes #321