From 6a3d4b128141691bcf770efcd449d0341e827f44 Mon Sep 17 00:00:00 2001 From: Maurice Date: Tue, 8 Sep 2026 21:37:49 +0200 Subject: [PATCH] fix(budget): stop receipts deleting files, and share the upload path Attaching a receipt no longer lets the budget domain delete one. Trashing the file was reachable from the REST route, the MCP tool and the plugin RPC, none of which check file_delete, and a receipt id is any file on the trip, so anyone who could edit an expense could trash a document they were not allowed to touch. A concurrent edit could do it by accident: the client sends the full set of receipt ids from a snapshot taken when the form opened, so saving an expense someone else had just attached a file to moved that file to the trash. Removing a receipt now removes the link and nothing else. The link row is treated as shared, because it is. One row can tie a file to a place and to an expense at once, so the sync clears only the receipt half and leaves the rest, the foreign key is SET NULL rather than CASCADE, and a receipt already in the trash keeps its link so restoring it comes back attached. The upload and its rollback moved into one module both shells call. It was copied into six places, and all six rolled back with the permanent-delete route, which only accepts a file that is already in the trash and therefore answered 404 every time: a failed save left every uploaded file on the trip, attached to the expense, and the next attempt uploaded another copy. It now trashes first, keeps the pending files for the retry, and says so when a file could not be removed instead of swallowing it. Escape in the receipt viewer no longer closes the expense form behind it, the download fallback uses a real accent class rather than a utility that does not exist, and the upload indicator is actually switched on. Tests: the receipt link behaviour on both service paths, the four new branches in the files service, the upload module, and the viewer. --- client/src/components/Budget/CostsPanel.tsx | 67 +++-------- .../Budget/ReceiptPreviewModal.test.tsx | 99 ++++++++++++++++ .../components/Budget/ReceiptPreviewModal.tsx | 19 +++- .../components/Budget/receiptUploads.test.ts | 91 +++++++++++++++ .../src/components/Budget/receiptUploads.ts | 87 ++++++++++++++ .../mobile/screens/trip/sheets/MCostSheet.tsx | 62 +++------- .../unit/mobile/trip/MCostSheet.test.tsx | 3 + server/src/db/migrations.ts | 9 +- server/src/nest/budget/budget.service.ts | 106 ++++++++---------- .../tests/unit/nest/budget.service.db.test.ts | 99 ++++++++++------ server/tests/unit/nest/files.service.test.ts | 58 ++++++++++ shared/src/i18n/ar/budget.ts | 1 + shared/src/i18n/br/budget.ts | 1 + shared/src/i18n/ca/budget.ts | 1 + shared/src/i18n/cs/budget.ts | 1 + shared/src/i18n/de/budget.ts | 1 + shared/src/i18n/en/budget.ts | 1 + shared/src/i18n/es/budget.ts | 1 + shared/src/i18n/fr/budget.ts | 1 + shared/src/i18n/gr/budget.ts | 1 + shared/src/i18n/hu/budget.ts | 1 + shared/src/i18n/id/budget.ts | 1 + shared/src/i18n/it/budget.ts | 1 + shared/src/i18n/ja/budget.ts | 1 + shared/src/i18n/ko/budget.ts | 1 + shared/src/i18n/nl/budget.ts | 1 + shared/src/i18n/pl/budget.ts | 1 + shared/src/i18n/ru/budget.ts | 1 + shared/src/i18n/sv/budget.ts | 1 + shared/src/i18n/tr/budget.ts | 1 + shared/src/i18n/uk/budget.ts | 1 + shared/src/i18n/vi/budget.ts | 1 + shared/src/i18n/zh-TW/budget.ts | 1 + shared/src/i18n/zh/budget.ts | 1 + 34 files changed, 525 insertions(+), 198 deletions(-) create mode 100644 client/src/components/Budget/ReceiptPreviewModal.test.tsx create mode 100644 client/src/components/Budget/receiptUploads.test.ts create mode 100644 client/src/components/Budget/receiptUploads.ts diff --git a/client/src/components/Budget/CostsPanel.tsx b/client/src/components/Budget/CostsPanel.tsx index 820651cb0..c4d015fdf 100644 --- a/client/src/components/Budget/CostsPanel.tsx +++ b/client/src/components/Budget/CostsPanel.tsx @@ -7,7 +7,8 @@ import { useSettingsStore } from '../../store/settingsStore' import { useCanDo } from '../../store/permissionsStore' import { useToast } from '../shared/Toast' import { useTranslation } from '../../i18n' -import { budgetApi, filesApi } from '../../api/client' +import { budgetApi } from '../../api/client' +import { saveWithReceipts } from './receiptUploads' import { useExchangeRates } from '../../hooks/useExchangeRates' import { useIsMobile } from '../../hooks/useIsMobile' import { formatMoney, currencyDecimals, currencyLocale, localizeAmountInput, amountToInputString } from '../../utils/formatters' @@ -1372,57 +1373,23 @@ export function ExpenseModal({ tripId, base, people, me, editing, prefill, onClo ...(!editing && prefill?.placeId ? { place_id: prefill.placeId } : {}), } try { - // Upload any pending receipt files - const uploadedFileIds: number[] = [] - try { - for (const f of pendingReceiptFiles) { - const fd = new FormData() - fd.append('file', f) - if (editing) { - fd.append('budget_item_id', String(editing.id)) - } - const res = await filesApi.upload(tripId, fd) - if (res?.file?.id) { - uploadedFileIds.push(res.file.id) - } - } - } catch (err) { - for (const fid of uploadedFileIds) { - try { await filesApi.permanentDelete(tripId, fid) } catch {} - } - throw err - } - - let createdOrUpdated: BudgetItem - if (editing) { - try { - createdOrUpdated = await updateBudgetItem(tripId, editing.id, { - ...data, - receipt_file_ids: [...receipts.map(r => r.id), ...uploadedFileIds], - }) - } catch (err) { - for (const fid of uploadedFileIds) { - try { await filesApi.permanentDelete(tripId, fid) } catch {} - } - throw err - } - } else { - try { - createdOrUpdated = await addBudgetItem(tripId, { - ...data, - receipt_file_ids: uploadedFileIds, - }) - } catch (err) { - for (const fid of uploadedFileIds) { - try { await filesApi.permanentDelete(tripId, fid) } catch {} - } - throw err - } - } + setUploadingReceipt(pendingReceiptFiles.length > 0) + await saveWithReceipts(tripId, pendingReceiptFiles, editing ? editing.id : null, ids => ( + editing + ? updateBudgetItem(tripId, editing.id, { ...data, receipt_file_ids: [...receipts.map(r => r.id), ...ids] }) + : addBudgetItem(tripId, { ...data, receipt_file_ids: ids }) + )) + // Only cleared once the save went through, so a retry after a failure + // does not upload a second copy of every file. + setPendingReceiptFiles([]) onSaved() - } catch { - toast.error(t('common.unknownError')) + } catch (err) { + // A receipt the rollback could not remove is still on the trip, and the + // user is the only one who can clear it out of the Files tab. + const stuck = (err as { stuckReceiptIds?: number[] })?.stuckReceiptIds + toast.error(stuck?.length ? t('costs.receiptLeftBehind', { count: stuck.length }) : t('common.unknownError')) } finally { + setUploadingReceipt(false) setSaving(false) } } diff --git a/client/src/components/Budget/ReceiptPreviewModal.test.tsx b/client/src/components/Budget/ReceiptPreviewModal.test.tsx new file mode 100644 index 000000000..dbcfd0cf4 --- /dev/null +++ b/client/src/components/Budget/ReceiptPreviewModal.test.tsx @@ -0,0 +1,99 @@ +// FE-BUDGET-PREVIEW-001 to FE-BUDGET-PREVIEW-008: the receipt viewer that opens +// on top of the expense form. +import { vi } from 'vitest' +import { render, screen, waitFor, fireEvent } from '../../../tests/helpers/render' +import { ReceiptPreviewModal } from './ReceiptPreviewModal' +import type { BudgetItemReceipt } from '../../types' + +const authUrl = vi.hoisted(() => ({ get: vi.fn() })) +vi.mock('../../api/authUrl', () => ({ getAuthUrl: (...a: unknown[]) => authUrl.get(...a) })) + +function receipt(over: Partial = {}): BudgetItemReceipt { + return { + id: 1, + filename: 'stored.jpg', + original_name: 'lunch.jpg', + mime_type: 'image/jpeg', + file_size: 100, + url: '/uploads/files/stored.jpg', + ...over, + } as BudgetItemReceipt +} + +beforeEach(() => { + authUrl.get.mockReset().mockResolvedValue('/signed/stored.jpg') +}) + +describe('ReceiptPreviewModal', () => { + it('FE-BUDGET-PREVIEW-001: renders an image receipt once the signed url is in', async () => { + render() + await waitFor(() => expect(screen.getByRole('img')).toHaveAttribute('src', '/signed/stored.jpg')) + }) + + it('FE-BUDGET-PREVIEW-002: renders a PDF in a frame rather than an image', async () => { + render() + // The viewer renders through a portal, so it lives on document.body. + await waitFor(() => expect(document.body.querySelector('object[type="application/pdf"]')).not.toBeNull()) + expect(screen.queryByRole('img')).toBeNull() + }) + + it('FE-BUDGET-PREVIEW-003: a type it cannot show falls back instead of rendering an empty frame', async () => { + render() + await waitFor(() => expect(authUrl.get).toHaveBeenCalled()) + expect(document.body.querySelector('object')).toBeNull() + expect(screen.queryByRole('img')).toBeNull() + expect(screen.getAllByText('receipts.zip').length).toBeGreaterThan(0) + }) + + it('FE-BUDGET-PREVIEW-004: a rejected signed url leaves the viewer up rather than blank-screening', async () => { + authUrl.get.mockRejectedValue(new Error('403')) + render() + await waitFor(() => expect(authUrl.get).toHaveBeenCalled()) + expect(screen.getAllByText('lunch.jpg').length).toBeGreaterThan(0) + }) + + it('FE-BUDGET-PREVIEW-005: arrows walk the list and stop at both ends', async () => { + const two = [receipt({ id: 1, original_name: 'one.jpg' }), receipt({ id: 2, original_name: 'two.jpg' })] + render() + await waitFor(() => expect(screen.getAllByText('one.jpg').length).toBeGreaterThan(0)) + + fireEvent.keyDown(window, { key: 'ArrowLeft' }) + expect(screen.getAllByText('one.jpg').length).toBeGreaterThan(0) + + fireEvent.keyDown(window, { key: 'ArrowRight' }) + await waitFor(() => expect(screen.getAllByText('two.jpg').length).toBeGreaterThan(0)) + + fireEvent.keyDown(window, { key: 'ArrowRight' }) + expect(screen.getAllByText('two.jpg').length).toBeGreaterThan(0) + }) + + it('FE-BUDGET-PREVIEW-006: an out-of-range initialIndex is clamped instead of rendering nothing', async () => { + const two = [receipt({ id: 1, original_name: 'one.jpg' }), receipt({ id: 2, original_name: 'two.jpg' })] + render() + await waitFor(() => expect(screen.getAllByText('two.jpg').length).toBeGreaterThan(0)) + }) + + it('FE-BUDGET-PREVIEW-007: Escape closes the viewer and is stopped before the form behind it sees it', async () => { + const onClose = vi.fn() + const parent = vi.fn() + document.addEventListener('keydown', parent) + try { + render() + await waitFor(() => expect(authUrl.get).toHaveBeenCalled()) + + fireEvent.keyDown(window, { key: 'Escape' }) + expect(onClose).toHaveBeenCalledTimes(1) + // The expense form registers its own Escape handler on `document`; one + // keypress must not close both and throw away the user's edits. + expect(parent).not.toHaveBeenCalled() + } finally { + document.removeEventListener('keydown', parent) + } + }) + + it('FE-BUDGET-PREVIEW-008: an empty list renders nothing at all', () => { + render() + expect(document.body.querySelector('object')).toBeNull() + expect(screen.queryByRole('img')).toBeNull() + }) +}) diff --git a/client/src/components/Budget/ReceiptPreviewModal.tsx b/client/src/components/Budget/ReceiptPreviewModal.tsx index ec965ccc3..351c2afec 100644 --- a/client/src/components/Budget/ReceiptPreviewModal.tsx +++ b/client/src/components/Budget/ReceiptPreviewModal.tsx @@ -45,12 +45,23 @@ export function ReceiptPreviewModal({ receipts, initialIndex = 0, onClose }: Rec useEffect(() => { const handleKey = (e: KeyboardEvent) => { - if (e.key === 'Escape') onClose() + if (e.key === 'Escape') { + // The preview opens on top of the expense form, and both the desktop + // Modal and the phone MSheet close themselves on Escape from a listener + // on `document`. Without this the one keypress closed the form too and + // threw away whatever the user had typed into it. + e.preventDefault() + e.stopPropagation() + onClose() + return + } if (e.key === 'ArrowLeft' && index > 0) setIndex(i => i - 1) if (e.key === 'ArrowRight' && index < receipts.length - 1) setIndex(i => i + 1) } - window.addEventListener('keydown', handleKey) - return () => window.removeEventListener('keydown', handleKey) + // Capture phase, so this runs before the listeners the parents put on + // `document`; stopPropagation on the bubble phase would be too late. + window.addEventListener('keydown', handleKey, true) + return () => window.removeEventListener('keydown', handleKey, true) }, [index, receipts.length, onClose]) if (!current) return null @@ -371,7 +382,7 @@ export function ReceiptPreviewModal({ receipts, initialIndex = 0, onClose }: Rec