feat(gallery): colour labels for client proofing, and one global default per feedback type (#1044) (#1137)
Colour labels for client proofing, plus the photographer's own stars and colours in the admin grid. - Guest colour labels alongside likes/reactions, opt-in per event (defaults off so live galleries do not change mid-proofing), with 'colors' and 'lightroom' keybind schemes. - One global default per feedback type, replacing the per-type scatter. - Admin marks live in their own table (photo_admin_marks) so they can never reach a guest-facing surface. - XMP export prefers a real label, keeping the rating-derived mapping as a fallback. Review: concurrent-write loss on the mark update path, migration index idempotency and error classification all fixed in 7139bcae; migrations renumbered to 182/183 in 8fecdfae after 180/181 were taken on main. Merged with admin privileges: bypass-size-gate is a required check that fails on size alone for review-bypass authors and never re-evaluates on review, which is its designed behaviour once a maintainer has approved.
This commit is contained in:
Luca
@@ -0,0 +1,176 @@
|
||||
/**
|
||||
* Global guest-feedback defaults (#1044).
|
||||
*
|
||||
* Before this module the creation defaults lived in four places that had
|
||||
* already drifted: the admin create route's destructuring defaults, the v1
|
||||
* create route's hard-coded insert (which omitted `allow_reactions`
|
||||
* entirely), getEventFeedbackSettings()'s no-row fallback, and the migration
|
||||
* column defaults. Pinned here:
|
||||
*
|
||||
* - with no app_settings rows, the built-in fallbacks apply (so installs
|
||||
* that never open Settings > Events keep behaving exactly as before)
|
||||
* - a stored global overrides its fallback, whether it was written as a JSON
|
||||
* boolean, a bare "true"/"false" string, or SQLite's 0/1
|
||||
* - garbage in a settings row falls back rather than writing null into a
|
||||
* boolean column
|
||||
* - an explicitly-sent body value always beats the global
|
||||
* - keybind_mode only accepts the known schemes
|
||||
* - getEventFeedbackSettings()'s no-row fallback reads the same globals
|
||||
*/
|
||||
|
||||
const path = require('path');
|
||||
const fs = require('fs');
|
||||
const os = require('os');
|
||||
|
||||
process.env.NODE_ENV = 'test';
|
||||
process.env.TEST_DATABASE_PATH = path.join(
|
||||
fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-feedback-defaults-')), 'db.sqlite',
|
||||
);
|
||||
process.env.JWT_SECRET = process.env.JWT_SECRET || 'feedback-defaults-test-secret';
|
||||
|
||||
const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb');
|
||||
|
||||
const {
|
||||
FEEDBACK_TOGGLES,
|
||||
resolveEventFeedbackDefaults,
|
||||
applyFeedbackDefaults,
|
||||
} = require('../../src/services/feedbackDefaults');
|
||||
const feedbackService = require('../../src/services/feedbackService');
|
||||
|
||||
let db;
|
||||
let cleanup;
|
||||
|
||||
async function setGlobal(key, value) {
|
||||
await db('app_settings').where('setting_key', key).delete();
|
||||
await db('app_settings').insert({
|
||||
setting_key: key,
|
||||
setting_value: value,
|
||||
setting_type: 'general',
|
||||
updated_at: new Date().toISOString(),
|
||||
});
|
||||
}
|
||||
|
||||
async function clearGlobals() {
|
||||
await db('app_settings').where('setting_key', 'like', 'event_default_%').delete();
|
||||
}
|
||||
|
||||
beforeAll(async () => {
|
||||
({ db, cleanup } = await bootCrmDb());
|
||||
await seedMinimal(db);
|
||||
}, 120000);
|
||||
|
||||
afterAll(async () => {
|
||||
if (cleanup) await cleanup();
|
||||
});
|
||||
|
||||
beforeEach(async () => {
|
||||
await clearGlobals();
|
||||
});
|
||||
|
||||
describe('resolveEventFeedbackDefaults (#1044)', () => {
|
||||
it('falls back to the built-ins when nothing is configured', async () => {
|
||||
const defaults = await resolveEventFeedbackDefaults();
|
||||
expect(defaults).toEqual({
|
||||
allow_ratings: true,
|
||||
allow_likes: true,
|
||||
allow_favorites: true,
|
||||
allow_comments: true,
|
||||
allow_reactions: true,
|
||||
// Colour labels are opt-in so no existing gallery grows a colour bar
|
||||
// on upgrade.
|
||||
allow_color_labels: false,
|
||||
keybind_mode: 'colors',
|
||||
});
|
||||
});
|
||||
|
||||
it('covers every toggle the UI shows — no type can be silently missing', async () => {
|
||||
const defaults = await resolveEventFeedbackDefaults();
|
||||
for (const toggle of FEEDBACK_TOGGLES) {
|
||||
expect(defaults).toHaveProperty(toggle.column);
|
||||
}
|
||||
});
|
||||
|
||||
it('lets a stored global override its fallback', async () => {
|
||||
await setGlobal('event_default_allow_comments', JSON.stringify(false));
|
||||
await setGlobal('event_default_allow_color_labels', JSON.stringify(true));
|
||||
|
||||
const defaults = await resolveEventFeedbackDefaults();
|
||||
expect(defaults.allow_comments).toBe(false);
|
||||
expect(defaults.allow_color_labels).toBe(true);
|
||||
expect(defaults.allow_likes).toBe(true); // untouched
|
||||
});
|
||||
|
||||
it('accepts the value shapes older writers left behind', async () => {
|
||||
await setGlobal('event_default_allow_likes', 'false'); // bare string, not JSON-quoted
|
||||
await setGlobal('event_default_allow_ratings', '0'); // SQLite boolean
|
||||
const defaults = await resolveEventFeedbackDefaults();
|
||||
expect(defaults.allow_likes).toBe(false);
|
||||
expect(defaults.allow_ratings).toBe(false);
|
||||
});
|
||||
|
||||
it('falls back rather than writing garbage into a boolean column', async () => {
|
||||
await setGlobal('event_default_allow_favorites', 'not-a-boolean');
|
||||
const defaults = await resolveEventFeedbackDefaults();
|
||||
expect(defaults.allow_favorites).toBe(true);
|
||||
});
|
||||
|
||||
it('only accepts known keybind schemes', async () => {
|
||||
await setGlobal('event_default_keybind_mode', JSON.stringify('lightroom'));
|
||||
expect((await resolveEventFeedbackDefaults()).keybind_mode).toBe('lightroom');
|
||||
|
||||
await setGlobal('event_default_keybind_mode', JSON.stringify('vim'));
|
||||
expect((await resolveEventFeedbackDefaults()).keybind_mode).toBe('colors');
|
||||
});
|
||||
});
|
||||
|
||||
describe('applyFeedbackDefaults (#1044)', () => {
|
||||
const globals = {
|
||||
allow_ratings: true,
|
||||
allow_likes: true,
|
||||
allow_favorites: true,
|
||||
allow_comments: true,
|
||||
allow_reactions: true,
|
||||
allow_color_labels: false,
|
||||
keybind_mode: 'colors',
|
||||
};
|
||||
|
||||
it('inherits every value the caller omitted', () => {
|
||||
expect(applyFeedbackDefaults({}, globals)).toEqual(globals);
|
||||
expect(applyFeedbackDefaults(undefined, globals)).toEqual(globals);
|
||||
});
|
||||
|
||||
it('lets an explicit body value win — including an explicit false', () => {
|
||||
const resolved = applyFeedbackDefaults(
|
||||
{ allow_likes: false, allow_color_labels: true, keybind_mode: 'lightroom' },
|
||||
globals,
|
||||
);
|
||||
expect(resolved.allow_likes).toBe(false);
|
||||
expect(resolved.allow_color_labels).toBe(true);
|
||||
expect(resolved.keybind_mode).toBe('lightroom');
|
||||
expect(resolved.allow_ratings).toBe(true); // still inherited
|
||||
});
|
||||
|
||||
it('ignores an unknown keybind mode from the body', () => {
|
||||
expect(applyFeedbackDefaults({ keybind_mode: 'emacs' }, globals).keybind_mode).toBe('colors');
|
||||
});
|
||||
|
||||
it('does not mutate the globals object it was handed', () => {
|
||||
const snapshot = { ...globals };
|
||||
applyFeedbackDefaults({ allow_likes: false }, globals);
|
||||
expect(globals).toEqual(snapshot);
|
||||
});
|
||||
});
|
||||
|
||||
describe('getEventFeedbackSettings no-row fallback (#1044)', () => {
|
||||
it('reads the same globals instead of a fourth hard-coded copy', async () => {
|
||||
await setGlobal('event_default_allow_comments', JSON.stringify(false));
|
||||
await setGlobal('event_default_allow_color_labels', JSON.stringify(true));
|
||||
|
||||
// An event id with no event_feedback_settings row.
|
||||
const settings = await feedbackService.getEventFeedbackSettings(999999);
|
||||
expect(settings.feedback_enabled).toBe(false); // no row still means feedback off
|
||||
expect(settings.allow_comments).toBe(false);
|
||||
expect(settings.allow_color_labels).toBe(true);
|
||||
expect(settings.keybind_mode).toBe('colors');
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,241 @@
|
||||
/**
|
||||
* The photographer's own stars and colour labels (#1044 follow-up).
|
||||
*
|
||||
* The property that matters most here is isolation: admin marks live in their
|
||||
* own table precisely so they can never reach a guest-facing surface. Pinned:
|
||||
*
|
||||
* - rating and colour are independent halves of one mark — writing one must
|
||||
* not wipe the other
|
||||
* - a mark with neither half left is deleted, not kept as an empty row
|
||||
* - two admins on the same event keep separate marks
|
||||
* - marks never touch photo_feedback or the denormalized photos.* counters
|
||||
* the gallery reads
|
||||
* - invalid values are rejected the same way guest colour labels are
|
||||
*/
|
||||
|
||||
const path = require('path');
|
||||
const fs = require('fs');
|
||||
const os = require('os');
|
||||
|
||||
process.env.NODE_ENV = 'test';
|
||||
process.env.TEST_DATABASE_PATH = path.join(
|
||||
fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-admin-marks-')), 'db.sqlite',
|
||||
);
|
||||
process.env.JWT_SECRET = process.env.JWT_SECRET || 'admin-marks-test-secret';
|
||||
|
||||
const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb');
|
||||
|
||||
const marks = require('../../src/services/photoAdminMarksService');
|
||||
const feedbackService = require('../../src/services/feedbackService');
|
||||
|
||||
const ADMIN_A = 11;
|
||||
const ADMIN_B = 22;
|
||||
|
||||
let db;
|
||||
let cleanup;
|
||||
let eventId;
|
||||
let photoIds;
|
||||
|
||||
beforeAll(async () => {
|
||||
({ db, cleanup } = await bootCrmDb());
|
||||
await seedMinimal(db);
|
||||
const inserted = await db('events').insert({
|
||||
slug: 'admin-marks-test-event',
|
||||
event_type: 'wedding',
|
||||
event_name: 'Admin Marks Test',
|
||||
event_date: '2026-07-20',
|
||||
host_email: '[email protected]',
|
||||
admin_email: '[email protected]',
|
||||
password_hash: 'x',
|
||||
share_link: '/gallery/admin-marks-test-event/share',
|
||||
share_token: 'admin-marks-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 = inserted[0]?.id ?? inserted[0];
|
||||
|
||||
photoIds = [];
|
||||
for (let i = 0; i < 3; i++) {
|
||||
const photo = await db('photos').insert({
|
||||
event_id: eventId,
|
||||
filename: `photo-${i}.jpg`,
|
||||
path: `events/admin-marks/${i}.jpg`,
|
||||
type: 'individual',
|
||||
uploaded_at: new Date().toISOString(),
|
||||
}).returning('id');
|
||||
photoIds.push(photo[0]?.id ?? photo[0]);
|
||||
}
|
||||
}, 120000);
|
||||
|
||||
afterAll(async () => {
|
||||
if (cleanup) await cleanup();
|
||||
});
|
||||
|
||||
describe('photoAdminMarksService (#1044 follow-up)', () => {
|
||||
it('sets a colour without touching the rating, and vice versa', async () => {
|
||||
expect(await marks.setMark(eventId, photoIds[0], ADMIN_A, { colorLabel: 'green' }))
|
||||
.toEqual({ rating: null, color_label: 'green' });
|
||||
|
||||
// Writing the rating must leave the colour alone — the two lightbox key
|
||||
// groups write independently.
|
||||
expect(await marks.setMark(eventId, photoIds[0], ADMIN_A, { rating: 4 }))
|
||||
.toEqual({ rating: 4, color_label: 'green' });
|
||||
|
||||
expect(await marks.setMark(eventId, photoIds[0], ADMIN_A, { colorLabel: 'red' }))
|
||||
.toEqual({ rating: 4, color_label: 'red' });
|
||||
});
|
||||
|
||||
it('keeps exactly one row per photo per admin', async () => {
|
||||
const rows = await db('photo_admin_marks')
|
||||
.where({ photo_id: photoIds[0], admin_id: ADMIN_A });
|
||||
expect(rows).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('clears one half with null and leaves the other', async () => {
|
||||
expect(await marks.setMark(eventId, photoIds[0], ADMIN_A, { colorLabel: null }))
|
||||
.toEqual({ rating: 4, color_label: null });
|
||||
});
|
||||
|
||||
it('deletes the row once neither half is left', async () => {
|
||||
expect(await marks.setMark(eventId, photoIds[0], ADMIN_A, { rating: null })).toBeNull();
|
||||
const rows = await db('photo_admin_marks')
|
||||
.where({ photo_id: photoIds[0], admin_id: ADMIN_A });
|
||||
expect(rows).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('keeps two admins on the same photo independent', async () => {
|
||||
await marks.setMark(eventId, photoIds[1], ADMIN_A, { colorLabel: 'green' });
|
||||
await marks.setMark(eventId, photoIds[1], ADMIN_B, { colorLabel: 'red', rating: 2 });
|
||||
|
||||
expect((await marks.getEventMarks(eventId, ADMIN_A))[photoIds[1]])
|
||||
.toEqual({ rating: null, color_label: 'green' });
|
||||
expect((await marks.getEventMarks(eventId, ADMIN_B))[photoIds[1]])
|
||||
.toEqual({ rating: 2, color_label: 'red' });
|
||||
});
|
||||
|
||||
it('rejects colours outside the set and ratings outside 1-5', async () => {
|
||||
await expect(marks.setMark(eventId, photoIds[2], ADMIN_A, { colorLabel: 'chartreuse' }))
|
||||
.rejects.toThrow('Invalid color label');
|
||||
await expect(marks.setMark(eventId, photoIds[2], ADMIN_A, { rating: 6 }))
|
||||
.rejects.toThrow('Rating must be between 1 and 5');
|
||||
await expect(marks.setMark(eventId, photoIds[2], ADMIN_A, { rating: 0 }))
|
||||
.rejects.toThrow('Rating must be between 1 and 5');
|
||||
// Nothing was written by any of the rejected calls.
|
||||
expect(await marks.getEventMarks(eventId, ADMIN_A, [photoIds[2]])).toEqual({});
|
||||
});
|
||||
|
||||
it('tags caller mistakes with a code, not just a message', async () => {
|
||||
// The route maps this to a 400. Keyed on the code so rewording the
|
||||
// message can't silently turn a bad request into a 500.
|
||||
await expect(marks.setMark(eventId, photoIds[2], ADMIN_A, { rating: 9 }))
|
||||
.rejects.toMatchObject({ code: marks.INVALID_MARK });
|
||||
await expect(marks.setMark(eventId, photoIds[2], ADMIN_A, { colorLabel: 'beige' }))
|
||||
.rejects.toMatchObject({ code: marks.INVALID_MARK });
|
||||
});
|
||||
|
||||
it('narrows to a page of photo ids', async () => {
|
||||
expect(Object.keys(await marks.getEventMarks(eventId, ADMIN_A, [photoIds[1]])))
|
||||
.toEqual([String(photoIds[1])]);
|
||||
expect(await marks.getEventMarks(eventId, ADMIN_A, [])).toEqual({});
|
||||
});
|
||||
|
||||
it('converges instead of 500ing when two marks race into the same row', async () => {
|
||||
// Simulate the check-then-insert race a keyboard triage pass produces:
|
||||
// both calls read "no row", one inserts, the other hits the unique index.
|
||||
// The loser must land on the winner's row, not throw — and the half it
|
||||
// did not address must keep the winner's value, not the stale pre-read.
|
||||
const photoId = photoIds[2];
|
||||
await db('photo_admin_marks').where({ photo_id: photoId }).delete();
|
||||
|
||||
const [first, second] = await Promise.all([
|
||||
marks.setMark(eventId, photoId, ADMIN_A, { rating: 4 }),
|
||||
marks.setMark(eventId, photoId, ADMIN_A, { colorLabel: 'green' }),
|
||||
]);
|
||||
|
||||
expect(first).not.toBeNull();
|
||||
expect(second).not.toBeNull();
|
||||
|
||||
const rows = await db('photo_admin_marks').where({ photo_id: photoId, admin_id: ADMIN_A });
|
||||
expect(rows).toHaveLength(1);
|
||||
// Whichever order they landed in, neither half may be lost.
|
||||
expect(rows[0].rating).toBe(4);
|
||||
expect(rows[0].color_label).toBe('green');
|
||||
|
||||
await db('photo_admin_marks').where({ photo_id: photoId }).delete();
|
||||
});
|
||||
|
||||
it('does not lose a concurrent half when the row already exists', async () => {
|
||||
// luap's finding on #1137: the insert race was handled, but two keystrokes
|
||||
// on a photo that ALREADY has a mark took a plain read-modify-write —
|
||||
// both calls read the old row, both wrote the full pair, and the second
|
||||
// clobbered the first with its stale value. That is the COMMON case in a
|
||||
// triage pass, since by the second keystroke there usually is a row.
|
||||
const photoId = photoIds[2];
|
||||
await db('photo_admin_marks').where({ photo_id: photoId }).delete();
|
||||
await marks.setMark(eventId, photoId, ADMIN_A, { rating: 3 });
|
||||
|
||||
await Promise.all([
|
||||
marks.setMark(eventId, photoId, ADMIN_A, { rating: 4 }),
|
||||
marks.setMark(eventId, photoId, ADMIN_A, { colorLabel: 'blue' }),
|
||||
]);
|
||||
|
||||
const row = await db('photo_admin_marks')
|
||||
.where({ photo_id: photoId, admin_id: ADMIN_A }).first();
|
||||
expect(row.rating).toBe(4); // was 3 before the fix — the colour
|
||||
expect(row.color_label).toBe('blue'); // write reverted it
|
||||
|
||||
await db('photo_admin_marks').where({ photo_id: photoId }).delete();
|
||||
});
|
||||
|
||||
it('leaves the untouched half alone even when the caller sends only one', async () => {
|
||||
// The mechanism behind the fix: an unaddressed column is never written,
|
||||
// so it cannot be written with a stale value.
|
||||
const photoId = photoIds[2];
|
||||
await db('photo_admin_marks').where({ photo_id: photoId }).delete();
|
||||
await marks.setMark(eventId, photoId, ADMIN_A, { rating: 2, colorLabel: 'purple' });
|
||||
|
||||
const before = await db('photo_admin_marks')
|
||||
.where({ photo_id: photoId, admin_id: ADMIN_A }).first();
|
||||
|
||||
await marks.setMark(eventId, photoId, ADMIN_A, { colorLabel: 'green' });
|
||||
const after = await db('photo_admin_marks')
|
||||
.where({ photo_id: photoId, admin_id: ADMIN_A }).first();
|
||||
|
||||
expect(after.rating).toBe(before.rating);
|
||||
expect(after.color_label).toBe('green');
|
||||
|
||||
await db('photo_admin_marks').where({ photo_id: photoId }).delete();
|
||||
});
|
||||
|
||||
it('counts colours per admin for the filter chips', async () => {
|
||||
await marks.setMark(eventId, photoIds[2], ADMIN_A, { colorLabel: 'green' });
|
||||
expect(await marks.getEventMarkColorCounts(eventId, ADMIN_A)).toEqual({ green: 2 });
|
||||
expect(await marks.getEventMarkColorCounts(eventId, ADMIN_B)).toEqual({ red: 1 });
|
||||
// A rating-only mark contributes no colour count.
|
||||
await marks.setMark(eventId, photoIds[0], ADMIN_A, { rating: 5 });
|
||||
expect(await marks.getEventMarkColorCounts(eventId, ADMIN_A)).toEqual({ green: 2 });
|
||||
});
|
||||
|
||||
it('never leaks into guest feedback or the denormalized gallery counters', async () => {
|
||||
// Everything above wrote marks on all three photos.
|
||||
expect(await db('photo_feedback').count('* as count').first())
|
||||
.toEqual(expect.objectContaining({ count: 0 }));
|
||||
|
||||
for (const photoId of photoIds) {
|
||||
// updatePhotoFeedbackStats is what the gallery payload reads; it must
|
||||
// still see an untouched photo.
|
||||
await feedbackService.updatePhotoFeedbackStats(photoId);
|
||||
const photo = await db('photos').where('id', photoId).first();
|
||||
expect(Number(photo.color_label_count) || 0).toBe(0);
|
||||
expect(Number(photo.average_rating) || 0).toBe(0);
|
||||
expect(Number(photo.feedback_count) || 0).toBe(0);
|
||||
}
|
||||
|
||||
// And the guest-facing tallies stay empty.
|
||||
expect(await feedbackService.getPhotoColorLabelCounts(photoIds[1])).toEqual({});
|
||||
expect(await feedbackService.getEventColorLabelCounts(eventId)).toEqual({});
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user