fix(event-types): harden setup window + catalog validation (codex review)
Three review rounds on PR #801; fixes in response: - isValidEventType: live catalog is authoritative when it has rows — a deleted or deactivated slug no longer validates via the legacy fallback (fallback now only serves an empty-catalog install) - deleteEventType: refuse deleting the last (and last ACTIVE) type; updateEventType: refuse deactivating the last active type (unknown slugs are rejected since the validator change, so an empty active catalog would brick event creation) - setup window fails closed: only an explicit stored `false` opens it (a portable-backup restore can leave the key absent) and a normal admin login durably closes it (abandoned-wizard case) - reserved bootstrap keys (setup_wizard_completed, setup_token) are stripped from ALL generic settings upserts (/general, /security, /analytics, /seo) so the marker is genuinely one-way - wizard step: deletes ordered so the catalog can never end up empty, and a genuinely failed system-type deletion reloads the list and stays on the step instead of advancing past the only window in which it can be retried - CreateEventPage: snap the hardcoded initial 'wedding' selection to the first active type when the catalog no longer contains it - v1 API: new GET /event-types (read scope) so token clients can discover valid slugs; OpenAPI enum replaced with the live-catalog description
This commit is contained in:
@@ -75,6 +75,11 @@ describe('event type deletion during the setup window (#800)', () => {
|
||||
|
||||
expect(await db('event_types').where({ slug_prefix: 'wedding' }).first()).toBeFalsy();
|
||||
expect(await db('email_templates').where({ template_key: 'event_reminder_wedding' }).first()).toBeFalsy();
|
||||
|
||||
// The deleted slug must NOT stay creatable through the legacy fallback —
|
||||
// the live catalog is authoritative while it has rows.
|
||||
expect(await eventTypeService.isValidEventType('wedding')).toBe(false);
|
||||
expect(await eventTypeService.isValidEventType('birthday')).toBe(true);
|
||||
});
|
||||
|
||||
it('does not resurrect reminder templates for deleted types on the next self-heal pass', async () => {
|
||||
@@ -118,4 +123,28 @@ describe('event type deletion during the setup window (#800)', () => {
|
||||
|
||||
await db('event_types').where({ id: other.id }).update({ is_active: 1 });
|
||||
});
|
||||
|
||||
it('fails closed when the completion marker row is missing', async () => {
|
||||
// A portable-backup restore can replace app_settings with a set that
|
||||
// predates migration 161 (which will not rerun) — absence must mean
|
||||
// "configured instance", never an open deletion window.
|
||||
await db('app_settings').where({ setting_key: 'setup_wizard_completed' }).del();
|
||||
expect(await setupService.isSetupWizardCompleted()).toBe(true);
|
||||
await setupService.markSetupWizardCompleted();
|
||||
});
|
||||
|
||||
it('refuses to delete the last remaining event type', async () => {
|
||||
// Reduce the catalog to a single custom type via direct db writes (the
|
||||
// service paths are already covered above), then hit the guard.
|
||||
const solo = await eventTypeService.createEventType({ name: 'Solo', slug_prefix: 'solo' });
|
||||
await db('events').del();
|
||||
await db('event_types').whereNot('id', solo.id).del();
|
||||
|
||||
await expect(eventTypeService.deleteEventType(solo.id))
|
||||
.rejects.toMatchObject({ code: 'LAST_TYPE' });
|
||||
|
||||
// Deactivating it would empty the ACTIVE catalog just the same.
|
||||
await expect(eventTypeService.updateEventType(solo.id, { is_active: false }))
|
||||
.rejects.toMatchObject({ code: 'LAST_ACTIVE' });
|
||||
});
|
||||
});
|
||||
|
||||
@@ -178,7 +178,7 @@ router.put('/:id', adminAuth, requirePermission('settings.edit'), [
|
||||
if (error.code === 'NOT_FOUND') {
|
||||
return res.status(404).json({ error: error.message });
|
||||
}
|
||||
if (error.code === 'DUPLICATE_SLUG_PREFIX') {
|
||||
if (error.code === 'DUPLICATE_SLUG_PREFIX' || error.code === 'LAST_ACTIVE') {
|
||||
return res.status(400).json({ error: error.message });
|
||||
}
|
||||
|
||||
@@ -216,7 +216,7 @@ router.delete('/:id', adminAuth, requirePermission('settings.edit'), [
|
||||
if (error.code === 'NOT_FOUND') {
|
||||
return res.status(404).json({ error: error.message });
|
||||
}
|
||||
if (error.code === 'SYSTEM_TYPE' || error.code === 'IN_USE') {
|
||||
if (error.code === 'SYSTEM_TYPE' || error.code === 'IN_USE' || error.code === 'LAST_TYPE') {
|
||||
return res.status(400).json({ error: error.message });
|
||||
}
|
||||
|
||||
|
||||
@@ -31,6 +31,19 @@ const watermarkGeneratorService = require('../services/watermarkGeneratorService
|
||||
|
||||
const getStoragePath = () => process.env.STORAGE_PATH || path.join(__dirname, '../../../storage');
|
||||
|
||||
// Reserved first-run bootstrap keys — never writable through the generic
|
||||
// settings upserts in this file: setup_wizard_completed is a one-way marker
|
||||
// (#800; writing false would reopen system-event-type deletion) and
|
||||
// setup_token is the first-run bootstrap secret. Every handler that loops
|
||||
// arbitrary request keys into app_settings must strip these first.
|
||||
const RESERVED_SETTING_KEYS = ['setup_wizard_completed', 'setup_token'];
|
||||
const stripReservedSettingKeys = (settings) => {
|
||||
for (const key of RESERVED_SETTING_KEYS) {
|
||||
delete settings[key];
|
||||
}
|
||||
return settings;
|
||||
};
|
||||
|
||||
// Configure multer for logo uploads
|
||||
const storage = multer.diskStorage({
|
||||
destination: async (req, file, cb) => {
|
||||
@@ -906,7 +919,7 @@ router.put('/theme', adminAuth, requirePermission('settings.edit'), async (req,
|
||||
// Update general settings
|
||||
router.put('/general', adminAuth, requirePermission('settings.edit'), async (req, res) => {
|
||||
try {
|
||||
const settings = { ...req.body };
|
||||
const settings = stripReservedSettingKeys({ ...req.body });
|
||||
let uploadLimitTouched = false;
|
||||
|
||||
const publicSiteKeysTouched = Object.keys(settings).some((key) => key.startsWith('general_public_site_'));
|
||||
@@ -1017,7 +1030,7 @@ router.put('/general', adminAuth, requirePermission('settings.edit'), async (req
|
||||
// Update security settings
|
||||
router.put('/security', adminAuth, requirePermission('settings.edit'), async (req, res) => {
|
||||
try {
|
||||
const settings = req.body;
|
||||
const settings = stripReservedSettingKeys({ ...req.body });
|
||||
|
||||
// Update or insert each setting
|
||||
for (const [key, value] of Object.entries(settings)) {
|
||||
@@ -1055,7 +1068,7 @@ router.put('/security', adminAuth, requirePermission('settings.edit'), async (re
|
||||
// Update analytics settings
|
||||
router.put('/analytics', adminAuth, requirePermission('settings.edit'), async (req, res) => {
|
||||
try {
|
||||
const settings = req.body;
|
||||
const settings = stripReservedSettingKeys({ ...req.body });
|
||||
|
||||
// Validate the provider switch (#663 Phase 1). Reject unknown values
|
||||
// so the dashboard route's factory doesn't have to defensively guard.
|
||||
@@ -1110,7 +1123,7 @@ router.put('/analytics', adminAuth, requirePermission('settings.edit'), async (r
|
||||
// Update SEO settings
|
||||
router.put('/seo', adminAuth, requirePermission('settings.edit'), async (req, res) => {
|
||||
try {
|
||||
const settings = req.body;
|
||||
const settings = stripReservedSettingKeys({ ...req.body });
|
||||
|
||||
// Validate seo_blocked_ai_agents is an array of strings
|
||||
if (settings.seo_blocked_ai_agents !== undefined) {
|
||||
|
||||
@@ -45,6 +45,17 @@ const router = express.Router();
|
||||
async function completeAdminLogin(req, res, admin, ipAddress, userAgent, lockoutKey) {
|
||||
await trackSuccessfulLogin(lockoutKey, ipAddress, userAgent);
|
||||
|
||||
// A normal login means the first-run wizard is over — the wizard never hits
|
||||
// this route (setup sets its cookie directly). Close the system-event-type
|
||||
// deletion window durably even when the wizard was abandoned mid-way (#800).
|
||||
// Best-effort: a failure here must never block a login.
|
||||
try {
|
||||
const setupService = require('../services/setupService');
|
||||
if (!(await setupService.isSetupWizardCompleted())) {
|
||||
await setupService.markSetupWizardCompleted();
|
||||
}
|
||||
} catch (_) { /* best-effort */ }
|
||||
|
||||
await db('admin_users').where('id', admin.id).update({
|
||||
last_login: new Date(),
|
||||
last_login_ip: ipAddress
|
||||
|
||||
@@ -81,7 +81,7 @@ const photoUpload = multer({
|
||||
* event_name: { type: string }
|
||||
* event_type:
|
||||
* type: string
|
||||
* enum: [wedding, birthday, corporate, other, family]
|
||||
* description: "Slug of an active event type from the catalog (Settings → Event Types). Defaults on a fresh install: wedding, birthday, corporate, other. GET /api/v1/event-types lists the live values."
|
||||
* event_date: { type: string, format: date, nullable: true }
|
||||
* customer_name: { type: string, nullable: true }
|
||||
* customer_email: { type: string, format: email, nullable: true }
|
||||
@@ -455,6 +455,48 @@ router.get(
|
||||
}
|
||||
);
|
||||
|
||||
// ──────────────────────────────────────────────────────────────────────────
|
||||
// GET /event-types — read (catalog discovery for event creation, #800)
|
||||
// ──────────────────────────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
* @openapi
|
||||
* /event-types:
|
||||
* get:
|
||||
* tags: [Events]
|
||||
* summary: List active event types
|
||||
* description: The slugs accepted as `event_type` when creating events. The catalog is admin-customizable (Settings → Event Types), so integrations should discover values here instead of hardcoding them.
|
||||
* security: [{ bearerAuth: [] }]
|
||||
* responses:
|
||||
* 200:
|
||||
* description: Active event types
|
||||
* content:
|
||||
* application/json:
|
||||
* schema:
|
||||
* type: object
|
||||
* properties:
|
||||
* eventTypes:
|
||||
* type: array
|
||||
* items:
|
||||
* type: object
|
||||
* properties:
|
||||
* slug_prefix: { type: string }
|
||||
* name: { type: string }
|
||||
* emoji: { type: string }
|
||||
*/
|
||||
router.get('/event-types', apiTokenAuth, requireApiScope('read'), async (req, res) => {
|
||||
try {
|
||||
const types = await db('event_types')
|
||||
.where('is_active', formatBoolean(true))
|
||||
.orderBy('display_order', 'asc')
|
||||
.select('slug_prefix', 'name', 'emoji');
|
||||
res.json({ eventTypes: types });
|
||||
} catch (error) {
|
||||
logger.error('v1 GET /event-types failed', { error: error.message });
|
||||
res.status(500).json({ error: 'Failed to list event types' });
|
||||
}
|
||||
});
|
||||
|
||||
// ──────────────────────────────────────────────────────────────────────────
|
||||
// GET /events/:id — read
|
||||
// ──────────────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -67,13 +67,20 @@ const getEventTypeBySlugPrefix = async (slugPrefix) => {
|
||||
const isValidEventType = async (slugPrefix) => {
|
||||
const normalized = slugPrefix.toLowerCase();
|
||||
|
||||
// Check in database
|
||||
// The live catalog is authoritative: a row decides by its active flag, and
|
||||
// a slug the admin deleted (setup wizard, #800) or deactivated must NOT
|
||||
// sneak back in through the legacy list below.
|
||||
const eventType = await getEventTypeBySlugPrefix(normalized);
|
||||
if (eventType && eventType.is_active) {
|
||||
return true;
|
||||
if (eventType) {
|
||||
return Boolean(eventType.is_active);
|
||||
}
|
||||
const anyType = await db('event_types').first('id');
|
||||
if (anyType) {
|
||||
return false;
|
||||
}
|
||||
|
||||
// Legacy fallback: Accept old hardcoded values for backward compatibility
|
||||
// Legacy fallback: only for a degenerate install with an EMPTY catalog
|
||||
// (pre-catalog schema drift) — accept the old hardcoded values.
|
||||
const legacyTypes = ['wedding', 'birthday', 'corporate', 'other'];
|
||||
return legacyTypes.includes(normalized);
|
||||
};
|
||||
@@ -198,6 +205,19 @@ const updateEventType = async (id, updates) => {
|
||||
}
|
||||
|
||||
if (updates.is_active !== undefined) {
|
||||
// Deactivating the last active type would empty the ACTIVE catalog and
|
||||
// brick event creation (unknown slugs are rejected since #800).
|
||||
if (updates.is_active === false && eventType.is_active) {
|
||||
const otherActive = await db('event_types')
|
||||
.whereNot('id', id)
|
||||
.where('is_active', formatBoolean(true))
|
||||
.first('id');
|
||||
if (!otherActive) {
|
||||
const error = new Error('Cannot deactivate the last active event type — activate another one first.');
|
||||
error.code = 'LAST_ACTIVE';
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
updateData.is_active = formatBoolean(updates.is_active);
|
||||
}
|
||||
|
||||
@@ -292,6 +312,28 @@ const deleteEventType = async (id) => {
|
||||
throw error;
|
||||
}
|
||||
|
||||
// Never delete the last remaining type — and never delete the last ACTIVE
|
||||
// one either: event creation and the quote/contract default-type resolution
|
||||
// both need at least one active catalog entry.
|
||||
const remaining = await db('event_types').whereNot('id', id).count('id as count').first();
|
||||
if (!remaining || parseInt(remaining.count) === 0) {
|
||||
const error = new Error('Cannot delete the last event type — at least one must remain.');
|
||||
error.code = 'LAST_TYPE';
|
||||
throw error;
|
||||
}
|
||||
if (eventType.is_active) {
|
||||
const remainingActive = await db('event_types')
|
||||
.whereNot('id', id)
|
||||
.where('is_active', formatBoolean(true))
|
||||
.count('id as count')
|
||||
.first();
|
||||
if (!remainingActive || parseInt(remainingActive.count) === 0) {
|
||||
const error = new Error('Cannot delete the last active event type — activate another one first.');
|
||||
error.code = 'LAST_TYPE';
|
||||
throw error;
|
||||
}
|
||||
}
|
||||
|
||||
// Quotes carry event_type too (migration 146) — a dangling slug there would
|
||||
// corrupt the quote→event conversion default chain.
|
||||
if (await hasColumnCached('quotes', 'event_type')) {
|
||||
|
||||
@@ -28,7 +28,12 @@ const SETUP_TOKEN_KEY = 'setup_token';
|
||||
const SETUP_WIZARD_COMPLETED_KEY = 'setup_wizard_completed';
|
||||
|
||||
async function isSetupWizardCompleted() {
|
||||
return (await getAppSetting(SETUP_WIZARD_COMPLETED_KEY)) === true;
|
||||
// Fail closed: only an explicit stored `false` (seeded by migration 161 on
|
||||
// a fresh, admin-less install) opens the deletion window. A missing row —
|
||||
// e.g. app_settings replaced by a portable-backup restore that predates the
|
||||
// migration, which will not rerun — means a configured instance, not a
|
||||
// first run.
|
||||
return (await getAppSetting(SETUP_WIZARD_COMPLETED_KEY)) !== false;
|
||||
}
|
||||
|
||||
async function markSetupWizardCompleted() {
|
||||
|
||||
Reference in New Issue
Block a user