diff --git a/backend/__tests__/integration/facePrivacy.test.js b/backend/__tests__/integration/facePrivacy.test.js index d0a97a25..43c08171 100644 --- a/backend/__tests__/integration/facePrivacy.test.js +++ b/backend/__tests__/integration/facePrivacy.test.js @@ -139,6 +139,82 @@ describe('face privacy and visibility (#1074)', () => { expect(guestPerson.cover.photo_id).not.toBe(hidden.photoId); }); + it('prefers the cover the photographer chose (#1096)', async () => { + const eventId = await seedEvent('chosen-cover'); + // The auto-pick would take the 0.99 face. The photographer picked the + // other one — without this the PATCH saved, the toast said so, and the + // avatar reverted on the very next read. + const best = await addPhotoWithFace(eventId, makeEmbedding(9, 0), { score: 0.99 }); + const chosen = await addPhotoWithFace(eventId, makeEmbedding(9, 1), { score: 0.70 }); + await clustering.assignFaces(eventId, [best.face, chosen.face]); + + const [before] = await peopleService.listPeople(eventId, { isClient: true, minClusterSize: 1 }); + expect(before.cover.photo_id).toBe(best.photoId); + // Clustering must not have written one: an automatic seed here would be + // indistinguishable from a real choice the moment listPeople honours it. + const seeded = await db('event_people').where({ id: before.id }).first(); + expect(seeded.cover_face_id).toBeFalsy(); + + await db('event_people').where({ id: before.id }).update({ cover_face_id: chosen.face.id }); + + const [after] = await peopleService.listPeople(eventId, { isClient: true, minClusterSize: 1 }); + expect(after.cover.photo_id).toBe(chosen.photoId); + }); + + it('carries a chosen cover through a merge', async () => { + const eventId = await seedEvent('cover-merge'); + const a = await addPhotoWithFace(eventId, makeEmbedding(20, 0), { score: 0.90 }); + const b = await addPhotoWithFace(eventId, makeEmbedding(60, 0), { score: 0.95 }); + await clustering.assignFaces(eventId, [a.face]); + await clustering.assignFaces(eventId, [b.face]); + const people = await db('event_people').where({ event_id: eventId }).orderBy('id'); + expect(people.length).toBeGreaterThan(1); + + // The SOURCE carries the choice; the target has none. + await db('event_people').where({ id: people[1].id }).update({ cover_face_id: b.face.id }); + await clustering.mergePeople(eventId, [people[1].id], people[0].id); + + const target = await db('event_people').where({ id: people[0].id }).first(); + expect(target.cover_face_id).toBe(b.face.id); + }); + + it('carries a chosen cover through a recluster', async () => { + const eventId = await seedEvent('cover-recluster'); + const faces = []; + for (let v = 0; v < 3; v++) { + faces.push((await addPhotoWithFace(eventId, makeEmbedding(21, v), { score: 0.9 - v * 0.1 })).face); + } + await clustering.assignFaces(eventId, faces); + const [person] = await db('event_people').where({ event_id: eventId }); + // Pick the WORST-scoring face, so an automatic re-pick would differ. + const chosen = faces[2].id; + await db('event_people').where({ id: person.id }).update({ cover_face_id: chosen }); + + await clustering.recluster(eventId); + + const after = await db('event_people').where({ event_id: eventId }).whereNotNull('cover_face_id'); + expect(after).toHaveLength(1); + expect(after[0].cover_face_id).toBe(chosen); + }); + + it('falls back to a visible face when the chosen cover is hidden from this audience', async () => { + const eventId = await seedEvent('chosen-cover-hidden'); + // Choosing a cover must never override the visibility scoping — that + // would hand a guest a crop of a photo they cannot open. + const hidden = await addPhotoWithFace(eventId, makeEmbedding(10, 0), { + visibility: 'hidden', score: 0.99, + }); + const visible = await addPhotoWithFace(eventId, makeEmbedding(10, 1), { score: 0.70 }); + await clustering.assignFaces(eventId, [hidden.face, visible.face]); + + const [person] = await peopleService.listPeople(eventId, { isClient: true, minClusterSize: 1 }); + await db('event_people').where({ id: person.id }).update({ cover_face_id: hidden.face.id }); + + const [guestView] = await peopleService.listPeople(eventId, { isClient: false, minClusterSize: 1 }); + expect(guestView.cover.photo_id).toBe(visible.photoId); + expect(guestView.cover.photo_id).not.toBe(hidden.photoId); + }); + it('drops a person entirely when all their photos are hidden', async () => { const eventId = await seedEvent('all-hidden'); const faces = []; diff --git a/backend/__tests__/personFacesQuery.test.js b/backend/__tests__/personFacesQuery.test.js new file mode 100644 index 00000000..1b521a77 --- /dev/null +++ b/backend/__tests__/personFacesQuery.test.js @@ -0,0 +1,63 @@ +/** + * The person-faces query must table-qualify its WHERE (#1096). + * + * `photo_faces` and `photos` BOTH have an event_id, so the moment the join was + * added a bare `where({ event_id })` became ambiguous. Postgres refuses it — + * + * column reference "event_id" is ambiguous + * + * — and the endpoint 500s, which took the Split dialog down with it on every + * PostgreSQL install. SQLite resolves the ambiguity silently, which is why the + * suite stayed green and this reached production. + * + * Two deliberate choices about HOW this is tested: + * + * 1. It imports the builder the route actually calls. An earlier version of + * this file re-declared the query locally, which meant the route could + * regress to the bare form while these assertions kept passing — a test + * that documents a bug without guarding it. + * 2. It asserts on the emitted SQL rather than executing it. A round-trip test + * would run against the SQLite the suite uses and prove nothing about the + * engine the bug affects. + */ + +process.env.NODE_ENV = 'test'; + +const knex = require('knex')({ client: 'pg' }); +const { buildPersonFacesQuery, PERSON_FACES_LIMIT } = require('../src/routes/adminEvents/faces'); + +const sql = () => buildPersonFacesQuery(knex, 857, 143).toString(); + +describe('person faces query', () => { + it('is the query the route runs, not a copy of it', () => { + expect(typeof buildPersonFacesQuery).toBe('function'); + expect(sql()).toContain('from "photo_faces"'); + }); + + it('qualifies event_id with its table', () => { + // The bare form is what Postgres rejects. + expect(sql()).toContain('"photo_faces"."event_id"'); + expect(sql()).not.toMatch(/where\s+"event_id"/i); + }); + + it('qualifies person_id too, so the join cannot shadow it either', () => { + expect(sql()).toContain('"photo_faces"."person_id"'); + expect(sql()).not.toMatch(/and\s+"person_id"\s*=/i); + }); + + it('still joins photos for the original dimensions', () => { + // The dimensions are what faceCropStyle scales the bbox against; without + // the join the crop maths has nothing to work from. + const s = sql(); + expect(s).toContain('inner join "photos"'); + expect(s).toContain('"photos"."width"'); + expect(s).toContain('"photos"."height"'); + }); + + it('caps the list at the limit the UI is told about', () => { + // The viewer reports truncation using this same number; if they drift, it + // silently claims a person has fewer appearances than they do. + expect(PERSON_FACES_LIMIT).toBe(500); + expect(sql()).toContain(`limit ${PERSON_FACES_LIMIT}`); + }); +}); diff --git a/backend/migrations/core/179_person_cover_manual_only.js b/backend/migrations/core/179_person_cover_manual_only.js new file mode 100644 index 00000000..71fb0291 --- /dev/null +++ b/backend/migrations/core/179_person_cover_manual_only.js @@ -0,0 +1,34 @@ +/** + * event_people.cover_face_id now means one thing: the photographer picked this + * face (#1096). + * + * It used to mean two things at once. `assignFaces` seeded it with whichever + * face happened to open the cluster, and `recomputeCentroid` overwrote it with + * the highest-scoring one — so the column held an automatic guess that was + * indistinguishable from a deliberate choice. The moment the cover picker + * started honouring it, every uncurated person would have had its avatar + * pinned to that guess, which is worse than the score-ordered fallback it + * replaced: the fallback is computed per audience and skips photos a guest + * cannot open. + * + * Both writers are gone. The automatic cover is derived at read time in + * facePeopleService.listPeople, where the visibility scoping already lives. + * + * Clearing every existing value is safe rather than destructive: no install has + * ever been able to SET a cover deliberately — the UI for it ships in the same + * change as this migration — so every stored value is an automatic guess by + * construction. Keeping them would silently promote guesses to choices. + */ + +exports.up = async function up(knex) { + if (!(await knex.schema.hasTable('event_people'))) return; + if (!(await knex.schema.hasColumn('event_people', 'cover_face_id'))) return; + + await knex('event_people').update({ cover_face_id: null }); +}; + +exports.down = async function down() { + // Irreversible by design, and nothing is lost: the values this cleared were + // derivable guesses, and listPeople regenerates that answer on every read. + // Restoring them would mean re-inventing a number, not recovering one. +}; diff --git a/backend/src/routes/adminEvents/faces.js b/backend/src/routes/adminEvents/faces.js index d1ddfb09..e594331d 100644 --- a/backend/src/routes/adminEvents/faces.js +++ b/backend/src/routes/adminEvents/faces.js @@ -25,6 +25,40 @@ const faceClient = require('../../services/faceClient'); const requireFaces = requireFeatureFlag('faces', 'FACES_DISABLED'); +/** Cap on one person's face list. Surfaced so the UI can say when it bites. */ +const PERSON_FACES_LIMIT = 500; + +/** + * Every face of one person, newest-confidence first, with the ORIGINAL photo + * dimensions the bbox was measured against. + * + * Exported so a test can assert the shape of the SQL this actually emits. It + * has to stay TABLE-QUALIFIED: `photos` also has an event_id, so the bare form + * became ambiguous the moment the join was added — Postgres refuses it + * ("column reference event_id is ambiguous") and the endpoint 500s, which took + * the Split dialog down with it on every PG install. SQLite resolves the + * ambiguity silently, which is why the suite stayed green and it shipped. + */ +function buildPersonFacesQuery(dbi, eventId, personId) { + return dbi('photo_faces') + .where({ + 'photo_faces.event_id': eventId, + 'photo_faces.person_id': personId, + }) + .orderBy('det_score', 'desc') + .limit(PERSON_FACES_LIMIT) + .join('photos', 'photos.id', 'photo_faces.photo_id') + .select( + 'photo_faces.id', 'photo_faces.photo_id', + 'photo_faces.bbox_x', 'photo_faces.bbox_y', + 'photo_faces.bbox_w', 'photo_faces.bbox_h', + 'photo_faces.det_score', 'photo_faces.blur', + // Needed to crop the box — it is in original-image pixels. + 'photos.width as photo_width', 'photos.height as photo_height' + ); +} + + async function loadOwnedEvent(req) { let q = db('events').where('id', req.params.id); if (req.admin.roleName === 'editor') { @@ -274,19 +308,7 @@ module.exports = (router) => { const event = await loadOwnedEvent(req); if (!event) return res.status(404).json({ error: 'Event not found' }); - const faces = await db('photo_faces') - .where({ event_id: event.id, person_id: req.params.personId }) - .orderBy('det_score', 'desc') - .limit(500) - .join('photos', 'photos.id', 'photo_faces.photo_id') - .select( - 'photo_faces.id', 'photo_faces.photo_id', - 'photo_faces.bbox_x', 'photo_faces.bbox_y', - 'photo_faces.bbox_w', 'photo_faces.bbox_h', - 'photo_faces.det_score', 'photo_faces.blur', - // Needed to crop the box — it is in original-image pixels. - 'photos.width as photo_width', 'photos.height as photo_height' - ); + const faces = await buildPersonFacesQuery(db, event.id, req.params.personId); res.json({ faces: faces.map((f) => ({ @@ -473,3 +495,6 @@ module.exports = (router) => { }); }; + +module.exports.buildPersonFacesQuery = buildPersonFacesQuery; +module.exports.PERSON_FACES_LIMIT = PERSON_FACES_LIMIT; diff --git a/backend/src/services/faceClustering.js b/backend/src/services/faceClustering.js index 36ced23e..5f10997f 100644 --- a/backend/src/services/faceClustering.js +++ b/backend/src/services/faceClustering.js @@ -208,7 +208,13 @@ async function assignFacesLocked(eventId, faceRows, thresholds, trx) { centroid: packEmbedding(embedding), face_count_total: 1, model_version: face.model_version, - cover_face_id: face.id, + // cover_face_id is deliberately NOT set (#1096). It means one thing + // now — the photographer picked this face — and seeding it with + // whichever face happened to start the cluster made that + // indistinguishable from a real choice. listPeople derives the + // automatic cover at read time instead, which it has to anyway: the + // best face for an ADMIN may sit in a photo a guest cannot open. + cover_face_id: null, created_at: new Date().toISOString(), updated_at: new Date().toISOString(), }).returning('id'); @@ -309,7 +315,8 @@ async function mergePeople(eventId, sourceIds, targetId) { // person they had suppressed — the target keeps its own values where it // has them, and inherits from a source only where it does not. const target = await trx('event_people').where({ id: targetId }).first(); - const sources = await trx('event_people').whereIn('id', ids).select('label', 'is_hidden', 'is_ignored'); + const sources = await trx('event_people').whereIn('id', ids) + .select('label', 'is_hidden', 'is_ignored', 'cover_face_id'); const inherited = {}; if (target && !target.label) { @@ -319,6 +326,13 @@ async function mergePeople(eventId, sourceIds, targetId) { // Suppression is one-way on merge: if ANY party was hidden or ignored, // the survivor stays that way. Re-exposing someone by merging is the // failure that matters; leaving them hidden is trivially reversible. + // A chosen cover is human state like the label, and the merged face is + // still in the cluster afterwards — so it survives the merge rather than + // reverting to the automatic pick (#1096). The target's own choice wins. + if (target && !target.cover_face_id) { + const curated = sources.find((p) => p.cover_face_id); + if (curated) inherited.cover_face_id = curated.cover_face_id; + } if (sources.some((p) => p.is_hidden) || target?.is_hidden) inherited.is_hidden = true; if (sources.some((p) => p.is_ignored) || target?.is_ignored) inherited.is_ignored = true; @@ -395,15 +409,13 @@ async function recomputeCentroid(personId, trx = db) { } for (let i = 0; i < mean.length; i++) mean[i] /= vectors.length; - // Cover face: the highest-scoring detection in the cluster, so the avatar - // is the sharpest available crop of that person rather than whichever face - // happened to arrive first. - const cover = faces.reduce((a, b) => ((b.det_score ?? 0) > (a.det_score ?? 0) ? b : a)); - + // cover_face_id is left alone (#1096): it holds the photographer's pick and + // nothing else, so a rescan or a merge must not touch it. If the chosen face + // is gone the id simply dangles, and listPeople falls back to the derived + // cover on the next read — self-healing, no reassociation needed. await trx('event_people').where({ id: personId }).update({ centroid: packEmbedding(normalize(mean)), face_count_total: faces.length, - cover_face_id: cover.id, updated_at: new Date().toISOString(), }); } @@ -427,9 +439,15 @@ async function recluster(eventId) { const previousLabels = await db('event_people') .where({ event_id: eventId }) .where(function () { - this.whereNotNull('label').orWhere('is_hidden', true).orWhere('is_ignored', true); + this.whereNotNull('label') + .orWhere('is_hidden', true) + .orWhere('is_ignored', true) + // A chosen cover is human state too, and re-grouping used to drop it + // (#1096). Faces keep their ids across a recluster, so the choice can + // follow its FACE into whichever new cluster ends up holding it. + .orWhereNotNull('cover_face_id'); }) - .select('id', 'label', 'is_hidden', 'is_ignored'); + .select('id', 'label', 'is_hidden', 'is_ignored', 'cover_face_id'); const priorMembership = new Map(); if (previousLabels.length) { @@ -501,8 +519,21 @@ async function recluster(eventId) { if (old?.label && !labelFor.has(newId)) labelFor.set(newId, old.label); } - for (const [newId, flags] of suppression) { + // The chosen COVER follows its face, not the majority: the point of the + // choice is that specific photo, so it belongs to whichever cluster now + // holds it — which is not always the one that inherited the most faces. + const coverFor = new Map(); // newId -> faceId + const newPersonOfFace = new Map(assignments.map((a) => [a.faceId, a.personId])); + for (const old of previousLabels) { + if (!old.cover_face_id) continue; + const newId = newPersonOfFace.get(old.cover_face_id); + if (newId != null && !coverFor.has(newId)) coverFor.set(newId, old.cover_face_id); + } + + for (const newId of new Set([...suppression.keys(), ...coverFor.keys()])) { + const flags = suppression.get(newId) || {}; const update = { ...flags, updated_at: new Date().toISOString() }; + if (coverFor.has(newId)) update.cover_face_id = coverFor.get(newId); if (labelFor.has(newId)) update.label = labelFor.get(newId); await db('event_people').where({ id: newId }).update(update); } diff --git a/backend/src/services/facePeopleService.js b/backend/src/services/facePeopleService.js index 414205e7..2d3bf1e9 100644 --- a/backend/src/services/facePeopleService.js +++ b/backend/src/services/facePeopleService.js @@ -100,11 +100,28 @@ async function listPeople(eventId, { isClient = false, forAdmin = false, minClus 'photos.height as photo_height' ); + // The photographer's explicit pick wins whenever this audience can see it + // (#1096). Without this the stored cover_face_id was loaded and discarded — + // the picker saved, said so, and the avatar reverted to the best-scoring + // face on the very next read, making the whole feature a no-op. + // + // The score-ordered fallback still stands for everything it always covered: + // a person who has never been curated, and a chosen cover that sits in a + // photo this audience is not allowed to see. + const chosenByPerson = new Map( + people.filter((p) => p.cover_face_id).map((p) => [p.id, p.cover_face_id]) + ); + const visibleById = new Map(covers.map((row) => [row.id, row])); + const coverByPerson = new Map(); for (const row of covers) { // Rows arrive best-score-first, so the first hit per person wins. if (!coverByPerson.has(row.person_id)) coverByPerson.set(row.person_id, row); } + for (const [personId, faceId] of chosenByPerson) { + const chosen = visibleById.get(faceId); + if (chosen && chosen.person_id === personId) coverByPerson.set(personId, chosen); + } const out = []; for (const person of people) { diff --git a/frontend/src/components/admin/PeopleManagerModal.tsx b/frontend/src/components/admin/PeopleManagerModal.tsx index 064e1b72..0de4cf72 100644 --- a/frontend/src/components/admin/PeopleManagerModal.tsx +++ b/frontend/src/components/admin/PeopleManagerModal.tsx @@ -14,16 +14,21 @@ * the control. */ import React, { useMemo, useState } from 'react'; -import { useQuery } from '@tanstack/react-query'; +import { useQuery, useQueryClient } from '@tanstack/react-query'; import { useTranslation } from 'react-i18next'; import { toast } from 'react-toastify'; -import { X, Check, Merge, Scissors, EyeOff, Ban, Loader2 } from 'lucide-react'; +import { X, Check, Merge, Scissors, EyeOff, Ban, Loader2, Image as ImageIcon } from 'lucide-react'; import { Button, Loading } from '../common'; import { api } from '../../config/api'; import { faceCropStyle } from '../gallery/faceCrop'; import { adminFacePreviewUrl } from '../gallery/imageTiers'; +/** Mirrors PERSON_FACES_LIMIT in adminEvents/faces.js. The endpoint caps a + * person's face list, so any surface that renders it has to admit when it is + * showing a truncated one rather than implying it is everything. */ +const PERSON_FACES_LIMIT = 500; + interface AdminPerson { id: number; label: string | null; @@ -109,11 +114,16 @@ export const PeopleManagerModal: React.FC = ({ eventId, open, onClose, onChanged, }) => { const { t } = useTranslation(); + const queryClient = useQueryClient(); const [selected, setSelected] = useState([]); const [renaming, setRenaming] = useState(null); const [draftLabel, setDraftLabel] = useState(''); const [splitting, setSplitting] = useState(null); const [splitFaceIds, setSplitFaceIds] = useState([]); + // Cover picker (#1096). Clustering picks the cover, and its idea of a good + // one and a human's do not always agree — a cluster whose avatar is softer + // or turned away stays that way in the guest-facing strip too. + const [coverFor, setCoverFor] = useState(null); const [busy, setBusy] = useState(false); const { data, isLoading, refetch } = useQuery<{ people: AdminPerson[] }>({ @@ -122,16 +132,25 @@ export const PeopleManagerModal: React.FC = ({ enabled: open, }); + // Both pickers browse the same face list; only the action on a click + // differs. Keyed by person id so switching between them reuses the cache. + const facesFor = splitting || coverFor; + const { data: faceData, isLoading: facesLoading } = useQuery<{ faces: PersonFace[] }>({ - queryKey: ['admin-person-faces', eventId, splitting?.id], + queryKey: ['admin-person-faces', eventId, facesFor?.id], queryFn: async () => - (await api.get(`/admin/events/${eventId}/people/${splitting!.id}/faces`)).data, - enabled: !!splitting, + (await api.get(`/admin/events/${eventId}/people/${facesFor!.id}/faces`)).data, + enabled: !!facesFor, }); const people = useMemo(() => data?.people || [], [data]); const after = async (message: string) => { + // The face list is cached per person and split/merge move faces between + // them. Until the cover picker landed, the only reader closed itself after + // acting so nobody saw the stale copy; now a second surface reads the same + // key and would offer faces that are no longer this person's. + await queryClient.invalidateQueries({ queryKey: ['admin-person-faces'] }); await refetch(); onChanged?.(); setSelected([]); @@ -194,6 +213,18 @@ export const PeopleManagerModal: React.FC = ({ ); }; + const chooseCover = (faceId: number) => { + if (!coverFor) return; + const personId = coverFor.id; + setCoverFor(null); + run( + async () => { + await api.patch(`/admin/events/${eventId}/people/${personId}`, { cover_face_id: faceId }); + }, + t('admin.people.coverSet', { defaultValue: 'Cover updated' }) + ); + }; + const setFlag = (person: AdminPerson, field: 'is_hidden' | 'is_ignored', value: boolean) => run( async () => { @@ -229,8 +260,66 @@ export const PeopleManagerModal: React.FC = ({ - {/* --- split picker ------------------------------------------------ */} - {splitting ? ( + {/* --- cover picker ------------------------------------------------ */} + {coverFor ? ( + <> +
+ {t('admin.people.coverHelp', { + defaultValue: 'Pick the photo that best shows this person. It becomes their avatar here and in the guest-facing people strip.', + })} + {(faceData?.faces?.length || 0) >= Math.min( + PERSON_FACES_LIMIT, coverFor.total_face_count ?? PERSON_FACES_LIMIT + ) && (coverFor.total_face_count ?? 0) > PERSON_FACES_LIMIT && ( + + {t('admin.people.coverTruncated', { + limit: PERSON_FACES_LIMIT, + defaultValue: `Showing the ${PERSON_FACES_LIMIT} highest-confidence faces of this person.`, + })} + + )} +
+
+ {facesLoading ? : ( +
+ {(faceData?.faces || []).map((face) => { + const current = coverFor.cover?.face_id === face.id; + return ( + + ); + })} +
+ )} +
+
+ +
+ + ) : splitting ? ( <>
{t('admin.people.splitHelp', { @@ -362,6 +451,15 @@ export const PeopleManagerModal: React.FC = ({
+