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