From 6334275d02fc7126a14081377aaed8011839bb82 Mon Sep 17 00:00:00 2001 From: syntaxbullet Date: Wed, 24 Dec 2025 21:23:58 +0100 Subject: [PATCH] refactor: modernize transaction patterns and improve type safety - Refactored user.service.ts to use withTransaction() helper - Added 14 comprehensive unit tests for user.service.ts - Removed duplicate user creation in interactionCreate.ts - Improved type safety in interaction.routes.ts --- src/events/interactionCreate.ts | 8 +- src/lib/interaction.routes.ts | 16 ++- src/modules/user/user.service.test.ts | 162 +++++++++++++++++++++++++- src/modules/user/user.service.ts | 39 ++++--- 4 files changed, 197 insertions(+), 28 deletions(-) diff --git a/src/events/interactionCreate.ts b/src/events/interactionCreate.ts index 5461c81..ed61724 100644 --- a/src/events/interactionCreate.ts +++ b/src/events/interactionCreate.ts @@ -48,13 +48,9 @@ const event: Event = { // Ensure user exists in database try { - const user = await userService.getUserById(interaction.user.id); - if (!user) { - console.log(`🆕 Creating new user entry for ${interaction.user.tag}`); - await userService.createUser(interaction.user.id, interaction.user.username); - } + await userService.getOrCreateUser(interaction.user.id, interaction.user.username); } catch (error) { - console.error("Failed to check/create user:", error); + console.error("Failed to ensure user exists:", error); } try { diff --git a/src/lib/interaction.routes.ts b/src/lib/interaction.routes.ts index 0e7f536..4bf6fc8 100644 --- a/src/lib/interaction.routes.ts +++ b/src/lib/interaction.routes.ts @@ -1,10 +1,20 @@ import { ButtonInteraction, ModalSubmitInteraction, StringSelectMenuInteraction } from "discord.js"; -type InteractionHandler = (interaction: any) => Promise; +// Union type for all component interactions +type ComponentInteraction = ButtonInteraction | StringSelectMenuInteraction | ModalSubmitInteraction; +// Type for the handler function that modules export +type InteractionHandler = (interaction: ComponentInteraction) => Promise; + +// Type for the dynamically imported module containing the handler +interface InteractionModule { + [key: string]: (...args: any[]) => Promise | any; +} + +// Route definition interface InteractionRoute { - predicate: (interaction: ButtonInteraction | StringSelectMenuInteraction | ModalSubmitInteraction) => boolean; - handler: () => Promise; + predicate: (interaction: ComponentInteraction) => boolean; + handler: () => Promise; method: string; } diff --git a/src/modules/user/user.service.test.ts b/src/modules/user/user.service.test.ts index d1d8b46..168cdd7 100644 --- a/src/modules/user/user.service.test.ts +++ b/src/modules/user/user.service.test.ts @@ -20,6 +20,7 @@ mockUpdate.mockReturnValue({ set: mockSet }); mockSet.mockReturnValue({ where: mockWhere }); mockWhere.mockReturnValue({ returning: mockReturning }); +mockDelete.mockReturnValue({ where: mockWhere }); // Mock DrizzleClient mock.module("@/lib/DrizzleClient", () => { @@ -51,12 +52,39 @@ mock.module("@/lib/DrizzleClient", () => { }; }); +// Mock withTransaction helper to use the same pattern as DrizzleClient.transaction +mock.module("@/lib/db", () => { + return { + withTransaction: async (callback: any, tx?: any) => { + if (tx) { + return callback(tx); + } + // Simulate transaction by calling the callback with mock db + return callback({ + query: { + users: { + findFirst: mockFindFirst, + }, + }, + insert: mockInsert, + update: mockUpdate, + delete: mockDelete, + }); + } + }; +}); + + describe("userService", () => { beforeEach(() => { mockFindFirst.mockReset(); mockInsert.mockClear(); mockValues.mockClear(); mockReturning.mockClear(); + mockUpdate.mockClear(); + mockSet.mockClear(); + mockWhere.mockClear(); + mockDelete.mockClear(); }); describe("getUserById", () => { @@ -80,7 +108,91 @@ describe("userService", () => { }); }); - describe("createUser", () => { + describe("getUserByUsername", () => { + it("should return user when username exists", async () => { + const mockUser = { id: 456n, username: "alice", balance: 100n }; + mockFindFirst.mockResolvedValue(mockUser); + + const result = await userService.getUserByUsername("alice"); + + expect(result).toEqual(mockUser as any); + expect(mockFindFirst).toHaveBeenCalledTimes(1); + }); + + it("should return undefined when username not found", async () => { + mockFindFirst.mockResolvedValue(undefined); + + const result = await userService.getUserByUsername("nonexistent"); + + expect(result).toBeUndefined(); + }); + }); + + describe("getUserClass", () => { + it("should return user class when user has a class", async () => { + const mockClass = { id: 1n, name: "Warrior", emoji: "⚔️" }; + const mockUser = { id: 123n, username: "testuser", class: mockClass }; + mockFindFirst.mockResolvedValue(mockUser); + + const result = await userService.getUserClass("123"); + + expect(result).toEqual(mockClass as any); + }); + + it("should return null when user has no class", async () => { + const mockUser = { id: 123n, username: "testuser", class: null }; + mockFindFirst.mockResolvedValue(mockUser); + + const result = await userService.getUserClass("123"); + + expect(result).toBeNull(); + }); + + it("should return undefined when user not found", async () => { + mockFindFirst.mockResolvedValue(undefined); + + const result = await userService.getUserClass("999"); + + expect(result).toBeUndefined(); + }); + }); + + describe("getOrCreateUser (withTransaction)", () => { + it("should return existing user if found", async () => { + const mockUser = { id: 123n, username: "existinguser", class: null }; + mockFindFirst.mockResolvedValue(mockUser); + + const result = await userService.getOrCreateUser("123", "existinguser"); + + expect(result).toEqual(mockUser as any); + expect(mockFindFirst).toHaveBeenCalledTimes(1); + expect(mockInsert).not.toHaveBeenCalled(); + }); + + it("should create new user if not found", async () => { + const newUser = { id: 789n, username: "newuser", classId: null }; + + // First call returns undefined (user not found) + // Second call returns the newly created user (after insert + re-query) + mockFindFirst + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce({ id: 789n, username: "newuser", class: null }); + + mockReturning.mockResolvedValue([newUser]); + + const result = await userService.getOrCreateUser("789", "newuser"); + + expect(mockInsert).toHaveBeenCalledTimes(1); + expect(mockValues).toHaveBeenCalledWith({ + id: 789n, + username: "newuser" + }); + // Should query twice: once to check, once after insert + expect(mockFindFirst).toHaveBeenCalledTimes(2); + }); + }); + + describe("createUser (withTransaction)", () => { it("should create and return a new user", async () => { const newUser = { id: 456n, username: "newuser", classId: null }; mockReturning.mockResolvedValue([newUser]); @@ -95,5 +207,53 @@ describe("userService", () => { classId: undefined }); }); + + it("should create user with classId when provided", async () => { + const newUser = { id: 999n, username: "warrior", classId: 5n }; + mockReturning.mockResolvedValue([newUser]); + + const result = await userService.createUser("999", "warrior", 5n); + + expect(result).toEqual(newUser as any); + expect(mockValues).toHaveBeenCalledWith({ + id: 999n, + username: "warrior", + classId: 5n + }); + }); + }); + + describe("updateUser (withTransaction)", () => { + it("should update user data", async () => { + const updatedUser = { id: 123n, username: "testuser", balance: 500n }; + mockReturning.mockResolvedValue([updatedUser]); + + const result = await userService.updateUser("123", { balance: 500n }); + + expect(result).toEqual(updatedUser as any); + expect(mockUpdate).toHaveBeenCalledTimes(1); + expect(mockSet).toHaveBeenCalledWith({ balance: 500n }); + }); + + it("should update multiple fields", async () => { + const updatedUser = { id: 456n, username: "alice", xp: 100n, level: 5 }; + mockReturning.mockResolvedValue([updatedUser]); + + const result = await userService.updateUser("456", { xp: 100n, level: 5 }); + + expect(result).toEqual(updatedUser as any); + expect(mockSet).toHaveBeenCalledWith({ xp: 100n, level: 5 }); + }); + }); + + describe("deleteUser (withTransaction)", () => { + it("should delete user from database", async () => { + mockWhere.mockResolvedValue(undefined); + + await userService.deleteUser("123"); + + expect(mockDelete).toHaveBeenCalledTimes(1); + expect(mockWhere).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/src/modules/user/user.service.ts b/src/modules/user/user.service.ts index 29eb2f7..759385c 100644 --- a/src/modules/user/user.service.ts +++ b/src/modules/user/user.service.ts @@ -1,6 +1,8 @@ import { users } from "@/db/schema"; import { eq } from "drizzle-orm"; import { DrizzleClient } from "@/lib/DrizzleClient"; +import { withTransaction } from "@/lib/db"; +import type { Transaction } from "@/lib/types"; export const userService = { getUserById: async (id: string) => { @@ -14,23 +16,27 @@ export const userService = { const user = await DrizzleClient.query.users.findFirst({ where: eq(users.username, username) }); return user; }, - getOrCreateUser: async (id: string, username: string, tx?: any) => { - const execute = async (txFn: any) => { + getOrCreateUser: async (id: string, username: string, tx?: Transaction) => { + return await withTransaction(async (txFn) => { let user = await txFn.query.users.findFirst({ where: eq(users.id, BigInt(id)), with: { class: true } }); if (!user) { - const [newUser] = await txFn.insert(users).values({ + await txFn.insert(users).values({ id: BigInt(id), username, }).returning(); - user = { ...newUser, class: null }; + + // Re-query to get the user with class relation + user = await txFn.query.users.findFirst({ + where: eq(users.id, BigInt(id)), + with: { class: true } + }); } return user; - }; - return tx ? await execute(tx) : await DrizzleClient.transaction(execute); + }, tx); }, getUserClass: async (id: string) => { const user = await DrizzleClient.query.users.findFirst({ @@ -39,31 +45,28 @@ export const userService = { }); return user?.class; }, - createUser: async (id: string | bigint, username: string, classId?: bigint, tx?: any) => { - const execute = async (txFn: any) => { + createUser: async (id: string | bigint, username: string, classId?: bigint, tx?: Transaction) => { + return await withTransaction(async (txFn) => { const [user] = await txFn.insert(users).values({ id: BigInt(id), username, classId, }).returning(); return user; - }; - return tx ? await execute(tx) : await DrizzleClient.transaction(execute); + }, tx); }, - updateUser: async (id: string, data: Partial, tx?: any) => { - const execute = async (txFn: any) => { + updateUser: async (id: string, data: Partial, tx?: Transaction) => { + return await withTransaction(async (txFn) => { const [user] = await txFn.update(users) .set(data) .where(eq(users.id, BigInt(id))) .returning(); return user; - }; - return tx ? await execute(tx) : await DrizzleClient.transaction(execute); + }, tx); }, - deleteUser: async (id: string, tx?: any) => { - const execute = async (txFn: any) => { + deleteUser: async (id: string, tx?: Transaction) => { + return await withTransaction(async (txFn) => { await txFn.delete(users).where(eq(users.id, BigInt(id))); - }; - return tx ? await execute(tx) : await DrizzleClient.transaction(execute); + }, tx); }, }; \ No newline at end of file