From e7fdf75a6c82a64272e5a76f99a6ec57a2e402c0 Mon Sep 17 00:00:00 2001 From: Matt Date: Tue, 2 Jun 2026 13:18:28 +0200 Subject: [PATCH] =?UTF-8?q?fix(audit):=20residential/tenancies=20=E2=80=94?= =?UTF-8?q?=20M28=20(unified=20stage=20validation),=20M29=20(explicit-disa?= =?UTF-8?q?ble=20wins),=20L31=20(active-tenancy=20warning),=20L32=20(socke?= =?UTF-8?q?t=20event=20+=20saveStages=20tx)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updated tenancy-auto-create integration test to assert M29 (explicit disable respected) instead of the old re-enable behavior. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../v1/residential/interests/bulk/route.ts | 22 ++++- src/components/berths/berth-tenancies-tab.tsx | 1 + src/components/clients/client-detail.tsx | 1 + src/lib/services/berth-tenancies.service.ts | 31 ++++++- .../services/residential-stages.service.ts | 83 +++++++++++++------ src/lib/services/residential.service.ts | 10 +++ src/lib/services/tenancies-module.service.ts | 24 +++++- src/lib/socket/events.ts | 2 + tests/integration/tenancy-auto-create.test.ts | 9 +- 9 files changed, 149 insertions(+), 34 deletions(-) diff --git a/src/app/api/v1/residential/interests/bulk/route.ts b/src/app/api/v1/residential/interests/bulk/route.ts index 5acd1bd7..2e95b5bb 100644 --- a/src/app/api/v1/residential/interests/bulk/route.ts +++ b/src/app/api/v1/residential/interests/bulk/route.ts @@ -5,11 +5,11 @@ import { withAuth, withRateLimit } from '@/lib/api/helpers'; import { parseBody } from '@/lib/api/route-helpers'; import { errorResponse } from '@/lib/errors'; import { assertResidentialModuleEnabled } from '@/lib/services/residential-module.service'; +import { assertValidStage } from '@/lib/services/residential-stages.service'; import { archiveResidentialInterest, updateResidentialInterest, } from '@/lib/services/residential.service'; -import { PIPELINE_STAGES } from '@/lib/validators/residential'; /** * Synchronous bulk endpoint for the residential interests list - mirrors @@ -24,7 +24,13 @@ const bulkSchema = z.discriminatedUnion('action', [ z.object({ action: z.literal('change_stage'), ids: z.array(z.string().min(1)).min(1).max(100), - pipelineStage: z.enum(PIPELINE_STAGES), + // Accept any non-empty string at the schema layer; membership against + // the port's live stage list (built-ins OR admin-customized) is + // enforced at runtime via `assertValidStage` inside the handler. A + // hardcoded enum here would 400 every valid custom stage after an + // admin renames/adds stages, while the per-row PATCH wrote arbitrary + // strings unchecked — this unifies both on one runtime check. + pipelineStage: z.string().min(1), }), z.object({ action: z.literal('archive'), @@ -62,6 +68,18 @@ export const POST = withAuth( return NextResponse.json({ error: 'Forbidden' }, { status: 403 }); } + // Validate the target stage once up-front against the port's live + // stage list so an invalid stage 400s the whole request instead of + // reporting every row as failed. `updateResidentialInterest` also + // re-checks per row (defense in depth for direct callers). + if (body.action === 'change_stage') { + try { + await assertValidStage(ctx.portId, body.pipelineStage); + } catch (error) { + return errorResponse(error); + } + } + const meta = { userId: ctx.userId, portId: ctx.portId, diff --git a/src/components/berths/berth-tenancies-tab.tsx b/src/components/berths/berth-tenancies-tab.tsx index c8cb33fa..b250959f 100644 --- a/src/components/berths/berth-tenancies-tab.tsx +++ b/src/components/berths/berth-tenancies-tab.tsx @@ -34,6 +34,7 @@ export function BerthTenanciesTab({ berthId }: BerthTenanciesTabProps) { useRealtimeInvalidation({ 'berth_tenancy:created': [['berths', berthId, 'tenancies']], 'berth_tenancy:activated': [['berths', berthId, 'tenancies']], + 'berth_tenancy:updated': [['berths', berthId, 'tenancies']], 'berth_tenancy:ended': [['berths', berthId, 'tenancies']], 'berth_tenancy:cancelled': [['berths', berthId, 'tenancies']], }); diff --git a/src/components/clients/client-detail.tsx b/src/components/clients/client-detail.tsx index ecaa9a9d..b36a88e6 100644 --- a/src/components/clients/client-detail.tsx +++ b/src/components/clients/client-detail.tsx @@ -116,6 +116,7 @@ export function ClientDetail({ clientId, currentUserId }: ClientDetailProps) { 'company_membership:ended': [['clients', clientId]], 'berth_tenancy:created': [['clients', clientId]], 'berth_tenancy:activated': [['clients', clientId]], + 'berth_tenancy:updated': [['clients', clientId]], 'berth_tenancy:ended': [['clients', clientId]], 'berth_tenancy:cancelled': [['clients', clientId]], }); diff --git a/src/lib/services/berth-tenancies.service.ts b/src/lib/services/berth-tenancies.service.ts index efc8b3cc..8fccec4f 100644 --- a/src/lib/services/berth-tenancies.service.ts +++ b/src/lib/services/berth-tenancies.service.ts @@ -94,7 +94,7 @@ export async function createPending( portId: string, data: CreatePendingInput, meta: AuditMeta, -): Promise { +): Promise { // Tenant-scoped existence checks (berth, client, yacht). const berth = await db.query.berths.findFirst({ where: and(eq(berths.id, data.berthId), eq(berths.portId, portId)), @@ -117,6 +117,23 @@ export async function createPending( data.clientId, ); + // Soft-block surface: the partial unique index `idx_bt_active` only + // covers `status='active'`, so any number of `pending` rows can be + // minted on a berth that already has an active tenant — they'd all + // ConflictError one-at-a-time at activate. We don't hard-reject here + // (a queued/back-to-back pending tenancy is a legitimate workflow), + // but we surface the existing active tenancy in the create response so + // the UI can warn the rep instead of letting them walk into a + // dead-end at activation time. + const activeTenancy = await db.query.berthTenancies.findFirst({ + where: and( + eq(berthTenancies.berthId, data.berthId), + eq(berthTenancies.portId, portId), + eq(berthTenancies.status, 'active'), + ), + columns: { id: true }, + }); + // Re-parse to apply coercions/defaults locally - Drizzle's .values() // wants the post-coercion shape (Date, defaulted enum), and v4's // z.input is too loose to satisfy that. @@ -175,6 +192,13 @@ export async function createPending( berthId: tenancy.berthId, }); + if (activeTenancy) { + return { + ...tenancy, + activeTenancyWarning: `This berth already has an active tenancy (${activeTenancy.id}). The new pending tenancy cannot be activated until the existing one is ended, transferred, or cancelled.`, + }; + } + return tenancy; } @@ -398,7 +422,10 @@ export async function updateTenancy( userAgent: meta.userAgent, }); - emitToRoom(`port:${portId}`, 'berth_tenancy:activated', { + // Metadata-only edit — emit `:updated`, NOT `:activated`. The latter + // would fire false "tenancy activated" toasts and the wrong cache + // invalidation on connected clients (the status hasn't changed here). + emitToRoom(`port:${portId}`, 'berth_tenancy:updated', { tenancyId, berthId: updated.berthId, }); diff --git a/src/lib/services/residential-stages.service.ts b/src/lib/services/residential-stages.service.ts index 3cfb438f..207c4834 100644 --- a/src/lib/services/residential-stages.service.ts +++ b/src/lib/services/residential-stages.service.ts @@ -21,7 +21,7 @@ import { db } from '@/lib/db'; import { residentialInterests } from '@/lib/db/schema/residential'; import { systemSettings } from '@/lib/db/schema'; import { createAuditLog, type AuditMeta } from '@/lib/audit'; -import { ConflictError } from '@/lib/errors'; +import { ConflictError, ValidationError } from '@/lib/errors'; const SETTING_KEY = 'residential_pipeline_stages'; @@ -59,6 +59,25 @@ export async function listStages(portId: string): Promise { return valid.length > 0 ? valid : DEFAULT_STAGES; } +/** + * Runtime membership check for a pipeline-stage id against the port's + * live stage list (per-port custom set, or the defaults when unset). + * Both the bulk endpoint and the per-row PATCH route mutations through + * here so they accept admin-customized stages and reject non-existent + * ones identically — closing the split where bulk hardcoded the 7 + * built-ins while the per-row path wrote arbitrary strings unchecked. + */ +export async function assertValidStage(portId: string, stageId: string): Promise { + const stages = await listStages(portId); + if (!stages.some((s) => s.id === stageId)) { + throw new ValidationError( + `Pipeline stage '${stageId}' is not configured for this port. Valid stages: ${stages + .map((s) => s.id) + .join(', ')}.`, + ); + } +} + /** * Return interest ids currently parked at a stage that is NOT in the * proposed new list. Empty array means the swap is safe. @@ -105,10 +124,11 @@ export async function saveStages(args: SaveStagesArgs, meta: AuditMeta): Promise ); } - // Apply reassignments first (so any orphan handlers see the new - // stage ids). One UPDATE per target stage to keep the SQL simple. + // Pre-compute the reassignment groups (one UPDATE per target stage) + // and validate every target is in the new list BEFORE opening the + // transaction, so an invalid target fails fast without a rollback. + const byTarget = new Map(); if (args.reassignments) { - const byTarget = new Map(); for (const [interestId, newStage] of Object.entries(args.reassignments)) { if (!ids.includes(newStage)) { throw new ConflictError(`Reassignment target stage '${newStage}' is not in the new list`); @@ -117,8 +137,25 @@ export async function saveStages(args: SaveStagesArgs, meta: AuditMeta): Promise list.push(interestId); byTarget.set(newStage, list); } + } + + // Read the prior setting for the audit-log diff before the write. + // The onConflictDoUpdate guards against concurrent admin saves + // race-inserting duplicates (migration 0047 made the index NULLS NOT + // DISTINCT). + const existing = await db.query.systemSettings.findFirst({ + where: and(eq(systemSettings.key, SETTING_KEY), eq(systemSettings.portId, args.portId)), + }); + + // Single transaction: the orphan reassignments and the stage-list + // UPSERT must land together. A crash between them previously left + // interests reassigned to stages that were never persisted (or vice + // versa), contradicting the docstring's one-transaction contract. + await db.transaction(async (tx) => { + // Apply reassignments first so any orphan handlers see the new + // stage ids. One UPDATE per target stage to keep the SQL simple. for (const [target, interestIds] of byTarget.entries()) { - await db + await tx .update(residentialInterests) .set({ pipelineStage: target, updatedAt: new Date() }) .where( @@ -128,30 +165,24 @@ export async function saveStages(args: SaveStagesArgs, meta: AuditMeta): Promise ), ); } - } - // Upsert the stage list. Read first for the audit-log diff; the actual - // write goes through onConflictDoUpdate so concurrent admin saves can't - // race-insert duplicates (migration 0047 made the index NULLS NOT DISTINCT). - const existing = await db.query.systemSettings.findFirst({ - where: and(eq(systemSettings.key, SETTING_KEY), eq(systemSettings.portId, args.portId)), - }); - await db - .insert(systemSettings) - .values({ - key: SETTING_KEY, - value: args.stages, - portId: args.portId, - updatedBy: meta.userId, - }) - .onConflictDoUpdate({ - target: [systemSettings.key, systemSettings.portId], - set: { + await tx + .insert(systemSettings) + .values({ + key: SETTING_KEY, value: args.stages, + portId: args.portId, updatedBy: meta.userId, - updatedAt: new Date(), - }, - }); + }) + .onConflictDoUpdate({ + target: [systemSettings.key, systemSettings.portId], + set: { + value: args.stages, + updatedBy: meta.userId, + updatedAt: new Date(), + }, + }); + }); void createAuditLog({ userId: meta.userId, diff --git a/src/lib/services/residential.service.ts b/src/lib/services/residential.service.ts index 3b25dd11..c8547181 100644 --- a/src/lib/services/residential.service.ts +++ b/src/lib/services/residential.service.ts @@ -18,6 +18,7 @@ import type { UpdateResidentialInterestInput, } from '@/lib/validators/residential'; import { sendEmail } from '@/lib/email'; +import { assertValidStage } from '@/lib/services/residential-stages.service'; import { SETTING_KEYS, getPortBrandingConfig, readSetting } from '@/lib/services/port-config'; import { brandingPrimaryColor, renderShell } from '@/lib/email/shell'; @@ -550,6 +551,15 @@ export async function updateResidentialInterest( }); if (!before) throw new NotFoundError('Residential interest'); + // Reject moves to a stage that isn't in this port's live stage list. + // The validator accepts any string (so admins can add custom stages + // without a deploy); the membership check is enforced here at write + // time so a PATCH can't park an interest on a non-existent stage that + // would then surface as an orphan in funnel reports. + if (data.pipelineStage !== undefined) { + await assertValidStage(portId, data.pipelineStage); + } + const [updated] = await db .update(residentialInterests) .set({ ...data, updatedAt: new Date() }) diff --git a/src/lib/services/tenancies-module.service.ts b/src/lib/services/tenancies-module.service.ts index ce2ccaca..9f045533 100644 --- a/src/lib/services/tenancies-module.service.ts +++ b/src/lib/services/tenancies-module.service.ts @@ -69,11 +69,33 @@ export async function isTenanciesModuleEnabled(portId: string): Promise } /** - * Idempotent helper for the webhook auto-create branch + admin-driven + * Idempotent helper for the lazy auto-create branch + admin-driven * enables to call. Inserts/updates the system_settings row to true. * Safe to call when already enabled (UPSERT on key+port). + * + * Precedence guard: an EXPLICIT admin `false` always wins. If a port- + * scoped (or global) `tenancies_module_enabled` row is stored as + * `false`, this is a NO-OP — auto-create paths (e.g. `createPending` + * minting the first-ever tenancy) must never silently flip a module the + * admin deliberately disabled back on. When the setting is UNSET, the + * lazy "first row surfaces the module" behaviour is preserved: the + * UPSERT writes `true` so the module appears without manual toggling. */ export async function enableTenanciesModule(portId: string): Promise { + // Respect an explicit stored `false` (port-scoped row first, then the + // global fallback). Only an UNSET setting auto-enables. + const settingRow = await db + .select({ value: systemSettings.value }) + .from(systemSettings) + .where( + and( + eq(systemSettings.key, 'tenancies_module_enabled'), + or(eq(systemSettings.portId, portId), isNull(systemSettings.portId)), + ), + ) + .limit(1); + if (settingRow[0]?.value === false) return; + await db .insert(systemSettings) .values({ diff --git a/src/lib/socket/events.ts b/src/lib/socket/events.ts index 9135f67b..8ce0b240 100644 --- a/src/lib/socket/events.ts +++ b/src/lib/socket/events.ts @@ -137,6 +137,8 @@ export interface ServerToClientEvents { // Berth tenancy events 'berth_tenancy:created': (payload: { tenancyId: string; berthId: string }) => void; 'berth_tenancy:activated': (payload: { tenancyId: string; berthId: string }) => void; + /** Metadata-only edit (start/end date, tenure type, notes) — status unchanged. */ + 'berth_tenancy:updated': (payload: { tenancyId: string; berthId: string }) => void; 'berth_tenancy:ended': (payload: { tenancyId: string; berthId: string }) => void; 'berth_tenancy:cancelled': (payload: { tenancyId: string; berthId: string }) => void; diff --git a/tests/integration/tenancy-auto-create.test.ts b/tests/integration/tenancy-auto-create.test.ts index 6769ecd8..29548719 100644 --- a/tests/integration/tenancy-auto-create.test.ts +++ b/tests/integration/tenancy-auto-create.test.ts @@ -145,8 +145,8 @@ describe('autoCreatePendingTenancies', () => { }); }); -describe('isTenanciesModuleEnabled (P3 lazy auto-enable on first manual createPending)', () => { - it('flips to enabled after first manual createPending', async () => { +describe('Tenancies module enable-on-createPending (audit M29: explicit-disable wins)', () => { + it('does NOT re-enable a module an admin explicitly disabled (M29)', async () => { const port = await makePort(); const portId = port.id; await disableModule(portId); @@ -167,6 +167,9 @@ describe('isTenanciesModuleEnabled (P3 lazy auto-enable on first manual createPe { userId: 'system', portId, ipAddress: '0.0.0.0', userAgent: 'test' }, ); - expect(await isTenanciesModuleEnabled(portId)).toBe(true); + // Audit M29: a manual createPending (or a Reservation-Agreement webhook) + // must respect an EXPLICIT `false` and not silently flip the module back + // on. The lazy auto-surface applies only when the setting is UNSET. + expect(await isTenanciesModuleEnabled(portId)).toBe(false); }); });