diff --git a/frontend/src/components/admin/PhotoUpload.tsx b/frontend/src/components/admin/PhotoUpload.tsx index f85e98a3..7d3f0ea1 100644 --- a/frontend/src/components/admin/PhotoUpload.tsx +++ b/frontend/src/components/admin/PhotoUpload.tsx @@ -68,6 +68,11 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl // validation rejections (response.errors) and whole-chunk failures. // Processing failures are merged in from the progress hook below. const [transferFailures, setTransferFailures] = useState([]); + // Processing failures are captured into state (not read live) because the + // completion effect clears uploadIds, which empties the progress hook's + // failedPhotos — reading live would make the rows vanish the instant they + // appear. + const [processingFailures, setProcessingFailures] = useState([]); const [failuresDismissed, setFailuresDismissed] = useState(false); const fileInputRef = useRef(null); @@ -76,16 +81,12 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl }); // Single source of truth for the "which files failed" report: transfer - // stage failures (collected during handleUpload) plus processing - // failures (live, from the progress hook). Both carry filename + reason. - const failures = useMemo(() => { - const processing: UploadFailure[] = processingAggregate.failedPhotos.map((p) => ({ - filename: p.filename, - reason: p.error || t('upload.failures.unknownReason', 'Unknown error'), - kind: 'processing', - })); - return [...transferFailures, ...processing]; - }, [transferFailures, processingAggregate.failedPhotos, t]); + // stage failures (collected during handleUpload) plus processing failures + // (captured on completion). Both carry filename + reason. + const failures = useMemo( + () => [...transferFailures, ...processingFailures], + [transferFailures, processingFailures] + ); // Fetch categories for this event const { data: categories = [] } = useQuery({ @@ -203,6 +204,7 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl setUploadIds([]); // Clear any prior failure report before this run. setTransferFailures([]); + setProcessingFailures([]); setFailuresDismissed(false); // For large uploads, chunk the files by both count AND size to prevent memory/network issues. @@ -317,9 +319,12 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl }); } } - // Backend returns a per-request upload_id. Track it so the - // processing-status hook can poll/stream live progress. - if (response.data?.upload_id) { + // Track the per-request upload_id so the processing hook can poll + // for live progress — but only when photos were actually queued + // (count > 0). The backend returns an upload_id even when every + // file was rejected (count 0); tracking it there would make us wait + // for a processing phase that never starts, hanging the spinner. + if (response.data?.upload_id && (response.data?.count ?? 0) > 0) { anyQueued = true; const newId = response.data.upload_id as string; setUploadIds((prev) => (prev.includes(newId) ? prev : [...prev, newId])); @@ -371,25 +376,20 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl onUploadComplete(); } - // Tell the host the transfer stage settled. A clean upload lets the - // modal auto-close (unchanged behaviour); a partial failure keeps it - // open so the failure report below stays visible. - onUploadSettled?.({ hasFailures: collected.length > 0 }); - - // Nothing was queued for background processing (every chunk failed, - // or a pre-async backend) — there's no processing phase to wait on, - // so reset the transfer UI here. The failure report persists. + // Settling (and the modal's close decision) is deferred until we know + // the WHOLE outcome, including background processing. If nothing was + // queued (every file rejected, or a pre-async backend), the transfer + // stage is already terminal — settle now and reset the UI. Otherwise + // the processing effect below settles once the worker finishes, so + // processing failures are included before the modal decides to close. if (!anyQueued) { + onUploadSettled?.({ hasFailures: collected.length > 0 }); setIsUploading(false); setUploadProgress(0); setCurrentChunk(0); setTotalChunks(0); setPhase({ kind: 'idle' }); } - - // If the backend never returned an upload_id (e.g. only failures - // or pre-async-backend deployment), we have nothing to wait for — - // fall through to the finally cleanup which resets state. } catch (error: any) { console.error('Upload error:', error); toast.error(error.response?.data?.error || t('toast.uploadError')); @@ -410,6 +410,15 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl if (!processingAggregate.isComplete) return; if (processingAggregate.failed > 0) { + // Persist the failed photos into the report before uploadIds is cleared + // below (which would otherwise empty the progress hook's failedPhotos). + setProcessingFailures( + processingAggregate.failedPhotos.map((p) => ({ + filename: p.filename, + reason: p.error || t('upload.failures.unknownReason', 'Unknown error'), + kind: 'processing' as const, + })) + ); toast.warning( t('upload.processingFailed', { count: processingAggregate.failed }) || `${processingAggregate.failed} photo(s) failed to process` @@ -421,6 +430,12 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl } if (onUploadComplete) onUploadComplete(); + // Now the whole outcome is known — settle. The modal auto-closes only + // when nothing failed at either stage; any transfer OR processing + // failure keeps it open so the report (which lists both) stays visible. + onUploadSettled?.({ + hasFailures: transferFailures.length > 0 || processingAggregate.failed > 0, + }); setIsUploading(false); setUploadProgress(0); setCurrentChunk(0); @@ -578,6 +593,8 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl {!failuresDismissed && failures.length > 0 && (
diff --git a/frontend/src/components/admin/__tests__/PhotoUpload.failureReport.test.tsx b/frontend/src/components/admin/__tests__/PhotoUpload.failureReport.test.tsx index b6bd9ce3..0896b971 100644 --- a/frontend/src/components/admin/__tests__/PhotoUpload.failureReport.test.tsx +++ b/frontend/src/components/admin/__tests__/PhotoUpload.failureReport.test.tsx @@ -3,12 +3,14 @@ * * Before this, a partial upload only showed a count ("some files failed"), * and the backend's per-file `errors[]` were dropped entirely. These tests - * pin that every failure stage is now named with its reason: + * pin that every failure stage is named with its reason: * - rejected: from the upload response's `errors: [{filename, error}]` + * - transfer: from a whole-chunk POST failure (the `catch` path) * - processing: from useUploadProgress's `failedPhotos` - * and that the report can be dismissed. + * plus the settle contract (hasFailures true/false) the modal relies on to + * decide whether to auto-close, and dismissal. */ -import { render, screen, within } from '@testing-library/react'; +import { render, screen, waitFor, within } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; @@ -30,26 +32,26 @@ vi.mock('react-toastify', () => ({ toast: { warning: vi.fn(), info: vi.fn(), error: vi.fn(), success: vi.fn() }, })); -// The upload POST: 202 Accepted, one file queued, one rejected per-file. -const postMock = vi.fn().mockResolvedValue({ - data: { - successCount: 1, - upload_id: 'u1', - errors: [{ filename: 'too-big.png', error: 'File too large' }], - }, -}); +const postMock = vi.fn(); vi.mock('../../../config/api', () => ({ api: { post: (...a: any[]) => postMock(...a), get: vi.fn() } })); -// One photo failed in the background worker. +// Mutable processing aggregate, swapped per test. Returned only once photos +// are queued (uploadIds non-empty), mirroring the real hook flipping from +// "nothing to track" to "complete" — the transition that fires the settle +// effect. +const clean = () => ({ + total: 0, pending: 0, processing: 0, complete: 0, failed: 0, + failedPhotos: [] as { id: number; filename: string; error: string | null }[], + isComplete: false, isReady: true, +}); +const hoisted = vi.hoisted(() => ({ aggregate: null as any })); vi.mock('../../../hooks/useUploadProgress', () => ({ - useUploadProgress: () => ({ + useUploadProgress: (ids: string[]) => ({ snapshots: {}, error: null, - aggregate: { - total: 2, pending: 0, processing: 0, complete: 1, failed: 1, - failedPhotos: [{ id: 5, filename: 'corrupt.jpg', error: 'Unsupported format' }], - isComplete: true, isReady: true, - }, + aggregate: ids && ids.length > 0 + ? hoisted.aggregate + : { total: 0, pending: 0, processing: 0, complete: 0, failed: 0, failedPhotos: [], isComplete: false, isReady: true }, }), })); @@ -65,40 +67,83 @@ const renderWithClient = (ui: ReactElement) => { return render({ui}); }; -async function uploadOneFile(container: HTMLElement, user: ReturnType) { +async function uploadFile(container: HTMLElement, user: ReturnType) { const fileInput = container.querySelector('input[type="file"]') as HTMLInputElement; - await user.upload(fileInput, new File([new Uint8Array([1, 2, 3])], 'too-big.png', { type: 'image/png' })); + await user.upload(fileInput, new File([new Uint8Array([1, 2, 3])], 'good-photo.png', { type: 'image/png' })); await user.click(screen.getByRole('button', { name: /common\.upload/ })); } describe('PhotoUpload failure report', () => { - beforeEach(() => postMock.mockClear()); + beforeEach(() => { + postMock.mockReset(); + hoisted.aggregate = clean(); + }); afterEach(() => vi.clearAllMocks()); - it('names each failed file with its reason and failure stage', async () => { + it('names rejected + processing failures and settles with hasFailures', async () => { + postMock.mockResolvedValue({ + data: { successCount: 1, count: 1, upload_id: 'u1', errors: [{ filename: 'too-big.png', error: 'File too large' }] }, + }); + hoisted.aggregate = { + total: 2, pending: 0, processing: 0, complete: 1, failed: 1, + failedPhotos: [{ id: 5, filename: 'corrupt.jpg', error: 'Unsupported format' }], + isComplete: true, isReady: true, + }; + const onUploadSettled = vi.fn(); const user = userEvent.setup(); - const { container } = renderWithClient(); + const { container } = renderWithClient(); - await uploadOneFile(container, user); + await uploadFile(container, user); const report = await screen.findByTestId('upload-failure-report'); - - // Rejected file (from the response errors[] that used to be dropped) expect(within(report).getByText('too-big.png')).toBeInTheDocument(); expect(within(report).getByText(/File too large/)).toBeInTheDocument(); expect(within(report).getByText('Rejected')).toBeInTheDocument(); - - // Processing failure (from useUploadProgress.failedPhotos) expect(within(report).getByText('corrupt.jpg')).toBeInTheDocument(); - expect(within(report).getByText(/Unsupported format/)).toBeInTheDocument(); expect(within(report).getByText('Processing failed')).toBeInTheDocument(); + + // Contract with the modal: the real component fires onUploadSettled and, + // because something failed, asks the host NOT to auto-close. + await waitFor(() => expect(onUploadSettled).toHaveBeenCalledWith({ hasFailures: true })); }); - it('can be dismissed', async () => { + it('reports a whole-chunk transfer failure by name', async () => { + postMock.mockRejectedValue({ response: { data: { error: 'Network error' } } }); const user = userEvent.setup(); const { container } = renderWithClient(); - await uploadOneFile(container, user); + await uploadFile(container, user); + + const report = await screen.findByTestId('upload-failure-report'); + expect(within(report).getByText('good-photo.png')).toBeInTheDocument(); + expect(within(report).getByText(/Network error/)).toBeInTheDocument(); + expect(within(report).getByText('Transfer failed')).toBeInTheDocument(); + }); + + it('settles clean when nothing fails, so the modal can auto-close', async () => { + postMock.mockResolvedValue({ data: { successCount: 1, count: 1, upload_id: 'u1', errors: [] } }); + hoisted.aggregate = { + total: 1, pending: 0, processing: 0, complete: 1, failed: 0, + failedPhotos: [], isComplete: true, isReady: true, + }; + const onUploadSettled = vi.fn(); + const user = userEvent.setup(); + const { container } = renderWithClient(); + + await uploadFile(container, user); + + await waitFor(() => expect(onUploadSettled).toHaveBeenCalledWith({ hasFailures: false })); + expect(screen.queryByTestId('upload-failure-report')).not.toBeInTheDocument(); + }); + + it('can be dismissed', async () => { + postMock.mockResolvedValue({ + data: { successCount: 0, count: 0, errors: [{ filename: 'too-big.png', error: 'File too large' }] }, + }); + const user = userEvent.setup(); + const { container } = renderWithClient(); + + await uploadFile(container, user); const report = await screen.findByTestId('upload-failure-report'); await user.click(within(report).getByRole('button', { name: /Dismiss/i }));