From ed815a1cabde59edbea734ed7e10a15bf9a845ff Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Sat, 26 Nov 2022 14:09:02 +0400 Subject: [PATCH 1/8] Simplify Help Thread Warning Log The current warning log includes the thread name, which means the log message varies wildly between threads. This causes issues with sentry since the actual error message gets trimmed, and sentry fails to group issues from this log as they appear as different messages. Signed-off-by: Hassan Abouelela --- bot/exts/help_channels/_channel.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/exts/help_channels/_channel.py b/bot/exts/help_channels/_channel.py index cc3d831b0c..d4a7420002 100644 --- a/bot/exts/help_channels/_channel.py +++ b/bot/exts/help_channels/_channel.py @@ -80,7 +80,7 @@ async def send_opened_post_dm(thread: discord.Thread) -> None: try: message = await thread.fetch_message(thread.id) except discord.HTTPException: - log.warning(f"Could not fetch message for thread {thread.name} ({thread.id})") + log.warning(f"Could not fetch message for thread {thread.id}") return formatted_message = textwrap.shorten(message.content, width=100, placeholder="...") From fcf7e1cc2cdef58b27cc7d418f22b7bc22b78f07 Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Sat, 26 Nov 2022 14:11:27 +0400 Subject: [PATCH 2/8] Handle Discord API Not Being Ready When discord sends us the thread create event in help channels, it is not ready to perform other operations on the thread such as getting or pinning messages. This causes it to error out when we try to do these actions and claim that those channels don't exist. Instead, we sleep for a short time to try and wait for it to be ready. Signed-off-by: Hassan Abouelela --- bot/exts/help_channels/_channel.py | 7 ++++++- bot/exts/help_channels/_cog.py | 3 +-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/bot/exts/help_channels/_channel.py b/bot/exts/help_channels/_channel.py index d4a7420002..3dc9e81ef7 100644 --- a/bot/exts/help_channels/_channel.py +++ b/bot/exts/help_channels/_channel.py @@ -1,5 +1,5 @@ """Contains all logic to handle changes to posts in the help forum.""" - +import asyncio import textwrap import discord @@ -109,6 +109,11 @@ async def help_thread_opened(opened_thread: discord.Thread, *, reopen: bool = Fa await _close_help_thread(opened_thread, _stats.ClosingReason.CLEANUP) return + # Discord sends the open event long before the thread is ready for actions in the API. + # This causes actions such as fetching the message, pinning message, etc to fail. + # We sleep here to try and delay our code enough so the thread is ready in the API. + await asyncio.sleep(2) + await send_opened_post_dm(opened_thread) if opened_thread.starter_message: diff --git a/bot/exts/help_channels/_cog.py b/bot/exts/help_channels/_cog.py index 50f8416fc4..bb2f43c5a9 100644 --- a/bot/exts/help_channels/_cog.py +++ b/bot/exts/help_channels/_cog.py @@ -119,9 +119,8 @@ async def on_thread_create(self, thread: discord.Thread) -> None: if thread.parent_id != self.help_forum_channel_id: return - await _channel.help_thread_opened(thread) - await self.post_with_disallowed_title_check(thread) + await _channel.help_thread_opened(thread) @commands.Cog.listener() async def on_thread_update(self, before: discord.Thread, after: discord.Thread) -> None: From 4779d4240ae43fd48699ac80c0779665604abebb Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Sat, 26 Nov 2022 14:21:58 +0400 Subject: [PATCH 3/8] Don't Remove Cooldown Role From Non-Existing Users We try to remove the cooldown role from users before checking if the user is still in the server, which can cause an error since the thread object will just contain `None` as the user. Signed-off-by: Hassan Abouelela --- bot/exts/help_channels/_channel.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/bot/exts/help_channels/_channel.py b/bot/exts/help_channels/_channel.py index 3dc9e81ef7..a41fcd63f9 100644 --- a/bot/exts/help_channels/_channel.py +++ b/bot/exts/help_channels/_channel.py @@ -52,6 +52,15 @@ async def _close_help_thread(closed_thread: discord.Thread, closed_on: _stats.Cl poster = closed_thread.owner cooldown_role = closed_thread.guild.get_role(constants.Roles.help_cooldown) + + if poster is None: + # We can't include the owner ID/name here since the thread only contains None + log.info( + f"Failed to remove cooldown role for owner of thread ({closed_thread.id}). " + f"The user is likely no longer on the server." + ) + return + await members.handle_role_change(poster, poster.remove_roles, cooldown_role) From 81a68784559d231afcc055c2d34cb71314d5bd61 Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Sat, 26 Nov 2022 14:28:18 +0400 Subject: [PATCH 4/8] Catch Failure In Pining Help Starter Message The old method for detecting deleted opener messages does not seem to work, probably because the message is fetched from a cache or similar. Instead we simply try/except pinning the message and pass if the pinning failed. Signed-off-by: Hassan Abouelela --- bot/exts/help_channels/_channel.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/bot/exts/help_channels/_channel.py b/bot/exts/help_channels/_channel.py index a41fcd63f9..04c2cc454f 100644 --- a/bot/exts/help_channels/_channel.py +++ b/bot/exts/help_channels/_channel.py @@ -125,9 +125,14 @@ async def help_thread_opened(opened_thread: discord.Thread, *, reopen: bool = Fa await send_opened_post_dm(opened_thread) - if opened_thread.starter_message: - # To cover the case where the user deletes their starter message before code execution reaches this line. + try: await opened_thread.starter_message.pin() + except discord.HTTPException as e: + if e.code == 10008: + # The message was not found, most likely deleted + pass + else: + raise e await send_opened_post_message(opened_thread) From 555ed4e9195aae7b5af742c59125833ce8b01b4c Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Sat, 26 Nov 2022 14:40:04 +0400 Subject: [PATCH 5/8] Check If Thread Is Closed In wait_for_deletion The wait_for_deletion utility would try to remove reactions from a message after the timeout expires, which would normally be fine. In threads however, they can be closed while waiting for the timeout to expire. In such a case, the bot will try to remove the reactions after the channel has been closed and fail. A special exception was added for this case to do nothing, since this is only a QoL feature. Signed-off-by: Hassan Abouelela --- bot/utils/messages.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/bot/utils/messages.py b/bot/utils/messages.py index a5ed84351d..cc7e6dccbe 100644 --- a/bot/utils/messages.py +++ b/bot/utils/messages.py @@ -98,7 +98,14 @@ async def wait_for_deletion( try: await bot.instance.wait_for('reaction_add', check=check, timeout=timeout) except asyncio.TimeoutError: - await message.clear_reactions() + try: + await message.clear_reactions() + except discord.HTTPException as e: + if isinstance(message.channel, discord.Thread): + # Threads might not be accessible by the time we try to remove the reaction. + pass + else: + raise e else: await message.delete() except discord.NotFound: From dddf9734f898474ebd841537a49fb8d6905da346 Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Sat, 26 Nov 2022 15:16:54 +0400 Subject: [PATCH 6/8] Check If Thread Is Closed In Pagination Similar to 555ed4e9, the pagination utility needs to catch when it's trying to act on an archived thread. Signed-off-by: Hassan Abouelela --- bot/pagination.py | 93 +++++++++++++++++++---------------------------- 1 file changed, 38 insertions(+), 55 deletions(-) diff --git a/bot/pagination.py b/bot/pagination.py index 10bef1c9f9..5b96d8fbb2 100644 --- a/bot/pagination.py +++ b/bot/pagination.py @@ -301,42 +301,30 @@ async def paginate( if str(reaction.emoji) == DELETE_EMOJI: log.debug("Got delete reaction") return await message.delete() - - if reaction.emoji == FIRST_EMOJI: - await message.remove_reaction(reaction.emoji, user) - current_page = 0 - - log.debug(f"Got first page reaction - changing to page 1/{len(paginator.pages)}") - - embed.description = paginator.pages[current_page] - if footer_text: - embed.set_footer(text=f"{footer_text} (Page {current_page + 1}/{len(paginator.pages)})") - else: - embed.set_footer(text=f"Page {current_page + 1}/{len(paginator.pages)}") - await message.edit(embed=embed) - - if reaction.emoji == LAST_EMOJI: - await message.remove_reaction(reaction.emoji, user) - current_page = len(paginator.pages) - 1 - - log.debug(f"Got last page reaction - changing to page {current_page + 1}/{len(paginator.pages)}") - - embed.description = paginator.pages[current_page] - if footer_text: - embed.set_footer(text=f"{footer_text} (Page {current_page + 1}/{len(paginator.pages)})") - else: - embed.set_footer(text=f"Page {current_page + 1}/{len(paginator.pages)}") - await message.edit(embed=embed) - - if reaction.emoji == LEFT_EMOJI: + elif reaction.emoji in PAGINATION_EMOJI: + total_pages = len(paginator.pages) await message.remove_reaction(reaction.emoji, user) - if current_page <= 0: - log.debug("Got previous page reaction, but we're on the first page - ignoring") - continue - - current_page -= 1 - log.debug(f"Got previous page reaction - changing to page {current_page + 1}/{len(paginator.pages)}") + if reaction.emoji == FIRST_EMOJI: + current_page = 0 + log.debug(f"Got first page reaction - changing to page 1/{total_pages}") + elif reaction.emoji == LAST_EMOJI: + current_page = len(paginator.pages) - 1 + log.debug(f"Got last page reaction - changing to page {current_page + 1}/{total_pages}") + elif reaction.emoji == LEFT_EMOJI: + if current_page <= 0: + log.debug("Got previous page reaction, but we're on the first page - ignoring") + continue + + current_page -= 1 + log.debug(f"Got previous page reaction - changing to page {current_page + 1}/{total_pages}") + elif reaction.emoji == RIGHT_EMOJI: + if current_page >= len(paginator.pages) - 1: + log.debug("Got next page reaction, but we're on the last page - ignoring") + continue + + current_page += 1 + log.debug(f"Got next page reaction - changing to page {current_page + 1}/{total_pages}") embed.description = paginator.pages[current_page] @@ -345,27 +333,22 @@ async def paginate( else: embed.set_footer(text=f"Page {current_page + 1}/{len(paginator.pages)}") - await message.edit(embed=embed) - - if reaction.emoji == RIGHT_EMOJI: - await message.remove_reaction(reaction.emoji, user) - - if current_page >= len(paginator.pages) - 1: - log.debug("Got next page reaction, but we're on the last page - ignoring") - continue - - current_page += 1 - log.debug(f"Got next page reaction - changing to page {current_page + 1}/{len(paginator.pages)}") - - embed.description = paginator.pages[current_page] - - if footer_text: - embed.set_footer(text=f"{footer_text} (Page {current_page + 1}/{len(paginator.pages)})") - else: - embed.set_footer(text=f"Page {current_page + 1}/{len(paginator.pages)}") - - await message.edit(embed=embed) + try: + await message.edit(embed=embed) + except discord.HTTPException as e: + if e.code == 50083: + # Trying to act on an archived thread, just ignore and abort + break + else: + raise e log.debug("Ending pagination and clearing reactions.") with suppress(discord.NotFound): - await message.clear_reactions() + try: + await message.clear_reactions() + except discord.HTTPException as e: + if e.code == 50083: + # Trying to act on an archived thread, just ignore + pass + else: + raise e From 4ca04f60c122f534700e2a350bcbafd2471b96e4 Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Sat, 26 Nov 2022 15:20:14 +0400 Subject: [PATCH 7/8] Handle Images As Starter Messages In the case of an image or other media as the starter message, the formatted message in the help DM will be empty, which is invalid for the embed. We populate the field with some more useful text in this case. Signed-off-by: Hassan Abouelela --- bot/exts/help_channels/_channel.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/bot/exts/help_channels/_channel.py b/bot/exts/help_channels/_channel.py index 04c2cc454f..b9a79a4768 100644 --- a/bot/exts/help_channels/_channel.py +++ b/bot/exts/help_channels/_channel.py @@ -92,7 +92,11 @@ async def send_opened_post_dm(thread: discord.Thread) -> None: log.warning(f"Could not fetch message for thread {thread.id}") return - formatted_message = textwrap.shorten(message.content, width=100, placeholder="...") + formatted_message = textwrap.shorten(message.content, width=100, placeholder="...").strip() + if formatted_message is None: + # This most likely means the initial message is only an image or similar + formatted_message = "No text content." + embed.add_field(name="Your message", value=formatted_message, inline=False) embed.add_field( name="Conversation", From 8cfa7ed5b39b85e432420c5749c0fe1569e2d2cb Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Sat, 26 Nov 2022 16:12:42 +0400 Subject: [PATCH 8/8] Refactor Thread Error Catching Co-authored-by: Boris Muratov <8bee278@gmail.com> Co-authored-by: Amrou Bellalouna <48383734+shtlrs@users.noreply.github.com> Signed-off-by: Hassan Abouelela --- bot/exts/help_channels/_channel.py | 6 ++---- bot/pagination.py | 13 ++++++++----- bot/utils/messages.py | 15 +++++++-------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/bot/exts/help_channels/_channel.py b/bot/exts/help_channels/_channel.py index b9a79a4768..5fc39b6236 100644 --- a/bot/exts/help_channels/_channel.py +++ b/bot/exts/help_channels/_channel.py @@ -132,10 +132,8 @@ async def help_thread_opened(opened_thread: discord.Thread, *, reopen: bool = Fa try: await opened_thread.starter_message.pin() except discord.HTTPException as e: - if e.code == 10008: - # The message was not found, most likely deleted - pass - else: + # Suppress if the message was not found, most likely deleted + if e.code != 10008: raise e await send_opened_post_message(opened_thread) diff --git a/bot/pagination.py b/bot/pagination.py index 5b96d8fbb2..0ef5808ccc 100644 --- a/bot/pagination.py +++ b/bot/pagination.py @@ -303,7 +303,12 @@ async def paginate( return await message.delete() elif reaction.emoji in PAGINATION_EMOJI: total_pages = len(paginator.pages) - await message.remove_reaction(reaction.emoji, user) + try: + await message.remove_reaction(reaction.emoji, user) + except discord.HTTPException as e: + # Suppress if trying to act on an archived thread. + if e.code != 50083: + raise e if reaction.emoji == FIRST_EMOJI: current_page = 0 @@ -347,8 +352,6 @@ async def paginate( try: await message.clear_reactions() except discord.HTTPException as e: - if e.code == 50083: - # Trying to act on an archived thread, just ignore - pass - else: + # Suppress if trying to act on an archived thread. + if e.code != 50083: raise e diff --git a/bot/utils/messages.py b/bot/utils/messages.py index cc7e6dccbe..8a968f6598 100644 --- a/bot/utils/messages.py +++ b/bot/utils/messages.py @@ -98,19 +98,18 @@ async def wait_for_deletion( try: await bot.instance.wait_for('reaction_add', check=check, timeout=timeout) except asyncio.TimeoutError: - try: - await message.clear_reactions() - except discord.HTTPException as e: - if isinstance(message.channel, discord.Thread): - # Threads might not be accessible by the time we try to remove the reaction. - pass - else: - raise e + await message.clear_reactions() else: await message.delete() + except discord.NotFound: log.trace(f"wait_for_deletion: message {message.id} deleted prematurely.") + except discord.HTTPException: + if not isinstance(message.channel, discord.Thread): + # Threads might not be accessible by the time the timeout expires + raise + async def send_attachments( message: discord.Message,