diff --git a/app/actions/sales.ts b/app/actions/sales.ts index 497b2b75..838cbae1 100644 --- a/app/actions/sales.ts +++ b/app/actions/sales.ts @@ -1224,11 +1224,17 @@ export async function applySalesOrderStatusTransition( id: string, targetStatus: SoStatus, extra?: { trackingNumber?: string; shipFromWarehouseId?: string }, - options?: { pushStatusToWooCommerce?: boolean; internalBypassToken?: symbol }, + options?: { pushStatusToWooCommerce?: boolean; internalBypassToken?: symbol; skipPermissionCheck?: boolean }, ): Promise<{ success: boolean; error?: string }> { try { + // internalBypassToken skips BOTH the permission check and the state-machine + // guard — used by external systems (WooCommerce) that may legitimately force + // a mapped status. skipPermissionCheck is narrower: it skips ONLY the + // permission check (for sessionless internal callers such as the delivery + // cron) while the state-machine guard still runs, so a stale transition + // (e.g. an order cancelled after the poll's SHIPPED query) is still rejected. const bypassPermission = options?.internalBypassToken === INTERNAL_STATUS_TRANSITION_BYPASS - if (!bypassPermission) { + if (!bypassPermission && !options?.skipPermissionCheck) { await requirePermission('sales.process') } const so = await db.salesOrder.findUnique({ diff --git a/docs/workflows.md b/docs/workflows.md index 9cf5ac81..1d224982 100644 --- a/docs/workflows.md +++ b/docs/workflows.md @@ -31,6 +31,16 @@ rules. | `REFUNDED` | None | Terminal. | | `SHIPPED` | `COMPLETED`, `DELIVERED`, `PARTIALLY_REFUNDED`, `REFUNDED` | - | +The `DELIVERED` transition can be driven automatically by delivery-tracking +polling (TrackShip or the active shopping connector, via `/api/cron/delivery-status`). +That cron path runs the **same** transition guard and side effects as a manual +delivery — it does not write the status directly. Because the cron has no user +session it skips only the permission check (not the state-machine guard, unlike +the WooCommerce status-sync path), so if an order has moved out of a deliverable +state (e.g. cancelled or refunded after dispatch) between the poll's SHIPPED +query and the under-lock write, the guard rejects the change and the cron logs a +`delivery_status_skipped` warning instead of forcing it. + ### Shipments | Status | Allowed next statuses | Notes | diff --git a/lib/trackship.ts b/lib/trackship.ts index 779372bd..08648f38 100644 --- a/lib/trackship.ts +++ b/lib/trackship.ts @@ -71,6 +71,70 @@ export async function queryTrackShip(apiKey: string, trackingNumber: string, car } } +// --------------------------------------------------------------------------- +// Mark a delivered order +// --------------------------------------------------------------------------- + +type DeliveredTarget = { id: string; externalOrderNumber: string | null; source: string } +type MarkDeliveredDeps = { + transition: ( + id: string, + targetStatus: 'DELIVERED', + extra?: { trackingNumber?: string; shipFromWarehouseId?: string }, + options?: { skipPermissionCheck?: boolean }, + ) => Promise<{ success: boolean; error?: string }> + log: typeof logActivity +} + +/** + * Mark an order DELIVERED from a delivery-tracking poll. + * + * Routes through the real status-transition path rather than a raw status + * write (audit C2): the transition re-validates the state machine under a lock + * — so an order that moved to CANCELLED / REFUNDED / etc. between the SHIPPED + * query and now is rejected, not silently overwritten — and runs the same side + * effects a manual DELIVERED transition does (status_changed log, WooCommerce + * status push, cache revalidation). The cron has no session, so it passes + * skipPermissionCheck to skip ONLY the sales.process permission check; the + * state-machine guard still runs (unlike the WooCommerce bypass token, which + * skips both). Dependency-injected so the routing + skip-on-reject behaviour + * is unit-testable without the DB or external API. + */ +export async function markOrderDelivered( + target: DeliveredTarget, + deps: MarkDeliveredDeps, +): Promise<{ delivered: boolean }> { + const result = await deps.transition(target.id, 'DELIVERED', undefined, { + skipPermissionCheck: true, + }) + if (result.success) { + await deps.log({ + entityType: 'SALES_ORDER', + entityId: target.id, + action: 'delivered', + tag: 'sales', + level: 'INFO', + description: `Order ${target.externalOrderNumber} marked as delivered via ${target.source}`, + metadata: { orderNumber: target.externalOrderNumber, source: target.source }, + resolveUser: false, + }) + return { delivered: true } + } + // No longer in a state that can transition to DELIVERED (e.g. cancelled or + // refunded after dispatch). Log and skip rather than forcing it. + await deps.log({ + entityType: 'SALES_ORDER', + entityId: target.id, + action: 'delivery_status_skipped', + tag: 'sales', + level: 'WARNING', + description: `Skipped marking order ${target.externalOrderNumber} delivered via ${target.source}: ${result.error ?? 'transition rejected'}`, + metadata: { orderNumber: target.externalOrderNumber, source: target.source, error: result.error ?? null }, + resolveUser: false, + }) + return { delivered: false } +} + // Check delivery status for all shipped orders // --------------------------------------------------------------------------- @@ -106,6 +170,9 @@ export async function checkDeliveryStatus(): Promise<{ checked: number; delivere let checked = 0 let delivered = 0 + // Hoisted: the transition module is loaded once, not per delivered order. + const { applySalesOrderStatusTransition } = await import('@/app/actions/sales') + for (const order of shippedOrders) { let isDelivered = false @@ -135,22 +202,18 @@ export async function checkDeliveryStatus(): Promise<{ checked: number; delivere } if (isDelivered) { - await db.salesOrder.update({ - where: { id: order.id }, - data: { status: 'DELIVERED' }, - }) - delivered++ const resolvedSource = useShoppingConnector ? 'shopping_connector' : 'trackship' - await logActivity({ - entityType: 'SALES_ORDER', - entityId: order.id, - action: 'delivered', - tag: 'sales', - level: 'INFO', - description: `Order ${order.externalOrderNumber} marked as delivered via ${resolvedSource}`, - metadata: { orderNumber: order.externalOrderNumber, source: resolvedSource }, - resolveUser: false, - }) + try { + const result = await markOrderDelivered( + { id: order.id, externalOrderNumber: order.externalOrderNumber, source: resolvedSource }, + { transition: applySalesOrderStatusTransition, log: logActivity }, + ) + if (result.delivered) delivered++ + } catch (error) { + // Isolate per-order failures (e.g. a transient DB error while logging) + // so one bad order does not abort delivery processing for the rest. + console.error(`[delivery-status] failed to mark order ${order.id} delivered:`, error) + } } } diff --git a/tests/trackship-mark-delivered.test.ts b/tests/trackship-mark-delivered.test.ts new file mode 100644 index 00000000..ef1ccd4b --- /dev/null +++ b/tests/trackship-mark-delivered.test.ts @@ -0,0 +1,61 @@ +import assert from 'node:assert/strict' +import { test } from 'node:test' +import { markOrderDelivered } from '@/lib/trackship' + +type TransitionCall = { + id: string + targetStatus: string + extra?: { trackingNumber?: string; shipFromWarehouseId?: string } + options?: { skipPermissionCheck?: boolean } +} + +function makeDeps(transitionResult: { success: boolean; error?: string }) { + const calls: { transition: TransitionCall[]; logs: Array> } = { transition: [], logs: [] } + const deps = { + transition: async ( + id: string, + targetStatus: 'DELIVERED', + extra?: { trackingNumber?: string; shipFromWarehouseId?: string }, + options?: { skipPermissionCheck?: boolean }, + ) => { + calls.transition.push({ id, targetStatus, extra, options }) + return transitionResult + }, + log: async (entry: Record) => { calls.logs.push(entry) }, + } + return { deps, calls } +} + +test('routes delivery through the transition path skipping only permission (not a raw write)', async () => { + const { deps, calls } = makeDeps({ success: true }) + const result = await markOrderDelivered( + { id: 'so-1', externalOrderNumber: 'WC-100', source: 'trackship' }, + deps, + ) + assert.equal(result.delivered, true) + assert.equal(calls.transition.length, 1) + const call = calls.transition[0] + assert.equal(call.id, 'so-1') + assert.equal(call.targetStatus, 'DELIVERED') + // skipPermissionCheck skips only the permission gate — the state-machine + // guard still runs, so a since-cancelled order is rejected (success:false). + assert.equal(call.options?.skipPermissionCheck, true) + // Logs the successful delivery. + assert.equal(calls.logs.length, 1) + assert.equal(calls.logs[0].action, 'delivered') +}) + +test('skips and warns when the transition is rejected (order no longer SHIPPED)', async () => { + const { deps, calls } = makeDeps({ success: false, error: 'Cannot move from CANCELLED to DELIVERED' }) + const result = await markOrderDelivered( + { id: 'so-2', externalOrderNumber: 'WC-200', source: 'shopping_connector' }, + deps, + ) + // Not counted as delivered, and no forced write — only a warning log. + assert.equal(result.delivered, false) + assert.equal(calls.transition.length, 1) + assert.equal(calls.logs.length, 1) + assert.equal(calls.logs[0].action, 'delivery_status_skipped') + assert.equal(calls.logs[0].level, 'WARNING') + assert.match(String(calls.logs[0].description), /Cannot move from CANCELLED to DELIVERED/) +})