Implement review-suggested changes

userid -> user ID
maybevalid -> maybe_valid
remove collections import
and added a new function that handles the "format user ID log message" and should_ping_everyone feature

2 files changed, 91 insertions, 67 deletions

diff --git a/bot/exts/filters/token_remover.py b/bot/exts/filters/token_remover.py
index a31912d5b..54f0bc034 100644
--- a/bot/exts/filters/token_remover.py
+++ b/bot/exts/filters/token_remover.py
@@ -1,6 +1,5 @@
 import base64
 import binascii
-import collections
 import logging
 import re
 import typing as t
@@ -98,14 +97,8 @@ class TokenRemover(Cog):
 
         await msg.channel.send(DELETION_MESSAGE_TEMPLATE.format(mention=msg.author.mention))
 
-        user_name = None
-        user_id = self.extract_user_id(found_token.user_id)
-        user = msg.guild.get_member(user_id)
-
-        if user:
-            user_name = str(user)
-
-        log_message = self.format_log_message(msg, found_token, user_id, user_name)
+        log_message = self.format_log_message(msg, found_token)
+        userid_message, mention_everyone = self.format_userid_log_message(msg, found_token)
         log.debug(log_message)
 
         # Send pretty mod log embed to mod-alerts
@@ -113,26 +106,35 @@ class TokenRemover(Cog):
             icon_url=Icons.token_removed,
             colour=Colour(Colours.soft_red),
             title="Token removed!",
-            text=log_message,
+            text=log_message + "\n" + userid_message,
             thumbnail=msg.author.avatar_url_as(static_format="png"),
             channel_id=Channels.mod_alerts,
-            ping_everyone=user_name is not None,
+            ping_everyone=mention_everyone,
         )
 
         self.bot.stats.incr("tokens.removed_tokens")
 
-    @staticmethod
-    def format_log_message(
-        msg: Message,
-        token: Token,
-        user_id: int,
-        user_name: t.Optional[str] = None,
-    ) -> str:
+    @classmethod
+    def format_userid_log_message(cls, msg: Message, token: Token) -> t.Tuple[str, bool]:
         """
-        Return the log message to send for `token` being censored in `msg`.
+        Format the potion of the log message that includes details about the detected user ID.
 
-        Additonally, mention if the token was decodable into a user id, and if that resolves to a user on the server.
+        Includes the user ID and, if present on the server, their name and a toggle to
+         mention everyone.
+
+        Returns a tuple of (log_message, mention_everyone)
         """
+        user_id = cls.extract_user_id(token.user_id)
+        user = msg.guild.get_member(user_id)
+
+        if user:
+            return USER_TOKEN_MESSAGE.format(user_id=user_id, user_name=str(user)), True
+        else:
+            return DECODED_LOG_MESSAGE.format(user_id=user_id), False
+
+    @staticmethod
+    def format_log_message(msg: Message, token: Token) -> str:
+        """Return the generic portion of the log message to send for `token` being censored in `msg`."""
         message = LOG_MESSAGE.format(
             author=msg.author,
             author_id=msg.author.id,
@@ -141,11 +143,8 @@ class TokenRemover(Cog):
             timestamp=token.timestamp,
             hmac='x' * len(token.hmac),
         )
-        if user_name:
-            more = USER_TOKEN_MESSAGE.format(user_id=user_id, user_name=user_name)
-        else:
-            more = DECODED_LOG_MESSAGE.format(user_id=user_id)
-        return message + "\n" + more
+
+        return message
 
     @classmethod
     def find_token_in_message(cls, msg: Message) -> t.Optional[Token]:
@@ -154,9 +153,11 @@ class TokenRemover(Cog):
         # token check (e.g. `message.channel.send` also matches our token pattern)
         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) \
-                    and cls.is_maybevalid_hmac(token.hmac):
+            if (
+                cls.is_valid_user_id(token.user_id)
+                and cls.is_valid_timestamp(token.timestamp)
+                and cls.is_maybe_valid_hmac(token.hmac)
+            ):
                 # Short-circuit on first match
                 return token
 
@@ -165,7 +166,7 @@ class TokenRemover(Cog):
 
     @staticmethod
     def extract_user_id(b64_content: str) -> t.Optional[int]:
-        """Return a userid integer from part of a potential token, or None if it couldn't be decoded."""
+        """Return a user ID integer from part of a potential token, or None if it couldn't be decoded."""
         b64_content = utils.pad_base64(b64_content)
 
         try:
@@ -218,17 +219,19 @@ class TokenRemover(Cog):
             return False
 
     @staticmethod
-    def is_maybevalid_hmac(b64_content: str) -> bool:
+    def is_maybe_valid_hmac(b64_content: str) -> bool:
         """
-        Determine if a given hmac portion of a token is potentially valid.
+        Determine if a given HMAC portion of a token is potentially valid.
 
         If the HMAC has 3 or less characters, it's probably a dummy value like "xxxxxxxxxx",
         and thus the token can probably be skipped.
         """
-        unique = len(collections.Counter(b64_content.lower()).keys())
+        unique = len(set(b64_content.lower()))
         if unique <= 3:
-            log.debug(f"Considering the hmac {b64_content} a dummy because it has {unique}"
-                      " case-insensitively unique characters")
+            log.debug(
+                f"Considering the HMAC {b64_content} a dummy because it has {unique}"
+                " case-insensitively unique characters"
+            )
             return False
         else:
             return True
diff --git a/tests/bot/exts/filters/test_token_remover.py b/tests/bot/exts/filters/test_token_remover.py
index 8742b73c5..92dce201b 100644
--- a/tests/bot/exts/filters/test_token_remover.py
+++ b/tests/bot/exts/filters/test_token_remover.py
@@ -87,7 +87,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase):
                 self.assertFalse(result)
 
     def test_is_valid_hmac_valid(self):
-        """Should consider hmac valid if it is a valid hmac with a variety of characters."""
+        """Should consider an HMAC valid if it has at least 3 unique characters."""
         valid_hmacs = (
             "VXmErH7j511turNpfURmb0rVNm8",
             "Ysnu2wacjaKs7qnoo46S8Dm2us8",
@@ -97,11 +97,11 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase):
 
         for hmac in valid_hmacs:
             with self.subTest(msg=hmac):
-                result = TokenRemover.is_maybevalid_hmac(hmac)
+                result = TokenRemover.is_maybe_valid_hmac(hmac)
                 self.assertTrue(result)
 
     def test_is_invalid_hmac_invalid(self):
-        """Should consider hmac invalid if it possesses too little variety."""
+        """Should consider an HMAC invalid if has fewer than 3 unique characters."""
         invalid_hmacs = (
             ("xxxxxxxxxxxxxxxxxx", "Single character"),
             ("XxXxXxXxXxXxXxXxXx", "Single character alternating case"),
@@ -111,7 +111,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase):
 
         for hmac, msg in invalid_hmacs:
             with self.subTest(msg=msg):
-                result = TokenRemover.is_maybevalid_hmac(hmac)
+                result = TokenRemover.is_maybe_valid_hmac(hmac)
                 self.assertFalse(result)
 
     def test_mod_log_property(self):
@@ -171,11 +171,11 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase):
         self.assertIsNone(return_value)
         token_re.finditer.assert_called_once_with(self.msg.content)
 
-    @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp", "is_maybevalid_hmac")
+    @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp", "is_maybe_valid_hmac")
     @autospec("bot.exts.filters.token_remover", "Token")
     @autospec("bot.exts.filters.token_remover", "TOKEN_RE")
-    def test_find_token_valid_match(self, token_re, token_cls, is_valid_id, is_valid_timestamp, is_maybevalid_hmac):
-        """The first match with a valid user ID. timestamp and hmac should be returned as a `Token`."""
+    def test_find_token_valid_match(self, token_re, token_cls, is_valid_id, is_valid_timestamp, is_maybe_valid_hmac):
+        """The first match with a valid user ID, timestamp, and HMAC 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),
@@ -189,23 +189,30 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase):
         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
-        is_maybevalid_hmac.return_value = True
+        is_maybe_valid_hmac.return_value = True
 
         return_value = TokenRemover.find_token_in_message(self.msg)
 
         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", "is_maybevalid_hmac")
+    @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp", "is_maybe_valid_hmac")
     @autospec("bot.exts.filters.token_remover", "Token")
     @autospec("bot.exts.filters.token_remover", "TOKEN_RE")
-    def test_find_token_invalid_matches(self, token_re, token_cls, is_valid_id, is_valid_timestamp, is_maybevalid_hmac):
+    def test_find_token_invalid_matches(
+        self,
+        token_re,
+        token_cls,
+        is_valid_id,
+        is_valid_timestamp,
+        is_maybe_valid_hmac,
+    ):
         """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
-        is_maybevalid_hmac.return_value = False
+        is_maybe_valid_hmac.return_value = False
 
         return_value = TokenRemover.find_token_in_message(self.msg)
 
@@ -261,18 +268,17 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase):
         results = [match[0] for match in results]
         self.assertCountEqual((token_1, token_2), results)
 
-    @autospec("bot.exts.filters.token_remover", "LOG_MESSAGE", "DECODED_LOG_MESSAGE")
-    def test_format_log_message(self, log_message, decoded_log_message):
+    @autospec("bot.exts.filters.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("NDcyMjY1OTQzMDYyNDEzMzMy", "XsySD_", "s45jqDV_Iisn-symw0yDRrk_jf4")
         log_message.format.return_value = "Howdy"
-        decoded_log_message.format.return_value = " Partner"
 
-        return_value = TokenRemover.format_log_message(self.msg, token, 472265943062413332, None)
+        return_value = TokenRemover.format_log_message(self.msg, token)
 
         self.assertEqual(
             return_value,
-            log_message.format.return_value + "\n" + decoded_log_message.format.return_value,
+            log_message.format.return_value,
         )
         log_message.format.assert_called_once_with(
             author=self.msg.author,
@@ -283,26 +289,38 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase):
             hmac="x" * len(token.hmac),
         )
 
-    @autospec("bot.exts.filters.token_remover", "LOG_MESSAGE", "USER_TOKEN_MESSAGE")
-    def test_format_log_message_user_token(self, log_message, user_token_message):
+    @autospec("bot.exts.filters.token_remover", "DECODED_LOG_MESSAGE")
+    def test_format_userid_log_message_bot(self, decoded_log_message):
+        """
+        Should correctly format the user ID portion of the log message when the user ID is
+         not found in the server.
+        """
+        token = Token("NDcyMjY1OTQzMDYyNDEzMzMy", "XsySD_", "s45jqDV_Iisn-symw0yDRrk_jf4")
+        decoded_log_message.format.return_value = " Partner"
+        msg = MockMessage(id=555, content="hello world")
+        msg.guild.get_member = MagicMock(return_value=None)
+
+        return_value = TokenRemover.format_userid_log_message(msg, token)
+
+        self.assertEqual(
+            return_value,
+            (decoded_log_message.format.return_value, False),
+        )
+        decoded_log_message.format.assert_called_once_with(
+            user_id=472265943062413332,
+        )
+
+    @autospec("bot.exts.filters.token_remover", "USER_TOKEN_MESSAGE")
+    def test_format_log_message_user_token_user(self, user_token_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"
         user_token_message.format.return_value = "Partner"
 
-        return_value = TokenRemover.format_log_message(self.msg, token, 467223230650777641, "Bob")
+        return_value = TokenRemover.format_userid_log_message(self.msg, token)
 
         self.assertEqual(
             return_value,
-            log_message.format.return_value + "\n" + user_token_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=token.user_id,
-            timestamp=token.timestamp,
-            hmac="x" * len(token.hmac),
+            (user_token_message.format.return_value, True),
         )
         user_token_message.format.assert_called_once_with(
             user_id=467223230650777641,
@@ -311,17 +329,19 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase):
 
     @mock.patch.object(TokenRemover, "mod_log", new_callable=mock.PropertyMock)
     @autospec("bot.exts.filters.token_remover", "log")
-    @autospec(TokenRemover, "format_log_message")
-    async def test_take_action(self, format_log_message, logger, mod_log_property):
+    @autospec(TokenRemover, "format_log_message", "format_userid_log_message")
+    async def test_take_action(self, format_log_message, format_userid_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 = mock.create_autospec(Token, spec_set=True, instance=True)
         token.user_id = "no-id"
         log_msg = "testing123"
+        userid_log_message = "userid-log-message"
 
         mod_log_property.return_value = mod_log
         format_log_message.return_value = log_msg
+        format_userid_log_message.return_value = (userid_log_message, True)
 
         await cog.take_action(self.msg, token)
 
@@ -330,7 +350,8 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase):
             token_remover.DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention)
         )
 
-        format_log_message.assert_called_once_with(self.msg, token, None, "Bob")
+        format_log_message.assert_called_once_with(self.msg, token)
+        format_userid_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")
 
@@ -339,7 +360,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase):
             icon_url=constants.Icons.token_removed,
             colour=Colour(constants.Colours.soft_red),
             title="Token removed!",
-            text=log_msg,
+            text=log_msg + "\n" + userid_log_message,
             thumbnail=self.msg.author.avatar_url_as.return_value,
             channel_id=constants.Channels.mod_alerts,
             ping_everyone=True,