From df897e57ea0dbbaeb3e2a07a1e443c5b2df34c36 Mon Sep 17 00:00:00 2001 From: Hassan Abouelela <47495861+HassanAbouelela@users.noreply.github.com> Date: Mon, 30 Nov 2020 22:26:23 +0300 Subject: [PATCH 1/4] Adds Member Checks Before Changing Voice Adds a check that checks if the user object is an instance of guild member, before performing guild operations. Adds tests. Signed-off-by: Hassan Abouelela <47495861+HassanAbouelela@users.noreply.github.com> --- bot/exts/moderation/infraction/infractions.py | 12 ++++++++++-- .../moderation/infraction/test_infractions.py | 15 ++++++++++++++- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/bot/exts/moderation/infraction/infractions.py b/bot/exts/moderation/infraction/infractions.py index 18e937e872..78c326c47e 100644 --- a/bot/exts/moderation/infraction/infractions.py +++ b/bot/exts/moderation/infraction/infractions.py @@ -257,6 +257,10 @@ async def apply_mute(self, ctx: Context, user: Member, reason: t.Optional[str], self.mod_log.ignore(Event.member_update, user.id) async def action() -> None: + # Skip members that left the server + if not isinstance(user, Member): + return + await user.add_roles(self._muted_role, reason=reason) log.trace(f"Attempting to kick {user} from voice because they've been muted.") @@ -351,9 +355,13 @@ async def apply_voice_ban(self, ctx: Context, user: UserSnowflake, reason: t.Opt if reason: reason = textwrap.shorten(reason, width=512, placeholder="...") - await user.move_to(None, reason="Disconnected from voice to apply voiceban.") + action = None + + # Skip members that left the server + if isinstance(user, Member): + await user.move_to(None, reason="Disconnected from voice to apply voiceban.") + action = user.remove_roles(self._voice_verified_role, reason=reason) - action = user.remove_roles(self._voice_verified_role, reason=reason) await self.apply_infraction(ctx, infraction, user, action) # endregion diff --git a/tests/bot/exts/moderation/infraction/test_infractions.py b/tests/bot/exts/moderation/infraction/test_infractions.py index bf557a4843..4ba5a4feb0 100644 --- a/tests/bot/exts/moderation/infraction/test_infractions.py +++ b/tests/bot/exts/moderation/infraction/test_infractions.py @@ -4,7 +4,7 @@ from bot.constants import Event from bot.exts.moderation.infraction.infractions import Infractions -from tests.helpers import MockBot, MockContext, MockGuild, MockMember, MockRole +from tests.helpers import MockBot, MockContext, MockGuild, MockMember, MockRole, MockUser class TruncationTests(unittest.IsolatedAsyncioTestCase): @@ -164,6 +164,19 @@ async def test_voice_ban_truncate_reason(self, get_active_infraction, post_infra ) self.cog.apply_infraction.assert_awaited_once_with(self.ctx, {"foo": "bar"}, self.user, "my_return_value") + @patch("bot.exts.moderation.infraction._utils.get_active_infraction", return_value=None) + @patch("bot.exts.moderation.infraction._utils.post_infraction") + @patch("bot.exts.moderation.infraction.infractions.Infractions.apply_infraction") + async def test_voice_ban_user_left_guild(self, apply_infraction_mock, post_infraction_mock, _): + """Should voice ban user that left the guild without throwing an error.""" + infraction = {"foo": "bar"} + post_infraction_mock.return_value = {"foo": "bar"} + + user = MockUser() + await self.cog.voiceban(self.cog, self.ctx, user, reason=None) + post_infraction_mock.assert_called_once_with(self.ctx, user, "voice_ban", None, active=True) + apply_infraction_mock.assert_called_once_with(self.ctx, infraction, user, None) + async def test_voice_unban_user_not_found(self): """Should include info to return dict when user was not found from guild.""" self.guild.get_member.return_value = None From 37bfd7569cdeb0900ee131c22f67aed3f78692ba Mon Sep 17 00:00:00 2001 From: Hassan Abouelela <47495861+HassanAbouelela@users.noreply.github.com> Date: Wed, 20 Jan 2021 20:19:47 +0300 Subject: [PATCH 2/4] Cleans Up Tests --- .../bot/exts/moderation/infraction/test_infractions.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/bot/exts/moderation/infraction/test_infractions.py b/tests/bot/exts/moderation/infraction/test_infractions.py index 4ba5a4feb0..13efee0543 100644 --- a/tests/bot/exts/moderation/infraction/test_infractions.py +++ b/tests/bot/exts/moderation/infraction/test_infractions.py @@ -3,8 +3,9 @@ from unittest.mock import AsyncMock, MagicMock, Mock, patch from bot.constants import Event +from bot.exts.moderation.infraction import _utils from bot.exts.moderation.infraction.infractions import Infractions -from tests.helpers import MockBot, MockContext, MockGuild, MockMember, MockRole, MockUser +from tests.helpers import MockBot, MockContext, MockGuild, MockMember, MockRole, MockUser, autospec class TruncationTests(unittest.IsolatedAsyncioTestCase): @@ -164,9 +165,8 @@ async def test_voice_ban_truncate_reason(self, get_active_infraction, post_infra ) self.cog.apply_infraction.assert_awaited_once_with(self.ctx, {"foo": "bar"}, self.user, "my_return_value") - @patch("bot.exts.moderation.infraction._utils.get_active_infraction", return_value=None) - @patch("bot.exts.moderation.infraction._utils.post_infraction") - @patch("bot.exts.moderation.infraction.infractions.Infractions.apply_infraction") + @autospec(_utils, "post_infraction", "get_active_infraction", return_value=None) + @autospec(Infractions, "apply_infraction") async def test_voice_ban_user_left_guild(self, apply_infraction_mock, post_infraction_mock, _): """Should voice ban user that left the guild without throwing an error.""" infraction = {"foo": "bar"} @@ -175,7 +175,7 @@ async def test_voice_ban_user_left_guild(self, apply_infraction_mock, post_infra user = MockUser() await self.cog.voiceban(self.cog, self.ctx, user, reason=None) post_infraction_mock.assert_called_once_with(self.ctx, user, "voice_ban", None, active=True) - apply_infraction_mock.assert_called_once_with(self.ctx, infraction, user, None) + apply_infraction_mock.assert_called_once_with(self.cog, self.ctx, infraction, user, None) async def test_voice_unban_user_not_found(self): """Should include info to return dict when user was not found from guild.""" From 06c5eba63f735237b44f2b1da9944bc7cddd2177 Mon Sep 17 00:00:00 2001 From: Hassan Abouelela <47495861+HassanAbouelela@users.noreply.github.com> Date: Thu, 21 Jan 2021 01:26:41 +0300 Subject: [PATCH 3/4] Restructures Voice Ban Action Updates the voice ban action so the infraction pardoning is always run, and so all operations are handled in the _scheduler. Updates tests. --- bot/exts/moderation/infraction/infractions.py | 11 +++--- .../moderation/infraction/test_infractions.py | 37 +++++++++++++------ 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/bot/exts/moderation/infraction/infractions.py b/bot/exts/moderation/infraction/infractions.py index 78c326c47e..be4327bb0c 100644 --- a/bot/exts/moderation/infraction/infractions.py +++ b/bot/exts/moderation/infraction/infractions.py @@ -355,14 +355,15 @@ async def apply_voice_ban(self, ctx: Context, user: UserSnowflake, reason: t.Opt if reason: reason = textwrap.shorten(reason, width=512, placeholder="...") - action = None + async def action() -> None: + # Skip members that left the server + if not isinstance(user, Member): + return - # Skip members that left the server - if isinstance(user, Member): await user.move_to(None, reason="Disconnected from voice to apply voiceban.") - action = user.remove_roles(self._voice_verified_role, reason=reason) + await user.remove_roles(self._voice_verified_role, reason=reason) - await self.apply_infraction(ctx, infraction, user, action) + await self.apply_infraction(ctx, infraction, user, action()) # endregion # region: Base pardon functions diff --git a/tests/bot/exts/moderation/infraction/test_infractions.py b/tests/bot/exts/moderation/infraction/test_infractions.py index 13efee0543..86c2617ea5 100644 --- a/tests/bot/exts/moderation/infraction/test_infractions.py +++ b/tests/bot/exts/moderation/infraction/test_infractions.py @@ -1,6 +1,7 @@ +import inspect import textwrap import unittest -from unittest.mock import AsyncMock, MagicMock, Mock, patch +from unittest.mock import ANY, AsyncMock, MagicMock, Mock, patch from bot.constants import Event from bot.exts.moderation.infraction import _utils @@ -133,20 +134,29 @@ async def test_voice_ban_mod_log_ignore(self, get_active_infraction, post_infrac self.assertIsNone(await self.cog.apply_voice_ban(self.ctx, self.user, "foobar")) self.cog.mod_log.ignore.assert_called_once_with(Event.member_update, self.user.id) + async def action_tester(self, action, reason: str) -> None: + """Helper method to test voice ban action.""" + self.assertTrue(inspect.iscoroutine(action)) + await action + + self.user.move_to.assert_called_once_with(None, reason=ANY) + self.user.remove_roles.assert_called_once_with(self.cog._voice_verified_role, reason=reason) + @patch("bot.exts.moderation.infraction.infractions._utils.post_infraction") @patch("bot.exts.moderation.infraction.infractions._utils.get_active_infraction") async def test_voice_ban_apply_infraction(self, get_active_infraction, post_infraction_mock): """Should ignore Voice Verified role removing.""" self.cog.mod_log.ignore = MagicMock() self.cog.apply_infraction = AsyncMock() - self.user.remove_roles = MagicMock(return_value="my_return_value") get_active_infraction.return_value = None post_infraction_mock.return_value = {"foo": "bar"} - self.assertIsNone(await self.cog.apply_voice_ban(self.ctx, self.user, "foobar")) - self.user.remove_roles.assert_called_once_with(self.cog._voice_verified_role, reason="foobar") - self.cog.apply_infraction.assert_awaited_once_with(self.ctx, {"foo": "bar"}, self.user, "my_return_value") + reason = "foobar" + self.assertIsNone(await self.cog.apply_voice_ban(self.ctx, self.user, reason)) + self.cog.apply_infraction.assert_awaited_once_with(self.ctx, {"foo": "bar"}, self.user, ANY) + + await self.action_tester(self.cog.apply_infraction.call_args[0][-1], reason) @patch("bot.exts.moderation.infraction.infractions._utils.post_infraction") @patch("bot.exts.moderation.infraction.infractions._utils.get_active_infraction") @@ -154,16 +164,16 @@ async def test_voice_ban_truncate_reason(self, get_active_infraction, post_infra """Should truncate reason for voice ban.""" self.cog.mod_log.ignore = MagicMock() self.cog.apply_infraction = AsyncMock() - self.user.remove_roles = MagicMock(return_value="my_return_value") get_active_infraction.return_value = None post_infraction_mock.return_value = {"foo": "bar"} self.assertIsNone(await self.cog.apply_voice_ban(self.ctx, self.user, "foobar" * 3000)) - self.user.remove_roles.assert_called_once_with( - self.cog._voice_verified_role, reason=textwrap.shorten("foobar" * 3000, 512, placeholder="...") - ) - self.cog.apply_infraction.assert_awaited_once_with(self.ctx, {"foo": "bar"}, self.user, "my_return_value") + self.cog.apply_infraction.assert_awaited_once_with(self.ctx, {"foo": "bar"}, self.user, ANY) + + # Test action + action = self.cog.apply_infraction.call_args[0][-1] + await self.action_tester(action, textwrap.shorten("foobar" * 3000, 512, placeholder="...")) @autospec(_utils, "post_infraction", "get_active_infraction", return_value=None) @autospec(Infractions, "apply_infraction") @@ -175,7 +185,12 @@ async def test_voice_ban_user_left_guild(self, apply_infraction_mock, post_infra user = MockUser() await self.cog.voiceban(self.cog, self.ctx, user, reason=None) post_infraction_mock.assert_called_once_with(self.ctx, user, "voice_ban", None, active=True) - apply_infraction_mock.assert_called_once_with(self.cog, self.ctx, infraction, user, None) + apply_infraction_mock.assert_called_once_with(self.cog, self.ctx, infraction, user, ANY) + + # Test action + action = self.cog.apply_infraction.call_args[0][-1] + self.assertTrue(inspect.iscoroutine(action)) + await action async def test_voice_unban_user_not_found(self): """Should include info to return dict when user was not found from guild.""" From a41b932436b8b5c07c0f5ea3bd7ee886fa01e88e Mon Sep 17 00:00:00 2001 From: Hassan Abouelela <47495861+HassanAbouelela@users.noreply.github.com> Date: Thu, 21 Jan 2021 02:25:46 +0300 Subject: [PATCH 4/4] Updates Apply Infraction Docstring --- bot/exts/moderation/infraction/_scheduler.py | 1 + 1 file changed, 1 insertion(+) diff --git a/bot/exts/moderation/infraction/_scheduler.py b/bot/exts/moderation/infraction/_scheduler.py index 242b2d30f7..a73f2e8daa 100644 --- a/bot/exts/moderation/infraction/_scheduler.py +++ b/bot/exts/moderation/infraction/_scheduler.py @@ -102,6 +102,7 @@ async def apply_infraction( """ Apply an infraction to the user, log the infraction, and optionally notify the user. + `action_coro`, if not provided, will result in the infraction not getting scheduled for deletion. `user_reason`, if provided, will be sent to the user in place of the infraction reason. `additional_info` will be attached to the text field in the mod-log embed.