* feat(api): Lightroom round-trip — read marks, put edits back (#745)
Gets a client's proofing verdict into a desktop catalogue and a finished
edit back over its proof, without anyone re-matching files by hand.
Three parts:
**Keep the camera filename.** photos.original_filename is the only carrier
of `IMG_1234.JPG` — the stored filename is rewritten by
generatePhotoFilename. But replacePhoto() overwrites original_filename with
whatever name the new file arrives under, so the first re-upload of a
renamed render destroys the key the NEXT round-trip needs. Migration 185
adds photos.source_filename, written once at ingest and never touched by a
replace, backfilled from original_filename so existing galleries can still
match on their first pass. The backfill sits outside the column guard and
keys on whereNull, so a run that dies partway self-heals instead of leaving
half the rows empty forever.
**Read the marks.** GET /api/v1/events/:id/photos returns each photo with
its client colour tallies, the caller's own marks, and a merged colour +
rating. Guards copied from the sibling upload route (apiTokenAuth +
read scope + photos.view + requireEventOwnership). Filters: marked_only,
mark_source, color_labels, my_color_labels, min_rating, my_min_rating.
The route filters to a page of ids with PhotoFilterBuilder, then enriches
just those through photoExportService.getPhotosWithFeedback — the two
halves already existed and neither does both, and going id-first keeps the
per-colour tally query bounded by page size.
services/markMerge.js decides how three possible opinions (guest colours,
guest star average, the photographer's own row in photo_admin_marks)
collapse into the one colour and one rating Lightroom has room for. Colour
goes to the photographer on a tie — one deliberate choice beats an
aggregate a tie-break already had to guess at. Rating takes the max,
because a rating is a magnitude and losing the higher one quietly demotes
a photo somebody rated highly. Its roundRating matches
XmpGenerator.mapRating exactly so the API and an XMP sidecar can never
disagree about how many stars a photo has.
**Put the edit back.** POST /api/v1/events/:id/photos accepts an optional
replaces_photo_id and routes to the existing replacePhoto(), preserving
the photo's id, feedback, comments and position. The plugin stores the
picpeak id on the catalogue photo, so the id survives the editor renaming
the render — which makes it the reliable key, not the filename. Scoped to
the event in the URL: a token inherits its owner's powers across every
event they can see, so an unscoped id would let one gallery overwrite
another's photo.
For renders whose RAW never went through the plugin, findReplacementCandidate
gains an opt-in number_token mode matching on the trailing digit run.
Deliberately the LONGEST run and never a fixed last-N slice: multi-camera
shoots disambiguate by prefixing the camera index into the number
(cam11234.jpg / cam21234.jpg), and a last-4 slice reads 1234 from both
bodies and reintroduces exactly the collision the prefix removes.
Ambiguity is refused, never guessed.
Also drops the multer temp file on the two new early returns — this route
only unlinks in its catch block.
* refactor(api): one rating-rounding rule, and apply match_mode where it counts
Three things the pre-review pass turned up on the round-trip work:
- `match_mode` reached the photo-cap pre-count but not the loop that
actually picks the replacement target, so asking for `number_token`
would have been counted and then quietly ignored. Both call sites now
take it.
- `number_token` matching read `select('*')` over every photo in the
event to compare one digit run. It now reads the three columns the
match needs and re-reads the single winner in full, so a 5000-photo
event doesn't pull 5000 full rows through memory to answer one
question.
- `XmpGenerator.mapRating` and `markMerge.roundRating` were the same
five thresholds written twice — the second way to do one thing that
drifts the moment either is touched. The thresholds now live in
markMerge and the generator delegates, which is what keeps a sidecar
and the v1 API from ever disagreeing about a photo's star count.
* fix(api): keep the new route in the generated OpenAPI spec
The `color_labels` description carried an inline JSON example. In an
unquoted YAML scalar `{ "green": 2 }` parses as a flow mapping, so
swagger-jsdoc threw YAMLSemanticError and dropped the WHOLE route from
the spec — visible only as a warning on boot, with the route still
serving normally, which is exactly the kind of failure that survives to
release.
Found by booting a real instance rather than by reading the diff.
* fix(api): close the four blockers from review on #1165
1. Replacing an external photo silently kept serving the old file.
resolvePhotoStorageKey gives photo.source_origin precedence and
returns null for 'reference'/'external', so the edit was uploaded,
the row updated and 200 returned while every viewer kept getting the
untouched NAS original and the upload sat orphaned. replacePhoto now
repoints the row to managed and clears external_relpath. The file on
the share is never touched — this moves the pointer, not the data.
2. Every replacement leaked its temp file. putFromFile COPIES on local
and uploads on S3; neither consumes the source, and replacePhoto
never unlinked it — while the v1 route had disabled its own cleanup
on the belief that replacePhoto moved the file. Cleanup now lives in
replacePhoto, which closes the admin path too (adminPhotos only
unlinks in its new-files branch, so replaced files leaked there as
well). The v1 route also unlinks on the FAILURE path, which returned
before any cleanup ran.
3. The download-all ZIP is invalidated after a replacement, as
adminPhotos.js already does. Without it guests kept downloading the
pre-edit photo indefinitely, which defeats the point of the feature.
4. The round-trip could not see reference or watcher galleries at all.
fileWatcher and adminExternalMedia never set original_filename — the
camera name lives in `filename` for those rows — so the backfill and
the GET fallback both produced NULL for exactly the galleries most
likely to be driven from Lightroom. The backfill now COALESCEs, both
ingest paths set source_filename, and the GET falls back to filename.
Concerns:
- number_token no longer reads every photo row in the event per file. A
LIKE on the digit run narrows the candidate set in SQL first; the
exact trailing-run check still decides, so semantics are unchanged.
The token is a regex-extracted digit run, so it cannot carry a
wildcard.
- The replacement's activity entry is scoped to event.id instead of
null. The dashboard feed excludes NULL-event rows for scoped callers
(GHSA-jhcf), so it was vanishing from the audit trail of the
photographer who owns the event.
Nit: dropped the unused higherPriorityColor export from markMerge.
Three regression tests cover the external repoint, the temp cleanup and
the COALESCE backfill. 21/21 pass.
* chore(migrations): renumber 185 -> 193 after gallery-folders landed
185_add_category_is_folder.js merged to main while this was in review,
so the number the PR reserved is taken and main is now at 192. Knex keys
on filename rather than the prefix, so both would have run — but
picpeakImportService guards restores with migrationOrder(), which parses
that prefix, and two files answering 185 make the forward-only check
pass a backup onto a schema missing its columns.
Renumbered with every reference: the header comment, the test that
requires the path, and the four call-site comments that cite it. The
'migration 182' reference inside it is the colour-labels migration and
is unrelated; gallery.js:1134 cites upstream's 185 and is untouched.
* fix(api): keep external_relpath when a replacement converts the row
The external-photo blocker fix cleared external_relpath along with
flipping source_origin, which closed one hole and opened another.
adminExternalMedia dedupes a re-scan on (event_id, external_relpath)
— routes/adminExternalMedia.js:195 — and migration 186 puts a unique
index on exactly that pair. With the column nulled, the next scan of
the share would not recognise the NAS original as already imported and
would insert it again, so the gallery would end up holding both the
edit and a fresh copy of the file it replaced.
Only source_origin needs to change: it is what resolvePhotoStorageKey
keys on, and every other consumer of external_relpath reads the two
together and lets source_origin decide. The stale relpath on a managed
row is inert for resolution and still correct as a dedupe key.
Test updated to assert the value is kept rather than cleared.
* fix(uploads): say when exiftool is missing instead of blaming the RAW
A server without exiftool reported `No usable embedded preview in RAW
file X.CR3: spawn exiftool ENOENT` for every RAW upload. The headline
describes a corrupt photo; the actual cause is a package that was never
installed, demoted to a trailing detail. It sends people hunting through
their camera files.
Hit while testing the Lightroom round-trip (#745): an export of RAW
originals failed 11 times with that message, and the file was fine.
RAW upload is the only feature that needs exiftool, so an install can be
missing it indefinitely and only find out when someone uploads a CR3 —
which makes the wording the whole diagnosis.
ENOENT now produces a message naming the dependency and the install
command for Debian/Alpine/macOS, and breaks out of the tag loop instead
of spawning the same missing binary twice more to report the last
failure as if it described the photo. A genuinely preview-less RAW still
gets the original message.
Verified both paths by making exiftool unreachable via PATH rather than
mocking: missing tool and unreadable file now report differently.
* fix(external): a delivered edit must win a relpath-fold collision
Follow-up to keeping external_relpath on a replaced photo. Keeping it is
what lets adminExternalMedia still dedupe the folder re-scan, but it also
leaves the row inside externalRelpathFold's sweep — and that sweep does
not merely rewrite paths, it DELETES collision losers via
externalPhotoDedupe.
The survivor was whichever row happened to be claimed first, which is
iteration order. So a replaced photo — source_origin 'managed', holding
the edit the photographer just delivered — could be deleted in favour of
the untouched camera original sitting next to it on the share.
Managed rows now claim first and therefore survive. The external row
that loses is the recoverable one: it is still on the share and a
re-scan re-imports it. The edit is not recoverable.
Note this is deliberately NOT the "skip managed rows in the fold"
shape suggested in review. Skipping would leave those rows holding a
base-relative path while every other row moved to root-relative, so the
scanner — which computes root-relative — would stop matching them and
import the camera original again as a duplicate. That is the exact bug
keeping external_relpath exists to prevent, reintroduced through a
different door. Rebasing them and protecting them from deletion keeps
both properties.
Luca
8db8527f9e