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') + ); } };