diff --git a/src/server/api/routers/expense.ts b/src/server/api/routers/expense.ts index ea821f39..0def7dd5 100644 --- a/src/server/api/routers/expense.ts +++ b/src/server/api/routers/expense.ts @@ -13,7 +13,12 @@ import { getBatchCurrencyRatesSchema, getCurrencyRateSchema, } from '~/types/expense.types'; -import { createExpense, deleteExpense, editExpense } from '../services/splitService'; +import { + assertExpenseGroupReassignment, + createExpense, + deleteExpense, + editExpense, +} from '../services/splitService'; import { currencyRateProvider } from '../services/currencyRateService'; import { isCurrencyCode } from '~/lib/currency'; import { SplitType } from '@prisma/client'; @@ -139,11 +144,24 @@ export const expenseRouter = createTRPCRouter({ // Expense writes must preserve input order and stop on the first validation/write failure. // oxlint-disable no-await-in-loop for (const input of expenses) { - await validateExpenseWritePermission(input.expenseId, input.groupId, ctx.session.user.id); + const storedGroupId = await validateExpenseWritePermission( + input.expenseId, + input.groupId, + ctx.session.user.id, + ); if (input.splitType === SplitType.CURRENCY_CONVERSION) { throw new TRPCError({ code: 'BAD_REQUEST', message: 'Invalid split type' }); } + if (input.expenseId) { + await assertExpenseGroupReassignment( + storedGroupId, + input.groupId, + input.paidBy, + input.participants.map((participant) => participant.userId), + ); + } + try { const expense = input.expenseId ? await editExpense(input, ctx.session.user.id) @@ -151,6 +169,9 @@ export const expenseRouter = createTRPCRouter({ results.push(expense); } catch (error) { + if (error instanceof TRPCError) { + throw error; + } console.error(error); throw new TRPCError({ code: 'INTERNAL_SERVER_ERROR', @@ -167,7 +188,11 @@ export const expenseRouter = createTRPCRouter({ .input(createCurrencyConversionSchema) .mutation(async ({ input, ctx }) => { const { amount, rate, from, to, senderId, receiverId, groupId, expenseId } = input; - await validateExpenseWritePermission(expenseId, groupId, ctx.session.user.id); + const storedGroupId = await validateExpenseWritePermission( + expenseId, + groupId, + ctx.session.user.id, + ); if (!isCurrencyCode(from) || !isCurrencyCode(to)) { throw new TRPCError({ code: 'BAD_REQUEST', message: 'Invalid currency code' }); @@ -209,6 +234,12 @@ export const expenseRouter = createTRPCRouter({ if (!expenseId) { res = await createExpense(conversionTo, ctx.session.user.id, conversionFrom); } else { + await assertExpenseGroupReassignment( + storedGroupId, + conversionFrom.groupId, + conversionFrom.paidBy, + conversionFrom.participants.map((participant) => participant.userId), + ); res = await editExpense(conversionFrom, ctx.session.user.id, conversionTo); } @@ -648,26 +679,33 @@ const validateEditExpensePermission = async (expenseId: string, userId: number): } }; +/** + * Checks that the user may write the expense into `groupId` and returns the expense's stored + * group (null for a new or ungrouped expense). + */ const validateExpenseWritePermission = async ( expenseId: string | undefined, groupId: number | null, userId: number, -): Promise => { +): Promise => { + let storedGroupId: number | null = null; if (expenseId) { await validateEditExpensePermission(expenseId, userId); const expense = await db.expense.findUniqueOrThrow({ where: { id: expenseId }, select: { groupId: true }, }); - if (expense.groupId !== groupId) { + // An ungrouped expense may be assigned to a group; a grouped expense keeps its group. + if (null !== expense.groupId && expense.groupId !== groupId) { throw new TRPCError({ code: 'BAD_REQUEST', message: 'Cannot move an expense to another group', }); } + storedGroupId = expense.groupId; } if (null === groupId) { - return; + return storedGroupId; } const group = await db.group.findUnique({ where: { id: groupId }, @@ -682,6 +720,7 @@ const validateExpenseWritePermission = async ( if (group.archivedAt) { throw new TRPCError({ code: 'BAD_REQUEST', message: 'Group is archived' }); } + return storedGroupId; }; export type ExpenseRouter = typeof expenseRouter; diff --git a/src/server/api/services/splitService.ts b/src/server/api/services/splitService.ts index dbd53d7b..c41ac06c 100644 --- a/src/server/api/services/splitService.ts +++ b/src/server/api/services/splitService.ts @@ -1,4 +1,5 @@ -import { SplitType, type User } from '@prisma/client'; +import { Prisma, SplitType, type User } from '@prisma/client'; +import { TRPCError } from '@trpc/server'; import { nanoid } from 'nanoid'; import { db } from '~/server/db'; @@ -192,10 +193,49 @@ export async function deleteExpense(expenseId: string, deletedBy: number) { sendExpensePushNotification(expenseId).catch(console.error); } +/** + * Checks the payer and participants when an ungrouped expense is assigned to a group. Callers run + * validateExpenseWritePermission first, which already rejects changing a grouped expense's group + * and checks that the target group exists, is not archived and has the actor as a member. + */ +export async function assertExpenseGroupReassignment( + storedGroupId: number | null, + groupId: number | null, + paidBy: number, + participantUserIds: number[], +) { + if (null !== storedGroupId || null === groupId) { + return; + } + + const members = await db.groupUser.findMany({ + where: { + groupId, + userId: { + in: [paidBy, ...participantUserIds], + }, + }, + select: { userId: true }, + }); + const memberIds = new Set(members.map((member) => member.userId)); + + if (!memberIds.has(paidBy)) { + throw new TRPCError({ code: 'BAD_REQUEST', message: 'Payer is not a member of the group' }); + } + + if (participantUserIds.some((userId) => !memberIds.has(userId))) { + throw new TRPCError({ + code: 'BAD_REQUEST', + message: 'Participant is not a member of the group', + }); + } +} + export async function editExpense( { expenseId, paidBy, + groupId, name, category, amount, @@ -230,6 +270,13 @@ export async function editExpense( throw new Error('Expense not found'); } + if (expense.conversionToId && !conversionToParams) { + throw new TRPCError({ + code: 'BAD_REQUEST', + message: 'Currency conversions must be edited together', + }); + } + // Determine if this is a template or derived expense const templateId = expense.recurrence?.job?.command ? extractTemplateExpenseId(expense.recurrence.job.command) @@ -250,9 +297,11 @@ export async function editExpense( // Update expense with new details and create new participants operations.push( db.expense.update({ - where: { id: expenseId }, + // Write-time check: a concurrent edit may have grouped the expense after validation. + where: { id: expenseId, OR: [{ groupId: null }, { groupId }] }, data: { paidBy, + groupId, name, category, amount, @@ -276,7 +325,11 @@ export async function editExpense( operations.push( db.expense.update({ - where: { id: expense.conversionToId }, + // The target can be grouped on its own, so it gets the same write-time check. + where: { + id: expense.conversionToId, + OR: [{ groupId: null }, { groupId: toExpenseData.groupId }], + }, data: { ...toExpenseData, expenseParticipants: { @@ -310,7 +363,17 @@ export async function editExpense( } // For derived expenses, cronExpression is ignored entirely - await db.$transaction(operations); + try { + await db.$transaction(operations); + } catch (error) { + if (error instanceof Prisma.PrismaClientKnownRequestError && 'P2025' === error.code) { + throw new TRPCError({ + code: 'CONFLICT', + message: 'This expense was changed by someone else. Reload it and try again.', + }); + } + throw error; + } sendExpensePushNotification(expenseId).catch(console.error); return { id: expenseId }; // Return the updated expense } diff --git a/src/tests/integration/expenseGroupReassignment.integration.test.ts b/src/tests/integration/expenseGroupReassignment.integration.test.ts new file mode 100644 index 00000000..fa204446 --- /dev/null +++ b/src/tests/integration/expenseGroupReassignment.integration.test.ts @@ -0,0 +1,282 @@ +import { editExpense } from '~/server/api/services/splitService'; + +import { db } from './database'; +import { accountingSnapshot, expenseInput, testGroup, testScenario, testUser } from './factories'; +import { callerFor } from './trpc'; + +describe('assigning an ungrouped expense to a group', () => { + it('persists the group when an ungrouped expense is edited into a group', async () => { + const { owner, member, group } = await testScenario(); + const caller = callerFor(owner.id); + const input = expenseInput(owner.id, member.id); + const [expense] = await caller.expense.addOrEditExpense(input); + + await caller.expense.addOrEditExpense({ ...input, expenseId: expense!.id, groupId: group.id }); + + expect(await db.expense.findUnique({ where: { id: expense!.id } })).toMatchObject({ + groupId: group.id, + }); + expect(await caller.expense.getGroupExpenses({ groupId: group.id })).toEqual([ + expect.objectContaining({ id: expense!.id }), + ]); + expect( + await db.balanceView.findMany({ where: { groupId: group.id }, orderBy: { userId: 'asc' } }), + ).toEqual([ + expect.objectContaining({ userId: owner.id, friendId: member.id, amount: 1000n }), + expect.objectContaining({ userId: member.id, friendId: owner.id, amount: -1000n }), + ]); + }); + + it('assigns both linked records of an ungrouped currency conversion', async () => { + const { owner, member, group } = await testScenario(); + const conversion = { + amount: 1000n, + rate: 0.9, + from: 'USD', + to: 'EUR', + senderId: owner.id, + receiverId: member.id, + groupId: null, + }; + await callerFor(owner.id).expense.addOrEditCurrencyConversion(conversion); + const expense = await db.expense.findFirstOrThrow({ where: { conversionToId: { not: null } } }); + + await callerFor(owner.id).expense.addOrEditCurrencyConversion({ + ...conversion, + expenseId: expense.id, + groupId: group.id, + }); + + expect( + (await db.expense.findMany({ select: { groupId: true } })).map(({ groupId }) => groupId), + ).toEqual([group.id, group.id]); + expect(await callerFor(owner.id).expense.getGroupExpenses({ groupId: group.id })).toEqual([ + expect.objectContaining({ + id: expense.id, + groupId: group.id, + conversionTo: expect.objectContaining({ groupId: group.id }), + }), + ]); + expect( + await db.balanceView.findMany({ + where: { groupId: group.id }, + orderBy: [{ currency: 'asc' }, { userId: 'asc' }], + }), + ).toEqual([ + expect.objectContaining({ + currency: 'EUR', + userId: owner.id, + friendId: member.id, + amount: -900n, + }), + expect.objectContaining({ + currency: 'EUR', + userId: member.id, + friendId: owner.id, + amount: 900n, + }), + expect.objectContaining({ + currency: 'USD', + userId: owner.id, + friendId: member.id, + amount: 1000n, + }), + expect.objectContaining({ + currency: 'USD', + userId: member.id, + friendId: owner.id, + amount: -1000n, + }), + ]); + }); + + it('rejects a conversion whose target a concurrent edit assigned to another group', async () => { + const { owner, member, group } = await testScenario(); + const otherGroup = await testGroup(owner.id, [member.id]); + const conversion = { + amount: 1000n, + rate: 0.9, + from: 'USD', + to: 'EUR', + senderId: owner.id, + receiverId: member.id, + groupId: null, + }; + await callerFor(owner.id).expense.addOrEditCurrencyConversion(conversion); + const source = await db.expense.findFirstOrThrow({ where: { conversionToId: { not: null } } }); + await db.expense.update({ + where: { id: source.conversionToId! }, + data: { groupId: otherGroup.id }, + }); + const before = await accountingSnapshot(); + + await expect( + callerFor(owner.id).expense.addOrEditCurrencyConversion({ + ...conversion, + expenseId: source.id, + groupId: group.id, + }), + ).rejects.toMatchObject({ code: 'CONFLICT' }); + expect(await accountingSnapshot()).toEqual(before); + }); + + it('rejects an ordinary edit of a currency conversion', async () => { + const { owner, member, group } = await testScenario(); + await callerFor(owner.id).expense.addOrEditCurrencyConversion({ + amount: 1000n, + rate: 0.9, + from: 'USD', + to: 'EUR', + senderId: owner.id, + receiverId: member.id, + groupId: null, + }); + const source = await db.expense.findFirstOrThrow({ where: { conversionToId: { not: null } } }); + const before = await accountingSnapshot(); + + await expect( + callerFor(owner.id).expense.addOrEditExpense({ + ...expenseInput(owner.id, member.id), + expenseId: source.id, + groupId: group.id, + }), + ).rejects.toMatchObject({ + code: 'BAD_REQUEST', + message: 'Currency conversions must be edited together', + }); + expect(await accountingSnapshot()).toEqual(before); + }); + + it('rejects a caller who neither added nor participates in the expense', async () => { + const { owner, member, outsider } = await testScenario(); + const outsiderGroup = await testGroup(outsider.id, [owner.id, member.id]); + const input = expenseInput(owner.id, member.id); + const [expense] = await callerFor(owner.id).expense.addOrEditExpense(input); + const before = await accountingSnapshot(); + + await expect( + callerFor(outsider.id).expense.addOrEditExpense({ + ...input, + expenseId: expense!.id, + groupId: outsiderGroup.id, + }), + ).rejects.toMatchObject({ code: 'UNAUTHORIZED' }); + expect(await accountingSnapshot()).toEqual(before); + }); + + it.each([ + { + label: 'the caller is not a member', + payer: 'member', + groupMembers: ['other', 'member'], + code: 'FORBIDDEN', + message: 'Not a group member', + }, + { + label: 'the payer is not a member', + payer: 'member', + groupMembers: ['owner'], + code: 'BAD_REQUEST', + message: 'Payer is not a member of the group', + }, + { + label: 'a participant is not a member', + payer: 'owner', + groupMembers: ['owner'], + code: 'BAD_REQUEST', + message: 'Participant is not a member of the group', + }, + ] as const)( + 'rejects the assignment when $label', + async ({ payer, groupMembers, code, message }) => { + const users = { + owner: await testUser('Owner'), + member: await testUser('Member'), + other: await testUser('Other'), + }; + const otherParticipant = 'owner' === payer ? users.member : users.owner; + const input = expenseInput(users[payer].id, otherParticipant.id); + const [expense] = await callerFor(users.owner.id).expense.addOrEditExpense(input); + const [groupOwner, ...rest] = groupMembers.map((name) => users[name].id); + const group = await testGroup(groupOwner!, rest); + const before = await accountingSnapshot(); + + await expect( + callerFor(users.owner.id).expense.addOrEditExpense({ + ...input, + expenseId: expense!.id, + groupId: group.id, + }), + ).rejects.toMatchObject({ code, message }); + expect(await accountingSnapshot()).toEqual(before); + }, + ); + + it('rejects an edit that a concurrent group assignment got to first', async () => { + const { owner, member, group } = await testScenario(); + const otherGroup = await testGroup(owner.id, [member.id]); + const caller = callerFor(owner.id); + const input = expenseInput(owner.id, member.id); + const [expense] = await caller.expense.addOrEditExpense(input); + await caller.expense.addOrEditExpense({ ...input, expenseId: expense!.id, groupId: group.id }); + const before = await accountingSnapshot(); + + // Calling editExpense directly stands in for an edit that passed validation + // While the expense was still ungrouped. + await expect( + editExpense({ ...input, expenseId: expense!.id, groupId: otherGroup.id }, owner.id), + ).rejects.toMatchObject({ code: 'CONFLICT' }); + + expect(await db.expense.findUnique({ where: { id: expense!.id } })).toMatchObject({ + groupId: group.id, + }); + expect(await accountingSnapshot()).toEqual(before); + }); + + it('returns the conflict to a caller whose edit lost the group assignment', async () => { + const { owner, member, group } = await testScenario(); + const otherGroup = await testGroup(owner.id, [member.id]); + const caller = callerFor(owner.id); + const input = expenseInput(owner.id, member.id); + const [expense] = await caller.expense.addOrEditExpense(input); + const readStoredGroup = db.expense.findUniqueOrThrow.bind(db.expense); + const validationRead = jest.spyOn(db.expense, 'findUniqueOrThrow'); + let afterWinningEdit: Awaited> | null = null; + // The first edit lands just after the second one read the expense as ungrouped. + validationRead.mockImplementationOnce((async (args: Parameters[0]) => { + const stored = await readStoredGroup(args); + await db.expense.update({ where: { id: expense!.id }, data: { groupId: group.id } }); + afterWinningEdit = await accountingSnapshot(); + return stored; + }) as unknown as typeof db.expense.findUniqueOrThrow); + + await expect( + caller.expense.addOrEditExpense({ + ...input, + expenseId: expense!.id, + groupId: otherGroup.id, + }), + ).rejects.toMatchObject({ code: 'CONFLICT' }); + validationRead.mockRestore(); + + expect(await db.expense.findUnique({ where: { id: expense!.id } })).toMatchObject({ + groupId: group.id, + }); + expect(afterWinningEdit).not.toBeNull(); + expect(await accountingSnapshot()).toEqual(afterWinningEdit); + }); + + it('rejects assigning an expense to an archived group', async () => { + const { owner, member, group } = await testScenario(); + const caller = callerFor(owner.id); + const input = expenseInput(owner.id, member.id); + const [expense] = await caller.expense.addOrEditExpense(input); + await db.group.update({ where: { id: group.id }, data: { archivedAt: new Date() } }); + const before = await accountingSnapshot(); + + await expect( + caller.expense.addOrEditExpense({ ...input, expenseId: expense!.id, groupId: group.id }), + ).rejects.toMatchObject({ code: 'BAD_REQUEST', message: 'Group is archived' }); + expect(await accountingSnapshot()).toEqual(before); + }); +});