diff --git a/.env.example b/.env.example index ac4cac87..8dcf17c1 100644 --- a/.env.example +++ b/.env.example @@ -10,6 +10,13 @@ NODE_ENV=production # Generate one with: openssl rand -base64 64 #JWT_SECRET=your_very_long_random_jwt_secret_here +# How long a gallery guest stays recognised (#1210). Default 30d. It was 24h, +# which meant a client reviewing a gallery across two weekends registered again +# in between — and each re-registration is a separate guest whose likes and +# favourites no longer join up with the first visit's. Takes any jsonwebtoken +# duration ('7d', '12h'); shorten it if your galleries hold sensitive work. +#GUEST_TOKEN_TTL=30d + # OIDC SSO for admins (#798) — configured in the admin UI; only these two # values live in the environment: # Key encrypting the OIDC client secret at rest (defaults to JWT_SECRET). diff --git a/backend/__tests__/integration/guestDuplicateDetection.test.js b/backend/__tests__/integration/guestDuplicateDetection.test.js new file mode 100644 index 00000000..dcdbe53f --- /dev/null +++ b/backend/__tests__/integration/guestDuplicateDetection.test.js @@ -0,0 +1,229 @@ +/** + * Spotting one person registered twice (#1210). + * + * Guest registration always inserts. A client whose token expired, or who + * opens the gallery on a second device, becomes a new gallery_guests row and + * their likes and favourites split across the copies — so the photographer's + * "final selection" is only trustworthy after someone notices two Tinas with + * half the picks each and merges them. + * + * Merging already worked. This is the half that was missing: saying which rows + * are the same person, so the admin does not have to find them by eye. + * + * Detection only — the registration path is deliberately untouched. Reusing a + * row because someone typed a matching email would let anyone who knows that + * email inherit the identity and its selections, and answering differently for + * a known email would leak which addresses are in the gallery, which is + * exactly what /guest/recover goes out of its way to avoid. + */ + +const request = require('supertest'); +const express = require('express'); + +const { bootCrmDb, seedMinimal } = require('./helpers/crmDb'); + +describe('duplicate guest detection (#1210)', () => { + let db; let cleanup; let app; let eventId; + + const listGuests = async () => { + const res = await request(app).get(`/api/admin/events/${eventId}/guests`); + expect(res.status).toBe(200); + return res.body; + }; + + const addGuest = async (name, email, extra = {}) => { + const [g] = await db('gallery_guests').insert({ + event_id: eventId, name, email, + identifier: `id-${name}-${Math.random()}`, + created_at: new Date().toISOString(), + last_seen_at: new Date().toISOString(), + is_deleted: false, + ...extra, + }).returning('id'); + return typeof g === 'object' ? g.id : g; + }; + + const byId = (body, id) => body.guests.find((g) => g.id === id); + + 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: 'dupe-guests', event_type: 'wedding', event_name: 'Dupe Guests', + event_date: '2026-08-01', host_email: 'h@example.com', admin_email: 'a@example.com', + password_hash: 'x', share_link: '/gallery/dupe-guests/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; + + app = express(); + app.use(express.json()); + app.use('/api/admin', require('../../src/routes/adminGuests')); + }, 180000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + beforeEach(async () => { + await db('gallery_guests').where({ event_id: eventId }).del(); + }); + + it('groups duplicates under a shared key', async () => { + const first = await addGuest('Tina', 'tina@example.com'); + const second = await addGuest('Tina', 'tina@example.com'); + const other = await addGuest('Marc', 'marc@example.com'); + + const body = await listGuests(); + + // A shared group key rather than a list of sibling ids: the payload stays + // linear in the number of guests, and the case/whitespace folding lives in + // one place instead of being reimplemented on the client. + expect(byId(body, first).duplicate_group).toBe('tina@example.com'); + expect(byId(body, second).duplicate_group).toBe('tina@example.com'); + expect(byId(body, other).duplicate_group).toBeNull(); + }); + + it('counts the groups and the rows in them', async () => { + await addGuest('Tina', 'tina@example.com'); + await addGuest('Tina', 'tina@example.com'); + await addGuest('Ben', 'ben@example.com'); + await addGuest('Ben', 'ben@example.com'); + await addGuest('Ben', 'ben@example.com'); + await addGuest('Marc', 'marc@example.com'); + + expect((await listGuests()).duplicates).toEqual({ groups: 2, guests: 5 }); + }); + + it('matches the same address typed with different capitals or a stray space', async () => { + // The same person on a different day. Both read as distinct rows in the + // admin list, which is precisely why they need catching here. + const a = await addGuest('Tina', 'tina@example.com'); + const b = await addGuest('Tina', 'Tina@Example.com '); + + const body = await listGuests(); + expect(byId(body, a).duplicate_group).toBe('tina@example.com'); + expect(byId(body, b).duplicate_group).toBe('tina@example.com'); + expect(body.duplicates).toEqual({ groups: 1, guests: 2 }); + }); + + it('does not treat two guests without an email as the same person', async () => { + // require_name_email is off by default, so a shared gallery link produces + // plenty of these. Grouping them would merge strangers. + const a = await addGuest('Anon', null); + const b = await addGuest('Anon', null); + + const body = await listGuests(); + expect(byId(body, a).duplicate_group).toBeNull(); + expect(byId(body, b).duplicate_group).toBeNull(); + expect(body.duplicates).toEqual({ groups: 0, guests: 0 }); + }); + + it('does not treat a shared name as evidence of anything', async () => { + const a = await addGuest('Anna', 'anna.k@example.com'); + const b = await addGuest('Anna', 'anna.m@example.com'); + + const body = await listGuests(); + expect(byId(body, a).duplicate_group).toBeNull(); + expect(byId(body, b).duplicate_group).toBeNull(); + }); + + it('moves a pending invite to the survivor instead of stranding it', async () => { + // Creating an invite inserts a real gallery_guests row, so an admin who + // pre-mints one and then sees the guest self-register has two rows — and + // this feature now points that pair out and offers the merge. Redemption + // resolves guest_invites.guest_id with `is_deleted: false`, so merging + // without moving the invite leaves the emailed link returning 404 + // guest_missing while the invite dialog still shows it as Pending. + const placeholder = await addGuest('Tina', 'tina@example.com'); + const selfRegistered = await addGuest('Tina', 'tina@example.com'); + const [inv] = await db('guest_invites').insert({ + event_id: eventId, guest_id: placeholder, token: 'invite-token-1', + created_by_admin_id: 1, created_at: new Date().toISOString(), + }).returning('id'); + const inviteId = typeof inv === 'object' ? inv.id : inv; + + const res = await request(app) + .post(`/api/admin/events/${eventId}/guests/${selfRegistered}/merge`) + .send({ mergeIds: [placeholder] }); + expect(res.status).toBe(200); + + const invite = await db('guest_invites').where({ id: inviteId }).first(); + expect(invite.guest_id).toBe(selfRegistered); + // And it still resolves: the survivor is not soft-deleted. + const target = await db('gallery_guests').where({ id: invite.guest_id }).first(); + expect(Boolean(target.is_deleted)).toBe(false); + }); + + it('leaves a spent invite pointing at what it actually redeemed', async () => { + // A redeemed invite is a record of who redeemed what. Retargeting it would + // rewrite that history to name a guest who was never on the other end. + const old = await addGuest('Tina', 'tina@example.com'); + const kept = await addGuest('Tina', 'tina@example.com'); + const [inv] = await db('guest_invites').insert({ + event_id: eventId, guest_id: old, token: 'invite-token-2', + created_by_admin_id: 1, redeemed_at: new Date().toISOString(), + created_at: new Date().toISOString(), + }).returning('id'); + const inviteId = typeof inv === 'object' ? inv.id : inv; + + await request(app) + .post(`/api/admin/events/${eventId}/guests/${kept}/merge`) + .send({ mergeIds: [old] }); + + expect((await db('guest_invites').where({ id: inviteId }).first()).guest_id).toBe(old); + }); + + it('canonicalises the survivor\'s address so recovery can still find them', async () => { + // Grouping folds case and whitespace, so a merge can be proposed between a + // clean address and a legacy one that is not. /guest/recover lowercases + // what the guest types and matches on equality, so a survivor left holding + // the raw value becomes permanently unrecoverable by email. + const legacy = await addGuest('Tina', 'Tina@Example.com '); + const other = await addGuest('Tina', 'tina@example.com'); + + const res = await request(app) + .post(`/api/admin/events/${eventId}/guests/${legacy}/merge`) + .send({ mergeIds: [other] }); + expect(res.status).toBe(200); + + expect((await db('gallery_guests').where({ id: legacy }).first()).email).toBe('tina@example.com'); + }); + + it('leaves an already-canonical survivor untouched', async () => { + const kept = await addGuest('Tina', 'tina@example.com'); + const dupe = await addGuest('Tina', 'Tina@Example.com'); + + await request(app) + .post(`/api/admin/events/${eventId}/guests/${kept}/merge`) + .send({ mergeIds: [dupe] }); + + expect((await db('gallery_guests').where({ id: kept }).first()).email).toBe('tina@example.com'); + }); + + it('ignores a removed guest', async () => { + const kept = await addGuest('Tina', 'tina@example.com'); + await addGuest('Tina', 'tina@example.com', { is_deleted: true }); + + const body = await listGuests(); + // The deleted row is not listed at all, so the survivor is not a duplicate + // of something the admin cannot see or merge. + expect(body.guests.map((g) => g.id)).toEqual([kept]); + expect(byId(body, kept).duplicate_group).toBeNull(); + expect(body.duplicates).toEqual({ groups: 0, guests: 0 }); + }); +}); diff --git a/backend/__tests__/routes/guestTokenTtl.test.js b/backend/__tests__/routes/guestTokenTtl.test.js new file mode 100644 index 00000000..70c10c51 --- /dev/null +++ b/backend/__tests__/routes/guestTokenTtl.test.js @@ -0,0 +1,51 @@ +/** + * How long a guest stays recognised (#1210). + * + * The expiry was 24h and every call site took the default, so a client + * reviewing a gallery across two weekends lost their identity in between and + * registered again — and each re-registration is a fresh gallery_guests row + * whose likes and favourites no longer join up with the first visit's. + * + * Pinned as a test because the value is the whole fix: a silent revert to 24h + * would restore the duplicate churn without breaking anything visible. + */ + +const jwt = require('jsonwebtoken'); + +describe('guest token lifetime (#1210)', () => { + const load = (ttl) => { + jest.resetModules(); + process.env.JWT_SECRET = 'guest-ttl-secret'; + if (ttl === undefined) delete process.env.GUEST_TOKEN_TTL; + else process.env.GUEST_TOKEN_TTL = ttl; + return require('../../src/middleware/guestAuth'); + }; + + const lifetimeDays = (token) => { + const { iat, exp } = jwt.decode(token); + return Math.round((exp - iat) / 86400); + }; + + const sign = (mod) => mod.signGuestToken({ + guestId: 1, eventId: 2, identifier: 'abc', name: 'Tina', + }); + + afterAll(() => { delete process.env.GUEST_TOKEN_TTL; }); + + it('defaults to 30 days, not 24 hours', () => { + expect(lifetimeDays(sign(load(undefined)))).toBe(30); + }); + + it('honours an operator override', () => { + // The value goes straight to jsonwebtoken, so anything it accepts works; + // an operator who wants the old behaviour back can have it. + expect(lifetimeDays(sign(load('7d')))).toBe(7); + expect(lifetimeDays(sign(load('24h')))).toBe(1); + }); + + it('still signs a token the guest middleware accepts', () => { + const mod = load(undefined); + const decoded = jwt.verify(sign(mod), process.env.JWT_SECRET, { issuer: 'picpeak-auth' }); + expect(decoded).toMatchObject({ type: 'guest', guestId: 1, eventId: 2, identifier: 'abc' }); + }); +}); diff --git a/backend/src/middleware/guestAuth.js b/backend/src/middleware/guestAuth.js index a21eddb5..2a6c8617 100644 --- a/backend/src/middleware/guestAuth.js +++ b/backend/src/middleware/guestAuth.js @@ -79,10 +79,26 @@ function requireGuest(req, res, next) { } /** - * Sign a new guest JWT. Scoped to a specific event and guest row. - * Expiry matches the gallery token default (24h). + * How long a guest stays recognised (#1210). + * + * Was 24h, matching the gallery token, and every call site took that default — + * so a client reviewing a gallery across two weekends lost their identity in + * between and registered again. Each of those re-registrations is a fresh + * gallery_guests row, and their likes and favourites scatter across the + * duplicates until an admin merges them, which is the harm reported in #1210. + * + * A guest token is a weaker thing than an admin session: it is scoped to one + * event, carries no admin capability, and the gallery itself is already behind + * whatever password or share link protects it. 30 days is the shape of a real + * proofing cycle. Configurable for operators who want it shorter — the value + * goes straight to jsonwebtoken, so it takes any zeit/ms string ('7d', '12h'). */ -function signGuestToken({ guestId, eventId, identifier, name }, expiresIn = '24h') { +const GUEST_TOKEN_TTL = process.env.GUEST_TOKEN_TTL || '30d'; + +/** + * Sign a new guest JWT. Scoped to a specific event and guest row. + */ +function signGuestToken({ guestId, eventId, identifier, name }, expiresIn = GUEST_TOKEN_TTL) { return jwt.sign( { type: 'guest', diff --git a/backend/src/routes/adminGuests.js b/backend/src/routes/adminGuests.js index 320f0c55..82ab1a8d 100644 --- a/backend/src/routes/adminGuests.js +++ b/backend/src/routes/adminGuests.js @@ -88,19 +88,64 @@ router.get( ) .orderBy('gallery_guests.created_at', 'desc'); - const guests = rows.map((r) => ({ - ...serializeGuest(r), - stats: { - likes: parseInt(r.likes, 10) || 0, - favorites: parseInt(r.favorites, 10) || 0, - comments: parseInt(r.comments, 10) || 0, - ratings: parseInt(r.ratings, 10) || 0, - reactions: parseInt(r.reactions, 10) || 0, - distinct_photos: parseInt(r.distinct_photos, 10) || 0, - }, - })); + // Which rows are the same person registered more than once (#1210). + // + // Registration always inserts, so a returning client whose token has + // expired — or who opens the gallery on a second device — becomes a new + // guest, and their picks split across the copies. Merging those already + // works; nothing told the admin which rows to merge, so the split + // selection had to be spotted by eye before the "final" list could be + // trusted. + // + // Grouped in JS rather than a second grouped query: the list is one + // event's guests, and the rows are already in hand. Case-folded because + // the same person types Tina@ and tina@ on different days, and trimmed + // because a trailing space is invisible in the admin list — both would + // otherwise read as distinct people. Email is the only key used: two + // guests genuinely called "Anna" are not evidence of anything. + const byEmail = new Map(); + for (const r of rows) { + const key = (r.email || '').trim().toLowerCase(); + if (!key) continue; + if (!byEmail.has(key)) byEmail.set(key, []); + byEmail.get(key).push(r.id); + } - res.json({ guests }); + const guests = rows.map((r) => { + const key = (r.email || '').trim().toLowerCase(); + const sharing = key ? byEmail.get(key) || [] : []; + return { + ...serializeGuest(r), + // A group key, not the list of sibling ids (#1210 review). Listing + // the others meant every row carried the other n-1 ids, so a group of + // n registrations serialised n² ids — and nothing consumed them: the + // UI only asks whether a row is in a group and then regroups by this + // key anyway. Emitting the normalised email keeps the payload linear + // AND keeps the case/whitespace folding in one place instead of + // reimplemented on the client. + duplicate_group: sharing.length > 1 ? key : null, + stats: { + likes: parseInt(r.likes, 10) || 0, + favorites: parseInt(r.favorites, 10) || 0, + comments: parseInt(r.comments, 10) || 0, + ratings: parseInt(r.ratings, 10) || 0, + reactions: parseInt(r.reactions, 10) || 0, + distinct_photos: parseInt(r.distinct_photos, 10) || 0, + }, + }; + }); + + // One number for the banner, so the UI does not have to derive it and + // then disagree with the badges when the derivation drifts. + const duplicateGroups = [...byEmail.values()].filter((ids) => ids.length > 1); + + res.json({ + guests, + duplicates: { + groups: duplicateGroups.length, + guests: duplicateGroups.reduce((n, ids) => n + ids.length, 0), + }, + }); } catch (error) { errorResponse(res, error, 500, 'Failed to list guests'); } @@ -598,6 +643,38 @@ router.post( const result = await feedbackService.mergeGuestFeedback(Number(keepId), mergeIds.map(Number)); + // Canonicalise the survivor's address (#1210 review). Rows are grouped + // for review with the case and whitespace folded out, so a merge can be + // proposed between `tina@example.com` and `Tina@Example.com ` — and if + // the non-canonical one survives, guest recovery can never find it + // again: /guest/recover lowercases and trims what the guest types, then + // matches on equality (galleryGuests.js). Every write path normalises + // today, so this is for rows that predate that, which are exactly the + // rows case-folded grouping surfaces. + const survivor = all.find((g) => Number(g.id) === Number(keepId)); + if (survivor?.email) { + const canonical = String(survivor.email).trim().toLowerCase(); + if (canonical !== survivor.email) { + await db('gallery_guests').where({ id: Number(keepId) }).update({ email: canonical }); + } + } + + // Carry any unredeemed invite over to the survivor BEFORE the source row + // is soft-deleted (#1210 review). guest_invites.guest_id points at a real + // gallery_guests row — creating an invite inserts one — and redemption + // looks it up with `is_deleted: false`. Merging without this leaves the + // emailed link resolving to a deleted guest: the client gets a 404 + // `guest_missing` while the admin's invite dialog still shows the invite + // as Pending, so nothing anywhere says the link is dead. + // + // Only unredeemed, unrevoked invites move. A spent invite is a historical + // record of who redeemed what and retargeting it would rewrite that. + await db('guest_invites') + .whereIn('guest_id', mergeIds.map(Number)) + .whereNull('redeemed_at') + .whereNull('revoked_at') + .update({ guest_id: Number(keepId) }); + // Soft-delete the merged (source) guests. await db('gallery_guests') .whereIn('id', mergeIds.map(Number)) diff --git a/docker-compose.yml b/docker-compose.yml index 540db892..aaa8e699 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -45,6 +45,10 @@ services: - ADMIN_USERNAME=${ADMIN_USERNAME:-admin} - ADMIN_EMAIL=${ADMIN_EMAIL:-admin@example.com} - ADMIN_PASSWORD=${ADMIN_PASSWORD:-} + # How long a gallery guest stays recognised (#1210). Unset = 30d. + # Listed explicitly because this service takes an environment list, so a + # value in .env that is not named here never reaches the container. + - GUEST_TOKEN_TTL=${GUEST_TOKEN_TTL:-} - DATABASE_CLIENT=pg - DB_TYPE=postgresql - DB_HOST=postgres diff --git a/frontend/src/components/admin/AdminGuestsList.tsx b/frontend/src/components/admin/AdminGuestsList.tsx index b8178a95..16aa62eb 100644 --- a/frontend/src/components/admin/AdminGuestsList.tsx +++ b/frontend/src/components/admin/AdminGuestsList.tsx @@ -1,4 +1,4 @@ -import React, { useState } from 'react'; +import React, { useMemo, useState } from 'react'; import { useQuery } from '@tanstack/react-query'; import { useTranslation } from 'react-i18next'; import { Trash2, Eye, Download, UserPlus, Grid3x3, List } from 'lucide-react'; @@ -25,6 +25,8 @@ export const AdminGuestsList: React.FC = ({ eventId, event const [selectedGuest, setSelectedGuest] = useState(null); const [mergeMode, setMergeMode] = useState(false); const [mergeSelection, setMergeSelection] = useState([]); + // Which row absorbs the others. Never defaulted: see the grouping comment. + const [keepId, setKeepId] = useState(null); const inviteModal = useModal(); const { data, isLoading, refetch } = useQuery({ @@ -47,6 +49,7 @@ export const AdminGuestsList: React.FC = ({ eventId, event onSuccess: () => { setMergeMode(false); setMergeSelection([]); + setKeepId(null); }, errorMessage: () => t('admin.guests.mergedError', 'Failed to merge guests'), }); @@ -100,23 +103,58 @@ export const AdminGuestsList: React.FC = ({ eventId, event toast.warning(t('admin.guests.mergeSelectAtLeastTwo', 'Select at least 2 guests to merge')); return; } - const [keepId, ...mergeIds] = mergeSelection; - const keepName = data?.guests.find((g) => g.id === keepId)?.name; + if (keepId === null || !mergeSelection.includes(keepId)) { + toast.warning(t('admin.guests.mergePickKeep', 'Choose which entry to keep')); + return; + } + const mergeIds = mergeSelection.filter((id) => id !== keepId); + const keep = data?.guests.find((g) => g.id === keepId); + // Name plus email (#1210 review): duplicates are the same person, so the + // names are usually identical — "Merge 2 guests into Tina?" told the admin + // nothing about which Tina is about to absorb the other. + const keepLabel = keep + ? [keep.name, keep.email].filter(Boolean).join(' · ') + : `#${keepId}`; const confirmMsg = t( 'admin.guests.mergeConfirm', 'Merge {{count}} guests into {{name}}? This cannot be undone.', - { count: mergeSelection.length, name: keepName || '#' + keepId } + { count: mergeSelection.length, name: keepLabel } ); if (window.confirm(confirmMsg)) { mergeMutation.mutate({ keepId, mergeIds }); } }; + // Stable identity so the duplicate grouping below is not recomputed on + // every render by a fresh [] literal. + const guests = useMemo(() => data?.guests || [], [data?.guests]); + + // Derived from the rows the badges render, not from the API's summary count, + // so a banner saying "3 entries" can never sit above rows where only 2 are + // badged. The API returns the summary too; it is a cheap cross-check, not a + // second source of truth. + const duplicateGroups = useMemo(() => { + const byGroup = new Map(); + for (const g of guests) { + if (!g.duplicate_group) continue; + if (!byGroup.has(g.duplicate_group)) byGroup.set(g.duplicate_group, []); + byGroup.get(g.duplicate_group)!.push(g); + } + + // Deliberately NOT ordered to imply a survivor (#1210 review, three + // rounds on this one point). Every automatic rule was wrong somewhere: + // most-feedback is guest-controlled, and oldest-first keeps the row whose + // token expired while deleting the visitor's currently active identity — + // the exact shape of the common case. The data does not say which row is + // really the person, so the UI asks instead of guessing. + return [...byGroup.values()].filter((group) => group.length > 1); + }, [guests]); + const duplicateCount = duplicateGroups.reduce((n, group) => n + group.length, 0); + if (isLoading) { return ; } - const guests = data?.guests || []; if (view === 'aggregate') { return ( @@ -145,10 +183,15 @@ export const AdminGuestsList: React.FC = ({ eventId, event {t('admin.guests.mergeSelected', '{{count}} selected', { count: mergeSelection.length })} - - @@ -199,6 +242,40 @@ export const AdminGuestsList: React.FC = ({ eventId, event + {/* The one thing the admin could not see (#1210). Registration always + inserts, so a client returning after their token expired — or on a + second device — becomes another row and their picks split across the + copies. Merging was already here; knowing WHICH rows to merge was + not, and a split selection is invisible until someone notices two + "Tina"s with half the likes each. + + Preselects the group rather than merging for them: which row survives + decides which name and verification state the merged guest keeps, and + that is the admin's call, not a default. */} + {duplicateGroups.length > 0 && !mergeMode && ( +
+

+ {t('admin.guests.duplicatesFound', { + guests: duplicateCount, + groups: duplicateGroups.length, + defaultValue: '{{guests}} guest entries look like {{groups}} returning visitor(s) — same email, registered more than once. Their picks are split until they are merged.', + })} +

+ +
+ )} + {guests.length === 0 ? (
@@ -212,6 +289,11 @@ export const AdminGuestsList: React.FC = ({ eventId, event {mergeMode && } + {mergeMode && ( + + {t('admin.guests.mergeKeepColumn', 'Keep')} + + )} {t('admin.guests.columns.name', 'Name')} @@ -249,12 +331,28 @@ export const AdminGuestsList: React.FC = ({ eventId, event toggleMergeSelection(guest.id)} className="w-4 h-4 text-accent rounded focus:ring-primary-500" /> )} + {mergeMode && ( + + {/* The survivor, chosen rather than derived. Only + selectable among the rows actually being merged. */} + setKeepId(guest.id)} + className="w-4 h-4 text-accent focus:ring-primary-500 disabled:opacity-40" + /> + + )} {guest.name} {guest.email_verified_at && ( @@ -263,6 +361,14 @@ export const AdminGuestsList: React.FC = ({ eventId, event {guest.email || '—'} + {guest.duplicate_group && ( + + {t('admin.guests.duplicateBadge', 'duplicate?')} + + )} {guest.stats.likes} diff --git a/frontend/src/components/admin/__tests__/AdminGuestsList.duplicates.test.tsx b/frontend/src/components/admin/__tests__/AdminGuestsList.duplicates.test.tsx new file mode 100644 index 00000000..ee6a906c --- /dev/null +++ b/frontend/src/components/admin/__tests__/AdminGuestsList.duplicates.test.tsx @@ -0,0 +1,199 @@ +/** + * Surfacing duplicate guests in the admin list (#1210). + * + * Merging two rows into one already worked. What the admin had no way to see + * was WHICH rows were the same person — so a client who registered again after + * their token expired left their picks split across two entries, and the + * "final selection" was only trustworthy if someone happened to notice. + * + * The banner offers the group to the merge mode that already exists; it does + * not merge anything. Which row survives decides the name and verification + * state the merged guest keeps, and that is the admin's call. + */ +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { describe, expect, it, vi, beforeEach } from 'vitest'; +import type { ReactNode } from 'react'; + +import { AdminGuestsList } from '../AdminGuestsList'; + +const getEventGuests = vi.fn(); + +vi.mock('react-i18next', async () => { + const actual = await vi.importActual('react-i18next'); + return { + ...actual, + useTranslation: () => ({ + // Interpolates like the real i18n so aria-labels built from + // `t(key, 'Keep {{name}}', { name })` are queryable by their rendered text. + t: (_key: string, fallback?: any, opts?: any) => { + if (typeof fallback === 'string') { + if (!opts) return fallback; + return Object.entries(opts).reduce( + (acc, [k, v]) => acc.replaceAll(`{{${k}}}`, String(v)), + fallback, + ); + } + if (fallback && typeof fallback === 'object' && 'defaultValue' in fallback) { + return String(fallback.defaultValue) + .replace('{{guests}}', String(fallback.guests)) + .replace('{{groups}}', String(fallback.groups)); + } + return _key; + }, + i18n: { language: 'en' } + }) + }; +}); + +vi.mock('../../../services/guests.service', () => ({ + guestsService: { + getEventGuests: (...a: any[]) => getEventGuests(...a), + deleteGuest: vi.fn(), + mergeGuests: vi.fn(), + exportGuest: vi.fn(), + exportAllGuests: vi.fn(), + } +})); + +const wrapper = ({ children }: { children: ReactNode }) => { + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return {children}; +}; + +const guest = ( + id: number, + name: string, + email: string | null, + duplicate_group: string | null = null, + extra: Partial<{ email_verified_at: string | null; created_at: string; distinct_photos: number }> = {}, +) => ({ + id, name, email, duplicate_group, + created_at: extra.created_at ?? '2026-08-01T10:00:00Z', + last_seen_at: '2026-08-02T10:00:00Z', + email_verified_at: extra.email_verified_at ?? null, + is_deleted: false, + stats: { + likes: 3, favorites: 1, comments: 0, ratings: 0, reactions: 0, color_labels: 0, + distinct_photos: extra.distinct_photos ?? 3, + }, +}); + +const renderList = () => render(, { wrapper }); + +describe('duplicate guests in the admin list (#1210)', () => { + beforeEach(() => { + getEventGuests.mockReset(); + // Calls leak between cases otherwise — the survivor test below performs a + // real merge, and the "does not merge on its own" case asserts on the + // absence of exactly that call. + vi.clearAllMocks(); + vi.restoreAllMocks(); + }); + + it('says how many entries look like returning visitors', async () => { + getEventGuests.mockResolvedValue({ + guests: [guest(1, 'Tina', 'tina@example.com', 'tina@example.com'), guest(2, 'Tina', 'tina@example.com', 'tina@example.com')], + duplicates: { groups: 1, guests: 2 }, + }); + + renderList(); + + expect(await screen.findByText(/2 guest entries look like 1 returning visitor/i)).toBeInTheDocument(); + }); + + it('badges the rows the banner is talking about', async () => { + getEventGuests.mockResolvedValue({ + guests: [ + guest(1, 'Tina', 'tina@example.com', 'tina@example.com'), + guest(2, 'Tina', 'tina@example.com', 'tina@example.com'), + guest(3, 'Marc', 'marc@example.com'), + ], + duplicates: { groups: 1, guests: 2 }, + }); + + renderList(); + + // Two badged, and Marc left alone — the banner's claim is checkable + // against the rows rather than being taken on trust. + expect(await screen.findAllByText(/duplicate\?/i)).toHaveLength(2); + }); + + it('stays out of the way when nobody is duplicated', async () => { + getEventGuests.mockResolvedValue({ + guests: [guest(1, 'Tina', 'tina@example.com'), guest(2, 'Marc', 'marc@example.com')], + duplicates: { groups: 0, guests: 0 }, + }); + + renderList(); + + expect(await screen.findByText('Marc')).toBeInTheDocument(); + expect(screen.queryByText(/returning visitor/i)).not.toBeInTheDocument(); + expect(screen.queryByText(/duplicate\?/i)).not.toBeInTheDocument(); + }); + + it('refuses to merge until the admin says which entry survives', async () => { + // Three review rounds went into this: every automatic survivor rule was + // wrong somewhere. Most-feedback is guest-controlled; oldest-first keeps + // the row whose token expired and deletes the visitor's live identity. The + // data cannot answer it, so the UI must ask. + const { guestsService } = await import('../../../services/guests.service'); + getEventGuests.mockResolvedValue({ + guests: [ + guest(2, 'Tina', 'tina@example.com', 'tina@example.com'), + guest(1, 'Tina Ferrarelli', 'tina@example.com', 'tina@example.com', + { email_verified_at: '2026-08-01T10:14:00Z' }), + ], + duplicates: { groups: 1, guests: 2 }, + }); + + renderList(); + await userEvent.click(await screen.findByRole('button', { name: /^Review$/i })); + + // Group is ticked, but nothing is nominated to survive yet. + expect((await screen.findAllByRole('checkbox')).filter((c) => (c as HTMLInputElement).checked)).toHaveLength(2); + expect(screen.getAllByRole('radio').every((r) => !(r as HTMLInputElement).checked)).toBe(true); + expect(screen.getByRole('button', { name: /merge selected/i })).toBeDisabled(); + + // Choosing one enables it, and that is the id the merge keeps. + await userEvent.click(screen.getByRole('radio', { name: /Keep Tina Ferrarelli/i })); + vi.spyOn(window, 'confirm').mockReturnValue(true); + await userEvent.click(screen.getByRole('button', { name: /merge selected/i })); + + expect(guestsService.mergeGuests).toHaveBeenCalledWith(7, 1, [2]); + }); + + it('will not nominate a row that is not part of the merge', async () => { + getEventGuests.mockResolvedValue({ + guests: [ + guest(1, 'Tina', 'tina@example.com', 'tina@example.com'), + guest(2, 'Tina', 'tina@example.com', 'tina@example.com'), + guest(3, 'Marc', 'marc@example.com'), + ], + duplicates: { groups: 1, guests: 2 }, + }); + + renderList(); + await userEvent.click(await screen.findByRole('button', { name: /^Review$/i })); + + // Marc is not in the group, so he cannot be made the survivor of it. + expect(await screen.findByRole('radio', { name: /Keep Marc/i })).toBeDisabled(); + }); + + it('hands the group to the merge flow instead of merging on its own', async () => { + const { guestsService } = await import('../../../services/guests.service'); + getEventGuests.mockResolvedValue({ + guests: [guest(1, 'Tina', 'tina@example.com', 'tina@example.com'), guest(2, 'Tina', 'tina@example.com', 'tina@example.com')], + duplicates: { groups: 1, guests: 2 }, + }); + + renderList(); + await userEvent.click(await screen.findByRole('button', { name: /^Review$/i })); + + // Merge mode is open with the pair preselected, and nothing has been + // merged — the admin still chooses which row survives. + expect(guestsService.mergeGuests).not.toHaveBeenCalled(); + expect(screen.queryByText(/returning visitor/i)).not.toBeInTheDocument(); + }); +}); diff --git a/frontend/src/i18n/locales/de.json b/frontend/src/i18n/locales/de.json index 9093b0d1..367982bc 100644 --- a/frontend/src/i18n/locales/de.json +++ b/frontend/src/i18n/locales/de.json @@ -3011,7 +3011,16 @@ "pending": "Ausstehend", "redeemed": "Eingelöst", "revoked": "Widerrufen" - } + }, + "duplicatesFound": "{{guests}} Gasteinträge sehen nach {{groups}} wiederkehrenden Besucher(n) aus — gleiche E-Mail, mehrfach registriert. Ihre Auswahl bleibt geteilt, bis sie zusammengeführt werden.", + "reviewDuplicates": "Prüfen", + "duplicateBadge": "Duplikat?", + "duplicateHint": "Ein anderer Eintrag in dieser Galerie nutzt dieselbe E-Mail — vermutlich dieselbe Person, zweimal registriert.", + "mergePickKeep": "Bitte wählen, welcher Eintrag bleiben soll", + "mergePickKeepHint": "Eintrag zum Behalten wählen", + "mergeKeepColumn": "Behalten", + "mergeKeepRow": "{{name}} behalten", + "mergeInclude": "{{name}} in die Zusammenführung einbeziehen" }, "analytics": "Analytik", "activities": { diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index 38328340..2fcf7919 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -2586,7 +2586,16 @@ "pending": "Pending", "redeemed": "Redeemed", "revoked": "Revoked" - } + }, + "duplicatesFound": "{{guests}} guest entries look like {{groups}} returning visitor(s) — same email, registered more than once. Their picks are split until they are merged.", + "reviewDuplicates": "Review", + "duplicateBadge": "duplicate?", + "duplicateHint": "Another entry on this gallery uses the same email — likely the same person registered twice.", + "mergePickKeep": "Choose which entry to keep", + "mergePickKeepHint": "Pick the entry to keep", + "mergeKeepColumn": "Keep", + "mergeKeepRow": "Keep {{name}}", + "mergeInclude": "Include {{name}} in the merge" }, "analytics": "Analytics", "activities": { diff --git a/frontend/src/i18n/locales/fr.json b/frontend/src/i18n/locales/fr.json index 9e40c286..229109ba 100644 --- a/frontend/src/i18n/locales/fr.json +++ b/frontend/src/i18n/locales/fr.json @@ -1787,7 +1787,16 @@ "pending": "En attente", "redeemed": "Utilisé", "revoked": "Révoqué" - } + }, + "duplicatesFound": "{{guests}} entrées invité ressemblent à {{groups}} visiteur(s) de retour — même e-mail, inscrit plusieurs fois. Leurs choix restent séparés tant qu’ils ne sont pas fusionnés.", + "reviewDuplicates": "Vérifier", + "duplicateBadge": "doublon ?", + "duplicateHint": "Une autre entrée de cette galerie utilise le même e-mail — probablement la même personne inscrite deux fois.", + "mergePickKeep": "Choisissez l'entrée à conserver", + "mergePickKeepHint": "Choisir l'entrée à conserver", + "mergeKeepColumn": "Conserver", + "mergeKeepRow": "Conserver {{name}}", + "mergeInclude": "Inclure {{name}} dans la fusion" }, "analytics": "Analytique", "activities": { diff --git a/frontend/src/i18n/locales/nl.json b/frontend/src/i18n/locales/nl.json index 67653427..7c0846fd 100644 --- a/frontend/src/i18n/locales/nl.json +++ b/frontend/src/i18n/locales/nl.json @@ -1756,7 +1756,16 @@ "pending": "In behandeling", "redeemed": "Ingewisseld", "revoked": "Ingetrokken" - } + }, + "duplicatesFound": "{{guests}} gastvermeldingen lijken {{groups}} terugkerende bezoeker(s) — zelfde e-mail, meerdere keren geregistreerd. Hun keuzes blijven gesplitst tot ze zijn samengevoegd.", + "reviewDuplicates": "Bekijken", + "duplicateBadge": "duplicaat?", + "duplicateHint": "Een andere vermelding in deze galerij gebruikt hetzelfde e-mailadres — waarschijnlijk dezelfde persoon die zich twee keer registreerde.", + "mergePickKeep": "Kies welk item behouden blijft", + "mergePickKeepHint": "Kies het item om te behouden", + "mergeKeepColumn": "Behouden", + "mergeKeepRow": "{{name}} behouden", + "mergeInclude": "{{name}} in de samenvoeging opnemen" }, "analytics": "Analyse", "activities": { diff --git a/frontend/src/i18n/locales/pt.json b/frontend/src/i18n/locales/pt.json index f9e52813..52033cad 100644 --- a/frontend/src/i18n/locales/pt.json +++ b/frontend/src/i18n/locales/pt.json @@ -1781,7 +1781,16 @@ "pending": "Pendente", "redeemed": "Utilizado", "revoked": "Revogado" - } + }, + "duplicatesFound": "{{guests}} entradas de convidado parecem {{groups}} visitante(s) recorrente(s) — mesmo e-mail, registado mais do que uma vez. As escolhas ficam divididas até serem unidas.", + "reviewDuplicates": "Rever", + "duplicateBadge": "duplicado?", + "duplicateHint": "Outra entrada nesta galeria usa o mesmo e-mail — provavelmente a mesma pessoa registada duas vezes.", + "mergePickKeep": "Escolha qual entrada manter", + "mergePickKeepHint": "Escolha a entrada a manter", + "mergeKeepColumn": "Manter", + "mergeKeepRow": "Manter {{name}}", + "mergeInclude": "Incluir {{name}} na junção" }, "analytics": "Análise", "activities": { diff --git a/frontend/src/i18n/locales/ru.json b/frontend/src/i18n/locales/ru.json index 384445fa..36336023 100644 --- a/frontend/src/i18n/locales/ru.json +++ b/frontend/src/i18n/locales/ru.json @@ -1806,7 +1806,16 @@ "pending": "Ожидает", "redeemed": "Использовано", "revoked": "Отозвано" - } + }, + "duplicatesFound": "{{guests}} записей гостей похожи на {{groups}} вернувшихся посетителей — тот же e-mail, регистрация несколько раз. Их выбор останется разделённым, пока записи не объединят.", + "reviewDuplicates": "Проверить", + "duplicateBadge": "дубликат?", + "duplicateHint": "Другая запись в этой галерее использует тот же e-mail — вероятно, тот же человек зарегистрировался дважды.", + "mergePickKeep": "Выберите, какую запись сохранить", + "mergePickKeepHint": "Выберите запись для сохранения", + "mergeKeepColumn": "Сохранить", + "mergeKeepRow": "Сохранить {{name}}", + "mergeInclude": "Включить {{name}} в объединение" }, "analytics": "Аналитика", "activities": { diff --git a/frontend/src/i18n/locales/sl.json b/frontend/src/i18n/locales/sl.json index a618d5ca..2257fa7f 100644 --- a/frontend/src/i18n/locales/sl.json +++ b/frontend/src/i18n/locales/sl.json @@ -1776,7 +1776,16 @@ "pending": "Čaka", "redeemed": "Unovčeno", "revoked": "Preklicano" - } + }, + "duplicatesFound": "{{guests}} vnosov gostov je videti kot {{groups}} vračajočih se obiskovalcev — isti e-naslov, večkrat registriran. Njihova izbira ostane razdeljena, dokler jih ne združite.", + "reviewDuplicates": "Preveri", + "duplicateBadge": "dvojnik?", + "duplicateHint": "Drug vnos v tej galeriji uporablja isti e-naslov — najverjetneje ista oseba, registrirana dvakrat.", + "mergePickKeep": "Izberite, kateri vnos naj ostane", + "mergePickKeepHint": "Izberite vnos za ohranitev", + "mergeKeepColumn": "Ohrani", + "mergeKeepRow": "Ohrani {{name}}", + "mergeInclude": "Vključi {{name}} v združitev" }, "analytics": "Analitika", "activities": { diff --git a/frontend/src/services/guests.service.ts b/frontend/src/services/guests.service.ts index 4b040b72..ee08b014 100644 --- a/frontend/src/services/guests.service.ts +++ b/frontend/src/services/guests.service.ts @@ -30,6 +30,11 @@ export interface AdminGuest { last_seen_at: string; email_verified_at: string | null; is_deleted: boolean; + // Set when this row shares an email with another on the same event (#1210) + // — the same person who registered again after their token expired or on a + // second device. The value is the normalised email, so it doubles as the + // grouping key; null for everyone else. + duplicate_group?: string | null; stats: AdminGuestStats; } @@ -110,7 +115,10 @@ class GuestsService { // Admin-side // =================================================================== - async getEventGuests(eventId: number): Promise<{ guests: AdminGuest[] }> { + async getEventGuests(eventId: number): Promise<{ + guests: AdminGuest[]; + duplicates?: { groups: number; guests: number }; + }> { const response = await api.get(`/admin/events/${eventId}/guests`); return response.data; }