diff --git a/.changeset/lifecycle-guards-and-dead-fields.md b/.changeset/lifecycle-guards-and-dead-fields.md new file mode 100644 index 00000000..167e099e --- /dev/null +++ b/.changeset/lifecycle-guards-and-dead-fields.md @@ -0,0 +1,49 @@ +--- +'hotcrm': patch +--- + +Retire two rules that could never fire, give `crm_case.first_response_date` its +missing writer, and constrain the quote and contract lifecycles (#575 group B). + +`crm_lead`'s `cannot_edit_converted` validation is deleted. The code described +it as the friendly, recoverable half of a two-layer converted-lead lock, but the +`beforeUpdate` throw in `lead.hook.ts` aborts the write first, so the validation +never produced that error on any field — the same dead-configuration shape as +the `revenue_positive` rule removed in #571. The hook is now the single guard, +and its message (which names the offending fields) is the one users see. + +`crm_opportunity.created_date` is deleted. It duplicated the platform's own +`created_at` and had no writer at all, so it was null on every row while +`deal_timeline` used it as `startDateField` — the timeline had no start dates. +The view now reads `created_at` (the spelling the lead activity calendar already +used) and the four locale packs no longer label a field that does not exist. +`crm_case.created_date` is a different field with a real writer and is untouched. + +`crm_case.first_response_date` was the only member of the case SLA family with +no writer — `sla_due_date` and `resolution_time_hours` come from `case.hook`, +`is_sla_violated` from the `case_sla_monitor` flow — so the most standard +service-desk metric was permanently null. It is now stamped by the shared +`logActivityAction` body, on the first `sys_activity` a case receives: the +industry definition (Salesforce `FirstResponseDateTime`, Zendesk first reply +time) is when the customer first heard back, so a logged call or meeting is the +event, deliberately NOT a status change — an agent can move a case to "in +progress" and investigate for an hour while the customer hears nothing. The +field drops `readonly`, which would otherwise silently discard the write +(#2948), and the body reads the stored value rather than the dispatched record +so a projected record cannot turn "first response" into "last response". + +`crm_quote` and `crm_contract` gain `state_machine` validations. Neither had a +transition table OR a status guard in its hook, so `draft → accepted` on a quote +(binding numbers nobody reviewed or sent) and `draft → activated` on a contract +(which stamps `signed_date`, promotes the account to `customer` and starts the +renewal clock) were both legal. Warning severity, matching the lead / opportunity +/ case machines. `crm_campaign` and `crm_task` deliberately get nothing — their +status is descriptive, not a controlled lifecycle — and a new test pins that +absence so it stays a decision rather than a gap. + +New guards live in `test/converted-lead-guard.test.ts`, +`test/case-first-response.test.ts`, `test/opportunity-creation-date.test.ts` and +`test/status-state-machines.test.ts`. The first-response tests run the shipped +action body through the real QuickJS sandbox added in #575 A1, because the two +things that can break the stamp — the `api.read` capability and the engine +facade's `update(data, options)` signature — only exist there. diff --git a/docs/developers/api_reference.md b/docs/developers/api_reference.md index d94f2719..531c3045 100644 --- a/docs/developers/api_reference.md +++ b/docs/developers/api_reference.md @@ -36,7 +36,7 @@ Source: `src/objects/opportunity.object.ts` Key fields: -`name`, `crm_account`, `primary_contact`, `owner`, `amount`, `expected_revenue`, `stage`, `probability`, `close_date`, `created_date`, `stage_entry_date`, `type`, `lead_source`, `competitors`, `crm_campaign`, `days_in_stage`, `next_step`, `is_private`, `forecast_category`, `approval_status`, `approved_date`, `win_reason`, `loss_reason`, `loss_details` +`name`, `crm_account`, `primary_contact`, `owner`, `amount`, `expected_revenue`, `stage`, `probability`, `close_date`, `stage_entry_date`, `type`, `lead_source`, `competitors`, `crm_campaign`, `days_in_stage`, `next_step`, `is_private`, `forecast_category`, `approval_status`, `approved_date`, `win_reason`, `loss_reason`, `loss_details` ### `crm_opportunity_line_item` - Opportunity Line Item diff --git a/src/actions/global.actions.ts b/src/actions/global.actions.ts index 78c71492..017bd56b 100644 --- a/src/actions/global.actions.ts +++ b/src/actions/global.actions.ts @@ -67,6 +67,11 @@ type LogActivitySpec = { * is the direction that keeps both forms submittable, and it is the one the * body was already written for — the `duration ? … : subject` summary branch * is unreachable while the field is mandatory. + * + * The shared body is also where `crm_case.first_response_date` is stamped + * (#575 B2) — because every activity twin routes through here, "the first + * outbound contact on a case" has exactly one implementation instead of one + * per action. */ function logActivityAction(spec: LogActivitySpec): Action { const extras = Object.entries(spec.metadataExtras) @@ -117,9 +122,47 @@ function logActivityAction(spec: LogActivitySpec): Action { record_label: record[nameField] ?? null, metadata: JSON.stringify({ kind: '${spec.kind}', duration_minutes: duration, notes, ${extras} }), }); + // SLA first-response stamp (#575 B2). \`first_response_date\` was the one + // member of the case SLA family with no writer at all — \`sla_due_date\` + // and \`resolution_time_hours\` come from case.hook, \`is_sla_violated\` + // from the case_sla_monitor flow — so the metric was permanently null. + // A logged call or meeting is the only record of outbound contact a case + // carries, which makes the FIRST \`sys_activity\` on the case the moment + // the customer first heard back: the industry definition (Salesforce + // \`FirstResponseDateTime\`, Zendesk first reply time). A status change is + // deliberately NOT used — an agent can move a case to "in progress" and + // investigate for an hour while the customer hears nothing. + // + // CONVENTION: any future customer-facing path on a case (a reply-email + // action, an inbound portal reply) MUST stamp this too, or the metric + // silently under-reports. + // + // The stored value is read rather than taken from \`ctx.record\`: the + // list_item / record_related dispatch paths hand the body a PROJECTED + // record, and a field missing from that projection reads as blank — which + // would re-stamp on every log and turn "first response" into "last". + if (objectName === 'crm_case' && recordId) { + const raw = await ctx.api.object('crm_case').find({ + where: { id: recordId }, + fields: ['first_response_date'], + top: 1, + }); + const found = Array.isArray(raw) ? raw : (raw?.records ?? []); + const stored = found.length ? found[0].first_response_date : record.first_response_date; + if (!stored) { + // \`update(data, options)\` — \`ctx.api\` is the engine repo facade, + // whose update takes a DOCUMENT, not an id (mass_update_stage is the + // action that got this wrong; test/action-sandbox.test.ts pins the + // contract against a real kernel). + await ctx.api.object('crm_case').update( + { id: recordId, first_response_date: new Date().toISOString() }, + { where: { id: recordId } }, + ); + } + } return { activityId: activity?.id }; `, - capabilities: ['api.write'], + capabilities: ['api.read', 'api.write'], timeoutMs: 5000, }, locations: ['record_header', 'list_item', 'record_related'], diff --git a/src/objects/case.object.ts b/src/objects/case.object.ts index c2c79649..82a942c8 100644 --- a/src/objects/case.object.ts +++ b/src/objects/case.object.ts @@ -165,10 +165,19 @@ export const Case = ObjectSchema.create({ readonly: true, }), + // NOT `readonly`: stamped by the `log_call` / `log_meeting` activity + // actions (`src/actions/global.actions.ts`), and 16.x drops writes to + // readonly fields on user-context writes (#2948) — an action body runs as + // the acting user, so `readonly` here silently disabled the stamp. Same + // reason `is_sla_violated` and `escalated_date` below are not readonly. + // + // Definition: the moment the customer first heard back from us, matching + // Salesforce `FirstResponseDateTime` / Zendesk first reply time — NOT an + // internal status change, which would report "responded" while the + // customer is still waiting (#575 B2). first_response_date: Field.datetime({ label: 'First Response Date', group: 'sla', - readonly: true, }), resolution_time_hours: Field.number({ diff --git a/src/objects/contract.object.ts b/src/objects/contract.object.ts index 211a3381..04f24049 100644 --- a/src/objects/contract.object.ts +++ b/src/objects/contract.object.ts @@ -230,6 +230,31 @@ export const Contract = ObjectSchema.create({ // dropped in the 9.8.0 upgrade — its CEL predicate used `monthsBetween()`, // which 9.8.0's aligned CEL stdlib (ADR-0032) no longer provides. It was a // non-blocking warning; the hard end-date>start-date rule above remains. + { + // #575 B4. Like `crm_quote`, this object shipped a lifecycle vocabulary + // with no transition constraint and no status guard in `contract.hook.ts` + // — so `draft → activated` was a legal move, activating an agreement that + // never passed approval. Activation is the consequential edge here: + // `contract_on_activation` stamps `signed_date`, promotes the account to + // `customer`, and the renewal flow starts counting from it. + name: 'contract_status_progression', + type: 'state_machine', + severity: 'warning', + message: 'Invalid contract status transition', + field: 'status', + transitions: { + draft: ['in_approval', 'terminated'], + // Approval either lands (`activated`) or sends the paperwork back. + in_approval: ['draft', 'activated', 'terminated'], + // `→ expired` is the contract_expiration flow, whose own sweep filter + // is `status: 'activated'` — nothing else ages out. + activated: ['expired', 'terminated'], + // Both ends are terminal: a renewal is a NEW contract (the + // contract_renewal flow files a task, it does not revive the old row). + expired: [], + terminated: [], + }, + }, ], // Workflow Rules diff --git a/src/objects/lead.hook.ts b/src/objects/lead.hook.ts index e8ac5e82..d9ac3d4c 100644 --- a/src/objects/lead.hook.ts +++ b/src/objects/lead.hook.ts @@ -157,10 +157,15 @@ const leadHook: Hook = { // Converted-lead lock — USER edits only (`ctx.user?.id` is this repo's // system-write signal, cf. opportunity/quote/account hooks): a blanket // throw also rejected system writes (demo-bootstrap owner claims, flow - // backfills) and blocked ALL fields, far beyond the schema's own - // `cannot_edit_converted` validation (identity fields only). Narrative - // notes and framework-managed columns stay editable; identity and - // conversion fields stay locked. + // backfills). Narrative notes and framework-managed columns stay editable; + // identity and conversion fields stay locked. + // + // This is the ONLY converted-lead guard. `crm_lead` also carried a + // `cannot_edit_converted` script validation over the four identity fields, + // documented as the friendlier half of a two-layer design; #575 B1 removed + // it because this throw always won the race, so the second layer was a + // second implementation that could only drift. The message below therefore + // has to carry the whole story — hence the attempted-field list. if (event === 'beforeUpdate' && ctx.user?.id) { const previous = ctx.previous; const wasConverted = previous?.is_converted === true || previous?.status === 'converted'; diff --git a/src/objects/lead.object.ts b/src/objects/lead.object.ts index 03e711bb..482271ba 100644 --- a/src/objects/lead.object.ts +++ b/src/objects/lead.object.ts @@ -157,10 +157,15 @@ export const Lead = ObjectSchema.create({ // Conversion tracking. // NOT `readonly`: since 16.x the platform drops writes to readonly fields // outright (#2948), including the lead_conversion flow's mark_converted - // update. Edit-protection comes from two places: the `cannot_edit_converted` - // validation below (4 identity fields, recoverable error) and the broader - // beforeUpdate guard in lead.hook.ts, which rejects ANY edit to a - // converted lead. + // update. Edit-protection is the beforeUpdate guard in lead.hook.ts, which + // rejects any USER edit to a converted lead outside a small allow-list. + // A `cannot_edit_converted` validation used to sit beside it covering the + // four identity fields, described as the friendlier recoverable half of a + // two-layer design. It was dead configuration and was removed in #575 B1: + // the hook's beforeUpdate throws first, so the validation never produced + // the error it promised (measured on 16.1.0 — a `PATCH company` on a + // converted lead returns the hook's message). Same shape as the + // `revenue_positive` rule removed in #571. is_converted: Field.boolean({ label: 'Converted', defaultValue: false, @@ -316,13 +321,6 @@ export const Lead = ObjectSchema.create({ message: 'Disqualification reason is required when a lead is Unqualified', condition: P`record.status == "unqualified" && isBlank(record.disqualification_reason)`, }, - { - name: 'cannot_edit_converted', - type: 'script', - severity: 'error', - message: 'Cannot edit a converted lead', - condition: P`record.is_converted == true && (record.company != previous.company || record.email != previous.email || record.first_name != previous.first_name || record.last_name != previous.last_name)`, - }, { // Migrated from the removed top-level `stateMachines` key (LeadStateMachine). name: 'lead_status_progression', diff --git a/src/objects/opportunity.object.ts b/src/objects/opportunity.object.ts index 6fc0ec9e..9e933e08 100644 --- a/src/objects/opportunity.object.ts +++ b/src/objects/opportunity.object.ts @@ -120,12 +120,12 @@ export const Opportunity = ObjectSchema.create({ trackHistory: true, }), - created_date: Field.datetime({ - label: 'Created Date', - readonly: true, - group: 'sales_process', - }), - + // NO `created_date` here: the platform already injects `created_at` on + // every object, and this duplicate had no writer at all — not the seed + // data, not a hook, not a flow — so it was permanently null while + // `created_at` carried the real value (#575 B2). Surfaces that need the + // creation instant read `created_at` (see the deal_timeline view). + // // Stage-age clock (#489). This is the STORED half of the pair: a real, // indexed date column, so it is what automation and views may filter and // sort on. `days_in_stage` below is a formula derived from it. diff --git a/src/objects/quote.object.ts b/src/objects/quote.object.ts index 7698f34f..48a0e5e8 100644 --- a/src/objects/quote.object.ts +++ b/src/objects/quote.object.ts @@ -235,6 +235,35 @@ export const Quote = ObjectSchema.create({ message: 'Discount cannot exceed 100%', condition: P`record.discount != null && record.discount > 100`, }, + { + // #575 B4. `crm_quote` had a full lifecycle vocabulary and no transition + // constraint whatsoever — nor a status guard in `quote.hook.ts` — so a + // quote could go straight from `draft` to `accepted`, which in CPQ terms + // is a signed number nobody reviewed or sent. `warning` severity matches + // the lead / opportunity / case machines: it flags the jump on the record + // without blocking a support-driven correction. + name: 'quote_status_progression', + type: 'state_machine', + severity: 'warning', + message: 'Invalid quote status transition', + field: 'status', + transitions: { + // `→ expired` is legal from every UNSETTLED state: the quote_expiration + // flow sweeps on `expiration_date` alone and expires drafts that were + // never sent as readily as presented ones. + draft: ['in_review', 'expired'], + in_review: ['draft', 'presented', 'rejected', 'expired'], + presented: ['accepted', 'rejected', 'expired'], + // `accepted` and `expired` are terminal, and not only by convention — + // `quote_pricing_guard` in quote.hook.ts freezes both, allowing edits + // to `internal_notes` and nothing else. + accepted: [], + expired: [], + // A rejected quote is not frozen: the rep revises the numbers and + // re-issues, which starts again at `draft`. + rejected: ['draft'], + }, + }, ], // Workflow Rules diff --git a/src/translations/en.ts b/src/translations/en.ts index dadff8e7..7e73e1ff 100644 --- a/src/translations/en.ts +++ b/src/translations/en.ts @@ -310,7 +310,6 @@ export const en: TranslationData = { }, description: { label: 'Description' }, next_step: { label: 'Next Step' }, - created_date: { label: 'Created Date' }, lead_source: { label: 'Lead Source', options: { diff --git a/src/translations/es-ES.ts b/src/translations/es-ES.ts index b3f519b6..b57adc0f 100644 --- a/src/translations/es-ES.ts +++ b/src/translations/es-ES.ts @@ -288,7 +288,6 @@ export const esES: TranslationData = { }, description: { label: 'Descripción' }, next_step: { label: 'Próximo Paso' }, - created_date: { label: 'Fecha de Creación' }, lead_source: { label: 'Origen del Prospecto', options: { diff --git a/src/translations/ja-JP.ts b/src/translations/ja-JP.ts index 151f549e..2b47bb5f 100644 --- a/src/translations/ja-JP.ts +++ b/src/translations/ja-JP.ts @@ -288,7 +288,6 @@ export const jaJP: TranslationData = { }, description: { label: '説明' }, next_step: { label: '次のステップ' }, - created_date: { label: '作成日' }, lead_source: { label: 'リードソース', options: { diff --git a/src/translations/zh-CN.ts b/src/translations/zh-CN.ts index 4507a417..ea136298 100644 --- a/src/translations/zh-CN.ts +++ b/src/translations/zh-CN.ts @@ -725,7 +725,6 @@ export const zhCN: TranslationData = { }, description: { label: '描述' }, next_step: { label: '下一步' }, - created_date: { label: '创建日期' }, lead_source: { label: '线索来源', options: { diff --git a/src/views/opportunity.view.ts b/src/views/opportunity.view.ts index 0e82f6ac..9b5d1b81 100644 --- a/src/views/opportunity.view.ts +++ b/src/views/opportunity.view.ts @@ -146,7 +146,11 @@ export const OpportunityViews = defineView({ data: { provider: 'object', object: 'crm_opportunity' }, columns: ['name', 'crm_account', 'amount'], timeline: { - startDateField: 'created_date', + // `created_at`, the platform's own creation stamp — the object's + // duplicate `created_date` was removed in #575 B2 because nothing ever + // wrote it, so this timeline started every bar at null. Same spelling + // as the lead activity calendar. + startDateField: 'created_at', endDateField: 'close_date', titleField: 'name', groupByField: 'owner', diff --git a/test/case-first-response.test.ts b/test/case-first-response.test.ts new file mode 100644 index 00000000..70221909 --- /dev/null +++ b/test/case-first-response.test.ts @@ -0,0 +1,159 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import stack from '../objectstack.config'; +import { makeSandboxEngine, runActionBody, type Rec } from './helpers/action-sandbox'; + +/** + * `crm_case.first_response_date` finally has a writer (#575 B2). + * + * It was the only member of the case SLA family without one. `sla_due_date` and + * `resolution_time_hours` are stamped by `case.hook`, `is_sla_violated` by the + * `case_sla_monitor` flow — and first response, the most standard SLA metric a + * service desk reports, was permanently null while the field, its four + * translations and its place on the case detail page all advertised otherwise. + * + * The writer is the shared `logActivityAction` body in + * `src/actions/global.actions.ts`: the first `sys_activity` on a case is the + * first record that the customer heard back from anyone. An earlier proposal — + * stamp when the status first leaves `new` — was rejected and is worth stating + * as a non-goal: an agent can move a case to "in progress" and investigate for + * an hour while the customer hears nothing, so a status-derived number would + * report a response that never happened. + * + * These run the SHIPPED body under the real QuickJS sandbox rather than the + * handler-as-JS shortcut, because two of the things that can break this stamp + * only exist there: the `api.read` capability the new lookup needs, and the + * engine facade's `update(data, options)` signature (`mass_update_stage` is the + * action in this repo that got that wrong and silently never wrote). + */ + +type AnyRec = Record; + +const stackActions: AnyRec[] = (stack as any).actions ?? []; +const action = (name: string): AnyRec => { + const found = stackActions.find((a) => a.name === name); + if (!found) throw new Error(`no ${name} action registered`); + return found; +}; + +const objects: AnyRec[] = (stack as any).objects ?? []; +const crmCase = objects.find((o) => o.name === 'crm_case') as AnyRec | undefined; + +/** Both activity twins are built by the same constructor — both must stamp. */ +const TWINS = ['log_call', 'log_meeting'] as const; + +const seeded = (first_response_date: string | null = null): Record => ({ + crm_case: [{ id: 'case_1', subject: 'Login issues', status: 'new', first_response_date }], +}); + +const logOn = (name: string, engine: ReturnType, record: Rec = { id: 'case_1' }) => + runActionBody(action(name), { + objectName: 'crm_case', + record, + input: { subject: 'Called the customer back', duration: 12 }, + engine, + }); + +describe('the field is writable at all', () => { + it('crm_case.first_response_date is not readonly', () => { + // 16.x drops writes to readonly fields on user-context writes (#2948), and + // an action body runs as the acting user — so `readonly: true` here would + // make every assertion below pass in this harness and write nothing in + // production. Same reason `is_sla_violated` and `escalated_date` dropped it. + const field = crmCase?.fields?.first_response_date; + expect(field, 'crm_case.first_response_date missing').toBeTruthy(); + expect(field.readonly ?? false).toBe(false); + }); + + it('the rest of the SLA family still has its writers', () => { + // Guard against "fixed" by deletion: this test exists because the field is + // part of a set, and the set is the argument for keeping it. + for (const name of ['sla_due_date', 'resolution_time_hours', 'is_sla_violated', 'closed_date']) { + expect(crmCase?.fields?.[name], `crm_case.${name} missing`).toBeTruthy(); + } + }); +}); + +describe.each(TWINS)('%s stamps the first response', (name) => { + it('declares the read capability the lookup needs', () => { + // Without it the sandbox denies the `find` at call time and the whole + // action fails — the capability list is load-bearing metadata, not prose. + expect(action(name).body?.capabilities).toContain('api.read'); + expect(action(name).body?.capabilities).toContain('api.write'); + }); + + it('writes the current time onto a case that has none', async () => { + const before = Date.now(); + const engine = makeSandboxEngine(seeded()); + const { result } = await logOn(name, engine); + + expect(result.activityId, 'the activity itself must still be written').toBeTruthy(); + const stamped = engine.rows('crm_case')[0]!.first_response_date as string; + expect(stamped).toMatch(/^\d{4}-\d{2}-\d{2}T/); + expect(new Date(stamped).getTime()).toBeGreaterThanOrEqual(before); + expect(new Date(stamped).getTime()).toBeLessThanOrEqual(Date.now()); + }); + + it('uses the (data, options) update shape the engine facade requires', async () => { + const engine = makeSandboxEngine(seeded()); + await logOn(name, engine); + const [call] = engine.callsFor('crm_case', 'update'); + expect(call!.args[0]).toMatchObject({ id: 'case_1' }); + expect(call!.args[1]).toEqual({ where: { id: 'case_1' } }); + }); + + it('never moves a stamp that is already there', async () => { + const engine = makeSandboxEngine(seeded('2026-01-01T09:00:00.000Z')); + await logOn(name, engine); + expect(engine.rows('crm_case')[0]!.first_response_date).toBe('2026-01-01T09:00:00.000Z'); + expect(engine.callsFor('crm_case', 'update'), 'a second write would make it "last response"').toHaveLength(0); + }); + + it('leaves other objects alone', async () => { + // The twins are scoped to crm_case today, but the body's own comment keeps + // the global design alive as a restoration path — the object check is what + // stops this stamp from following it onto objects with no such field. + const engine = makeSandboxEngine({ crm_lead: [{ id: 'lead_1' }] }); + await runActionBody(action(name), { + objectName: 'crm_lead', + record: { id: 'lead_1' }, + input: { subject: 'Intro call' }, + engine, + }); + expect(engine.callsFor('crm_case')).toHaveLength(0); + }); +}); + +describe('the stamp survives the ways a case reaches the body', () => { + it('is written once across BOTH twins on the same case', async () => { + // One case, a call and then a meeting: "first response" is a property of + // the case, not of either action, so the second must find the first. + const engine = makeSandboxEngine(seeded()); + await logOn('log_call', engine); + const first = engine.rows('crm_case')[0]!.first_response_date; + await logOn('log_meeting', engine); + expect(engine.rows('crm_case')[0]!.first_response_date).toBe(first); + expect(engine.callsFor('crm_case', 'update')).toHaveLength(1); + }); + + it('reads the stored row, not the record the dispatcher handed it', async () => { + // `list_item` / `record_related` dispatch can hand the body a PROJECTED + // record. If the body trusted `ctx.record.first_response_date`, a + // projection that omits the field would read as blank and re-stamp on + // every single log — which is precisely the metric being wrong. + const engine = makeSandboxEngine(seeded('2026-01-01T09:00:00.000Z')); + await logOn('log_call', engine, { id: 'case_1', display_title: 'CASE-0001' }); + expect(engine.rows('crm_case')[0]!.first_response_date).toBe('2026-01-01T09:00:00.000Z'); + expect(engine.callsFor('crm_case', 'update')).toHaveLength(0); + }); + + it('still logs the activity when the case row cannot be read', async () => { + // A read that comes back empty must not cost the user their activity entry + // — the timeline write is the action's actual job. + const engine = makeSandboxEngine(); + const { result } = await logOn('log_call', engine); + expect(result.activityId).toBeTruthy(); + expect(engine.rows('sys_activity')).toHaveLength(1); + }); +}); diff --git a/test/converted-lead-guard.test.ts b/test/converted-lead-guard.test.ts new file mode 100644 index 00000000..5e340fd3 --- /dev/null +++ b/test/converted-lead-guard.test.ts @@ -0,0 +1,130 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import stack from '../objectstack.config'; +import leadHooks from '../src/objects/lead.hook'; +import { hookNamed, makeCtx, makeHarness } from './helpers/hook-harness'; + +/** + * The converted-lead lock is ONE guard, and it is the hook (#575 B1). + * + * `crm_lead` used to carry two: a `cannot_edit_converted` script validation + * over the four identity fields, and the `beforeUpdate` throw in + * `lead.hook.ts`. A code comment described the pair as a deliberate division of + * labour — the validation for a friendly, recoverable error on identity fields, + * the hook for a hard stop on everything else. + * + * That division never existed at runtime. Patching `company` on a converted + * lead returned the HOOK's message on 16.1.0, because a `beforeUpdate` throw + * aborts the write before validations are evaluated; the validation could not + * produce the friendlier error it promised, on any field, ever. It was the same + * shape as the `revenue_positive` rule deleted in #571: two implementations of + * one rule, one of them dead and free to drift. + * + * So the validation is gone and these tests pin what replaced it — which is + * nothing, deliberately. The hook has to carry the whole contract now, and its + * message has to be good enough to be the only one a user sees. + */ + +type AnyRec = Record; + +const objects: AnyRec[] = (stack as any).objects ?? []; +const lead = objects.find((o) => o.name === 'crm_lead') as AnyRec | undefined; +const validations = (lead?.validations ?? []) as AnyRec[]; + +const guard = hookNamed(leadHooks, 'lead_automation'); + +/** A converted lead as the hook sees it, with one attempted edit on top. */ +const editConverted = (input: AnyRec, previous: AnyRec = {}) => + guard.handler( + makeCtx({ + event: 'beforeUpdate', + input: { id: 'lead_1', ...input }, + previous: { id: 'lead_1', is_converted: true, status: 'converted', company: 'Acme', email: 'a@acme.example.com', first_name: 'Ada', last_name: 'Lovelace', ...previous }, + user: { id: 'usr_1' }, + api: makeHarness().api, + }), + ); + +describe('crm_lead declares no second converted-lead rule', () => { + it('found the lead object at all', () => { + // Otherwise every assertion below passes over an empty list. + expect(lead, 'crm_lead not registered on the stack').toBeTruthy(); + expect(validations.length).toBeGreaterThan(2); + }); + + it('has no cannot_edit_converted validation', () => { + expect(validations.map((v) => v.name)).not.toContain('cannot_edit_converted'); + }); + + it('has no validation of any name that re-implements the lock', () => { + // The name is not the point — a second implementation under a different + // name is the same defect. Anything comparing `is_converted` against the + // previous record is that rule wearing a hat. + const celSource = (condition: unknown): string => + typeof condition === 'string' ? condition : String((condition as AnyRec | null)?.source ?? ''); + const offenders = validations + .filter((v) => v.type === 'script') + .filter((v) => { + const src = celSource(v.condition); + return src.includes('is_converted') && src.includes('previous.'); + }) + .map((v) => v.name as string); + expect( + offenders, + `converted-lead edit rules re-added as validations: ${offenders.join(', ')}. ` + + 'The beforeUpdate throw in lead.hook.ts runs first, so a validation here can never fire.', + ).toEqual([]); + }); +}); + +describe('the hook is the guard that actually speaks', () => { + it.each(['company', 'email', 'first_name', 'last_name'])( + 'rejects an edit to %s — the fields the deleted validation covered', + async (field) => { + await expect(editConverted({ [field]: 'changed' })).rejects.toThrow( + /Cannot edit a converted lead/, + ); + }, + ); + + it('names the offending field, because no second error follows it', async () => { + // The deleted validation's whole claim was a friendlier message. Nothing + // gets a turn after this throw, so the diagnostic has to live here. + await expect(editConverted({ company: 'Globex' })).rejects.toThrow(/attempted: company/); + }); + + it('still rejects fields the deleted validation never covered', async () => { + await expect(editConverted({ rating: 99 })).rejects.toThrow(/attempted: rating/); + }); + + it('leaves narrative fields and system writes alone', async () => { + await expect(editConverted({ description: 'Post-conversion note' })).resolves.toBeUndefined(); + // No `user` ⇒ system / flow / seed write: the lock is for user edits only. + await expect( + guard.handler( + makeCtx({ + event: 'beforeUpdate', + input: { id: 'lead_1', company: 'Globex' }, + previous: { id: 'lead_1', is_converted: true, company: 'Acme' }, + user: undefined, + api: makeHarness().api, + }), + ), + ).resolves.toBeUndefined(); + }); + + it('does not lock an unconverted lead', async () => { + await expect( + guard.handler( + makeCtx({ + event: 'beforeUpdate', + input: { id: 'lead_1', company: 'Globex' }, + previous: { id: 'lead_1', is_converted: false, status: 'qualified', company: 'Acme' }, + user: { id: 'usr_1' }, + api: makeHarness().api, + }), + ), + ).resolves.toBeUndefined(); + }); +}); diff --git a/test/opportunity-creation-date.test.ts b/test/opportunity-creation-date.test.ts new file mode 100644 index 00000000..805d6960 --- /dev/null +++ b/test/opportunity-creation-date.test.ts @@ -0,0 +1,111 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import stack from '../objectstack.config'; + +/** + * `crm_opportunity` has one creation stamp, and the platform owns it (#575 B2). + * + * The object declared a `readonly` `created_date` beside the `created_at` the + * platform injects on every record. Nothing wrote it — not the seed loader, + * which legitimately backdates readonly fields to give the demo reports + * history, not a hook, not a flow — so it was null on every row, and the + * `deal_timeline` view that used it as `startDateField` had no start dates at + * all. Four locale packs translated a label for it. + * + * The field is deleted rather than given a writer, because the writer already + * exists and is the platform's. `crm_lead`'s activity calendar had already + * settled the spelling: `startDateField: 'created_at'`. + * + * `crm_case.created_date` is a different field with the same name and is NOT + * affected — the case seeds write it deliberately (see the seed note in + * `src/data/index.ts`) and the Service dashboard windows on it. + */ + +type AnyRec = Record; + +const objects: AnyRec[] = (stack as any).objects ?? []; +const views: AnyRec[] = (stack as any).views ?? []; +const opportunity = objects.find((o) => o.name === 'crm_opportunity') as AnyRec | undefined; + +/** A view names its object on its default list (cf. metadata-references.test.ts). */ +const objectOf = (v: AnyRec): string | undefined => + v.list?.data?.object ?? v.form?.data?.object ?? v.object; +const opportunityView = views.find((v) => objectOf(v) === 'crm_opportunity') as AnyRec | undefined; + +/** + * Every locale pack the stack ships, as `[locale, pack]`. + * + * `stack.translations` is a list of bundles each keyed BY locale, not a list of + * per-locale records — see the longer note in metadata-references.test.ts. Get + * this wrong and the staleness check below walks an empty list and passes + * forever, which is why it is paired with a non-vacuity assertion. + */ +const localePacks: [string, AnyRec][] = ((stack as any).translations ?? []).flatMap( + (bundle: AnyRec) => Object.entries(bundle) as [string, AnyRec][], +); + +describe('the duplicate creation field is gone', () => { + it('found the opportunity object and its views', () => { + expect(opportunity, 'crm_opportunity not registered').toBeTruthy(); + expect(opportunityView?.listViews, 'no crm_opportunity listViews found').toBeTruthy(); + }); + + it('crm_opportunity declares no created_date', () => { + expect(Object.keys(opportunity?.fields ?? {})).not.toContain('created_date'); + }); + + it('crm_case keeps its own created_date — this was not a global rename', () => { + const crmCase = objects.find((o) => o.name === 'crm_case') as AnyRec | undefined; + expect(crmCase?.fields?.created_date, 'crm_case.created_date was collateral damage').toBeTruthy(); + }); + + it('no locale pack still translates it', () => { + const stale = localePacks + .filter(([, pack]) => pack?.objects?.crm_opportunity?.fields?.created_date) + .map(([locale]) => locale); + expect( + stale, + `locales still labelling crm_opportunity.created_date: ${stale.join(', ')}`, + ).toEqual([]); + }); + + it('the locale packs were actually walked', () => { + // The flatten above is easy to get wrong (see the bundle-shape note in + // metadata-references.test.ts); without this the assertion above is vacuous. + expect(localePacks.length).toBeGreaterThan(3); + expect(localePacks.some(([, p]) => p?.objects?.crm_opportunity?.fields?.close_date)).toBe(true); + }); +}); + +describe('the deal timeline starts from a date that exists', () => { + const timelineViews = Object.values(opportunityView?.listViews ?? {}).filter( + (v) => v?.timeline || v?.calendar, + ); + + it('finds the timeline / calendar views to check', () => { + expect(timelineViews.length, 'no timeline or calendar view on crm_opportunity').toBeGreaterThan(0); + }); + + it('deal_timeline reads created_at', () => { + const timeline = (opportunityView?.listViews ?? {}).deal_timeline as AnyRec | undefined; + expect(timeline, 'deal_timeline view missing').toBeTruthy(); + expect(timeline!.timeline?.startDateField).toBe('created_at'); + }); + + it('no date-driven opportunity view points at a field the object does not have', () => { + // The generalisation: a `startDateField` naming a dropped field produces no + // error anywhere — the bars just have no start. `created_at` and friends + // are real, sortable columns the platform injects but never lists in + // `fields` (same allowance metadata-references.test.ts makes). + const SYSTEM_FIELDS = ['id', 'created_at', 'updated_at', 'created_by', 'updated_by']; + const known = new Set([...Object.keys(opportunity?.fields ?? {}), ...SYSTEM_FIELDS]); + const bad = timelineViews.flatMap((v) => + [v.timeline?.startDateField, v.timeline?.endDateField, v.calendar?.startDateField, v.calendar?.endDateField] + .filter((f): f is string => typeof f === 'string') + .filter((f) => !known.has(f)) + .map((f) => `${String(v.name)}: ${f}`), + ); + expect(bad, `opportunity views anchored on non-existent date fields:\n ${bad.join('\n ')}`).toEqual([]); + }); +}); diff --git a/test/status-state-machines.test.ts b/test/status-state-machines.test.ts new file mode 100644 index 00000000..083ea035 --- /dev/null +++ b/test/status-state-machines.test.ts @@ -0,0 +1,167 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import stack from '../objectstack.config'; + +/** + * Which status fields are governed by a transition table, and why (#575 B4). + * + * Only `crm_lead`, `crm_opportunity` and `crm_case` declared a `state_machine` + * validation. That looked like a coverage gap across every remaining object + * with a status field, and it was not — the test is whether an illegal + * transition has a business consequence: + * + * • `crm_quote` and `crm_contract` — YES, and neither had so much as a status + * guard in its hook. A quote could go `draft → accepted` (a number nobody + * reviewed or sent, now binding), and a contract `draft → activated`, which + * stamps `signed_date`, promotes the account to `customer` and starts the + * renewal clock — on an agreement that never passed approval. + * • `crm_campaign` and `crm_task` — NO. Their `status` is DESCRIPTIVE + * (running / paused, not started / in progress), not a controlled + * lifecycle. A transition table there would reject ordinary edits and teach + * users to ignore the warning. + * + * The second half is the part worth pinning: "deliberately absent" and + * "forgotten" look identical in the metadata, and only one of them should stay + * that way. + */ + +type AnyRec = Record; + +const objects: AnyRec[] = (stack as any).objects ?? []; +const objectNamed = (name: string): AnyRec => { + const found = objects.find((o) => o.name === name); + if (!found) throw new Error(`object ${name} not registered`); + return found; +}; + +const machinesOf = (name: string): AnyRec[] => + ((objectNamed(name).validations ?? []) as AnyRec[]).filter((v) => v.type === 'state_machine'); + +/** Objects that must be governed, and the field the table governs. */ +const GOVERNED = [ + { object: 'crm_lead', field: 'status' }, + { object: 'crm_opportunity', field: 'stage' }, + { object: 'crm_case', field: 'status' }, + { object: 'crm_quote', field: 'status' }, + { object: 'crm_contract', field: 'status' }, +] as const; + +/** Objects whose status is descriptive — a table here is a regression. */ +const UNGOVERNED = ['crm_campaign', 'crm_task'] as const; + +describe.each(GOVERNED)('$object lifecycle is constrained', ({ object, field }) => { + const [machine, ...extra] = machinesOf(object); + + it('declares exactly one state machine, over the lifecycle field', () => { + expect(machine, `${object} has no state_machine validation`).toBeTruthy(); + expect(extra, `${object} declares more than one state machine`).toEqual([]); + expect(machine!.field).toBe(field); + }); + + it('warns rather than blocks, like the three that came first', () => { + // Every machine in this repo is advisory. A hard `error` would make a + // legitimate support correction impossible without a data fix. + expect(machine!.severity).toBe('warning'); + expect(String(machine!.message ?? '')).not.toBe(''); + }); + + it('covers every option of the field, on both sides of the arrow', () => { + // A status missing from the table's KEYS is unconstrained (nothing + // describes what may follow it); a status missing from every VALUE is + // unreachable. Both are silent. + const options: string[] = ((objectNamed(object).fields?.[field]?.options ?? []) as AnyRec[]).map( + (o) => String(o.value), + ); + expect(options.length, `${object}.${field} has no options`).toBeGreaterThan(2); + + const transitions = machine!.transitions as Record; + const states = Object.keys(transitions); + expect( + options.filter((o) => !states.includes(o)), + `${object}.${field} values with no outbound rule`, + ).toEqual([]); + + const targets = new Set(Object.values(transitions).flat()); + const unreachable = options.filter((o) => !targets.has(o)); + // The initial state is legitimately unreachable — nothing transitions INTO + // `draft` / `new` / `prospecting`; a record is created there. + const initial = ((objectNamed(object).fields?.[field]?.options ?? []) as AnyRec[]) + .filter((o) => o.default === true) + .map((o) => String(o.value)); + expect( + unreachable.filter((o) => !initial.includes(o)), + `${object}.${field} values nothing can transition into`, + ).toEqual([]); + }); + + it('names no state outside the field vocabulary', () => { + const legal = new Set( + ((objectNamed(object).fields?.[field]?.options ?? []) as AnyRec[]).map((o) => String(o.value)), + ); + const transitions = machine!.transitions as Record; + const bad = Object.entries(transitions).flatMap(([from, tos]) => + [from, ...tos].filter((s) => !legal.has(s)).map((s) => `${from} → ${s}`), + ); + expect(bad, `${object} transitions naming non-existent states:\n ${bad.join('\n ')}`).toEqual([]); + }); +}); + +describe('the new tables agree with the automation that drives them', () => { + const quote = machinesOf('crm_quote')[0]!.transitions as Record; + const contract = machinesOf('crm_contract')[0]!.transitions as Record; + + it('every unsettled quote status can still expire', () => { + // `quote_expiration` sweeps on `expiration_date` alone — it expires a draft + // that was never sent as readily as a presented one. A table that omitted + // one of those edges would make the nightly sweep emit warnings. + for (const from of ['draft', 'in_review', 'presented']) { + expect(quote[from], `quote ${from} → expired is not allowed`).toContain('expired'); + } + }); + + it('quote states the pricing guard freezes are terminal', () => { + // `quote_pricing_guard` lets nothing but `internal_notes` change on an + // accepted or expired quote, so any outbound edge here would be a rule the + // hook forbids one layer down. + expect(quote.accepted).toEqual([]); + expect(quote.expired).toEqual([]); + }); + + it('a quote cannot jump from draft straight to accepted', () => { + expect(quote.draft).not.toContain('accepted'); + expect(quote.draft).not.toContain('presented'); + }); + + it('a contract reaches activation only through approval', () => { + expect(contract.draft).not.toContain('activated'); + expect(contract.in_approval).toContain('activated'); + }); + + it('only an activated contract expires', () => { + // `contract_expiration` filters on `status: 'activated'`; nothing else ages + // out, and both end states are final because a renewal is a NEW contract. + const expiring = Object.entries(contract) + .filter(([, tos]) => tos.includes('expired')) + .map(([from]) => from); + expect(expiring).toEqual(['activated']); + expect(contract.expired).toEqual([]); + expect(contract.terminated).toEqual([]); + }); +}); + +describe.each(UNGOVERNED)('%s status stays descriptive', (object) => { + it('has a status field to be tempted by', () => { + // Without this the assertion below would also pass for an object that lost + // its status field entirely. + expect(objectNamed(object).fields?.status, `${object}.status missing`).toBeTruthy(); + }); + + it('declares no state machine', () => { + expect( + machinesOf(object).map((m) => m.name), + `${object} gained a transition table. Its status describes what the record ` + + 'is doing, not a controlled lifecycle — see #575 B4 before adding one.', + ).toEqual([]); + }); +});