fix(events): honour ?tab=, show a load error, and stop lying about uploads
Three warnings on the event-details surface. ?tab= deep links were ignored -- activeTab was hardcoded to 'overview' and nothing read or wrote the search param, unlike Settings. Mirrors SettingsPage's pattern exactly (module-level key list + type guard, seed useState from the param, write-back and reflect-back effects), plus a snap-back for the `guests` tab, which only renders when identity_mode is 'guest' -- a deep link to it on any other event would otherwise show a tab bar with no content. The snap-back is guarded on the query's isLoading so it cannot fire against undefined settings and kill a legitimate deep link. Worth recording: the two effects ping-pong infinitely if activeTab and a valid URL tab disagree at mount, which is exactly the pre-fix state. The seeding is what makes them agree, so the fix is also what makes the pair safe. Offline Photos tab rendered the "no media uploaded yet" empty state on a failed fetch, because `data: photos = []` makes a rejected query indistinguishable from an empty one -- a user could reasonably think their photos were gone. Threaded isError through and added a third branch, reusing TaxReportPage's existing error-with-retry shape. Needed no new keys. The spurious "Upload completed successfully" toast was in the host, not the uploader: PhotosTab hung toast.success off PhotoUpload's onUploadComplete, which is documented as a grid-refresh signal and fires as soon as the transfer loop exits -- including when the request 400'd on the photo cap or every file was rejected by magic-byte validation. PhotoUpload's own toasts were already correct. Removed it, and added a real partial-success branch reporting the actual split instead of a plain "Upload complete!". The guest uploader had a variant of the same bug in a different place: its toast is gated on successCount, but successCount++ fired on any resolved request -- and the upload route answers 202 with count: 0 and an errors[] entry when the file is refused. So a refused guest photo produced "Upload completed successfully (1 photos)" and pushed a useless upload_id into the processing poll. Now gated on count. Refs testplan REPORT.md, ?tab= / offline-empty-state / spurious-toast warnings.
This commit is contained in:
Paul Nothaft
@@ -453,6 +453,17 @@ export const PhotoUpload: React.FC<PhotoUploadProps> = ({ eventId, onUploadCompl
|
||||
t('upload.processingFailed', { count: processingAggregate.failed }) ||
|
||||
`${processingAggregate.failed} photo(s) failed to process`
|
||||
);
|
||||
} else if (transferFailures.length > 0) {
|
||||
// Processing was clean, but files were rejected or lost before they got
|
||||
// there. A plain "Upload complete!" here would contradict the failure
|
||||
// report right below it (QA P4-B.05 / 7.05) — report the real split.
|
||||
toast.warning(
|
||||
t('upload.partialComplete', '{{uploaded}} of {{total}} files uploaded — {{failed}} could not be uploaded.', {
|
||||
uploaded: processingAggregate.complete,
|
||||
total: processingAggregate.complete + transferFailures.length,
|
||||
failed: transferFailures.length,
|
||||
})
|
||||
);
|
||||
} else {
|
||||
toast.success(
|
||||
t('upload.uploadComplete') || `Successfully uploaded ${processingAggregate.complete} photo(s)`
|
||||
|
||||
@@ -0,0 +1,189 @@
|
||||
/**
|
||||
* The completion toast must describe what actually happened.
|
||||
*
|
||||
* QA P4-B.05 / 7.05: uploading past a photo cap (whole request 400s) and
|
||||
* uploading a `.txt` renamed to `.jpg` (magic-byte rejection) both produced a
|
||||
* generic "Upload completed successfully" toast *alongside* the rejection
|
||||
* toast, with 0 of N files in the gallery. The success toast came from the
|
||||
* host's `onUploadComplete` handler, which PhotoUpload fires purely as a
|
||||
* "refresh the grid" signal — including on runs where nothing landed.
|
||||
*
|
||||
* These pin the outcome contract:
|
||||
* - nothing landed -> no success toast (and the refresh still fires)
|
||||
* - some landed -> an accurate partial message, not "complete!"
|
||||
* - all landed -> success
|
||||
* plus the host-side rule that the refresh callback never announces success.
|
||||
*/
|
||||
import { render, screen, waitFor } 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';
|
||||
import fs from 'fs';
|
||||
import path from 'path';
|
||||
import type { ReactElement } from 'react';
|
||||
|
||||
import { PhotoUpload } from '../PhotoUpload';
|
||||
|
||||
// Interpolating t() — the partial message is only meaningful with its numbers
|
||||
// substituted, so the mock has to do what i18next would.
|
||||
vi.mock('react-i18next', async () => {
|
||||
const actual = await vi.importActual<typeof import('react-i18next')>('react-i18next');
|
||||
return {
|
||||
...actual,
|
||||
useTranslation: () => ({
|
||||
t: (key: string, second?: any, third?: any) => {
|
||||
const fallback = typeof second === 'string' ? second : undefined;
|
||||
const vars = (typeof second === 'object' ? second : third) || {};
|
||||
let out = fallback ?? key;
|
||||
for (const [k, v] of Object.entries(vars)) {
|
||||
out = out.split(`{{${k}}}`).join(String(v));
|
||||
}
|
||||
return out;
|
||||
},
|
||||
}),
|
||||
};
|
||||
});
|
||||
|
||||
const toastMock = vi.hoisted(() => ({
|
||||
warning: vi.fn(), info: vi.fn(), error: vi.fn(), success: vi.fn(),
|
||||
}));
|
||||
vi.mock('react-toastify', () => ({ toast: toastMock }));
|
||||
|
||||
const postMock = vi.fn();
|
||||
vi.mock('../../../config/api', () => ({ api: { post: (...a: any[]) => postMock(...a), get: vi.fn() } }));
|
||||
|
||||
const hoisted = vi.hoisted(() => ({ aggregate: null as any }));
|
||||
const idle = {
|
||||
total: 0, pending: 0, processing: 0, complete: 0, failed: 0,
|
||||
failedPhotos: [] as { id: number; filename: string; error: string | null }[],
|
||||
isComplete: false, isReady: true,
|
||||
};
|
||||
vi.mock('../../../hooks/useUploadProgress', () => ({
|
||||
useUploadProgress: (ids: string[]) => ({
|
||||
snapshots: {},
|
||||
error: null,
|
||||
aggregate: ids && ids.length > 0 ? hoisted.aggregate : idle,
|
||||
}),
|
||||
}));
|
||||
|
||||
vi.mock('../../../services/categories.service', () => ({
|
||||
categoriesService: { getEventCategories: vi.fn().mockResolvedValue([]) },
|
||||
}));
|
||||
vi.mock('../../../services/settings.service', () => ({
|
||||
settingsService: { getAllSettings: vi.fn().mockResolvedValue({}) },
|
||||
}));
|
||||
|
||||
const renderWithClient = (ui: ReactElement) => {
|
||||
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
|
||||
return render(<QueryClientProvider client={queryClient}>{ui}</QueryClientProvider>);
|
||||
};
|
||||
|
||||
const makeFile = (name: string) =>
|
||||
new File([new Uint8Array([1, 2, 3])], name, { type: 'image/png' });
|
||||
|
||||
async function uploadFiles(container: HTMLElement, user: ReturnType<typeof userEvent.setup>, names: string[]) {
|
||||
const fileInput = container.querySelector('input[type="file"]') as HTMLInputElement;
|
||||
await user.upload(fileInput, names.map(makeFile));
|
||||
await user.click(screen.getByRole('button', { name: /common\.upload/ }));
|
||||
}
|
||||
|
||||
describe('PhotoUpload completion toast', () => {
|
||||
beforeEach(() => {
|
||||
postMock.mockReset();
|
||||
hoisted.aggregate = { ...idle };
|
||||
});
|
||||
afterEach(() => vi.clearAllMocks());
|
||||
|
||||
it('stays silent on success when the whole request was refused (photo cap)', async () => {
|
||||
// Backend 400s the entire upload — every file is a transfer failure.
|
||||
postMock.mockRejectedValue({
|
||||
response: { data: { error: 'Photo cap exceeded. This event allows a maximum of 3 photos.' } },
|
||||
});
|
||||
const onUploadComplete = vi.fn();
|
||||
const user = userEvent.setup();
|
||||
const { container } = renderWithClient(
|
||||
<PhotoUpload eventId={1} onUploadComplete={onUploadComplete} />
|
||||
);
|
||||
|
||||
await uploadFiles(container, user, ['a.png', 'b.png']);
|
||||
|
||||
await screen.findByTestId('upload-failure-report');
|
||||
expect(toastMock.success).not.toHaveBeenCalled();
|
||||
// The grid refresh still has to happen — it is a refresh signal, which is
|
||||
// exactly why the host must not hang a success toast off it.
|
||||
expect(onUploadComplete).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('stays silent on success when every file was rejected per-file', async () => {
|
||||
// 202, but count 0: nothing was queued (renamed .txt / magic-byte check).
|
||||
postMock.mockResolvedValue({
|
||||
data: {
|
||||
count: 0,
|
||||
upload_id: 'u1',
|
||||
errors: [{ filename: 'fake.jpg', error: 'File content does not match declared type' }],
|
||||
},
|
||||
});
|
||||
const user = userEvent.setup();
|
||||
const { container } = renderWithClient(<PhotoUpload eventId={1} />);
|
||||
|
||||
await uploadFiles(container, user, ['fake.jpg']);
|
||||
|
||||
await screen.findByTestId('upload-failure-report');
|
||||
expect(toastMock.success).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('reports the real split when some files land and others do not', async () => {
|
||||
postMock.mockResolvedValue({
|
||||
data: {
|
||||
count: 1,
|
||||
upload_id: 'u1',
|
||||
errors: [{ filename: 'fake.jpg', error: 'File content does not match declared type' }],
|
||||
},
|
||||
});
|
||||
hoisted.aggregate = {
|
||||
total: 1, pending: 0, processing: 0, complete: 1, failed: 0,
|
||||
failedPhotos: [], isComplete: true, isReady: true,
|
||||
};
|
||||
const user = userEvent.setup();
|
||||
const { container } = renderWithClient(<PhotoUpload eventId={1} />);
|
||||
|
||||
await uploadFiles(container, user, ['good.png', 'fake.jpg']);
|
||||
|
||||
await waitFor(() =>
|
||||
expect(toastMock.warning).toHaveBeenCalledWith(
|
||||
'1 of 2 files uploaded — 1 could not be uploaded.'
|
||||
)
|
||||
);
|
||||
expect(toastMock.success).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('still congratulates a clean upload', async () => {
|
||||
postMock.mockResolvedValue({ data: { count: 1, upload_id: 'u1', errors: [] } });
|
||||
hoisted.aggregate = {
|
||||
total: 1, pending: 0, processing: 0, complete: 1, failed: 0,
|
||||
failedPhotos: [], isComplete: true, isReady: true,
|
||||
};
|
||||
const user = userEvent.setup();
|
||||
const { container } = renderWithClient(<PhotoUpload eventId={1} />);
|
||||
|
||||
await uploadFiles(container, user, ['good.png']);
|
||||
|
||||
await waitFor(() => expect(toastMock.success).toHaveBeenCalled());
|
||||
expect(toastMock.warning).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('host refresh callback', () => {
|
||||
it('does not announce success from the Photos tab refresh handler', () => {
|
||||
const source = fs.readFileSync(
|
||||
path.join(__dirname, '..', '..', '..', 'pages', 'admin', 'event-details', 'PhotosTab.tsx'),
|
||||
'utf8'
|
||||
);
|
||||
const handler = source.slice(
|
||||
source.indexOf('onUploadComplete={'),
|
||||
source.indexOf('{/* Photo Filters */}')
|
||||
);
|
||||
expect(handler).toContain('refetchPhotos()');
|
||||
expect(handler).not.toContain('toast.success');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user