* 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 <[email protected]>
This commit is contained in:
Paul Nothaft
co-authored by
Paul Nothaft
Paul Nothaft
parent
180f19d70d
commit
52db982661
@@ -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: '[email protected]',
|
||||
admin_email: '[email protected]',
|
||||
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');
|
||||
});
|
||||
});
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user