diff --git a/client/src/components/Trips/TripMembersModal.tsx b/client/src/components/Trips/TripMembersModal.tsx index f1bb06a69..d9c32d8bd 100644 --- a/client/src/components/Trips/TripMembersModal.tsx +++ b/client/src/components/Trips/TripMembersModal.tsx @@ -290,6 +290,7 @@ export default function TripMembersModal({ isOpen, onClose, tripId, tripTitle, o const { t } = useTranslation() const can = useCanDo() const trip = useTripStore((s) => s.trip) + const loadBudgetItems = useTripStore((s) => s.loadBudgetItems) const canManageMembers = can('member_manage', trip) const canManageShare = can('share_manage', trip) @@ -387,6 +388,7 @@ export default function TripMembersModal({ isOpen, onClose, tripId, tripTitle, o try { await tripsApi.deleteGuest(tripId, userId) await loadMembers(true) + await loadBudgetItems(tripId) toast.success(t('members.guestRemoved')) } catch { toast.error(t('members.removeError')) diff --git a/server/src/services/budgetService.ts b/server/src/services/budgetService.ts index 6715add00..7156aaddf 100644 --- a/server/src/services/budgetService.ts +++ b/server/src/services/budgetService.ts @@ -30,13 +30,23 @@ function loadItemPayers(itemId: number | string) { return rows.map(p => ({ ...p, avatar_url: avatarUrl(p) })); } +function knownUserIds(userIds: number[]): Set { + const unique = [...new Set(userIds)]; + if (unique.length === 0) return new Set(); + const rows = db.prepare( + `SELECT id FROM users WHERE id IN (${unique.map(() => '?').join(',')})` + ).all(...unique) as { id: number }[]; + return new Set(rows.map(r => r.id)); +} + /** Replace the payer rows of an item and keep total_price = sum of payer amounts. */ function writeItemPayers(itemId: number | string, payers: { user_id: number; amount: number }[]) { db.prepare('DELETE FROM budget_item_payers WHERE budget_item_id = ?').run(itemId); const insert = db.prepare('INSERT OR IGNORE INTO budget_item_payers (budget_item_id, user_id, amount) VALUES (?, ?, ?)'); + const known = knownUserIds(payers.map(p => p.user_id)); let total = 0; for (const p of payers) { - if (!(p.amount > 0)) continue; + if (!(p.amount > 0) || !known.has(p.user_id)) continue; insert.run(itemId, p.user_id, p.amount); total += p.amount; } @@ -242,6 +252,11 @@ export function createBudgetItem( const payerTotal = (data.payers || []).reduce((a, p) => a + (p.amount > 0 ? p.amount : 0), 0); const total = data.payers && data.payers.length > 0 ? payerTotal : (data.total_price || 0); + const knownMembers = data.members ? knownUserIds(data.members.map(m => m.user_id)) : null; + const members = data.members && knownMembers ? data.members.filter(m => knownMembers.has(m.user_id)) : undefined; + const knownIds = data.member_ids ? knownUserIds(data.member_ids) : null; + const memberIds = data.member_ids && knownIds ? data.member_ids.filter(uid => knownIds.has(uid)) : undefined; + const result = db.prepare( 'INSERT INTO budget_items (trip_id, category, name, total_price, currency, exchange_rate, persons, days, note, sort_order, expense_date, reservation_id) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)' ).run( @@ -251,7 +266,7 @@ export function createBudgetItem( total, data.currency || null, data.exchange_rate != null ? data.exchange_rate : 1, - data.member_ids ? data.member_ids.length : (data.persons != null ? data.persons : null), + memberIds ? memberIds.length : (data.persons != null ? data.persons : null), data.days !== undefined && data.days !== null ? data.days : null, data.note || null, sortOrder, @@ -261,12 +276,12 @@ export function createBudgetItem( const itemId = result.lastInsertRowid as number; if (data.payers && data.payers.length > 0) writeItemPayers(itemId, data.payers); - if (data.members && data.members.length > 0) { + if (members && members.length > 0) { const insert = db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, ?)'); - for (const m of data.members) insert.run(itemId, m.user_id, m.amount !== undefined && m.amount !== null ? m.amount : null); - } else if (data.member_ids && data.member_ids.length > 0) { + for (const m of members) insert.run(itemId, m.user_id, m.amount !== undefined && m.amount !== null ? m.amount : null); + } else if (memberIds && memberIds.length > 0) { const insert = db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, NULL)'); - for (const uid of data.member_ids) insert.run(itemId, uid); + for (const uid of memberIds) insert.run(itemId, uid); } const item = db.prepare('SELECT * FROM budget_items WHERE id = ?').get(itemId) as BudgetItem; @@ -347,15 +362,19 @@ export function updateBudgetItem( } } if (data.members !== undefined) { + const known = knownUserIds(data.members.map(m => m.user_id)); + const members = data.members.filter(m => known.has(m.user_id)); db.prepare('DELETE FROM budget_item_members WHERE budget_item_id = ?').run(id); const insert = db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, ?)'); - for (const m of data.members) insert.run(id, m.user_id, m.amount !== undefined && m.amount !== null ? m.amount : null); - db.prepare('UPDATE budget_items SET persons = ? WHERE id = ?').run(data.members.length || null, id); + for (const m of members) insert.run(id, m.user_id, m.amount !== undefined && m.amount !== null ? m.amount : null); + db.prepare('UPDATE budget_items SET persons = ? WHERE id = ?').run(members.length || null, id); } else if (data.member_ids !== undefined) { + const known = knownUserIds(data.member_ids); + const memberIds = data.member_ids.filter(uid => known.has(uid)); db.prepare('DELETE FROM budget_item_members WHERE budget_item_id = ?').run(id); const insert = db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, NULL)'); - for (const uid of data.member_ids) insert.run(id, uid); - db.prepare('UPDATE budget_items SET persons = ? WHERE id = ?').run(data.member_ids.length || null, id); + for (const uid of memberIds) insert.run(id, uid); + db.prepare('UPDATE budget_items SET persons = ? WHERE id = ?').run(memberIds.length || null, id); } // If category changed, update category order table @@ -409,10 +428,12 @@ export function updateMembers(id: string | number, tripId: string | number, user db.prepare('DELETE FROM budget_item_members WHERE budget_item_id = ?').run(id); - if (userIds.length > 0) { + const known = knownUserIds(userIds); + const memberIds = userIds.filter(uid => known.has(uid)); + if (memberIds.length > 0) { const insert = db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid) VALUES (?, ?, ?)'); - for (const userId of userIds) insert.run(id, userId, existingPaid[userId] || 0); - db.prepare('UPDATE budget_items SET persons = ? WHERE id = ?').run(userIds.length, id); + for (const userId of memberIds) insert.run(id, userId, existingPaid[userId] || 0); + db.prepare('UPDATE budget_items SET persons = ? WHERE id = ?').run(memberIds.length, id); } else { db.prepare('UPDATE budget_items SET persons = NULL WHERE id = ?').run(id); } @@ -422,6 +443,23 @@ export function updateMembers(id: string | number, tripId: string | number, user return { members, item: updated }; } +export function removeUserFromBudgetItems(userId: number): void { + const itemIds = (db.prepare('SELECT DISTINCT budget_item_id FROM budget_item_members WHERE user_id = ?') + .all(userId) as { budget_item_id: number }[]).map(r => r.budget_item_id); + if (itemIds.length === 0) { + return; + } + + db.prepare('DELETE FROM budget_item_members WHERE user_id = ?').run(userId); + + const remaining = db.prepare('SELECT COUNT(*) AS count FROM budget_item_members WHERE budget_item_id = ?'); + const setPersons = db.prepare('UPDATE budget_items SET persons = ? WHERE id = ?'); + for (const itemId of itemIds) { + const { count } = remaining.get(itemId) as { count: number }; + setPersons.run(count || null, itemId); + } +} + export function toggleMemberPaid(id: string | number, tripId: string | number, userId: string | number, paid: boolean) { // Resolve the item within the caller's trip before updating. const item = db.prepare('SELECT id FROM budget_items WHERE id = ? AND trip_id = ?').get(id, tripId); diff --git a/server/src/services/tripService.ts b/server/src/services/tripService.ts index 65aa7d99d..ff1c1f808 100644 --- a/server/src/services/tripService.ts +++ b/server/src/services/tripService.ts @@ -7,7 +7,7 @@ import { erasePluginUserData } from './userCleanupService'; import { emitUserDeleted } from '../plugin-user-lifecycle'; import { Trip, User } from '../types'; import { listDays, listAccommodations, addDays } from './dayService'; -import { listBudgetItems } from './budgetService'; +import { listBudgetItems, removeUserFromBudgetItems } from './budgetService'; import { listItems as listPackingItems } from './packingService'; import { listReservations, loadEndpointsByTrip, resyncReservationDays } from './reservationService'; import { listNotes as listCollabNotes } from './collabService'; @@ -525,6 +525,9 @@ export function deleteGuest(tripId: string | number, guestUserId: number): boole // host-side per-user tables + a durable own-db erasure per granted plugin — exactly // like a full account deletion (otherwise a deleted guest's plugin data lingers). erasePluginUserData(guestUserId); + // Re-split the expenses they were part of before the cascade takes their member + // rows away — the divisor is denormalized and cannot follow a foreign key (#1553). + removeUserFromBudgetItems(guestUserId); // Deleting the guest's users row cascades its membership and every assignment join // (trip_members, budget/packing/assignment links) via the ON DELETE foreign keys. db.prepare('DELETE FROM users WHERE id = ? AND is_guest = 1').run(guestUserId); diff --git a/server/src/services/userCleanupService.ts b/server/src/services/userCleanupService.ts index 81f226644..1bcffdd5a 100644 --- a/server/src/services/userCleanupService.ts +++ b/server/src/services/userCleanupService.ts @@ -1,6 +1,7 @@ import fs from 'node:fs'; import { db } from '../db/database'; import { pluginsDataRoot } from '../nest/plugins/paths'; +import { removeUserFromBudgetItems } from './budgetService'; /** * Erase a user's PLUGIN-held data on account deletion. Two parts: @@ -44,6 +45,7 @@ export function erasePluginUserData(userId: number): void { function cleanupUserReferences(userId: number): void { db.prepare('UPDATE trip_members SET invited_by = NULL WHERE invited_by = ?').run(userId); db.prepare('UPDATE budget_items SET paid_by_user_id = NULL WHERE paid_by_user_id = ?').run(userId); + removeUserFromBudgetItems(userId); db.prepare('DELETE FROM share_tokens WHERE created_by = ?').run(userId); db.prepare('DELETE FROM journey_share_tokens WHERE created_by = ?').run(userId); // Owned journeys cascade-delete their entries/contributors/share_tokens/photos via journey_id FKs diff --git a/server/tests/unit/services/budgetServiceDb.test.ts b/server/tests/unit/services/budgetServiceDb.test.ts index c68ef67c2..741e52db7 100644 --- a/server/tests/unit/services/budgetServiceDb.test.ts +++ b/server/tests/unit/services/budgetServiceDb.test.ts @@ -41,7 +41,8 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser, createTrip } from '../../helpers/factories'; -import { createBudgetItem, updateMembers, toggleMemberPaid, calculateSettlement, rebaseTripCurrency } from '../../../src/services/budgetService'; +import { createBudgetItem, updateBudgetItem, updateMembers, toggleMemberPaid, calculateSettlement, rebaseTripCurrency } from '../../../src/services/budgetService'; +import { createGuest, deleteGuest } from '../../../src/services/tripService'; beforeAll(() => { createTables(testDb); @@ -63,6 +64,87 @@ function paidFlag(itemId: number, memberId: number): number | undefined { return row?.paid; } +describe('deleting a member re-splits their expenses (#1553)', () => { + function personsOf(itemId: number): number | null { + return (testDb.prepare('SELECT persons FROM budget_items WHERE id = ?').get(itemId) as { persons: number | null }).persons; + } + function memberCount(itemId: number): number { + return (testDb.prepare('SELECT COUNT(*) AS count FROM budget_item_members WHERE budget_item_id = ?') + .get(itemId) as { count: number }).count; + } + + it('BUDGET-SVC-DB-010: re-derives the persons divisor when a guest in the split is deleted', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const guests = ['G1', 'G2', 'G3'].map(n => createGuest(trip.id, n, owner.id).member); + const item = createBudgetItem(trip.id, { + name: 'Dinner', total_price: 400, + member_ids: [owner.id, ...guests.map(g => g.id)], + }); + expect(personsOf(item.id)).toBe(4); + + deleteGuest(trip.id, guests[0].id); + deleteGuest(trip.id, guests[1].id); + + // The member rows cascade with the users row; `persons` is denormalized and has to + // be re-derived, or the per-person column keeps dividing by the departed. + expect(memberCount(item.id)).toBe(2); + expect(personsOf(item.id)).toBe(2); + }); + + it('BUDGET-SVC-DB-011: leaves a manually entered persons count alone', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const guest = createGuest(trip.id, 'G1', owner.id).member; + // No member rows — `persons` is just a number someone typed. + const item = createBudgetItem(trip.id, { name: 'Rental', total_price: 300, persons: 6 }); + + deleteGuest(trip.id, guest.id); + + expect(personsOf(item.id)).toBe(6); + }); + + it('BUDGET-SVC-DB-012: drops the last member to a null divisor rather than zero', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const guest = createGuest(trip.id, 'G1', owner.id).member; + const item = createBudgetItem(trip.id, { name: 'Taxi', total_price: 50, member_ids: [guest.id] }); + + deleteGuest(trip.id, guest.id); + + expect(memberCount(item.id)).toBe(0); + expect(personsOf(item.id)).toBeNull(); + }); + + it('BUDGET-SVC-DB-013: saves a split from a stale client instead of failing on the users FK', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const guest = createGuest(trip.id, 'G1', owner.id).member; + const item = createBudgetItem(trip.id, { name: 'Dinner', total_price: 200, member_ids: [owner.id, guest.id] }); + + deleteGuest(trip.id, guest.id); + + // A client that loaded before the deletion still sends the guest back (#1553). + const updated = updateBudgetItem(item.id, trip.id, { member_ids: [owner.id, guest.id] }); + + expect(updated!.members.map(m => m.user_id)).toEqual([owner.id]); + expect(personsOf(item.id)).toBe(1); + }); + + it('BUDGET-SVC-DB-014: ignores a deleted member arriving through updateMembers', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const guest = createGuest(trip.id, 'G1', owner.id).member; + const item = createBudgetItem(trip.id, { name: 'Drinks', total_price: 60, member_ids: [owner.id, guest.id] }); + + deleteGuest(trip.id, guest.id); + const result = updateMembers(item.id, trip.id, [owner.id, guest.id]); + + expect(result!.members.map(m => m.user_id)).toEqual([owner.id]); + expect(personsOf(item.id)).toBe(1); + }); +}); + describe('toggleMemberPaid trip-scoping', () => { it('BUDGET-SVC-DB-001: toggles paid for an item that belongs to the given trip', () => { const { user } = createUser(testDb);