refactor(slug): extract shared slugify util + scope adminPhotos category lookup (#525)

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
2026-05-18 23:50:35 +02:00
parent 4b4ecfdf71
commit e8c2212dad
8 changed files with 277 additions and 19 deletions
@@ -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');
                  
@@ -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');
@@ -226,16 +226,29 @@ 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) {
+      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
      photoType = 'collage';
@@ -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;
    
@@ -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',
+    });
+  });
+});
@@ -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
@@ -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');
+  });
+});
@@ -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 };