feat: add ban mechanism by Fruktus · Pull Request #211 · Fruktus/QuadradiusPreservationProject

Fruktus · 2025-11-17T15:38:02Z

adds mechanism which will allow us to ban/unban users (in-game)

Fruktus · 2025-11-17T15:49:45Z

Fruktus · 2025-11-17T21:02:49Z

    async def test_migration_v6(self):
        await migrations.execute_migrations(self.c, self.dbconn.config, 5)

-        with patch('uuid.uuid4') as mock_uuid:


TLDR: Migration tests should not cover anything else than migrations, no other connector-related functions, since these are not versioned in the same way as migrations.

These tests had to go, since they rely on other functions, which have no concept of migration and do not work now (since at v6 there is no is_banned column) - this would be a problem with any other modification to users table.

This could be a separate patch

Created a separate PR, will update this one afterwards
#212

Folloup: the general idea was to remove the test which did not fit. The test did not fit, since it relied on connector's functions, which were updated without any relation to migrations, which caused it to fail here. The initial idea was to delete the test, since it tested the stuff outside of migration, but during the analysis, KJ noticed that the test was, actually, not misplaced, since it tested the migration from one set of data (matches) to two (matches and ranking scores). therefore, to make it work, we will replace the connector-related functions (which are subject to change) with raw sql queries

Fruktus · 2025-11-17T21:03:43Z

-    async def _send_user_notification(self, message: str) -> None:
-        # Send a message to the user notifications channel
-        if not self.user_notifications_channel_id:
+    async def _send_notification(self, message: str, channel: str) -> None:


I thought that duplicating this function for other notifications channel would be meh, so now there is a common one.

You can have both _send_notification and _send_user_notification

I could but I don't think it's a good idea, especially since the only difference is the target channel

Fruktus · 2025-11-17T21:03:57Z


+        if db_user.is_banned:
+            await self.send_msg(LobbyStateResponse.new([]))
+            await self.send_msg(LobbyChatMessage.new(None, f"You have been banned. Reason: {db_user.ban_reason}"))


I'm open to a different message

Fruktus · 2025-11-17T21:04:27Z

            cli_args=[],
            description='Maximum number of aliases per user',
            default_value=1)
+        self.discord_bot_channel_bans_notifications_id = ConfigKey(


Perhaps the description should indicate that ban channel and user notifications channel can be the same one

Fruktus · 2025-11-17T21:05:37Z


-        @self.tree.command(name="resetpassword", description="Reset password for a member")
+        @self.tree.command(name="resetpassword", description="Reset password for a member",
+                           guild=discord.Object(id=self.guild_id))


Missing guild ids are mistake from before, which no-one spotted - without these, the commands are added globally. We (me) probably added all of these during initial testing and did not notice later on. If this param is not present, the tree commands won't be synced to a given server

This could be a separate patch

Added in #213, not to mention that I still forgot to add it for other bot commands here :/ will update this PR afterwards

#213 merged, will rebase this

Fruktus · 2025-11-17T21:08:39Z

+        await discord_user.send(
+            f"### Ban"
+            f"- Your account: `{username}` has been banned."
+            f"- Reason: *{reason}*"


We could add info:

who was the one to ban

smth along the lines of "This does not grant you additional accounts to register"

kjarosh · 2025-11-22T19:40:39Z

            name='discord.bot.channel_user_notifications.id',
            cli_args=[],
-            description='Discord Channel ID for user notifications such as registration, claiming of account or bans',
+            description='Discord Channel ID for user notifications such as registration or claiming of account',


claiming accounts or claiming an account

kjarosh · 2025-11-22T19:41:03Z

            cli_args=[],
            description='Maximum number of aliases per user',
            default_value=1)
+        self.discord_bot_channel_bans_notifications_id = ConfigKey(


ban_notifications

kjarosh · 2025-11-22T19:41:20Z

            default_value=1)
+        self.discord_bot_channel_bans_notifications_id = ConfigKey(
+            config=self,
+            name='discord.bot.channel_bans_notifications.id',


ban_notifications

kjarosh · 2025-11-22T19:42:46Z

    await _set_version(c, 8)
+
+
+async def _migration_upgrade_to_v9(c, config):


Wouldn't it be better to store bans in a separate table and join?

maybe, can do, will do

kjarosh · 2025-11-22T19:43:06Z


-        @self.tree.command(name="resetpassword", description="Reset password for a member")
+        @self.tree.command(name="resetpassword", description="Reset password for a member",
+                           guild=discord.Object(id=self.guild_id))


This could be a separate patch

kjarosh · 2025-11-22T19:44:04Z

-    async def _send_user_notification(self, message: str) -> None:
-        # Send a message to the user notifications channel
-        if not self.user_notifications_channel_id:
+    async def _send_notification(self, message: str, channel: str) -> None:


You can have both _send_notification and _send_user_notification

kjarosh · 2025-11-22T19:45:26Z

    async def test_migration_v6(self):
        await migrations.execute_migrations(self.c, self.dbconn.config, 5)

-        with patch('uuid.uuid4') as mock_uuid:


This could be a separate patch

kjarosh · 2025-11-22T19:47:09Z

Can you split this into two PRs: database changes + tests and bot changes?

Fruktus · 2025-11-27T18:10:05Z

Can you split this into two PRs: database changes + tests and bot changes?

Sure, once I'll get back to this after tournaments

Fruktus requested a review from kjarosh as a code owner November 17, 2025 15:38

Fruktus force-pushed the feat/bot-bans branch from ce3801c to 2534cca Compare November 17, 2025 15:40

Fruktus force-pushed the feat/bot-bans branch 4 times, most recently from 1bcd442 to 6fb47f1 Compare November 17, 2025 20:58

Fruktus commented Nov 17, 2025

View reviewed changes

Fruktus force-pushed the feat/bot-bans branch from 6fb47f1 to 8f53475 Compare November 17, 2025 21:06

Fruktus commented Nov 17, 2025

View reviewed changes

Fruktus force-pushed the feat/bot-bans branch 4 times, most recently from a430754 to 2118485 Compare November 21, 2025 18:13

kjarosh reviewed Nov 22, 2025

View reviewed changes

Fruktus force-pushed the feat/bot-bans branch 2 times, most recently from 946ff37 to ee5ad01 Compare November 23, 2025 17:17

feat: add ban mechanism

8d2981a

Fruktus force-pushed the feat/bot-bans branch from ee5ad01 to 8d2981a Compare November 23, 2025 17:28

		await _set_version(c, 8)


		async def _migration_upgrade_to_v9(c, config):

Conversation

Fruktus commented Nov 17, 2025

Uh oh!

Fruktus commented Nov 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fruktus Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjarosh commented Nov 22, 2025

Uh oh!

Fruktus commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fruktus Nov 17, 2025 •

edited

Loading