From 9b8d688657f7044ad27ff81f7eb7d50f7f593ed6 Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Fri, 25 Oct 2019 12:10:26 +0200 Subject: Autodelete offensive messages after one week. If the filter cog filter a message that's considered as offensive (filter["offensive_msg"] is True), the cog create a new offensive message object in the bot db with a delete_date of one week after it was sent. A background task run every day, pull up a list of message to delete, find them back, and delete them. --- bot/cogs/filtering.py | 89 +++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 79 insertions(+), 10 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 1d1d74e74..d1d28ac10 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -1,12 +1,15 @@ +import asyncio +import datetime import logging import re from typing import Optional, Union import discord.errors from dateutil.relativedelta import relativedelta -from discord import Colour, DMChannel, Member, Message, TextChannel +from discord import Colour, DMChannel, Member, Message, NotFound, TextChannel from discord.ext.commands import Bot, Cog +from bot.api import ResponseCodeError from bot.cogs.moderation import ModLog from bot.constants import ( Channels, Colours, DEBUG_MODE, @@ -16,13 +19,13 @@ from bot.constants import ( log = logging.getLogger(__name__) INVITE_RE = re.compile( - r"(?:discord(?:[\.,]|dot)gg|" # Could be discord.gg/ - r"discord(?:[\.,]|dot)com(?:\/|slash)invite|" # or discord.com/invite/ + r"(?:discord(?:[\.,]|dot)gg|" # Could be discord.gg/ + r"discord(?:[\.,]|dot)com(?:\/|slash)invite|" # or discord.com/invite/ r"discordapp(?:[\.,]|dot)com(?:\/|slash)invite|" # or discordapp.com/invite/ - r"discord(?:[\.,]|dot)me|" # or discord.me - r"discord(?:[\.,]|dot)io" # or discord.io. - r")(?:[\/]|slash)" # / or 'slash' - r"([a-zA-Z0-9]+)", # the invite code itself + r"discord(?:[\.,]|dot)me|" # or discord.me + r"discord(?:[\.,]|dot)io" # or discord.io. + r")(?:[\/]|slash)" # / or 'slash' + r"([a-zA-Z0-9]+)", # the invite code itself flags=re.IGNORECASE ) @@ -36,6 +39,8 @@ TOKEN_WATCHLIST_PATTERNS = [ re.compile(fr'{expression}', flags=re.IGNORECASE) for expression in Filter.token_watchlist ] +OFFENSIVE_MSG_DELETE_TIME = datetime.timedelta(days=7) # Time before an offensive msg is deleted. + class Filtering(Cog): """Filtering out invites, blacklisting domains, and warning us of certain regular expressions.""" @@ -54,7 +59,8 @@ class Filtering(Cog): "notification_msg": ( "Your post has been removed for abusing Unicode character rendering (aka Zalgo text). " f"{_staff_mistake_str}" - ) + ), + "offensive_msg": False }, "filter_invites": { "enabled": Filter.filter_invites, @@ -65,7 +71,8 @@ class Filtering(Cog): "notification_msg": ( f"Per Rule 10, your invite link has been removed. {_staff_mistake_str}\n\n" r"Our server rules can be found here: " - ) + ), + "offensive_msg": False }, "filter_domains": { "enabled": Filter.filter_domains, @@ -75,28 +82,47 @@ class Filtering(Cog): "user_notification": Filter.notify_user_domains, "notification_msg": ( f"Your URL has been removed because it matched a blacklisted domain. {_staff_mistake_str}" - ) + ), + "offensive_msg": False }, "watch_rich_embeds": { "enabled": Filter.watch_rich_embeds, "function": self._has_rich_embed, "type": "watchlist", "content_only": False, + "offensive_msg": False }, "watch_words": { "enabled": Filter.watch_words, "function": self._has_watchlist_words, "type": "watchlist", "content_only": True, + "offensive_msg": True }, "watch_tokens": { "enabled": Filter.watch_tokens, "function": self._has_watchlist_tokens, "type": "watchlist", "content_only": True, + "offensive_msg": True }, } + self.deletion_task = None + self.bot.loop.create_task(self.init_deletion_task()) + + def cog_unload(self) -> None: + """Cancel any running updater tasks on cog unload.""" + if self.deletion_task is not None: + self.deletion_task.cancel() + + async def init_deletion_task(self) -> None: + """Start offensive messages deletion event loop if it hasn't already started.""" + await self.bot.wait_until_ready() + if self.deletion_task is None: + coro = delete_offensive_msg(self.bot) + self.deletion_task = self.bot.loop.create_task(coro) + @property def mod_log(self) -> ModLog: """Get currently loaded ModLog cog instance.""" @@ -159,6 +185,21 @@ class Filtering(Cog): triggered = await _filter["function"](msg) if triggered: + # If the message is classed as offensive, we store it in the site db and + # it will be deleted it after one week. + if _filter["offensive_msg"]: + delete_date = msg.created_at.date() + OFFENSIVE_MSG_DELETE_TIME + await self.bot.api_client.post( + 'bot/offensive-message', + json={ + 'id': msg.id, + 'channel_id': msg.channel.id, + 'delete_date': delete_date.isoformat() + } + ) + log.trace(f"Offensive message will be deleted on " + f"{delete_date.isoformat()}") + # If this is a filter (not a watchlist), we should delete the message. if _filter["type"] == "filter": try: @@ -360,6 +401,34 @@ class Filtering(Cog): await channel.send(f"{filtered_member.mention} {reason}") +async def delete_offensive_msg(bot: Bot) -> None: + """Background task that pull up a list of offensive messages every day and delete them.""" + while True: + tomorrow = datetime.date.today() + datetime.timedelta(days=1) + time_until_next = datetime.datetime(tomorrow.year, tomorrow.month, tomorrow.day) - datetime.datetime.now() + try: + msg_list = await bot.api_client.get( + 'bot/offensive-message', + params={'delete_date': datetime.date.today().isoformat()} + ) + except ResponseCodeError as e: + log.error(f"Failed to get offending messages to delete (got code {e.response.status}), " + f"retrying in 30 minutes.") + time_until_next = datetime.timedelta(minutes=30) + msg_list = [] + for msg in msg_list: + try: + channel = bot.get_channel(msg['channel_id']) + if channel: + msg_obj = await channel.fetch_message(msg['id']) + await msg_obj.delete() + except NotFound: + log.info(f"Tried to delete message {msg['id']}, but the message can't be found " + f"(it has been probably already deleted).") + log.info(f"Deleted {len(msg_list)} offensive message(s).") + await asyncio.sleep(time_until_next.seconds) + + def setup(bot: Bot) -> None: """Filtering cog load.""" bot.add_cog(Filtering(bot)) -- cgit v1.2.3 From 59914bf3e741d654d999ff5eb7f9c12621628c6b Mon Sep 17 00:00:00 2001 From: Matteo Bertucci Date: Fri, 25 Oct 2019 15:00:20 +0200 Subject: Revert whitespace changes --- bot/cogs/filtering.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index d1d28ac10..4c99be0af 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -19,13 +19,13 @@ from bot.constants import ( log = logging.getLogger(__name__) INVITE_RE = re.compile( - r"(?:discord(?:[\.,]|dot)gg|" # Could be discord.gg/ - r"discord(?:[\.,]|dot)com(?:\/|slash)invite|" # or discord.com/invite/ + r"(?:discord(?:[\.,]|dot)gg|" # Could be discord.gg/ + r"discord(?:[\.,]|dot)com(?:\/|slash)invite|" # or discord.com/invite/ r"discordapp(?:[\.,]|dot)com(?:\/|slash)invite|" # or discordapp.com/invite/ - r"discord(?:[\.,]|dot)me|" # or discord.me - r"discord(?:[\.,]|dot)io" # or discord.io. - r")(?:[\/]|slash)" # / or 'slash' - r"([a-zA-Z0-9]+)", # the invite code itself + r"discord(?:[\.,]|dot)me|" # or discord.me + r"discord(?:[\.,]|dot)io" # or discord.io. + r")(?:[\/]|slash)" # / or 'slash' + r"([a-zA-Z0-9]+)", # the invite code itself flags=re.IGNORECASE ) -- cgit v1.2.3 From 9c78146c79dd7b4c3a633f3eabeef2036eb8ab7f Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Fri, 25 Oct 2019 18:39:58 +0200 Subject: Move offensive message delete time to config file. --- bot/cogs/filtering.py | 2 +- bot/constants.py | 1 + config-default.yml | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 4c99be0af..f9aee5a9a 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -39,7 +39,7 @@ TOKEN_WATCHLIST_PATTERNS = [ re.compile(fr'{expression}', flags=re.IGNORECASE) for expression in Filter.token_watchlist ] -OFFENSIVE_MSG_DELETE_TIME = datetime.timedelta(days=7) # Time before an offensive msg is deleted. +OFFENSIVE_MSG_DELETE_TIME = datetime.timedelta(days=Filter.offensive_msg_delete_time) class Filtering(Cog): diff --git a/bot/constants.py b/bot/constants.py index 4beae84e9..6106d911c 100644 --- a/bot/constants.py +++ b/bot/constants.py @@ -211,6 +211,7 @@ class Filter(metaclass=YAMLGetter): notify_user_domains: bool ping_everyone: bool + offensive_msg_delete_time: int guild_invite_whitelist: List[int] domain_blacklist: List[str] word_watchlist: List[str] diff --git a/config-default.yml b/config-default.yml index 197743296..fc702e991 100644 --- a/config-default.yml +++ b/config-default.yml @@ -161,7 +161,8 @@ filter: notify_user_domains: false # Filter configuration - ping_everyone: true # Ping @everyone when we send a mod-alert? + ping_everyone: true # Ping @everyone when we send a mod-alert? + offensive_msg_delete_time: 7 # How many days before deleting an offensive message? guild_invite_whitelist: - 280033776820813825 # Functional Programming -- cgit v1.2.3 From f67378c77a45c581c57d1dfdd5a704319e83ba3a Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Fri, 25 Oct 2019 18:43:27 +0200 Subject: Remove the possibility that we send a message to the API that the filter has already deleted. --- bot/cogs/filtering.py | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index f9aee5a9a..ea6919707 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -185,21 +185,6 @@ class Filtering(Cog): triggered = await _filter["function"](msg) if triggered: - # If the message is classed as offensive, we store it in the site db and - # it will be deleted it after one week. - if _filter["offensive_msg"]: - delete_date = msg.created_at.date() + OFFENSIVE_MSG_DELETE_TIME - await self.bot.api_client.post( - 'bot/offensive-message', - json={ - 'id': msg.id, - 'channel_id': msg.channel.id, - 'delete_date': delete_date.isoformat() - } - ) - log.trace(f"Offensive message will be deleted on " - f"{delete_date.isoformat()}") - # If this is a filter (not a watchlist), we should delete the message. if _filter["type"] == "filter": try: @@ -216,6 +201,21 @@ class Filtering(Cog): except discord.errors.NotFound: return + # If the message is classed as offensive, we store it in the site db and + # it will be deleted it after one week. + if _filter["offensive_msg"]: + delete_date = msg.created_at.date() + OFFENSIVE_MSG_DELETE_TIME + await self.bot.api_client.post( + 'bot/offensive-message', + json={ + 'id': msg.id, + 'channel_id': msg.channel.id, + 'delete_date': delete_date.isoformat() + } + ) + log.trace(f"Offensive message will be deleted on " + f"{delete_date.isoformat()}") + # Notify the user if the filter specifies if _filter["user_notification"]: await self.notify_member(msg.author, _filter["notification_msg"], msg.channel) -- cgit v1.2.3 From 1eb057b229c9dcae15834c55faf7188360b675a2 Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Fri, 25 Oct 2019 18:44:50 +0200 Subject: Rename offensive_msg flag to schedule_deletion. --- bot/cogs/filtering.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index ea6919707..1342dade8 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -60,7 +60,7 @@ class Filtering(Cog): "Your post has been removed for abusing Unicode character rendering (aka Zalgo text). " f"{_staff_mistake_str}" ), - "offensive_msg": False + "schedule_deletion": False }, "filter_invites": { "enabled": Filter.filter_invites, @@ -72,7 +72,7 @@ class Filtering(Cog): f"Per Rule 10, your invite link has been removed. {_staff_mistake_str}\n\n" r"Our server rules can be found here: " ), - "offensive_msg": False + "schedule_deletion": False }, "filter_domains": { "enabled": Filter.filter_domains, @@ -83,28 +83,28 @@ class Filtering(Cog): "notification_msg": ( f"Your URL has been removed because it matched a blacklisted domain. {_staff_mistake_str}" ), - "offensive_msg": False + "schedule_deletion": False }, "watch_rich_embeds": { "enabled": Filter.watch_rich_embeds, "function": self._has_rich_embed, "type": "watchlist", "content_only": False, - "offensive_msg": False + "schedule_deletion": False }, "watch_words": { "enabled": Filter.watch_words, "function": self._has_watchlist_words, "type": "watchlist", "content_only": True, - "offensive_msg": True + "schedule_deletion": True }, "watch_tokens": { "enabled": Filter.watch_tokens, "function": self._has_watchlist_tokens, "type": "watchlist", "content_only": True, - "offensive_msg": True + "schedule_deletion": True }, } @@ -203,7 +203,7 @@ class Filtering(Cog): # If the message is classed as offensive, we store it in the site db and # it will be deleted it after one week. - if _filter["offensive_msg"]: + if _filter["schedule_deletion"]: delete_date = msg.created_at.date() + OFFENSIVE_MSG_DELETE_TIME await self.bot.api_client.post( 'bot/offensive-message', -- cgit v1.2.3 From f306c4153a5d4fb969856a2282e7ff3f7a111885 Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Fri, 25 Oct 2019 21:16:04 +0200 Subject: Use Scheduler instead of a custom async loop --- bot/cogs/filtering.py | 84 ++++++++++++++++++++++++++------------------------- 1 file changed, 43 insertions(+), 41 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 1342dade8..35c14f101 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -2,19 +2,20 @@ import asyncio import datetime import logging import re -from typing import Optional, Union +from typing import Mapping, Optional, Union import discord.errors from dateutil.relativedelta import relativedelta from discord import Colour, DMChannel, Member, Message, NotFound, TextChannel from discord.ext.commands import Bot, Cog -from bot.api import ResponseCodeError from bot.cogs.moderation import ModLog from bot.constants import ( Channels, Colours, DEBUG_MODE, Filter, Icons, URLs ) +from bot.utils.scheduling import Scheduler +from bot.utils.time import wait_until log = logging.getLogger(__name__) @@ -42,11 +43,12 @@ TOKEN_WATCHLIST_PATTERNS = [ OFFENSIVE_MSG_DELETE_TIME = datetime.timedelta(days=Filter.offensive_msg_delete_time) -class Filtering(Cog): +class Filtering(Cog, Scheduler): """Filtering out invites, blacklisting domains, and warning us of certain regular expressions.""" def __init__(self, bot: Bot): self.bot = bot + super().__init__() _staff_mistake_str = "If you believe this was a mistake, please let staff know!" self.filters = { @@ -109,19 +111,7 @@ class Filtering(Cog): } self.deletion_task = None - self.bot.loop.create_task(self.init_deletion_task()) - - def cog_unload(self) -> None: - """Cancel any running updater tasks on cog unload.""" - if self.deletion_task is not None: - self.deletion_task.cancel() - - async def init_deletion_task(self) -> None: - """Start offensive messages deletion event loop if it hasn't already started.""" - await self.bot.wait_until_ready() - if self.deletion_task is None: - coro = delete_offensive_msg(self.bot) - self.deletion_task = self.bot.loop.create_task(coro) + self.bot.loop.create_task(self.reschedule_offensive_msg_deletion()) @property def mod_log(self) -> ModLog: @@ -400,33 +390,45 @@ class Filtering(Cog): except discord.errors.Forbidden: await channel.send(f"{filtered_member.mention} {reason}") + async def _scheduled_task(self, msg: dict) -> None: + """A coroutine which delete the offensive message once the delete date is reached.""" + delete_at = datetime.datetime.fromisoformat(msg['delete_date'][:-1]) + + await wait_until(delete_at) + await self.delete_offensive_msg(msg) + + self.cancel_task(msg['id']) + + async def reschedule_offensive_msg_deletion(self) -> None: + """Get all the pending message deletion from the API and reschedule them.""" + await self.bot.wait_until_ready() + response = await self.bot.api_client.get( + 'bot/offensive-message', + ) + + now = datetime.datetime.utcnow() + loop = asyncio.get_event_loop() -async def delete_offensive_msg(bot: Bot) -> None: - """Background task that pull up a list of offensive messages every day and delete them.""" - while True: - tomorrow = datetime.date.today() + datetime.timedelta(days=1) - time_until_next = datetime.datetime(tomorrow.year, tomorrow.month, tomorrow.day) - datetime.datetime.now() + for msg in response: + delete_at = datetime.datetime.fromisoformat(msg['delete_date'][:-1]) + + if delete_at < now: + await self.delete_offensive_msg(msg) + else: + self.schedule_task(loop, msg['id'], msg) + + async def delete_offensive_msg(self, msg: Mapping[str, str]) -> None: + """Delete an offensive message, and then delete it from the db.""" try: - msg_list = await bot.api_client.get( - 'bot/offensive-message', - params={'delete_date': datetime.date.today().isoformat()} - ) - except ResponseCodeError as e: - log.error(f"Failed to get offending messages to delete (got code {e.response.status}), " - f"retrying in 30 minutes.") - time_until_next = datetime.timedelta(minutes=30) - msg_list = [] - for msg in msg_list: - try: - channel = bot.get_channel(msg['channel_id']) - if channel: - msg_obj = await channel.fetch_message(msg['id']) - await msg_obj.delete() - except NotFound: - log.info(f"Tried to delete message {msg['id']}, but the message can't be found " - f"(it has been probably already deleted).") - log.info(f"Deleted {len(msg_list)} offensive message(s).") - await asyncio.sleep(time_until_next.seconds) + channel = self.bot.get_channel(msg['channel_id']) + if channel: + msg_obj = await channel.fetch_message(msg['id']) + await msg_obj.delete() + except NotFound: + log.info(f"Tried to delete message {msg['id']}, but the message can't be found " + f"(it has been probably already deleted).") + await self.bot.api_client.delete(f'bot/offensive-message/{msg["id"]}') + log.info(f"Deleted the offensive message with id {msg['id']}.") def setup(bot: Bot) -> None: -- cgit v1.2.3 From 95c6e56891a21ebb2d1555cf850daad375d57afe Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Sat, 26 Oct 2019 11:11:15 +0200 Subject: Switch to datetime.datetime --- bot/cogs/filtering.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 5bd72a584..8962a85c1 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -187,25 +187,25 @@ class Filtering(Cog, Scheduler): except discord.errors.NotFound: return + # Notify the user if the filter specifies + if _filter["user_notification"]: + await self.notify_member(msg.author, _filter["notification_msg"], msg.channel) + # If the message is classed as offensive, we store it in the site db and # it will be deleted it after one week. if _filter["schedule_deletion"]: - delete_date = msg.created_at.date() + OFFENSIVE_MSG_DELETE_TIME + delete_date = msg.created_at + OFFENSIVE_MSG_DELETE_TIME await self.bot.api_client.post( 'bot/offensive-message', json={ 'id': msg.id, 'channel_id': msg.channel.id, - 'delete_date': delete_date.isoformat() + 'delete_date': delete_date.isoformat()[:-1] } ) log.trace(f"Offensive message will be deleted on " f"{delete_date.isoformat()}") - # Notify the user if the filter specifies - if _filter["user_notification"]: - await self.notify_member(msg.author, _filter["notification_msg"], msg.channel) - if isinstance(msg.channel, DMChannel): channel_str = "via DM" else: -- cgit v1.2.3 From b57811ea12aa41285e4e4585b951dd105be4b275 Mon Sep 17 00:00:00 2001 From: Matteo Bertucci Date: Thu, 12 Dec 2019 09:25:53 +0100 Subject: Add space for readability Co-Authored-By: Mark --- bot/cogs/filtering.py | 1 + 1 file changed, 1 insertion(+) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 61c8f389b..39ba22354 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -436,6 +436,7 @@ class Filtering(Cog, Scheduler): except NotFound: log.info(f"Tried to delete message {msg['id']}, but the message can't be found " f"(it has been probably already deleted).") + await self.bot.api_client.delete(f'bot/offensive-message/{msg["id"]}') log.info(f"Deleted the offensive message with id {msg['id']}.") -- cgit v1.2.3 From bc9b335454d0183013b4f89f2923b638dcc127ba Mon Sep 17 00:00:00 2001 From: Matteo Bertucci Date: Thu, 12 Dec 2019 09:38:26 +0100 Subject: Make use of the Bot subclass --- bot/cogs/filtering.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index ca84e6240..9d88d9153 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -7,7 +7,7 @@ from typing import Mapping, Optional, Union import discord.errors from dateutil.relativedelta import relativedelta from discord import Colour, DMChannel, Member, Message, NotFound, TextChannel -from discord.ext.commands import Bot, Cog +from discord.ext.commands import Cog from bot.bot import Bot from bot.cogs.moderation import ModLog -- cgit v1.2.3 From 7eea7ad353239a0be1dfd744486e3f46a99cd661 Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Sat, 14 Dec 2019 11:21:44 +0100 Subject: Filtering cog clean up --- bot/cogs/filtering.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 9d88d9153..172c5fa7e 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -111,7 +111,6 @@ class Filtering(Cog, Scheduler): }, } - self.deletion_task = None self.bot.loop.create_task(self.reschedule_offensive_msg_deletion()) @property @@ -201,11 +200,13 @@ class Filtering(Cog, Scheduler): json={ 'id': msg.id, 'channel_id': msg.channel.id, - 'delete_date': delete_date.isoformat()[:-1] + 'delete_date': delete_date.isoformat() } ) - log.trace(f"Offensive message will be deleted on " - f"{delete_date.isoformat()}") + log.trace( + f"Offensive message will be deleted on " + f"{delete_date.isoformat()}" + ) if isinstance(msg.channel, DMChannel): channel_str = "via DM" @@ -412,9 +413,7 @@ class Filtering(Cog, Scheduler): async def reschedule_offensive_msg_deletion(self) -> None: """Get all the pending message deletion from the API and reschedule them.""" await self.bot.wait_until_ready() - response = await self.bot.api_client.get( - 'bot/offensive-message', - ) + response = await self.bot.api_client.get('bot/offensive-message',) now = datetime.datetime.utcnow() loop = asyncio.get_event_loop() @@ -435,8 +434,10 @@ class Filtering(Cog, Scheduler): msg_obj = await channel.fetch_message(msg['id']) await msg_obj.delete() except NotFound: - log.info(f"Tried to delete message {msg['id']}, but the message can't be found " - f"(it has been probably already deleted).") + log.info( + f"Tried to delete message {msg['id']}, but the message can't be found " + f"(it has been probably already deleted)." + ) await self.bot.api_client.delete(f'bot/offensive-message/{msg["id"]}') log.info(f"Deleted the offensive message with id {msg['id']}.") -- cgit v1.2.3 From ba5af375a3acfc160de1ffefa063a915318e6bdd Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Sat, 14 Dec 2019 11:25:59 +0100 Subject: Make use of dateutil.parser.isoparse --- bot/cogs/filtering.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 172c5fa7e..4388b29ad 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -4,6 +4,7 @@ import logging import re from typing import Mapping, Optional, Union +import dateutil import discord.errors from dateutil.relativedelta import relativedelta from discord import Colour, DMChannel, Member, Message, NotFound, TextChannel @@ -403,7 +404,7 @@ class Filtering(Cog, Scheduler): async def _scheduled_task(self, msg: dict) -> None: """A coroutine which delete the offensive message once the delete date is reached.""" - delete_at = datetime.datetime.fromisoformat(msg['delete_date'][:-1]) + delete_at = dateutil.parser.isoparse(msg['delete_date']) await wait_until(delete_at) await self.delete_offensive_msg(msg) @@ -419,7 +420,7 @@ class Filtering(Cog, Scheduler): loop = asyncio.get_event_loop() for msg in response: - delete_at = datetime.datetime.fromisoformat(msg['delete_date'][:-1]) + delete_at = dateutil.parser.isoparse(msg['delete_date']) if delete_at < now: await self.delete_offensive_msg(msg) -- cgit v1.2.3 From e6940b938882aaeda18baa9fcc23cc297c6cfcd2 Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Sat, 14 Dec 2019 11:31:32 +0100 Subject: Rename config entry to offensive_msg_delete_days --- bot/cogs/filtering.py | 2 +- bot/constants.py | 2 +- config-default.yml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 4388b29ad..c0e115a8f 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -42,7 +42,7 @@ TOKEN_WATCHLIST_PATTERNS = [ re.compile(fr'{expression}', flags=re.IGNORECASE) for expression in Filter.token_watchlist ] -OFFENSIVE_MSG_DELETE_TIME = datetime.timedelta(days=Filter.offensive_msg_delete_time) +OFFENSIVE_MSG_DELETE_TIME = datetime.timedelta(days=Filter.offensive_msg_delete_day) class Filtering(Cog, Scheduler): diff --git a/bot/constants.py b/bot/constants.py index 075722a01..e6f23ff61 100644 --- a/bot/constants.py +++ b/bot/constants.py @@ -211,7 +211,7 @@ class Filter(metaclass=YAMLGetter): notify_user_domains: bool ping_everyone: bool - offensive_msg_delete_time: int + offensive_msg_delete_day: int guild_invite_whitelist: List[int] domain_blacklist: List[str] word_watchlist: List[str] diff --git a/config-default.yml b/config-default.yml index 0765407af..33072790b 100644 --- a/config-default.yml +++ b/config-default.yml @@ -180,7 +180,7 @@ filter: # Filter configuration ping_everyone: true # Ping @everyone when we send a mod-alert? - offensive_msg_delete_time: 7 # How many days before deleting an offensive message? + offensive_msg_delete_days: 7 # How many days before deleting an offensive message? guild_invite_whitelist: - 280033776820813825 # Functional Programming -- cgit v1.2.3 From b2a5ab90622e986ace37be6204d6a59823390cce Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Sat, 14 Dec 2019 11:36:37 +0100 Subject: Catch all HTTPExecption --- bot/cogs/filtering.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index c0e115a8f..c0a2c7d3b 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -7,7 +7,7 @@ from typing import Mapping, Optional, Union import dateutil import discord.errors from dateutil.relativedelta import relativedelta -from discord import Colour, DMChannel, Member, Message, NotFound, TextChannel +from discord import Colour, DMChannel, HTTPException, Member, Message, NotFound, TextChannel from discord.ext.commands import Cog from bot.bot import Bot @@ -439,6 +439,10 @@ class Filtering(Cog, Scheduler): f"Tried to delete message {msg['id']}, but the message can't be found " f"(it has been probably already deleted)." ) + except HTTPException: + log.warning( + f"Failed to delete message {msg['id']}." + ) await self.bot.api_client.delete(f'bot/offensive-message/{msg["id"]}') log.info(f"Deleted the offensive message with id {msg['id']}.") -- cgit v1.2.3 From ab6b0032ceb6ad638cee6f174778bc38254ab038 Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Sat, 14 Dec 2019 11:41:30 +0100 Subject: Actually schedule message for deletion --- bot/cogs/filtering.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index c0a2c7d3b..0879bfee6 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -204,6 +204,8 @@ class Filtering(Cog, Scheduler): 'delete_date': delete_date.isoformat() } ) + loop = asyncio.get_event_loop() + self.schedule_task(loop, msg.id, {'id': msg.id, 'channel_id': msg.channel.id}) log.trace( f"Offensive message will be deleted on " f"{delete_date.isoformat()}" -- cgit v1.2.3 From a81ad8f1f7d548f20d1f2428a808e14dbbfe22bc Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Sat, 14 Dec 2019 11:42:10 +0100 Subject: Fix docstring typo --- bot/cogs/filtering.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 0879bfee6..2c3f41c05 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -405,7 +405,7 @@ class Filtering(Cog, Scheduler): await channel.send(f"{filtered_member.mention} {reason}") async def _scheduled_task(self, msg: dict) -> None: - """A coroutine which delete the offensive message once the delete date is reached.""" + """A coroutine that delete the offensive message once the delete date is reached.""" delete_at = dateutil.parser.isoparse(msg['delete_date']) await wait_until(delete_at) -- cgit v1.2.3 From 15958986ae6bcb508c4e2dd23c51624d4ee26cb5 Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Sat, 14 Dec 2019 11:47:08 +0100 Subject: Rename route /bot/offensive-message to /bot/offensive-messages --- bot/cogs/filtering.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 2c3f41c05..63f8685c9 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -197,7 +197,7 @@ class Filtering(Cog, Scheduler): if _filter["schedule_deletion"]: delete_date = msg.created_at + OFFENSIVE_MSG_DELETE_TIME await self.bot.api_client.post( - 'bot/offensive-message', + 'bot/offensive-messages', json={ 'id': msg.id, 'channel_id': msg.channel.id, @@ -416,7 +416,7 @@ class Filtering(Cog, Scheduler): async def reschedule_offensive_msg_deletion(self) -> None: """Get all the pending message deletion from the API and reschedule them.""" await self.bot.wait_until_ready() - response = await self.bot.api_client.get('bot/offensive-message',) + response = await self.bot.api_client.get('bot/offensive-messages',) now = datetime.datetime.utcnow() loop = asyncio.get_event_loop() @@ -446,7 +446,7 @@ class Filtering(Cog, Scheduler): f"Failed to delete message {msg['id']}." ) - await self.bot.api_client.delete(f'bot/offensive-message/{msg["id"]}') + await self.bot.api_client.delete(f'bot/offensive-messages/{msg["id"]}') log.info(f"Deleted the offensive message with id {msg['id']}.") -- cgit v1.2.3 From 2f5b9fbd7bc2a11452a982f61c7603d589ded95a Mon Sep 17 00:00:00 2001 From: Akarys42 Date: Sat, 14 Dec 2019 11:52:54 +0100 Subject: Make setting filter.offensive_msg_delete_days plural --- bot/cogs/filtering.py | 2 +- bot/constants.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 63f8685c9..a709fe7cd 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -42,7 +42,7 @@ TOKEN_WATCHLIST_PATTERNS = [ re.compile(fr'{expression}', flags=re.IGNORECASE) for expression in Filter.token_watchlist ] -OFFENSIVE_MSG_DELETE_TIME = datetime.timedelta(days=Filter.offensive_msg_delete_day) +OFFENSIVE_MSG_DELETE_TIME = datetime.timedelta(days=Filter.offensive_msg_delete_days) class Filtering(Cog, Scheduler): diff --git a/bot/constants.py b/bot/constants.py index e6f23ff61..f47688185 100644 --- a/bot/constants.py +++ b/bot/constants.py @@ -211,7 +211,7 @@ class Filter(metaclass=YAMLGetter): notify_user_domains: bool ping_everyone: bool - offensive_msg_delete_day: int + offensive_msg_delete_days: int guild_invite_whitelist: List[int] domain_blacklist: List[str] word_watchlist: List[str] -- cgit v1.2.3 From 4cf2166d6dee6a04a055db7e14cf4ece0656213a Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 15 Dec 2019 09:39:58 -0800 Subject: Filtering: log the status code of caught HTTPException --- bot/cogs/filtering.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index a709fe7cd..4d91432e7 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -441,10 +441,8 @@ class Filtering(Cog, Scheduler): f"Tried to delete message {msg['id']}, but the message can't be found " f"(it has been probably already deleted)." ) - except HTTPException: - log.warning( - f"Failed to delete message {msg['id']}." - ) + except HTTPException as e: + log.warning(f"Failed to delete message {msg['id']}: status {e.status}") await self.bot.api_client.delete(f'bot/offensive-messages/{msg["id"]}') log.info(f"Deleted the offensive message with id {msg['id']}.") -- cgit v1.2.3 From 7e4a43546490d90c619d08837da69466768fae80 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 15 Dec 2019 09:39:00 -0800 Subject: Filtering: refactor scheduling of deletion task --- bot/cogs/filtering.py | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 4d91432e7..4c94b73a5 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -1,4 +1,3 @@ -import asyncio import datetime import logging import re @@ -195,21 +194,19 @@ class Filtering(Cog, Scheduler): # If the message is classed as offensive, we store it in the site db and # it will be deleted it after one week. if _filter["schedule_deletion"]: - delete_date = msg.created_at + OFFENSIVE_MSG_DELETE_TIME + delete_date = (msg.created_at + OFFENSIVE_MSG_DELETE_TIME).isoformat() await self.bot.api_client.post( 'bot/offensive-messages', json={ 'id': msg.id, 'channel_id': msg.channel.id, - 'delete_date': delete_date.isoformat() + 'delete_date': delete_date } ) - loop = asyncio.get_event_loop() - self.schedule_task(loop, msg.id, {'id': msg.id, 'channel_id': msg.channel.id}) - log.trace( - f"Offensive message will be deleted on " - f"{delete_date.isoformat()}" - ) + + task_data = {'id': msg.id, 'channel_id': msg.channel.id} + self.schedule_task(self.bot.loop, msg.id, task_data) + log.trace(f"Offensive message {msg.id} will be deleted on {delete_date}") if isinstance(msg.channel, DMChannel): channel_str = "via DM" @@ -335,7 +332,7 @@ class Filtering(Cog, Scheduler): Attempts to catch some of common ways to try to cheat the system. """ - # Remove backslashes to prevent escape character aroundfuckery like + # Remove backslashes to prevent escape character around fuckery like # discord\.gg/gdudes-pony-farm text = text.replace("\\", "") @@ -405,7 +402,7 @@ class Filtering(Cog, Scheduler): await channel.send(f"{filtered_member.mention} {reason}") async def _scheduled_task(self, msg: dict) -> None: - """A coroutine that delete the offensive message once the delete date is reached.""" + """Delete an offensive message once its deletion date is reached.""" delete_at = dateutil.parser.isoparse(msg['delete_date']) await wait_until(delete_at) @@ -419,7 +416,6 @@ class Filtering(Cog, Scheduler): response = await self.bot.api_client.get('bot/offensive-messages',) now = datetime.datetime.utcnow() - loop = asyncio.get_event_loop() for msg in response: delete_at = dateutil.parser.isoparse(msg['delete_date']) @@ -427,7 +423,7 @@ class Filtering(Cog, Scheduler): if delete_at < now: await self.delete_offensive_msg(msg) else: - self.schedule_task(loop, msg['id'], msg) + self.schedule_task(self.bot.loop, msg['id'], msg) async def delete_offensive_msg(self, msg: Mapping[str, str]) -> None: """Delete an offensive message, and then delete it from the db.""" -- cgit v1.2.3 From 832762add664186665904817e9ffc25f79ffb20a Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 15 Dec 2019 10:03:19 -0800 Subject: Filtering: fix comparison between tz naïve and aware datetimes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- bot/cogs/filtering.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 4c94b73a5..02e3011ab 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -418,7 +418,7 @@ class Filtering(Cog, Scheduler): now = datetime.datetime.utcnow() for msg in response: - delete_at = dateutil.parser.isoparse(msg['delete_date']) + delete_at = dateutil.parser.isoparse(msg['delete_date']).replace(tzinfo=None) if delete_at < now: await self.delete_offensive_msg(msg) -- cgit v1.2.3 From b36206e8c62459f22685c285fb1a7e299f08b1bd Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 15 Dec 2019 11:12:56 -0800 Subject: Filtering: fix missing deletion date in scheduled task data --- bot/cogs/filtering.py | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 02e3011ab..83e706a26 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -195,17 +195,14 @@ class Filtering(Cog, Scheduler): # it will be deleted it after one week. if _filter["schedule_deletion"]: delete_date = (msg.created_at + OFFENSIVE_MSG_DELETE_TIME).isoformat() - await self.bot.api_client.post( - 'bot/offensive-messages', - json={ - 'id': msg.id, - 'channel_id': msg.channel.id, - 'delete_date': delete_date - } - ) - - task_data = {'id': msg.id, 'channel_id': msg.channel.id} - self.schedule_task(self.bot.loop, msg.id, task_data) + data = { + 'id': msg.id, + 'channel_id': msg.channel.id, + 'delete_date': delete_date + } + + await self.bot.api_client.post('bot/offensive-messages', json=data) + self.schedule_task(self.bot.loop, msg.id, data) log.trace(f"Offensive message {msg.id} will be deleted on {delete_date}") if isinstance(msg.channel, DMChannel): -- cgit v1.2.3 From 0323919b08342fd650ff32e1f3a2fc2d9eee9c59 Mon Sep 17 00:00:00 2001 From: Matteo Bertucci Date: Sat, 29 Feb 2020 08:30:14 +0100 Subject: Make sure that the offensive message deletion date returned by the API is naive It could have caused an issue later with a mix of naive and aware datetime Co-Authored-By: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com> --- bot/cogs/filtering.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 83e706a26..2d91695f3 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -400,7 +400,7 @@ class Filtering(Cog, Scheduler): async def _scheduled_task(self, msg: dict) -> None: """Delete an offensive message once its deletion date is reached.""" - delete_at = dateutil.parser.isoparse(msg['delete_date']) + delete_at = dateutil.parser.isoparse(msg['delete_date']).replace(tzinfo=None) await wait_until(delete_at) await self.delete_offensive_msg(msg) -- cgit v1.2.3 From ae14de8745c40ebc9e09d785c55f0fd472dc50ae Mon Sep 17 00:00:00 2001 From: Matteo Bertucci Date: Mon, 2 Mar 2020 17:42:34 +0100 Subject: Remove task self cancel. Recent changes to the scheduler requires this line to be removed. --- bot/cogs/filtering.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 2d91695f3..6c9e9a4b7 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -405,8 +405,6 @@ class Filtering(Cog, Scheduler): await wait_until(delete_at) await self.delete_offensive_msg(msg) - self.cancel_task(msg['id']) - async def reschedule_offensive_msg_deletion(self) -> None: """Get all the pending message deletion from the API and reschedule them.""" await self.bot.wait_until_ready() -- cgit v1.2.3 From 0fc2ebbfeb6f6296f8eb74a7191785bfaa551f90 Mon Sep 17 00:00:00 2001 From: Matteo Bertucci Date: Mon, 2 Mar 2020 17:48:40 +0100 Subject: Delete the loop argument from schedule_task calls The function doesn't take the loop as an argument anymore --- bot/cogs/filtering.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 6c9e9a4b7..347554ae2 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -202,7 +202,7 @@ class Filtering(Cog, Scheduler): } await self.bot.api_client.post('bot/offensive-messages', json=data) - self.schedule_task(self.bot.loop, msg.id, data) + self.schedule_task(msg.id, data) log.trace(f"Offensive message {msg.id} will be deleted on {delete_date}") if isinstance(msg.channel, DMChannel): @@ -418,7 +418,7 @@ class Filtering(Cog, Scheduler): if delete_at < now: await self.delete_offensive_msg(msg) else: - self.schedule_task(self.bot.loop, msg['id'], msg) + self.schedule_task(msg['id'], msg) async def delete_offensive_msg(self, msg: Mapping[str, str]) -> None: """Delete an offensive message, and then delete it from the db.""" -- cgit v1.2.3 From b366d655af0e0f5a9ff3e053a693838d49884ea2 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 13:28:32 -0700 Subject: Token remover: catch ValueError when non-ASCII chars are present The token uses base64 and base64 only allows ASCII characters. Thus, if a match has non-ASCII characters, it's not a valid token. Catching the ValueError is simpler than trying to adjust the regex to only match valid base64. Fixes #928 Fixes BOT-3X --- bot/cogs/token_remover.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 6721f0e02..860ae9f3a 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -135,7 +135,7 @@ class TokenRemover(Cog): try: content: bytes = base64.b64decode(b64_content) return content.decode('utf-8').isnumeric() - except (binascii.Error, UnicodeDecodeError): + except (binascii.Error, ValueError): return False @staticmethod @@ -150,7 +150,7 @@ class TokenRemover(Cog): try: content = base64.urlsafe_b64decode(b64_content) snowflake = struct.unpack('i', content)[0] - except (binascii.Error, struct.error): + except (binascii.Error, struct.error, ValueError): return False return snowflake_time(snowflake + TOKEN_EPOCH) < DISCORD_EPOCH_TIMESTAMP -- cgit v1.2.3 From f03ae8e49bb3d62776528e6339d6c713c93b7674 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 14:08:02 -0700 Subject: Token remover: reduce duplicated code in `on_message_edit` --- bot/cogs/token_remover.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 860ae9f3a..e90d5ab8b 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -65,9 +65,7 @@ class TokenRemover(Cog): See: https://discordapp.com/developers/docs/reference#snowflakes """ - found_token = self.find_token_in_message(after) - if found_token: - await self.take_action(after, found_token) + await self.on_message(after) async def take_action(self, msg: Message, found_token: str) -> None: """Remove the `msg` containing a token an send a mod_log message.""" -- cgit v1.2.3 From d193a93828582965eb361dc6f3185291fff649a7 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 14:11:39 -0700 Subject: Test on_message_edit of token remover uses on_message --- tests/bot/cogs/test_token_remover.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 33d1ec170..e7b5a9bea 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -1,6 +1,7 @@ import asyncio import logging import unittest +from unittest import mock from unittest.mock import AsyncMock, MagicMock from discord import Colour @@ -14,7 +15,7 @@ from bot.constants import Channels, Colours, Event, Icons from tests.helpers import MockBot, MockMessage -class TokenRemoverTests(unittest.TestCase): +class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): """Tests the `TokenRemover` cog.""" def setUp(self): @@ -58,6 +59,13 @@ class TokenRemoverTests(unittest.TestCase): self.assertEqual(self.cog.mod_log, self.bot.get_cog.return_value) self.bot.get_cog.assert_called_once_with('ModLog') + async def test_on_message_edit_uses_on_message(self): + """The edit listener should delegate handling of the message to the normal listener.""" + self.cog.on_message = mock.create_autospec(self.cog.on_message, spec_set=True) + + await self.cog.on_message_edit(MockMessage(), self.msg) + self.cog.on_message.assert_awaited_once_with(self.msg) + def test_ignores_bot_messages(self): """When the message event handler is called with a bot message, nothing is done.""" self.msg.author.bot = True @@ -77,7 +85,7 @@ class TokenRemoverTests(unittest.TestCase): for content in ('foo.bar.baz', 'x.y.'): with self.subTest(content=content): self.msg.content = content - coroutine = self.cog.on_message(self.msg) + coroutine = self.cog.is_maybe_token(self.msg) self.assertIsNone(asyncio.run(coroutine)) def test_censors_valid_tokens(self): -- cgit v1.2.3 From 0bfd003dbfc5919220129f984dc043421e535f8c Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 14:38:12 -0700 Subject: Add a test helper function to patch multiple attributes with autospecs This helper reduces redundancy/boilerplate by setting default values. It also has the consequence of shortening the length of the invocation, which makes it faster to use and easier to read. --- tests/helpers.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/helpers.py b/tests/helpers.py index 2b79a6c2a..d444cc49d 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -23,6 +23,15 @@ for logger in logging.Logger.manager.loggerDict.values(): logger.setLevel(logging.CRITICAL) +def autospec(target, *attributes: str, **kwargs) -> unittest.mock._patch: + """Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True.""" + # Caller's kwargs should take priority and overwrite the defaults. + kwargs = {'spec_set': True, 'autospec': True, **kwargs} + attributes = {attribute: unittest.mock.DEFAULT for attribute in attributes} + + return unittest.mock.patch.multiple(target, **attributes, **kwargs) + + class HashableMixin(discord.mixins.EqualityComparable): """ Mixin that provides similar hashing and equality functionality as discord.py's `Hashable` mixin. -- cgit v1.2.3 From e8bd69a6c556d78eca1a1eb2adfa26248273a1cd Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 14:42:07 -0700 Subject: Test token remover takes action if a token is found --- tests/bot/cogs/test_token_remover.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index e7b5a9bea..e0ec67684 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -12,7 +12,7 @@ from bot.cogs.token_remover import ( setup as setup_cog, ) from bot.constants import Channels, Colours, Event, Icons -from tests.helpers import MockBot, MockMessage +from tests.helpers import MockBot, MockMessage, autospec class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @@ -66,6 +66,18 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): await self.cog.on_message_edit(MockMessage(), self.msg) self.cog.on_message.assert_awaited_once_with(self.msg) + @autospec(TokenRemover, "find_token_in_message", "take_action") + async def test_on_message_takes_action(self, find_token_in_message, take_action): + """Should take action if a valid token is found when a message is sent.""" + cog = TokenRemover(self.bot) + found_token = "foobar" + find_token_in_message.return_value = found_token + + await cog.on_message(self.msg) + + find_token_in_message.assert_called_once_with(self.msg) + take_action.assert_awaited_once_with(cog, self.msg, found_token) + def test_ignores_bot_messages(self): """When the message event handler is called with a bot message, nothing is done.""" self.msg.author.bot = True -- cgit v1.2.3 From 4cf7996a1d4630ccb05f57569ca62b1798dc7a93 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 14:44:54 -0700 Subject: Test token remover skips messages without tokens --- tests/bot/cogs/test_token_remover.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index e0ec67684..2b377e221 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -78,6 +78,17 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): find_token_in_message.assert_called_once_with(self.msg) take_action.assert_awaited_once_with(cog, self.msg, found_token) + @autospec(TokenRemover, "find_token_in_message", "take_action") + async def test_on_message_skips_missing_token(self, find_token_in_message, take_action): + """Shouldn't take action if a valid token isn't found when a message is sent.""" + cog = TokenRemover(self.bot) + find_token_in_message.return_value = False + + await cog.on_message(self.msg) + + find_token_in_message.assert_called_once_with(self.msg) + take_action.assert_not_awaited() + def test_ignores_bot_messages(self): """When the message event handler is called with a bot message, nothing is done.""" self.msg.author.bot = True -- cgit v1.2.3 From 593e09299c6e4115d41bfd5b074785a5e304a8d0 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 15:41:14 -0700 Subject: Allow using arbitrary parameter names with the autospec decorator This gives the caller more flexibility. Sometimes attribute names are too long or they don't follow a naming scheme accepted by the linter. --- tests/helpers.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/tests/helpers.py b/tests/helpers.py index d444cc49d..1ab8b455f 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -24,12 +24,25 @@ for logger in logging.Logger.manager.loggerDict.values(): def autospec(target, *attributes: str, **kwargs) -> unittest.mock._patch: - """Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True.""" + """ + Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True. + + To allow for arbitrary parameter names to be used by the decorated function, the patchers have + no attribute names associated with them. As a consequence, it will not be possible to retrieve + mocks by their attribute names when using this as a context manager, + """ # Caller's kwargs should take priority and overwrite the defaults. kwargs = {'spec_set': True, 'autospec': True, **kwargs} attributes = {attribute: unittest.mock.DEFAULT for attribute in attributes} - return unittest.mock.patch.multiple(target, **attributes, **kwargs) + patcher = unittest.mock.patch.multiple(target, **attributes, **kwargs) + + # Unset attribute names to allow arbitrary parameter names for the decorator function. + patcher.attribute_name = None + for additional_patcher in patcher.additional_patchers: + additional_patcher.attribute_name = None + + return patcher class HashableMixin(discord.mixins.EqualityComparable): -- cgit v1.2.3 From b0dd290710799c342240d066abaebbe9e6940b54 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 15:09:22 -0700 Subject: Fix test for token remover ignoring bot messages It's not possible to test this via asserting the return value of `on_message` since it never returns anything. Instead, the actual relevant unit, `find_token_in_message,` should be tested. --- tests/bot/cogs/test_token_remover.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 2b377e221..e8b641101 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -89,11 +89,16 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): find_token_in_message.assert_called_once_with(self.msg) take_action.assert_not_awaited() - def test_ignores_bot_messages(self): - """When the message event handler is called with a bot message, nothing is done.""" + @autospec("bot.cogs.token_remover", "TOKEN_RE") + def test_find_token_ignores_bot_messages(self, token_re): + """The token finder should ignore messages authored by bots.""" + cog = TokenRemover(self.bot) self.msg.author.bot = True - coroutine = self.cog.on_message(self.msg) - self.assertIsNone(asyncio.run(coroutine)) + + return_value = cog.find_token_in_message(self.msg) + + self.assertIsNone(return_value) + token_re.findall.assert_not_called() def test_ignores_messages_without_tokens(self): """Messages without anything looking like a token are ignored.""" -- cgit v1.2.3 From 52f0f8a29d7f239c961beaa81881bf4b09da4749 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 15:53:06 -0700 Subject: Test `find_token_in_message` returns None if no matches found --- tests/bot/cogs/test_token_remover.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index e8b641101..5932cf4f0 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -100,6 +100,20 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.assertIsNone(return_value) token_re.findall.assert_not_called() + @autospec(TokenRemover, "is_maybe_token") + @autospec("bot.cogs.token_remover", "TOKEN_RE") + def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): + """None should be returned if the regex matches no tokens in a message.""" + cog = TokenRemover(self.bot) + token_re.findall.return_value = () + self.msg.content = "foobar" + + return_value = cog.find_token_in_message(self.msg) + + self.assertIsNone(return_value) + token_re.findall.assert_called_once_with(self.msg.content) + is_maybe_token.assert_not_called() + def test_ignores_messages_without_tokens(self): """Messages without anything looking like a token are ignored.""" for content in ('', 'lemon wins'): -- cgit v1.2.3 From cf658bd58559b2683527443f2908257f197ef0bb Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 16:06:47 -0700 Subject: Test `find_token_in_message` returns the found token --- tests/bot/cogs/test_token_remover.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 5932cf4f0..2b946778b 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -114,6 +114,30 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): token_re.findall.assert_called_once_with(self.msg.content) is_maybe_token.assert_not_called() + @autospec(TokenRemover, "is_maybe_token") + @autospec("bot.cogs.token_remover", "TOKEN_RE") + def test_find_token_returns_found_token(self, token_re, is_maybe_token): + """The found token should be returned.""" + true_index = 1 + matches = ("foo", "bar", "baz") + side_effects = [False] * len(matches) + side_effects[true_index] = True + + cog = TokenRemover(self.bot) + self.msg.content = "foobar" + token_re.findall.return_value = matches + is_maybe_token.side_effect = side_effects + + return_value = cog.find_token_in_message(self.msg) + + self.assertEqual(return_value, matches[true_index]) + token_re.findall.assert_called_once_with(self.msg.content) + + # assert_has_calls isn't used cause it'd allow for extra calls before or after. + # The function should short-circuit, so nothing past true_index should have been used. + calls = [mock.call(match) for match in matches[:true_index + 1]] + self.assertEqual(is_maybe_token.mock_calls, calls) + def test_ignores_messages_without_tokens(self): """Messages without anything looking like a token are ignored.""" for content in ('', 'lemon wins'): -- cgit v1.2.3 From f92bc80d6bddb5c57c190187adaa528ae44536f6 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 16:25:14 -0700 Subject: Test token regex doesn't match invalid tokens --- tests/bot/cogs/test_token_remover.py | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 2b946778b..b67602eb9 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -8,6 +8,7 @@ from discord import Colour from bot.cogs.token_remover import ( DELETION_MESSAGE_TEMPLATE, + TOKEN_RE, TokenRemover, setup as setup_cog, ) @@ -138,13 +139,30 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): calls = [mock.call(match) for match in matches[:true_index + 1]] self.assertEqual(is_maybe_token.mock_calls, calls) - def test_ignores_messages_without_tokens(self): - """Messages without anything looking like a token are ignored.""" - for content in ('', 'lemon wins'): - with self.subTest(content=content): - self.msg.content = content - coroutine = self.cog.on_message(self.msg) - self.assertIsNone(asyncio.run(coroutine)) + def test_regex_invalid_tokens(self): + """Messages without anything looking like a token are not matched.""" + tokens = ( + "", + "lemon wins", + "..", + "x.y", + "x.y.", + ".y.z", + ".y.", + "..z", + "x..z", + " . . ", + "\n.\n.\n", + "'.'.'", + '"."."', + "(.(.(", + ").).)" + ) + + for token in tokens: + with self.subTest(token=token): + results = TOKEN_RE.findall(token) + self.assertEquals(len(results), 0) def test_ignores_messages_with_invalid_tokens(self): """Messages with values that are invalid tokens are ignored.""" -- cgit v1.2.3 From 34b836a8eba0f006c77a7b3f48f7ab14c37d31ee Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 17:47:09 -0700 Subject: Fix autospec decorator when used with multiple attributes The original approach of messing with the `attribute_name` didn't work for reasons I won't discuss here (would require knowledge of patcher internals). The new approach doesn't use patch.multiple but mimics it by applying multiple patch decorators to the function. As a consequence, this can no longer be used as a context manager. --- tests/helpers.py | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/tests/helpers.py b/tests/helpers.py index 1ab8b455f..dfbe539ec 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -24,25 +24,21 @@ for logger in logging.Logger.manager.loggerDict.values(): def autospec(target, *attributes: str, **kwargs) -> unittest.mock._patch: - """ - Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True. - - To allow for arbitrary parameter names to be used by the decorated function, the patchers have - no attribute names associated with them. As a consequence, it will not be possible to retrieve - mocks by their attribute names when using this as a context manager, - """ + """Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True.""" # Caller's kwargs should take priority and overwrite the defaults. kwargs = {'spec_set': True, 'autospec': True, **kwargs} - attributes = {attribute: unittest.mock.DEFAULT for attribute in attributes} - - patcher = unittest.mock.patch.multiple(target, **attributes, **kwargs) - - # Unset attribute names to allow arbitrary parameter names for the decorator function. - patcher.attribute_name = None - for additional_patcher in patcher.additional_patchers: - additional_patcher.attribute_name = None - return patcher + # Import the target if it's a string. + # This is to support both object and string targets like patch.multiple. + if type(target) is str: + target = unittest.mock._importer(target) + + def decorator(func): + for attribute in attributes: + patcher = unittest.mock.patch.object(target, attribute, **kwargs) + func = patcher(func) + return func + return decorator class HashableMixin(discord.mixins.EqualityComparable): -- cgit v1.2.3 From 834bd543d1d301bb853e713560a7447dc75f1ab8 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 17:53:40 -0700 Subject: Test `is_maybe_token` returns False for missing parts In practice, this won't ever happen since the regex wouldn't match strings with missing parts. However, the function does check it so may as well test it. It's not necessarily bound to always use inputs from the regex either I suppose. --- tests/bot/cogs/test_token_remover.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index b67602eb9..9e1d96a37 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -164,6 +164,16 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): results = TOKEN_RE.findall(token) self.assertEquals(len(results), 0) + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") + def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): + """False should be returned for tokens which do not have all 3 parts.""" + cog = TokenRemover(self.bot) + return_value = cog.is_maybe_token("x.y") + + self.assertFalse(return_value) + valid_user.assert_not_called() + valid_time.assert_not_called() + def test_ignores_messages_with_invalid_tokens(self): """Messages with values that are invalid tokens are ignored.""" for content in ('foo.bar.baz', 'x.y.'): -- cgit v1.2.3 From 4248f88a7407b6e9a5d80800a96f8707003634d3 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 18:07:17 -0700 Subject: Token remover: fix `is_maybe_token` returning None instead of False It's annotated as returning a bool and when the split fails it already returns False. To be consistent, it should always return a bool. --- bot/cogs/token_remover.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index e90d5ab8b..543f4c5a7 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -121,6 +121,8 @@ class TokenRemover(Cog): if cls.is_valid_user_id(user_id) and cls.is_valid_timestamp(creation_timestamp): return True + return False + @staticmethod def is_valid_user_id(b64_content: str) -> bool: """ -- cgit v1.2.3 From ab5d194b90a7e068c8ab7171939f471e252ee073 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 18:11:31 -0700 Subject: Test is_maybe_token --- tests/bot/cogs/test_token_remover.py | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 9e1d96a37..85bbbdf6b 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -174,13 +174,30 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): valid_user.assert_not_called() valid_time.assert_not_called() - def test_ignores_messages_with_invalid_tokens(self): - """Messages with values that are invalid tokens are ignored.""" - for content in ('foo.bar.baz', 'x.y.'): - with self.subTest(content=content): - self.msg.content = content - coroutine = self.cog.is_maybe_token(self.msg) - self.assertIsNone(asyncio.run(coroutine)) + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") + def test_is_maybe_token(self, valid_user, valid_time): + """Should return True if the user ID and timestamp are valid or return False otherwise.""" + cog = TokenRemover(self.bot) + subtests = ( + (False, True, False), + (True, False, False), + (True, True, True), + ) + + for user_return, time_return, expected in subtests: + valid_user.reset_mock() + valid_time.reset_mock() + + with self.subTest(user_return=user_return, time_return=time_return, expected=expected): + valid_user.return_value = user_return + valid_time.return_value = time_return + + actual = cog.is_maybe_token("x.y.z") + self.assertIs(actual, expected) + + valid_user.assert_called_once_with("x") + if user_return: + valid_time.assert_called_once_with("y") def test_censors_valid_tokens(self): """Valid tokens are censored.""" -- cgit v1.2.3 From 4b6fde69a7e193382701dccf80a5471ea7ccea72 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 18:22:31 -0700 Subject: Test token regex matches valid tokens --- tests/bot/cogs/test_token_remover.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 85bbbdf6b..7310b4637 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -164,6 +164,27 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): results = TOKEN_RE.findall(token) self.assertEquals(len(results), 0) + def test_regex_valid_tokens(self): + """Messages that look like tokens should be matched.""" + # Don't worry, the token's been invalidated. + tokens = ( + "x1.y2.z_3", + "NDcyMjY1OTQzMDYyNDEzMzMy.Xrim9Q.Ysnu2wacjaKs7qnoo46S8Dm2us8" + ) + + for token in tokens: + with self.subTest(token=token): + results = TOKEN_RE.findall(token) + self.assertIn(token, results) + + def test_regex_matches_multiple_valid(self): + """Should support multiple matches in the middle of a string.""" + tokens = ["x.y.z", "a.b.c"] + message = f"garbage {tokens[0]} hello {tokens[1]} world" + + results = TOKEN_RE.findall(message) + self.assertEquals(tokens, results) + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): """False should be returned for tokens which do not have all 3 parts.""" -- cgit v1.2.3 From d8d8e144adfe4c2de15dbbf4346e2eec548a9f67 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 18:28:06 -0700 Subject: Correct the return type annotation for the autospec decorator --- tests/helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/helpers.py b/tests/helpers.py index dfbe539ec..3cd8a63c0 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -4,7 +4,7 @@ import collections import itertools import logging import unittest.mock -from typing import Iterable, Optional +from typing import Callable, Iterable, Optional import discord from discord.ext.commands import Context @@ -23,7 +23,7 @@ for logger in logging.Logger.manager.loggerDict.values(): logger.setLevel(logging.CRITICAL) -def autospec(target, *attributes: str, **kwargs) -> unittest.mock._patch: +def autospec(target, *attributes: str, **kwargs) -> Callable: """Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True.""" # Caller's kwargs should take priority and overwrite the defaults. kwargs = {'spec_set': True, 'autospec': True, **kwargs} -- cgit v1.2.3 From ab860e23a7e6206e68cb350257b63083cfbe1a15 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 18:53:42 -0700 Subject: Token remover: split some of `take_action` into separate functions --- bot/cogs/token_remover.py | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 543f4c5a7..d6919839e 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -68,31 +68,41 @@ class TokenRemover(Cog): await self.on_message(after) async def take_action(self, msg: Message, found_token: str) -> None: - """Remove the `msg` containing a token an send a mod_log message.""" - user_id, creation_timestamp, hmac = found_token.split('.') + """Remove the `msg` containing the `found_token` and send a mod log message.""" self.mod_log.ignore(Event.message_delete, msg.id) - await msg.delete() - await msg.channel.send(DELETION_MESSAGE_TEMPLATE.format(mention=msg.author.mention)) + await self.delete_message(msg) - message = ( - "Censored a seemingly valid token sent by " - f"{msg.author} (`{msg.author.id}`) in {msg.channel.mention}, token was " - f"`{user_id}.{creation_timestamp}.{'x' * len(hmac)}`" - ) - log.debug(message) + log_message = self.format_log_message(msg, found_token) + log.debug(log_message) # Send pretty mod log embed to mod-alerts await self.mod_log.send_log_message( icon_url=Icons.token_removed, colour=Colour(Colours.soft_red), title="Token removed!", - text=message, + text=log_message, thumbnail=msg.author.avatar_url_as(static_format="png"), channel_id=Channels.mod_alerts, ) self.bot.stats.incr("tokens.removed_tokens") + @staticmethod + async def delete_message(msg: Message) -> None: + """Remove a `msg` containing a token and send an explanatory message in the same channel.""" + await msg.delete() + await msg.channel.send(DELETION_MESSAGE_TEMPLATE.format(mention=msg.author.mention)) + + @staticmethod + def format_log_message(msg: Message, found_token: str) -> str: + """Return the log message to send for `found_token` being censored in `msg`.""" + user_id, creation_timestamp, hmac = found_token.split('.') + return ( + "Censored a seemingly valid token sent by " + f"{msg.author} (`{msg.author.id}`) in {msg.channel.mention}, token was " + f"`{user_id}.{creation_timestamp}.{'x' * len(hmac)}`" + ) + @classmethod def find_token_in_message(cls, msg: Message) -> t.Optional[str]: """Return a seemingly valid token found in `msg` or `None` if no token is found.""" -- cgit v1.2.3 From 09a6c2e211c0f209b258a02d9677240282c4fab3 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 18:55:24 -0700 Subject: Token remover: use a string template for the log message --- bot/cogs/token_remover.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index d6919839e..c576a67d0 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -16,6 +16,10 @@ from bot.constants import Channels, Colours, Event, Icons log = logging.getLogger(__name__) +LOG_MESSAGE = ( + "Censored a seemingly valid token sent by {author} (`{author_id}`) in {channel}," + "token was `{user_id}.{timestamp}.{hmac}`" +) DELETION_MESSAGE_TEMPLATE = ( "Hey {mention}! I noticed you posted a seemingly valid Discord API " "token in your message and have removed your message. " @@ -97,10 +101,13 @@ class TokenRemover(Cog): def format_log_message(msg: Message, found_token: str) -> str: """Return the log message to send for `found_token` being censored in `msg`.""" user_id, creation_timestamp, hmac = found_token.split('.') - return ( - "Censored a seemingly valid token sent by " - f"{msg.author} (`{msg.author.id}`) in {msg.channel.mention}, token was " - f"`{user_id}.{creation_timestamp}.{'x' * len(hmac)}`" + return LOG_MESSAGE.format( + author=msg.author, + author_id=msg.author.id, + channel=msg.channel.mention, + user_id=user_id, + timestamp=creation_timestamp, + hmac='x' * len(hmac), ) @classmethod -- cgit v1.2.3 From 5b9bf9aba686f570322cb9996dd35d3ab669a162 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 10:26:16 -0700 Subject: Avoid instantiating the cog when testing static/class methods --- tests/bot/cogs/test_token_remover.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 7310b4637..6a8247070 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -93,10 +93,9 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec("bot.cogs.token_remover", "TOKEN_RE") def test_find_token_ignores_bot_messages(self, token_re): """The token finder should ignore messages authored by bots.""" - cog = TokenRemover(self.bot) self.msg.author.bot = True - return_value = cog.find_token_in_message(self.msg) + return_value = TokenRemover.find_token_in_message(self.msg) self.assertIsNone(return_value) token_re.findall.assert_not_called() @@ -105,11 +104,10 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec("bot.cogs.token_remover", "TOKEN_RE") def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): """None should be returned if the regex matches no tokens in a message.""" - cog = TokenRemover(self.bot) token_re.findall.return_value = () self.msg.content = "foobar" - return_value = cog.find_token_in_message(self.msg) + return_value = TokenRemover.find_token_in_message(self.msg) self.assertIsNone(return_value) token_re.findall.assert_called_once_with(self.msg.content) @@ -124,12 +122,11 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): side_effects = [False] * len(matches) side_effects[true_index] = True - cog = TokenRemover(self.bot) self.msg.content = "foobar" token_re.findall.return_value = matches is_maybe_token.side_effect = side_effects - return_value = cog.find_token_in_message(self.msg) + return_value = TokenRemover.find_token_in_message(self.msg) self.assertEqual(return_value, matches[true_index]) token_re.findall.assert_called_once_with(self.msg.content) @@ -188,8 +185,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): """False should be returned for tokens which do not have all 3 parts.""" - cog = TokenRemover(self.bot) - return_value = cog.is_maybe_token("x.y") + return_value = TokenRemover.is_maybe_token("x.y") self.assertFalse(return_value) valid_user.assert_not_called() @@ -198,7 +194,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token(self, valid_user, valid_time): """Should return True if the user ID and timestamp are valid or return False otherwise.""" - cog = TokenRemover(self.bot) subtests = ( (False, True, False), (True, False, False), @@ -213,7 +208,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): valid_user.return_value = user_return valid_time.return_value = time_return - actual = cog.is_maybe_token("x.y.z") + actual = TokenRemover.is_maybe_token("x.y.z") self.assertIs(actual, expected) valid_user.assert_called_once_with("x") -- cgit v1.2.3 From 2127239840085ba523d411899e0b7a188530df07 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 10:33:05 -0700 Subject: Simplify token remover's message mock * Rely on default values for the author * Set the content to a non-empty string --- tests/bot/cogs/test_token_remover.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 6a8247070..5ca863926 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -26,14 +26,10 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.bot.get_cog.return_value.send_log_message = AsyncMock() self.cog = TokenRemover(bot=self.bot) - self.msg = MockMessage(id=555, content='') - self.msg.author.__str__ = MagicMock() - self.msg.author.__str__.return_value = 'lemon' - self.msg.author.bot = False - self.msg.author.avatar_url_as.return_value = 'picture-lemon.png' - self.msg.author.id = 42 - self.msg.author.mention = '@lemon' + self.msg = MockMessage(id=555, content="hello world") self.msg.channel.mention = "#lemonade-stand" + self.msg.author.__str__ = MagicMock(return_value=self.msg.author.name) + self.msg.author.avatar_url_as.return_value = "picture-lemon.png" def test_is_valid_user_id_is_true_for_numeric_content(self): """A string decoding to numeric characters is a valid user ID.""" @@ -105,7 +101,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): """None should be returned if the regex matches no tokens in a message.""" token_re.findall.return_value = () - self.msg.content = "foobar" return_value = TokenRemover.find_token_in_message(self.msg) @@ -122,7 +117,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): side_effects = [False] * len(matches) side_effects[true_index] = True - self.msg.content = "foobar" token_re.findall.return_value = matches is_maybe_token.side_effect = side_effects -- cgit v1.2.3 From e4790b330da1605573b5d23615bfe62b481e1e04 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 10:37:59 -0700 Subject: Test token remover's message deletion --- tests/bot/cogs/test_token_remover.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 5ca863926..d65ce2ce5 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -209,6 +209,15 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): if user_return: valid_time.assert_called_once_with("y") + async def test_delete_message(self): + """The message should be deleted, and a message should be sent to the same channel.""" + await TokenRemover.delete_message(self.msg) + + self.msg.delete.assert_called_once_with() + self.msg.channel.send.assert_called_once_with( + DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) + ) + def test_censors_valid_tokens(self): """Valid tokens are censored.""" cases = ( -- cgit v1.2.3 From 567a5f9242912d6a3340c088c0ae1a62977a141e Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 10:46:02 -0700 Subject: Test TokenRemover.format_log_message --- tests/bot/cogs/test_token_remover.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index d65ce2ce5..f5412e692 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -218,6 +218,22 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) ) + @autospec("bot.cogs.token_remover", "LOG_MESSAGE") + async def test_format_log_message(self, log_message): + """Should correctly format the log message with info from the message and token.""" + log_message.format.return_value = "Howdy" + return_value = TokenRemover.format_log_message(self.msg, "MTIz.DN9R_A.xyz") + + self.assertEqual(return_value, log_message.format.return_value) + log_message.format.assert_called_once_with( + author=self.msg.author, + author_id=self.msg.author.id, + channel=self.msg.channel.mention, + user_id="MTIz", + timestamp="DN9R_A", + hmac="xxx", + ) + def test_censors_valid_tokens(self): """Valid tokens are censored.""" cases = ( -- cgit v1.2.3 From f47cbef0b47ef11b8c1fd63076105e4cb7d73601 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:29:28 -0700 Subject: Test TokenRemover.take_action * Remove `bot.get_cog` mocks in `setUp` * Mock the logger cause it's easier to assert logs * Remove subtests * Assert helper functions were called * Create an autospec for ModLog --- tests/bot/cogs/test_token_remover.py | 73 +++++++++++++++--------------------- 1 file changed, 30 insertions(+), 43 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index f5412e692..3546e7964 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -1,11 +1,10 @@ -import asyncio -import logging import unittest from unittest import mock -from unittest.mock import AsyncMock, MagicMock +from unittest.mock import MagicMock from discord import Colour +from bot.cogs.moderation import ModLog from bot.cogs.token_remover import ( DELETION_MESSAGE_TEMPLATE, TOKEN_RE, @@ -22,8 +21,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): def setUp(self): """Adds the cog, a bot, and a message to the instance for usage in tests.""" self.bot = MockBot() - self.bot.get_cog.return_value = MagicMock() - self.bot.get_cog.return_value.send_log_message = AsyncMock() self.cog = TokenRemover(bot=self.bot) self.msg = MockMessage(id=555, content="hello world") @@ -234,46 +231,36 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): hmac="xxx", ) - def test_censors_valid_tokens(self): - """Valid tokens are censored.""" - cases = ( - # (content, censored_token) - ('MTIz.DN9R_A.xyz', 'MTIz.DN9R_A.xxx'), + @mock.patch.object(TokenRemover, "mod_log", new_callable=mock.PropertyMock) + @autospec("bot.cogs.token_remover", "log") + @autospec(TokenRemover, "delete_message", "format_log_message") + async def test_take_action(self, delete_message, format_log_message, logger, mod_log_property): + """Should delete the message and send a mod log.""" + cog = TokenRemover(self.bot) + mod_log = mock.create_autospec(ModLog, spec_set=True, instance=True) + token = "MTIz.DN9R_A.xyz" + log_msg = "testing123" + + mod_log_property.return_value = mod_log + format_log_message.return_value = log_msg + + await cog.take_action(self.msg, token) + + delete_message.assert_awaited_once_with(self.msg) + format_log_message.assert_called_once_with(self.msg, token) + logger.debug.assert_called_with(log_msg) + self.bot.stats.incr.assert_called_once_with("tokens.removed_tokens") + + mod_log.ignore.assert_called_once_with(Event.message_delete, self.msg.id) + mod_log.send_log_message.assert_called_once_with( + icon_url=Icons.token_removed, + colour=Colour(Colours.soft_red), + title="Token removed!", + text=log_msg, + thumbnail=self.msg.author.avatar_url_as.return_value, + channel_id=Channels.mod_alerts ) - for content, censored_token in cases: - with self.subTest(content=content, censored_token=censored_token): - self.msg.content = content - coroutine = self.cog.on_message(self.msg) - with self.assertLogs(logger='bot.cogs.token_remover', level=logging.DEBUG) as cm: - self.assertIsNone(asyncio.run(coroutine)) # no return value - - [line] = cm.output - log_message = ( - "Censored a seemingly valid token sent by " - "lemon (`42`) in #lemonade-stand, " - f"token was `{censored_token}`" - ) - self.assertIn(log_message, line) - - self.msg.delete.assert_called_once_with() - self.msg.channel.send.assert_called_once_with( - DELETION_MESSAGE_TEMPLATE.format(mention='@lemon') - ) - self.bot.get_cog.assert_called_with('ModLog') - self.msg.author.avatar_url_as.assert_called_once_with(static_format='png') - - mod_log = self.bot.get_cog.return_value - mod_log.ignore.assert_called_once_with(Event.message_delete, self.msg.id) - mod_log.send_log_message.assert_called_once_with( - icon_url=Icons.token_removed, - colour=Colour(Colours.soft_red), - title="Token removed!", - text=log_message, - thumbnail='picture-lemon.png', - channel_id=Channels.mod_alerts - ) - class TokenRemoverSetupTests(unittest.TestCase): """Tests setup of the `TokenRemover` cog.""" -- cgit v1.2.3 From 5734a4d84922a9497014dfeb3eba31ad3c57536f Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:44:08 -0700 Subject: Refactor `TokenRemoverSetupTests` and add a more thorough test The test now ensures the cog is instantiated and that the instance is passed as an argument to `add_cog`. --- tests/bot/cogs/test_token_remover.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 3546e7964..c377de7b2 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -262,11 +262,15 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): ) -class TokenRemoverSetupTests(unittest.TestCase): - """Tests setup of the `TokenRemover` cog.""" +class TokenRemoverExtensionTests(unittest.TestCase): + """Tests for the token_remover extension.""" - def test_setup(self): - """Setup of the extension should call add_cog.""" + @autospec("bot.cogs.token_remover", "TokenRemover") + def test_extension_setup(self, cog): + """The TokenRemover cog should be added.""" bot = MockBot() setup_cog(bot) + + cog.assert_called_once_with(bot) bot.add_cog.assert_called_once() + self.assertTrue(isinstance(bot.add_cog.call_args.args[0], TokenRemover)) -- cgit v1.2.3 From d0303d715d485842a2d5c906099d767d74cf8bd8 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:45:50 -0700 Subject: Replace deprecated assertion methods --- tests/bot/cogs/test_token_remover.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index c377de7b2..aecb51403 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -150,7 +150,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): for token in tokens: with self.subTest(token=token): results = TOKEN_RE.findall(token) - self.assertEquals(len(results), 0) + self.assertEqual(len(results), 0) def test_regex_valid_tokens(self): """Messages that look like tokens should be matched.""" @@ -171,7 +171,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): message = f"garbage {tokens[0]} hello {tokens[1]} world" results = TOKEN_RE.findall(message) - self.assertEquals(tokens, results) + self.assertEqual(tokens, results) @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): -- cgit v1.2.3 From 862153f2e4ab5b1408719fb2c1abc5143cfb15ce Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:47:40 -0700 Subject: Clean up token remover test imports --- tests/bot/cogs/test_token_remover.py | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index aecb51403..5cc8c7ad1 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -4,14 +4,10 @@ from unittest.mock import MagicMock from discord import Colour +from bot import constants +from bot.cogs import token_remover from bot.cogs.moderation import ModLog -from bot.cogs.token_remover import ( - DELETION_MESSAGE_TEMPLATE, - TOKEN_RE, - TokenRemover, - setup as setup_cog, -) -from bot.constants import Channels, Colours, Event, Icons +from bot.cogs.token_remover import TokenRemover from tests.helpers import MockBot, MockMessage, autospec @@ -149,7 +145,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): for token in tokens: with self.subTest(token=token): - results = TOKEN_RE.findall(token) + results = token_remover.TOKEN_RE.findall(token) self.assertEqual(len(results), 0) def test_regex_valid_tokens(self): @@ -162,7 +158,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): for token in tokens: with self.subTest(token=token): - results = TOKEN_RE.findall(token) + results = token_remover.TOKEN_RE.findall(token) self.assertIn(token, results) def test_regex_matches_multiple_valid(self): @@ -170,7 +166,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): tokens = ["x.y.z", "a.b.c"] message = f"garbage {tokens[0]} hello {tokens[1]} world" - results = TOKEN_RE.findall(message) + results = token_remover.TOKEN_RE.findall(message) self.assertEqual(tokens, results) @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") @@ -212,7 +208,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.msg.delete.assert_called_once_with() self.msg.channel.send.assert_called_once_with( - DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) + token_remover.DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) ) @autospec("bot.cogs.token_remover", "LOG_MESSAGE") @@ -251,14 +247,14 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): logger.debug.assert_called_with(log_msg) self.bot.stats.incr.assert_called_once_with("tokens.removed_tokens") - mod_log.ignore.assert_called_once_with(Event.message_delete, self.msg.id) + mod_log.ignore.assert_called_once_with(constants.Event.message_delete, self.msg.id) mod_log.send_log_message.assert_called_once_with( - icon_url=Icons.token_removed, - colour=Colour(Colours.soft_red), + icon_url=constants.Icons.token_removed, + colour=Colour(constants.Colours.soft_red), title="Token removed!", text=log_msg, thumbnail=self.msg.author.avatar_url_as.return_value, - channel_id=Channels.mod_alerts + channel_id=constants.Channels.mod_alerts ) @@ -269,7 +265,7 @@ class TokenRemoverExtensionTests(unittest.TestCase): def test_extension_setup(self, cog): """The TokenRemover cog should be added.""" bot = MockBot() - setup_cog(bot) + token_remover.setup(bot) cog.assert_called_once_with(bot) bot.add_cog.assert_called_once() -- cgit v1.2.3 From 4701b0da36c7f42792c0af258b785076237fd661 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:56:15 -0700 Subject: Use subtests for valid ID/timestamp tests and test non-ASCII inputs --- tests/bot/cogs/test_token_remover.py | 43 +++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 5cc8c7ad1..f1a56c235 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -24,24 +24,31 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.msg.author.__str__ = MagicMock(return_value=self.msg.author.name) self.msg.author.avatar_url_as.return_value = "picture-lemon.png" - def test_is_valid_user_id_is_true_for_numeric_content(self): - """A string decoding to numeric characters is a valid user ID.""" - # MTIz = base64(123) - self.assertTrue(TokenRemover.is_valid_user_id('MTIz')) - - def test_is_valid_user_id_is_false_for_alphabetic_content(self): - """A string decoding to alphabetic characters is not a valid user ID.""" - # YWJj = base64(abc) - self.assertFalse(TokenRemover.is_valid_user_id('YWJj')) - - def test_is_valid_timestamp_is_true_for_valid_timestamps(self): - """A string decoding to a valid timestamp should be recognized as such.""" - self.assertTrue(TokenRemover.is_valid_timestamp('DN9r_A')) - - def test_is_valid_timestamp_is_false_for_invalid_values(self): - """A string not decoding to a valid timestamp should not be recognized as such.""" - # MTIz = base64(123) - self.assertFalse(TokenRemover.is_valid_timestamp('MTIz')) + def test_is_valid_user_id(self): + """Should correctly discern valid user IDs and ignore non-numeric and non-ASCII IDs.""" + subtests = ( + ("MTIz", True), # base64(123) + ("YWJj", False), # base64(abc) + ("λδµ", False), + ) + + for user_id, is_valid in subtests: + with self.subTest(user_id=user_id, is_valid=is_valid): + result = TokenRemover.is_valid_user_id(user_id) + self.assertIs(result, is_valid) + + def test_is_valid_timestamp(self): + """Should correctly discern valid timestamps.""" + subtests = ( + ("DN9r_A", True), + ("MTIz", False), # base64(123) + ("λδµ", False), + ) + + for timestamp, is_valid in subtests: + with self.subTest(timestamp=timestamp, is_valid=is_valid): + result = TokenRemover.is_valid_timestamp(timestamp) + self.assertIs(result, is_valid) def test_mod_log_property(self): """The `mod_log` property should ask the bot to return the `ModLog` cog.""" -- cgit v1.2.3 From 31aff51655d3783bc70f04628f189cf3c3591028 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 13 May 2020 18:58:43 -0700 Subject: Fix a test needlessly being a coroutine --- tests/bot/cogs/test_token_remover.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index f1a56c235..8e743a715 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -219,7 +219,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): ) @autospec("bot.cogs.token_remover", "LOG_MESSAGE") - async def test_format_log_message(self, log_message): + def test_format_log_message(self, log_message): """Should correctly format the log message with info from the message and token.""" log_message.format.return_value = "Howdy" return_value = TokenRemover.format_log_message(self.msg, "MTIz.DN9R_A.xyz") -- cgit v1.2.3 From ab44bb38d874dfdec9d7dc61bbf13b06144b9a0e Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 13 May 2020 19:18:50 -0700 Subject: Add missing comma to token remover log message --- bot/cogs/token_remover.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index c576a67d0..c57e7764e 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -17,7 +17,7 @@ from bot.constants import Channels, Colours, Event, Icons log = logging.getLogger(__name__) LOG_MESSAGE = ( - "Censored a seemingly valid token sent by {author} (`{author_id}`) in {channel}," + "Censored a seemingly valid token sent by {author} (`{author_id}`) in {channel}, " "token was `{user_id}.{timestamp}.{hmac}`" ) DELETION_MESSAGE_TEMPLATE = ( -- cgit v1.2.3 From 297089cde278ea09a27240f71f41006fab2b2ca4 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 13 May 2020 19:36:44 -0700 Subject: Token remover: add logs to clarify why token is invalid --- bot/cogs/token_remover.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index c57e7764e..244d52edb 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -133,12 +133,14 @@ class TokenRemover(Cog): try: user_id, creation_timestamp, hmac = test_str.split('.') except ValueError: + log.debug(f"Invalid token format in '{test_str}': does not have all 3 parts.") return False if cls.is_valid_user_id(user_id) and cls.is_valid_timestamp(creation_timestamp): return True - - return False + else: + log.debug(f"Invalid user ID or timestamp in '{test_str}'.") + return False @staticmethod def is_valid_user_id(b64_content: str) -> bool: -- cgit v1.2.3 From 73bcb2b434a30761494bbedd914508964c6fbbad Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 14 May 2020 10:34:37 -0700 Subject: Token remover: fix timestamp check The timestamp calculation was incorrect. The bytes need to be interpreted as big-endian and the result is just a timestamp rather than a snowflake. --- bot/cogs/token_remover.py | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 244d52edb..957c8a690 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -2,13 +2,10 @@ import base64 import binascii import logging import re -import struct import typing as t -from datetime import datetime from discord import Colour, Message from discord.ext.commands import Cog -from discord.utils import snowflake_time from bot.bot import Bot from bot.cogs.moderation import ModLog @@ -29,7 +26,7 @@ DELETION_MESSAGE_TEMPLATE = ( "Feel free to re-post it with the token removed. " "If you believe this was a mistake, please let us know!" ) -DISCORD_EPOCH_TIMESTAMP = datetime(2017, 1, 1) +DISCORD_EPOCH = 1_420_070_400_000 TOKEN_EPOCH = 1_293_840_000 TOKEN_RE = re.compile( r"[^\s\.()\"']+" # Matches token part 1: The user ID string, encoded as base64 @@ -160,18 +157,27 @@ class TokenRemover(Cog): @staticmethod def is_valid_timestamp(b64_content: str) -> bool: """ - Check potential token to see if it contains a valid timestamp. + Return True if `b64_content` decodes to a valid timestamp. - See: https://discordapp.com/developers/docs/reference#snowflakes + If the timestamp is greater than the Discord epoch, it's probably valid. + See: https://i.imgur.com/7WdehGn.png """ b64_content += '=' * (-len(b64_content) % 4) try: - content = base64.urlsafe_b64decode(b64_content) - snowflake = struct.unpack('i', content)[0] - except (binascii.Error, struct.error, ValueError): + decoded_bytes = base64.urlsafe_b64decode(b64_content) + timestamp = int.from_bytes(decoded_bytes, byteorder="big") + except (binascii.Error, ValueError) as e: + log.debug(f"Failed to decode token timestamp '{b64_content}': {e}") + return False + + # Seems like newer tokens don't need the epoch added, but add anyway since an upper bound + # is not checked. + if timestamp + TOKEN_EPOCH >= DISCORD_EPOCH: + return True + else: + log.debug(f"Invalid token timestamp '{b64_content}': smaller than Discord epoch") return False - return snowflake_time(snowflake + TOKEN_EPOCH) < DISCORD_EPOCH_TIMESTAMP def setup(bot: Bot) -> None: -- cgit v1.2.3 From 4a73c24678d4a893304f0b2f3a5f1e326cae817a Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Fri, 15 May 2020 08:54:36 -0700 Subject: Token remover: use strict check for digits in token ID `isnumeric` would be true for a wide range of characters in Unicode, but the ID must only consist of the characters 0-9 (ASCII digits). In fact, `isdigit` on its own would also match other Unicode characters too. --- bot/cogs/token_remover.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 957c8a690..43c12c4f7 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -149,8 +149,11 @@ class TokenRemover(Cog): b64_content += '=' * (-len(b64_content) % 4) try: - content: bytes = base64.b64decode(b64_content) - return content.decode('utf-8').isnumeric() + decoded_bytes: bytes = base64.b64decode(b64_content) + string = decoded_bytes.decode('utf-8') + + # isdigit on its own would match a lot of other Unicode characters, hence the isascii. + return string.isascii() and string.isdigit() except (binascii.Error, ValueError): return False -- cgit v1.2.3 From ad154f7f0d7daa3f962433f77d1cdd11cc66bfe0 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 16 May 2020 22:43:00 -0700 Subject: Add a utility function to pad base64 data --- bot/cogs/token_remover.py | 5 +++-- bot/utils/__init__.py | 5 +++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 43c12c4f7..cae482e6e 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -7,6 +7,7 @@ import typing as t from discord import Colour, Message from discord.ext.commands import Cog +from bot import utils from bot.bot import Bot from bot.cogs.moderation import ModLog from bot.constants import Channels, Colours, Event, Icons @@ -146,7 +147,7 @@ class TokenRemover(Cog): See: https://discordapp.com/developers/docs/reference#snowflakes """ - b64_content += '=' * (-len(b64_content) % 4) + b64_content = utils.pad_base64(b64_content) try: decoded_bytes: bytes = base64.b64decode(b64_content) @@ -165,7 +166,7 @@ class TokenRemover(Cog): If the timestamp is greater than the Discord epoch, it's probably valid. See: https://i.imgur.com/7WdehGn.png """ - b64_content += '=' * (-len(b64_content) % 4) + b64_content = utils.pad_base64(b64_content) try: decoded_bytes = base64.urlsafe_b64decode(b64_content) diff --git a/bot/utils/__init__.py b/bot/utils/__init__.py index 9b32e515d..1dd0636df 100644 --- a/bot/utils/__init__.py +++ b/bot/utils/__init__.py @@ -7,3 +7,8 @@ class CogABCMeta(CogMeta, ABCMeta): """Metaclass for ABCs meant to be implemented as Cogs.""" pass + + +def pad_base64(data: str) -> str: + """Return base64 `data` with padding characters to ensure its length is a multiple of 4.""" + return data + "=" * (-len(data) % 4) -- cgit v1.2.3 From e23aa887959059e17fc21dcab9c83db20dc987f5 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 21 May 2020 20:29:44 -0700 Subject: Token remover: decode ID using URL-safe base64 Though I've not seen an ID with neither + and \ nor - and _, given that the timestamp uses URL-safe encoding, the ID probably does too. --- bot/cogs/token_remover.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index cae482e6e..5b4598959 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -150,7 +150,7 @@ class TokenRemover(Cog): b64_content = utils.pad_base64(b64_content) try: - decoded_bytes: bytes = base64.b64decode(b64_content) + decoded_bytes = base64.urlsafe_b64decode(b64_content) string = decoded_bytes.decode('utf-8') # isdigit on its own would match a lot of other Unicode characters, hence the isascii. -- cgit v1.2.3 From 95ef2dc01143902289c9aacde7969fb5c9e1a85c Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 21 May 2020 21:34:10 -0700 Subject: Token remover: match only base64 in regex Making the regex more accurate reduces false positives at an earlier stage. There's no benefit to matching non-base64 as that would just be weeded out as invalid at a later stage anyway when it tries to decode it. --- bot/cogs/token_remover.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 5b4598959..fa0647828 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -29,13 +29,12 @@ DELETION_MESSAGE_TEMPLATE = ( ) DISCORD_EPOCH = 1_420_070_400_000 TOKEN_EPOCH = 1_293_840_000 -TOKEN_RE = re.compile( - r"[^\s\.()\"']+" # Matches token part 1: The user ID string, encoded as base64 - r"\." # Matches a literal dot between the token parts - r"[^\s\.()\"']+" # Matches token part 2: The creation timestamp, as an integer - r"\." # Matches a literal dot between the token parts - r"[^\s\.()\"']+" # Matches token part 3: The HMAC, unused by us, but check that it isn't empty -) + +# Three parts delimited by dots: user ID, creation timestamp, HMAC. +# The HMAC isn't parsed further, but it's in the regex to ensure it at least exists in the string. +# Each part only matches base64 URL-safe characters. +# Padding has never been observed, but the padding character '=' is matched just in case. +TOKEN_RE = re.compile(r"[\w-=]+\.[\w-=]+\.[\w-=]+", re.ASCII) class TokenRemover(Cog): -- cgit v1.2.3 From 2c7ff94c956691dafa35c92dd0baa95a60aafacf Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 23 May 2020 18:02:39 -0700 Subject: Token remover: escape dashes in regex They need to be escaped when they're in a character set. By default, they are interpreted as part of the character range syntax. --- bot/cogs/token_remover.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index fa0647828..f23eba89b 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -34,7 +34,7 @@ TOKEN_EPOCH = 1_293_840_000 # The HMAC isn't parsed further, but it's in the regex to ensure it at least exists in the string. # Each part only matches base64 URL-safe characters. # Padding has never been observed, but the padding character '=' is matched just in case. -TOKEN_RE = re.compile(r"[\w-=]+\.[\w-=]+\.[\w-=]+", re.ASCII) +TOKEN_RE = re.compile(r"[\w\-=]+\.[\w\-=]+\.[\w\-=]+", re.ASCII) class TokenRemover(Cog): -- cgit v1.2.3 From 161bf818ed0f1690c63f4f54cc9549e298e3e45c Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 19:45:04 -0700 Subject: Token remover: use regex groups and pass the token as a NamedTuple It felt redundant to be splitting the token in two different functions when regex could take care of this from the outset. ' A NamedTuple was created to house the token. This is nicer than passing an re.Match object, because it's clearer which attributes are available. Even if the regex used named groups, it wouldn't be as obvious which group names exist. Without the split, `is_maybe_token` is dwindled down to a redundant function. Therefore, it's been removed. --- bot/cogs/token_remover.py | 47 ++++++++++++++++++++--------------------------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index f23eba89b..e5d0ae838 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -34,7 +34,15 @@ TOKEN_EPOCH = 1_293_840_000 # The HMAC isn't parsed further, but it's in the regex to ensure it at least exists in the string. # Each part only matches base64 URL-safe characters. # Padding has never been observed, but the padding character '=' is matched just in case. -TOKEN_RE = re.compile(r"[\w\-=]+\.[\w\-=]+\.[\w\-=]+", re.ASCII) +TOKEN_RE = re.compile(r"([\w\-=]+)\.([\w\-=]+)\.([\w\-=]+)", re.ASCII) + + +class Token(t.NamedTuple): + """A Discord Bot token.""" + + user_id: str + timestamp: str + hmac: str class TokenRemover(Cog): @@ -68,7 +76,7 @@ class TokenRemover(Cog): """ await self.on_message(after) - async def take_action(self, msg: Message, found_token: str) -> None: + async def take_action(self, msg: Message, found_token: Token) -> None: """Remove the `msg` containing the `found_token` and send a mod log message.""" self.mod_log.ignore(Event.message_delete, msg.id) await self.delete_message(msg) @@ -95,20 +103,19 @@ class TokenRemover(Cog): await msg.channel.send(DELETION_MESSAGE_TEMPLATE.format(mention=msg.author.mention)) @staticmethod - def format_log_message(msg: Message, found_token: str) -> str: - """Return the log message to send for `found_token` being censored in `msg`.""" - user_id, creation_timestamp, hmac = found_token.split('.') + def format_log_message(msg: Message, token: Token) -> str: + """Return the log message to send for `token` being censored in `msg`.""" return LOG_MESSAGE.format( author=msg.author, author_id=msg.author.id, channel=msg.channel.mention, - user_id=user_id, - timestamp=creation_timestamp, - hmac='x' * len(hmac), + user_id=token.user_id, + timestamp=token.timestamp, + hmac='x' * len(token.hmac), ) @classmethod - def find_token_in_message(cls, msg: Message) -> t.Optional[str]: + def find_token_in_message(cls, msg: Message) -> t.Optional[Token]: """Return a seemingly valid token found in `msg` or `None` if no token is found.""" if msg.author.bot: return @@ -116,29 +123,15 @@ class TokenRemover(Cog): # Use findall rather than search to guard against method calls prematurely returning the # token check (e.g. `message.channel.send` also matches our token pattern) maybe_matches = TOKEN_RE.findall(msg.content) - for substr in maybe_matches: - if cls.is_maybe_token(substr): + for match_groups in maybe_matches: + token = Token(*match_groups) + if cls.is_valid_user_id(token.user_id) and cls.is_valid_timestamp(token.timestamp): # Short-circuit on first match - return substr + return token # No matching substring return - @classmethod - def is_maybe_token(cls, test_str: str) -> bool: - """Check the provided string to see if it is a seemingly valid token.""" - try: - user_id, creation_timestamp, hmac = test_str.split('.') - except ValueError: - log.debug(f"Invalid token format in '{test_str}': does not have all 3 parts.") - return False - - if cls.is_valid_user_id(user_id) and cls.is_valid_timestamp(creation_timestamp): - return True - else: - log.debug(f"Invalid user ID or timestamp in '{test_str}'.") - return False - @staticmethod def is_valid_user_id(b64_content: str) -> bool: """ -- cgit v1.2.3 From bfe79efdfe699bf7289cba9db95d5637a7fb965a Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 19:46:51 -0700 Subject: Token remover: use finditer instead of findall It makes more sense to use the lazy function when the loop is already short-circuiting on the first valid token it finds. --- bot/cogs/token_remover.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index e5d0ae838..8913ca64d 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -120,11 +120,10 @@ class TokenRemover(Cog): if msg.author.bot: return - # Use findall rather than search to guard against method calls prematurely returning the + # Use finditer rather than search to guard against method calls prematurely returning the # token check (e.g. `message.channel.send` also matches our token pattern) - maybe_matches = TOKEN_RE.findall(msg.content) - for match_groups in maybe_matches: - token = Token(*match_groups) + for match in TOKEN_RE.finditer(msg.content): + token = Token(*match.groups()) if cls.is_valid_user_id(token.user_id) and cls.is_valid_timestamp(token.timestamp): # Short-circuit on first match return token -- cgit v1.2.3 From 5386eda1731bb8eae287c20ed70a76399db2ae0e Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 19:55:23 -0700 Subject: Token remover: specify Discord epoch in seconds The timestamp in the token is in seconds and is being compared against the epoch. To make life easier, they should use the same unit. Previously, the epoch was in milliseconds. --- bot/cogs/token_remover.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 8913ca64d..46329e207 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -27,7 +27,7 @@ DELETION_MESSAGE_TEMPLATE = ( "Feel free to re-post it with the token removed. " "If you believe this was a mistake, please let us know!" ) -DISCORD_EPOCH = 1_420_070_400_000 +DISCORD_EPOCH = 1_420_070_400 TOKEN_EPOCH = 1_293_840_000 # Three parts delimited by dots: user ID, creation timestamp, HMAC. -- cgit v1.2.3 From 47886501fb7d030f1cb91c69413058e3ffcb76bf Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 20:47:32 -0700 Subject: Test token regex won't match non-base64 characters --- tests/bot/cogs/test_token_remover.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 8e743a715..dbea5ad1b 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -144,10 +144,9 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): "x..z", " . . ", "\n.\n.\n", - "'.'.'", - '"."."', - "(.(.(", - ").).)" + "hellö.world.bye", + "base64.nötbåse64.morebase64", + "19jd3J.dfkm3d.€víł§tüff", ) for token in tokens: -- cgit v1.2.3 From e76099d48b9a895c48e58c5f5489886f4191eeb6 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 20:50:30 -0700 Subject: Add more valid tokens to test the regex with --- tests/bot/cogs/test_token_remover.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index dbea5ad1b..6a280f358 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -156,10 +156,12 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): def test_regex_valid_tokens(self): """Messages that look like tokens should be matched.""" - # Don't worry, the token's been invalidated. + # Don't worry, these tokens have been invalidated. tokens = ( - "x1.y2.z_3", - "NDcyMjY1OTQzMDYyNDEzMzMy.Xrim9Q.Ysnu2wacjaKs7qnoo46S8Dm2us8" + "NDcyMjY1OTQzMDYy_DEzMz-y.XsyRkw.VXmErH7j511turNpfURmb0rVNm8", + "NDcyMjY1OTQzMDYyNDEzMzMy.Xrim9Q.Ysnu2wacjaKs7qnoo46S8Dm2us8", + "NDc1MDczNjI5Mzk5NTQ3OTA0.XsyR-w.sJf6omBPORBPju3WJEIAcwW9Zds", + "NDY3MjIzMjMwNjUwNzc3NjQx.XsySD_.s45jqDV_Iisn-symw0yDRrk_jf4", ) for token in tokens: -- cgit v1.2.3 From a8a216d0803b67a330ae092a17bea563f5012275 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 21:02:24 -0700 Subject: Fix valid token regex test It was broken due to the addition of groups. Rather than returning the full match, `findall` returns groups if any exist. The test was comparing a tuple of groups to the token string, which was of course failing. Now `fullmatch` is used cause it's simpler - just check for `None` and don't worry about iterating matches to search. --- tests/bot/cogs/test_token_remover.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 6a280f358..518bf91ca 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -166,8 +166,8 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): for token in tokens: with self.subTest(token=token): - results = token_remover.TOKEN_RE.findall(token) - self.assertIn(token, results) + results = token_remover.TOKEN_RE.fullmatch(token) + self.assertIsNotNone(results, f"{token} was not matched by the regex") def test_regex_matches_multiple_valid(self): """Should support multiple matches in the middle of a string.""" -- cgit v1.2.3 From 19cc849d4c70bc3e792460ad712aa308fa500462 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 21:07:21 -0700 Subject: Fix multiple match text for token regex It has to account for the addition of groups. It's easiest to compare the entire string so `finditer` is used to return re.Match objects; the tuples of `findall` would be cumbersome. Also threw in a change to use `assertCountEqual` cause the order doesn't really matter. --- tests/bot/cogs/test_token_remover.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 518bf91ca..2ecfae2bd 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -174,8 +174,9 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): tokens = ["x.y.z", "a.b.c"] message = f"garbage {tokens[0]} hello {tokens[1]} world" - results = token_remover.TOKEN_RE.findall(message) - self.assertEqual(tokens, results) + results = token_remover.TOKEN_RE.finditer(message) + results = [match[0] for match in results] + self.assertCountEqual(tokens, results) @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): -- cgit v1.2.3 From 300f8c093edea03855d94be179c64c328ec842ac Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 21:09:04 -0700 Subject: Use real token values for testing multiple matches in regex --- tests/bot/cogs/test_token_remover.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 2ecfae2bd..971bc93fc 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -171,12 +171,13 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): def test_regex_matches_multiple_valid(self): """Should support multiple matches in the middle of a string.""" - tokens = ["x.y.z", "a.b.c"] - message = f"garbage {tokens[0]} hello {tokens[1]} world" + token_1 = "NDY3MjIzMjMwNjUwNzc3NjQx.XsyWGg.uFNEQPCc4ePwGh7egG8UicQssz8" + token_2 = "NDcyMjY1OTQzMDYyNDEzMzMy.XsyWMw.l8XPnDqb0lp-EiQ2g_0xVFT1pyc" + message = f"garbage {token_1} hello {token_2} world" results = token_remover.TOKEN_RE.finditer(message) results = [match[0] for match in results] - self.assertCountEqual(tokens, results) + self.assertCountEqual((token_1, token_2), results) @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): -- cgit v1.2.3 From 96db6087254c957fcb8fb45aad7ffcddb46ee839 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 27 May 2020 17:08:18 -0700 Subject: Switch findall to finditer in assertions `find_token_in_message` now uses the latter so the tests should adjust accordingly. --- tests/bot/cogs/test_token_remover.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 971bc93fc..4fff3ab33 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -94,18 +94,18 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): return_value = TokenRemover.find_token_in_message(self.msg) self.assertIsNone(return_value) - token_re.findall.assert_not_called() + token_re.finditer.assert_not_called() @autospec(TokenRemover, "is_maybe_token") @autospec("bot.cogs.token_remover", "TOKEN_RE") def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): """None should be returned if the regex matches no tokens in a message.""" - token_re.findall.return_value = () + token_re.finditer.return_value = () return_value = TokenRemover.find_token_in_message(self.msg) self.assertIsNone(return_value) - token_re.findall.assert_called_once_with(self.msg.content) + token_re.finditer.assert_called_once_with(self.msg.content) is_maybe_token.assert_not_called() @autospec(TokenRemover, "is_maybe_token") @@ -123,7 +123,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): return_value = TokenRemover.find_token_in_message(self.msg) self.assertEqual(return_value, matches[true_index]) - token_re.findall.assert_called_once_with(self.msg.content) + token_re.finditer.assert_called_once_with(self.msg.content) # assert_has_calls isn't used cause it'd allow for extra calls before or after. # The function should short-circuit, so nothing past true_index should have been used. -- cgit v1.2.3 From f937032466a4124bacf217d1bfd0af097fc3395d Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 27 May 2020 19:31:55 -0700 Subject: Adjust token remover tests to use the Token NamedTuple --- tests/bot/cogs/test_token_remover.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 4fff3ab33..65bc1ee58 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -7,7 +7,7 @@ from discord import Colour from bot import constants from bot.cogs import token_remover from bot.cogs.moderation import ModLog -from bot.cogs.token_remover import TokenRemover +from bot.cogs.token_remover import Token, TokenRemover from tests.helpers import MockBot, MockMessage, autospec @@ -224,17 +224,19 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec("bot.cogs.token_remover", "LOG_MESSAGE") def test_format_log_message(self, log_message): """Should correctly format the log message with info from the message and token.""" + token = Token("NDY3MjIzMjMwNjUwNzc3NjQx", "XsySD_", "s45jqDV_Iisn-symw0yDRrk_jf4") log_message.format.return_value = "Howdy" - return_value = TokenRemover.format_log_message(self.msg, "MTIz.DN9R_A.xyz") + + return_value = TokenRemover.format_log_message(self.msg, token) self.assertEqual(return_value, log_message.format.return_value) log_message.format.assert_called_once_with( author=self.msg.author, author_id=self.msg.author.id, channel=self.msg.channel.mention, - user_id="MTIz", - timestamp="DN9R_A", - hmac="xxx", + user_id=token.user_id, + timestamp=token.timestamp, + hmac="x" * len(token.hmac), ) @mock.patch.object(TokenRemover, "mod_log", new_callable=mock.PropertyMock) @@ -244,7 +246,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): """Should delete the message and send a mod log.""" cog = TokenRemover(self.bot) mod_log = mock.create_autospec(ModLog, spec_set=True, instance=True) - token = "MTIz.DN9R_A.xyz" + token = mock.create_autospec(Token, spec_set=True, instance=True) log_msg = "testing123" mod_log_property.return_value = mod_log -- cgit v1.2.3 From 12b8f5002807144451a313180c639bf6b4925f2e Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 27 May 2020 20:00:33 -0700 Subject: Add more thorough and realistic inputs for token ID and timestamp tests The tests for valid inputs and invalid inputs were split to make them more readable. --- tests/bot/cogs/test_token_remover.py | 70 ++++++++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 18 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 65bc1ee58..ffe76865a 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -24,31 +24,65 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.msg.author.__str__ = MagicMock(return_value=self.msg.author.name) self.msg.author.avatar_url_as.return_value = "picture-lemon.png" - def test_is_valid_user_id(self): - """Should correctly discern valid user IDs and ignore non-numeric and non-ASCII IDs.""" - subtests = ( - ("MTIz", True), # base64(123) - ("YWJj", False), # base64(abc) - ("λδµ", False), + def test_is_valid_user_id_valid(self): + """Should consider user IDs valid if they decode entirely to ASCII digits.""" + ids = ( + "NDcyMjY1OTQzMDYyNDEzMzMy", + "NDc1MDczNjI5Mzk5NTQ3OTA0", + "NDY3MjIzMjMwNjUwNzc3NjQx", ) - for user_id, is_valid in subtests: - with self.subTest(user_id=user_id, is_valid=is_valid): + for user_id in ids: + with self.subTest(user_id=user_id): result = TokenRemover.is_valid_user_id(user_id) - self.assertIs(result, is_valid) + self.assertTrue(result) + + def test_is_valid_user_id_invalid(self): + """Should consider non-digit and non-ASCII IDs invalid.""" + ids = ( + ("SGVsbG8gd29ybGQ", "non-digit ASCII"), + ("0J_RgNC40LLQtdGCINC80LjRgA", "cyrillic text"), + ("4pO14p6L4p6C4pG34p264pGl8J-EiOKSj-KCieKBsA", "Unicode digits"), + ("4oaA4oaB4oWh4oWi4Lyz4Lyq4Lyr4LG9", "Unicode numerals"), + ("8J2fjvCdn5nwnZ-k8J2fr_Cdn7rgravvvJngr6c", "Unicode decimals"), + ("{hello}[world]&(bye!)", "ASCII invalid Base64"), + ("Þíß-ï§-ňøẗ-våłìÐ", "Unicode invalid Base64"), + ) - def test_is_valid_timestamp(self): - """Should correctly discern valid timestamps.""" - subtests = ( - ("DN9r_A", True), - ("MTIz", False), # base64(123) - ("λδµ", False), + for user_id, msg in ids: + with self.subTest(msg=msg): + result = TokenRemover.is_valid_user_id(user_id) + self.assertFalse(result) + + def test_is_valid_timestamp_valid(self): + """Should consider timestamps valid if they're greater than the Discord epoch.""" + timestamps = ( + "XsyRkw", + "Xrim9Q", + "XsyR-w", + "XsySD_", + "Dn9r_A", + ) + + for timestamp in timestamps: + with self.subTest(timestamp=timestamp): + result = TokenRemover.is_valid_timestamp(timestamp) + self.assertTrue(result) + + def test_is_valid_timestamp_invalid(self): + """Should consider timestamps invalid if they're before Discord epoch or can't be parsed.""" + timestamps = ( + ("B4Yffw", "DISCORD_EPOCH - TOKEN_EPOCH - 1"), + ("ew", "123"), + ("AoIKgA", "42076800"), + ("{hello}[world]&(bye!)", "ASCII invalid Base64"), + ("Þíß-ï§-ňøẗ-våłìÐ", "Unicode invalid Base64"), ) - for timestamp, is_valid in subtests: - with self.subTest(timestamp=timestamp, is_valid=is_valid): + for timestamp, msg in timestamps: + with self.subTest(msg=msg): result = TokenRemover.is_valid_timestamp(timestamp) - self.assertIs(result, is_valid) + self.assertFalse(result) def test_mod_log_property(self): """The `mod_log` property should ask the bot to return the `ModLog` cog.""" -- cgit v1.2.3 From 67472080fef5c38b21d74daa2178c3f35081b58f Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 28 May 2020 19:52:41 -0700 Subject: Remove is_maybe_token tests The function was removed due to redundancy. Therefore, its tests are obsolete. --- tests/bot/cogs/test_token_remover.py | 33 --------------------------------- 1 file changed, 33 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index ffe76865a..5dd12636c 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -213,39 +213,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): results = [match[0] for match in results] self.assertCountEqual((token_1, token_2), results) - @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") - def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): - """False should be returned for tokens which do not have all 3 parts.""" - return_value = TokenRemover.is_maybe_token("x.y") - - self.assertFalse(return_value) - valid_user.assert_not_called() - valid_time.assert_not_called() - - @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") - def test_is_maybe_token(self, valid_user, valid_time): - """Should return True if the user ID and timestamp are valid or return False otherwise.""" - subtests = ( - (False, True, False), - (True, False, False), - (True, True, True), - ) - - for user_return, time_return, expected in subtests: - valid_user.reset_mock() - valid_time.reset_mock() - - with self.subTest(user_return=user_return, time_return=time_return, expected=expected): - valid_user.return_value = user_return - valid_time.return_value = time_return - - actual = TokenRemover.is_maybe_token("x.y.z") - self.assertIs(actual, expected) - - valid_user.assert_called_once_with("x") - if user_return: - valid_time.assert_called_once_with("y") - async def test_delete_message(self): """The message should be deleted, and a message should be sent to the same channel.""" await TokenRemover.delete_message(self.msg) -- cgit v1.2.3 From 84cd8235863acc80b7f140309424c33180cc34ea Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 28 May 2020 20:32:48 -0700 Subject: Adjust find_token_in_message tests for the recent cog changes It now supports the changes that switched to finditer, added match groups, and added the Token NamedTuple. It also accounts for the is_maybe_token function being removed. For the sake of simplicity, call assertions on is_valid_user_id and is_valid_timestamp were not made. --- tests/bot/cogs/test_token_remover.py | 39 ++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 5dd12636c..8238e235a 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -1,4 +1,5 @@ import unittest +from re import Match from unittest import mock from unittest.mock import MagicMock @@ -130,9 +131,8 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.assertIsNone(return_value) token_re.finditer.assert_not_called() - @autospec(TokenRemover, "is_maybe_token") @autospec("bot.cogs.token_remover", "TOKEN_RE") - def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): + def test_find_token_no_matches(self, token_re): """None should be returned if the regex matches no tokens in a message.""" token_re.finditer.return_value = () @@ -140,30 +140,31 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.assertIsNone(return_value) token_re.finditer.assert_called_once_with(self.msg.content) - is_maybe_token.assert_not_called() - @autospec(TokenRemover, "is_maybe_token") + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") + @autospec("bot.cogs.token_remover", "Token") @autospec("bot.cogs.token_remover", "TOKEN_RE") - def test_find_token_returns_found_token(self, token_re, is_maybe_token): - """The found token should be returned.""" - true_index = 1 - matches = ("foo", "bar", "baz") - side_effects = [False] * len(matches) - side_effects[true_index] = True - - token_re.findall.return_value = matches - is_maybe_token.side_effect = side_effects + def test_find_token_valid_match(self, token_re, token_cls, is_valid_id, is_valid_timestamp): + """The first match with a valid user ID and timestamp should be returned as a `Token`.""" + matches = [ + mock.create_autospec(Match, spec_set=True, instance=True), + mock.create_autospec(Match, spec_set=True, instance=True), + ] + tokens = [ + mock.create_autospec(Token, spec_set=True, instance=True), + mock.create_autospec(Token, spec_set=True, instance=True), + ] + + token_re.finditer.return_value = matches + token_cls.side_effect = tokens + is_valid_id.side_effect = (False, True) # The 1st match will be invalid, 2nd one valid. + is_valid_timestamp.return_value = True return_value = TokenRemover.find_token_in_message(self.msg) - self.assertEqual(return_value, matches[true_index]) + self.assertEqual(tokens[1], return_value) token_re.finditer.assert_called_once_with(self.msg.content) - # assert_has_calls isn't used cause it'd allow for extra calls before or after. - # The function should short-circuit, so nothing past true_index should have been used. - calls = [mock.call(match) for match in matches[:true_index + 1]] - self.assertEqual(is_maybe_token.mock_calls, calls) - def test_regex_invalid_tokens(self): """Messages without anything looking like a token are not matched.""" tokens = ( -- cgit v1.2.3 From 5930a044b8347019d474a809fc86f89263574ad0 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 28 May 2020 20:33:34 -0700 Subject: Test find_token_in_message returns None for invalid matches This covers the case when a token is matched, but its user ID and timestamp turn out to be invalid. --- tests/bot/cogs/test_token_remover.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 8238e235a..9b4b04ecd 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -165,6 +165,21 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.assertEqual(tokens[1], return_value) token_re.finditer.assert_called_once_with(self.msg.content) + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") + @autospec("bot.cogs.token_remover", "Token") + @autospec("bot.cogs.token_remover", "TOKEN_RE") + def test_find_token_invalid_matches(self, token_re, token_cls, is_valid_id, is_valid_timestamp): + """None should be returned if no matches have valid user IDs or timestamps.""" + token_re.finditer.return_value = [mock.create_autospec(Match, spec_set=True, instance=True)] + token_cls.return_value = mock.create_autospec(Token, spec_set=True, instance=True) + is_valid_id.return_value = False + is_valid_timestamp.return_value = False + + return_value = TokenRemover.find_token_in_message(self.msg) + + self.assertIsNone(return_value) + token_re.finditer.assert_called_once_with(self.msg.content) + def test_regex_invalid_tokens(self): """Messages without anything looking like a token are not matched.""" tokens = ( -- cgit v1.2.3 From 876b4846f612fe0011cc2e0b498b4df9e54d74cb Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sun, 31 May 2020 19:17:07 +0200 Subject: Add support for bool values in RedisCache We're gonna need this for the help channel handling, and it seems like a reasonable type to support anyway. It requires a tiny bit of special handling, but nothing outrageous. --- bot/utils/redis_cache.py | 14 ++++++++++++-- tests/bot/utils/test_redis_cache.py | 4 +++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index de80cee84..2926e7a89 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -2,6 +2,7 @@ from __future__ import annotations import asyncio import logging +from distutils.util import strtobool from functools import partialmethod from typing import Any, Dict, ItemsView, Optional, Tuple, Union @@ -11,7 +12,7 @@ log = logging.getLogger(__name__) # Type aliases RedisKeyType = Union[str, int] -RedisValueType = Union[str, int, float] +RedisValueType = Union[str, int, float, bool] RedisKeyOrValue = Union[RedisKeyType, RedisValueType] # Prefix tuples @@ -20,6 +21,7 @@ _VALUE_PREFIXES = ( ("f|", float), ("i|", int), ("s|", str), + ("b|", bool), ) _KEY_PREFIXES = ( ("i|", int), @@ -117,7 +119,8 @@ class RedisCache: def _to_typestring(key_or_value: RedisKeyOrValue, prefixes: _PrefixTuple) -> str: """Turn a valid Redis type into a typestring.""" for prefix, _type in prefixes: - if isinstance(key_or_value, _type): + # isinstance is a bad idea here, because isintance(False, int) == True. + if type(key_or_value) is _type: return f"{prefix}{key_or_value}" raise TypeError(f"RedisCache._to_typestring only supports the following: {prefixes}.") @@ -131,6 +134,13 @@ class RedisCache: # Now we convert our unicode string back into the type it originally was. for prefix, _type in prefixes: if key_or_value.startswith(prefix): + + # For booleans, we need special handling because bool("False") is True. + if prefix == "b|": + value = key_or_value[len(prefix):] + return bool(strtobool(value)) + + # Otherwise we can just convert normally. return _type(key_or_value[len(prefix):]) raise TypeError(f"RedisCache._from_typestring only supports the following: {prefixes}.") diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index 8c1a40640..62c411681 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -59,7 +59,9 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): test_cases = ( ('favorite_fruit', 'melon'), ('favorite_number', 86), - ('favorite_fraction', 86.54) + ('favorite_fraction', 86.54), + ('favorite_boolean', False), + ('other_boolean', True), ) # Test that we can get and set different types. -- cgit v1.2.3 From eac3be892a31a508f966fd73f1802086d83ed954 Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Mon, 1 Jun 2020 19:09:22 +0200 Subject: Use Scheduler instead of asyncio.sleep on silence cog `asyncio.sleep` doesn't provide us with the ability to stop that timer, while in most of the cases, this is fine, there is a possibility that channel will be unsilenced manually and silenced again, but this sleep from the first silence will cancel the second (new) silence. This will replace this `asyncio.sleep` with Scheduler which provides the ability to cancel the unsilencing task when aborted manually. That means we also have the ability to send a response if the channel is not silenced and someone tries to unsilence it. --- bot/cogs/moderation/silence.py | 66 +++++++++++++++++++++++++++++++++++------- 1 file changed, 56 insertions(+), 10 deletions(-) diff --git a/bot/cogs/moderation/silence.py b/bot/cogs/moderation/silence.py index 25febfa51..a2fd39906 100644 --- a/bot/cogs/moderation/silence.py +++ b/bot/cogs/moderation/silence.py @@ -1,19 +1,54 @@ import asyncio +import datetime import logging +from collections import namedtuple from contextlib import suppress from typing import Optional -from discord import TextChannel -from discord.ext import commands, tasks -from discord.ext.commands import Context - from bot.bot import Bot -from bot.constants import Channels, Emojis, Guild, MODERATION_ROLES, Roles +from bot.constants import MODERATION_ROLES, Channels, Emojis, Guild, Roles from bot.converters import HushDurationConverter +from bot.utils import time from bot.utils.checks import with_role_check +from bot.utils.scheduling import Scheduler +from discord import TextChannel +from discord.ext import commands, tasks +from discord.ext.commands import Context log = logging.getLogger(__name__) +SilencedChannel = namedtuple( + "SilencedChannel", ("id", "ctx", "silence", "stop") +) + + +class UnsilenceScheduler(Scheduler): + """Scheduler for unsilencing channels""" + + def __init__(self, bot: Bot): + super().__init__() + + self.bot = bot + + async def schedule_unsilence(self, channel: SilencedChannel) -> None: + """Schedule expiration for silenced channels""" + await self.bot.wait_until_guild_available() + log.debug("Scheduling unsilencer") + self.schedule_task(channel.id, channel) + + async def _scheduled_task(self, channel: SilencedChannel) -> None: + """ + Removes expired silenced channel from `silence.muted_channels` + and calls `silence.unsilence` to unsilence the channel + after the silence expires + """ + await time.wait_until(channel.stop) + log.info("Unsilencing channel after set delay.") + + # Because `silence.unsilence` explicitly cancels this scheduled task, it is shielded + # to avoid prematurely cancelling itself. + await asyncio.shield(channel.ctx.invoke(channel.silence.unsilence)) + class SilenceNotifier(tasks.Loop): """Loop notifier for posting notices to `alert_channel` containing added channels.""" @@ -61,6 +96,7 @@ class Silence(commands.Cog): self.muted_channels = set() self._get_instance_vars_task = self.bot.loop.create_task(self._get_instance_vars()) self._get_instance_vars_event = asyncio.Event() + self.scheduler = UnsilenceScheduler(bot) async def _get_instance_vars(self) -> None: """Get instance variables after they're available to get from the guild.""" @@ -90,9 +126,15 @@ class Silence(commands.Cog): return await ctx.send(f"{Emojis.check_mark} silenced current channel for {duration} minute(s).") - await asyncio.sleep(duration*60) - log.info("Unsilencing channel after set delay.") - await ctx.invoke(self.unsilence) + + channel = SilencedChannel( + id=ctx.channel.id, + ctx=ctx, + silence=self, + stop=datetime.datetime.now() + datetime.timedelta(minutes=duration), + ) + + await self.scheduler.schedule_unsilence(channel) @commands.command(aliases=("unhush",)) async def unsilence(self, ctx: Context) -> None: @@ -103,8 +145,11 @@ class Silence(commands.Cog): """ await self._get_instance_vars_event.wait() log.debug(f"Unsilencing channel #{ctx.channel} from {ctx.author}'s command.") - if await self._unsilence(ctx.channel): - await ctx.send(f"{Emojis.check_mark} unsilenced current channel.") + if not await self._unsilence(ctx.channel): + await ctx.send(f"{Emojis.cross_mark} current channel is not silenced.") + return + + await ctx.send(f"{Emojis.check_mark} unsilenced current channel.") async def _silence(self, channel: TextChannel, persistent: bool, duration: Optional[int]) -> bool: """ @@ -141,6 +186,7 @@ class Silence(commands.Cog): await channel.set_permissions(self._verified_role, **dict(current_overwrite, send_messages=None)) log.info(f"Unsilenced channel #{channel} ({channel.id}).") self.notifier.remove_channel(channel) + self.scheduler.cancel_task(channel.id) self.muted_channels.discard(channel) return True log.info(f"Tried to unsilence channel #{channel} ({channel.id}) but the channel was not silenced.") -- cgit v1.2.3 From 9b0df33c90326f62d699afc70c13d3d375affeab Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Mon, 1 Jun 2020 23:08:32 +0200 Subject: Fix Formatting/Styling --- bot/cogs/moderation/silence.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/bot/cogs/moderation/silence.py b/bot/cogs/moderation/silence.py index a2fd39906..448f17966 100644 --- a/bot/cogs/moderation/silence.py +++ b/bot/cogs/moderation/silence.py @@ -6,11 +6,12 @@ from contextlib import suppress from typing import Optional from bot.bot import Bot -from bot.constants import MODERATION_ROLES, Channels, Emojis, Guild, Roles +from bot.constants import Channels, Emojis, Guild, MODERATION_ROLES, Roles from bot.converters import HushDurationConverter from bot.utils import time from bot.utils.checks import with_role_check from bot.utils.scheduling import Scheduler + from discord import TextChannel from discord.ext import commands, tasks from discord.ext.commands import Context @@ -23,7 +24,7 @@ SilencedChannel = namedtuple( class UnsilenceScheduler(Scheduler): - """Scheduler for unsilencing channels""" + """Scheduler for unsilencing channels.""" def __init__(self, bot: Bot): super().__init__() @@ -31,17 +32,13 @@ class UnsilenceScheduler(Scheduler): self.bot = bot async def schedule_unsilence(self, channel: SilencedChannel) -> None: - """Schedule expiration for silenced channels""" + """Schedule expiration for silenced channels.""" await self.bot.wait_until_guild_available() log.debug("Scheduling unsilencer") self.schedule_task(channel.id, channel) async def _scheduled_task(self, channel: SilencedChannel) -> None: - """ - Removes expired silenced channel from `silence.muted_channels` - and calls `silence.unsilence` to unsilence the channel - after the silence expires - """ + """Calls `silence.unsilence` on expired silenced channel to unsilence it.""" await time.wait_until(channel.stop) log.info("Unsilencing channel after set delay.") -- cgit v1.2.3 From 0a7b64df552d394e9d1f38fb167ec93334b9bead Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Mon, 1 Jun 2020 23:24:07 +0200 Subject: Optimize Imports --- bot/cogs/moderation/silence.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bot/cogs/moderation/silence.py b/bot/cogs/moderation/silence.py index 448f17966..5dfa9cc8a 100644 --- a/bot/cogs/moderation/silence.py +++ b/bot/cogs/moderation/silence.py @@ -5,6 +5,10 @@ from collections import namedtuple from contextlib import suppress from typing import Optional +from discord import TextChannel +from discord.ext import commands, tasks +from discord.ext.commands import Context + from bot.bot import Bot from bot.constants import Channels, Emojis, Guild, MODERATION_ROLES, Roles from bot.converters import HushDurationConverter @@ -12,10 +16,6 @@ from bot.utils import time from bot.utils.checks import with_role_check from bot.utils.scheduling import Scheduler -from discord import TextChannel -from discord.ext import commands, tasks -from discord.ext.commands import Context - log = logging.getLogger(__name__) SilencedChannel = namedtuple( -- cgit v1.2.3 From 9b3ab7df5ae1ecf95705f2fab7d99fdb36eb98ea Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Tue, 2 Jun 2020 19:22:49 -0700 Subject: Token remover: remove the `delete_message` function It's redundant; there's no benefit here in abstracting two lines of code into a function. --- bot/cogs/token_remover.py | 9 ++------- tests/bot/cogs/test_token_remover.py | 19 +++++++------------ 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 46329e207..d55e079e9 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -79,7 +79,8 @@ class TokenRemover(Cog): async def take_action(self, msg: Message, found_token: Token) -> None: """Remove the `msg` containing the `found_token` and send a mod log message.""" self.mod_log.ignore(Event.message_delete, msg.id) - await self.delete_message(msg) + await msg.delete() + await msg.channel.send(DELETION_MESSAGE_TEMPLATE.format(mention=msg.author.mention)) log_message = self.format_log_message(msg, found_token) log.debug(log_message) @@ -96,12 +97,6 @@ class TokenRemover(Cog): self.bot.stats.incr("tokens.removed_tokens") - @staticmethod - async def delete_message(msg: Message) -> None: - """Remove a `msg` containing a token and send an explanatory message in the same channel.""" - await msg.delete() - await msg.channel.send(DELETION_MESSAGE_TEMPLATE.format(mention=msg.author.mention)) - @staticmethod def format_log_message(msg: Message, token: Token) -> str: """Return the log message to send for `token` being censored in `msg`.""" diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 9b4b04ecd..a10124d2d 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -229,15 +229,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): results = [match[0] for match in results] self.assertCountEqual((token_1, token_2), results) - async def test_delete_message(self): - """The message should be deleted, and a message should be sent to the same channel.""" - await TokenRemover.delete_message(self.msg) - - self.msg.delete.assert_called_once_with() - self.msg.channel.send.assert_called_once_with( - token_remover.DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) - ) - @autospec("bot.cogs.token_remover", "LOG_MESSAGE") def test_format_log_message(self, log_message): """Should correctly format the log message with info from the message and token.""" @@ -258,8 +249,8 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @mock.patch.object(TokenRemover, "mod_log", new_callable=mock.PropertyMock) @autospec("bot.cogs.token_remover", "log") - @autospec(TokenRemover, "delete_message", "format_log_message") - async def test_take_action(self, delete_message, format_log_message, logger, mod_log_property): + @autospec(TokenRemover, "format_log_message") + async def test_take_action(self, format_log_message, logger, mod_log_property): """Should delete the message and send a mod log.""" cog = TokenRemover(self.bot) mod_log = mock.create_autospec(ModLog, spec_set=True, instance=True) @@ -271,7 +262,11 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): await cog.take_action(self.msg, token) - delete_message.assert_awaited_once_with(self.msg) + self.msg.delete.assert_called_once_with() + self.msg.channel.send.assert_called_once_with( + token_remover.DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) + ) + format_log_message.assert_called_once_with(self.msg, token) logger.debug.assert_called_with(log_msg) self.bot.stats.incr.assert_called_once_with("tokens.removed_tokens") -- cgit v1.2.3 From 469692d53a4ed74500a5806273e9c778a97afae8 Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Thu, 4 Jun 2020 22:50:36 +0200 Subject: Use Scheduler inside the cog - There shouldn't be another class only for Scheduler instead, we can implement it directly into Silence class --- bot/cogs/moderation/silence.py | 50 +++++++++++++++++------------------------- 1 file changed, 20 insertions(+), 30 deletions(-) diff --git a/bot/cogs/moderation/silence.py b/bot/cogs/moderation/silence.py index 5dfa9cc8a..398a70e51 100644 --- a/bot/cogs/moderation/silence.py +++ b/bot/cogs/moderation/silence.py @@ -19,34 +19,10 @@ from bot.utils.scheduling import Scheduler log = logging.getLogger(__name__) SilencedChannel = namedtuple( - "SilencedChannel", ("id", "ctx", "silence", "stop") + "SilencedChannel", ("id", "ctx", "stop") ) -class UnsilenceScheduler(Scheduler): - """Scheduler for unsilencing channels.""" - - def __init__(self, bot: Bot): - super().__init__() - - self.bot = bot - - async def schedule_unsilence(self, channel: SilencedChannel) -> None: - """Schedule expiration for silenced channels.""" - await self.bot.wait_until_guild_available() - log.debug("Scheduling unsilencer") - self.schedule_task(channel.id, channel) - - async def _scheduled_task(self, channel: SilencedChannel) -> None: - """Calls `silence.unsilence` on expired silenced channel to unsilence it.""" - await time.wait_until(channel.stop) - log.info("Unsilencing channel after set delay.") - - # Because `silence.unsilence` explicitly cancels this scheduled task, it is shielded - # to avoid prematurely cancelling itself. - await asyncio.shield(channel.ctx.invoke(channel.silence.unsilence)) - - class SilenceNotifier(tasks.Loop): """Loop notifier for posting notices to `alert_channel` containing added channels.""" @@ -85,7 +61,7 @@ class SilenceNotifier(tasks.Loop): await self._alert_channel.send(f"<@&{Roles.moderators}> currently silenced channels: {channels_text}") -class Silence(commands.Cog): +class Silence(Scheduler, commands.Cog): """Commands for stopping channel messages for `verified` role in a channel.""" def __init__(self, bot: Bot): @@ -93,7 +69,22 @@ class Silence(commands.Cog): self.muted_channels = set() self._get_instance_vars_task = self.bot.loop.create_task(self._get_instance_vars()) self._get_instance_vars_event = asyncio.Event() - self.scheduler = UnsilenceScheduler(bot) + super().__init__() + + async def schedule_unsilence(self, channel: SilencedChannel) -> None: + """Schedule expiration for silenced channels.""" + await self.bot.wait_until_guild_available() + log.debug("Scheduling unsilencer") + self.schedule_task(channel.id, channel) + + async def _scheduled_task(self, channel: SilencedChannel) -> None: + """Calls `self.unsilence` on expired silenced channel to unsilence it.""" + await time.wait_until(channel.stop) + log.info("Unsilencing channel after set delay.") + + # Because `self.unsilence` explicitly cancels this scheduled tas, it is shielded + # to avoid prematurely cancelling itself + await asyncio.shield(channel.ctx.invoke(self.unsilence)) async def _get_instance_vars(self) -> None: """Get instance variables after they're available to get from the guild.""" @@ -127,11 +118,10 @@ class Silence(commands.Cog): channel = SilencedChannel( id=ctx.channel.id, ctx=ctx, - silence=self, stop=datetime.datetime.now() + datetime.timedelta(minutes=duration), ) - await self.scheduler.schedule_unsilence(channel) + await self.schedule_unsilence(channel) @commands.command(aliases=("unhush",)) async def unsilence(self, ctx: Context) -> None: @@ -183,7 +173,7 @@ class Silence(commands.Cog): await channel.set_permissions(self._verified_role, **dict(current_overwrite, send_messages=None)) log.info(f"Unsilenced channel #{channel} ({channel.id}).") self.notifier.remove_channel(channel) - self.scheduler.cancel_task(channel.id) + self.cancel_task(channel.id) self.muted_channels.discard(channel) return True log.info(f"Tried to unsilence channel #{channel} ({channel.id}) but the channel was not silenced.") -- cgit v1.2.3 From 699760a3d803c379dad1236f36c919eb0775e490 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sat, 6 Jun 2020 00:49:33 +0200 Subject: Refactor help_channels.py to use RedisCache. More specifically, we're turning three dicts into RedisCaches: - help_channel_claimants - unanswered - claim_times These will still work the same way, but will now persist their contents across restarts. --- bot/cogs/help_channels.py | 60 +++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 28 deletions(-) diff --git a/bot/cogs/help_channels.py b/bot/cogs/help_channels.py index 70cef339a..8c01e5dc4 100644 --- a/bot/cogs/help_channels.py +++ b/bot/cogs/help_channels.py @@ -9,12 +9,14 @@ from contextlib import suppress from datetime import datetime from pathlib import Path +import dateutil import discord import discord.abc from discord.ext import commands from bot import constants from bot.bot import Bot +from bot.utils import RedisCache from bot.utils.checks import with_role_check from bot.utils.scheduling import Scheduler @@ -99,13 +101,24 @@ class HelpChannels(Scheduler, commands.Cog): Help channels are named after the chemical elements in `bot/resources/elements.json`. """ + # This cache tracks which channels are claimed by which members. + # RedisCache[discord.TextChannel.id, t.Union[discord.User.id, discord.Member.id]] + help_channel_claimants = RedisCache() + + # This cache maps a help channel to whether it has had any + # activity other than the original claimant. True being no other + # activity and False being other activity. + # RedisCache[discord.TextChannel.id, bool] + unanswered = RedisCache() + + # This dictionary maps a help channel to the time it was claimed + # RedisCache[discord.TextChannel.id, datetime.datetime] + claim_times = RedisCache() + def __init__(self, bot: Bot): super().__init__() self.bot = bot - self.help_channel_claimants: ( - t.Dict[discord.TextChannel, t.Union[discord.Member, discord.User]] - ) = {} # Categories self.available_category: discord.CategoryChannel = None @@ -125,16 +138,6 @@ class HelpChannels(Scheduler, commands.Cog): self.on_message_lock = asyncio.Lock() self.init_task = self.bot.loop.create_task(self.init_cog()) - # Stats - - # This dictionary maps a help channel to the time it was claimed - self.claim_times: t.Dict[int, datetime] = {} - - # This dictionary maps a help channel to whether it has had any - # activity other than the original claimant. True being no other - # activity and False being other activity. - self.unanswered: t.Dict[int, bool] = {} - def cog_unload(self) -> None: """Cancel the init task and scheduled tasks when the cog unloads.""" log.trace("Cog unload: cancelling the init_cog task") @@ -197,7 +200,7 @@ class HelpChannels(Scheduler, commands.Cog): async def dormant_check(self, ctx: commands.Context) -> bool: """Return True if the user is the help channel claimant or passes the role check.""" - if self.help_channel_claimants.get(ctx.channel) == ctx.author: + if await self.help_channel_claimants.get(ctx.channel.id) == ctx.author.id: log.trace(f"{ctx.author} is the help channel claimant, passing the check for dormant.") self.bot.stats.incr("help.dormant_invoke.claimant") return True @@ -223,7 +226,7 @@ class HelpChannels(Scheduler, commands.Cog): if ctx.channel.category == self.in_use_category: if await self.dormant_check(ctx): with suppress(KeyError): - del self.help_channel_claimants[ctx.channel] + await self.help_channel_claimants.delete(ctx.channel.id) await self.remove_cooldown_role(ctx.author) # Ignore missing task when cooldown has passed but the channel still isn't dormant. @@ -546,13 +549,14 @@ class HelpChannels(Scheduler, commands.Cog): self.bot.stats.incr(f"help.dormant_calls.{caller}") - if channel.id in self.claim_times: - claimed = self.claim_times[channel.id] + if await self.claim_times.contains(channel.id): + claimed_datestring = await self.claim_times.get(channel.id) + claimed = dateutil.parser.parse(claimed_datestring) in_use_time = datetime.now() - claimed self.bot.stats.timing("help.in_use_time", in_use_time) - if channel.id in self.unanswered: - if self.unanswered[channel.id]: + if await self.unanswered.contains(channel.id): + if await self.unanswered.get(channel.id): self.bot.stats.incr("help.sessions.unanswered") else: self.bot.stats.incr("help.sessions.answered") @@ -638,16 +642,16 @@ class HelpChannels(Scheduler, commands.Cog): log.trace(f"Checking if #{channel} ({channel.id}) has been answered.") # Check if there is an entry in unanswered (does not persist across restarts) - if channel.id in self.unanswered: - claimant = self.help_channel_claimants.get(channel) - if not claimant: - # The mapping for this channel was lost, we can't do anything. + if await self.unanswered.contains(channel.id): + claimant_id = await self.help_channel_claimants.get(channel.id) + if not claimant_id: + # The mapping for this channel doesn't exist, we can't do anything. return # Check the message did not come from the claimant - if claimant.id != message.author.id: + if claimant_id != message.author.id: # Mark the channel as answered - self.unanswered[channel.id] = False + await self.unanswered.set(channel.id, False) @commands.Cog.listener() async def on_message(self, message: discord.Message) -> None: @@ -680,12 +684,12 @@ class HelpChannels(Scheduler, commands.Cog): await self.move_to_in_use(channel) await self.revoke_send_permissions(message.author) # Add user with channel for dormant check. - self.help_channel_claimants[channel] = message.author + await self.help_channel_claimants.set(channel.id, message.author.id) self.bot.stats.incr("help.claimed") - self.claim_times[channel.id] = datetime.now() - self.unanswered[channel.id] = True + await self.claim_times.set(channel.id, str(datetime.now())) + await self.unanswered.set(channel.id, True) log.trace(f"Releasing on_message lock for {message.id}.") -- cgit v1.2.3 From fc4eddc7eee3670fdbe5726b13d28ddba57a156b Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sat, 6 Jun 2020 11:57:18 +0200 Subject: Store booleans as integers instead of strings. This means we don't need to rely on strtobool, and is a cleaner implementation overall. Thanks @MarkKoz. --- bot/utils/redis_cache.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index 2926e7a89..347a0e54a 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -2,7 +2,6 @@ from __future__ import annotations import asyncio import logging -from distutils.util import strtobool from functools import partialmethod from typing import Any, Dict, ItemsView, Optional, Tuple, Union @@ -119,9 +118,15 @@ class RedisCache: def _to_typestring(key_or_value: RedisKeyOrValue, prefixes: _PrefixTuple) -> str: """Turn a valid Redis type into a typestring.""" for prefix, _type in prefixes: + # Convert bools into integers before storing them. + if type(key_or_value) is bool: + bool_int = int(key_or_value) + return f"{prefix}{bool_int}" + # isinstance is a bad idea here, because isintance(False, int) == True. if type(key_or_value) is _type: return f"{prefix}{key_or_value}" + raise TypeError(f"RedisCache._to_typestring only supports the following: {prefixes}.") @staticmethod @@ -138,7 +143,7 @@ class RedisCache: # For booleans, we need special handling because bool("False") is True. if prefix == "b|": value = key_or_value[len(prefix):] - return bool(strtobool(value)) + return bool(int(value)) # Otherwise we can just convert normally. return _type(key_or_value[len(prefix):]) -- cgit v1.2.3 From 2d7c8e8238179ece54b388deeeb0734ce330b707 Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Sat, 6 Jun 2020 12:17:24 +0200 Subject: Apply suggestions from review --- bot/cogs/moderation/silence.py | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/bot/cogs/moderation/silence.py b/bot/cogs/moderation/silence.py index 398a70e51..94c560c8e 100644 --- a/bot/cogs/moderation/silence.py +++ b/bot/cogs/moderation/silence.py @@ -1,9 +1,7 @@ import asyncio -import datetime import logging -from collections import namedtuple from contextlib import suppress -from typing import Optional +from typing import Optional, NamedTuple from discord import TextChannel from discord.ext import commands, tasks @@ -12,14 +10,13 @@ from discord.ext.commands import Context from bot.bot import Bot from bot.constants import Channels, Emojis, Guild, MODERATION_ROLES, Roles from bot.converters import HushDurationConverter -from bot.utils import time from bot.utils.checks import with_role_check from bot.utils.scheduling import Scheduler log = logging.getLogger(__name__) -SilencedChannel = namedtuple( - "SilencedChannel", ("id", "ctx", "stop") +SilencedChannel = NamedTuple( + "SilencedChannel", [("ctx", Context), ("delay", int)] ) @@ -65,11 +62,11 @@ class Silence(Scheduler, commands.Cog): """Commands for stopping channel messages for `verified` role in a channel.""" def __init__(self, bot: Bot): + super().__init__() self.bot = bot self.muted_channels = set() self._get_instance_vars_task = self.bot.loop.create_task(self._get_instance_vars()) self._get_instance_vars_event = asyncio.Event() - super().__init__() async def schedule_unsilence(self, channel: SilencedChannel) -> None: """Schedule expiration for silenced channels.""" @@ -79,10 +76,10 @@ class Silence(Scheduler, commands.Cog): async def _scheduled_task(self, channel: SilencedChannel) -> None: """Calls `self.unsilence` on expired silenced channel to unsilence it.""" - await time.wait_until(channel.stop) + await asyncio.sleep(channel.delay) log.info("Unsilencing channel after set delay.") - # Because `self.unsilence` explicitly cancels this scheduled tas, it is shielded + # Because `self.unsilence` explicitly cancels this scheduled task, it is shielded # to avoid prematurely cancelling itself await asyncio.shield(channel.ctx.invoke(self.unsilence)) @@ -116,12 +113,11 @@ class Silence(Scheduler, commands.Cog): await ctx.send(f"{Emojis.check_mark} silenced current channel for {duration} minute(s).") channel = SilencedChannel( - id=ctx.channel.id, ctx=ctx, - stop=datetime.datetime.now() + datetime.timedelta(minutes=duration), + stop=duration*60, ) - await self.schedule_unsilence(channel) + await self.schedule_task(ctx.channel.id, channel) @commands.command(aliases=("unhush",)) async def unsilence(self, ctx: Context) -> None: @@ -134,9 +130,8 @@ class Silence(Scheduler, commands.Cog): log.debug(f"Unsilencing channel #{ctx.channel} from {ctx.author}'s command.") if not await self._unsilence(ctx.channel): await ctx.send(f"{Emojis.cross_mark} current channel is not silenced.") - return - - await ctx.send(f"{Emojis.check_mark} unsilenced current channel.") + else: + await ctx.send(f"{Emojis.check_mark} unsilenced current channel.") async def _silence(self, channel: TextChannel, persistent: bool, duration: Optional[int]) -> bool: """ -- cgit v1.2.3 From 1859ae7a43bc794b735c3445b1873420da5a7001 Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Sat, 6 Jun 2020 12:20:26 +0200 Subject: Fix import order --- bot/cogs/moderation/silence.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/cogs/moderation/silence.py b/bot/cogs/moderation/silence.py index 94c560c8e..c451cea0b 100644 --- a/bot/cogs/moderation/silence.py +++ b/bot/cogs/moderation/silence.py @@ -1,7 +1,7 @@ import asyncio import logging from contextlib import suppress -from typing import Optional, NamedTuple +from typing import NamedTuple, Optional from discord import TextChannel from discord.ext import commands, tasks -- cgit v1.2.3 From 3c3c8c210507170a2502a4906265af6a2b2525ac Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Sat, 6 Jun 2020 12:26:14 +0200 Subject: Remove unnecessary schedule_unsilence - As suggested, this function is not necessary - Also fixed no longer valid`stop`in SilencedChannel NamedTuple --- bot/cogs/moderation/silence.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/bot/cogs/moderation/silence.py b/bot/cogs/moderation/silence.py index c451cea0b..8223df491 100644 --- a/bot/cogs/moderation/silence.py +++ b/bot/cogs/moderation/silence.py @@ -68,12 +68,6 @@ class Silence(Scheduler, commands.Cog): self._get_instance_vars_task = self.bot.loop.create_task(self._get_instance_vars()) self._get_instance_vars_event = asyncio.Event() - async def schedule_unsilence(self, channel: SilencedChannel) -> None: - """Schedule expiration for silenced channels.""" - await self.bot.wait_until_guild_available() - log.debug("Scheduling unsilencer") - self.schedule_task(channel.id, channel) - async def _scheduled_task(self, channel: SilencedChannel) -> None: """Calls `self.unsilence` on expired silenced channel to unsilence it.""" await asyncio.sleep(channel.delay) @@ -114,7 +108,7 @@ class Silence(Scheduler, commands.Cog): channel = SilencedChannel( ctx=ctx, - stop=duration*60, + delay=duration*60, ) await self.schedule_task(ctx.channel.id, channel) -- cgit v1.2.3 From 858e2e301234862e66ce03ccd71f46518dcc953f Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Sat, 6 Jun 2020 12:31:00 +0200 Subject: Do not await self.schedule_task - self.schedule_task shouldn't be awaited as it isn't a coroutine --- bot/cogs/moderation/silence.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/cogs/moderation/silence.py b/bot/cogs/moderation/silence.py index 8223df491..13f84009f 100644 --- a/bot/cogs/moderation/silence.py +++ b/bot/cogs/moderation/silence.py @@ -111,7 +111,7 @@ class Silence(Scheduler, commands.Cog): delay=duration*60, ) - await self.schedule_task(ctx.channel.id, channel) + self.schedule_task(ctx.channel.id, channel) @commands.command(aliases=("unhush",)) async def unsilence(self, ctx: Context) -> None: -- cgit v1.2.3 From 94f096fab3bde10ba0da767c568c7a8c3ff3259f Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sat, 6 Jun 2020 12:33:06 +0200 Subject: Store epoch timestamps instead of strings. We're also switching from datetime.now() to datetime.utcnow(). --- bot/cogs/help_channels.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/bot/cogs/help_channels.py b/bot/cogs/help_channels.py index 8c01e5dc4..dd3e3cb8b 100644 --- a/bot/cogs/help_channels.py +++ b/bot/cogs/help_channels.py @@ -9,7 +9,6 @@ from contextlib import suppress from datetime import datetime from pathlib import Path -import dateutil import discord import discord.abc from discord.ext import commands @@ -550,9 +549,9 @@ class HelpChannels(Scheduler, commands.Cog): self.bot.stats.incr(f"help.dormant_calls.{caller}") if await self.claim_times.contains(channel.id): - claimed_datestring = await self.claim_times.get(channel.id) - claimed = dateutil.parser.parse(claimed_datestring) - in_use_time = datetime.now() - claimed + claimed_timestamp = await self.claim_times.get(channel.id) + claimed = datetime.fromtimestamp(claimed_timestamp) + in_use_time = datetime.utcnow() - claimed self.bot.stats.timing("help.in_use_time", in_use_time) if await self.unanswered.contains(channel.id): @@ -688,7 +687,7 @@ class HelpChannels(Scheduler, commands.Cog): self.bot.stats.incr("help.claimed") - await self.claim_times.set(channel.id, str(datetime.now())) + await self.claim_times.set(channel.id, datetime.utcnow().timestamp()) await self.unanswered.set(channel.id, True) log.trace(f"Releasing on_message lock for {message.id}.") -- cgit v1.2.3 From 40a774e0bb6ed8947a17fe0116e2f1dc0cf89156 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sat, 6 Jun 2020 12:37:01 +0200 Subject: Fix potential race condition. Instead of first checking if the channel.id exists and then checking what it is, we just do a single API call, to prevent cases where something fucky might happen inbetween the first and the second call. --- bot/cogs/help_channels.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/bot/cogs/help_channels.py b/bot/cogs/help_channels.py index dd3e3cb8b..01c38b408 100644 --- a/bot/cogs/help_channels.py +++ b/bot/cogs/help_channels.py @@ -548,20 +548,20 @@ class HelpChannels(Scheduler, commands.Cog): self.bot.stats.incr(f"help.dormant_calls.{caller}") - if await self.claim_times.contains(channel.id): - claimed_timestamp = await self.claim_times.get(channel.id) + claimed_timestamp = await self.claim_times.get(channel.id) + if claimed_timestamp: claimed = datetime.fromtimestamp(claimed_timestamp) in_use_time = datetime.utcnow() - claimed self.bot.stats.timing("help.in_use_time", in_use_time) - if await self.unanswered.contains(channel.id): - if await self.unanswered.get(channel.id): + unanswered = await self.unanswered.get(channel.id) + if unanswered is not None: + if unanswered: self.bot.stats.incr("help.sessions.unanswered") else: self.bot.stats.incr("help.sessions.answered") log.trace(f"Position of #{channel} ({channel.id}) is actually {channel.position}.") - log.trace(f"Sending dormant message for #{channel} ({channel.id}).") embed = discord.Embed(description=DORMANT_MSG) await channel.send(embed=embed) -- cgit v1.2.3 From 6e34de6397e6ab7d23c6e6abb74a2156375b2c2d Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Sat, 6 Jun 2020 21:29:33 +0200 Subject: Move cancel_task before notifier.remove_channel - as sugested notifier.remove_channel and muted_channels.discard should be together --- bot/cogs/moderation/silence.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/cogs/moderation/silence.py b/bot/cogs/moderation/silence.py index 13f84009f..d5b9621d2 100644 --- a/bot/cogs/moderation/silence.py +++ b/bot/cogs/moderation/silence.py @@ -161,8 +161,8 @@ class Silence(Scheduler, commands.Cog): if current_overwrite.send_messages is False: await channel.set_permissions(self._verified_role, **dict(current_overwrite, send_messages=None)) log.info(f"Unsilenced channel #{channel} ({channel.id}).") - self.notifier.remove_channel(channel) self.cancel_task(channel.id) + self.notifier.remove_channel(channel) self.muted_channels.discard(channel) return True log.info(f"Tried to unsilence channel #{channel} ({channel.id}) but the channel was not silenced.") -- cgit v1.2.3 From 27e269b874fcd038388031959186ffe682c777c0 Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Sat, 6 Jun 2020 21:36:02 +0200 Subject: Change `is` to `was` for unsilenced channel message - As suggested, `was` is more fitting in the message than `is` --- bot/cogs/moderation/silence.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/cogs/moderation/silence.py b/bot/cogs/moderation/silence.py index d5b9621d2..08f3973b0 100644 --- a/bot/cogs/moderation/silence.py +++ b/bot/cogs/moderation/silence.py @@ -123,7 +123,7 @@ class Silence(Scheduler, commands.Cog): await self._get_instance_vars_event.wait() log.debug(f"Unsilencing channel #{ctx.channel} from {ctx.author}'s command.") if not await self._unsilence(ctx.channel): - await ctx.send(f"{Emojis.cross_mark} current channel is not silenced.") + await ctx.send(f"{Emojis.cross_mark} current channel was not silenced.") else: await ctx.send(f"{Emojis.check_mark} unsilenced current channel.") -- cgit v1.2.3 From be4902cbd66c2f7223608ddbfee4aa4f0e1a011a Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Sat, 6 Jun 2020 22:11:53 +0200 Subject: Test for channel not silenced message --- tests/bot/cogs/moderation/test_silence.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/tests/bot/cogs/moderation/test_silence.py b/tests/bot/cogs/moderation/test_silence.py index 3fd149f04..ab3d0742a 100644 --- a/tests/bot/cogs/moderation/test_silence.py +++ b/tests/bot/cogs/moderation/test_silence.py @@ -127,10 +127,20 @@ class SilenceTests(unittest.IsolatedAsyncioTestCase): self.ctx.reset_mock() async def test_unsilence_sent_correct_discord_message(self): - """Proper reply after a successful unsilence.""" - with mock.patch.object(self.cog, "_unsilence", return_value=True): - await self.cog.unsilence.callback(self.cog, self.ctx) - self.ctx.send.assert_called_once_with(f"{Emojis.check_mark} unsilenced current channel.") + """Check if proper message was sent when unsilencing channel.""" + test_cases = ( + (True, f"{Emojis.check_mark} unsilenced current channel."), + (False, f"{Emojis.cross_mark} current channel was not silenced.") + ) + for _unsilence_patch_return, result_message in test_cases: + with self.subTest( + starting_silenced_state=_unsilence_patch_return, + result_message=result_message + ): + with mock.patch.object(self.cog, "_unsilence", return_value=_unsilence_patch_return): + await self.cog.unsilence.callback(self.cog, self.ctx) + self.ctx.send.assert_called_once_with(result_message) + self.ctx.reset_mock() async def test_silence_private_for_false(self): """Permissions are not set and `False` is returned in an already silenced channel.""" -- cgit v1.2.3 From 73f258488d32be6c00aaea2cfa20ff2b24d48b30 Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Sat, 6 Jun 2020 23:41:59 +0200 Subject: Use class instead of NamedTuple - Using a class is more readable than using a NamedTuple --- bot/cogs/moderation/silence.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/bot/cogs/moderation/silence.py b/bot/cogs/moderation/silence.py index 08f3973b0..c8ab6443b 100644 --- a/bot/cogs/moderation/silence.py +++ b/bot/cogs/moderation/silence.py @@ -15,9 +15,12 @@ from bot.utils.scheduling import Scheduler log = logging.getLogger(__name__) -SilencedChannel = NamedTuple( - "SilencedChannel", [("ctx", Context), ("delay", int)] -) + +class TaskData(NamedTuple): + """Data for a scheduled task.""" + + delay: int + ctx: Context class SilenceNotifier(tasks.Loop): @@ -68,14 +71,14 @@ class Silence(Scheduler, commands.Cog): self._get_instance_vars_task = self.bot.loop.create_task(self._get_instance_vars()) self._get_instance_vars_event = asyncio.Event() - async def _scheduled_task(self, channel: SilencedChannel) -> None: + async def _scheduled_task(self, task: TaskData) -> None: """Calls `self.unsilence` on expired silenced channel to unsilence it.""" - await asyncio.sleep(channel.delay) + await asyncio.sleep(task.delay) log.info("Unsilencing channel after set delay.") # Because `self.unsilence` explicitly cancels this scheduled task, it is shielded # to avoid prematurely cancelling itself - await asyncio.shield(channel.ctx.invoke(self.unsilence)) + await asyncio.shield(task.ctx.invoke(self.unsilence)) async def _get_instance_vars(self) -> None: """Get instance variables after they're available to get from the guild.""" @@ -106,12 +109,12 @@ class Silence(Scheduler, commands.Cog): await ctx.send(f"{Emojis.check_mark} silenced current channel for {duration} minute(s).") - channel = SilencedChannel( - ctx=ctx, + task_data = TaskData( delay=duration*60, + ctx=ctx ) - self.schedule_task(ctx.channel.id, channel) + self.schedule_task(ctx.channel.id, task_data) @commands.command(aliases=("unhush",)) async def unsilence(self, ctx: Context) -> None: -- cgit v1.2.3 From 8570bd2c9d644c82e69e4c3bbae3af24f95180e2 Mon Sep 17 00:00:00 2001 From: Daniel Nash <22755628+crazygmr101@users.noreply.github.com> Date: Sun, 7 Jun 2020 03:15:51 -0500 Subject: Create cooldown.md --- bot/resources/tags/cooldown.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 bot/resources/tags/cooldown.md diff --git a/bot/resources/tags/cooldown.md b/bot/resources/tags/cooldown.md new file mode 100644 index 000000000..a4e237872 --- /dev/null +++ b/bot/resources/tags/cooldown.md @@ -0,0 +1,22 @@ +**Cooldowns** + +Cooldowns are used in discord.py to rate-limit. + +```python +from discord.ext import commands + +class SomeCog(commands.Cog): + def __init__(self): + self._cd = commands.CooldownMapping.from_cooldown(1.0, 60.0, commands.BucketType.user) + + async def cog_check(self, ctx): + bucket = self._cd.get_bucket(ctx.message) + retry_after = bucket.update_rate_limit() + if retry_after: + # you're rate limited + # helpful message here + pass + # you're not rate limited +``` + +`from_cooldown` takes the amount of `update_rate_limit()`s needed to trigger the cooldown, the time in which the cooldown is triggered, and a [`BucketType`](discordpy.readthedocs.io/en/latest/ext/commands/api.html#discord.discord.ext.commands.BucketType). -- cgit v1.2.3 From 97710d5bb8145d10983187bebe554b845a9c0ef1 Mon Sep 17 00:00:00 2001 From: Daniel Nash <22755628+crazygmr101@users.noreply.github.com> Date: Sun, 7 Jun 2020 03:40:59 -0500 Subject: Update cooldown.md --- bot/resources/tags/cooldown.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/resources/tags/cooldown.md b/bot/resources/tags/cooldown.md index a4e237872..3d34c078b 100644 --- a/bot/resources/tags/cooldown.md +++ b/bot/resources/tags/cooldown.md @@ -1,7 +1,7 @@ **Cooldowns** Cooldowns are used in discord.py to rate-limit. - + ```python from discord.ext import commands -- cgit v1.2.3 From 15dbbcf865dd24f5f8697fd85bd60d53d9450fcf Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sun, 7 Jun 2020 12:37:31 +0200 Subject: Remove pointless suppress. Since help_channel_claimants.delete will never raise a KeyError, it's not necessary to suppress one. --- bot/cogs/help_channels.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bot/cogs/help_channels.py b/bot/cogs/help_channels.py index 01c38b408..e521e3301 100644 --- a/bot/cogs/help_channels.py +++ b/bot/cogs/help_channels.py @@ -5,7 +5,6 @@ import logging import random import typing as t from collections import deque -from contextlib import suppress from datetime import datetime from pathlib import Path @@ -224,10 +223,11 @@ class HelpChannels(Scheduler, commands.Cog): log.trace("close command invoked; checking if the channel is in-use.") if ctx.channel.category == self.in_use_category: if await self.dormant_check(ctx): - with suppress(KeyError): - await self.help_channel_claimants.delete(ctx.channel.id) + # Remove the claimant and the cooldown role + await self.help_channel_claimants.delete(ctx.channel.id) await self.remove_cooldown_role(ctx.author) + # Ignore missing task when cooldown has passed but the channel still isn't dormant. self.cancel_task(ctx.author.id, ignore_missing=True) -- cgit v1.2.3 From 378ef81383050cf4c477afc2c23abb51b700ea68 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 8 Jun 2020 18:41:25 -0700 Subject: Help channels: fix claim timestamp being local MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The datetime module returns a local timestamp for naïve datetimes. It has to be timezone-aware to ensure it will always be in UTC. --- bot/cogs/help_channels.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/bot/cogs/help_channels.py b/bot/cogs/help_channels.py index e521e3301..40e625338 100644 --- a/bot/cogs/help_channels.py +++ b/bot/cogs/help_channels.py @@ -5,7 +5,7 @@ import logging import random import typing as t from collections import deque -from datetime import datetime +from datetime import datetime, timezone from pathlib import Path import discord @@ -110,7 +110,7 @@ class HelpChannels(Scheduler, commands.Cog): unanswered = RedisCache() # This dictionary maps a help channel to the time it was claimed - # RedisCache[discord.TextChannel.id, datetime.datetime] + # RedisCache[discord.TextChannel.id, UtcPosixTimestamp] claim_times = RedisCache() def __init__(self, bot: Bot): @@ -550,7 +550,7 @@ class HelpChannels(Scheduler, commands.Cog): claimed_timestamp = await self.claim_times.get(channel.id) if claimed_timestamp: - claimed = datetime.fromtimestamp(claimed_timestamp) + claimed = datetime.utcfromtimestamp(claimed_timestamp) in_use_time = datetime.utcnow() - claimed self.bot.stats.timing("help.in_use_time", in_use_time) @@ -687,7 +687,10 @@ class HelpChannels(Scheduler, commands.Cog): self.bot.stats.incr("help.claimed") - await self.claim_times.set(channel.id, datetime.utcnow().timestamp()) + # Must use a timezone-aware datetime to ensure a correct POSIX timestamp. + timestamp = datetime.now(timezone.utc).timestamp() + await self.claim_times.set(channel.id, timestamp) + await self.unanswered.set(channel.id, True) log.trace(f"Releasing on_message lock for {message.id}.") -- cgit v1.2.3 From 62cb3f7d9ecd861a13b594f3c63aed83dead2e0e Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 8 Jun 2020 19:01:58 -0700 Subject: Help channels: add a function to get in use time Future code will also need to get this time, so moving it out to a separate function reduces redundancy. --- bot/cogs/help_channels.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/bot/cogs/help_channels.py b/bot/cogs/help_channels.py index 40e625338..13dee8e80 100644 --- a/bot/cogs/help_channels.py +++ b/bot/cogs/help_channels.py @@ -5,7 +5,7 @@ import logging import random import typing as t from collections import deque -from datetime import datetime, timezone +from datetime import datetime, timedelta, timezone from pathlib import Path import discord @@ -286,6 +286,15 @@ class HelpChannels(Scheduler, commands.Cog): if channel.category_id == category.id and not self.is_excluded_channel(channel): yield channel + async def get_in_use_time(self, channel_id: int) -> t.Optional[timedelta]: + """Return the duration `channel_id` has been in use. Return None if it's not in use.""" + log.trace(f"Calculating in use time for channel {channel_id}.") + + claimed_timestamp = await self.claim_times.get(channel_id) + if claimed_timestamp: + claimed = datetime.utcfromtimestamp(claimed_timestamp) + return datetime.utcnow() - claimed + @staticmethod def get_names() -> t.List[str]: """ @@ -548,10 +557,8 @@ class HelpChannels(Scheduler, commands.Cog): self.bot.stats.incr(f"help.dormant_calls.{caller}") - claimed_timestamp = await self.claim_times.get(channel.id) - if claimed_timestamp: - claimed = datetime.utcfromtimestamp(claimed_timestamp) - in_use_time = datetime.utcnow() - claimed + in_use_time = await self.get_in_use_time(channel.id) + if in_use_time: self.bot.stats.timing("help.in_use_time", in_use_time) unanswered = await self.unanswered.get(channel.id) -- cgit v1.2.3 From 04b37cd054565368e5af42deec5d5ca14fc94199 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 8 Jun 2020 19:37:37 -0700 Subject: Help channels: add a function to schedule cooldown expiration Moving this code into a separate function reduces redundancy down the line. This will also get used to re-scheduled cooldowns after a restart. --- bot/cogs/help_channels.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/bot/cogs/help_channels.py b/bot/cogs/help_channels.py index 13dee8e80..f2785c932 100644 --- a/bot/cogs/help_channels.py +++ b/bot/cogs/help_channels.py @@ -794,11 +794,14 @@ class HelpChannels(Scheduler, commands.Cog): # Would mean the user somehow bypassed the lack of permissions (e.g. user is guild owner). self.cancel_task(member.id, ignore_missing=True) - timeout = constants.HelpChannels.claim_minutes * 60 - callback = self.remove_cooldown_role(member) + await self.schedule_cooldown_expiration(member, constants.HelpChannels.claim_minutes * 60) + + async def schedule_cooldown_expiration(self, member: discord.Member, seconds: int) -> None: + """Schedule the cooldown role for `member` to be removed after a duration of `seconds`.""" + log.trace(f"Scheduling removal of {member}'s ({member.id}) cooldown.") - log.trace(f"Scheduling {member}'s ({member.id}) send message permissions to be reinstated.") - self.schedule_task(member.id, TaskData(timeout, callback)) + callback = self.remove_cooldown_role(member) + self.schedule_task(member.id, TaskData(seconds, callback)) async def send_available_message(self, channel: discord.TextChannel) -> None: """Send the available message by editing a dormant message or sending a new message.""" -- cgit v1.2.3 From b49f3e5f4e707dece2de38882be44405563d82e4 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 8 Jun 2020 19:52:46 -0700 Subject: Help channels: use cache to remove cooldowns or re-schedule them Using the cache is more efficient since it can check only the users it expects to have a cooldown rather than searching all guild members. Furthermore, re-scheduling the cooldowns ensures members experience the full duration of the cooldown. Previously, all cooldowns were removed, regardless of whether they were expired. --- bot/cogs/help_channels.py | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/bot/cogs/help_channels.py b/bot/cogs/help_channels.py index f2785c932..098634e96 100644 --- a/bot/cogs/help_channels.py +++ b/bot/cogs/help_channels.py @@ -397,7 +397,7 @@ class HelpChannels(Scheduler, commands.Cog): log.trace("Initialising the cog.") await self.init_categories() - await self.reset_send_permissions() + await self.check_cooldowns() self.channel_queue = self.create_channel_queue() self.name_queue = self.create_name_queue() @@ -733,15 +733,28 @@ class HelpChannels(Scheduler, commands.Cog): msg = await self.get_last_message(channel) return self.match_bot_embed(msg, AVAILABLE_MSG) - async def reset_send_permissions(self) -> None: - """Reset send permissions in the Available category for claimants.""" - log.trace("Resetting send permissions in the Available category.") + async def check_cooldowns(self) -> None: + """Remove expired cooldowns and re-schedule active ones.""" + log.trace("Checking all cooldowns to remove or re-schedule them.") guild = self.bot.get_guild(constants.Guild.id) + cooldown = constants.HelpChannels.claim_minutes * 60 - # TODO: replace with a persistent cache cause checking every member is quite slow - for member in guild.members: - if self.is_claimant(member): + for channel_id, member_id in await self.help_channel_claimants.items(): + member = guild.get_member(member_id) + if not member: + continue # Member probably left the guild. + + in_use_time = await self.get_in_use_time(channel_id) + + if not in_use_time or in_use_time.seconds > cooldown: + # Remove the role if no claim time could be retrieved or if the cooldown expired. + # Since the channel is in the claimants cache, it is definitely strange for a time + # to not exist. However, it isn't a reason to keep the user stuck with a cooldown. await self.remove_cooldown_role(member) + else: + # The member is still on a cooldown; re-schedule it for the remaining time. + remaining = cooldown - in_use_time.seconds + await self.schedule_cooldown_expiration(member, remaining) async def add_cooldown_role(self, member: discord.Member) -> None: """Add the help cooldown role to `member`.""" -- cgit v1.2.3 From be78a86abea68e05ac80c8a07085cb7f0fa6d3c1 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 8 Jun 2020 20:11:38 -0700 Subject: Help channels: revise inaccurate comment --- bot/cogs/help_channels.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/cogs/help_channels.py b/bot/cogs/help_channels.py index 098634e96..86579e940 100644 --- a/bot/cogs/help_channels.py +++ b/bot/cogs/help_channels.py @@ -647,7 +647,7 @@ class HelpChannels(Scheduler, commands.Cog): if self.is_in_category(channel, constants.Categories.help_in_use): log.trace(f"Checking if #{channel} ({channel.id}) has been answered.") - # Check if there is an entry in unanswered (does not persist across restarts) + # Check if there is an entry in unanswered if await self.unanswered.contains(channel.id): claimant_id = await self.help_channel_claimants.get(channel.id) if not claimant_id: -- cgit v1.2.3 From 06d8ab2f7203d4ee92a040444bbb1999a36accb3 Mon Sep 17 00:00:00 2001 From: Daniel Nash Date: Wed, 10 Jun 2020 15:49:14 -0500 Subject: Rename to customcooldown.md --- bot/resources/tags/cooldown.md | 22 ---------------------- bot/resources/tags/customcooldown.md | 22 ++++++++++++++++++++++ 2 files changed, 22 insertions(+), 22 deletions(-) delete mode 100644 bot/resources/tags/cooldown.md create mode 100644 bot/resources/tags/customcooldown.md diff --git a/bot/resources/tags/cooldown.md b/bot/resources/tags/cooldown.md deleted file mode 100644 index 3d34c078b..000000000 --- a/bot/resources/tags/cooldown.md +++ /dev/null @@ -1,22 +0,0 @@ -**Cooldowns** - -Cooldowns are used in discord.py to rate-limit. - -```python -from discord.ext import commands - -class SomeCog(commands.Cog): - def __init__(self): - self._cd = commands.CooldownMapping.from_cooldown(1.0, 60.0, commands.BucketType.user) - - async def cog_check(self, ctx): - bucket = self._cd.get_bucket(ctx.message) - retry_after = bucket.update_rate_limit() - if retry_after: - # you're rate limited - # helpful message here - pass - # you're not rate limited -``` - -`from_cooldown` takes the amount of `update_rate_limit()`s needed to trigger the cooldown, the time in which the cooldown is triggered, and a [`BucketType`](discordpy.readthedocs.io/en/latest/ext/commands/api.html#discord.discord.ext.commands.BucketType). diff --git a/bot/resources/tags/customcooldown.md b/bot/resources/tags/customcooldown.md new file mode 100644 index 000000000..3d34c078b --- /dev/null +++ b/bot/resources/tags/customcooldown.md @@ -0,0 +1,22 @@ +**Cooldowns** + +Cooldowns are used in discord.py to rate-limit. + +```python +from discord.ext import commands + +class SomeCog(commands.Cog): + def __init__(self): + self._cd = commands.CooldownMapping.from_cooldown(1.0, 60.0, commands.BucketType.user) + + async def cog_check(self, ctx): + bucket = self._cd.get_bucket(ctx.message) + retry_after = bucket.update_rate_limit() + if retry_after: + # you're rate limited + # helpful message here + pass + # you're not rate limited +``` + +`from_cooldown` takes the amount of `update_rate_limit()`s needed to trigger the cooldown, the time in which the cooldown is triggered, and a [`BucketType`](discordpy.readthedocs.io/en/latest/ext/commands/api.html#discord.discord.ext.commands.BucketType). -- cgit v1.2.3 From b71ff0f2bbd7be64d1a0009b9e6530ba3c179926 Mon Sep 17 00:00:00 2001 From: Daniel Nash Date: Wed, 10 Jun 2020 15:53:33 -0500 Subject: Update example to not be in a cog --- bot/resources/tags/customcooldown.md | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/bot/resources/tags/customcooldown.md b/bot/resources/tags/customcooldown.md index 3d34c078b..35f28a1e5 100644 --- a/bot/resources/tags/customcooldown.md +++ b/bot/resources/tags/customcooldown.md @@ -1,22 +1,20 @@ **Cooldowns** -Cooldowns are used in discord.py to rate-limit. +Cooldowns can be used in discord.py to rate-limit. In this example, we're using it in an on_message. ```python from discord.ext import commands -class SomeCog(commands.Cog): - def __init__(self): - self._cd = commands.CooldownMapping.from_cooldown(1.0, 60.0, commands.BucketType.user) +_cd = commands.CooldownMapping.from_cooldown(1.0, 60.0, commands.BucketType.user) - async def cog_check(self, ctx): - bucket = self._cd.get_bucket(ctx.message) - retry_after = bucket.update_rate_limit() - if retry_after: - # you're rate limited - # helpful message here - pass - # you're not rate limited +@bot.event +async def on_message(message): + bucket = _cd.get_bucket(message) + retry_after = bucket.update_rate_limit() + if retry_after: + await message.channel.send("Slow down! You're sending messages too fast") + pass + # you're not rate limited ``` `from_cooldown` takes the amount of `update_rate_limit()`s needed to trigger the cooldown, the time in which the cooldown is triggered, and a [`BucketType`](discordpy.readthedocs.io/en/latest/ext/commands/api.html#discord.discord.ext.commands.BucketType). -- cgit v1.2.3 From b2195f1990c6720dad6819cc118920b89e24beba Mon Sep 17 00:00:00 2001 From: Daniel Nash Date: Wed, 10 Jun 2020 15:56:00 -0500 Subject: Move the not rate-limited message into else --- bot/resources/tags/customcooldown.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bot/resources/tags/customcooldown.md b/bot/resources/tags/customcooldown.md index 35f28a1e5..b44d6b12f 100644 --- a/bot/resources/tags/customcooldown.md +++ b/bot/resources/tags/customcooldown.md @@ -13,8 +13,10 @@ async def on_message(message): retry_after = bucket.update_rate_limit() if retry_after: await message.channel.send("Slow down! You're sending messages too fast") + else: + # you're not rate limited pass - # you're not rate limited + # more code here ``` `from_cooldown` takes the amount of `update_rate_limit()`s needed to trigger the cooldown, the time in which the cooldown is triggered, and a [`BucketType`](discordpy.readthedocs.io/en/latest/ext/commands/api.html#discord.discord.ext.commands.BucketType). -- cgit v1.2.3 From 76da8f09c7c8d62f8451bc561f020f489b3a0970 Mon Sep 17 00:00:00 2001 From: Daniel Nash <22755628+crazygmr101@users.noreply.github.com> Date: Wed, 10 Jun 2020 17:09:17 -0500 Subject: change _cd to message_cooldown Apply suggestions from code review Co-authored-by: Joseph Banks --- bot/resources/tags/customcooldown.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bot/resources/tags/customcooldown.md b/bot/resources/tags/customcooldown.md index b44d6b12f..f304de246 100644 --- a/bot/resources/tags/customcooldown.md +++ b/bot/resources/tags/customcooldown.md @@ -5,11 +5,11 @@ Cooldowns can be used in discord.py to rate-limit. In this example, we're using ```python from discord.ext import commands -_cd = commands.CooldownMapping.from_cooldown(1.0, 60.0, commands.BucketType.user) +message_cooldown = commands.CooldownMapping.from_cooldown(1.0, 60.0, commands.BucketType.user) @bot.event async def on_message(message): - bucket = _cd.get_bucket(message) + bucket = message_cooldown.get_bucket(message) retry_after = bucket.update_rate_limit() if retry_after: await message.channel.send("Slow down! You're sending messages too fast") -- cgit v1.2.3 From b1b1765596469b76d6321e81732a54e46d3b5865 Mon Sep 17 00:00:00 2001 From: Daniel Nash <22755628+crazygmr101@users.noreply.github.com> Date: Wed, 10 Jun 2020 17:35:44 -0500 Subject: Update bot/resources/tags/customcooldown.md Co-authored-by: Joseph Banks --- bot/resources/tags/customcooldown.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/bot/resources/tags/customcooldown.md b/bot/resources/tags/customcooldown.md index f304de246..f7987556d 100644 --- a/bot/resources/tags/customcooldown.md +++ b/bot/resources/tags/customcooldown.md @@ -14,9 +14,7 @@ async def on_message(message): if retry_after: await message.channel.send("Slow down! You're sending messages too fast") else: - # you're not rate limited - pass - # more code here + await message.channel.send("Not ratelimited!") ``` `from_cooldown` takes the amount of `update_rate_limit()`s needed to trigger the cooldown, the time in which the cooldown is triggered, and a [`BucketType`](discordpy.readthedocs.io/en/latest/ext/commands/api.html#discord.discord.ext.commands.BucketType). -- cgit v1.2.3 From ccd3a17d6a8723cabb21a4b2c7e0d8a835dc9e99 Mon Sep 17 00:00:00 2001 From: Daniel Nash <22755628+crazygmr101@users.noreply.github.com> Date: Fri, 12 Jun 2020 08:42:30 -0500 Subject: Make title more specific Co-authored-by: Mark --- bot/resources/tags/customcooldown.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/resources/tags/customcooldown.md b/bot/resources/tags/customcooldown.md index f7987556d..4060a9827 100644 --- a/bot/resources/tags/customcooldown.md +++ b/bot/resources/tags/customcooldown.md @@ -1,4 +1,4 @@ -**Cooldowns** +**Cooldowns in discord.py** Cooldowns can be used in discord.py to rate-limit. In this example, we're using it in an on_message. -- cgit v1.2.3 From 844726f7bedc8c1d772c5f866a29a52cda6f3a9f Mon Sep 17 00:00:00 2001 From: Daniel Nash <22755628+crazygmr101@users.noreply.github.com> Date: Fri, 12 Jun 2020 08:48:16 -0500 Subject: Update customcooldown.md --- bot/resources/tags/customcooldown.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bot/resources/tags/customcooldown.md b/bot/resources/tags/customcooldown.md index 4060a9827..3f6db0ec6 100644 --- a/bot/resources/tags/customcooldown.md +++ b/bot/resources/tags/customcooldown.md @@ -10,6 +10,8 @@ message_cooldown = commands.CooldownMapping.from_cooldown(1.0, 60.0, commands.Bu @bot.event async def on_message(message): bucket = message_cooldown.get_bucket(message) + # update_rate_limit returns a time you need to wait before + # trying again retry_after = bucket.update_rate_limit() if retry_after: await message.channel.send("Slow down! You're sending messages too fast") -- cgit v1.2.3 From 08db3df18711ff57a56fef99d0ad725669448f3b Mon Sep 17 00:00:00 2001 From: Daniel Nash <22755628+crazygmr101@users.noreply.github.com> Date: Fri, 12 Jun 2020 08:50:30 -0500 Subject: Add scheme to URL --- bot/resources/tags/customcooldown.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/resources/tags/customcooldown.md b/bot/resources/tags/customcooldown.md index 3f6db0ec6..e877e4dae 100644 --- a/bot/resources/tags/customcooldown.md +++ b/bot/resources/tags/customcooldown.md @@ -19,4 +19,4 @@ async def on_message(message): await message.channel.send("Not ratelimited!") ``` -`from_cooldown` takes the amount of `update_rate_limit()`s needed to trigger the cooldown, the time in which the cooldown is triggered, and a [`BucketType`](discordpy.readthedocs.io/en/latest/ext/commands/api.html#discord.discord.ext.commands.BucketType). +`from_cooldown` takes the amount of `update_rate_limit()`s needed to trigger the cooldown, the time in which the cooldown is triggered, and a [`BucketType`](https://discordpy.readthedocs.io/en/latest/ext/commands/api.html#discord.discord.ext.commands.BucketType). -- cgit v1.2.3 From 46e770ba772e3c7048903efff41a5b969717e0d4 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Fri, 12 Jun 2020 15:32:25 -0700 Subject: Escape markdown in charinfo embed The embed displays the original character. If it's a markdown char, it would interfere with the embed's actual markdown. The backtick was especially troublesome. Fixes #996 --- bot/cogs/utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bot/cogs/utils.py b/bot/cogs/utils.py index 73b4a1c0a..697bf60ce 100644 --- a/bot/cogs/utils.py +++ b/bot/cogs/utils.py @@ -6,7 +6,7 @@ from email.parser import HeaderParser from io import StringIO from typing import Tuple, Union -from discord import Colour, Embed +from discord import Colour, Embed, utils from discord.ext.commands import BadArgument, Cog, Context, command from bot.bot import Bot @@ -145,7 +145,7 @@ class Utils(Cog): u_code = f"\\U{digit:>08}" url = f"https://www.compart.com/en/unicode/U+{digit:>04}" name = f"[{unicodedata.name(char, '')}]({url})" - info = f"`{u_code.ljust(10)}`: {name} - {char}" + info = f"`{u_code.ljust(10)}`: {name} - {utils.escape_markdown(char)}" return info, u_code charlist, rawlist = zip(*(get_info(c) for c in characters)) -- cgit v1.2.3 From 1d0cbbeb46a811b5a049d712aae1a90a1f3a7359 Mon Sep 17 00:00:00 2001 From: Dennis Pham Date: Mon, 15 Jun 2020 00:36:53 -0400 Subject: Add the C# guild to the whitelist --- config-default.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/config-default.yml b/config-default.yml index 3388e5f78..aff5fb2e1 100644 --- a/config-default.yml +++ b/config-default.yml @@ -299,6 +299,7 @@ filter: - 185590609631903755 # Blender Hub - 420324994703163402 # /r/FlutterDev - 488751051629920277 # Python Atlanta + - 143867839282020352 # C# domain_blacklist: - pornhub.com -- cgit v1.2.3 From 09f53ca77ae79ceccad91da5e0d44d7013757f0e Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 14 Jun 2020 22:36:28 -0700 Subject: Check infraction reason isn't None before shortening it --- bot/cogs/moderation/infractions.py | 10 +++++++--- bot/cogs/moderation/scheduler.py | 3 +-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/bot/cogs/moderation/infractions.py b/bot/cogs/moderation/infractions.py index 5bfaad796..f685f6991 100644 --- a/bot/cogs/moderation/infractions.py +++ b/bot/cogs/moderation/infractions.py @@ -226,7 +226,10 @@ class Infractions(InfractionScheduler, commands.Cog): self.mod_log.ignore(Event.member_remove, user.id) - action = user.kick(reason=textwrap.shorten(reason, width=512, placeholder="...")) + if reason: + reason = textwrap.shorten(reason, width=512, placeholder="...") + + action = user.kick(reason=reason) await self.apply_infraction(ctx, infraction, user, action) @respect_role_hierarchy() @@ -259,9 +262,10 @@ class Infractions(InfractionScheduler, commands.Cog): self.mod_log.ignore(Event.member_remove, user.id) - truncated_reason = textwrap.shorten(reason, width=512, placeholder="...") + if reason: + reason = textwrap.shorten(reason, width=512, placeholder="...") - action = ctx.guild.ban(user, reason=truncated_reason, delete_message_days=0) + action = ctx.guild.ban(user, reason=reason, delete_message_days=0) await self.apply_infraction(ctx, infraction, user, action) if infraction.get('expires_at') is not None: diff --git a/bot/cogs/moderation/scheduler.py b/bot/cogs/moderation/scheduler.py index b03d89537..beb201b8c 100644 --- a/bot/cogs/moderation/scheduler.py +++ b/bot/cogs/moderation/scheduler.py @@ -127,11 +127,10 @@ class InfractionScheduler(Scheduler): dm_result = ":incoming_envelope: " dm_log_text = "\nDM: Sent" - if infraction["actor"] == self.bot.user.id: + if reason and infraction["actor"] == self.bot.user.id: log.trace( f"Infraction #{id_} actor is bot; including the reason in the confirmation message." ) - end_msg = f" (reason: {textwrap.shorten(reason, width=1500, placeholder='...')})" elif ctx.channel.id not in STAFF_CHANNELS: log.trace( -- cgit v1.2.3 From 08c96f9eb07a2a86e68fb0e0837b9d07c40dab5e Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 14 Jun 2020 22:41:09 -0700 Subject: Fix check for bot actor in infractions The reason None check should be nested to avoid affecting the else/elif statements that follow. --- bot/cogs/moderation/scheduler.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bot/cogs/moderation/scheduler.py b/bot/cogs/moderation/scheduler.py index beb201b8c..d75a72ddb 100644 --- a/bot/cogs/moderation/scheduler.py +++ b/bot/cogs/moderation/scheduler.py @@ -127,17 +127,17 @@ class InfractionScheduler(Scheduler): dm_result = ":incoming_envelope: " dm_log_text = "\nDM: Sent" - if reason and infraction["actor"] == self.bot.user.id: + end_msg = "" + if infraction["actor"] == self.bot.user.id: log.trace( f"Infraction #{id_} actor is bot; including the reason in the confirmation message." ) - end_msg = f" (reason: {textwrap.shorten(reason, width=1500, placeholder='...')})" + if reason: + end_msg = f" (reason: {textwrap.shorten(reason, width=1500, placeholder='...')})" elif ctx.channel.id not in STAFF_CHANNELS: log.trace( f"Infraction #{id_} context is not in a staff channel; omitting infraction count." ) - - end_msg = "" else: log.trace(f"Fetching total infraction count for {user}.") -- cgit v1.2.3 From efd27cf29f726627b9ba630e257a2e3e89d3a286 Mon Sep 17 00:00:00 2001 From: Daniel Nash <22755628+crazygmr101@users.noreply.github.com> Date: Mon, 15 Jun 2020 13:18:36 -0400 Subject: Update bot/resources/tags/customcooldown.md Co-authored-by: Mark --- bot/resources/tags/customcooldown.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/bot/resources/tags/customcooldown.md b/bot/resources/tags/customcooldown.md index e877e4dae..ac7e70aee 100644 --- a/bot/resources/tags/customcooldown.md +++ b/bot/resources/tags/customcooldown.md @@ -10,11 +10,9 @@ message_cooldown = commands.CooldownMapping.from_cooldown(1.0, 60.0, commands.Bu @bot.event async def on_message(message): bucket = message_cooldown.get_bucket(message) - # update_rate_limit returns a time you need to wait before - # trying again retry_after = bucket.update_rate_limit() if retry_after: - await message.channel.send("Slow down! You're sending messages too fast") + await message.channel.send(f"Slow down! Try again in {retry_after} seconds.") else: await message.channel.send("Not ratelimited!") ``` -- cgit v1.2.3 From ae44563fe132436d98f50e074e5eb4421eda5538 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 15 Jun 2020 17:41:34 -0700 Subject: Log exception info for failed attachment uploads --- bot/utils/messages.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/utils/messages.py b/bot/utils/messages.py index de8e186f3..23519a514 100644 --- a/bot/utils/messages.py +++ b/bot/utils/messages.py @@ -97,7 +97,7 @@ async def send_attachments( if link_large and e.status == 413: large.append(attachment) else: - log.warning(f"{failure_msg} with status {e.status}.") + log.warning(f"{failure_msg} with status {e.status}.", exc_info=e) if link_large and large: desc = "\n".join(f"[{attachment.filename}]({attachment.url})" for attachment in large) -- cgit v1.2.3 From 2020df342d3aa43acdf7ead026d593f779264002 Mon Sep 17 00:00:00 2001 From: kosayoda Date: Tue, 16 Jun 2020 20:03:15 +0800 Subject: Refactor nested if-statement --- bot/cogs/help_channels.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/bot/cogs/help_channels.py b/bot/cogs/help_channels.py index 86579e940..4c464a7d2 100644 --- a/bot/cogs/help_channels.py +++ b/bot/cogs/help_channels.py @@ -562,11 +562,10 @@ class HelpChannels(Scheduler, commands.Cog): self.bot.stats.timing("help.in_use_time", in_use_time) unanswered = await self.unanswered.get(channel.id) - if unanswered is not None: - if unanswered: - self.bot.stats.incr("help.sessions.unanswered") - else: - self.bot.stats.incr("help.sessions.answered") + if unanswered: + self.bot.stats.incr("help.sessions.unanswered") + elif unanswered is not None: + self.bot.stats.incr("help.sessions.answered") log.trace(f"Position of #{channel} ({channel.id}) is actually {channel.position}.") log.trace(f"Sending dormant message for #{channel} ({channel.id}).") -- cgit v1.2.3 From 433f6b6843006aff57cf1e125d340e703c85669f Mon Sep 17 00:00:00 2001 From: kosayoda Date: Tue, 16 Jun 2020 20:10:23 +0800 Subject: Revise inaccurate docstring in RedisCache --- bot/utils/redis_cache.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index 347a0e54a..f342bbb62 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -48,8 +48,8 @@ class RedisCache: behaves, and should be familiar to Python users. The biggest difference is that all the public methods in this class are coroutines, and must be awaited. - Because of limitations in Redis, this cache will only accept strings, integers and - floats both for keys and values. + Because of limitations in Redis, this cache will only accept strings and integers for keys, + and strings, integers, floats and booleans for values. Please note that this class MUST be created as a class attribute, and that that class must also contain an attribute with an instance of our Bot. See `__get__` and `__set_name__` -- cgit v1.2.3 From 2426ea2141dd75aba21562d925244c1a43af94fc Mon Sep 17 00:00:00 2001 From: kosayoda Date: Tue, 16 Jun 2020 21:55:14 +0800 Subject: Revise inaccurate typehint for Optional reason --- bot/cogs/moderation/infractions.py | 49 ++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/bot/cogs/moderation/infractions.py b/bot/cogs/moderation/infractions.py index f685f6991..3db788eb9 100644 --- a/bot/cogs/moderation/infractions.py +++ b/bot/cogs/moderation/infractions.py @@ -53,7 +53,7 @@ class Infractions(InfractionScheduler, commands.Cog): # region: Permanent infractions @command() - async def warn(self, ctx: Context, user: Member, *, reason: str = None) -> None: + async def warn(self, ctx: Context, user: Member, *, reason: t.Optional[str] = None) -> None: """Warn a user for the given reason.""" infraction = await utils.post_infraction(ctx, user, "warning", reason, active=False) if infraction is None: @@ -62,12 +62,12 @@ class Infractions(InfractionScheduler, commands.Cog): await self.apply_infraction(ctx, infraction, user) @command() - async def kick(self, ctx: Context, user: Member, *, reason: str = None) -> None: + async def kick(self, ctx: Context, user: Member, *, reason: t.Optional[str] = None) -> None: """Kick a user for the given reason.""" await self.apply_kick(ctx, user, reason, active=False) @command() - async def ban(self, ctx: Context, user: FetchedMember, *, reason: str = None) -> None: + async def ban(self, ctx: Context, user: FetchedMember, *, reason: t.Optional[str] = None) -> None: """Permanently ban a user for the given reason and stop watching them with Big Brother.""" await self.apply_ban(ctx, user, reason) @@ -75,7 +75,9 @@ class Infractions(InfractionScheduler, commands.Cog): # region: Temporary infractions @command(aliases=["mute"]) - async def tempmute(self, ctx: Context, user: Member, duration: Expiry, *, reason: str = None) -> None: + async def tempmute( + self, ctx: Context, user: Member, duration: Expiry, *, reason: t.Optional[str] = None + ) -> None: """ Temporarily mute a user for the given reason and duration. @@ -94,7 +96,9 @@ class Infractions(InfractionScheduler, commands.Cog): await self.apply_mute(ctx, user, reason, expires_at=duration) @command() - async def tempban(self, ctx: Context, user: FetchedMember, duration: Expiry, *, reason: str = None) -> None: + async def tempban( + self, ctx: Context, user: FetchedMember, duration: Expiry, *, reason: t.Optional[str] = None + ) -> None: """ Temporarily ban a user for the given reason and duration. @@ -116,7 +120,7 @@ class Infractions(InfractionScheduler, commands.Cog): # region: Permanent shadow infractions @command(hidden=True) - async def note(self, ctx: Context, user: FetchedMember, *, reason: str = None) -> None: + async def note(self, ctx: Context, user: FetchedMember, *, reason: t.Optional[str] = None) -> None: """Create a private note for a user with the given reason without notifying the user.""" infraction = await utils.post_infraction(ctx, user, "note", reason, hidden=True, active=False) if infraction is None: @@ -125,12 +129,14 @@ class Infractions(InfractionScheduler, commands.Cog): await self.apply_infraction(ctx, infraction, user) @command(hidden=True, aliases=['shadowkick', 'skick']) - async def shadow_kick(self, ctx: Context, user: Member, *, reason: str = None) -> None: + async def shadow_kick(self, ctx: Context, user: Member, *, reason: t.Optional[str] = None) -> None: """Kick a user for the given reason without notifying the user.""" await self.apply_kick(ctx, user, reason, hidden=True, active=False) @command(hidden=True, aliases=['shadowban', 'sban']) - async def shadow_ban(self, ctx: Context, user: FetchedMember, *, reason: str = None) -> None: + async def shadow_ban( + self, ctx: Context, user: FetchedMember, *, reason: t.Optional[str] = None + ) -> None: """Permanently ban a user for the given reason without notifying the user.""" await self.apply_ban(ctx, user, reason, hidden=True) @@ -138,7 +144,14 @@ class Infractions(InfractionScheduler, commands.Cog): # region: Temporary shadow infractions @command(hidden=True, aliases=["shadowtempmute, stempmute", "shadowmute", "smute"]) - async def shadow_tempmute(self, ctx: Context, user: Member, duration: Expiry, *, reason: str = None) -> None: + async def shadow_tempmute( + self, + ctx: Context, + user: Member, + duration: Expiry, + *, + reason: t.Optional[str] = None + ) -> None: """ Temporarily mute a user for the given reason and duration without notifying the user. @@ -158,12 +171,12 @@ class Infractions(InfractionScheduler, commands.Cog): @command(hidden=True, aliases=["shadowtempban, stempban"]) async def shadow_tempban( - self, - ctx: Context, - user: FetchedMember, - duration: Expiry, - *, - reason: str = None + self, + ctx: Context, + user: FetchedMember, + duration: Expiry, + *, + reason: t.Optional[str] = None ) -> None: """ Temporarily ban a user for the given reason and duration without notifying the user. @@ -198,7 +211,7 @@ class Infractions(InfractionScheduler, commands.Cog): # endregion # region: Base apply functions - async def apply_mute(self, ctx: Context, user: Member, reason: str, **kwargs) -> None: + async def apply_mute(self, ctx: Context, user: Member, reason: t.Optional[str], **kwargs) -> None: """Apply a mute infraction with kwargs passed to `post_infraction`.""" if await utils.get_active_infraction(ctx, user, "mute"): return @@ -218,7 +231,7 @@ class Infractions(InfractionScheduler, commands.Cog): await self.apply_infraction(ctx, infraction, user, action()) @respect_role_hierarchy() - async def apply_kick(self, ctx: Context, user: Member, reason: str, **kwargs) -> None: + async def apply_kick(self, ctx: Context, user: Member, reason: t.Optional[str], **kwargs) -> None: """Apply a kick infraction with kwargs passed to `post_infraction`.""" infraction = await utils.post_infraction(ctx, user, "kick", reason, active=False, **kwargs) if infraction is None: @@ -233,7 +246,7 @@ class Infractions(InfractionScheduler, commands.Cog): await self.apply_infraction(ctx, infraction, user, action) @respect_role_hierarchy() - async def apply_ban(self, ctx: Context, user: UserSnowflake, reason: str, **kwargs) -> None: + async def apply_ban(self, ctx: Context, user: UserSnowflake, reason: t.Optional[str], **kwargs) -> None: """ Apply a ban infraction with kwargs passed to `post_infraction`. -- cgit v1.2.3 From 6fe0bc1c9ce35459b7d9b5bd2309b41dcc4c0dcc Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Tue, 16 Jun 2020 12:40:12 -0700 Subject: Add optional type annotations to reason in pardon funcs --- bot/cogs/moderation/infractions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bot/cogs/moderation/infractions.py b/bot/cogs/moderation/infractions.py index 3db788eb9..f7747e7f8 100644 --- a/bot/cogs/moderation/infractions.py +++ b/bot/cogs/moderation/infractions.py @@ -298,7 +298,7 @@ class Infractions(InfractionScheduler, commands.Cog): # endregion # region: Base pardon functions - async def pardon_mute(self, user_id: int, guild: discord.Guild, reason: str) -> t.Dict[str, str]: + async def pardon_mute(self, user_id: int, guild: discord.Guild, reason: t.Optional[str]) -> t.Dict[str, str]: """Remove a user's muted role, DM them a notification, and return a log dict.""" user = guild.get_member(user_id) log_text = {} @@ -324,7 +324,7 @@ class Infractions(InfractionScheduler, commands.Cog): return log_text - async def pardon_ban(self, user_id: int, guild: discord.Guild, reason: str) -> t.Dict[str, str]: + async def pardon_ban(self, user_id: int, guild: discord.Guild, reason: t.Optional[str]) -> t.Dict[str, str]: """Remove a user's ban on the Discord guild and return a log dict.""" user = discord.Object(user_id) log_text = {} -- cgit v1.2.3 From 20a8b6fe92c398fdc246d78591600bb7bde78bca Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Tue, 16 Jun 2020 12:48:13 -0700 Subject: Format parameters with a more consistent style --- bot/cogs/moderation/infractions.py | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/bot/cogs/moderation/infractions.py b/bot/cogs/moderation/infractions.py index f7747e7f8..3b28526b2 100644 --- a/bot/cogs/moderation/infractions.py +++ b/bot/cogs/moderation/infractions.py @@ -75,9 +75,7 @@ class Infractions(InfractionScheduler, commands.Cog): # region: Temporary infractions @command(aliases=["mute"]) - async def tempmute( - self, ctx: Context, user: Member, duration: Expiry, *, reason: t.Optional[str] = None - ) -> None: + async def tempmute(self, ctx: Context, user: Member, duration: Expiry, *, reason: t.Optional[str] = None) -> None: """ Temporarily mute a user for the given reason and duration. @@ -97,7 +95,12 @@ class Infractions(InfractionScheduler, commands.Cog): @command() async def tempban( - self, ctx: Context, user: FetchedMember, duration: Expiry, *, reason: t.Optional[str] = None + self, + ctx: Context, + user: FetchedMember, + duration: Expiry, + *, + reason: t.Optional[str] = None ) -> None: """ Temporarily ban a user for the given reason and duration. @@ -134,9 +137,7 @@ class Infractions(InfractionScheduler, commands.Cog): await self.apply_kick(ctx, user, reason, hidden=True, active=False) @command(hidden=True, aliases=['shadowban', 'sban']) - async def shadow_ban( - self, ctx: Context, user: FetchedMember, *, reason: t.Optional[str] = None - ) -> None: + async def shadow_ban(self, ctx: Context, user: FetchedMember, *, reason: t.Optional[str] = None) -> None: """Permanently ban a user for the given reason without notifying the user.""" await self.apply_ban(ctx, user, reason, hidden=True) @@ -145,12 +146,11 @@ class Infractions(InfractionScheduler, commands.Cog): @command(hidden=True, aliases=["shadowtempmute, stempmute", "shadowmute", "smute"]) async def shadow_tempmute( - self, - ctx: Context, - user: Member, - duration: Expiry, - *, - reason: t.Optional[str] = None + self, ctx: Context, + user: Member, + duration: Expiry, + *, + reason: t.Optional[str] = None ) -> None: """ Temporarily mute a user for the given reason and duration without notifying the user. @@ -171,12 +171,12 @@ class Infractions(InfractionScheduler, commands.Cog): @command(hidden=True, aliases=["shadowtempban, stempban"]) async def shadow_tempban( - self, - ctx: Context, - user: FetchedMember, - duration: Expiry, - *, - reason: t.Optional[str] = None + self, + ctx: Context, + user: FetchedMember, + duration: Expiry, + *, + reason: t.Optional[str] = None ) -> None: """ Temporarily ban a user for the given reason and duration without notifying the user. -- cgit v1.2.3 From 778635241bf6c1a97f60f48a2bc9b40791a524e9 Mon Sep 17 00:00:00 2001 From: Joseph Banks Date: Wed, 17 Jun 2020 19:15:31 +0100 Subject: Add LMGTFY to domain blacklist --- config-default.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/config-default.yml b/config-default.yml index aff5fb2e1..f111c64f5 100644 --- a/config-default.yml +++ b/config-default.yml @@ -331,6 +331,7 @@ filter: - ssteam.site - steamwalletgift.com - discord.gift + - lmgtfy.com word_watchlist: - goo+ks* -- cgit v1.2.3 From 47b6f65e231305c2ceb4f48a2a772a734ae190db Mon Sep 17 00:00:00 2001 From: Joseph Banks Date: Wed, 17 Jun 2020 21:20:27 +0100 Subject: Update deletion scheduler to use latest watchlist configuration --- bot/cogs/filtering.py | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index f7cf4c3ea..76ea68660 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -7,7 +7,7 @@ from typing import List, Mapping, Optional, Union import dateutil import discord.errors from dateutil.relativedelta import relativedelta -from discord import Colour, DMChannel, HTTPException, Member, Message, NotFound, TextChannel +from discord import Colour, HTTPException, Member, Message, NotFound, TextChannel from discord.ext.commands import Cog from discord.utils import escape_markdown @@ -56,6 +56,7 @@ def expand_spoilers(text: str) -> str: split_text[0::2] + split_text[1::2] + split_text ) + OFFENSIVE_MSG_DELETE_TIME = timedelta(days=Filter.offensive_msg_delete_days) @@ -113,6 +114,7 @@ class Filtering(Cog, Scheduler): "function": self._has_watch_regex_match, "type": "watchlist", "content_only": True, + "schedule_deletion": True }, "watch_rich_embeds": { "enabled": Filter.watch_rich_embeds, @@ -120,21 +122,7 @@ class Filtering(Cog, Scheduler): "type": "watchlist", "content_only": False, "schedule_deletion": False - }, - "watch_words": { - "enabled": Filter.watch_words, - "function": self._has_watchlist_words, - "type": "watchlist", - "content_only": True, - "schedule_deletion": True - }, - "watch_tokens": { - "enabled": Filter.watch_tokens, - "function": self._has_watchlist_tokens, - "type": "watchlist", - "content_only": True, - "schedule_deletion": True - }, + } } self.bot.loop.create_task(self.reschedule_offensive_msg_deletion()) @@ -481,7 +469,7 @@ class Filtering(Cog, Scheduler): await self.bot.wait_until_ready() response = await self.bot.api_client.get('bot/offensive-messages',) - now = datetime.datetime.utcnow() + now = datetime.utcnow() for msg in response: delete_at = dateutil.parser.isoparse(msg['delete_date']).replace(tzinfo=None) -- cgit v1.2.3