From e8c2212dad4716ec7b71c28953548d1009b9aa3b Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Mon, 18 May 2026 23:50:35 +0200 Subject: [PATCH] refactor(slug): extract shared slugify util + scope adminPhotos category lookup (#525) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folds all three follow-up items tracked in #525 into one commit: 1. Mirror PR #500's category scoping on adminPhotos.js. The admin upload route at adminPhotos.js:231 still accepted any category_id without event scoping — quietly less strict than the public v1 API after #500 landed. Same one-liner fix (event_id OR is_global) with a matching 400 response shape so admin + v1 stay consistent. 2. Extract a shared slugify() in backend/src/utils/slug.js with the NFD-strip-combining-marks fix from #502, and route 5 callers through it: - adminEvents.js (event-name slug) - events.js (event-create slug) - v1/events.js (replaces local slugify helper) - adminArchives.js (archive→category slug) For pure-ASCII input the output is byte-identical to each old inline pipeline, so existing slugs in the DB keep round-tripping cleanly via lookup. Accented inputs now transliterate (Família → familia) instead of dropping the diacritic (Família → f-mlia). adminCategories.js stays with its own pipeline (underscores-as- word-chars semantics differ from the events-style transform — changing would silently shift wedding_party → wedding-party on new inserts). xmpGenerator.sanitizeKeyword stays unchanged for the same compat-cautious reason. 3. Cover the v1 upload happy path. Existing test only exercised the 400-out-of-scope branch. Add two happy-path cases that stub sharp / generateThumbnail / storage.putFromFile and pin the response shape (id, category_id, type, etc.) plus the collage- slug → type='collage' flip. Temp file recreated in beforeEach because the handler unlinks it on success. Tests: - New slug.test.js: 22 cases pinning ASCII parity with the legacy pipeline (so the refactor is provably non-breaking for existing data) and the corrected accent handling across de/es/fr/nl/pt inputs, plus CJK and edge-case behaviour. - events.category.test.js: 4 tests total (2 existing + 2 new happy path). - galleryOgService.shareImage.test.js: 11 (3 added in #521 + 8 pre- existing) still pass. 37 tests pass across the three touched files. Refs: #525, follows up #500 and #502 --- backend/src/routes/adminArchives.js | 3 +- backend/src/routes/adminEvents.js | 11 +- backend/src/routes/adminPhotos.js | 29 +++-- backend/src/routes/events.js | 5 +- .../v1/__tests__/events.category.test.js | 115 ++++++++++++++++++ backend/src/routes/v1/events.js | 5 +- backend/src/utils/__tests__/slug.test.js | 93 ++++++++++++++ backend/src/utils/slug.js | 35 ++++++ 8 files changed, 277 insertions(+), 19 deletions(-) create mode 100644 backend/src/utils/__tests__/slug.test.js create mode 100644 backend/src/utils/slug.js diff --git a/backend/src/routes/adminArchives.js b/backend/src/routes/adminArchives.js index 57fdecbf..d290e26e 100644 --- a/backend/src/routes/adminArchives.js +++ b/backend/src/routes/adminArchives.js @@ -3,6 +3,7 @@ const path = require('path'); const fs = require('fs').promises; const { db } = require('../database/db'); const { formatBoolean } = require('../utils/dbCompat'); +const { slugify } = require('../utils/slug'); const { adminAuth } = require('../middleware/auth'); const { requirePermission } = require('../middleware/permissions'); const archiver = require('archiver'); @@ -217,7 +218,7 @@ router.post('/:id/restore', adminAuth, requirePermission('archives.restore'), re const insertResult = await db('photo_categories').insert({ event_id: archive.id, name: categoryName, - slug: categoryName.toLowerCase().replace(/[^a-z0-9]/g, '-'), + slug: slugify(categoryName), created_at: new Date() }).returning('id'); diff --git a/backend/src/routes/adminEvents.js b/backend/src/routes/adminEvents.js index d1bbab3e..4ce28d14 100644 --- a/backend/src/routes/adminEvents.js +++ b/backend/src/routes/adminEvents.js @@ -2,6 +2,7 @@ const express = require('express'); const { body, validationResult } = require('express-validator'); const { db, logActivity } = require('../database/db'); const { formatBoolean } = require('../utils/dbCompat'); +const { slugify } = require('../utils/slug'); const { adminAuth } = require('../middleware/auth'); const { requirePermission } = require('../middleware/permissions'); const router = express.Router(); @@ -549,12 +550,10 @@ router.post('/', adminAuth, requirePermission('events.create'), [ } } - // Generate unique slug - const processedEventName = event_name - .toLowerCase() - .replace(/[^a-z0-9]/g, '-') // Replace non-alphanumeric with dash - .replace(/-+/g, '-') // Replace multiple dashes with single dash - .replace(/^-|-$/g, ''); // Remove leading/trailing dashes + // Generate unique slug. Uses the shared util so accented names + // (Família, Decoração, etc.) get transliterated instead of dropped + // — see backend/src/utils/slug.js for the why (#525). + const processedEventName = slugify(event_name); // Use event_date in slug if provided, otherwise use random suffix const slugSuffix = event_date || crypto.randomBytes(3).toString('hex'); diff --git a/backend/src/routes/adminPhotos.js b/backend/src/routes/adminPhotos.js index 4f48016a..50496a52 100644 --- a/backend/src/routes/adminPhotos.js +++ b/backend/src/routes/adminPhotos.js @@ -226,15 +226,28 @@ router.post('/:eventId/upload', adminAuth, requirePermission('photos.upload'), r let photoType = 'individual'; // default let categoryName = 'individual'; - // Look up the actual category from database if provided + // Look up the actual category from database if provided. Scope the + // lookup to (event_id = event.id OR is_global = true) — same contract + // the public v1 upload route enforces (#500 / #525). Without it, the + // admin upload silently accepts any category id including ones that + // belong to a different event. The v1 route rejects out-of-scope ids + // with 400; mirror that here so admin and v1 stay consistent. if (parsedCategoryId && !isNaN(parsedCategoryId)) { - const category = await db('photo_categories').where({ id: parsedCategoryId }).first(); - if (category) { - categoryName = category.slug || category.name.toLowerCase().replace(/\s+/g, '_'); - // Use category slug for type determination - if (category.slug === 'collage' || category.slug === 'collages') { - photoType = 'collage'; - } + const category = await db('photo_categories') + .where({ id: parsedCategoryId }) + .andWhere(function () { + this.where({ event_id: event.id }).orWhere('is_global', true); + }) + .first(); + if (!category) { + return res.status(400).json({ + error: `Unknown or out-of-scope category_id ${parsedCategoryId}` + }); + } + categoryName = category.slug || category.name.toLowerCase().replace(/\s+/g, '_'); + // Use category slug for type determination + if (category.slug === 'collage' || category.slug === 'collages') { + photoType = 'collage'; } } else if (category_id === 'collage') { // For backwards compatibility, accept string values diff --git a/backend/src/routes/events.js b/backend/src/routes/events.js index a42942a9..4122bd5d 100644 --- a/backend/src/routes/events.js +++ b/backend/src/routes/events.js @@ -4,6 +4,7 @@ const bcrypt = require('bcrypt'); const crypto = require('crypto'); const { db } = require('../database/db'); const { formatBoolean } = require('../utils/dbCompat'); +const { slugify } = require('../utils/slug'); const { validatePasswordInContext, getBcryptRounds } = require('../utils/passwordValidation'); const { adminAuth } = require('../middleware/auth'); const fs = require('fs').promises; @@ -125,8 +126,8 @@ router.post('/', adminAuth, [ } } - // Generate unique slug - const baseSlug = `${event_type}-${event_name.toLowerCase().replace(/[^a-z0-9]/g, '-')}-${event_date}`; + // Generate unique slug — slugify() handles accents (see #525). + const baseSlug = `${event_type}-${slugify(event_name)}-${event_date}`; let slug = baseSlug; let counter = 1; diff --git a/backend/src/routes/v1/__tests__/events.category.test.js b/backend/src/routes/v1/__tests__/events.category.test.js index 6ea0f7f4..872ba5fd 100644 --- a/backend/src/routes/v1/__tests__/events.category.test.js +++ b/backend/src/routes/v1/__tests__/events.category.test.js @@ -79,6 +79,32 @@ jest.mock('multer', () => { return factory; }); +// Stub sharp so the happy-path test doesn't actually decode an image +// (the temp file is a 0-byte placeholder — see the beforeAll below). +jest.mock('sharp', () => jest.fn(() => ({ + metadata: jest.fn().mockResolvedValue({ width: 1920, height: 1080 }), +}))); + +// Thumbnail + storage are network/fs-heavy; stub to constant resolves +// so the test stays a pure unit test of the route handler's contract. +jest.mock('../../../services/imageProcessor', () => ({ + generateThumbnail: jest.fn().mockResolvedValue('thumbnails/fake_thumb.jpg'), +})); + +jest.mock('../../../services/storage', () => ({ + getStorage: jest.fn(() => ({ + putFromFile: jest.fn().mockResolvedValue(undefined), + })), +})); + +// webhookService.fire is wrapped in try/catch in the route, so a +// missing mock would still let the test pass — but stubbing it +// silences the predictable failure log so the test output stays clean. +jest.mock('../../../services/webhookService', () => ({ + fire: jest.fn().mockResolvedValue(undefined), +})); + +const fsSync = require('fs'); const { db } = require('../../../database/db'); const eventsRouter = require('../events'); @@ -145,3 +171,92 @@ describe('v1 POST /events/:id/photos — category scoping', () => { }); }); }); + +describe('v1 POST /events/:id/photos — happy path (#525)', () => { + const FAKE_TMP = '/tmp/fake-v1-upload.jpg'; + + beforeEach(() => { + jest.clearAllMocks(); + // Recreate the temp file on every test — the handler calls + // fs.unlink(tempPath) after a successful upload, so a beforeAll + // would leave the second test without an inode for statSync to + // read (manifests as 500 Internal Server Error). + fsSync.writeFileSync(FAKE_TMP, ''); + }); + + afterAll(() => { + try { fsSync.unlinkSync(FAKE_TMP); } catch { /* may have been unlinked by the handler */ } + }); + + it('inserts the photo and returns 201 with the resolved category_id', async () => { + // Three db() calls in sequence on the happy path: + // 1. events lookup + // 2. photo_categories lookup (returns a valid in-scope row) + // 3. photos insert returning the new id + const eventChain = buildChain({ + firstResult: { id: 42, slug: 'wedding-2026', event_name: 'Wedding 2026' }, + }); + const categoryChain = buildChain({ + firstResult: { id: 7, slug: 'ceremony', name: 'Ceremony', event_id: 42 }, + }); + const insertChain = { + ...buildChain({ insertResult: [{ id: 101 }] }), + returning: jest.fn().mockResolvedValue([{ id: 101 }]), + }; + // Override insert so the returning() call is chainable + insertChain.insert = jest.fn(() => insertChain); + db.__setImplementations(eventChain, categoryChain, insertChain); + + const response = await request(buildApp()) + .post('/events/42/photos') + .send({ category_id: '7' }) + .expect(201); + + // Response shape pins the v1 API contract — id + category_id are + // the fields the n8n / API-token use case depends on (see #500). + expect(response.body).toMatchObject({ + id: 101, + category_id: 7, + size_bytes: 0, + thumbnail_path: 'thumbnails/fake_thumb.jpg', + }); + expect(response.body.filename).toMatch(/^\d+_[a-f0-9]+\.jpg$/); + expect(response.body.path).toMatch(/^wedding-2026\/\d+_[a-f0-9]+\.jpg$/); + + // The insert payload should carry the resolved category_id and the + // 'individual' photo type (the test category slug isn't 'collage'). + const insertedRow = insertChain.insert.mock.calls[0][0]; + expect(insertedRow).toMatchObject({ + event_id: 42, + category_id: 7, + type: 'individual', + media_type: 'image', + mime_type: 'image/jpeg', + }); + }); + + it('flips photo type to collage when the category slug is "collage"', async () => { + const eventChain = buildChain({ + firstResult: { id: 42, slug: 'wedding-2026' }, + }); + const categoryChain = buildChain({ + firstResult: { id: 9, slug: 'collage', name: 'Collage', event_id: 42 }, + }); + const insertChain = { + ...buildChain(), + returning: jest.fn().mockResolvedValue([{ id: 202 }]), + }; + insertChain.insert = jest.fn(() => insertChain); + db.__setImplementations(eventChain, categoryChain, insertChain); + + await request(buildApp()) + .post('/events/42/photos') + .send({ category_id: '9' }) + .expect(201); + + expect(insertChain.insert.mock.calls[0][0]).toMatchObject({ + category_id: 9, + type: 'collage', + }); + }); +}); diff --git a/backend/src/routes/v1/events.js b/backend/src/routes/v1/events.js index 78c68f94..436d8467 100644 --- a/backend/src/routes/v1/events.js +++ b/backend/src/routes/v1/events.js @@ -23,6 +23,7 @@ const { apiTokenAuth, requireApiScope } = require('../../middleware/apiTokenAuth const { buildShareLinkVariants } = require('../../services/shareLinkService'); const { generateThumbnail } = require('../../services/imageProcessor'); const logger = require('../../utils/logger'); +const { slugify } = require('../../utils/slug'); const router = express.Router(); @@ -51,8 +52,8 @@ const photoUpload = multer({ } }); -const slugify = (s) => - String(s).toLowerCase().replace(/[^a-z0-9]/g, '-').replace(/-+/g, '-').replace(/^-|-$/g, ''); +// slugify now imported from ../../utils/slug — shared with adminEvents +// and events.js so the diacritic fix from #502 lands here too (#525). // ────────────────────────────────────────────────────────────────────────── // POST /events — create event diff --git a/backend/src/utils/__tests__/slug.test.js b/backend/src/utils/__tests__/slug.test.js new file mode 100644 index 00000000..1e7cfa78 --- /dev/null +++ b/backend/src/utils/__tests__/slug.test.js @@ -0,0 +1,93 @@ +/** + * Tests for the shared slug util extracted in #525 from the inline + * pipelines in adminEvents.js, events.js, v1/events.js, adminArchives.js. + * + * Two contracts to pin: + * 1. ASCII inputs produce byte-identical output to the previous + * inline pipelines, so existing event/archive slugs in the DB + * keep resolving via the same lookup path after the refactor. + * 2. Accented characters (Portuguese, German, French, Spanish) are + * transliterated to their ASCII bases (Decoração → decoracao) + * instead of being dropped (Decoração → decorao) as the legacy + * pipelines did — same fix as #502 for category slugs. + */ + +const { slugify } = require('../slug'); + +describe('slugify — ASCII parity with the legacy event-style pipeline', () => { + // Replays the exact transformation used by adminEvents.js before the + // refactor: lowercase → replace [^a-z0-9] with '-' → collapse → trim. + const legacy = (s) => + String(s).toLowerCase() + .replace(/[^a-z0-9]/g, '-') + .replace(/-+/g, '-') + .replace(/^-|-$/g, ''); + + const samples = [ + 'Wedding 2026', + ' Hello World ', + 'birthday-party-42', + 'event_with_underscores', + 'CamelCase Event Name', + '', + 'event.with.dots', + 'event!@#$%^&*()chars', + '2026-06-12', + ]; + + it.each(samples)('matches legacy output for ASCII input: %j', (input) => { + expect(slugify(input)).toBe(legacy(input)); + }); +}); + +describe('slugify — accented characters (the #502 fix, now shared)', () => { + // The legacy pipeline produced f-mlia for "Família" because the í + // got replaced with '-' rather than being NFD-normalised to 'i'. + // These tests pin the corrected behaviour across the locales the + // app already ships in (de, es, fr, nl, pt, ru). + it.each([ + ['Decoração', 'decoracao'], + ['Família', 'familia'], + ['Recepção', 'recepcao'], + ['Über uns', 'uber-uns'], + ['Niño', 'nino'], + ['Fête de famille', 'fete-de-famille'], + ['L\'Évènement', 'l-evenement'], + ['Crème Brûlée', 'creme-brulee'], + ])('transliterates %j → %j', (input, expected) => { + expect(slugify(input)).toBe(expected); + }); + + it('CJK and other scripts without NFD decompositions still strip cleanly', () => { + // NFD doesn't decompose Chinese characters to ASCII, so they get + // dropped by the [^a-z0-9]+ replace. Output is sensible if not + // perfect — the surrounding ASCII tokens survive. + expect(slugify('Photo 混合 Test')).toBe('photo-test'); + // Pure-CJK names collapse to empty after trim — caller's job to + // handle (typically by appending a uniqueness suffix). + expect(slugify('婚礼')).toBe(''); + }); +}); + +describe('slugify — input edge cases', () => { + it('returns empty string for null / undefined / empty', () => { + expect(slugify(null)).toBe(''); + expect(slugify(undefined)).toBe(''); + expect(slugify('')).toBe(''); + }); + + it('coerces non-string input to string before slugifying', () => { + expect(slugify(2026)).toBe('2026'); + expect(slugify(true)).toBe('true'); + }); + + it('collapses any run of non-alphanumeric chars into a single dash', () => { + expect(slugify('a!@#$%b')).toBe('a-b'); + expect(slugify('a b\t\nc')).toBe('a-b-c'); + }); + + it('trims leading and trailing dashes', () => { + expect(slugify('---hello---')).toBe('hello'); + expect(slugify('!!!world!!!')).toBe('world'); + }); +}); diff --git a/backend/src/utils/slug.js b/backend/src/utils/slug.js new file mode 100644 index 00000000..8799c508 --- /dev/null +++ b/backend/src/utils/slug.js @@ -0,0 +1,35 @@ +/** + * URL-safe slug generation shared across event, archive, and v1 upload + * routes (#525 follow-up to #502). Previously every caller had its own + * inline `name.toLowerCase().replace(/[^a-z0-9]/g, '-')` pipeline, each + * with the same latent bug: JS's `\w` and the ASCII alphanumeric class + * silently drop non-ASCII letters instead of transliterating them + * (`Decoração` → `decorao`, `Família` → `f-mlia`). + * + * Fix mirrors #502: NFD-normalize so accented characters split into a + * base letter + combining mark, then strip the combining-mark range + * (U+0300–U+036F) so the ASCII base survives. Single regex pass after + * that — `[^a-z0-9]+` collapses any run of non-alphanumerics into one + * dash, no separate collapse step needed. + * + * For pure-ASCII input the output is byte-identical to the previous + * inline pipelines, so existing slugs continue to round-trip cleanly + * via lookups; only new inserts with non-ASCII names start producing + * the corrected slugs. + * + * Not exported as the default category slug — `adminCategories.js` + * intentionally preserves underscores (the legacy category pipeline + * used `\w` not `[a-z0-9]`), so changing it here would silently shift + * "wedding_party" → "wedding-party" on new inserts. Categories keep + * their own pipeline as fixed in #502. + */ +function slugify(input) { + return String(input ?? '') + .normalize('NFD') + .replace(/[̀-ͯ]/g, '') + .toLowerCase() + .replace(/[^a-z0-9]+/g, '-') + .replace(/^-|-$/g, ''); +} + +module.exports = { slugify };