fix: don't include replied mentions in mention filter (#2017)

Co-authored-by: Izan <[email protected]>
Co-authored-by: TizzySaurus <[email protected]>
Co-authored-by: Xithrius <[email protected]>

3 files changed, 124 insertions, 12 deletions

diff --git a/bot/rules/mentions.py b/bot/rules/mentions.py
index 6f5addad1..ca1d0c01c 100644
--- a/bot/rules/mentions.py
+++ b/bot/rules/mentions.py
@@ -1,23 +1,65 @@
 from typing import Dict, Iterable, List, Optional, Tuple
 
-from discord import Member, Message
+from discord import DeletedReferencedMessage, Member, Message, MessageType, NotFound
+
+import bot
+from bot.log import get_logger
+
+log = get_logger(__name__)
 
 
 async def apply(
     last_message: Message, recent_messages: List[Message], config: Dict[str, int]
 ) -> Optional[Tuple[str, Iterable[Member], Iterable[Message]]]:
-    """Detects total mentions exceeding the limit sent by a single user."""
+    """
+    Detects total mentions exceeding the limit sent by a single user.
+
+    Excludes mentions that are bots, themselves, or replied users.
+
+    In very rare cases, may not be able to determine a
+    mention was to a reply, in which case it is not ignored.
+    """
     relevant_messages = tuple(
         msg
         for msg in recent_messages
         if msg.author == last_message.author
     )
+    # We use `msg.mentions` here as that is supplied by the api itself, to determine who was mentioned.
+    # Additionally, `msg.mentions` includes the user replied to, even if the mention doesn't occur in the body.
+    # In order to exclude users who are mentioned as a reply, we check if the msg has a reference
+    #
+    # While we could use regex to parse the message content, and get a list of
+    # the mentions, that solution is very prone to breaking.
+    # We would need to deal with codeblocks, escaping markdown, and any discrepancies between
+    # our implementation and discord's markdown parser which would cause false positives or false negatives.
+    total_recent_mentions = 0
+    for msg in relevant_messages:
+        # We check if the message is a reply, and if it is try to get the author
+        # since we ignore mentions of a user that we're replying to
+        reply_author = None
 
-    total_recent_mentions = sum(
-        not user.bot
-        for msg in relevant_messages
-        for user in msg.mentions
-    )
+        if msg.type == MessageType.reply:
+            ref = msg.reference
+
+            if not (resolved := ref.resolved):
+                # It is possible, in a very unusual situation, for a message to have a reference
+                # that is both not in the cache, and deleted while running this function.
+                # In such a situation, this will throw an error which we catch.
+                try:
+                    resolved = await bot.instance.get_partial_messageable(resolved.channel_id).fetch_message(
+                        resolved.message_id
+                    )
+                except NotFound:
+                    log.info('Could not fetch the reference message as it has been deleted.')
+
+            if resolved and not isinstance(resolved, DeletedReferencedMessage):
+                reply_author = resolved.author
+
+        for user in msg.mentions:
+            # Don't count bot or self mentions, or the user being replied to (if applicable)
+            if user.bot or user in {msg.author, reply_author}:
+                continue
+            total_recent_mentions += 1
 
     if total_recent_mentions > config['max']:
         return (
diff --git a/tests/bot/rules/test_mentions.py b/tests/bot/rules/test_mentions.py
index f8805ac48..e1f904917 100644
--- a/tests/bot/rules/test_mentions.py
+++ b/tests/bot/rules/test_mentions.py
@@ -1,15 +1,32 @@
-from typing import Iterable
+from typing import Iterable, Optional
+
+import discord
 
 from bot.rules import mentions
 from tests.bot.rules import DisallowedCase, RuleTest
-from tests.helpers import MockMember, MockMessage
+from tests.helpers import MockMember, MockMessage, MockMessageReference
 
 
-def make_msg(author: str, total_user_mentions: int, total_bot_mentions: int = 0) -> MockMessage:
-    """Makes a message with `total_mentions` mentions."""
+def make_msg(
+    author: str,
+    total_user_mentions: int,
+    total_bot_mentions: int = 0,
+    *,
+    reference: Optional[MockMessageReference] = None
+) -> MockMessage:
+    """Makes a message from `author` with `total_user_mentions` user mentions and `total_bot_mentions` bot mentions."""
     user_mentions = [MockMember() for _ in range(total_user_mentions)]
     bot_mentions = [MockMember(bot=True) for _ in range(total_bot_mentions)]
-    return MockMessage(author=author, mentions=user_mentions+bot_mentions)
+
+    mentions = user_mentions + bot_mentions
+    if reference is not None:
+        # For the sake of these tests we assume that all references are mentions.
+        mentions.append(reference.resolved.author)
+        msg_type = discord.MessageType.reply
+    else:
+        msg_type = discord.MessageType.default
+
+    return MockMessage(author=author, mentions=mentions, reference=reference, type=msg_type)
 
 
 class TestMentions(RuleTest):
@@ -56,6 +73,16 @@ class TestMentions(RuleTest):
                 ("bob",),
                 3,
             ),
+            DisallowedCase(
+                [make_msg("bob", 3, reference=MockMessageReference())],
+                ("bob",),
+                3,
+            ),
+            DisallowedCase(
+                [make_msg("bob", 3, reference=MockMessageReference(reference_author_is_bot=True))],
+                ("bob",),
+                3
+            )
         )
 
         await self.run_disallowed(cases)
@@ -71,6 +98,27 @@ class TestMentions(RuleTest):
 
         await self.run_allowed(cases)
 
+    async def test_ignore_reply_mentions(self):
+        """Messages with an allowed amount of mentions in the content, also containing reply mentions."""
+        cases = (
+            [
+                make_msg("bob", 2, reference=MockMessageReference())
+            ],
+            [
+                make_msg("bob", 2, reference=MockMessageReference(reference_author_is_bot=True))
+            ],
+            [
+                make_msg("bob", 2, reference=MockMessageReference()),
+                make_msg("bob", 0, reference=MockMessageReference())
+            ],
+            [
+                make_msg("bob", 2, reference=MockMessageReference(reference_author_is_bot=True)),
+                make_msg("bob", 0, reference=MockMessageReference(reference_author_is_bot=True))
+            ]
+        )
+
+        await self.run_allowed(cases)
+
     def relevant_messages(self, case: DisallowedCase) -> Iterable[MockMessage]:
         last_message = case.recent_messages[0]
         return tuple(
diff --git a/tests/helpers.py b/tests/helpers.py
index 17214553c..687e15b96 100644
--- a/tests/helpers.py
+++ b/tests/helpers.py
@@ -492,6 +492,28 @@ class MockAttachment(CustomMockMixin, unittest.mock.MagicMock):
     spec_set = attachment_instance
 
 
+message_reference_instance = discord.MessageReference(
+    message_id=unittest.mock.MagicMock(id=1),
+    channel_id=unittest.mock.MagicMock(id=2),
+    guild_id=unittest.mock.MagicMock(id=3)
+)
+
+
+class MockMessageReference(CustomMockMixin, unittest.mock.MagicMock):
+    """
+    A MagicMock subclass to mock MessageReference objects.
+
+    Instances of this class will follow the specification of `discord.MessageReference` instances.
+    For more information, see the `MockGuild` docstring.
+    """
+    spec_set = message_reference_instance
+
+    def __init__(self, *, reference_author_is_bot: bool = False, **kwargs):
+        super().__init__(**kwargs)
+        referenced_msg_author = MockMember(name="bob", bot=reference_author_is_bot)
+        self.resolved = MockMessage(author=referenced_msg_author)
+
+
 class MockMessage(CustomMockMixin, unittest.mock.MagicMock):
     """
     A MagicMock subclass to mock Message objects.