Keep expenses when removing a participant from a group

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()}`

messages/en-US.json

@@ -136,6 +136,8 @@
       "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.",
+      "removedParticipantsHaveExpenses": "Some removed participants are part of expenses, and can not be removed.",
       "new": "New",
       "add": "Add participant",
       "John": "John",

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')
+  },
+)

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;

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)

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,40 @@ 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 (
     <GroupForm
+      key={formKey}
       group={data?.group}
       onSubmit={async (groupFormValues, participantId) => {
-        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.
+          await utils.groups.getDetails.invalidate({ groupId })
+          const names = error.data.participantsWithExpenses ?? []
+          toast({
+            description:
+              names.length > 0
+                ? t('removedParticipantHasExpenses', {
+                    names: names.join(', '),
+                    count: names.length,
+                  })
+                : t('removedParticipantsHaveExpenses'),
+            variant: 'destructive',
+          })
+          setFormKey((key) => key + 1)
+          return
+        }
         await utils.groups.invalidate()
       }}
       protectedParticipantIds={data?.participantsWithExpenses}

src/lib/api.ts

@@ -1,6 +1,7 @@
 import {
   ActivityType,
   Expense,
+  Prisma,
   RecurrenceRule,
   RecurringExpenseLink,
 } from '@/generated/prisma/client'
@@ -355,45 +356,77 @@ 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')
-
-  await logActivity(groupId, ActivityType.UPDATE_GROUP, { participantId })
+  // 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')
 
-  return 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,
+              })),
+          },
         },
       },
-    },
+    })
+
+    await logActivity(groupId, ActivityType.UPDATE_GROUP, { participantId }, tx)
+    return group
   })
 }
 
@@ -534,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,

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

src/trpc/routers/groups/update.procedure.ts

@@ -1,8 +1,16 @@
-import { updateGroup } from '@/lib/api'
+import { Prisma } from '@/generated/prisma/client'
+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({
@@ -12,5 +20,30 @@ export const updateGroupProcedure = baseProcedure
     }),
   )
   .mutation(async ({ input: { groupId, groupFormValues, participantId } }) => {
-    await updateGroup(groupId, groupFormValues, participantId)
+    try {
+      await updateGroup(groupId, groupFormValues, participantId)
+    } catch (error) {
+      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' &&
+        PARTICIPANT_EXPENSE_CONSTRAINTS.includes(
+          (error.meta as any)?.driverAdapterError?.cause?.constraint?.index,
+        )
+      ) {
+        throw new TRPCError({
+          code: 'CONFLICT',
+          message: 'A removed participant is part of expenses.',
+        })
+      }
+      throw error
+    }
   })