feat(feedback): a third identity mode with one shared colour tag per photo (#1197) (#1208)

* feat(feedback): a third identity mode with one shared colour tag per photo (#1197)

Split out of #1178, where @boergu asked for a colour tag with no identity
dimension at all: not everyone sharing a device's state, but everyone — on any
device — sharing the PHOTO's state. Guest A marks it green, guest B later marks
it orange, and the tag simply becomes orange. One collaboratively-agreed verdict
per photo instead of per-person tallies.

identity_mode gains 'shared'. The mode is scoped to the colour tag: likes,
ratings, comments, favourites and reactions stay per-visitor exactly as in
'simple', because that is what was asked for and widening it would change what
every other control means.

Stored as an ordinary photo_feedback row under a reserved identifier rather
than as a column on photos. That is what keeps the rest of the system working
untouched — the per-colour tally simply has exactly one entry, so
dominant_color_label, color_label_count, the admin colour filter and the
XMP/CSV export that #745 reads all keep their existing shapes, and no consumer
has to learn a second one. The identifier cannot be claimed: real ones are
sha256 hashes or server-minted UUIDs, and the per-guest write path rejects it
outright.

Last write wins, inside a transaction that locks the photo row. Without the
lock two guests tapping different colours in the same instant both read 'no
tag', both insert, and the photo ends up carrying two shared tags — the
per-guest tally this mode exists to remove. Re-sending the colour already on a
photo clears it, from any guest: the same toggle every other colour path uses,
and the only way to remove a tag without inventing a second control.

Switching modes is non-destructive. Existing per-guest labels are left alone
and simply not read while shared is on; the shared tag starts empty rather than
collapsing marks nobody agreed on, and switching back restores every original
exactly. An event can hold both sets, only one of which is live.

The tag stays visible with show_feedback_to_guests off — it arrives through the
per-viewer channel, being the photo's own state rather than someone else's
opinion — while the per-colour tallies stay hidden. The colour filters answer
from it for the same reason, so a gallery with sharing off cannot show colours
on tiles that no filter can find.

Attribution is gone by design, and the settings panel says so before an
operator picks the mode.

Decisions (1), (4) and (5) from the issue were settled up front, as it asked.
Decision (3) turned out not to need anything: guest colour filters already read
my_color_label, and the admin's my_color_labels filters photo_admin_marks
(#1183), not guest identity — so nothing collapses on either side.

* fix(feedback): shared mode saves on Postgres, and dormant labels stay dormant (#1197)

Three findings from external review, all confirmed against source before fixing.

**The mode could not be saved on Postgres at all.** Migration 078 created
identity_mode with a CHECK constraint pinned to ('simple','guest'), guarded on
`client === 'pg'` — so SQLite never has it and no SQLite test can see it, while
the database every default production install runs rejects the new value
outright. Migration 192 drops and re-adds the constraint with 'shared' included;
its down() resets any event using the mode to 'simple' first, or the narrower
constraint could not be restored. Verified against a real Postgres on a scratch
database: the insert fails before, succeeds after, up() is re-runnable, and
down() puts the old constraint back.

**Dormant labels were still being read.** Switching modes is deliberately
non-destructive, which leaves both sets of colour labels in the table with only
one live — and every read that did not say which set it meant kept counting the
other. The per-colour tallies, color_label_count, the admin grid badge, the
XMP/CSV export, both admin colour filters and the guest colour filter all saw
labels the mode does not show; switching back exposed the shared row as an
anonymous other guest's dot. The settings panel promises these are 'kept but not
shown', and that has to mean every surface, not just the badge. Scoped at the
source — the two count helpers resolve the mode themselves — so the admin grid
and the export are fixed without touching either.

**The create form's identity mode was dropped.** CreateEventPage has always
rendered the chooser and the create route never read it, so a gallery created as
'guest' came out 'simple' and had to be set again on the event afterwards. A
pre-existing bug that adding a third option made worse; threaded through now,
which fixes it for all three modes.

Six regression tests, each verified to fail against the un-fixed code.

* fix(feedback): keep every colour surface consistent across a mode change (#1197)

Second review round, four findings, all confirmed in source first.

**Stored counters went stale on a mode switch.** photos.color_label_count is
denormalized and recomputed on feedback writes, so changing identity_mode —
which changes nothing about the rows, only which of them are live — left the
old mode's totals on the tiles, the admin grid and the filter summary until
each photo happened to be touched again. On a finished gallery that is never.
Recounted for the event when the mode actually changes, as two statements
rather than a per-photo recompute: four of the five counters cannot have moved.

**Duplicating an event dropped the mode**, the same shape as the create-form
bug from the last round — a gallery cloned to reuse its proofing setup came
back in 'simple'.

**The event feedback summary counted dormant labels**, inflating total_feedback
in the admin analytics and the guest /feedback-summary while every other
surface hid them.

**The swatch trusted its optimistic guess over the server.** In shared mode the
tag belongs to the photo, so another guest can move it between this viewer's
last read and their click: a viewer still showing green clicks green, the
server sets green because the tag had become red meanwhile, and the optimistic
'same colour, so clear' blanked the swatch against a server that holds one. The
response already says which happened, so it is used. The per-guest modes are
unaffected — only the guest can move their own label, so guess and answer
always agreed there.

Three regression tests, each verified to fail against the un-fixed code.

* fix(feedback): shared tag is not a participant, and the keyboard path reconciles too (#1197)

Third review round, two findings.

**feedback_count counted the shared tag as a guest.** It is COUNT(DISTINCT
guest identity) across all feedback types, and the reserved identifier looked
like a person: a photo with one rating and a shared tag reported two. The
column is exported as rating_count (photoExportService), so merely tagging a
photo inflated its rating count in the CSV and JSON exports.

**The lightbox keyboard path still trusted its own guess.** The reconciliation
from the last round covered clicks through PhotoColorLabels, but the proofing
shortcuts call PhotoLightbox.submitColorLabel directly and set local state from
a locally computed toggle. That is the path a proofing client actually uses, so
it had the divergence the previous fix was for: another guest moves the tag,
this viewer presses the key, the server sets a colour and the swatch blanks.
Both branches now read the outcome off the response.

One regression test, verified to fail against the un-fixed code.

* fix(feedback): identity-mode lookup must survive a migration-time caller (#1197)

updatePhotoFeedbackStats is called from migrations as well as from the request
path — migration 186's duplicate-photo dedupe (#1162) recomputes the survivor's
totals — and a migration runs against a half-built schema where
event_feedback_settings need not exist yet. The new inner join threw there,
which took the whole stats update down with it, so the reparented rows were
never counted and eight assertions in the 186 suite failed.

Falls back to 'simple', which is the right answer rather than merely a safe
one: an install with no feedback settings table has no event in shared mode, so
the non-shared scope is exactly correct.

Caught by CI, not by me — I had been running affected suites rather than the
full one after each review round.

* fix(feedback): atomic shared-tag write, scoped feedback list, safe PG fallback (#1197)

Round 4 of external review, and one of the three is about the fix I made for
the CI failure two rounds ago.

**The identity-mode fallback could poison a Postgres transaction.** The join
was wrapped in try/catch so a migration-time caller with a half-built schema
would fall back to 'simple'. On Postgres a failed statement aborts the entire
transaction, so catching it and carrying on left the caller's trx poisoned and
the aggregate that follows failed with 'current transaction is aborted' —
defeating the very compatibility the fallback was added for. It now asks
whether the table exists before issuing the join, which is safe to ask and
aborts nothing. Memoised once true, since a table does not un-create itself and
this sits on the feedback write path.

**The shared-tag stats were recomputed after the commit.** A failure there
returned 500 for a tag that had already been written, so the client reverted
its swatch and the next tap on the same colour toggled the committed tag off
instead of setting it. Two concurrent writers could also race their aggregate
updates. Recomputed inside the transaction now, while the photo row is still
locked.

**The raw feedback list still carried both label sets.** Only the tallies and
my_feedback had been scoped, so a dormant per-guest label was still visible to
anyone reading the list — and with sharing off it came back flagged is_mine.
getPhotoFeedback now filters colour labels to the active set.

One test for the list; the migration suite that caught the original CI
regression still passes.

---------

Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
2026-08-28 08:19:41 +02:00
committed by GitHub
co-authored by Paul Nothaft
parent 71eaf25d94
commit 22e00f80b6
26 changed files with 1309 additions and 53 deletions
@@ -0,0 +1,424 @@
/**
* The shared colour tag (#1197).
*
* A third identity model, requested in #1178: not "everyone shares a device's
* state" but "everyone, on any device, shares the PHOTO's state". One
* identity-less colour tag per photo, and whoever writes last wins.
*
* Stored as an ordinary photo_feedback row under a reserved identifier rather
* than as a column on photos, which is what keeps the per-colour tallies, the
* filters, the moderation queue and the XMP/CSV export working unchanged: the
* tally simply has exactly one entry.
*
* The mode is scoped to the colour tag. Likes, ratings and the rest stay
* per-guest, so the last test here is as important as the first.
*/
const request = require('supertest');
const express = require('express');
const cookieParser = require('cookie-parser');
const jwt = require('jsonwebtoken');
const { bootCrmDb, seedMinimal } = require('./helpers/crmDb');
const { SHARED_COLOR_LABEL_IDENTITY } = require('../../src/constants/colorLabels');
process.env.JWT_SECRET = process.env.JWT_SECRET || 'shared-tag-secret';
const SLUG = 'shared-color-tag';
describe('shared colour tag (#1197)', () => {
let db; let cleanup; let app; let feedbackService;
let eventId; let photoId; let otherPhotoId;
const galleryToken = () => jwt.sign(
{ eventId, eventSlug: SLUG, type: 'gallery' },
process.env.JWT_SECRET,
{ expiresIn: '1h', issuer: 'picpeak-auth' }
);
// Two different devices: distinct UA strings give distinct
// generateGuestIdentifier hashes, which is exactly how `simple` mode tells
// two anonymous guests apart.
const asGuest = (ua) => ({ 'Authorization': `Bearer ${galleryToken()}`, 'User-Agent': ua });
const tag = (ua, color, id = photoId) => request(app)
.post(`/api/gallery/${SLUG}/photos/${id}/feedback`)
.set(asGuest(ua))
.send({ feedback_type: 'color_label', color_label: color });
const photosFor = async (ua, query = '') => {
const res = await request(app)
.get(`/api/gallery/${SLUG}/photos${query}`)
.set(asGuest(ua));
expect(res.status).toBe(200);
return Array.isArray(res.body) ? res.body : res.body.photos;
};
const photoFor = async (ua) => (await photosFor(ua)).find((p) => p.id === photoId);
const feedbackFor = async (ua) => {
const res = await request(app)
.get(`/api/gallery/${SLUG}/photos/${photoId}/feedback`)
.set(asGuest(ua));
expect(res.status).toBe(200);
return res.body;
};
const sharedRows = () => db('photo_feedback').where({
photo_id: photoId, feedback_type: 'color_label',
guest_identifier: SHARED_COLOR_LABEL_IDENTITY,
});
const setMode = (mode) => db('event_feedback_settings')
.where({ event_id: eventId }).update({ identity_mode: mode });
const setSharing = (on) => db('event_feedback_settings')
.where({ event_id: eventId }).update({ show_feedback_to_guests: on });
beforeAll(async () => {
({ db, cleanup } = await bootCrmDb());
await seedMinimal(db);
feedbackService = require('../../src/services/feedbackService');
const [ev] = await db('events').insert({
slug: SLUG,
event_type: 'wedding',
event_name: 'Shared Colour Tag',
event_date: '2026-08-01',
host_email: '[email protected]',
admin_email: '[email protected]',
password_hash: 'x',
share_link: `/gallery/${SLUG}/share`,
share_token: 'shared-tag-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 [p] = await db('photos').insert({
event_id: eventId, filename: 'a.jpg', path: 'events/shared/a.jpg',
type: 'individual', uploaded_at: new Date().toISOString(),
}).returning('id');
photoId = typeof p === 'object' ? p.id : p;
const [p2] = await db('photos').insert({
event_id: eventId, filename: 'b.jpg', path: 'events/shared/b.jpg',
type: 'individual', uploaded_at: new Date().toISOString(),
}).returning('id');
otherPhotoId = typeof p2 === 'object' ? p2.id : p2;
await db('event_feedback_settings').insert({
event_id: eventId, feedback_enabled: true, allow_likes: true,
allow_color_labels: true, moderate_comments: false,
show_feedback_to_guests: true, identity_mode: 'shared',
});
app = express();
app.use(express.json());
app.use(cookieParser());
app.use('/api/gallery', require('../../src/routes/gallery'));
app.use('/api/gallery', require('../../src/routes/galleryFeedback'));
}, 180000);
afterAll(async () => { if (cleanup) await cleanup(); });
beforeEach(async () => {
await db('photo_feedback').where({ event_id: eventId }).del();
await db('photos').where('event_id', eventId).update({ color_label_count: 0, like_count: 0 });
await setMode('shared');
await setSharing(true);
});
describe('one tag per photo, last write wins', () => {
it('lets a second guest overwrite the first guest\'s colour', async () => {
// The request in the reporter's words: "if guest A marks a photo green
// and guest B later marks the same photo orange, the shared tag simply
// becomes orange".
expect((await tag('device-A', 'green')).status).toBe(200);
expect((await tag('device-B', 'blue')).status).toBe(200);
const rows = await sharedRows().select('color_label');
expect(rows).toHaveLength(1);
expect(rows[0].color_label).toBe('blue');
});
it('shows the same tag to a guest who never set one', async () => {
await tag('device-A', 'green');
// Different device, different identifier — in simple mode this would
// read back null, which is the whole reason the mode exists.
expect((await photoFor('device-B')).my_color_label).toBe('green');
});
it('stores no attribution against the tag', async () => {
await tag('device-A', 'green');
const row = await sharedRows().first();
expect(row.guest_id).toBeFalsy();
expect(row.guest_name).toBeFalsy();
expect(row.guest_email).toBeFalsy();
});
it('clears the tag when any guest re-sends the colour already on it', async () => {
await tag('device-A', 'green');
// B, not A: clearing is not owned by whoever set it.
expect((await tag('device-B', 'green')).status).toBe(200);
expect(await sharedRows().first()).toBeFalsy();
expect((await photoFor('device-A')).my_color_label).toBeFalsy();
});
it('leaves exactly one tag when two guests write at the same instant', async () => {
// The race the transaction exists for: both writers read "no tag", both
// insert, and the photo ends up carrying two shared tags — a per-guest
// tally in the one mode that is supposed to have none.
await Promise.all([
tag('device-A', 'green'),
tag('device-B', 'red'),
tag('device-C', 'blue'),
]);
const rows = await sharedRows().select('color_label');
expect(rows).toHaveLength(1);
expect(['green', 'red', 'blue']).toContain(rows[0].color_label);
});
it('keeps the tag on the photo it was set on', async () => {
await tag('device-A', 'green');
await tag('device-B', 'red', otherPhotoId);
const photos = await photosFor('device-C');
expect(photos.find((p) => p.id === photoId).my_color_label).toBe('green');
expect(photos.find((p) => p.id === otherPhotoId).my_color_label).toBe('red');
});
});
describe('the aggregates keep their existing shape', () => {
it('reports the tally as a single colour with a count of one', async () => {
await tag('device-A', 'green');
await tag('device-B', 'red');
// Unchanged consumers — grid badge, XMP export, admin filter — read
// this map and dominantColorLabel() over it. One entry, so the dominant
// colour is simply the tag.
expect(await feedbackService.getPhotoColorLabelCounts(photoId)).toEqual({ red: 1 });
const photo = await db('photos').where('id', photoId).first();
expect(photo.color_label_count).toBe(1);
});
it('does not render the tag a second time as another viewer\'s dot', async () => {
await tag('device-A', 'green');
// The shared row is not filed under device-B, so without the mode check
// it would come back as "someone else's label" and the tile would show
// the same green twice — once as the badge, once as a dot beside it.
const photo = await photoFor('device-B');
expect(photo.my_color_label).toBe('green');
expect(photo.other_color_labels || []).toHaveLength(0);
});
});
describe('with show_feedback_to_guests off', () => {
it('still shows the tag — it is the photo\'s state, not someone else\'s opinion', async () => {
await tag('device-A', 'green');
await setSharing(false);
expect((await photoFor('device-B')).my_color_label).toBe('green');
expect((await feedbackFor('device-B')).my_feedback.color_label).toBe('green');
});
it('still hides the per-colour tallies', async () => {
await tag('device-A', 'green');
await setSharing(false);
expect((await feedbackFor('device-B')).color_labels).toEqual({});
});
it('answers a colour filter from the shared tag', async () => {
await tag('device-A', 'green');
await setSharing(false);
// The aggregate half of this filter is gated on sharing; in shared mode
// the tag counts as the viewer's own, so the filter still works.
const filtered = await photosFor('device-B', '?filter=color:green');
expect(filtered.map((p) => p.id)).toEqual([photoId]);
});
});
describe('switching modes is not destructive', () => {
it('ignores per-guest labels while shared, and gives them back afterwards', async () => {
await setMode('simple');
await tag('device-A', 'green');
await tag('device-B', 'red');
const perGuestRows = await db('photo_feedback')
.where({ photo_id: photoId, feedback_type: 'color_label' }).count('* as c').first();
expect(Number(perGuestRows.c)).toBe(2);
await setMode('shared');
// Nothing collapsed, nothing guessed: the shared tag starts empty.
expect((await photoFor('device-A')).my_color_label).toBeFalsy();
expect(await sharedRows().first()).toBeFalsy();
await setMode('simple');
// ...and every original mark is exactly where its owner left it.
expect((await photoFor('device-A')).my_color_label).toBe('green');
expect((await photoFor('device-B')).my_color_label).toBe('red');
});
it('keeps the shared tag intact across a round trip through simple mode', async () => {
await tag('device-A', 'green');
await setMode('simple');
await setMode('shared');
expect((await photoFor('device-B')).my_color_label).toBe('green');
});
// "Kept but not shown" has to mean every surface, not just the badge. The
// dormant set is still sitting in photo_feedback, so a read that does not
// say which set it means will happily count, tally, filter and export it.
describe('the dormant set stays out of every reading of the live one', () => {
const seedDormantPerGuestLabels = async () => {
await setMode('simple');
await tag('device-A', 'green');
await tag('device-B', 'red', otherPhotoId);
await setMode('shared');
};
it('keeps dormant labels out of the per-colour tallies', async () => {
await seedDormantPerGuestLabels();
// The lightbox renders this map. Green belongs to a per-guest row the
// mode does not use, so the photo reads as untagged.
expect(await feedbackService.getPhotoColorLabelCounts(photoId)).toEqual({});
await tag('device-C', 'blue');
expect(await feedbackService.getPhotoColorLabelCounts(photoId)).toEqual({ blue: 1 });
});
it('keeps dormant labels out of color_label_count', async () => {
await seedDormantPerGuestLabels();
await feedbackService.updatePhotoFeedbackStats(photoId);
expect((await db('photos').where('id', photoId).first()).color_label_count).toBe(0);
await tag('device-C', 'blue');
expect((await db('photos').where('id', photoId).first()).color_label_count).toBe(1);
});
it('keeps dormant labels out of the admin grid and the XMP/CSV export', async () => {
await seedDormantPerGuestLabels();
// getEventColorLabelCounts feeds dominant_color_label, which is what
// the admin badge shows and what xmp:Label round-trips into Lightroom.
const map = await feedbackService.getEventColorLabelCounts(eventId, [photoId, otherPhotoId]);
expect(map[photoId]).toBeUndefined();
expect(map[otherPhotoId]).toBeUndefined();
});
it('keeps dormant labels out of the raw feedback list too', async () => {
// The per-colour tallies were scoped, but the endpoint also returns the
// rows themselves. Those carried both sets, so a dormant per-guest
// label was still visible to anyone reading the list — and with
// sharing off it came back flagged as the caller's own.
await seedDormantPerGuestLabels();
const body = await feedbackFor('device-A');
expect(body.feedback.filter((f) => f.feedback_type === 'color_label')).toHaveLength(0);
expect(body.my_feedback.color_label).toBeFalsy();
await tag('device-A', 'blue');
const after = await feedbackFor('device-B');
const labels = after.feedback.filter((f) => f.feedback_type === 'color_label');
expect(labels.map((f) => f.color_label)).toEqual(['blue']);
});
it('does not answer a guest colour filter from a dormant label', async () => {
await seedDormantPerGuestLabels();
expect(await photosFor('device-A', '?filter=color:green')).toHaveLength(0);
});
it('does not show the shared tag as another guest\'s dot after switching back', async () => {
await tag('device-A', 'green');
await setMode('simple');
// The shared row belongs to nobody, so in a per-guest mode it is not
// "another viewer's label" either — it is simply not in play.
const photo = await photoFor('device-B');
expect(photo.my_color_label).toBeFalsy();
expect(photo.other_color_labels || []).toHaveLength(0);
});
it('recounts the stored per-photo counter when the mode changes', async () => {
// color_label_count is denormalized and recomputed on feedback writes.
// A mode switch changes which rows are live without any write, so
// without an explicit recount the tiles and the admin summary keep
// reporting the old mode's totals until each photo is touched again —
// on a finished gallery, never.
await setMode('simple');
await tag('device-A', 'green');
expect((await db('photos').where('id', photoId).first()).color_label_count).toBe(1);
await feedbackService.updateEventFeedbackSettings(eventId, { identity_mode: 'shared' });
expect((await db('photos').where('id', photoId).first()).color_label_count).toBe(0);
await feedbackService.updateEventFeedbackSettings(eventId, { identity_mode: 'simple' });
expect((await db('photos').where('id', photoId).first()).color_label_count).toBe(1);
});
it('does not count the shared tag as a participant', async () => {
// feedback_count is COUNT(DISTINCT guest identity) and is exported as
// rating_count. The shared tag has a reserved identifier rather than a
// person's, so counting it made tagging a photo look like a second
// guest had left feedback — and inflated the exported rating count.
await request(app)
.post(`/api/gallery/${SLUG}/photos/${photoId}/feedback`)
.set(asGuest('device-A'))
.send({ feedback_type: 'rating', rating: 5 });
expect((await db('photos').where('id', photoId).first()).feedback_count).toBe(1);
await tag('device-B', 'green');
expect((await db('photos').where('id', photoId).first()).feedback_count).toBe(1);
});
it('keeps dormant labels out of the event feedback summary', async () => {
await seedDormantPerGuestLabels();
// Feeds the admin analytics total_feedback and the guest
// /feedback-summary response.
const summary = await feedbackService.getEventFeedbackSummary(eventId);
expect(Number(summary.stats.total_color_labels)).toBe(0);
await tag('device-C', 'blue');
expect(Number((await feedbackService.getEventFeedbackSummary(eventId)).stats.total_color_labels)).toBe(1);
});
it('does not count the shared tag once the event is back on per-guest labels', async () => {
await tag('device-A', 'green');
await setMode('simple');
await feedbackService.updatePhotoFeedbackStats(photoId);
expect((await db('photos').where('id', photoId).first()).color_label_count).toBe(0);
expect(await feedbackService.getPhotoColorLabelCounts(photoId)).toEqual({});
});
});
});
describe('the mode is scoped to the colour tag', () => {
it('keeps likes per-guest in shared mode', async () => {
const like = (ua) => request(app)
.post(`/api/gallery/${SLUG}/photos/${photoId}/feedback`)
.set(asGuest(ua))
.send({ feedback_type: 'like' });
expect((await like('device-A')).status).toBe(200);
expect((await photoFor('device-A')).is_liked).toBe(true);
// B has not liked it. If 'shared' leaked past colour labels, this would
// come back true and B's click would un-like A's like.
expect((await photoFor('device-B')).is_liked).toBe(false);
await like('device-B');
const photo = await db('photos').where('id', photoId).first();
expect(photo.like_count).toBe(2);
});
});
describe('the reserved identity cannot be claimed', () => {
it('refuses a per-guest write that arrives under it', async () => {
// Not reachable through the routes — a guest identifier is either a
// sha256 hex or a server-minted UUID — but a future caller must not be
// able to write the photo's shared tag as if it were their own.
await expect(feedbackService.submitFeedback(
photoId, eventId,
{ feedback_type: 'color_label', color_label: 'green' },
SHARED_COLOR_LABEL_IDENTITY,
)).rejects.toThrow('Reserved guest identifier');
});
});
});
@@ -0,0 +1,53 @@
/**
* Let identity_mode hold 'shared' (#1197).
*
* Migration 078 created the column with a Postgres CHECK constraint pinned to
* the two modes that existed then:
*
* CHECK (identity_mode IN ('simple','guest'))
*
* SQLite never got one — 078 guards that statement on `client === 'pg'` — so
* the third mode saves happily there and fails on Postgres, which is what the
* default production install runs. Widening the validator without this
* migration means the feature cannot be switched on at all in production, and
* no SQLite test can see it.
*
* DROP then ADD rather than an in-place edit: Postgres has no ALTER CONSTRAINT
* for a CHECK, and IF EXISTS makes the pair safe to re-run from either state.
*/
exports.up = async function (knex) {
if (knex.client.config.client !== 'pg') return;
const hasColumn = await knex.schema.hasColumn('event_feedback_settings', 'identity_mode');
if (!hasColumn) return;
await knex.raw('ALTER TABLE event_feedback_settings DROP CONSTRAINT IF EXISTS event_feedback_settings_identity_mode_check');
await knex.raw(`
ALTER TABLE event_feedback_settings
ADD CONSTRAINT event_feedback_settings_identity_mode_check
CHECK (identity_mode IN ('simple','guest','shared'))
`);
};
exports.down = async function (knex) {
if (knex.client.config.client !== 'pg') return;
const hasColumn = await knex.schema.hasColumn('event_feedback_settings', 'identity_mode');
if (!hasColumn) return;
// Any event actually using the mode has to come back to a value the old
// constraint accepts, or the ADD below fails and the down() cannot complete.
// 'simple' rather than 'guest': it is the column default and the mode that
// needs nothing of the guest identity machinery. The shared colour rows stay
// where they are — they simply stop being read, exactly as they do when an
// operator switches the mode by hand.
await knex('event_feedback_settings').where({ identity_mode: 'shared' }).update({ identity_mode: 'simple' });
await knex.raw('ALTER TABLE event_feedback_settings DROP CONSTRAINT IF EXISTS event_feedback_settings_identity_mode_check');
await knex.raw(`
ALTER TABLE event_feedback_settings
ADD CONSTRAINT event_feedback_settings_identity_mode_check
CHECK (identity_mode IN ('simple','guest'))
`);
};
+19
View File
@@ -83,11 +83,30 @@ function dominantColorLabel(counts) {
return best;
}
/**
* The identity a shared colour tag is stored under (#1197).
*
* In identity_mode='shared' the colour label has no owner: one tag per photo,
* any guest can overwrite it. It is still an ordinary photo_feedback row —
* which is what keeps the filters, the per-colour tallies, the moderation
* queue and the XMP/CSV export working unchanged — but its guest_identifier is
* this reserved value rather than a person or a device.
*
* Cannot collide with a real guest. generateGuestIdentifier returns either a
* 64-char sha256 hex or a guest's identifier, and those are crypto.randomUUID()
* values minted server-side and read back from the guests table
* (galleryGuests.js:85, guestAuth.js:57) — never a string the client supplies.
* The write path asserts it anyway: a caller must not reach the shared slot
* except through the shared-mode branch.
*/
const SHARED_COLOR_LABEL_IDENTITY = '__shared__';
module.exports = {
COLOR_LABELS,
COLOR_LABEL_TO_XMP,
COLOR_LABEL_PRIORITY,
KEYBIND_SCHEMES,
SHARED_COLOR_LABEL_IDENTITY,
isValidColorLabel,
dominantColorLabel,
};
+14
View File
@@ -194,6 +194,13 @@ module.exports = (router) => {
require_name_email = false,
moderate_comments = true,
show_feedback_to_guests = true,
// The create form has always shown the identity-mode chooser and this
// route has never read it, so a gallery created as 'guest' quietly came
// out 'simple' and the photographer had to set it again on the event.
// Surfaced by adding a third mode (#1197); the fix is the same for all
// three. Unknown values fall back rather than reaching the column,
// which on Postgres is guarded by a CHECK constraint.
identity_mode: identityModeInput,
// CSS Template
css_template_id = null,
// Hero logo settings
@@ -549,6 +556,9 @@ module.exports = (router) => {
require_name_email: formatBoolean(require_name_email),
moderate_comments: formatBoolean(moderate_comments),
show_feedback_to_guests: formatBoolean(show_feedback_to_guests),
identity_mode: ['simple', 'guest', 'shared'].includes(identityModeInput)
? identityModeInput
: 'simple',
created_at: new Date().toISOString(),
updated_at: new Date().toISOString()
});
@@ -1195,6 +1205,10 @@ module.exports = (router) => {
require_name_email: sourceFeedback.require_name_email,
moderate_comments: sourceFeedback.moderate_comments,
show_feedback_to_guests: sourceFeedback.show_feedback_to_guests,
// Including the identity mode (#1197): a clone that silently came
// back in 'simple' would drop the shared tag on a gallery duplicated
// precisely to reuse its proofing setup.
identity_mode: sourceFeedback.identity_mode || 'simple',
created_at: new Date().toISOString(),
updated_at: new Date().toISOString(),
});
+17 -5
View File
@@ -14,6 +14,7 @@ const { PhotoFilterBuilder } = require('../utils/photoFilterBuilder');
const { getPagination } = require('../utils/routeHelpers');
const { PhotoExportService } = require('../services/photoExportService');
const photoAdminMarksService = require('../services/photoAdminMarksService');
const feedbackService = require('../services/feedbackService');
const logger = require('../utils/logger');
const exportService = new PhotoExportService();
@@ -76,6 +77,11 @@ router.get('/:eventId/filtered', adminAuth, requirePermission('photos.view'), re
const order = req.query.order || 'desc';
const { page, limit } = getPagination(req, { limit: 50 });
// Which colour-label set this event is currently using (#1197). A dormant
// set left behind by a mode switch must not answer a colour filter.
const { identity_mode: identityMode } =
await feedbackService.getEventFeedbackSettings(eventId);
// Build filtered query
const filterBuilder = new PhotoFilterBuilder(
db('photos')
@@ -96,7 +102,8 @@ router.get('/:eventId/filtered', adminAuth, requirePermission('photos.view'), re
'photos.uploaded_at',
'photo_categories.name as category_name'
),
eventId
eventId,
identityMode
);
filterBuilder
@@ -108,13 +115,13 @@ router.get('/:eventId/filtered', adminAuth, requirePermission('photos.view'), re
// Get count of filtered photos
const countResult = await withRetry(() =>
PhotoFilterBuilder.buildCountQuery(db, eventId, filters)
PhotoFilterBuilder.buildCountQuery(db, eventId, filters, identityMode)
);
const filteredCount = parseInt(countResult[0]?.count) || 0;
// Get summary counts
const summary = await withRetry(() =>
PhotoFilterBuilder.getSummary(db, eventId)
PhotoFilterBuilder.getSummary(db, eventId, identityMode)
);
res.json({
@@ -144,9 +151,11 @@ router.get('/:eventId/filtered', adminAuth, requirePermission('photos.view'), re
router.get('/:eventId/filter-summary', adminAuth, requirePermission('photos.view'), requireEventOwnership, async (req, res) => {
try {
const eventId = parseInt(req.params.eventId);
const { identity_mode: identityMode } =
await feedbackService.getEventFeedbackSettings(eventId);
const summary = await withRetry(() =>
PhotoFilterBuilder.getSummary(db, eventId)
PhotoFilterBuilder.getSummary(db, eventId, identityMode)
);
// Per-colour counts for the caller's OWN marks (#1044 follow-up), so the
@@ -199,9 +208,12 @@ router.post('/:eventId/export', adminAuth, requirePermission('photos.download'),
let photoIds = photo_ids;
if (!photoIds && filter) {
const { identity_mode: identityMode } =
await feedbackService.getEventFeedbackSettings(eventId);
const filterBuilder = new PhotoFilterBuilder(
db('photos').select('id'),
eventId
eventId,
identityMode
);
// admin_id comes from the session, so a my-marks filter in the body can
// only ever mean the caller's own marks.
+14 -1
View File
@@ -13,7 +13,7 @@ const {
pickRawDownloadName,
} = require('../services/downloadFilenameService');
const { escapeLikePattern } = require('../utils/sqlSecurity');
const { COLOR_LABELS, dominantColorLabel } = require('../constants/colorLabels');
const { COLOR_LABELS, dominantColorLabel, SHARED_COLOR_LABEL_IDENTITY } = require('../constants/colorLabels');
const feedbackService = require('../services/feedbackService');
const photoAdminMarksService = require('../services/photoAdminMarksService');
const { validateUploadedFiles } = require('../middleware/uploadValidation');
@@ -1157,6 +1157,11 @@ router.get('/:eventId/photos', adminAuth, requirePermission('photos.view'), requ
.map(value => value.trim().toLowerCase())
.filter(value => COLOR_LABELS.includes(value));
if (requestedColorLabels.length > 0) {
// Only the colour-label set the event's mode actually uses (#1197).
// Switching identity_mode leaves the other set in place, and a filter
// that matched it would return photos whose badge shows no such colour.
const { identity_mode: identityMode } =
await feedbackService.getEventFeedbackSettings(eventId);
feedbackConditions.push(qb => qb.whereExists(function () {
this.select('*')
.from('photo_feedback')
@@ -1164,6 +1169,14 @@ router.get('/:eventId/photos', adminAuth, requirePermission('photos.view'), requ
.where('photo_feedback.feedback_type', 'color_label')
.where('photo_feedback.is_hidden', false)
.whereIn('photo_feedback.color_label', requestedColorLabels);
if (identityMode === 'shared') {
this.where('photo_feedback.guest_identifier', SHARED_COLOR_LABEL_IDENTITY);
} else {
this.where(function () {
this.whereNot('photo_feedback.guest_identifier', SHARED_COLOR_LABEL_IDENTITY)
.orWhereNull('photo_feedback.guest_identifier');
});
}
}));
}
// The same filter against the caller's OWN marks (#1044 follow-up).
+70 -7
View File
@@ -31,7 +31,7 @@ const { verifyGalleryAccess, denySlideshowToken, isAdminPreview } = require('../
const withPreview = (req, url) => (req.isAdminPreview ? `${url}${url.includes('?') ? '&' : '?'}admin_preview=1` : url);
const { resolveGuest } = require('../middleware/guestAuth');
const { generateGuestIdentifier } = require('../middleware/feedbackRateLimit');
const { COLOR_LABELS } = require('../constants/colorLabels');
const { COLOR_LABELS, SHARED_COLOR_LABEL_IDENTITY } = require('../constants/colorLabels');
const secureImageService = require('../services/secureImageService');
const logger = require('../utils/logger');
const { pipeStreamToResponse } = require('../utils/streamResponse');
@@ -752,6 +752,10 @@ router.get('/:slug/photos', verifyGalleryAccess, resolveGuest, async (req, res)
const feedbackService = require('../services/feedbackService');
const feedbackSettings = await feedbackService.getEventFeedbackSettings(req.event.id);
const showFeedbackToGuests = isClient || parseBooleanInput(feedbackSettings.show_feedback_to_guests, true);
// One identity-less colour tag per photo, any guest may overwrite it
// (#1197). Read in three places below: the colour filters, the per-viewer
// badge, and the "other viewers" dots that must not double-render it.
const sharedColorMode = feedbackSettings?.identity_mode === 'shared';
// Apply filtering if requested (supports global stats + per-guest interactions)
if (filter) {
@@ -833,6 +837,30 @@ router.get('/:slug/photos', verifyGalleryAccess, resolveGuest, async (req, res)
acc[row.color_label].add(row.photo_id);
return acc;
}, {});
// In shared mode the query above finds nothing for colours — the tag
// is not filed under this viewer — so "my greens" is answered from
// the shared rows instead (#1197). It belongs in the viewer's half
// rather than the aggregate half below, which is gated on
// show_feedback_to_guests: the shared tag is this viewer's tag, and
// a gallery with sharing off would otherwise show colours on the
// tiles while `color:green` returned nothing.
if (sharedColorMode) {
const sharedRows = await db('photo_feedback')
.where({
event_id: req.event.id,
feedback_type: 'color_label',
guest_identifier: SHARED_COLOR_LABEL_IDENTITY,
is_hidden: false,
})
.whereNotNull('color_label')
.select('photo_id', 'color_label');
guestColorLabels = sharedRows.reduce((acc, row) => {
if (!acc[row.color_label]) acc[row.color_label] = new Set();
acc[row.color_label].add(row.photo_id);
return acc;
}, {});
}
}
const includeGuestMatches = (type) => {
@@ -882,10 +910,20 @@ router.get('/:slug/photos', verifyGalleryAccess, resolveGuest, async (req, res)
guestColorLabels?.[color]?.forEach(id => include.add(id));
}
if (showFeedbackToGuests) {
const colorRows = await db('photo_feedback')
const aggregateColors = db('photo_feedback')
.where({ event_id: req.event.id, feedback_type: 'color_label', is_hidden: false })
.whereIn('color_label', requestedColors)
.select('photo_id');
.whereIn('color_label', requestedColors);
// Only the set the mode is actually using (#1197). A dormant
// per-guest label left behind by a switch would otherwise pull its
// photo into a colour filter while the tile shows no such colour.
if (sharedColorMode) {
aggregateColors.where('guest_identifier', SHARED_COLOR_LABEL_IDENTITY);
} else {
aggregateColors.where(function () {
this.whereNot('guest_identifier', SHARED_COLOR_LABEL_IDENTITY).orWhereNull('guest_identifier');
});
}
const colorRows = await aggregateColors.select('photo_id');
colorRows.forEach(row => include.add(row.photo_id));
}
}
@@ -952,8 +990,20 @@ router.get('/:slug/photos', verifyGalleryAccess, resolveGuest, async (req, res)
// above. NOT gated on showFeedbackToGuests: a guest's own label is their
// own selection, not shared aggregate data, and hiding it would blank the
// grid badges on every refresh in a gallery with sharing switched off.
//
// In shared identity mode (#1197) there is no per-viewer label to read:
// the photo carries one tag and it belongs to everyone, so it arrives on
// this same field. The badge, the lightbox swatch and the keyboard
// shortcuts then work unchanged — they were already reading "the colour on
// this photo, from my point of view", which is precisely what the shared
// tag is.
const myColorLabelByPhoto = {};
if (photos.length > 0) {
if (photos.length > 0 && sharedColorMode) {
Object.assign(
myColorLabelByPhoto,
await feedbackService.getSharedColorLabels(req.event.id, photos.map(p => p.id)),
);
} else if (photos.length > 0) {
const colorQuery = db('photo_feedback')
// Same rule as the heart above (#1150).
.where({ event_id: req.event.id, feedback_type: 'color_label', is_hidden: false })
@@ -984,12 +1034,25 @@ router.get('/:slug/photos', verifyGalleryAccess, resolveGuest, async (req, res)
//
// Gated on showFeedbackToGuests, like every other aggregate: this is other
// people's feedback, unlike my_color_label above.
//
// Skipped entirely in shared mode (#1197). There are no other viewers'
// labels there — there is one tag, already delivered as my_color_label
// above. Without this the shared row would come back here too (its
// reserved identity is not the viewer's), and every tile would render the
// same colour twice: once as the badge, once as a dot beside it.
const otherColorLabelsByPhoto = {};
if (photos.length > 0 && showFeedbackToGuests) {
if (photos.length > 0 && showFeedbackToGuests && !sharedColorMode) {
const othersQuery = db('photo_feedback')
.where({ event_id: req.event.id, feedback_type: 'color_label', is_hidden: false })
.whereIn('photo_id', photos.map(p => p.id))
.whereNotNull('color_label');
.whereNotNull('color_label')
// The other direction of the same rule (#1197): an event switched back
// out of shared mode keeps its shared tag, and it is nobody's — so
// without this it would show up as an anonymous other viewer's dot on
// every tile that still carries one.
.where(function () {
this.whereNot('guest_identifier', SHARED_COLOR_LABEL_IDENTITY).orWhereNull('guest_identifier');
});
if (req.guest?.id) {
othersQuery.where(function () {
this.whereNot('guest_id', req.guest.id).orWhereNull('guest_id');
+25 -3
View File
@@ -88,7 +88,9 @@ router.get('/:slug/photos/:photoId/feedback',
// Get feedback based on settings
const options = {
approved_only: true,
include_hidden: false
include_hidden: false,
// Only the colour-label set this event is actually using (#1197).
identity_mode: settings.identity_mode,
};
// Include guest's own feedback even if not approved
@@ -96,9 +98,15 @@ router.get('/:slug/photos/:photoId/feedback',
// Get guest's own feedback separately
const guestFeedback = await feedbackService.getPhotoFeedback(photoId, {
guest_identifier: guestIdentifier
guest_identifier: guestIdentifier,
identity_mode: settings.identity_mode,
});
// The photo's shared tag (#1197), when the event is in that mode.
const sharedColorLabel = settings.identity_mode === 'shared'
? (await feedbackService.getSharedColorLabels(event.id, [photoId]))[photoId] || null
: null;
// Combine and deduplicate
const allFeedback = [...feedback];
guestFeedback.forEach(gf => {
@@ -156,7 +164,17 @@ router.get('/:slug/photos/:photoId/feedback',
liked: !!guestFeedback.find(f => f.feedback_type === 'like'),
favorited: !!guestFeedback.find(f => f.feedback_type === 'favorite'),
reaction: guestFeedback.find(f => f.feedback_type === 'reaction')?.reaction || null,
color_label: guestFeedback.find(f => f.feedback_type === 'color_label')?.color_label || null
// In shared mode the photo's one tag IS this viewer's tag (#1197):
// there is no per-guest row to find, and returning null here would
// leave the lightbox swatch unselected while the photo plainly
// carries a colour. Delivered through my_feedback rather than the
// gated `color_labels` map above on purpose — the shared tag is the
// photo's own state, so it stays visible even with
// show_feedback_to_guests off, while the per-colour tallies (which
// are other people's data) stay hidden.
color_label: settings.identity_mode === 'shared'
? sharedColorLabel
: (guestFeedback.find(f => f.feedback_type === 'color_label')?.color_label || null)
}
});
} catch (error) {
@@ -258,6 +276,10 @@ router.post('/:slug/photos/:photoId/feedback',
comment_text: req.body.comment_text,
reaction: req.body.reaction,
color_label: req.body.color_label,
// Read by the service to route a colour label to the photo's one
// shared slot instead of the guest's own row (#1197). Passed rather
// than re-fetched: the settings are already in hand here.
identity_mode: settings.identity_mode,
guest_name: req.guest?.name ?? req.body.guest_name,
guest_email: req.guest?.email ?? req.body.guest_email,
guest_id: req.guest?.id ?? null,
+338 -12
View File
@@ -2,7 +2,7 @@ const { db, logActivity } = require('../database/db');
const logger = require('../utils/logger');
const { formatBoolean } = require('../utils/dbCompat');
const { REACTION_EMOJIS } = require('../constants/reactions');
const { isValidColorLabel } = require('../constants/colorLabels');
const { isValidColorLabel, SHARED_COLOR_LABEL_IDENTITY } = require('../constants/colorLabels');
const { resolveEventFeedbackDefaults, DEFAULT_KEYBIND_MODE, KEYBIND_MODES } = require('./feedbackDefaults');
// Every writable column on event_feedback_settings (#1030). The admin form
@@ -40,6 +40,46 @@ const SINGLE_VALUE_COLUMNS = {
color_label: 'color_label',
};
/**
* Is this write the identity-less shared colour tag (#1197)?
*
* Narrow on purpose. 'shared' is a mode for the colour tag, not for the event:
* a like or a rating in a shared-mode gallery is still that guest's own, so
* only feedback_type='color_label' takes the reserved slot. Everything else
* falls through to the per-guest path unchanged.
*/
// Set once the settings table has been seen. Module scope on purpose: the
// answer is a property of the schema, not of a request.
let settingsTableKnownToExist = false;
function isSharedColorLabel(identityMode, feedbackType) {
return identityMode === 'shared' && feedbackType === 'color_label';
}
/**
* Narrow a colour-label query to the rows the event's current mode actually
* uses (#1197).
*
* Switching modes is deliberately non-destructive: per-guest labels are kept
* when an event moves to shared, and the shared row is kept when it moves
* back. That leaves both sets in the table at once with only one of them live,
* so every read has to say which it means. Without this the dormant set is
* still counted, still tallied in the lightbox, still matches the admin colour
* filter and can still decide the exported dominant colour — the labels would
* be "hidden" only on the badge, which is not what the settings panel promises.
*
* The NULL arm matters: rows written before migration 078 carry no identifier
* at all, and they are per-guest rows, so they belong to the non-shared set.
*/
function scopeColorLabelsToMode(query, identityMode, column = 'guest_identifier') {
if (identityMode === 'shared') {
return query.where(column, SHARED_COLOR_LABEL_IDENTITY);
}
return query.where(function () {
this.whereNot(column, SHARED_COLOR_LABEL_IDENTITY).orWhereNull(column);
});
}
function pickSettingsColumns(settings) {
const picked = {};
for (const column of FEEDBACK_SETTINGS_COLUMNS) {
@@ -111,6 +151,16 @@ class FeedbackService {
const writable = pickSettingsColumns(settings);
// Changing identity_mode changes which colour labels are live, and
// photos.color_label_count is denormalized — recomputed on a feedback
// write, not on a settings write (#1197). Without this the tiles, the
// admin grid and PhotoFilterBuilder.getSummary keep reporting the
// previous mode's totals until each photo happens to receive another
// mutation, which on a finished gallery is never.
const modeChanged = Object.prototype.hasOwnProperty.call(writable, 'identity_mode')
&& existing
&& (existing.identity_mode || 'simple') !== (writable.identity_mode || 'simple');
if (existing) {
await db('event_feedback_settings')
.where('event_id', eventId)
@@ -127,6 +177,10 @@ class FeedbackService {
});
}
if (modeChanged) {
await this.recountEventColorLabels(eventId, writable.identity_mode || 'simple');
}
await logActivity('feedback_settings_updated', writable, eventId);
return this.getEventFeedbackSettings(eventId);
@@ -162,6 +216,195 @@ class FeedbackService {
return parseInt(result?.count, 10) || 0;
}
/**
* Write the photo's one shared colour tag (#1197).
*
* Last write wins, and re-sending the colour that is already there clears it
* — the same toggle every other colour path uses, and the only way to remove
* a tag without inventing a second control for it. Any guest can do either:
* that is the mode, not a hole in it.
*
* The transaction plus the lock on the photo row is what makes "last write
* wins" mean one value rather than two. Without it, two guests tapping
* different colours in the same instant both read "no tag", both insert, and
* the photo ends up carrying two shared tags at once — which is exactly the
* per-guest tally this mode exists to get rid of. SQLite ignores forUpdate
* but serialises writers anyway; on Postgres it is doing real work.
*
* No guest_name, guest_email or guest_id is stored. Attribution is gone by
* design here — the tag is the photo's state, not a person's opinion — so
* the admin feedback list shows a shared tag with no name against it.
*/
async submitSharedColorLabel(photoId, eventId, colorLabel, { ip_address, user_agent } = {}) {
const nowIso = () => new Date().toISOString();
let outcome;
await db.transaction(async (trx) => {
await trx('photos').where({ id: photoId }).forUpdate().first();
// Visible rows only, like every other single-value path (#1150): a
// hidden tag is the admin's moderation record, and a guest writing over
// it must create a fresh visible row rather than quietly unhide it. The
// unhide path in moderateFeedback collapses the pair back to one.
const sharedScope = () => trx('photo_feedback').where({
photo_id: photoId,
event_id: eventId,
feedback_type: 'color_label',
guest_identifier: SHARED_COLOR_LABEL_IDENTITY,
is_hidden: false,
});
const existing = await sharedScope().first();
if (existing && existing.color_label === colorLabel) {
await sharedScope().delete();
outcome = { removed: true, shared: true };
return;
}
if (existing) {
// Converge on one row, the same defence the per-guest path uses: if a
// race ever did leave duplicates behind, the next tap collapses them.
await sharedScope().whereNot('id', existing.id).delete();
await trx('photo_feedback')
.where('id', existing.id)
.update({ color_label: colorLabel, updated_at: nowIso() });
outcome = { id: existing.id, updated: true, shared: true };
return;
}
const inserted = await trx('photo_feedback').insert({
photo_id: photoId,
event_id: eventId,
feedback_type: 'color_label',
color_label: colorLabel,
guest_identifier: SHARED_COLOR_LABEL_IDENTITY,
guest_id: null,
guest_name: null,
guest_email: null,
ip_address,
user_agent,
is_approved: true,
created_at: nowIso(),
updated_at: nowIso(),
}).returning('id');
outcome = { id: inserted[0]?.id || inserted[0], created: true, shared: true };
// Inside the transaction, while the photo row is still locked (#1197
// review). Recomputing after the commit meant a failure there returned
// 500 for a tag that HAD been written — so the client reverted its
// swatch, the next tap on the same colour toggled the committed tag off
// instead of setting it, and the counters stayed stale meanwhile. It
// also let two concurrent writers race their aggregate updates.
await this.updatePhotoFeedbackStats(photoId, trx);
});
return outcome;
}
/**
* Rebuild photos.color_label_count for a whole event against the set the
* given mode makes live (#1197).
*
* Two statements rather than updatePhotoFeedbackStats per photo: a mode
* switch on a 5,000-photo gallery would otherwise be 5,000 aggregate queries
* and 5,000 updates, for a counter that four of its five components cannot
* have changed. Zero everything first, then write the photos that actually
* carry a live label — usually a small fraction of the event.
*/
async recountEventColorLabels(eventId, identityMode) {
await db.transaction(async (trx) => {
await trx('photos').where({ event_id: eventId }).update({ color_label_count: 0 });
const rows = await scopeColorLabelsToMode(
trx('photo_feedback')
.where({ event_id: eventId, feedback_type: 'color_label', is_hidden: false }),
identityMode,
)
.groupBy('photo_id')
.select('photo_id')
.count('id as count');
for (const row of rows) {
await trx('photos')
.where({ id: row.photo_id })
.update({ color_label_count: Number(row.count) || 0 });
}
});
}
/**
* The identity mode of the event a photo belongs to (#1197).
*
* One small join rather than threading the mode through every caller of
* updatePhotoFeedbackStats — including the duplicate-photo dedupe (#1162),
* which recomputes totals from a background service with no request settings
* in hand. Falls back to 'simple' for a photo whose event has no feedback
* settings row, which is the same default getEventFeedbackSettings applies.
*/
async getIdentityModeForPhoto(photoId, trx = db) {
try {
// Asked BEFORE the join, not recovered from afterwards (#1197 review).
// On Postgres a failed statement aborts the whole transaction, so
// catching the error and carrying on left the caller's trx poisoned:
// the aggregate and update that follow would fail with "current
// transaction is aborted", which is exactly the migration-time path the
// fallback exists to support. A metadata check is safe to ask and does
// not abort anything.
//
// Memoised once true because a table does not un-create itself, and this
// sits on the feedback write path; a false answer is not cached, so a
// migration that creates the table later is picked up.
if (!settingsTableKnownToExist) {
settingsTableKnownToExist = await trx.schema.hasTable('event_feedback_settings');
if (!settingsTableKnownToExist) return 'simple';
}
const row = await trx('photos')
.join('event_feedback_settings', 'photos.event_id', 'event_feedback_settings.event_id')
.where('photos.id', photoId)
.select('event_feedback_settings.identity_mode')
.first();
return row?.identity_mode || 'simple';
} catch (error) {
// updatePhotoFeedbackStats is called from MIGRATIONS as well as from the
// request path — migration 186's duplicate-photo dedupe (#1162)
// recomputes the survivor's totals — and a migration runs against a
// half-built schema where event_feedback_settings need not exist yet.
// Letting that throw took the whole stats update down with it, so the
// reparented rows were never counted.
//
// 'simple' is the right answer in that situation rather than merely a
// safe one: an install with no feedback settings table has no event in
// shared mode, so the non-shared scope is exactly correct.
logger.debug(`Identity mode lookup for photo ${photoId} fell back to 'simple': ${error.message}`);
return 'simple';
}
}
/**
* The shared tag for a set of photos (#1197), as { [photoId]: colour }.
*
* One query for the whole page — the gallery list renders hundreds of tiles
* and a per-photo lookup would be a query each.
*/
async getSharedColorLabels(eventId, photoIds) {
if (!photoIds || photoIds.length === 0) return {};
const rows = await db('photo_feedback')
.where({
event_id: eventId,
feedback_type: 'color_label',
guest_identifier: SHARED_COLOR_LABEL_IDENTITY,
is_hidden: false,
})
.whereIn('photo_id', photoIds)
.whereNotNull('color_label')
.select('photo_id', 'color_label');
const byPhoto = {};
rows.forEach((row) => { byPhoto[row.photo_id] = row.color_label; });
return byPhoto;
}
async submitFeedback(photoId, eventId, feedbackData, guestIdentifier) {
try {
const { feedback_type, rating, comment_text, reaction, color_label, guest_name, guest_email, ip_address, user_agent, guest_id } = feedbackData;
@@ -182,6 +425,24 @@ class FeedbackService {
throw new Error('Invalid color label');
}
// The shared tag (#1197) leaves before any of the per-guest machinery
// below runs: none of it applies to a row that belongs to the photo
// rather than to a person.
if (isSharedColorLabel(feedbackData.identity_mode, feedback_type)) {
return await this.submitSharedColorLabel(photoId, eventId, color_label, {
ip_address,
user_agent,
});
}
// Belt and braces: nothing but the branch above may write the reserved
// slot. If some future caller ever passed it as a guest identifier, a
// per-guest write would land on the photo's shared tag and every guest
// in the gallery would see it as their own.
if (guestIdentifier === SHARED_COLOR_LABEL_IDENTITY) {
throw new Error('Reserved guest identifier');
}
// Rating 0 clears the guest's rating (#884). Only the explicit zero
// sentinel (0, or "0" from callers that skip the route validator's
// toInt) triggers the destructive path — malformed input (undefined,
@@ -396,6 +657,22 @@ class FeedbackService {
query.where('is_approved', true);
}
// Colour labels belong to one of two sets, and only one is live (#1197
// review). Without this the raw feedback list handed back both — dormant
// per-guest rows while the event is in shared mode, and the reserved
// shared row after switching away — even though the settings panel
// promises the other set is not shown. With sharing off it was worse:
// the caller's own dormant row came back flagged is_mine.
if (options.identity_mode) {
query.where(function () {
this.whereNot('feedback_type', 'color_label');
this.orWhere(function () {
this.where('feedback_type', 'color_label');
scopeColorLabelsToMode(this, options.identity_mode);
});
});
}
if (!options.include_hidden) {
query.where('is_hidden', false);
}
@@ -426,6 +703,7 @@ class FeedbackService {
.orderBy('average_rating', 'desc')
.orderBy('like_count', 'desc');
const sharedColors = (await this.getEventFeedbackSettings(eventId)).identity_mode === 'shared';
const totalStats = await db('photo_feedback')
.where('event_id', eventId)
// Hidden rows do not count, the same rule the photo counters above
@@ -440,7 +718,19 @@ class FeedbackService {
db.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as total_comments', ['comment']),
db.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as total_favorites', ['favorite']),
db.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as total_reactions', ['reaction']),
db.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as total_color_labels', ['color_label'])
// Scoped to the live colour-label set (#1197), like every other
// colour read. Unscoped, a dormant set left behind by a mode switch
// inflated total_feedback in the admin analytics and the guest
// /feedback-summary while every other surface hid it.
sharedColors
? db.raw(
'COUNT(CASE WHEN feedback_type = ? AND guest_identifier = ? THEN 1 END) as total_color_labels',
['color_label', SHARED_COLOR_LABEL_IDENTITY],
)
: db.raw(
'COUNT(CASE WHEN feedback_type = ? AND (guest_identifier IS NULL OR guest_identifier <> ?) THEN 1 END) as total_color_labels',
['color_label', SHARED_COLOR_LABEL_IDENTITY],
)
)
.first();
@@ -483,11 +773,19 @@ class FeedbackService {
* Per-colour tallies for one photo (#1044) — the colour-label sibling of
* getPhotoReactionCounts.
*/
async getPhotoColorLabelCounts(photoId) {
async getPhotoColorLabelCounts(photoId, identityMode = undefined) {
try {
const rows = await db('photo_feedback')
.where({ photo_id: photoId, feedback_type: 'color_label' })
.where('is_hidden', false)
// Resolved here rather than pushed onto every caller (#1197): the admin
// grid, the XMP export and the lightbox all reach colour labels through
// this helper and its event-wide sibling, so scoping them at the source
// is what keeps a dormant label out of all three at once.
const mode = identityMode ?? await this.getIdentityModeForPhoto(photoId);
const rows = await scopeColorLabelsToMode(
db('photo_feedback')
.where({ photo_id: photoId, feedback_type: 'color_label' })
.where('is_hidden', false),
mode,
)
.groupBy('color_label')
.select('color_label')
.count('id as count');
@@ -512,11 +810,15 @@ class FeedbackService {
* @param {number[]} [photoIds] - optional narrowing to the visible page
* @returns {Promise<Object>} { [photoId]: { green: 2, red: 1 } }
*/
async getEventColorLabelCounts(eventId, photoIds = null) {
async getEventColorLabelCounts(eventId, photoIds = null, identityMode = undefined) {
try {
const query = db('photo_feedback')
.where({ event_id: eventId, feedback_type: 'color_label' })
.where('is_hidden', false)
const mode = identityMode ?? (await this.getEventFeedbackSettings(eventId)).identity_mode;
const query = scopeColorLabelsToMode(
db('photo_feedback')
.where({ event_id: eventId, feedback_type: 'color_label' })
.where('is_hidden', false),
mode,
)
.groupBy('photo_id', 'color_label')
.select('photo_id', 'color_label')
.count('id as count');
@@ -550,6 +852,20 @@ class FeedbackService {
*/
async updatePhotoFeedbackStats(photoId, trx = db) {
try {
// Which colour labels are live for this photo's event (#1197). The other
// four counters are identity-agnostic; only the colour tag has two
// possible sets sitting in the table at once.
const sharedColors = (await this.getIdentityModeForPhoto(photoId, trx)) === 'shared';
const colorLabelCount = sharedColors
? trx.raw(
'COUNT(CASE WHEN feedback_type = ? AND guest_identifier = ? THEN 1 END) as color_label_count',
['color_label', SHARED_COLOR_LABEL_IDENTITY],
)
: trx.raw(
'COUNT(CASE WHEN feedback_type = ? AND (guest_identifier IS NULL OR guest_identifier <> ?) THEN 1 END) as color_label_count',
['color_label', SHARED_COLOR_LABEL_IDENTITY],
);
// Get aggregated stats
const stats = await trx('photo_feedback')
.where('photo_id', photoId)
@@ -559,9 +875,19 @@ class FeedbackService {
trx.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as like_count', ['like']),
trx.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as favorite_count', ['favorite']),
trx.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as reaction_count', ['reaction']),
trx.raw('COUNT(CASE WHEN feedback_type = ? THEN 1 END) as color_label_count', ['color_label']),
colorLabelCount,
trx.raw('AVG(CASE WHEN feedback_type = ? THEN rating END) as average_rating', ['rating']),
trx.raw('COUNT(DISTINCT COALESCE(CAST(guest_id AS VARCHAR), guest_identifier)) as feedback_count')
// The shared tag is not a participant (#1197). It carries the
// reserved identifier rather than a person's, so counting it here
// added a phantom guest: a photo with one rating and a shared tag
// reported two, and this column is exported as `rating_count` in the
// CSV/JSON export (photoExportService.js) — so merely tagging a
// photo inflated its rating count.
trx.raw(
'COUNT(DISTINCT CASE WHEN guest_identifier IS NULL OR guest_identifier <> ? '
+ 'THEN COALESCE(CAST(guest_id AS VARCHAR), guest_identifier) END) as feedback_count',
[SHARED_COLOR_LABEL_IDENTITY],
)
)
.first();
+6 -2
View File
@@ -210,8 +210,12 @@ const validateFeedbackSettings = [
body('require_name_email').optional().isBoolean(),
body('moderate_comments').optional().isBoolean(),
body('show_feedback_to_guests').optional().isBoolean(),
body('identity_mode').optional().isIn(['simple', 'guest'])
.withMessage('identity_mode must be "simple" or "guest"'),
// 'shared' (#1197) is a third identity model, not a third kind of person:
// it drops the identity dimension from the COLOUR TAG only — one tag per
// photo that any guest can overwrite — and leaves likes, ratings, comments,
// favourites and reactions behaving exactly as in 'simple'.
body('identity_mode').optional().isIn(['simple', 'guest', 'shared'])
.withMessage('identity_mode must be "simple", "guest" or "shared"'),
// Per-guest caps (#655). null / 0 = unlimited; positive integers enforced.
// Upper bound is intentionally generous — operators occasionally run
// "everyone, pick everything you like" galleries.
+39 -14
View File
@@ -3,7 +3,20 @@
* Builds Knex queries for filtering photos by feedback metrics
*/
const { COLOR_LABELS } = require('../constants/colorLabels');
const { COLOR_LABELS, SHARED_COLOR_LABEL_IDENTITY } = require('../constants/colorLabels');
// The colour-label rows the event's current mode actually uses (#1197).
// Switching identity_mode is non-destructive, so an event can hold a dormant
// set alongside the live one; a filter that ignored the distinction would
// match photos on labels the mode does not show.
function scopeColorLabelsToMode(query, identityMode, column = 'photo_feedback.guest_identifier') {
if (identityMode === 'shared') {
return query.where(column, SHARED_COLOR_LABEL_IDENTITY);
}
return query.where(function () {
this.whereNot(column, SHARED_COLOR_LABEL_IDENTITY).orWhereNull(column);
});
}
/**
* Accept a colour filter as an array or a comma-separated string, drop
@@ -21,9 +34,13 @@ function normalizeColorLabels(value) {
}
class PhotoFilterBuilder {
constructor(queryBuilder, eventId) {
constructor(queryBuilder, eventId, identityMode = 'simple') {
this.query = queryBuilder;
this.eventId = eventId;
// Which colour-label set is live (#1197). Passed in rather than looked up:
// applyFilters is synchronous, and every caller already has the event's
// settings in hand.
this.identityMode = identityMode;
}
/**
@@ -85,13 +102,17 @@ class PhotoFilterBuilder {
// photo_feedback_color_label_idx index from migration 180.
const requestedColors = normalizeColorLabels(color_labels);
if (requestedColors.length > 0) {
const identityMode = this.identityMode;
conditions.push(builder => builder.whereExists(function () {
this.select('*')
.from('photo_feedback')
.whereRaw('photo_feedback.photo_id = photos.id')
.where('photo_feedback.feedback_type', 'color_label')
.where('photo_feedback.is_hidden', false)
.whereIn('photo_feedback.color_label', requestedColors);
scopeColorLabelsToMode(
this.select('*')
.from('photo_feedback')
.whereRaw('photo_feedback.photo_id = photos.id')
.where('photo_feedback.feedback_type', 'color_label')
.where('photo_feedback.is_hidden', false)
.whereIn('photo_feedback.color_label', requestedColors),
identityMode,
);
}));
}
@@ -173,10 +194,11 @@ class PhotoFilterBuilder {
/**
* Build a count query for the same filters
*/
static buildCountQuery(db, eventId, filters = {}) {
static buildCountQuery(db, eventId, filters = {}, identityMode = 'simple') {
const builder = new PhotoFilterBuilder(
db('photos').count('* as count'),
eventId
eventId,
identityMode
);
builder.applyFilters(filters);
return builder.getQuery();
@@ -185,7 +207,7 @@ class PhotoFilterBuilder {
/**
* Build a summary query for feedback counts
*/
static async getSummary(db, eventId) {
static async getSummary(db, eventId, identityMode = 'simple') {
const result = await db('photos')
.where('event_id', eventId)
.select(
@@ -200,9 +222,12 @@ class PhotoFilterBuilder {
// Per-colour totals for the filter chips (#1044) — the swatch row shows
// "Green 42" so the photographer knows which colours are worth filtering.
const colorRows = await db('photo_feedback')
.where({ event_id: eventId, feedback_type: 'color_label' })
.where('is_hidden', false)
const colorRows = await scopeColorLabelsToMode(
db('photo_feedback')
.where({ event_id: eventId, feedback_type: 'color_label' })
.where('is_hidden', false),
identityMode,
)
.groupBy('color_label')
.select('color_label')
.countDistinct('photo_id as count');
@@ -1,5 +1,5 @@
import React from 'react';
import { MessageSquare, Star, Heart, Bookmark, Shield, Eye, User, Users, Smile, Palette, Keyboard } from 'lucide-react';
import { MessageSquare, Star, Heart, Bookmark, Shield, Eye, User, Users, Smile, Palette, Keyboard, Tag } from 'lucide-react';
import { Card } from '../common';
import { useTranslation } from 'react-i18next';
import { COLOR_LABELS, COLOR_LABEL_SWATCHES, KEYBIND_SCHEMES, type KeybindMode } from '../../services/feedback.service';
@@ -25,7 +25,7 @@ interface FeedbackSettings {
enable_rate_limiting: boolean;
rate_limit_window_minutes?: number;
rate_limit_max_requests?: number;
identity_mode?: 'simple' | 'guest';
identity_mode?: 'simple' | 'guest' | 'shared';
// Per-guest caps (#655). null/0 = unlimited.
max_favorites_per_guest?: number | null;
max_likes_per_guest?: number | null;
@@ -141,7 +141,50 @@ export const FeedbackSettings: React.FC<FeedbackSettingsProps> = ({
</div>
</div>
</label>
{/* Shared colour tag (#1197). Deliberately worded around what
it changes and what it does not: it drops the identity from
the COLOUR TAG only, and it is the one mode where a guest
can overwrite someone else's mark both of which an
operator has to know before picking it. */}
<label
className={`flex items-start gap-3 p-3 rounded-lg cursor-pointer border transition ${
settings.identity_mode === 'shared'
? 'border-accent-dark bg-accent-dark/15'
: 'border-neutral-200 dark:border-neutral-700 hover:bg-neutral-50 dark:hover:bg-neutral-800'
}`}
>
<input
type="radio"
name="identity_mode"
value="shared"
checked={settings.identity_mode === 'shared'}
onChange={() => onChange({ ...settings, identity_mode: 'shared' })}
className="mt-0.5 w-4 h-4 text-accent focus:ring-primary-500"
/>
<Tag className="w-5 h-5 mt-0.5 text-neutral-600 dark:text-neutral-400" />
<div className="flex-1">
<div className="text-sm font-medium text-neutral-900 dark:text-neutral-100">
{t('feedback.settings.identityModeShared', 'One shared colour tag')}
</div>
<div className="text-xs text-neutral-500 dark:text-neutral-400">
{t(
'feedback.settings.identityModeSharedDesc',
'One colour per photo that everyone sees and anyone can change — for agreeing a single verdict. Likes, ratings and comments stay per-visitor.'
)}
</div>
</div>
</label>
</div>
{settings.identity_mode === 'shared' && (
<p className="text-xs text-amber-600 dark:text-amber-400">
{t(
'feedback.settings.identityModeSharedNote',
'Colour tags in this mode have no author, so the admin view cannot show who set one. Existing per-visitor colour labels are kept but not shown while this mode is on, and come back if you switch away.'
)}
</p>
)}
</div>
<div className="border-t border-neutral-200 dark:border-neutral-700 pt-4" />
@@ -0,0 +1,92 @@
/**
* The identity-mode chooser, and the third option added for #1197.
*
* The shared colour tag is the one mode where a guest can overwrite another
* guest's mark and where the admin loses attribution, so the consequences are
* spelled out in the panel rather than left to the release notes. These tests
* pin that the option exists, that picking it reports the right value, and
* that the warning is tied to the selection rather than always on.
*/
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { describe, expect, it, vi } from 'vitest';
import { FeedbackSettings } from '../FeedbackSettings';
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 baseSettings = {
feedback_enabled: true,
allow_ratings: true,
allow_likes: true,
allow_comments: true,
allow_favorites: true,
allow_reactions: true,
allow_color_labels: true,
require_name_email: false,
moderate_comments: false,
show_feedback_to_guests: true,
enable_rate_limiting: false,
};
const renderPanel = (overrides = {}) => {
const onChange = vi.fn();
render(
<FeedbackSettings
settings={{ ...baseSettings, ...overrides } as any}
onChange={onChange}
/>
);
return { onChange };
};
describe('identity mode chooser', () => {
it('offers all three modes', () => {
renderPanel();
expect(screen.getByRole('radio', { name: /Simple feedback/i })).toBeInTheDocument();
expect(screen.getByRole('radio', { name: /Per-guest selections/i })).toBeInTheDocument();
expect(screen.getByRole('radio', { name: /One shared colour tag/i })).toBeInTheDocument();
});
it('reports the shared mode when it is picked', async () => {
const { onChange } = renderPanel();
await userEvent.click(screen.getByRole('radio', { name: /One shared colour tag/i }));
expect(onChange).toHaveBeenCalledWith(
expect.objectContaining({ identity_mode: 'shared' })
);
});
it('says out loud that the colour tag loses its author', () => {
renderPanel({ identity_mode: 'shared' });
// Attribution disappearing is the point of the mode, not a bug — but an
// operator has to meet that fact before they pick it, not after.
expect(screen.getByText(/cannot show who set one/i)).toBeInTheDocument();
});
it('promises the existing per-visitor labels survive the switch', () => {
renderPanel({ identity_mode: 'shared' });
expect(screen.getByText(/come back if you switch away/i)).toBeInTheDocument();
});
it('keeps the warning out of the way in the other modes', () => {
renderPanel({ identity_mode: 'guest' });
expect(screen.queryByText(/cannot show who set one/i)).not.toBeInTheDocument();
});
it('marks only the selected mode as checked', () => {
renderPanel({ identity_mode: 'shared' });
expect(screen.getByRole('radio', { name: /One shared colour tag/i })).toBeChecked();
expect(screen.getByRole('radio', { name: /Simple feedback/i })).not.toBeChecked();
expect(screen.getByRole('radio', { name: /Per-guest selections/i })).not.toBeChecked();
});
});
@@ -71,7 +71,21 @@ export const PhotoColorLabels: React.FC<PhotoColorLabelsProps> = ({
}
return { previousColor };
},
onSuccess: () => {
onSuccess: (result, data) => {
// Correct the optimistic guess with what the server actually did.
//
// In shared mode the tag belongs to the photo, so another guest can have
// changed it between this viewer's last read and this click (#1197). The
// optimistic branch above assumes the toggle is computed against a
// current value: if this viewer still had green on screen while the tag
// had already become red, clicking green SETS green server-side, but the
// optimistic path read "same colour, so clear" and blanked the swatch.
// The response says which of the two happened, so use it rather than the
// guess. In the per-guest modes the two always agree — only the guest
// themself can move their own label — so this costs nothing there.
if (onColorLabelChange) {
onColorLabelChange(result?.removed ? null : data.color);
}
queryClient.invalidateQueries({ queryKey: ['photo-feedback', gallerySlug, photoId] });
},
onError: (_error, _data, context) => {
@@ -477,7 +477,12 @@ export const PhotoLightbox: React.FC<PhotoLightboxProps> = ({
// a repeat submission off. With nothing set there is nothing to clear.
const value = color ?? myColorLabel;
if (!value) return;
const willBeSet = value === myColorLabel ? null : value;
// What the server did, not what this client guessed. In shared mode the
// tag belongs to the photo and another guest can move it between this
// viewer's last read and this keypress (#1197), so a locally computed
// toggle can blank a swatch the server has just set. The per-guest modes
// always agree with the guess — only the guest can move their own label.
const resolve = (result: any) => (result?.removed ? null : value);
if (isGuestMode && guestIdentity) {
try {
@@ -486,11 +491,11 @@ export const PhotoLightbox: React.FC<PhotoLightboxProps> = ({
return;
}
try {
await feedbackService.submitFeedback(slug, String(currentPhoto.id), {
const result = await feedbackService.submitFeedback(slug, String(currentPhoto.id), {
feedback_type: 'color_label',
color_label: value,
});
setMyColorLabel(willBeSet);
setMyColorLabel(resolve(result));
if (onFeedbackChange) onFeedbackChange();
} catch (err) {
if (handleLimitError(err)) return;
@@ -506,13 +511,13 @@ export const PhotoLightbox: React.FC<PhotoLightboxProps> = ({
return;
}
try {
await feedbackService.submitFeedback(slug, String(currentPhoto.id), {
const result = await feedbackService.submitFeedback(slug, String(currentPhoto.id), {
feedback_type: 'color_label',
color_label: value,
guest_name: savedIdentity?.name,
guest_email: savedIdentity?.email,
});
setMyColorLabel(willBeSet);
setMyColorLabel(resolve(result));
if (onFeedbackChange) onFeedbackChange();
} catch (err) {
if (handleLimitError(err)) return;
@@ -0,0 +1,98 @@
/**
* The colour swatch reconciling with the server's answer (#1197).
*
* In the per-guest modes the optimistic toggle is always right only the
* guest can move their own label. The shared tag belongs to the photo, so
* another guest can change it between this viewer's last read and their click,
* and the optimistic branch ("same colour, so clear it") can then disagree
* with what the server actually did. The response says which happened, so the
* component follows it rather than its guess.
*/
import { render, screen, waitFor } 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 { PhotoColorLabels } from '../PhotoColorLabels';
const submitFeedback = vi.fn();
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' }
})
};
});
vi.mock('../../../services/feedback.service', async () => {
const actual = await vi.importActual<any>('../../../services/feedback.service');
return {
...actual,
feedbackService: { submitFeedback: (...args: any[]) => submitFeedback(...args) }
};
});
vi.mock('react-hot-toast', () => ({
default: { error: vi.fn(), success: vi.fn() },
toast: { error: vi.fn(), success: vi.fn() }
}));
const wrapper = ({ children }: { children: ReactNode }) => {
const client = new QueryClient({ defaultOptions: { queries: { retry: false }, mutations: { retry: false } } });
return <QueryClientProvider client={client}>{children}</QueryClientProvider>;
};
const renderLabels = (myColorLabel: string | null) => {
const onColorLabelChange = vi.fn();
render(
<PhotoColorLabels
gallerySlug="g"
photoId="1"
isEnabled
myColorLabel={myColorLabel as any}
onColorLabelChange={onColorLabelChange}
/>,
{ wrapper }
);
return { onColorLabelChange };
};
const clickGreen = async () => {
const green = screen.getAllByRole('button').find((b) => /green/i.test(b.getAttribute('title') || b.getAttribute('aria-label') || ''));
expect(green).toBeTruthy();
await userEvent.click(green!);
};
describe('shared colour tag reconciliation', () => {
beforeEach(() => submitFeedback.mockReset());
it('keeps the colour when the server says it was set, not cleared', async () => {
// The viewer's screen still says green; the tag had already moved on, so
// the server treats this click as a set rather than a toggle-off.
submitFeedback.mockResolvedValue({ success: true, updated: true });
const { onColorLabelChange } = renderLabels('green');
await clickGreen();
await waitFor(() => {
// The optimistic call blanked it; the server's answer puts it back.
expect(onColorLabelChange).toHaveBeenLastCalledWith('green');
});
});
it('clears the colour when the server says it removed the tag', async () => {
submitFeedback.mockResolvedValue({ success: true, removed: true });
const { onColorLabelChange } = renderLabels('green');
await clickGreen();
await waitFor(() => {
expect(onColorLabelChange).toHaveBeenLastCalledWith(null);
});
});
});
+3
View File
@@ -3732,6 +3732,9 @@
"identityModeSimpleDesc": "Anonym, gerätebasiert. Alle Besucher auf demselben Gerät teilen den Zustand.",
"identityModeGuest": "Gastbezogene Auswahl",
"identityModeGuestDesc": "Jeder Besucher gibt seinen Namen ein. Ermöglicht gastbezogenes Tracking und Admin-Einblicke.",
"identityModeShared": "Ein gemeinsamer Farb-Tag",
"identityModeSharedDesc": "Eine Farbe pro Foto, die alle sehen und jeder ändern kann — für ein gemeinsam abgestimmtes Urteil. Likes, Bewertungen und Kommentare bleiben besucherbezogen.",
"identityModeSharedNote": "Farb-Tags haben in diesem Modus keinen Urheber, die Admin-Ansicht kann also nicht zeigen, wer einen gesetzt hat. Vorhandene besucherbezogene Farbmarkierungen bleiben erhalten, werden in diesem Modus aber nicht angezeigt und kehren beim Wechsel zurück.",
"privacyModeration": "Datenschutz & Moderation",
"requireInfo": "Name & E-Mail erforderlich",
"requireInfoDesc": "Gäste müssen Name und E-Mail angeben, um Feedback zu hinterlassen",
+3
View File
@@ -3753,6 +3753,9 @@
"identityModeSimpleDesc": "Anonymous, device-based. All visitors on the same device share state.",
"identityModeGuest": "Per-guest selections",
"identityModeGuestDesc": "Each visitor enters their name. Enables per-guest tracking and admin insights.",
"identityModeShared": "One shared colour tag",
"identityModeSharedDesc": "One colour per photo that everyone sees and anyone can change — for agreeing a single verdict. Likes, ratings and comments stay per-visitor.",
"identityModeSharedNote": "Colour tags in this mode have no author, so the admin view cannot show who set one. Existing per-visitor colour labels are kept but not shown while this mode is on, and come back if you switch away.",
"privacyModeration": "Privacy & Moderation",
"requireInfo": "Require Name & Email",
"requireInfoDesc": "Guests must provide name and email to leave feedback",
+3
View File
@@ -2460,6 +2460,9 @@
"identityModeSimpleDesc": "Anónimo, basado en dispositivo. Visitantes del mismo dispositivo comparten estado.",
"identityModeGuest": "Por invitado",
"identityModeGuestDesc": "Cada visitante introduce su nombre. Habilita rastreo por invitado e insights admin.",
"identityModeShared": "Una etiqueta de color compartida",
"identityModeSharedDesc": "Un color por foto que todos ven y cualquiera puede cambiar — para acordar un único veredicto. Los me gusta, las valoraciones y los comentarios siguen siendo por visitante.",
"identityModeSharedNote": "Las etiquetas de color no tienen autor en este modo, así que la vista de administración no puede mostrar quién la puso. Las etiquetas por visitante existentes se conservan pero no se muestran mientras este modo esté activo, y vuelven si cambias de modo.",
"colorLabels": "Etiquetas de color",
"colorLabelsDesc": "Un color por invitado y foto, con el conjunto de colores de Lightroom para que la selección se traslade mediante XMP",
"keybindMode": "Atajos de teclado",
+3
View File
@@ -2612,6 +2612,9 @@
"identityModeSimpleDesc": "Anonyme, basé sur l'appareil. Tous les visiteurs sur le même appareil partagent le même état.",
"identityModeGuest": "Sélections par invité",
"identityModeGuestDesc": "Chaque visiteur entre son nom. Permet le suivi par invité et les informations administratives.",
"identityModeShared": "Une étiquette de couleur partagée",
"identityModeSharedDesc": "Une couleur par photo, visible par tous et modifiable par n'importe qui — pour convenir d'un verdict unique. Les mentions j'aime, notes et commentaires restent propres à chaque visiteur.",
"identityModeSharedNote": "Les étiquettes de couleur n'ont pas d'auteur dans ce mode : la vue d'administration ne peut donc pas indiquer qui les a posées. Les couleurs existantes par visiteur sont conservées mais masquées tant que ce mode est actif, et réapparaissent si vous en changez.",
"privacyModeration": "Confidentialité & Modération",
"requireInfo": "Nom et e-mail requis",
"requireInfoDesc": "Les invités doivent fournir leur nom et e-mail pour laisser des commentaires",
+3
View File
@@ -2581,6 +2581,9 @@
"identityModeSimpleDesc": "Anoniem, apparaatgebaseerd. Alle bezoekers op hetzelfde apparaat delen de toestand.",
"identityModeGuest": "Selecties per gast",
"identityModeGuestDesc": "Elke bezoeker voert zijn naam in. Maakt tracking per gast en beheerdersinzichten mogelijk.",
"identityModeShared": "Eén gedeelde kleurtag",
"identityModeSharedDesc": "Eén kleur per foto die iedereen ziet en iedereen kan wijzigen — om samen tot één oordeel te komen. Likes, beoordelingen en reacties blijven per bezoeker.",
"identityModeSharedNote": "Kleurtags hebben in deze modus geen auteur, dus de beheerdersweergave kan niet tonen wie er een heeft gezet. Bestaande kleurlabels per bezoeker blijven bewaard maar worden in deze modus niet getoond, en komen terug als je van modus wisselt.",
"privacyModeration": "Privacy & Moderatie",
"requireInfo": "Naam en e-mail vereisen",
"requireInfoDesc": "Gasten moeten naam en e-mail opgeven om feedback te plaatsen",
+3
View File
@@ -2606,6 +2606,9 @@
"identityModeSimpleDesc": "Anónimo, baseado no dispositivo. Todos os visitantes no mesmo dispositivo partilham o estado.",
"identityModeGuest": "Seleções por convidado",
"identityModeGuestDesc": "Cada visitante introduz o seu nome. Ativa o rastreamento por convidado e informações de administração.",
"identityModeShared": "Uma etiqueta de cor partilhada",
"identityModeSharedDesc": "Uma cor por foto que todos veem e qualquer pessoa pode alterar — para acordar um único veredicto. Gostos, avaliações e comentários continuam a ser por visitante.",
"identityModeSharedNote": "As etiquetas de cor não têm autor neste modo, por isso a vista de administração não consegue mostrar quem a definiu. As etiquetas por visitante existentes são mantidas mas não são mostradas enquanto este modo estiver ativo, e voltam se mudar de modo.",
"privacyModeration": "Privacidade e Moderação",
"requireInfo": "Exigir nome e e-mail",
"requireInfoDesc": "Os convidados devem fornecer nome e e-mail para deixar feedback",
+3
View File
@@ -2631,6 +2631,9 @@
"identityModeSimpleDesc": "Анонимно, на основе устройства. Все посетители на одном устройстве используют общее состояние.",
"identityModeGuest": "Выборки по гостям",
"identityModeGuestDesc": "Каждый посетитель вводит своё имя. Включает отслеживание по гостям и аналитику для администратора.",
"identityModeShared": "Один общий цветовой тег",
"identityModeSharedDesc": "Один цвет на фото, который видят все и может изменить любой — чтобы договориться об общем решении. Лайки, оценки и комментарии остаются индивидуальными.",
"identityModeSharedNote": "У цветовых тегов в этом режиме нет автора, поэтому в админ-панели не видно, кто его поставил. Существующие цветовые метки отдельных посетителей сохраняются, но не отображаются, пока включён этот режим, и возвращаются при его смене.",
"privacyModeration": "Конфиденциальность и модерация",
"requireInfo": "Требовать имя и email",
"requireInfoDesc": "Гости должны указать имя и email для оставления отзывов",
+3
View File
@@ -2601,6 +2601,9 @@
"identityModeSimpleDesc": "Anonimno, vezano na napravo. Vsi obiskovalci na isti napravi delijo stanje.",
"identityModeGuest": "Izbire po gostih",
"identityModeGuestDesc": "Vsak obiskovalec vnese svoje ime. Omogoča sledenje po gostih in vpoglede za administratorja.",
"identityModeShared": "Ena skupna barvna oznaka",
"identityModeSharedDesc": "Ena barva na fotografijo, ki jo vidijo vsi in jo lahko kdorkoli spremeni — za dogovor o enotni oceni. Všečki, ocene in komentarji ostanejo ločeni po obiskovalcu.",
"identityModeSharedNote": "Barvne oznake v tem načinu nimajo avtorja, zato skrbniški pogled ne more pokazati, kdo jo je nastavil. Obstoječe barvne oznake posameznih obiskovalcev se ohranijo, a niso prikazane, dokler je ta način vklopljen, in se vrnejo ob zamenjavi načina.",
"privacyModeration": "Zasebnost in moderiranje",
"requireInfo": "Zahtevaj ime in e-pošto",
"requireInfoDesc": "Gostje morajo za oddajo povratnih informacij navesti ime in e-pošto",
@@ -70,6 +70,7 @@ interface FormData {
require_name_email: boolean;
moderate_comments: boolean;
show_feedback_to_guests: boolean;
identity_mode: 'simple' | 'guest' | 'shared';
enable_rate_limiting: boolean;
rate_limit_window_minutes?: number;
rate_limit_max_requests?: number;
@@ -141,6 +142,7 @@ export const CreateEventPage: React.FC = () => {
require_name_email: false,
moderate_comments: true,
show_feedback_to_guests: true,
identity_mode: 'simple',
enable_rate_limiting: true,
rate_limit_window_minutes: 15,
rate_limit_max_requests: 10,
@@ -502,6 +504,9 @@ export const CreateEventPage: React.FC = () => {
require_name_email: feedbackSettings.require_name_email,
moderate_comments: feedbackSettings.moderate_comments,
show_feedback_to_guests: feedbackSettings.show_feedback_to_guests,
// The chooser has always been on this form; the value was never sent, so
// the gallery came out in the default mode whatever was picked (#1197).
identity_mode: feedbackSettings.identity_mode,
// Client access (#172)
client_access_enabled: formData.client_access_enabled,
client_password: formData.client_access_enabled ? formData.client_password : undefined,
+1 -1
View File
@@ -1,6 +1,6 @@
import { api } from '../config/api';
export type IdentityMode = 'simple' | 'guest';
export type IdentityMode = 'simple' | 'guest' | 'shared';
// Emoji reactions (#839): the fixed curated set. Mirrored in
// backend/src/constants/reactions.js — update both together.