From 3f6c81a8460145562931a0bc3248b84e62a40bae Mon Sep 17 00:00:00 2001
From: Paul Nothaft <paul@MacStudio-von-Paul.local>
Date: Tue, 1 Sep 2026 16:30:46 +0200
Subject: [PATCH] fix(photos): treat category_id 0 as uncategorized instead of
 storing it

Genuine product bug, found behind the adminPhotos.reference suite (which was
failing for an unrelated reason -- see below).

parseInt('0') is 0 and !isNaN(0) is true, so a '0' category_id was written
literally. photo_categories.id is an increments() column, so 0 can never be a
real category, and every read path already assumes it cannot happen: the list
mapper does `category_id || type` (0 is falsy, renders as uncategorized) and
the list filter explicitly skips '0'. The result was a filter black hole -- the
photo matches no numeric category filter, and misses the "uncategorized"
filter too because that is whereNull(). Displayed as uncategorized, reachable
by nothing.

null rather than a 400: unparseable input ('abc' -> NaN) already falls through
to null, so 400ing on '0' while silently accepting 'abc' would be incoherent,
and '0' is just the HTML <select> shape where the "none" option carries
value="0".

Fixed at all three call sites that share the branch -- PATCH /photos/:photoId,
POST /photos/bulk-update, and the upload route, where the dangling 0 was
written at creation time and the scope-validation guard
(`if (parsedCategoryId && ...)`) skipped on the falsy 0 and let it in
unvalidated. Only the PATCH one was behind the failing test; leaving the other
two would have left the bad state creatable.

The suite's 3 failures were all masked by a fixture gap, not this bug: it
stubs middleware/auth but not middleware/permissions, so requirePermission's
admin_users JOIN roles query hit tables the fixture never creates and every
request 500'd before reaching a handler. Stub it, bring the photos fixture up
to the 7 migrations it had drifted behind, and correct a stale 200 that became
202 when uploads went async in 851744c3.

Known adjacent gap, not fixed (wider than this bug): PATCH and bulk-update
accept any positive category_id with no existence or scope check, unlike the
upload route which validates event_id = X OR is_global per #500/#525 -- so a
photo can be PATCHed into another event's category.

Refs testplan REPORT.md #22 (Part 1.2.01).
---
 .../integration/adminPhotos.reference.test.js | 48 ++++++++++++++++++-
 backend/src/routes/adminPhotos.js             | 19 ++++++--
 2 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/backend/__tests__/integration/adminPhotos.reference.test.js b/backend/__tests__/integration/adminPhotos.reference.test.js
index 76248264..67424970 100644
--- a/backend/__tests__/integration/adminPhotos.reference.test.js
+++ b/backend/__tests__/integration/adminPhotos.reference.test.js
@@ -40,6 +40,15 @@ describe('Admin photos in reference mode', () => {
       }
     }));
 
+    // The routes gained requirePermission() after this fixture was written.
+    // It resolves the caller's role through admin_users/roles, which this
+    // minimal schema does not create, so every request died in the RBAC
+    // lookup before reaching the handler. RBAC is not what this suite is
+    // about — stub it out the same way adminAuth already is.
+    jest.doMock('../../src/middleware/permissions', () => ({
+      requirePermission: () => (_req, _res, next) => next()
+    }));
+
     jest.doMock('../../src/services/imageProcessor', () => ({
       generateThumbnail: jest.fn().mockResolvedValue('thumbnails/mock-thumb.jpg'),
       ensureThumbnail: jest.fn()
@@ -98,8 +107,21 @@ describe('Admin photos in reference mode', () => {
       table.string('type').notNullable();
       table.integer('size_bytes');
       table.integer('category_id');
-      table.string('source_origin');
+      // Mirrors migration 041: the upload route never writes this column, it
+      // relies on the NOT NULL DEFAULT 'managed' to mark managed originals.
+      table.string('source_origin').notNullable().defaultTo('managed');
       table.string('external_relpath');
+      // Columns the upload insert writes (migrations 048, 062, 071, 085, 193)
+      // and the PATCH handler writes (migration 178). Without them the insert
+      // and the update both fail on "no such column".
+      table.string('original_filename', 512);
+      table.string('source_filename', 255);
+      table.datetime('captured_at').nullable();
+      table.string('media_type').defaultTo('image');
+      table.string('mime_type');
+      table.string('processing_status', 16).notNullable().defaultTo('complete');
+      table.string('upload_id', 64).nullable();
+      table.boolean('auto_categorized');
       table.datetime('uploaded_at').defaultTo(db.fn.now());
       table.float('average_rating').defaultTo(0);
       table.integer('like_count').defaultTo(0);
@@ -153,7 +175,10 @@ describe('Admin photos in reference mode', () => {
       .field('category_id', String(categoryId))
       .attach('photos', Buffer.from('fake image data'), 'photo.jpg');
 
-    expect(uploadResponse.status).toBe(200);
+    // 202 Accepted since the upload route went async (851744c3): the files are
+    // stored and a pending row is inserted, thumbnails/EXIF follow in the
+    // background worker. This assertion still said 200 from before that.
+    expect(uploadResponse.status).toBe(202);
     expect(uploadResponse.body).toHaveProperty('photos');
     expect(Array.isArray(uploadResponse.body.photos)).toBe(true);
 
@@ -203,5 +228,24 @@ describe('Admin photos in reference mode', () => {
 
     const updated = await db('photos').where({ id: photo.id }).first();
     expect(updated.category_id).toBeNull();
+
+    // A real id still round-trips — the '0' guard must not swallow it.
+    await request(app)
+      .patch(`/api/admin/events/1/photos/${photo.id}`)
+      .send({ category_id: String(categoryId) })
+      .expect(200);
+    expect((await db('photos').where({ id: photo.id }).first()).category_id).toBe(categoryId);
+
+    // Numeric 0 and unparseable input clear the category too, rather than
+    // writing a category id that can never exist.
+    for (const value of [0, 'not-a-category']) {
+      await request(app)
+        .patch(`/api/admin/events/1/photos/${photo.id}`)
+        .send({ category_id: value })
+        .expect(200);
+      expect((await db('photos').where({ id: photo.id }).first()).category_id).toBeNull();
+
+      await db('photos').where({ id: photo.id }).update({ category_id: categoryId });
+    }
   });
 });
diff --git a/backend/src/routes/adminPhotos.js b/backend/src/routes/adminPhotos.js
index 65e1c139..62a3e1f6 100644
--- a/backend/src/routes/adminPhotos.js
+++ b/backend/src/routes/adminPhotos.js
@@ -243,8 +243,11 @@ router.post('/:eventId/upload', adminAuth, requirePermission('photos.upload'), r
     }
     
     // Parse category_id to number if provided (handle string values like 'individual', 'collage')
+    // Same 0-is-not-a-category rule as the PATCH route below: '0' is truthy, so
+    // it parsed to 0 and the scope-validation guard (`if (parsedCategoryId && ...)`)
+    // then skipped on the falsy 0 and let it into the insert unvalidated.
     const rawParsed = category_id ? parseInt(category_id, 10) : NaN;
-    const parsedCategoryId = !isNaN(rawParsed) ? rawParsed : null;
+    const parsedCategoryId = rawParsed > 0 ? rawParsed : null;
 
     // Determine photo type and category name
     let photoType = 'individual'; // default
@@ -846,9 +849,16 @@ router.patch('/:eventId/photos/:photoId', adminAuth, requirePermission('photos.e
       // Explicitly clear category
       updateData.category_id = null;
     } else {
-      // Handle numeric category IDs from photo_categories table
+      // Handle numeric category IDs from photo_categories table.
+      // 0 and negatives mean "no category", not category zero: photo_categories.id
+      // is an increments() column so it starts at 1, and a <select> whose "none"
+      // option carries value="0" is exactly how '0' reaches this route. Storing 0
+      // left the photo in a black hole — the grid's category filters never match
+      // it, and the "uncategorized" filter is whereNull() so it misses it too,
+      // while the list mapper renders it as uncategorized because 0 is falsy.
+      // NaN (unparseable input) already fell through to null and still does.
       const numericCategoryId = parseInt(category_id, 10);
-      if (!isNaN(numericCategoryId)) {
+      if (numericCategoryId > 0) {
         updateData.category_id = numericCategoryId;
       } else {
         updateData.category_id = null;
@@ -1031,8 +1041,9 @@ router.post('/:eventId/photos/bulk-update', adminAuth, requirePermission('photos
         updateData.category_id = null;
       } else {
         // Handle numeric category IDs from photo_categories table
+        // (0/negative mean "no category" — see the PATCH route above)
         const numericCategoryId = parseInt(updates.category_id, 10);
-        if (!isNaN(numericCategoryId)) {
+        if (numericCategoryId > 0) {
           updateData.category_id = numericCategoryId;
         } else {
           updateData.category_id = null;