From 933902810d2bba0310bbe7e9c95d796a35ab3d8a Mon Sep 17 00:00:00 2001 From: Kalle <38327916+Sendouc@users.noreply.github.com> Date: Sat, 1 Nov 2025 15:03:54 +0200 Subject: [PATCH] Migrate banned users fetching to Kysely --- .../admin/AdminRepository.server.test.ts | 303 ++++++++++++++++++ app/features/admin/AdminRepository.server.ts | 16 + app/features/admin/actions/admin.server.ts | 4 +- app/features/ban/core/banned.server.ts | 17 +- app/features/ban/loaders/suspended.server.ts | 5 +- .../ban/queries/allBannedUsers.server.ts | 29 -- app/features/sendouq/actions/q.server.ts | 2 +- 7 files changed, 330 insertions(+), 46 deletions(-) create mode 100644 app/features/admin/AdminRepository.server.test.ts delete mode 100644 app/features/ban/queries/allBannedUsers.server.ts diff --git a/app/features/admin/AdminRepository.server.test.ts b/app/features/admin/AdminRepository.server.test.ts new file mode 100644 index 000000000..cb85b40b1 --- /dev/null +++ b/app/features/admin/AdminRepository.server.test.ts @@ -0,0 +1,303 @@ +import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import * as UserRepository from "~/features/user-page/UserRepository.server"; +import { dbInsertUsers, dbReset } from "~/utils/Test"; +import * as AdminRepository from "./AdminRepository.server"; + +describe("allBannedUsers", () => { + beforeEach(async () => { + await dbInsertUsers(5); + }); + + afterEach(() => { + dbReset(); + }); + + test("returns empty Map when no users are banned", async () => { + const result = await AdminRepository.allBannedUsers(); + + expect(result.size).toBe(0); + }); + + test("returns Map with single banned user", async () => { + await AdminRepository.banUser({ + userId: 1, + banned: 1, + bannedReason: "Test ban", + bannedByUserId: 2, + }); + + const result = await AdminRepository.allBannedUsers(); + + expect(result.size).toBe(1); + expect(result.get(1)).toBeDefined(); + expect(result.get(1)?.userId).toBe(1); + expect(result.get(1)?.banned).toBe(1); + expect(result.get(1)?.bannedReason).toBe("Test ban"); + }); + + test("returns Map with multiple banned users", async () => { + await AdminRepository.banUser({ + userId: 1, + banned: 1, + bannedReason: "Reason 1", + bannedByUserId: 3, + }); + await AdminRepository.banUser({ + userId: 2, + banned: 1, + bannedReason: "Reason 2", + bannedByUserId: 3, + }); + + const result = await AdminRepository.allBannedUsers(); + + expect(result.size).toBe(2); + expect(result.get(1)?.userId).toBe(1); + expect(result.get(2)?.userId).toBe(2); + }); + + test("excludes non-banned users from results", async () => { + await AdminRepository.banUser({ + userId: 1, + banned: 1, + bannedReason: "Test ban", + bannedByUserId: 2, + }); + + const result = await AdminRepository.allBannedUsers(); + + expect(result.size).toBe(1); + expect(result.get(1)).toBeDefined(); + expect(result.get(2)).toBeUndefined(); + expect(result.get(3)).toBeUndefined(); + }); + + test("includes both permanently and temporarily banned users", async () => { + const futureDate = new Date(Date.now() + 1000 * 60 * 60 * 24 * 7); + + await AdminRepository.banUser({ + userId: 1, + banned: 1, + bannedReason: "Permanent ban", + bannedByUserId: 3, + }); + await AdminRepository.banUser({ + userId: 2, + banned: futureDate, + bannedReason: "Temporary ban", + bannedByUserId: 3, + }); + + const result = await AdminRepository.allBannedUsers(); + + expect(result.size).toBe(2); + expect(result.get(1)?.banned).toBe(1); + expect(result.get(2)?.banned).toBeGreaterThan(1); + }); +}); + +describe("banUser", () => { + beforeEach(async () => { + await dbInsertUsers(3); + }); + + afterEach(() => { + dbReset(); + }); + + test("permanently bans user (banned = 1)", async () => { + await AdminRepository.banUser({ + userId: 1, + banned: 1, + bannedReason: "Test permanent ban", + bannedByUserId: 2, + }); + + const result = await AdminRepository.allBannedUsers(); + + expect(result.get(1)?.banned).toBe(1); + expect(result.get(1)?.bannedReason).toBe("Test permanent ban"); + }); + + test("temporarily bans user (banned = Date)", async () => { + const futureDate = new Date(Date.now() + 1000 * 60 * 60 * 24 * 7); + + await AdminRepository.banUser({ + userId: 1, + banned: futureDate, + bannedReason: "Test temporary ban", + bannedByUserId: 2, + }); + + const result = await AdminRepository.allBannedUsers(); + + expect(result.get(1)?.banned).toBeGreaterThan(1); + expect(result.get(1)?.bannedReason).toBe("Test temporary ban"); + }); + + test("sets bannedReason correctly", async () => { + const reason = "Violating terms of service"; + + await AdminRepository.banUser({ + userId: 1, + banned: 1, + bannedReason: reason, + bannedByUserId: 2, + }); + + const result = await AdminRepository.allBannedUsers(); + + expect(result.get(1)?.bannedReason).toBe(reason); + }); + + test("creates BanLog entry when bannedByUserId is provided", async () => { + await AdminRepository.banUser({ + userId: 1, + banned: 1, + bannedReason: "Test ban", + bannedByUserId: 2, + }); + + const modInfo = await UserRepository.findModInfoById(1); + + expect(modInfo?.banLogs).toHaveLength(1); + expect(modInfo?.banLogs[0].banned).toBe(1); + expect(modInfo?.banLogs[0].bannedReason).toBe("Test ban"); + expect(modInfo?.banLogs[0].discordId).toBe("1"); + }); + + test("does not create BanLog when bannedByUserId is null (automatic ban)", async () => { + await AdminRepository.banUser({ + userId: 1, + banned: 1, + bannedReason: "Automatic ban", + bannedByUserId: null, + }); + + const modInfo = await UserRepository.findModInfoById(1); + + expect(modInfo?.banLogs).toHaveLength(0); + }); + + test("updates existing user correctly", async () => { + const bannedUsers = await AdminRepository.allBannedUsers(); + expect(bannedUsers.size).toBe(0); + + await AdminRepository.banUser({ + userId: 1, + banned: 1, + bannedReason: "First ban", + bannedByUserId: 2, + }); + + let result = await AdminRepository.allBannedUsers(); + expect(result.size).toBe(1); + + await AdminRepository.banUser({ + userId: 1, + banned: 1, + bannedReason: "Updated ban reason", + bannedByUserId: 2, + }); + + result = await AdminRepository.allBannedUsers(); + expect(result.size).toBe(1); + expect(result.get(1)?.bannedReason).toBe("Updated ban reason"); + + const modInfo = await UserRepository.findModInfoById(1); + expect(modInfo?.banLogs).toHaveLength(2); + expect(modInfo?.banLogs[0].bannedReason).toBe("First ban"); + expect(modInfo?.banLogs[1].bannedReason).toBe("Updated ban reason"); + }); +}); + +describe("unbanUser", () => { + beforeEach(async () => { + await dbInsertUsers(3); + }); + + afterEach(() => { + dbReset(); + }); + + test("unbans a previously banned user", async () => { + await AdminRepository.banUser({ + userId: 1, + banned: 1, + bannedReason: "Test ban", + bannedByUserId: 2, + }); + + let result = await AdminRepository.allBannedUsers(); + expect(result.size).toBe(1); + + await AdminRepository.unbanUser({ + userId: 1, + unbannedByUserId: 2, + }); + + result = await AdminRepository.allBannedUsers(); + expect(result.size).toBe(0); + }); + + test("creates BanLog entry with correct unbannedByUserId", async () => { + await AdminRepository.banUser({ + userId: 1, + banned: 1, + bannedReason: "Test ban", + bannedByUserId: 2, + }); + + await AdminRepository.unbanUser({ + userId: 1, + unbannedByUserId: 3, + }); + + const modInfo = await UserRepository.findModInfoById(1); + + expect(modInfo?.banLogs).toHaveLength(2); + + const unbanLog = modInfo?.banLogs.find((log) => log.banned === 0); + expect(unbanLog).toBeDefined(); + expect(unbanLog?.bannedReason).toBeNull(); + expect(unbanLog?.discordId).toBe("2"); + }); + + test("can unban permanently banned user", async () => { + await AdminRepository.banUser({ + userId: 1, + banned: 1, + bannedReason: "Permanent ban", + bannedByUserId: 2, + }); + + await AdminRepository.unbanUser({ + userId: 1, + unbannedByUserId: 2, + }); + + const result = await AdminRepository.allBannedUsers(); + + expect(result.size).toBe(0); + }); + + test("can unban temporarily banned user", async () => { + const futureDate = new Date(Date.now() + 1000 * 60 * 60 * 24 * 7); + + await AdminRepository.banUser({ + userId: 1, + banned: futureDate, + bannedReason: "Temporary ban", + bannedByUserId: 2, + }); + + await AdminRepository.unbanUser({ + userId: 1, + unbannedByUserId: 2, + }); + + const result = await AdminRepository.allBannedUsers(); + + expect(result.size).toBe(0); + }); +}); diff --git a/app/features/admin/AdminRepository.server.ts b/app/features/admin/AdminRepository.server.ts index 6fec3f68c..779fab534 100644 --- a/app/features/admin/AdminRepository.server.ts +++ b/app/features/admin/AdminRepository.server.ts @@ -247,6 +247,22 @@ export function forcePatron(args: { .execute(); } +export async function allBannedUsers() { + const rows = await db + .selectFrom("User") + .select(["User.id as userId", "User.banned", "User.bannedReason"]) + .where("User.banned", "!=", 0) + .execute(); + + const result: Map = new Map(); + + for (const row of rows) { + result.set(row.userId, row); + } + + return result; +} + export function banUser({ userId, banned, diff --git a/app/features/admin/actions/admin.server.ts b/app/features/admin/actions/admin.server.ts index 91aa16823..f554da9cf 100644 --- a/app/features/admin/actions/admin.server.ts +++ b/app/features/admin/actions/admin.server.ts @@ -129,7 +129,7 @@ export const action = async ({ request }: ActionFunctionArgs) => { bannedByUserId: user.id, }); - refreshBannedCache(); + await refreshBannedCache(); message = "User banned"; break; @@ -142,7 +142,7 @@ export const action = async ({ request }: ActionFunctionArgs) => { unbannedByUserId: user.id, }); - refreshBannedCache(); + await refreshBannedCache(); message = "User unbanned"; break; diff --git a/app/features/ban/core/banned.server.ts b/app/features/ban/core/banned.server.ts index 18c7c9172..0388fc08f 100644 --- a/app/features/ban/core/banned.server.ts +++ b/app/features/ban/core/banned.server.ts @@ -1,15 +1,10 @@ -import { cache, syncCached } from "~/utils/cache.server"; +import * as AdminRepository from "~/features/admin/AdminRepository.server"; import { databaseTimestampToDate } from "~/utils/dates"; -import { allBannedUsers } from "../queries/allBannedUsers.server"; -const BANNED_USERS_CACHE_KEY = "bannedUsers"; - -export function cachedBannedUsers() { - return syncCached(BANNED_USERS_CACHE_KEY, () => allBannedUsers()); -} +let bannedUsers = await AdminRepository.allBannedUsers(); export function userIsBanned(userId: number) { - const banStatus = cachedBannedUsers().get(userId); + const banStatus = bannedUsers.get(userId); if (!banStatus?.banned) return false; if (banStatus.banned === 1) return true; @@ -19,8 +14,6 @@ export function userIsBanned(userId: number) { return banExpiresAt > new Date(); } -export function refreshBannedCache() { - cache.delete(BANNED_USERS_CACHE_KEY); - - cachedBannedUsers(); +export async function refreshBannedCache() { + bannedUsers = await AdminRepository.allBannedUsers(); } diff --git a/app/features/ban/loaders/suspended.server.ts b/app/features/ban/loaders/suspended.server.ts index b81adfd3f..14fdc0120 100644 --- a/app/features/ban/loaders/suspended.server.ts +++ b/app/features/ban/loaders/suspended.server.ts @@ -1,18 +1,19 @@ import { type LoaderFunctionArgs, redirect } from "@remix-run/node"; +import * as AdminRepository from "~/features/admin/AdminRepository.server"; import { IMPERSONATED_SESSION_KEY, SESSION_KEY, } from "~/features/auth/core/authenticator.server"; import { authSessionStorage } from "~/features/auth/core/session.server"; import type { Nullish } from "~/utils/types"; -import { cachedBannedUsers, userIsBanned } from "../core/banned.server"; +import { userIsBanned } from "../core/banned.server"; export const loader = async ({ request }: LoaderFunctionArgs) => { const userId = await getUserIdEvenIfBanned(request); if (!userId || !userIsBanned(userId)) return redirect("/"); - const bannedStatus = cachedBannedUsers().get(userId)!; + const bannedStatus = (await AdminRepository.allBannedUsers()).get(userId)!; return { banned: bannedStatus.banned, diff --git a/app/features/ban/queries/allBannedUsers.server.ts b/app/features/ban/queries/allBannedUsers.server.ts deleted file mode 100644 index 4503d5097..000000000 --- a/app/features/ban/queries/allBannedUsers.server.ts +++ /dev/null @@ -1,29 +0,0 @@ -import { sql } from "~/db/sql"; -import type { Tables } from "~/db/tables"; - -const stm = sql.prepare(/*sql */ ` - select - "User"."id" as "userId", - "User"."banned", - "User"."bannedReason" - from - "User" - where - "User"."banned" != 0 -`); - -type BannedUserRow = Pick & { - userId: number; -}; - -export function allBannedUsers() { - const rows = stm.all() as Array; - - const result: Map = new Map(); - - for (const row of rows) { - result.set(row.userId, row); - } - - return result; -} diff --git a/app/features/sendouq/actions/q.server.ts b/app/features/sendouq/actions/q.server.ts index f172a05e2..3b089c6f4 100644 --- a/app/features/sendouq/actions/q.server.ts +++ b/app/features/sendouq/actions/q.server.ts @@ -113,7 +113,7 @@ export const action: ActionFunction = async ({ request }) => { bannedByUserId: null, }); - refreshBannedCache(); + await refreshBannedCache(); throw redirect(SUSPENDED_PAGE); }