From 4f684eb482ee3bef9a6d4d3e63fe35be0cb99700 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Sat, 29 Aug 2026 11:09:59 +0200 Subject: [PATCH] fix(feedback): name the camera original in the exports, not just the stored file (#1224) (#1228) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../utils/feedbackExportCameraName.test.js | 156 ++++++++++++++++++ backend/src/services/feedbackService.js | 27 +++ 2 files changed, 183 insertions(+) create mode 100644 backend/__tests__/utils/feedbackExportCameraName.test.js diff --git a/backend/__tests__/utils/feedbackExportCameraName.test.js b/backend/__tests__/utils/feedbackExportCameraName.test.js new file mode 100644 index 00000000..4ab5381d --- /dev/null +++ b/backend/__tests__/utils/feedbackExportCameraName.test.js @@ -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(''); + }); +}); diff --git a/backend/src/services/feedbackService.js b/backend/src/services/feedbackService.js index 96c22825..d4629c8a 100644 --- a/backend/src/services/feedbackService.js +++ b/backend/src/services/feedbackService.js @@ -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,