paul/picpeak
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(guests): surface duplicate guest registrations, and stop making so many (#1210) (#1216)

Browse Source 
* fix(guests): surface duplicate guest registrations, and stop making so many (#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. The photographer's 'final
selection' is then only trustworthy if somebody notices two Tinas with half the
picks each.

Two halves, neither of which touches the registration path.

**Say which rows are the same person.** Merging already worked, endpoint and UI
both; nothing said WHICH rows to merge. The guests list now marks each row with
the others sharing its email and returns a count for the banner, and the admin
list offers the group straight to the merge mode that already exists.
Case-folded and trimmed, because the same person types Tina@ one day and tina@
the next and both read as distinct rows. Email only — two guests called Anna
are not evidence of anything, and rows without an email are not grouped at all
since require_name_email is off by default and a shared link produces plenty of
them.

It preselects rather than merges: which row survives decides the name and
verification state the merged guest keeps, and that is the admin's call.

**Create fewer of them.** The guest token was 24h and every call site took that
default, so even the same browser lost its identity after a day of inactivity.
Now 30 days, GUEST_TOKEN_TTL to override. A guest token is scoped to one event,
carries no admin capability, and the gallery is already behind whatever
protects it — 30 days is the shape of a real proofing cycle.

Deliberately NOT done: reusing a guest row when a typed email matches, which
the report suggests first. It would let anyone who knows an address inherit
that person's identity and selections, and answering differently for a known
email would leak which addresses are in the gallery — the thing
/guest/recover already goes out of its way to avoid. Prevention at the entry
path needs the verification round-trip, which is a separate decision about
friction.

13 tests; 8 of the 9 backend ones fail without the change. The frontend ones
caught a real bug while being written — the new useMemo sat after the loading
early-return, so the hook count changed between renders.

* fix(guests): merge must not strand a pending invite (#1210)

Three findings from external review of #1216.

**A merge could kill an emailed invite link.** 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 sharing an email — which this feature now points out
and offers to merge. Redemption resolves guest_invites.guest_id with
is_deleted: false, so merging soft-deleted the row the link pointed at: the
client got 404 guest_missing while the invite dialog still showed the invite as
Pending. Nothing anywhere said the link was dead. Unredeemed, unrevoked invites
now move to the survivor first. Spent ones stay put — a redeemed invite records
who redeemed what, and retargeting it would rewrite that.

**The preselection silently chose the survivor.** performMerge keeps
mergeSelection[0], and the group was handed over in API order, which is
newest-first — so Review then Merge discarded an older, email-verified row
holding most of the picks in favour of a fresh re-registration. The proposal is
now ordered deliberately: verified first, then whoever holds the most feedback,
then the oldest. Still only a proposal, and the confirmation now names the
survivor by email as well as name, because duplicates share a name and 'Merge 2
guests into Tina?' said nothing.

**duplicate_of was quadratic.** Every row carried the other n-1 ids, so a group
of n serialised n² of them — and nothing consumed the list: the UI asked only
whether a row was in a group, then regrouped by email itself. Replaced with
duplicate_group, the normalised email, which keeps the payload linear and the
case/whitespace folding in one place instead of reimplemented on the client.

Two new backend tests for the invite paths, one frontend test asserting the
merge call keeps the verified row. The invite test fails against the un-fixed
code.

* fix(guests): keep guest-controlled input out of who survives a merge (#1210)

Round 2 of external review on #1216.

**The survivor ranking used an attacker-controlled signal.** Preferring
whoever holds the most feedback looked like the obvious tiebreak and is exactly
the wrong one: registration does not verify the address, so anyone who knows a
guest's email can register with it, mark enough photos to out-rank the real
person, and be preselected as the survivor. An admin accepting a confirmation
between two rows with the same name and email would then move the victim's
picks onto an identity whose token the visitor still holds. distinct_photos is
guest-controlled and has no business deciding this. The ranking is now
email_verified_at then created_at — both server-set.

**A merge could make the survivor unrecoverable.** Rows are grouped with case
and whitespace folded out, so a merge can be proposed between [email protected]
and [email protected]. /guest/recover lowercases what the guest types and then
matches on equality, so a survivor left holding the raw value can never be
recovered by email again. The kept row's address is now canonicalised during
the merge. Both write paths normalise today, so this covers rows that predate
that — which are exactly the rows case-folded grouping surfaces.

Two more backend tests. The residual, stated plainly: an admin can still merge
two unverified rows in either order. What is gone is the tool ranking them by
something a visitor controls.

* fix(compose): pass GUEST_TOKEN_TTL through to the backend (#1210)

The override was documented in .env.example and could never take effect: the
backend service takes an explicit environment list, so a variable not named
there never reaches the container. An operator following the documentation
would have shortened the guest session and seen nothing change.

docker-compose.production.yml uses env_file: .env and already passed it
through; docker-compose.dev.yml is gitignored, so only this file needs it.

* fix(guests): the admin picks the merge survivor, the tool does not (#1210)

Fourth review round on the same point, and the right conclusion is that there
is no correct automatic answer.

Every rule tried was wrong somewhere. Most-feedback is guest-controlled — the
address is never verified at registration, so anyone who knows it can register
and mark photos until they out-rank the real person. Oldest-first, the
replacement, is worse for the ordinary case: when a token expires the OLD row
is the dead identity and the new one is the visitor's live session, so keeping
the oldest deletes the identity they are actually using, and the frontend holds
that deleted guest in sessionStorage without clearing it on a 401. Registration
timing is visitor-controlled too.

The data does not say which row is really the person. So the UI asks: merge
mode gains a Keep column, the button stays disabled until a row is nominated,
and only rows included in the merge can be nominated. The group is still
preselected — finding the duplicates was always the point — but nothing about
who survives is decided by sort order any more.

This also makes the claim in the PR description true. It said the admin decides
which row survives; until now the preselection quietly decided it for them.

Two rewritten frontend tests: the merge is blocked until a survivor is chosen
and then keeps exactly that row, and a row outside the group cannot be
nominated. The test i18n mock now interpolates, so aria-labels are queryable by
their rendered text.

---------

Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
2026-08-28 08:27:15 +02:00
committed by GitHub
co-authored by Paul Nothaft
parent 27aff7c04e
commit 5c85e0c0e4
16 changed files with 790 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/components/admin/AdminGuestsList.tsx
+113 -7
View File
@@ -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<AdminGuestsListProps> = ({ eventId, event
const [selectedGuest, setSelectedGuest] = useState<AdminGuest | null>(null);
const [mergeMode, setMergeMode] = useState(false);
const [mergeSelection, setMergeSelection] = useState<number[]>([]);
// Which row absorbs the others. Never defaulted: see the grouping comment.
const [keepId, setKeepId] = useState<number | null>(null);
const inviteModal = useModal();
const { data, isLoading, refetch } = useQuery({
@@ -47,6 +49,7 @@ export const AdminGuestsList: React.FC<AdminGuestsListProps> = ({ 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<AdminGuestsListProps> = ({ 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<string, AdminGuest[]>();
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 <Loading size="lg" text={t('admin.guests.loading', 'Loading guests...')} />;
}
const guests = data?.guests || [];
if (view === 'aggregate') {
return (
@@ -145,10 +183,15 @@ export const AdminGuestsList: React.FC<AdminGuestsListProps> = ({ eventId, event
<span className="text-sm text-neutral-600 dark:text-neutral-400">
{t('admin.guests.mergeSelected', '{{count}} selected', { count: mergeSelection.length })}
</span>
<Button variant="primary" size="sm" onClick={performMerge} disabled={mergeSelection.length < 2}>
{keepId === null && (
<span className="text-sm text-amber-700 dark:text-amber-300">
{t('admin.guests.mergePickKeepHint', 'Pick the entry to keep')}
</span>
)}
<Button variant="primary" size="sm" onClick={performMerge} disabled={mergeSelection.length < 2 || keepId === null}>
{t('admin.guests.mergeNow', 'Merge selected')}
</Button>
<Button variant="ghost" size="sm" onClick={() => { setMergeMode(false); setMergeSelection([]); }}>
<Button variant="ghost" size="sm" onClick={() => { setMergeMode(false); setMergeSelection([]); setKeepId(null); }}>
{t('common.cancel', 'Cancel')}
</Button>
</>
@@ -199,6 +242,40 @@ export const AdminGuestsList: React.FC<AdminGuestsListProps> = ({ eventId, event
</div>
</div>
{/* 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 && (
<div className="mb-4 rounded-lg border border-amber-200 dark:border-amber-800 bg-amber-50 dark:bg-amber-900/30 px-4 py-3 flex items-center justify-between gap-4">
<p className="text-sm text-amber-800 dark:text-amber-200">
{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.',
})}
</p>
<Button
variant="secondary"
size="sm"
onClick={() => {
setMergeMode(true);
setMergeSelection(duplicateGroups[0].map((g) => g.id));
setKeepId(null);
}}
className="shrink-0"
>
{t('admin.guests.reviewDuplicates', 'Review')}
</Button>
</div>
)}
{guests.length === 0 ? (
<Card>
<div className="p-8 text-center text-neutral-500 dark:text-neutral-400">
@@ -212,6 +289,11 @@ export const AdminGuestsList: React.FC<AdminGuestsListProps> = ({ eventId, event
<thead className="bg-neutral-50 dark:bg-neutral-800 border-b border-neutral-200 dark:border-neutral-700">
<tr>
{mergeMode && <th className="px-4 py-3 w-8" />}
{mergeMode && (
<th className="px-4 py-3 w-16 text-left text-xs font-medium text-neutral-600 dark:text-neutral-400 uppercase">
{t('admin.guests.mergeKeepColumn', 'Keep')}
</th>
)}
<th className="px-4 py-3 text-left text-xs font-medium text-neutral-600 dark:text-neutral-400 uppercase">
{t('admin.guests.columns.name', 'Name')}
</th>
@@ -249,12 +331,28 @@ export const AdminGuestsList: React.FC<AdminGuestsListProps> = ({ eventId, event
<td className="px-4 py-3">
<input
type="checkbox"
aria-label={t('admin.guests.mergeInclude', 'Include {{name}} in the merge', { name: guest.name })}
checked={mergeSelection.includes(guest.id)}
onChange={() => toggleMergeSelection(guest.id)}
className="w-4 h-4 text-accent rounded focus:ring-primary-500"
/>
</td>
)}
{mergeMode && (
<td className="px-4 py-3">
{/* The survivor, chosen rather than derived. Only
selectable among the rows actually being merged. */}
<input
type="radio"
name="merge-keep"
aria-label={t('admin.guests.mergeKeepRow', 'Keep {{name}}', { name: guest.name })}
checked={keepId === guest.id}
disabled={!mergeSelection.includes(guest.id)}
onChange={() => setKeepId(guest.id)}
className="w-4 h-4 text-accent focus:ring-primary-500 disabled:opacity-40"
/>
</td>
)}
<td className="px-4 py-3 font-medium text-neutral-900 dark:text-neutral-100">
{guest.name}
{guest.email_verified_at && (
@@ -263,6 +361,14 @@ export const AdminGuestsList: React.FC<AdminGuestsListProps> = ({ eventId, event
</td>
<td className="px-4 py-3 text-sm text-neutral-600 dark:text-neutral-400">
{guest.email || '—'}
{guest.duplicate_group && (
<span
className="ml-2 inline-block rounded px-1.5 py-0.5 text-xs bg-amber-100 dark:bg-amber-900/50 text-amber-800 dark:text-amber-200"
title={t('admin.guests.duplicateHint', 'Another entry on this gallery uses the same email — likely the same person registered twice.')}
>
{t('admin.guests.duplicateBadge', 'duplicate?')}
</span>
)}
</td>
<td className="px-4 py-3 text-right text-sm text-neutral-900 dark:text-neutral-100">
{guest.stats.likes}
frontend/src/components/admin/__tests__/AdminGuestsList.duplicates.test.tsx
+199
View File
@@ -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<typeof import('react-i18next')>('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 <QueryClientProvider client={client}>{children}</QueryClientProvider>;
};
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(<AdminGuestsList eventId={7} eventName="Test" />, { 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', '[email protected]', '[email protected]'), guest(2, 'Tina', '[email protected]', '[email protected]')],
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', '[email protected]', '[email protected]'),
guest(2, 'Tina', '[email protected]', '[email protected]'),
guest(3, 'Marc', '[email protected]'),
],
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', '[email protected]'), guest(2, 'Marc', '[email protected]')],
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', '[email protected]', '[email protected]'),
guest(1, 'Tina Ferrarelli', '[email protected]', '[email protected]',
{ 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', '[email protected]', '[email protected]'),
guest(2, 'Tina', '[email protected]', '[email protected]'),
guest(3, 'Marc', '[email protected]'),
],
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', '[email protected]', '[email protected]'), guest(2, 'Tina', '[email protected]', '[email protected]')],
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();
});
});