fix(feedback): name the camera original in the exports, not just the stored file (#1224) (#1228)

Both feedback exports carried only `photos.filename` — the sanitized stored
name (`wedding-smith_individual_1755892345.jpg`). Acting on client picks means
finding the master on disk, and that name matches nothing in a Lightroom
catalog, so the export could not be joined to anything.

Adds the camera-original name to the long and pivot shapes, and by extension
to the archive's feedback_data.csv/.json, which reuse the same query.

COALESCE(source_filename, original_filename), not original_filename alone:
the latter is overwritten the first time an edited render is uploaded over a
proof (#745), so an export taken after a round-trip would name the render
rather than the master and silently stop matching. source_filename is written
once at ingest and survives a replace by design (migration 193). That case is
the load-bearing test.

Aliased to `original_filename` — the name the sibling photo export already
uses for this column, and the question the reader is asking. Left empty when
neither is known rather than echoing the stored name: blank reads as "no match
possible", where repeating the sanitized name invites a match attempt against
a file that does not exist under it.

The column is added, not swapped: `filename` is untouched, so anything reading
the old column keeps working.

Reported by the 8digit/picpeak fork, which has carried a narrower version of
this patch (original_filename only) across rebases.

Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
This commit is contained in:
Paul Nothaft
2026-08-29 11:09:59 +02:00
committed by GitHub
parent a8a8b7cc64
commit 4f684eb482
2 changed files with 183 additions and 0 deletions
@@ -0,0 +1,156 @@
/**
* Feedback exports carry the camera-original filename (#1224).
*
* Both exports used to select only `photos.filename` — the sanitized stored
* name — so a photographer acting on client picks had nothing to match the
* masters on disk with.
*
* The load-bearing case is the third one: `original_filename` is overwritten
* the first time an edited render is uploaded over a proof (#745), so an
* export that read it alone would name the render rather than the master and
* silently stop matching. `source_filename` survives a replace by design
* (migration 193) and must win.
*/
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-export-camera-')), 'db.sqlite',
);
process.env.JWT_SECRET = process.env.JWT_SECRET || 'feedback-export-camera-test-secret';
const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb');
const feedbackService = require('../../src/services/feedbackService');
const EVENT_SLUG = 'feedback-export-camera-event';
const GUEST = 'guest-camera-identifier';
let db;
let cleanup;
let eventId;
// filename is always the sanitized stored name; the other two vary per case.
async function addPhoto({ filename, originalFilename, sourceFilename }) {
const inserted = await db('photos').insert({
event_id: eventId,
filename,
original_filename: originalFilename,
source_filename: sourceFilename,
path: `events/camera-name/${filename}`,
type: 'individual',
uploaded_at: new Date().toISOString(),
}).returning('id');
return inserted[0]?.id ?? inserted[0];
}
async function like(photoId) {
return feedbackService.submitFeedback(photoId, eventId, {
feedback_type: 'like',
guest_id: null,
ip_address: '127.0.0.1',
user_agent: 'jest',
}, GUEST);
}
beforeAll(async () => {
({ db, cleanup } = await bootCrmDb());
await seedMinimal(db);
const inserted = await db('events').insert({
slug: EVENT_SLUG,
event_type: 'wedding',
event_name: 'Feedback Export Camera Name',
event_date: '2026-08-28',
host_email: 'host@example.com',
admin_email: 'admin@example.com',
password_hash: 'x',
share_link: `/gallery/${EVENT_SLUG}/share`,
share_token: 'feedback-export-camera-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];
}, 120000);
afterAll(async () => {
if (cleanup) await cleanup();
});
describe('feedback exports carry the camera-original filename (#1224)', () => {
it('reports the camera name alongside the stored name, in both shapes', async () => {
const photoId = await addPhoto({
filename: 'wedding-smith_individual_1755892345.jpg',
originalFilename: 'IMG_1234.JPG',
sourceFilename: 'IMG_1234.JPG',
});
await like(photoId);
const [longRow] = (await feedbackService.exportEventFeedback(eventId))
.filter((r) => r.filename === 'wedding-smith_individual_1755892345.jpg');
expect(longRow.original_filename).toBe('IMG_1234.JPG');
// The stored name is still there — this adds a column, it does not swap one.
expect(longRow.filename).toBe('wedding-smith_individual_1755892345.jpg');
const [pivotRow] = (await feedbackService.exportEventFeedbackPivoted(eventId))
.filter((r) => r.filename === 'wedding-smith_individual_1755892345.jpg');
expect(pivotRow.original_filename).toBe('IMG_1234.JPG');
});
it('keeps naming the master after a replace overwrote original_filename', async () => {
// Exactly the post-round-trip row: the edited render's name landed in
// original_filename, source_filename still holds what the camera wrote.
const photoId = await addPhoto({
filename: 'wedding-smith_individual_1755892999.jpg',
originalFilename: 'Smith_Wedding_1234.jpg',
sourceFilename: 'IMG_1234.JPG',
});
await like(photoId);
const [longRow] = (await feedbackService.exportEventFeedback(eventId))
.filter((r) => r.filename === 'wedding-smith_individual_1755892999.jpg');
expect(longRow.original_filename).toBe('IMG_1234.JPG');
const [pivotRow] = (await feedbackService.exportEventFeedbackPivoted(eventId))
.filter((r) => r.filename === 'wedding-smith_individual_1755892999.jpg');
expect(pivotRow.original_filename).toBe('IMG_1234.JPG');
});
it('falls back to original_filename when nothing was captured at ingest', async () => {
const photoId = await addPhoto({
filename: 'wedding-smith_individual_1755893111.jpg',
originalFilename: 'DSC_9001.NEF',
sourceFilename: null,
});
await like(photoId);
const [longRow] = (await feedbackService.exportEventFeedback(eventId))
.filter((r) => r.filename === 'wedding-smith_individual_1755893111.jpg');
expect(longRow.original_filename).toBe('DSC_9001.NEF');
});
it('leaves the column empty rather than repeating the sanitized name', async () => {
// Blank reads as "no match possible". Echoing the stored name would invite
// a match attempt against a file that does not exist under that name.
const photoId = await addPhoto({
filename: 'wedding-smith_individual_1755893222.jpg',
originalFilename: null,
sourceFilename: null,
});
await like(photoId);
const [longRow] = (await feedbackService.exportEventFeedback(eventId))
.filter((r) => r.filename === 'wedding-smith_individual_1755893222.jpg');
expect(longRow.original_filename).toBeNull();
const [pivotRow] = (await feedbackService.exportEventFeedbackPivoted(eventId))
.filter((r) => r.filename === 'wedding-smith_individual_1755893222.jpg');
// The pivot builds plain objects, so its empty is '' rather than null.
expect(pivotRow.original_filename).toBe('');
});
});
+27
View File
@@ -5,6 +5,30 @@ const { REACTION_EMOJIS } = require('../constants/reactions');
const { isValidColorLabel, SHARED_COLOR_LABEL_IDENTITY } = require('../constants/colorLabels');
const { resolveEventFeedbackDefaults, DEFAULT_KEYBIND_MODE, KEYBIND_MODES } = require('./feedbackDefaults');
// The camera-original name, for the feedback exports (#1224). Both exports
// used to carry only `photos.filename` — the sanitized stored name
// (`wedding-smith_individual_1755892345.jpg`), which matches nothing in a
// Lightroom catalog. Acting on client picks means finding the master file, so
// the export has to name it.
//
// `source_filename` first, NOT `original_filename` alone: the latter is
// overwritten the first time an edited render is uploaded over a proof, so an
// export taken after a Lightroom round-trip (#745) would name the render
// rather than the master and silently stop matching. `source_filename` is
// written once at ingest and survives a replace by design (migration 193),
// and its backfill already covers rows that predate it.
//
// Aliased to `original_filename` because that is what the sibling photo export
// calls this column, and it is the question the reader is asking ("what did
// the camera call it"). Left empty rather than falling back to the stored
// name: blank reads as "no match possible", where repeating the sanitized
// name invites a match attempt that cannot succeed.
//
// A SQL string rather than a prebuilt db.raw(): the raw is constructed per
// query, so this does not depend on `db` being connected at module load.
const CAMERA_NAME_SQL =
'COALESCE(photos.source_filename, photos.original_filename) as original_filename';
// Every writable column on event_feedback_settings (#1030). The admin form
// posts its whole client-side state back, including UI-only keys that were
// never columns — `enable_rate_limiting`, `rate_limit_window_minutes`,
@@ -1065,6 +1089,7 @@ class FeedbackService {
.where('photo_feedback.event_id', eventId)
.select(
'photos.filename',
db.raw(CAMERA_NAME_SQL),
'photo_feedback.feedback_type',
'photo_feedback.rating',
'photo_feedback.comment_text',
@@ -1104,6 +1129,7 @@ class FeedbackService {
.where('photo_feedback.is_hidden', false)
.select(
'photos.filename',
db.raw(CAMERA_NAME_SQL),
'photo_feedback.feedback_type',
'photo_feedback.rating',
'photo_feedback.comment_text',
@@ -1128,6 +1154,7 @@ class FeedbackService {
if (!entry) {
entry = {
filename: row.filename,
original_filename: row.original_filename || '',
guest_name: row.guest_name || '',
guest_email: row.guest_email || '',
is_favorited: false,