fix(audit): residential/tenancies — M28 (unified stage validation), M29 (explicit-disable wins), L31 (active-tenancy warning), L32 (socket event + saveStages tx)
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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']],
|
||||
});
|
||||
|
||||
@@ -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]],
|
||||
});
|
||||
|
||||
@@ -94,7 +94,7 @@ export async function createPending(
|
||||
portId: string,
|
||||
data: CreatePendingInput,
|
||||
meta: AuditMeta,
|
||||
): Promise<BerthTenancy> {
|
||||
): Promise<BerthTenancy & { activeTenancyWarning?: string }> {
|
||||
// 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,
|
||||
});
|
||||
|
||||
@@ -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<ResidentialStage[]> {
|
||||
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<void> {
|
||||
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<string, string[]>();
|
||||
if (args.reassignments) {
|
||||
const byTarget = new Map<string, string[]>();
|
||||
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,
|
||||
|
||||
@@ -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() })
|
||||
|
||||
@@ -69,11 +69,33 @@ export async function isTenanciesModuleEnabled(portId: string): Promise<boolean>
|
||||
}
|
||||
|
||||
/**
|
||||
* 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<void> {
|
||||
// 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({
|
||||
|
||||
@@ -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;
|
||||
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user