fix(admin): the "Uncategorized" photo filter returns every photo (#1211) (#1214)

The dropdown offered the filter and it never worked. It rendered as
`value="0"`, and adminPhotos.js skips '0' outright:

    if (category_id !== undefined && category_id !== '' && category_id !== '0') {

so no category condition was applied and the whole event came back. Four lines
below that guard sits the branch that does the work, keyed on the literal
'uncategorized' — which nothing was sending. The two ends have never agreed on
the wire value, and neither is wrong on its own.

It fails silently, which is why it went unnoticed: a full list reads as 'the
filter found nothing to narrow' rather than 'the filter did not run'.

Send what the backend already understands rather than teaching it a second
spelling. The onChange passes non-numeric values through unchanged, so the
string arrives intact.

Reported in #1209 by someone re-categorising a few thousand photos imported
without a category — the filter is the first step of filter, Select All, bulk
assign, so its failure takes the whole path with it.

Tests both ends of the contract, since the bug was the pairing rather than
either half: the frontend emits 'uncategorized', and the endpoint answers it
with only the null-category rows. The backend test also pins that 0 means no
filter, so a future change there has to be a decision rather than an accident.

Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
2026-08-28 08:05:32 +02:00
committed by GitHub
co-authored by Paul Nothaft
parent 0a36ca6056
commit a490b64954
3 changed files with 197 additions and 1 deletions
@@ -0,0 +1,107 @@
/**
* The admin photo list's category filter, and the value it answers to (#1211).
*
* The frontend used to send `category_id=0` for "Uncategorized". This route
* skips `'0'` outright — the guard reads `category_id !== '0'` — so no
* condition was applied and the whole event came back. Four lines below that
* guard sits the branch that does the work, keyed on the literal
* `uncategorized`, which nothing was sending.
*
* Reported in #1209 by someone trying to isolate a few thousand uncategorised
* imports. The frontend half is fixed in PhotoFilters; this pins the backend
* half of the same contract, because the failure mode was the two ends
* disagreeing about a string and neither one being wrong on its own.
*/
const request = require('supertest');
const express = require('express');
const { bootCrmDb, seedMinimal } = require('./helpers/crmDb');
describe('admin photo list — uncategorized filter (#1211)', () => {
let db; let cleanup; let app;
let eventId; let categoryId;
let uncategorisedIds; let categorisedId;
const list = async (query = '') => {
const res = await request(app).get(`/api/admin/events/${eventId}/photos${query}`);
expect(res.status).toBe(200);
const photos = Array.isArray(res.body) ? res.body : res.body.photos;
return (photos || []).map((p) => p.id).sort((a, b) => a - b);
};
beforeAll(async () => {
jest.resetModules();
jest.doMock('../../src/middleware/auth', () => ({
adminAuth: (req, _res, next) => { req.admin = { id: 1, username: 'tester' }; next(); },
}));
jest.doMock('../../src/middleware/permissions', () => ({
requirePermission: () => (_req, _res, next) => next(),
}));
jest.doMock('../../src/middleware/ownership', () => ({
requireEventOwnership: (_req, _res, next) => next(),
}));
jest.doMock('../../src/utils/logger', () => ({
debug: jest.fn(), info: jest.fn(), warn: jest.fn(), error: jest.fn(),
}));
({ db, cleanup } = await bootCrmDb());
await seedMinimal(db);
const [ev] = await db('events').insert({
slug: 'uncat-filter', event_type: 'wedding', event_name: 'Uncat Filter',
event_date: '2026-08-01', host_email: '[email protected]', admin_email: '[email protected]',
password_hash: 'x', share_link: '/gallery/uncat-filter/share',
expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(),
is_active: 1, is_archived: 0, is_draft: 0, created_at: new Date().toISOString(),
}).returning('id');
eventId = typeof ev === 'object' ? ev.id : ev;
const [cat] = await db('photo_categories')
.insert({ name: 'Ceremony', slug: 'ceremony', event_id: eventId })
.returning('id');
categoryId = typeof cat === 'object' ? cat.id : cat;
const insertPhoto = async (filename, category) => {
const [p] = await db('photos').insert({
event_id: eventId, filename, path: `events/uncat/${filename}`,
type: 'individual', category_id: category,
uploaded_at: new Date().toISOString(),
}).returning('id');
return typeof p === 'object' ? p.id : p;
};
// Two with no category — the shape a plugin upload leaves behind — and one
// filed properly, so a filter that does nothing is visibly different from
// a filter that works.
uncategorisedIds = [await insertPhoto('a.jpg', null), await insertPhoto('b.jpg', null)];
categorisedId = await insertPhoto('c.jpg', categoryId);
uncategorisedIds.sort((a, b) => a - b);
app = express();
app.use(express.json());
app.use('/api/admin/events', require('../../src/routes/adminPhotos'));
}, 180000);
afterAll(async () => { if (cleanup) await cleanup(); });
it('returns only the photos with no category', async () => {
expect(await list('?category_id=uncategorized')).toEqual(uncategorisedIds);
});
it('returns everything when no category filter is given', async () => {
expect(await list()).toEqual([...uncategorisedIds, categorisedId].sort((a, b) => a - b));
});
it('still filters by a real category id', async () => {
expect(await list(`?category_id=${categoryId}`)).toEqual([categorisedId]);
});
it('treats 0 as no filter at all', async () => {
// Pinning the behaviour that made the bug silent rather than loud: '0' is
// not "uncategorized" and never was, it simply falls through the guard. A
// future change that made 0 mean uncategorized here would be fine too —
// but it must be a decision, not an accident, and this test forces it.
expect(await list('?category_id=0')).toEqual([...uncategorisedIds, categorisedId].sort((a, b) => a - b));
});
});
@@ -63,7 +63,12 @@ export const PhotoFilters: React.FC<PhotoFiltersProps> = ({
className="px-3 py-2 border border-neutral-300 dark:border-neutral-600 rounded-lg bg-white dark:bg-neutral-800 text-neutral-900 dark:text-neutral-100 focus:ring-2 focus:ring-primary-500 focus:border-accent-dark"
>
<option value="">{t('gallery.allCategories', 'All Categories')}</option>
<option value="0">{t('gallery.uncategorized', 'Uncategorized')}</option>
{/* The literal the backend understands, not 0 (#1211). It skips
'0' outright — `category_id !== '0'` — so this filter used to
apply no condition at all and quietly returned the whole event.
The onChange below passes non-numeric values through unchanged,
so the string arrives intact. */}
<option value="uncategorized">{t('gallery.uncategorized', 'Uncategorized')}</option>
{categories.map(cat => (
<option key={cat.id} value={cat.id}>
{cat.name}
@@ -0,0 +1,84 @@
/**
* The category filter's wire values (#1211).
*
* "Uncategorized" was rendered as `value="0"`, and the backend skips `'0'`
* outright (`adminPhotos.js`: `category_id !== '0'`), so the filter applied no
* condition and returned the whole event. The value it does understand is the
* literal `uncategorized`, four lines below that guard, which nothing sent.
*
* The failure was silent — a full list reads as "nothing to narrow" rather
* than "the filter did not run" — so this pins the wire value rather than the
* rendered label. Reported in #1209 by someone trying to isolate a few
* thousand uncategorised imports to re-assign them.
*/
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { describe, expect, it, vi } from 'vitest';
import { PhotoFilters } from '../PhotoFilters';
vi.mock('react-i18next', async () => {
const actual = await vi.importActual<typeof import('react-i18next')>('react-i18next');
return {
...actual,
useTranslation: () => ({
t: (_key: string, fallback?: any) => (typeof fallback === 'string' ? fallback : _key),
i18n: { language: 'en' }
})
};
});
const categories = [
{ id: 3, name: 'Ceremony' },
{ id: 4, name: 'Reception' },
];
const renderFilters = (selectedCategory: number | string | null = null) => {
const onCategoryChange = vi.fn();
render(
<PhotoFilters
selectedCategory={selectedCategory}
categories={categories as any}
onCategoryChange={onCategoryChange}
searchTerm=""
onSearchChange={vi.fn()}
/>
);
return { onCategoryChange };
};
const categorySelect = () => screen.getAllByRole('combobox')[0];
describe('category filter wire values (#1211)', () => {
it('sends the literal the backend understands for Uncategorized', async () => {
const { onCategoryChange } = renderFilters();
await userEvent.selectOptions(categorySelect(), 'uncategorized');
// Not 0 — the backend drops that and returns everything.
expect(onCategoryChange).toHaveBeenCalledWith('uncategorized');
});
it('still sends a numeric id for a real category', async () => {
const { onCategoryChange } = renderFilters();
await userEvent.selectOptions(categorySelect(), '3');
expect(onCategoryChange).toHaveBeenCalledWith(3);
});
it('clears back to null for All Categories', async () => {
const { onCategoryChange } = renderFilters('uncategorized');
await userEvent.selectOptions(categorySelect(), '');
expect(onCategoryChange).toHaveBeenCalledWith(null);
});
it('keeps Uncategorized selected once it is chosen', () => {
renderFilters('uncategorized');
// The select is controlled; if the option value and the state value ever
// drift apart the control silently falls back to the first option.
expect((categorySelect() as HTMLSelectElement).value).toBe('uncategorized');
});
});