From 299e2544cf970fc7e64d713debdafa327864f72c Mon Sep 17 00:00:00 2001 From: Harshil Jani Date: Fri, 9 Oct 2026 17:11:47 +0530 Subject: [PATCH 1/2] Keep expenses when removing a participant from a group Removing a participant deleted every expense they paid and their share of every other expense, through ON DELETE CASCADE. The settings page hides the remove button for participants in expenses, but only for the expenses it knew of when it loaded, and the server never checked. The two foreign keys are now NO ACTION, DEFERRABLE INITIALLY DEFERRED, so the database refuses the removal and deleting a whole group still cascades. The refusal is a CONFLICT the settings page explains, reloading the group. --- e2e/group-management.spec.ts | 41 +++++++++++++++++++ messages/en-US.json | 1 + .../migration.sql | 21 ++++++++++ prisma/schema.prisma | 8 +++- src/app/groups/[groupId]/edit/edit-group.tsx | 38 ++++++++++++++++- src/lib/api.ts | 9 ++-- src/trpc/routers/groups/update.procedure.ts | 20 ++++++++- 7 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 prisma/migrations/20261018000000_keep_expenses_when_removing_participant/migration.sql diff --git a/e2e/group-management.spec.ts b/e2e/group-management.spec.ts index 49f297e01..0256202f1 100644 --- a/e2e/group-management.spec.ts +++ b/e2e/group-management.spec.ts @@ -2,6 +2,7 @@ import { addExpense, createGroup, expectBalance, + expenseCard, openTab, paidForRow, uniqueSuffix, @@ -56,6 +57,46 @@ test('renames a group and adds a participant afterwards', async ({ page }) => { await expectBalance(page, 'Dave', 60) }) +test('refuses to remove a participant who got an expense while the settings were open', async ({ + page, +}) => { + const groupId = await createGroup(page, { + name: `E2E Remove race ${uniqueSuffix()}`, + participants: ['Alice', 'Bob', 'Carol'], + }) + + // Bob has no expense yet, so the settings page offers to remove him. + await openTab(page, 'Settings') + const bobRow = page + .locator('li') + .filter({ has: page.locator('input[name="participants.1.name"]') }) + await expect(bobRow.getByRole('button')).toBeEnabled() + + // Meanwhile, someone else adds an expense paid by Bob. + const otherTab = await page.context().newPage() + await addExpense(otherTab, groupId, { + title: 'Hotel', + amount: '90', + paidBy: 'Bob', + }) + await otherTab.close() + + // The stale settings page still lets Bob be removed and saved. + await bobRow.getByRole('button').click() + await page.getByRole('button', { name: 'Save', exact: true }).click() + await expect( + page + .getByRole('region', { name: /Notifications/ }) + .getByText('Bob is part of expenses, and can not be removed.'), + ).toBeVisible({ timeout: 30_000 }) + + // Bob and his expense are still there, and the balances still count it. + await page.goto(`/groups/${groupId}/expenses`) + await expect(expenseCard(page, 'Hotel')).toBeVisible() + await page.goto(`/groups/${groupId}/balances`) + await expectBalance(page, 'Bob', 60) +}) + test('lists visited groups under Recent groups', async ({ page }) => { const first = `E2E Recent A ${uniqueSuffix()}` const second = `E2E Recent B ${uniqueSuffix()}` diff --git a/messages/en-US.json b/messages/en-US.json index 77bfb4aee..b0d73fda2 100644 --- a/messages/en-US.json +++ b/messages/en-US.json @@ -136,6 +136,7 @@ "title": "Participants", "description": "Enter the name for each participant.", "protectedParticipant": "This participant is part of expenses, and can not be removed.", + "removedParticipantHasExpenses": "{names} {count, plural, one {is} other {are}} part of expenses, and can not be removed.", "new": "New", "add": "Add participant", "John": "John", diff --git a/prisma/migrations/20261018000000_keep_expenses_when_removing_participant/migration.sql b/prisma/migrations/20261018000000_keep_expenses_when_removing_participant/migration.sql new file mode 100644 index 000000000..7acfa361f --- /dev/null +++ b/prisma/migrations/20261018000000_keep_expenses_when_removing_participant/migration.sql @@ -0,0 +1,21 @@ +-- A participant who is part of an expense can no longer be deleted: removing +-- them used to delete every expense they paid and their share of every other +-- one, silently changing everybody's balances. +-- +-- DEFERRABLE INITIALLY DEFERRED (which Prisma cannot express, and ignores when +-- diffing) checks at commit rather than per statement. Deleting a whole group +-- cascades to its participants and its expenses as separate statements; a +-- per-statement check would refuse that as soon as the participants went +-- first. + +-- DropForeignKey +ALTER TABLE "Expense" DROP CONSTRAINT "Expense_paidById_fkey"; + +-- DropForeignKey +ALTER TABLE "ExpensePaidFor" DROP CONSTRAINT "ExpensePaidFor_participantId_fkey"; + +-- AddForeignKey +ALTER TABLE "Expense" ADD CONSTRAINT "Expense_paidById_fkey" FOREIGN KEY ("paidById") REFERENCES "Participant"("id") ON DELETE NO ACTION ON UPDATE CASCADE DEFERRABLE INITIALLY DEFERRED; + +-- AddForeignKey +ALTER TABLE "ExpensePaidFor" ADD CONSTRAINT "ExpensePaidFor_participantId_fkey" FOREIGN KEY ("participantId") REFERENCES "Participant"("id") ON DELETE NO ACTION ON UPDATE CASCADE DEFERRABLE INITIALLY DEFERRED; diff --git a/prisma/schema.prisma b/prisma/schema.prisma index d56fb26f9..3d0099a6c 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -53,7 +53,9 @@ model Expense { originalAmount Int? originalCurrency String? conversionRate Decimal? - paidBy Participant @relation(fields: [paidById], references: [id], onDelete: Cascade) + // No cascade: a participant in an expense can't be deleted. Checked at commit + // (DEFERRABLE, set in migration 20261018000000) so deleting a group still works. + paidBy Participant @relation(fields: [paidById], references: [id], onDelete: NoAction) paidById String paidFor ExpensePaidFor[] groupId String @@ -112,7 +114,9 @@ enum RecurrenceRule { model ExpensePaidFor { expense Expense @relation(fields: [expenseId], references: [id], onDelete: Cascade) - participant Participant @relation(fields: [participantId], references: [id], onDelete: Cascade) + // No cascade: a participant in an expense can't be deleted. Checked at commit + // (DEFERRABLE, set in migration 20261018000000) so deleting a group still works. + participant Participant @relation(fields: [participantId], references: [id], onDelete: NoAction) expenseId String participantId String shares Int @default(1) diff --git a/src/app/groups/[groupId]/edit/edit-group.tsx b/src/app/groups/[groupId]/edit/edit-group.tsx index 8d0b095cc..35dfac401 100644 --- a/src/app/groups/[groupId]/edit/edit-group.tsx +++ b/src/app/groups/[groupId]/edit/edit-group.tsx @@ -1,8 +1,12 @@ 'use client' import { GroupForm } from '@/components/group-form' +import { useToast } from '@/components/ui/use-toast' import { RuntimeFeatureFlags } from '@/lib/featureFlags' import { trpc } from '@/trpc/client' +import { isTRPCClientError } from '@trpc/client' +import { useTranslations } from 'next-intl' +import { useState } from 'react' import { useCurrentGroup } from '../current-group-context' export const EditGroup = ({ @@ -14,14 +18,46 @@ export const EditGroup = ({ const { data, isLoading } = trpc.groups.getDetails.useQuery({ groupId }) const { mutateAsync } = trpc.groups.update.useMutation() const utils = trpc.useUtils() + const { toast } = useToast() + const t = useTranslations('GroupForm.Participants') + // Bumped to remount the form on the refreshed group after a refused save. + const [formKey, setFormKey] = useState(0) if (isLoading) return <> return ( { - await mutateAsync({ groupId, participantId, groupFormValues }) + try { + await mutateAsync({ groupId, participantId, groupFormValues }) + } catch (error) { + if (!isTRPCClientError(error) || error.data?.code !== 'CONFLICT') + throw error + // Someone added an expense for a participant removed here since the + // page loaded. Reload the group so they come back, now protected. + const fresh = await utils.groups.getDetails.fetch( + { groupId }, + { staleTime: 0 }, + ) + const names = (data?.group?.participants ?? []) + .filter( + (p) => + !groupFormValues.participants.some(({ id }) => id === p.id) && + fresh.participantsWithExpenses.includes(p.id), + ) + .map((p) => p.name) + toast({ + description: t('removedParticipantHasExpenses', { + names: names.join(', '), + count: names.length, + }), + variant: 'destructive', + }) + setFormKey((key) => key + 1) + return + } await utils.groups.invalidate() }} protectedParticipantIds={data?.participantsWithExpenses} diff --git a/src/lib/api.ts b/src/lib/api.ts index 2cb490208..908287f21 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -363,9 +363,7 @@ export async function updateGroup( const existingGroup = await getGroup(groupId) if (!existingGroup) throw new Error('Invalid group ID') - await logActivity(groupId, ActivityType.UPDATE_GROUP, { participantId }) - - return prisma.group.update({ + const group = await prisma.group.update({ where: { id: groupId }, data: { name: groupFormValues.name, @@ -395,6 +393,11 @@ export async function updateGroup( }, }, }) + + // Logged after the update, which can be refused (a removed participant who + // is part of expenses), so a refused save leaves no activity behind. + await logActivity(groupId, ActivityType.UPDATE_GROUP, { participantId }) + return group } export async function getGroup(groupId: string) { diff --git a/src/trpc/routers/groups/update.procedure.ts b/src/trpc/routers/groups/update.procedure.ts index a9e36668a..4a8dabf3a 100644 --- a/src/trpc/routers/groups/update.procedure.ts +++ b/src/trpc/routers/groups/update.procedure.ts @@ -1,6 +1,8 @@ +import { Prisma } from '@/generated/prisma/client' import { updateGroup } from '@/lib/api' import { groupFormSchema } from '@/lib/schemas' import { baseProcedure } from '@/trpc/init' +import { TRPCError } from '@trpc/server' import { z } from 'zod' export const updateGroupProcedure = baseProcedure @@ -12,5 +14,21 @@ export const updateGroupProcedure = baseProcedure }), ) .mutation(async ({ input: { groupId, groupFormValues, participantId } }) => { - await updateGroup(groupId, groupFormValues, participantId) + try { + await updateGroup(groupId, groupFormValues, participantId) + } catch (error) { + // The database refuses to delete a participant who is still part of an + // expense. The form hides the remove button for them, but only for the + // expenses it knew of when it loaded. + if ( + error instanceof Prisma.PrismaClientKnownRequestError && + error.code === 'P2003' + ) { + throw new TRPCError({ + code: 'CONFLICT', + message: 'A removed participant is part of expenses.', + }) + } + throw error + } }) From ca47ca65c02d23e6b12860b9e4313ba733ecf1ed Mon Sep 17 00:00:00 2001 From: Harshil Jani Date: Mon, 12 Oct 2026 13:29:50 +0530 Subject: [PATCH 2/2] Check for expenses on the server before removing participants updateGroup now looks up the removed participants' expenses inside a transaction and refuses with their names, so the client no longer re-derives them. The update and its activity log are atomic. The foreign key refusal stays as a backstop, matched by constraint name only, with a generic message when no names are known. A jest test keeps the two constraints DEFERRABLE in later migrations. Co-Authored-By: Claude --- messages/en-US.json | 1 + prisma/migrations.test.ts | 28 ++++++ src/app/groups/[groupId]/edit/edit-group.tsx | 24 ++--- src/lib/api.ts | 97 +++++++++++++------- src/trpc/init.ts | 12 +++ src/trpc/routers/groups/update.procedure.ts | 25 ++++- 6 files changed, 134 insertions(+), 53 deletions(-) create mode 100644 prisma/migrations.test.ts diff --git a/messages/en-US.json b/messages/en-US.json index b0d73fda2..5f06a24af 100644 --- a/messages/en-US.json +++ b/messages/en-US.json @@ -137,6 +137,7 @@ "description": "Enter the name for each participant.", "protectedParticipant": "This participant is part of expenses, and can not be removed.", "removedParticipantHasExpenses": "{names} {count, plural, one {is} other {are}} part of expenses, and can not be removed.", + "removedParticipantsHaveExpenses": "Some removed participants are part of expenses, and can not be removed.", "new": "New", "add": "Add participant", "John": "John", diff --git a/prisma/migrations.test.ts b/prisma/migrations.test.ts new file mode 100644 index 000000000..fe5eaad9c --- /dev/null +++ b/prisma/migrations.test.ts @@ -0,0 +1,28 @@ +import { readdirSync, readFileSync } from 'fs' +import { join } from 'path' + +/** + * Prisma can't express DEFERRABLE and ignores it when diffing, so a future + * migration that touches these relations would recreate the foreign keys + * without it, and deleting a group (which removes its participants before its + * expenses) would start failing. The latest migration defining each one must + * keep it. + */ +const dir = join(__dirname, 'migrations') +const migrations = readdirSync(dir, { withFileTypes: true }) + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) + .sort() + .map((name) => readFileSync(join(dir, name, 'migration.sql'), 'utf8')) + +test.each(['Expense_paidById_fkey', 'ExpensePaidFor_participantId_fkey'])( + '%s stays DEFERRABLE INITIALLY DEFERRED', + (constraint) => { + const definitions = migrations + .flatMap((sql) => sql.split(';')) + .filter((statement) => + statement.includes(`ADD CONSTRAINT "${constraint}"`), + ) + expect(definitions.at(-1)).toContain('DEFERRABLE INITIALLY DEFERRED') + }, +) diff --git a/src/app/groups/[groupId]/edit/edit-group.tsx b/src/app/groups/[groupId]/edit/edit-group.tsx index 35dfac401..6895eebbf 100644 --- a/src/app/groups/[groupId]/edit/edit-group.tsx +++ b/src/app/groups/[groupId]/edit/edit-group.tsx @@ -37,22 +37,16 @@ export const EditGroup = ({ throw error // Someone added an expense for a participant removed here since the // page loaded. Reload the group so they come back, now protected. - const fresh = await utils.groups.getDetails.fetch( - { groupId }, - { staleTime: 0 }, - ) - const names = (data?.group?.participants ?? []) - .filter( - (p) => - !groupFormValues.participants.some(({ id }) => id === p.id) && - fresh.participantsWithExpenses.includes(p.id), - ) - .map((p) => p.name) + await utils.groups.getDetails.invalidate({ groupId }) + const names = error.data.participantsWithExpenses ?? [] toast({ - description: t('removedParticipantHasExpenses', { - names: names.join(', '), - count: names.length, - }), + description: + names.length > 0 + ? t('removedParticipantHasExpenses', { + names: names.join(', '), + count: names.length, + }) + : t('removedParticipantsHaveExpenses'), variant: 'destructive', }) setFormKey((key) => key + 1) diff --git a/src/lib/api.ts b/src/lib/api.ts index 908287f21..962577afb 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -1,6 +1,7 @@ import { ActivityType, Expense, + Prisma, RecurrenceRule, RecurringExpenseLink, } from '@/generated/prisma/client' @@ -355,49 +356,78 @@ export async function updateExpense( }) } +/** A group update tried to remove participants who are part of expenses. */ +export class ParticipantsHaveExpensesError extends Error { + constructor(public names: string[]) { + super(`${names.join(', ')} can not be removed: part of expenses.`) + } +} + export async function updateGroup( groupId: string, groupFormValues: GroupFormValues, participantId?: string, ) { - const existingGroup = await getGroup(groupId) - if (!existingGroup) throw new Error('Invalid group ID') + // One transaction, so a refused save changes nothing and logs nothing. + return prisma.$transaction(async (tx) => { + const existingGroup = await tx.group.findUnique({ + where: { id: groupId }, + include: { participants: true }, + }) + if (!existingGroup) throw new Error('Invalid group ID') - const group = await prisma.group.update({ - where: { id: groupId }, - data: { - name: groupFormValues.name, - information: groupFormValues.information, - currency: groupFormValues.currency, - currencyCode: groupFormValues.currencyCode, - participants: { - deleteMany: existingGroup.participants.filter( - (p) => !groupFormValues.participants.some((p2) => p2.id === p.id), - ), - updateMany: groupFormValues.participants - .filter((participant) => participant.id !== undefined) - .map((participant) => ({ - where: { id: participant.id }, - data: { - name: participant.name, - }, - })), - createMany: { - data: groupFormValues.participants - .filter((participant) => participant.id === undefined) + const removed = existingGroup.participants.filter( + (p) => !groupFormValues.participants.some((p2) => p2.id === p.id), + ) + // The form hides the remove button for participants who are part of + // expenses, but only for the expenses it knew of when it loaded. The + // foreign keys still refuse an expense added between this check and the + // commit. + const withExpenses = await tx.participant.findMany({ + where: { + id: { in: removed.map((p) => p.id) }, + OR: [ + { expensesPaidBy: { some: {} } }, + { expensesPaidFor: { some: {} } }, + ], + }, + select: { name: true }, + }) + if (withExpenses.length > 0) + throw new ParticipantsHaveExpensesError(withExpenses.map((p) => p.name)) + + const group = await tx.group.update({ + where: { id: groupId }, + data: { + name: groupFormValues.name, + information: groupFormValues.information, + currency: groupFormValues.currency, + currencyCode: groupFormValues.currencyCode, + participants: { + deleteMany: removed.map(({ id }) => ({ id })), + updateMany: groupFormValues.participants + .filter((participant) => participant.id !== undefined) .map((participant) => ({ - id: randomId(), - name: participant.name, + where: { id: participant.id }, + data: { + name: participant.name, + }, })), + createMany: { + data: groupFormValues.participants + .filter((participant) => participant.id === undefined) + .map((participant) => ({ + id: randomId(), + name: participant.name, + })), + }, }, }, - }, - }) + }) - // Logged after the update, which can be refused (a removed participant who - // is part of expenses), so a refused save leaves no activity behind. - await logActivity(groupId, ActivityType.UPDATE_GROUP, { participantId }) - return group + await logActivity(groupId, ActivityType.UPDATE_GROUP, { participantId }, tx) + return group + }) } export async function getGroup(groupId: string) { @@ -537,8 +567,9 @@ export async function logActivity( groupId: string, activityType: ActivityType, extra?: { participantId?: string; expenseId?: string; data?: string }, + client: Prisma.TransactionClient = prisma, ) { - return prisma.activity.create({ + return client.activity.create({ data: { id: randomId(), groupId, diff --git a/src/trpc/init.ts b/src/trpc/init.ts index 4fdb0714f..f26c28d92 100644 --- a/src/trpc/init.ts +++ b/src/trpc/init.ts @@ -1,4 +1,5 @@ import { Prisma } from '@/generated/prisma/client' +import { ParticipantsHaveExpensesError } from '@/lib/api' import { initTRPC } from '@trpc/server' import { cache } from 'react' import superjson from 'superjson' @@ -28,6 +29,17 @@ const t = initTRPC.create({ * @see https://trpc.io/docs/server/data-transformers */ transformer: superjson, + errorFormatter: ({ shape, error }) => ({ + ...shape, + data: { + ...shape.data, + // Who a refused group update tried to remove, for the error message. + participantsWithExpenses: + error.cause instanceof ParticipantsHaveExpensesError + ? error.cause.names + : undefined, + }, + }), }) // Base router and procedure helpers diff --git a/src/trpc/routers/groups/update.procedure.ts b/src/trpc/routers/groups/update.procedure.ts index 4a8dabf3a..123f89ad1 100644 --- a/src/trpc/routers/groups/update.procedure.ts +++ b/src/trpc/routers/groups/update.procedure.ts @@ -1,10 +1,16 @@ import { Prisma } from '@/generated/prisma/client' -import { updateGroup } from '@/lib/api' +import { ParticipantsHaveExpensesError, updateGroup } from '@/lib/api' import { groupFormSchema } from '@/lib/schemas' import { baseProcedure } from '@/trpc/init' import { TRPCError } from '@trpc/server' import { z } from 'zod' +// From the migration that stopped participant removal from deleting expenses. +const PARTICIPANT_EXPENSE_CONSTRAINTS = [ + 'Expense_paidById_fkey', + 'ExpensePaidFor_participantId_fkey', +] + export const updateGroupProcedure = baseProcedure .input( z.object({ @@ -17,12 +23,21 @@ export const updateGroupProcedure = baseProcedure try { await updateGroup(groupId, groupFormValues, participantId) } catch (error) { - // The database refuses to delete a participant who is still part of an - // expense. The form hides the remove button for them, but only for the - // expenses it knew of when it loaded. + if (error instanceof ParticipantsHaveExpensesError) { + throw new TRPCError({ + code: 'CONFLICT', + message: error.message, + cause: error, + }) + } + // An expense added between updateGroup's check and its commit: the + // foreign keys refuse it, but we don't know the names. if ( error instanceof Prisma.PrismaClientKnownRequestError && - error.code === 'P2003' + error.code === 'P2003' && + PARTICIPANT_EXPENSE_CONSTRAINTS.includes( + (error.meta as any)?.driverAdapterError?.cause?.constraint?.index, + ) ) { throw new TRPCError({ code: 'CONFLICT',