From 52db9826610a41f156b4878c142abfc71dea8b07 Mon Sep 17 00:00:00 2001 From: Paul Nothaft <53005142+the-luap@users.noreply.github.com> Date: Sun, 16 Aug 2026 21:37:17 +0200 Subject: [PATCH] fix(gallery): make per-event banner overrides actually work, both banners (#440, #932) (#1064) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(gallery): make per-event banner overrides actually work, both banners (#440, #932) The promo banner shipped with a per-event inherit/custom/off override that never reached a guest. GalleryView reads promo_mode from the /photos payload, and /photos never sent it — so every gallery resolved to 'inherit'. Setting a gallery's promo banner to "Off" did nothing; the global banner kept rendering. The info banner (#932) mirrored that shape and inherited the same gaps. Four places dropped the fields; all four now carry both banners: 1. GET /gallery/:slug/photos — send promo_mode/promo_markdown alongside the info fields. This is the fix that makes "Off" mean off. 2. POST /admin/events — the validators accepted both banners and the insert discarded them, so an API client could POST info_mode:'off', get 201, and find the row on 'inherit'. Markdown is stored only for 'custom', matching the PUT rule. 3. POST /admin/events/:id/duplicate — copy both from the source row. The dialog promises the copy "inherits the branding, behaviour, feedback, and category configuration"; a muted gallery un-muting on duplication is the opposite of that. 4. PUT /admin/events/:id — resolve the effective mode from the STORED row when a partial update sends only the markdown. Previously updates.promo_mode was undefined on such a request and the text was parked on an inherit/off gallery, then resurfaced when someone later switched it to 'custom'. The lookup is lazy: one extra query, only on that path. The two normalisation blocks are now one loop over both banners, so the pair can't drift apart again. Verified in a browser, both directions against the same global banner: promo_mode='off' -> not rendered; 'inherit' -> rendered. The /photos payload went from promo_mode ABSENT to carrying the value. * fix(gallery): thread promo into the reveal view, drop stale markdown on duplicate External review, round 1 on this PR. Two gaps in the plumbing it introduced: - The reveal-hidden branch copied only the info fields from /photos. Now that /photos carries promo too, a reveal-hidden gallery with promo_mode 'off' still fell back to 'inherit' and showed the global banner on the first load after login. Thread both banners there. - The duplicate copied markdown verbatim. A row written before the PUT normalisation landed can hold text while its mode is 'inherit'/'off', so the copy inherited hidden text that would resurface the moment someone switched it to 'custom' — violating the very invariant this PR establishes. Copy markdown only when the source mode is 'custom'. Test covers the stale-markdown source explicitly. --------- Co-authored-by: Paul Nothaft --- .../utils/bannerOverridePlumbing.test.js | 199 ++++++++++++++++++ backend/src/routes/adminEvents/crud.js | 85 ++++++-- backend/src/routes/gallery.js | 6 + .../src/components/gallery/GalleryView.tsx | 8 +- 4 files changed, 273 insertions(+), 25 deletions(-) create mode 100644 backend/__tests__/utils/bannerOverridePlumbing.test.js diff --git a/backend/__tests__/utils/bannerOverridePlumbing.test.js b/backend/__tests__/utils/bannerOverridePlumbing.test.js new file mode 100644 index 00000000..b9bf6e0e --- /dev/null +++ b/backend/__tests__/utils/bannerOverridePlumbing.test.js @@ -0,0 +1,199 @@ +/** + * Per-event banner overrides — end-to-end plumbing for BOTH banners. + * + * The promo banner (#440) shipped with per-event inherit/custom/off, but the + * override never actually reached a guest: GalleryView reads promo_mode from + * the /photos payload and /photos never sent it, so every gallery resolved to + * 'inherit'. Setting a gallery's promo banner to "Off" did nothing. The info + * banner (#932) mirrored that shape and inherited the same gaps. + * + * Four places dropped the fields. This pins all of them for both banners so + * the two stay in step: + * + * 1. GET /gallery/:slug/photos — must carry the columns + * 2. POST /admin/events — validators accepted them, insert dropped + * 3. POST /admin/events/:id/duplicate — copy promised, not delivered + * 4. PUT /admin/events/:id — partial update parked stale markdown + * + * The route-level normalisation is exercised directly against its own rules + * rather than through supertest: the intent is to pin the DATA contract, which + * is what silently broke. + */ + +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-banner-plumbing-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'banner-plumbing-secret'; + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +let db; +let cleanup; + +const baseEvent = (slug, extra = {}) => ({ + slug, + event_type: 'wedding', + event_name: slug, + 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(), + ...extra, +}); + +const insertEvent = async (slug, extra) => { + const [id] = await db('events').insert(baseEvent(slug, extra)).returning('id'); + return id?.id ?? id; +}; + +beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); +}); + +afterAll(async () => { + if (cleanup) await cleanup(); +}); + +// Mirrors the unified normalisation in adminEvents/crud.js. +function normalizeBannerUpdates(updates, stored) { + for (const field of ['promo', 'info']) { + const modeKey = `${field}_mode`; + const mdKey = `${field}_markdown`; + if (!Object.prototype.hasOwnProperty.call(updates, modeKey) + && !Object.prototype.hasOwnProperty.call(updates, mdKey)) continue; + + const effectiveMode = Object.prototype.hasOwnProperty.call(updates, modeKey) + ? updates[modeKey] + : stored[modeKey]; + + if (effectiveMode !== 'custom') { + updates[mdKey] = null; + } else if (Object.prototype.hasOwnProperty.call(updates, mdKey)) { + const md = typeof updates[mdKey] === 'string' ? updates[mdKey].trim() : ''; + updates[mdKey] = md || null; + } + } + return updates; +} + +describe('partial update resolves the mode from the stored row', () => { + it.each(['promo', 'info'])( + '%s: markdown-only PUT on an inherit gallery does not park hidden text', + (field) => { + const stored = { promo_mode: 'inherit', info_mode: 'inherit' }; + const updates = normalizeBannerUpdates({ [`${field}_markdown`]: 'hidden draft' }, stored); + + // Previously stored the text; a later switch to 'custom' resurrected it. + expect(updates[`${field}_markdown`]).toBeNull(); + }, + ); + + it.each(['promo', 'info'])('%s: markdown-only PUT on an off gallery also clears', (field) => { + const stored = { promo_mode: 'off', info_mode: 'off' }; + const updates = normalizeBannerUpdates({ [`${field}_markdown`]: 'hidden draft' }, stored); + + expect(updates[`${field}_markdown`]).toBeNull(); + }); + + it.each(['promo', 'info'])('%s: markdown-only PUT on a custom gallery is kept', (field) => { + const stored = { promo_mode: 'custom', info_mode: 'custom' }; + const updates = normalizeBannerUpdates({ [`${field}_markdown`]: ' keep me ' }, stored); + + expect(updates[`${field}_markdown`]).toBe('keep me'); + }); + + it.each(['promo', 'info'])('%s: switching away from custom clears the copy', (field) => { + const stored = { promo_mode: 'custom', info_mode: 'custom' }; + const updates = normalizeBannerUpdates( + { [`${field}_mode`]: 'off', [`${field}_markdown`]: 'stale' }, stored, + ); + + expect(updates[`${field}_markdown`]).toBeNull(); + }); + + it('leaves both banners alone when the request touches neither', () => { + const updates = normalizeBannerUpdates({ event_name: 'Renamed' }, { promo_mode: 'custom', info_mode: 'custom' }); + + expect(Object.prototype.hasOwnProperty.call(updates, 'promo_markdown')).toBe(false); + expect(Object.prototype.hasOwnProperty.call(updates, 'info_markdown')).toBe(false); + }); +}); + +describe('columns round-trip through the events table', () => { + it('stores and reads both banners independently', async () => { + const id = await insertEvent('banner-roundtrip', { + promo_mode: 'off', + info_mode: 'custom', + info_markdown: 'Use the menu button to filter.', + }); + + const row = await db('events').where({ id }).first(); + // Independent slots — muting one must not touch the other. + expect(row.promo_mode).toBe('off'); + expect(row.promo_markdown ?? null).toBeNull(); + expect(row.info_mode).toBe('custom'); + expect(row.info_markdown).toBe('Use the menu button to filter.'); + }); + + it('duplicating drops markdown left over on a non-custom source', async () => { + // A row written before the PUT normalisation landed can hold text while + // its mode is inherit/off. Copying that verbatim would smuggle hidden copy + // into the duplicate and resurrect it on the next switch to 'custom'. + const sourceId = await insertEvent('banner-dup-stale', { + promo_mode: 'off', + promo_markdown: 'stale promo text', + info_mode: 'inherit', + info_markdown: 'stale info text', + }); + const source = await db('events').where({ id: sourceId }).first(); + + const dupId = await insertEvent('banner-dup-stale-copy', { + promo_mode: source.promo_mode || 'inherit', + promo_markdown: source.promo_mode === 'custom' ? (source.promo_markdown || null) : null, + info_mode: source.info_mode || 'inherit', + info_markdown: source.info_mode === 'custom' ? (source.info_markdown || null) : null, + }); + + const dup = await db('events').where({ id: dupId }).first(); + expect(dup.promo_mode).toBe('off'); + expect(dup.promo_markdown).toBeNull(); + expect(dup.info_mode).toBe('inherit'); + expect(dup.info_markdown).toBeNull(); + }); + + it('duplicating an event carries both banners across', async () => { + const sourceId = await insertEvent('banner-dup-source', { + promo_mode: 'custom', + promo_markdown: 'Book your next session', + info_mode: 'off', + }); + const source = await db('events').where({ id: sourceId }).first(); + + // Mirrors the duplicate route's insert. + const dupId = await insertEvent('banner-dup-copy', { + promo_mode: source.promo_mode || 'inherit', + promo_markdown: source.promo_mode === 'custom' ? (source.promo_markdown || null) : null, + info_mode: source.info_mode || 'inherit', + info_markdown: source.info_mode === 'custom' ? (source.info_markdown || null) : null, + }); + + const dup = await db('events').where({ id: dupId }).first(); + expect(dup.promo_mode).toBe('custom'); + expect(dup.promo_markdown).toBe('Book your next session'); + // The muted info banner must stay muted in the copy. + expect(dup.info_mode).toBe('off'); + }); +}); diff --git a/backend/src/routes/adminEvents/crud.js b/backend/src/routes/adminEvents/crud.js index 7bdca580..39882ec9 100644 --- a/backend/src/routes/adminEvents/crud.js +++ b/backend/src/routes/adminEvents/crud.js @@ -196,7 +196,12 @@ module.exports = (router) => { // Draft mode is_draft = true, // Default photo sort - default_photo_sort = 'upload_date_desc' + default_photo_sort = 'upload_date_desc', + // Banner overrides (#440 / #932) — see the insert below. + promo_mode = 'inherit', + promo_markdown = null, + info_mode = 'inherit', + info_markdown = null } = req.body; const customerName = getCustomerNameFromPayload(req.body); @@ -448,6 +453,16 @@ module.exports = (router) => { hero_logo_visible: effectiveHeroLogoVisible, hero_logo_size: effectiveHeroLogoSize, hero_logo_position: effectiveHeroLogoPosition, + // Banner overrides. Both were accepted by the validators above and + // then dropped here, so an API client could POST info_mode:'off' or a + // custom banner, get 201, and find the row still on 'inherit'. + // Markdown is only stored for 'custom' — same rule the PUT applies. + promo_mode: ['inherit', 'custom', 'off'].includes(promo_mode) ? promo_mode : 'inherit', + promo_markdown: promo_mode === 'custom' && typeof promo_markdown === 'string' && promo_markdown.trim() + ? promo_markdown.trim() : null, + info_mode: ['inherit', 'custom', 'off'].includes(info_mode) ? info_mode : 'inherit', + info_markdown: info_mode === 'custom' && typeof info_markdown === 'string' && info_markdown.trim() + ? info_markdown.trim() : null, header_style: effectiveHeaderStyle || 'standard', hero_divider_style: effectiveDividerStyle || 'wave', hero_image_anchor: hero_image_anchor || 'center', @@ -1098,6 +1113,18 @@ module.exports = (router) => { hero_logo_visible: source.hero_logo_visible, hero_logo_size: source.hero_logo_size, hero_logo_position: source.hero_logo_position, + // Banner overrides travel with the copy: the duplicate dialog promises + // the new gallery 'inherits the branding, behaviour, feedback, and + // category configuration', and a gallery muted with 'off' silently + // un-muting on duplication is the opposite of that. + // Markdown rides along only while the mode is 'custom': rows written + // before the PUT normalisation landed can hold stale text on an + // 'inherit'/'off' gallery, and copying it would resurrect that text + // the moment someone switched the duplicate to 'custom'. + promo_mode: source.promo_mode || 'inherit', + promo_markdown: source.promo_mode === 'custom' ? (source.promo_markdown || null) : null, + info_mode: source.info_mode || 'inherit', + info_markdown: source.info_mode === 'custom' ? (source.info_markdown || null) : null, login_logo_visible: source.login_logo_visible, header_style: source.header_style || 'standard', hero_divider_style: source.hero_divider_style || 'wave', @@ -1537,30 +1564,44 @@ module.exports = (router) => { // NULL when mode is anything other than 'custom' so we don't carry // stale text after the admin switches modes. Empty markdown also // becomes NULL. - if (Object.prototype.hasOwnProperty.call(updates, 'promo_mode') - || Object.prototype.hasOwnProperty.call(updates, 'promo_markdown')) { - const mode = updates.promo_mode; - if (mode && mode !== 'custom') { - updates.promo_markdown = null; - } else if (Object.prototype.hasOwnProperty.call(updates, 'promo_markdown')) { - const md = typeof updates.promo_markdown === 'string' ? updates.promo_markdown.trim() : ''; - updates.promo_markdown = md || null; + // Banner overrides (#440 promo / #932 info). Markdown is stored ONLY + // while the mode is 'custom', so switching modes can't leave stale copy + // that reappears later. + // + // The mode must be resolved against the STORED row when a partial PUT + // sends only the markdown: reading updates.promo_mode alone leaves it + // undefined, which used to fall through to the "store it" branch and + // park hidden text on an 'inherit'/'off' gallery — text that then + // resurfaced the moment someone switched that gallery to 'custom'. + let storedBannerModes = null; + for (const field of ['promo', 'info']) { + const modeKey = `${field}_mode`; + const mdKey = `${field}_markdown`; + if (!Object.prototype.hasOwnProperty.call(updates, modeKey) + && !Object.prototype.hasOwnProperty.call(updates, mdKey)) continue; + + let effectiveMode = updates[modeKey]; + if (!Object.prototype.hasOwnProperty.call(updates, modeKey)) { + // Partial PUT that sent only the markdown — read the stored mode. + // Fetched at most once per request, and only on this path. + if (storedBannerModes === null) { + storedBannerModes = await db('events') + .where('id', id) + .select('promo_mode', 'info_mode') + .first() || {}; + } + effectiveMode = storedBannerModes[modeKey]; + } + + if (effectiveMode !== 'custom') { + updates[mdKey] = null; + } else if (Object.prototype.hasOwnProperty.call(updates, mdKey)) { + const md = typeof updates[mdKey] === 'string' ? updates[mdKey].trim() : ''; + updates[mdKey] = md || null; } } - // Per-event info-banner override (#932). Identical normalisation to - // promo above — mode != custom drops the stored text so switching - // modes can't leave stale copy behind. - if (Object.prototype.hasOwnProperty.call(updates, 'info_mode') - || Object.prototype.hasOwnProperty.call(updates, 'info_markdown')) { - const mode = updates.info_mode; - if (mode && mode !== 'custom') { - updates.info_markdown = null; - } else if (Object.prototype.hasOwnProperty.call(updates, 'info_markdown')) { - const md = typeof updates.info_markdown === 'string' ? updates.info_markdown.trim() : ''; - updates.info_markdown = md || null; - } - } + // Sync header_style / hero_divider_style from color_theme JSON when not // explicitly provided in the request body (#158). This ensures the diff --git a/backend/src/routes/gallery.js b/backend/src/routes/gallery.js index b7efe2b4..d194f012 100644 --- a/backend/src/routes/gallery.js +++ b/backend/src/routes/gallery.js @@ -945,6 +945,12 @@ router.get('/:slug/photos', verifyGalleryAccess, resolveGuest, async (req, res) hero_divider_style: req.event.hero_divider_style || 'wave', hero_image_anchor: req.event.hero_image_anchor || 'center', default_photo_sort: req.event.default_photo_sort || 'upload_date_desc', + // Promo banner override (#440). GalleryView has always read + // promo_mode from THIS payload, but it was never sent — so every + // per-event promo override silently resolved to 'inherit' and a + // gallery set to 'off' still showed the global banner. + promo_mode: req.event.promo_mode || 'inherit', + promo_markdown: req.event.promo_markdown || null, // Info banner override (#932). GalleryAuthContext refreshes its cached // event from THIS payload, so the fields have to travel here — /info // alone isn't enough, the context stops reading it once the guest is diff --git a/frontend/src/components/gallery/GalleryView.tsx b/frontend/src/components/gallery/GalleryView.tsx index a8729f53..82536567 100644 --- a/frontend/src/components/gallery/GalleryView.tsx +++ b/frontend/src/components/gallery/GalleryView.tsx @@ -765,11 +765,13 @@ export const GalleryView: React.FC = ({ slug, event }) => {