Silence: refactor _silence

* Rename to `_silence_overwrites`
* Reduce responsibilities to only setting permission overwrites
* Log in `silence` instead
* Add to notifier in `silence` instead

2 files changed, 32 insertions, 33 deletions

diff --git a/bot/cogs/moderation/silence.py b/bot/cogs/moderation/silence.py
index 08d0328ab..12896022f 100644
--- a/bot/cogs/moderation/silence.py
+++ b/bot/cogs/moderation/silence.py
@@ -104,17 +104,23 @@ class Silence(commands.Cog):
         Indefinitely silenced channels get added to a notifier which posts notices every 15 minutes from the start.
         """
         await self._init_task
-        log.debug(f"{ctx.author} is silencing channel #{ctx.channel}.")
 
-        if not await self._silence(ctx.channel, persistent=(duration is None), duration=duration):
+        channel_info = f"#{ctx.channel} ({ctx.channel.id})"
+        log.debug(f"{ctx.author} is silencing channel {channel_info}.")
+
+        if not await self._silence_overwrites(ctx.channel):
+            log.info(f"Tried to silence channel {channel_info} but the channel was already silenced.")
             await ctx.send(MSG_SILENCE_FAIL)
             return
 
         await self._schedule_unsilence(ctx, duration)
 
         if duration is None:
+            log.info(f"Silenced {channel_info} indefinitely.")
             await ctx.send(MSG_SILENCE_PERMANENT)
         else:
+            self.notifier.add_channel(ctx.channel)
+            log.info(f"Silenced {channel_info} for {duration} minute(s).")
             await ctx.send(MSG_SILENCE_SUCCESS.format(duration=duration))
 
     @commands.command(aliases=("unhush",))
@@ -139,31 +145,18 @@ class Silence(commands.Cog):
         else:
             await channel.send(MSG_UNSILENCE_SUCCESS)
 
-    async def _silence(self, channel: TextChannel, persistent: bool, duration: Optional[int]) -> bool:
-        """
-        Silence `channel` for `self._verified_role`.
-
-        If `persistent` is `True` add `channel` to notifier.
-        `duration` is only used for logging; if None is passed `persistent` should be True to not log None.
-        Return `True` if channel permissions were changed, `False` otherwise.
-        """
+    async def _silence_overwrites(self, channel: TextChannel) -> bool:
+        """Set silence permission overwrites for `channel` and return True if successful."""
         overwrite = channel.overwrites_for(self._verified_role)
         prev_overwrites = dict(send_messages=overwrite.send_messages, add_reactions=overwrite.add_reactions)
 
         if channel.id in self.scheduler or all(val is False for val in prev_overwrites.values()):
-            log.info(f"Tried to silence channel #{channel} ({channel.id}) but the channel was already silenced.")
             return False
 
         overwrite.update(send_messages=False, add_reactions=False)
         await channel.set_permissions(self._verified_role, overwrite=overwrite)
         await self.previous_overwrites.set(channel.id, json.dumps(prev_overwrites))
 
-        if persistent:
-            log.info(f"Silenced #{channel} ({channel.id}) indefinitely.")
-            self.notifier.add_channel(channel)
-            return True
-
-        log.info(f"Silenced #{channel} ({channel.id}) for {duration} minute(s).")
         return True
 
     async def _schedule_unsilence(self, ctx: Context, duration: Optional[int]) -> None:
diff --git a/tests/bot/cogs/moderation/test_silence.py b/tests/bot/cogs/moderation/test_silence.py
index d56a731b6..9dbdfd10a 100644
--- a/tests/bot/cogs/moderation/test_silence.py
+++ b/tests/bot/cogs/moderation/test_silence.py
@@ -245,8 +245,8 @@ class SilenceTests(unittest.IsolatedAsyncioTestCase):
         )
         for duration, message, was_silenced in test_cases:
             ctx = MockContext()
-            with self.subTest(was_silenced=was_silenced, message=message, duration=duration):
-                with mock.patch.object(self.cog, "_silence", return_value=was_silenced):
+            with mock.patch.object(self.cog, "_silence_overwrites", return_value=was_silenced):
+                with self.subTest(was_silenced=was_silenced, message=message, duration=duration):
                     await self.cog.silence.callback(self.cog, ctx, duration)
                     ctx.send.assert_called_once_with(message)
 
@@ -264,12 +264,12 @@ class SilenceTests(unittest.IsolatedAsyncioTestCase):
                 channel = MockTextChannel()
                 channel.overwrites_for.return_value = overwrite
 
-                self.assertFalse(await self.cog._silence(channel, True, None))
+                self.assertFalse(await self.cog._silence_overwrites(channel))
                 channel.set_permissions.assert_not_called()
 
     async def test_silenced_channel(self):
         """Channel had `send_message` and `add_reactions` permissions revoked for verified role."""
-        self.assertTrue(await self.cog._silence(self.channel, False, None))
+        self.assertTrue(await self.cog._silence_overwrites(self.channel))
         self.assertFalse(self.overwrite.send_messages)
         self.assertFalse(self.overwrite.add_reactions)
         self.channel.set_permissions.assert_awaited_once_with(
@@ -280,7 +280,7 @@ class SilenceTests(unittest.IsolatedAsyncioTestCase):
     async def test_preserved_other_overwrites(self):
         """Channel's other unrelated overwrites were not changed."""
         prev_overwrite_dict = dict(self.overwrite)
-        await self.cog._silence(self.channel, False, None)
+        await self.cog._silence_overwrites(self.channel)
         new_overwrite_dict = dict(self.overwrite)
 
         # Remove 'send_messages' & 'add_reactions' keys because they were changed by the method.
@@ -291,22 +291,28 @@ class SilenceTests(unittest.IsolatedAsyncioTestCase):
 
         self.assertDictEqual(prev_overwrite_dict, new_overwrite_dict)
 
-    async def test_added_removed_notifier(self):
-        """Channel was added to notifier if `persistent` was `True`, and removed if `False`."""
-        with mock.patch.object(self.cog, "notifier", create=True):
-            with self.subTest(persistent=True):
-                await self.cog._silence(self.channel, True, None)
-                self.cog.notifier.add_channel.assert_called_once()
+    async def test_temp_added_to_notifier(self):
+        """Channel was added to notifier if a duration was set for the silence."""
+        with mock.patch.object(self.cog, "_silence_overwrites", return_value=True):
+            await self.cog.silence.callback(self.cog, MockContext(), 15)
+            self.cog.notifier.add_channel.assert_called_once()
 
-        with mock.patch.object(self.cog, "notifier", create=True):
-            with self.subTest(persistent=False):
-                await self.cog._silence(self.channel, False, None)
-                self.cog.notifier.add_channel.assert_not_called()
+    async def test_indefinite_not_added_to_notifier(self):
+        """Channel was not added to notifier if a duration was not set for the silence."""
+        with mock.patch.object(self.cog, "_silence_overwrites", return_value=True):
+            await self.cog.silence.callback(self.cog, MockContext(), None)
+            self.cog.notifier.add_channel.assert_not_called()
+
+    async def test_silenced_not_added_to_notifier(self):
+        """Channel was not added to the notifier if it was already silenced."""
+        with mock.patch.object(self.cog, "_silence_overwrites", return_value=False):
+            await self.cog.silence.callback(self.cog, MockContext(), 15)
+            self.cog.notifier.add_channel.assert_not_called()
 
     async def test_cached_previous_overwrites(self):
         """Channel's previous overwrites were cached."""
         overwrite_json = '{"send_messages": true, "add_reactions": false}'
-        await self.cog._silence(self.channel, False, None)
+        await self.cog._silence_overwrites(self.channel)
         self.cog.previous_overwrites.set.assert_called_once_with(self.channel.id, overwrite_json)
 
     @autospec(silence, "datetime")