From de459c701f28532ca53d52773b02de44c9978073 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Thu, 13 Aug 2026 18:51:16 +0200 Subject: [PATCH] fix(feedback): persist guest feedback settings, unshadow the guest route (#1030) (#1032) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enabling Guest Feedback on an event could silently do nothing. 1. `updateEventFeedbackSettings` spread the request body straight into the knex UPDATE. The admin event form posts its whole client-side state, including three keys that were never columns on event_feedback_settings (`enable_rate_limiting`, `rate_limit_window_minutes`, `rate_limit_max_requests`), so the write threw and the route answered 500. Writable columns are now whitelisted; identity columns and timestamps stay server-managed. 2. EventDetailsPage swallowed that 500 in a bare `catch {}` ("Error already handled by mutation" — it is a different request), so the admin was left looking at "Event updated successfully" while the toggle never persisted. The error is surfaced now and the settings query is invalidated on success. 3. gallery.js declared a duplicate `GET /:slug/feedback-settings`. server.js mounts galleryRoutes before galleryFeedback, so it shadowed the real handler and dropped the per-guest caps (#655) from the guest payload — the gallery could never render the favorite/like limits or their counters. Timestamps are written as ISO strings so they round-trip on both engines. Claude-Session: https://claude.ai/code/session_0168gubtwYYacJv8weAjy8DM Co-authored-by: Paul Nothaft --- .../utils/feedbackSettingsUpdate.test.js | 161 ++++++++++++++++++ backend/src/routes/gallery.js | 25 +-- backend/src/services/feedbackService.js | 52 +++++- frontend/src/pages/admin/EventDetailsPage.tsx | 13 +- 4 files changed, 219 insertions(+), 32 deletions(-) create mode 100644 backend/__tests__/utils/feedbackSettingsUpdate.test.js diff --git a/backend/__tests__/utils/feedbackSettingsUpdate.test.js b/backend/__tests__/utils/feedbackSettingsUpdate.test.js new file mode 100644 index 00000000..31f5c536 --- /dev/null +++ b/backend/__tests__/utils/feedbackSettingsUpdate.test.js @@ -0,0 +1,161 @@ +/** + * Regression tests for the feedback-settings write path (#1030). + * + * The admin event form posts its whole client-side feedback state back, + * including three keys that were never columns on event_feedback_settings: + * `enable_rate_limiting`, `rate_limit_window_minutes` and + * `rate_limit_max_requests`. Spreading those into the knex UPDATE threw, + * the route answered 500, and EventDetailsPage swallowed it — so the admin + * saw "Event updated successfully" while "Enable feedback" never persisted + * and guests could not leave any feedback. + * + * Pinned here: + * - UI-only keys are dropped, not written, on BOTH the insert (no row yet) + * and update (row exists) branches. + * - Every real column still round-trips. + * - Identity columns can't be mass-assigned through the settings body. + * - gallery.js no longer declares a duplicate GET /:slug/feedback-settings. + * server.js mounts galleryRoutes before galleryFeedback, so the duplicate + * shadowed the real handler and dropped the #655 per-guest caps from the + * guest payload. + */ + +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-settings-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'feedback-settings-test-secret'; + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +const feedbackService = require('../../src/services/feedbackService'); + +// Exactly what EventDetailsPage holds in state before its settings GET +// resolves — the three rate-limit keys are UI-only. +const ADMIN_FORM_BODY = { + feedback_enabled: true, + allow_ratings: true, + allow_likes: true, + allow_comments: true, + allow_favorites: true, + allow_reactions: true, + require_name_email: false, + moderate_comments: true, + show_feedback_to_guests: true, + enable_rate_limiting: false, + rate_limit_window_minutes: 15, + rate_limit_max_requests: 10, +}; + +let db; +let cleanup; +let eventId; + +async function insertEvent(slug) { + const inserted = await db('events').insert({ + slug, + event_type: 'wedding', + event_name: 'Feedback Settings Test', + event_date: '2026-06-22', + host_email: 'host@example.com', + admin_email: 'admin@example.com', + password_hash: 'x', + share_link: `/gallery/${slug}/share`, + share_token: `${slug}-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'); + return inserted[0]?.id ?? inserted[0]; +} + +beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + eventId = await insertEvent('feedback-settings-test'); +}, 120000); + +afterAll(async () => { if (cleanup) await cleanup(); }); + +describe('updateEventFeedbackSettings ignores UI-only keys (#1030)', () => { + test('insert branch: enabling feedback on an event with no settings row persists', async () => { + const freshEventId = await insertEvent('feedback-settings-fresh'); + + const result = await feedbackService.updateEventFeedbackSettings(freshEventId, ADMIN_FORM_BODY); + + expect(result.feedback_enabled).toBeTruthy(); + const row = await db('event_feedback_settings').where('event_id', freshEventId).first(); + expect(row).toBeTruthy(); + expect(row.feedback_enabled).toBeTruthy(); + expect(row).not.toHaveProperty('enable_rate_limiting'); + }); + + test('update branch: flipping the toggle on an existing row persists', async () => { + await feedbackService.updateEventFeedbackSettings(eventId, { feedback_enabled: false }); + expect((await feedbackService.getEventFeedbackSettings(eventId)).feedback_enabled).toBeFalsy(); + + const result = await feedbackService.updateEventFeedbackSettings(eventId, ADMIN_FORM_BODY); + + expect(result.feedback_enabled).toBeTruthy(); + const rows = await db('event_feedback_settings').where('event_id', eventId); + expect(rows).toHaveLength(1); + expect(rows[0].feedback_enabled).toBeTruthy(); + }); + + test('every real column round-trips', async () => { + const result = await feedbackService.updateEventFeedbackSettings(eventId, { + ...ADMIN_FORM_BODY, + allow_comments: false, + show_feedback_to_guests: false, + identity_mode: 'guest', + max_favorites_per_guest: 10, + max_likes_per_guest: 5, + }); + + expect(result.allow_comments).toBeFalsy(); + expect(result.show_feedback_to_guests).toBeFalsy(); + expect(result.identity_mode).toBe('guest'); + expect(result.max_favorites_per_guest).toBe(10); + expect(result.max_likes_per_guest).toBe(5); + }); + + test('identity columns cannot be mass-assigned through the settings body', async () => { + const otherEventId = await insertEvent('feedback-settings-other'); + const before = await db('event_feedback_settings').where('event_id', eventId).first(); + + await feedbackService.updateEventFeedbackSettings(eventId, { + feedback_enabled: true, + id: 99999, + event_id: otherEventId, + }); + + const after = await db('event_feedback_settings').where('event_id', eventId).first(); + expect(after.id).toBe(before.id); + expect(after.event_id).toBe(eventId); + expect(await db('event_feedback_settings').where('event_id', otherEventId).first()).toBeUndefined(); + }); +}); + +describe('guest feedback-settings route is not shadowed (#1030)', () => { + test('gallery.js does not declare GET /:slug/feedback-settings', () => { + const source = fs.readFileSync( + path.resolve(__dirname, '..', '..', 'src', 'routes', 'gallery.js'), 'utf8', + ); + expect(source).not.toMatch(/router\.get\(\s*['"]\/:slug\/feedback-settings['"]/); + }); + + test('galleryFeedback.js still serves it, including the #655 per-guest caps', () => { + const source = fs.readFileSync( + path.resolve(__dirname, '..', '..', 'src', 'routes', 'galleryFeedback.js'), 'utf8', + ); + expect(source).toMatch(/router\.get\(\s*['"]\/:slug\/feedback-settings['"]/); + expect(source).toMatch(/max_favorites_per_guest/); + expect(source).toMatch(/max_likes_per_guest/); + }); +}); diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index 25838962..e2d8c331 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -1890,26 +1890,11 @@ router.get('/:slug/preview/:photoId', } ); -// Get feedback settings for gallery -router.get('/:slug/feedback-settings', verifyGalleryAccess, async (req, res) => { - try { - const feedbackService = require('../services/feedbackService'); - const settings = await feedbackService.getEventFeedbackSettings(req.event.id); - - res.json({ - feedback_enabled: settings.feedback_enabled || false, - allow_ratings: settings.allow_ratings, - allow_likes: settings.allow_likes, - allow_comments: settings.allow_comments, - allow_favorites: settings.allow_favorites, - show_feedback_to_guests: settings.show_feedback_to_guests, - require_name_email: settings.require_name_email || false, - identity_mode: settings.identity_mode || 'simple' - }); - } catch (error) { - errorResponse(res, error, 500, 'Failed to fetch feedback settings'); - } -}); +// GET /:slug/feedback-settings lives in galleryFeedback.js. A duplicate of it +// used to sit here, and since server.js mounts galleryRoutes before +// galleryFeedback it shadowed the real handler — dropping the per-guest caps +// (#655) from the guest payload, so the gallery could never render the +// favorite/like limits or their counters (#1030). // Get photo stats router.get('/:slug/stats', verifyGalleryAccess, async (req, res) => { diff --git a/backend/src/services/feedbackService.js b/backend/src/services/feedbackService.js index 96613dc9..1e87f9d5 100644 --- a/backend/src/services/feedbackService.js +++ b/backend/src/services/feedbackService.js @@ -2,6 +2,38 @@ const { db, logActivity } = require('../database/db'); const logger = require('../utils/logger'); const { formatBoolean } = require('../utils/dbCompat'); +// 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`, +// `rate_limit_max_requests` — and spreading those into the UPDATE made knex +// throw, so the request 500'd and the "Enable feedback" toggle silently +// never persisted. Identity columns (id/event_id) and the timestamps stay +// server-managed. New columns MUST be added here. +const FEEDBACK_SETTINGS_COLUMNS = [ + 'feedback_enabled', + 'allow_ratings', + 'allow_likes', + 'allow_comments', + 'allow_favorites', + 'require_name_email', + 'moderate_comments', + 'require_moderation', + 'show_feedback_to_guests', + 'identity_mode', + 'max_favorites_per_guest', + 'max_likes_per_guest' +]; + +function pickSettingsColumns(settings) { + const picked = {}; + for (const column of FEEDBACK_SETTINGS_COLUMNS) { + if (Object.prototype.hasOwnProperty.call(settings || {}, column)) { + picked[column] = settings[column]; + } + } + return picked; +} + class FeedbackService { /** * Get feedback settings for an event @@ -53,25 +85,27 @@ class FeedbackService { const existing = await db('event_feedback_settings') .where('event_id', eventId) .first(); - + + const writable = pickSettingsColumns(settings); + if (existing) { await db('event_feedback_settings') .where('event_id', eventId) .update({ - ...settings, - updated_at: new Date() + ...writable, + updated_at: new Date().toISOString() }); } else { await db('event_feedback_settings').insert({ event_id: eventId, - ...settings, - created_at: new Date(), - updated_at: new Date() + ...writable, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString() }); } - - await logActivity('feedback_settings_updated', settings, eventId); - + + await logActivity('feedback_settings_updated', writable, eventId); + return this.getEventFeedbackSettings(eventId); } catch (error) { logger.error('Error updating feedback settings:', error); diff --git a/frontend/src/pages/admin/EventDetailsPage.tsx b/frontend/src/pages/admin/EventDetailsPage.tsx index 19ffed85..043f8242 100644 --- a/frontend/src/pages/admin/EventDetailsPage.tsx +++ b/frontend/src/pages/admin/EventDetailsPage.tsx @@ -524,11 +524,18 @@ export const EventDetailsPage: React.FC = () => { // Update event details updateMutation.mutate(updateData); - // Update feedback settings separately + // Update feedback settings separately. This is its own request, so a + // failure here is NOT covered by updateMutation's onError (#1030) — the + // old bare catch left the admin looking at "Event updated successfully" + // while the Guest Feedback toggle silently never persisted. try { await feedbackService.updateEventFeedbackSettings(id!, feedbackSettings); - } catch { - // Error already handled by mutation + queryClient.invalidateQueries({ queryKey: ['admin-event-feedback-settings', id] }); + } catch (error: any) { + toast.error( + error?.response?.data?.error + || t('feedback.settingsUpdateError', 'Failed to update settings') + ); } };