diff --git a/.changeset/lead-disqualification-field-groups-priority-rank.md b/.changeset/lead-disqualification-field-groups-priority-rank.md new file mode 100644 index 00000000..9f017a55 --- /dev/null +++ b/.changeset/lead-disqualification-field-groups-priority-rank.md @@ -0,0 +1,50 @@ +--- +'hotcrm': patch +--- + +Enforce `crm_lead.disqualification_reason`, group the campaign and task fields, +and unify the `priority_rank` sentinel (#575 A2/A3/A4). + +**`disqualification_reason` promised required and enforced nothing.** The field +description has read "Required when status is Unqualified" since it was added, +with no validation, no hook, and no form that rendered the field at all — a +lead could sit in `unqualified` with no recorded reason, and the seeded demo +leads did. A `disqualification_reason_required` script validation now enforces +it, modelled on `crm_case.escalation_reason_required`. Because a rule with no +writer is worse than no rule — the save fails with an error the form gives the +user no way to clear — the field is now on every `crm_lead` form that exposes an +editable `status`, shown only when the status is `unqualified`, and on the lead +detail page beside the status. The four generated `unqualified` seed leads +carry a reason (rotated across four values, each with a matching note) instead +of a budget-flavoured note and nothing else. + +**`crm_campaign` and `crm_task` had no `fieldGroups`.** They were the last two +business objects with a full detail page and zero groups, so both rendered as +one flat grid — the campaign's ROI formulas inline with its name, the task's +five polymorphic `related_to_*` lookups and recurrence machinery inline with its +subject. Both now declare groups mirroring the sections their forms already use, +and every field is assigned to one. The two line-item objects stay ungrouped: +they are edited inline in the parent's grid and have no detail page to section. + +**`priority_rank` diverged between the two objects that use it.** The ordinal +exists because sorting on the `priority` select compares raw strings and inverts +urgency. Its unknown-priority fallback was `1` on `crm_case` and `2` on +`crm_task`, with field defaults to match, so the same unrecognised priority +sorted differently on the two objects — and on each it was indistinguishable +from a genuine priority (`low` / `normal` respectively). Both now use `0`, an +unranked sentinel that sorts below every real rank on the `priority_rank desc` +queues. The known ranks (1–4) are unchanged, so no seeded or stored row moves. + +The two rank maps stay hand-copied on purpose: L2 hook bodies run body-only in +the QuickJS sandbox, so a shared module constant resolves at authoring time and +arrives as `undefined` (see `_line-item-price-fill.ts`). Since the duplication +is forced, `test/priority-rank-parity.test.ts` is what keeps the copies in +agreement — it drives both hooks and asserts they rank an unknown priority +identically and consistently with each object's field default. + +New guards live in three new test files rather than being appended to +`test/metadata-references.test.ts`. Beyond the parity check above, +`test/field-groups-coverage.test.ts` requires every detail-page object to +declare groups and additionally catches the silent failures around them — a +field pointing at an undeclared group key vanishes from the layout, and a +declared group with no fields renders an empty section header. diff --git a/src/data/index.ts b/src/data/index.ts index 9eead76f..1f6be8e7 100644 --- a/src/data/index.ts +++ b/src/data/index.ts @@ -220,6 +220,23 @@ const contacts = defineSeed(Contact, { }); // ─── Leads ──────────────────────────────────────────────────────────── +/** + * Disqualification reasons for the generated `unqualified` demo leads, paired + * with the note that explains them. + * + * `crm_lead.disqualification_reason_required` rejects an unqualified lead that + * carries no reason, so every seeded unqualified row must supply one — the rows + * used to carry a budget-flavoured note and no reason at all. Rotated over four + * values so the demo's disqualification breakdown has more than one bar, and + * the note always matches the reason it sits next to. + */ +const DISQUALIFICATION_REASONS = [ + { reason: 'no_budget', note: 'No budget approved for this fiscal year — revisit next planning cycle.' }, + { reason: 'not_a_fit', note: 'Requirements sit outside what the product covers today.' }, + { reason: 'wrong_persona', note: 'Contact has no say in the buying decision and no path to an owner.' }, + { reason: 'unreachable', note: 'Six touches across email and phone over four weeks, no response.' }, +] as const; + const leads = defineSeed(Lead, { mode: 'upsert', externalId: 'email', @@ -312,7 +329,10 @@ const leads = defineSeed(Lead, { `Inbound via ${l.src.replace(/_/g, ' ')}. Evaluating a CRM to replace spreadsheets ` + `across their ${l.ind.replace(/_/g, ' ')} operation.`, ...(status === 'unqualified' - ? { notes: 'No budget approved for this fiscal year — revisit next planning cycle.' } + ? { + disqualification_reason: DISQUALIFICATION_REASONS[(i >> 2) % DISQUALIFICATION_REASONS.length].reason, + notes: DISQUALIFICATION_REASONS[(i >> 2) % DISQUALIFICATION_REASONS.length].note, + } : {}), }; }), diff --git a/src/objects/campaign.object.ts b/src/objects/campaign.object.ts index aafa838d..dfe9d690 100644 --- a/src/objects/campaign.object.ts +++ b/src/objects/campaign.object.ts @@ -26,16 +26,32 @@ export const Campaign = ObjectSchema.create({ // search silently return zero. These are real, indexed columns. searchableFields: ['name', 'campaign_code'], highlightFields: ['campaign_code', 'name', 'type', 'status', 'start_date'], - + + // Every other business object with a detail page groups its fields; campaign + // was one of the two that did not, so its detail page fell back to one flat + // 30-field grid with the ROI formulas sitting next to the campaign name. The + // keys mirror the sections the campaign form already uses (Overview / + // Schedule & Budget / Performance) so the two surfaces agree. + fieldGroups: [ + { key: 'basic', label: 'Campaign Information', icon: 'megaphone' }, + { key: 'schedule', label: 'Schedule', icon: 'calendar' }, + { key: 'budget', label: 'Budget & ROI', icon: 'dollar-sign' }, + { key: 'metrics', label: 'Performance', icon: 'bar-chart' }, + { key: 'assignment', label: 'Ownership', icon: 'user' }, + { key: 'assets', label: 'Campaign Assets', icon: 'link', defaultExpanded: false }, + ], + fields: { // AutoNumber field campaign_code: Field.autonumber({ + group: 'basic', label: 'Campaign Code', format: 'CPG-{0000}', }), // Basic Information name: Field.text({ + group: 'basic', label: 'Campaign Name', required: true, searchable: true, @@ -44,16 +60,19 @@ export const Campaign = ObjectSchema.create({ // ADR-0079 record title (was titleFormat '{campaign_code} - {name}'). display_title: Field.formula({ + group: 'basic', label: 'Display Title', expression: F`record.campaign_code + " - " + record.name`, }), description: Field.markdown({ + group: 'basic', label: 'Description', }), // Type & Channel type: Field.select({ + group: 'basic', label: 'Campaign Type', options: [ { label: 'Email', value: 'email', default: true }, @@ -68,6 +87,7 @@ export const Campaign = ObjectSchema.create({ }), channel: Field.select({ + group: 'basic', label: 'Primary Channel', options: [ { label: 'Digital', value: 'digital' }, @@ -80,6 +100,7 @@ export const Campaign = ObjectSchema.create({ // Status status: Field.select({ + group: 'basic', label: 'Status', options: [ { label: 'Planning', value: 'planning', color: '#999999', default: true }, @@ -93,29 +114,34 @@ export const Campaign = ObjectSchema.create({ // Dates start_date: Field.date({ + group: 'schedule', label: 'Start Date', required: true, }), end_date: Field.date({ + group: 'schedule', label: 'End Date', required: true, }), // Budget & ROI budgeted_cost: Field.currency({ + group: 'budget', label: 'Budgeted Cost', scale: 2, min: 0, }), actual_cost: Field.currency({ + group: 'budget', label: 'Actual Cost', scale: 2, min: 0, }), expected_revenue: Field.currency({ + group: 'budget', label: 'Expected Revenue', scale: 2, min: 0, @@ -127,6 +153,7 @@ export const Campaign = ObjectSchema.create({ // already opened up. With readonly on, the completion snapshot silently // persisted nothing but num_sent. actual_revenue: Field.currency({ + group: 'budget', label: 'Actual Revenue', scale: 2, min: 0, @@ -134,6 +161,7 @@ export const Campaign = ObjectSchema.create({ // Metrics target_size: Field.number({ + group: 'metrics', label: 'Target Size', description: 'Target number of leads/contacts', min: 0, @@ -142,43 +170,51 @@ export const Campaign = ObjectSchema.create({ // NOT `readonly`: the campaign_snapshot_metrics hook writes this rollup // (16.x drops readonly writes, #2948). Definition: total members enrolled. num_sent: Field.number({ + group: 'metrics', label: 'Number Sent', min: 0, }), num_responses: Field.number({ + group: 'metrics', label: 'Number of Responses', min: 0, }), num_leads: Field.number({ + group: 'metrics', label: 'Number of Leads', min: 0, }), num_converted_leads: Field.number({ + group: 'metrics', label: 'Converted Leads', min: 0, }), num_opportunities: Field.number({ + group: 'metrics', label: 'Opportunities Created', min: 0, }), num_won_opportunities: Field.number({ + group: 'metrics', label: 'Won Opportunities', min: 0, }), // Calculated Metrics (Formula Fields) response_rate: Field.formula({ + group: 'metrics', label: 'Response Rate %', expression: F`coalesce(record.num_sent, 0) > 0 ? (coalesce(record.num_responses, 0) * 100.0) / record.num_sent : 0.0`, scale: 2, }), roi: Field.formula({ + group: 'budget', label: 'ROI %', expression: F`coalesce(record.actual_cost, 0) > 0 ? ((coalesce(record.actual_revenue, 0) - record.actual_cost) * 100.0) / record.actual_cost : 0.0`, scale: 2, @@ -186,12 +222,13 @@ export const Campaign = ObjectSchema.create({ // Relationships parent_campaign: Field.lookup('crm_campaign', { + group: 'basic', label: 'Parent Campaign', description: 'Parent campaign in hierarchy', }), owner: Field.lookup('sys_user', { - + group: 'assignment', defaultValue: cel`os.user.id`, label: 'Campaign Owner', trackHistory: true, @@ -199,10 +236,12 @@ export const Campaign = ObjectSchema.create({ // Campaign Assets landing_page_url: Field.url({ + group: 'assets', label: 'Landing Page', }), is_active: Field.boolean({ + group: 'assets', label: 'Active', defaultValue: true, }), diff --git a/src/objects/case.hook.ts b/src/objects/case.hook.ts index 6fb0f70f..6e6f0e26 100644 --- a/src/objects/case.hook.ts +++ b/src/objects/case.hook.ts @@ -42,9 +42,17 @@ const caseValidation: Hook = { // Materialise the urgency ordinal so queue views can sort by it. Sorting // on `priority` itself compares raw strings and inverts urgency // (medium > low > high > critical). + // + // The map is declared INLINE and duplicated in task.hook.ts on purpose: L2 + // hook bodies run body-only in the QuickJS sandbox, so a shared module + // constant resolves at authoring time and arrives as `undefined` (see + // _line-item-price-fill.ts). The two maps key off different vocabularies + // (medium vs normal), but the UNKNOWN fallback must stay identical on both + // objects — `0`, the unranked sentinel that sorts below every real rank. + // `test/priority-rank-parity.test.ts` pins that agreement. if (priority) { const rank: Record = { low: 1, medium: 2, high: 3, critical: 4 }; - input.priority_rank = rank[priority] ?? 1; + input.priority_rank = rank[priority] ?? 0; } if (priority === 'critical' && !input.sla_due_date && !ctx.previous?.sla_due_date) { diff --git a/src/objects/case.object.ts b/src/objects/case.object.ts index 24fe5c2b..c2c79649 100644 --- a/src/objects/case.object.ts +++ b/src/objects/case.object.ts @@ -109,10 +109,16 @@ export const Case = ObjectSchema.create({ // buried every critical case at the bottom of the agent's queue. Select // options carry no ordinal in the spec, so the rank is materialised here // and stamped by `case_sla_defaults`; queue views sort on it instead. + // `0` is the UNRANKED sentinel, shared with crm_task.priority_rank: it is + // what a record carries when no recognised priority has been stamped, and + // it sorts below every real rank (1–4) on the `priority_rank desc` queues. + // It used to be `1` here and `2` on crm_task, so the same unknown priority + // ordered differently on the two objects — and on this object it was + // indistinguishable from a genuine `low`. priority_rank: Field.number({ label: 'Priority Rank', readonly: true, - defaultValue: 1, + defaultValue: 0, }), type: Field.select({ diff --git a/src/objects/lead.object.ts b/src/objects/lead.object.ts index 7c8d90f7..03e711bb 100644 --- a/src/objects/lead.object.ts +++ b/src/objects/lead.object.ts @@ -303,6 +303,19 @@ export const Lead = ObjectSchema.create({ message: 'Email is required', condition: P`isBlank(record.email)`, }, + { + // The field description has promised "Required when status is + // Unqualified" since the field was added, and nothing enforced it — a + // lead could sit in `unqualified` with no recorded reason, which is the + // one datum a disqualification review needs. Same shape as + // `crm_case.escalation_reason_required` (the repo's existing + // "required when state is X" idiom). + name: 'disqualification_reason_required', + type: 'script', + severity: 'error', + 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', diff --git a/src/objects/task.hook.ts b/src/objects/task.hook.ts index e3d74045..ee0a4a4f 100644 --- a/src/objects/task.hook.ts +++ b/src/objects/task.hook.ts @@ -36,9 +36,16 @@ const taskValidation: Hook = { (typeof input.priority === 'string' && input.priority) || (typeof previous?.priority === 'string' && (previous.priority as string)) || undefined; + // + // Inline and duplicated in case.hook.ts on purpose — L2 hook bodies run + // body-only in the QuickJS sandbox, so a shared module constant resolves at + // authoring time and arrives as `undefined` (see _line-item-price-fill.ts). + // The vocabularies differ (normal vs medium), but the UNKNOWN fallback must + // match crm_case: `0`, the unranked sentinel that sorts below every real + // rank. `test/priority-rank-parity.test.ts` pins that agreement. if (effPriority) { const rank: Record = { low: 1, normal: 2, high: 3, urgent: 4 }; - input.priority_rank = rank[effPriority] ?? 2; + input.priority_rank = rank[effPriority] ?? 0; } if (input.status === 'completed' && previous?.status !== 'completed') { diff --git a/src/objects/task.object.ts b/src/objects/task.object.ts index 92b8d53d..9ed9a670 100644 --- a/src/objects/task.object.ts +++ b/src/objects/task.object.ts @@ -14,9 +14,26 @@ export const Task = ObjectSchema.create({ // ADR-0090 D1/D7: OWD is an authored decision. Personal activity records. sharingModel: 'private', + // Every other business object with a detail page groups its fields; task was + // one of the two that did not, so its detail page fell back to one flat grid + // where the five polymorphic `related_to_*` lookups and the recurrence + // machinery sat inline with the subject. The keys mirror the sections the + // task form already uses (Task / Related Records / Recurrence & Effort) so + // the two surfaces agree. + fieldGroups: [ + { key: 'basic', label: 'Task Information', icon: 'info' }, + { key: 'scheduling', label: 'Scheduling', icon: 'calendar' }, + { key: 'assignment', label: 'Assignment', icon: 'user' }, + { key: 'related', label: 'Related Records', icon: 'link' }, + { key: 'recurrence', label: 'Recurrence', icon: 'refresh-ccw', defaultExpanded: false }, + { key: 'effort', label: 'Progress & Effort', icon: 'activity', defaultExpanded: false }, + { key: 'system', label: 'System', icon: 'database', defaultExpanded: false }, + ], + fields: { // Task Information subject: Field.text({ + group: 'basic', label: 'Subject', required: true, searchable: true, @@ -24,11 +41,13 @@ export const Task = ObjectSchema.create({ }), description: Field.markdown({ + group: 'basic', label: 'Description', }), // Task Management status: Field.select({ + group: 'basic', label: 'Status', required: true, trackHistory: true, @@ -47,6 +66,7 @@ export const Task = ObjectSchema.create({ }), priority: Field.select({ + group: 'basic', label: 'Priority', required: true, trackHistory: true, @@ -65,13 +85,19 @@ export const Task = ObjectSchema.create({ // Sortable ordinal for `priority` — see crm_case.priority_rank. Sorting on // the select itself compares raw strings (normal > low > high > urgent), // which pushes urgent work to the bottom of the to-do queue. + // + // `0` is the UNRANKED sentinel, identical to crm_case.priority_rank. It + // used to be `2` here and `1` there, so the same unknown priority ordered + // differently on the two objects; `2` also made an unranked task + // indistinguishable from a genuine `normal`. priority_rank: Field.number({ label: 'Priority Rank', readonly: true, - defaultValue: 2, + defaultValue: 0, }), type: Field.select({ + group: 'basic', label: 'Task Type', // Canonical set (#490) — the schedule_followup screen renders the same // list; see _picklists.ts. @@ -80,20 +106,24 @@ export const Task = ObjectSchema.create({ // Dates due_date: Field.date({ + group: 'scheduling', label: 'Due Date', }), reminder_date: Field.datetime({ + group: 'scheduling', label: 'Reminder Date/Time', }), completed_date: Field.datetime({ + group: 'scheduling', label: 'Completed Date', readonly: true, }), // Assignment owner: Field.lookup('sys_user', { + group: 'assignment', defaultValue: cel`os.user.id`, label: 'Assigned To', trackHistory: true, @@ -101,6 +131,7 @@ export const Task = ObjectSchema.create({ // Related To (Polymorphic relationship - can link to multiple object types) related_to_type: Field.select({ + group: 'related', label: 'Related To Type', options: [ { label: 'Account', value: 'crm_account' }, @@ -112,32 +143,39 @@ export const Task = ObjectSchema.create({ }), related_to_account: Field.lookup('crm_account', { + group: 'related', label: 'Related Account', }), related_to_contact: Field.lookup('crm_contact', { + group: 'related', label: 'Related Contact', }), related_to_opportunity: Field.lookup('crm_opportunity', { + group: 'related', label: 'Related Opportunity', }), related_to_lead: Field.lookup('crm_lead', { + group: 'related', label: 'Related Lead', }), related_to_case: Field.lookup('crm_case', { + group: 'related', label: 'Related Case', }), // Recurrence (for recurring tasks) is_recurring: Field.boolean({ + group: 'recurrence', label: 'Recurring Task', defaultValue: false, }), recurrence_type: Field.select({ + group: 'recurrence', label: 'Recurrence Type', options: [ { label: 'Daily', value: 'daily' }, @@ -148,23 +186,27 @@ export const Task = ObjectSchema.create({ }), recurrence_interval: Field.number({ + group: 'recurrence', label: 'Recurrence Interval', defaultValue: 1, min: 1, }), recurrence_end_date: Field.date({ + group: 'recurrence', label: 'Recurrence End Date', }), // Flags is_completed: Field.boolean({ + group: 'system', label: 'Is Completed', defaultValue: false, readonly: true, }), is_overdue: Field.boolean({ + group: 'system', label: 'Is Overdue', defaultValue: false, readonly: true, @@ -175,12 +217,14 @@ export const Task = ObjectSchema.create({ // audit trail). NOT readonly: 16.x drops flow writes to readonly fields // (#2948) — same reason crm_case.is_sla_violated/escalated_date are open. reminder_sent: Field.boolean({ + group: 'system', label: 'Reminder Sent', defaultValue: false, }), // Progress progress_percent: Field.percent({ + group: 'effort', label: 'Progress (%)', min: 0, max: 100, @@ -189,12 +233,14 @@ export const Task = ObjectSchema.create({ // Time tracking estimated_hours: Field.number({ + group: 'effort', label: 'Estimated Hours', scale: 2, min: 0, }), actual_hours: Field.number({ + group: 'effort', label: 'Actual Hours', scale: 2, min: 0, diff --git a/src/pages/lead_detail.page.ts b/src/pages/lead_detail.page.ts index 8149da3b..3b33ea84 100644 --- a/src/pages/lead_detail.page.ts +++ b/src/pages/lead_detail.page.ts @@ -140,7 +140,11 @@ export const LeadDetailPage: Page = { { name: 'detail', label: 'Lead Detail', - fields: ['status', 'rating', 'lead_source', 'owner', 'annual_revenue', 'number_of_employees'], + // `disqualification_reason` is mandatory on an + // Unqualified lead (see the validation on + // crm_lead) — the detail page has to show the + // recorded reason, not just the red status chip. + fields: ['status', 'disqualification_reason', 'rating', 'lead_source', 'owner', 'annual_revenue', 'number_of_employees'], }, { name: 'address', diff --git a/src/views/lead.view.ts b/src/views/lead.view.ts index 3f5a3dc1..980f0c54 100644 --- a/src/views/lead.view.ts +++ b/src/views/lead.view.ts @@ -163,6 +163,16 @@ export const LeadViews = defineView({ field: 'owner', required: true, }, + // `disqualification_reason` is enforced by the + // `disqualification_reason_required` validation on crm_lead, so every + // form that lets a user pick "Unqualified" must also offer the reason + // — otherwise the save fails with an error the form gives you no way + // to clear. Hidden until it applies. + { + field: 'disqualification_reason', + required: true, + visibleOn: 'status == "unqualified"', + }, ], }, { @@ -386,6 +396,13 @@ export const LeadViews = defineView({ // when the lead is keyed in. 'lead_source', 'owner', + // See the default form: `unqualified` requires a reason, so the + // reason must be reachable wherever the status can be set. + { + field: 'disqualification_reason', + required: true, + visibleOn: 'status == "unqualified"', + }, ], }, ], @@ -422,6 +439,12 @@ export const LeadViews = defineView({ 'industry', 'annual_revenue', 'number_of_employees', + // See the default form: `unqualified` requires a reason. + { + field: 'disqualification_reason', + required: true, + visibleOn: 'status == "unqualified"', + }, ], }, { @@ -485,6 +508,13 @@ export const LeadViews = defineView({ columns: 2, fields: [ { field: 'status', required: true }, + // See the default form: `unqualified` requires a reason. The status + // is editable in this step, so the wizard can land there too. + { + field: 'disqualification_reason', + required: true, + visibleOn: 'status == "unqualified"', + }, { field: 'rating', widget: 'star_rating' }, 'lead_source', { @@ -527,6 +557,12 @@ export const LeadViews = defineView({ 'email', { field: 'status', required: true }, 'owner', + // See the default form: `unqualified` requires a reason. + { + field: 'disqualification_reason', + required: true, + visibleOn: 'status == "unqualified"', + }, ], }, { @@ -573,6 +609,12 @@ export const LeadViews = defineView({ field: 'owner', visibleOn: 'status != "new"', // Only show owner after initial contact }, + // See the default form: `unqualified` requires a reason. + { + field: 'disqualification_reason', + required: true, + visibleOn: 'status == "unqualified"', + }, ], }, ], @@ -606,10 +648,18 @@ export const LeadViews = defineView({ widget: 'star_rating', visibleOn: 'status == "qualified"', // Only show rating for qualified leads }, + // The reason picklist, not just free-text notes: it is what the + // `disqualification_reason_required` validation checks, and this + // modal is the main place a rep flips a lead to Unqualified. + { + field: 'disqualification_reason', + required: true, + visibleOn: 'status == "unqualified"', + }, { field: 'notes', placeholder: 'Add notes about this status change', - visibleOn: 'status == "unqualified"', // Require notes for unqualified + visibleOn: 'status == "unqualified"', // Free-text context alongside the reason }, ], }, @@ -717,6 +767,12 @@ export const LeadViews = defineView({ required: true, visibleOn: P`record.status == "contacted" || record.status == "qualified"`, }, + // See the default form: `unqualified` requires a reason. + { + field: 'disqualification_reason', + required: true, + visibleOn: 'status == "unqualified"', + }, { field: 'notes', colSpan: 2, diff --git a/test/field-groups-coverage.test.ts b/test/field-groups-coverage.test.ts new file mode 100644 index 00000000..d1c8262b --- /dev/null +++ b/test/field-groups-coverage.test.ts @@ -0,0 +1,116 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import stack from '../objectstack.config'; + +/** + * `fieldGroups` coverage and integrity (#575 A3). + * + * `fieldGroups` is what turns a detail page from one flat grid of every column + * into the sectioned layout the rest of the app uses. It is pure metadata, so + * `os validate` is happy either way: an object with no groups renders, it just + * renders badly, and an object whose fields point at a group key that was + * renamed renders *those fields* into nothing at all. Both fail silently. + * + * `crm_campaign` and `crm_task` were the two business objects with a full + * detail page and zero groups. The two line-item objects are deliberately + * exempt: they are edited inline in their parent's grid and never get a + * sectioned detail page of their own. + */ + +type AnyRec = Record; + +const objects: AnyRec[] = (stack as any).objects ?? []; + +/** + * Objects that render as a record with a detail page, and so must group their + * fields. Line items are edited inline inside their parent's grid — they have + * no detail page to section, so they are out of scope rather than exceptions. + */ +const INLINE_ONLY = new Set(['crm_opportunity_line_item', 'crm_quote_line_item']); + +const businessObjects = objects.filter((o) => String(o.name).startsWith('crm_')); +const detailPageObjects = businessObjects.filter((o) => !INLINE_ONLY.has(o.name)); + +describe('every detail-page object groups its fields', () => { + it('the object set is non-empty and covers the line-item exemptions', () => { + // Guard the guard: a filter that stopped matching would make the coverage + // assertion below pass by asserting nothing, and a renamed line-item object + // would silently turn an exemption into a no-op. + expect(detailPageObjects.length).toBeGreaterThan(10); + const names = new Set(businessObjects.map((o) => o.name)); + for (const exempt of INLINE_ONLY) { + expect(names.has(exempt), `exempt object "${exempt}" no longer exists — drop it`).toBe(true); + } + }); + + it.each(detailPageObjects.map((o) => o.name))('%s declares fieldGroups', (name) => { + const groups = (objects.find((o) => o.name === name)?.fieldGroups ?? []) as AnyRec[]; + expect(groups.length, `${name} declares no fieldGroups`).toBeGreaterThan(0); + }); +}); + +describe('fieldGroups are internally consistent', () => { + it('group keys are unique within each object', () => { + const bad: string[] = []; + for (const obj of objects) { + const keys = ((obj.fieldGroups ?? []) as AnyRec[]).map((g) => g.key); + const seen = new Set(); + for (const key of keys) { + if (seen.has(key)) bad.push(`${obj.name}: duplicate group key "${key}"`); + seen.add(key); + } + } + expect(bad, `duplicate fieldGroup keys:\n ${bad.join('\n ')}`).toEqual([]); + }); + + it('every group carries a key and a human label', () => { + const bad: string[] = []; + for (const obj of objects) { + for (const group of (obj.fieldGroups ?? []) as AnyRec[]) { + if (!group.key) bad.push(`${obj.name}: a fieldGroup has no key`); + if (!group.label) bad.push(`${obj.name}.${group.key}: no label`); + } + } + expect(bad, `malformed fieldGroups:\n ${bad.join('\n ')}`).toEqual([]); + }); + + it('every field-level group resolves to a declared group key', () => { + // The silent failure this catches: a renamed group key leaves the fields + // that referenced it pointing at nothing, and they vanish from the layout. + const bad: string[] = []; + let checked = 0; + for (const obj of objects) { + const declared = new Set(((obj.fieldGroups ?? []) as AnyRec[]).map((g) => g.key)); + for (const [fieldName, field] of Object.entries((obj.fields ?? {}) as Record)) { + const group = field?.group; + if (group == null) continue; + checked++; + if (!declared.has(group)) { + bad.push(`${obj.name}.${fieldName}: group "${group}" is not declared in fieldGroups`); + } + } + } + expect(checked, 'no grouped fields found — the walker stopped matching').toBeGreaterThan(100); + expect(bad, `fields pointing at undeclared groups:\n ${bad.join('\n ')}`).toEqual([]); + }); + + it('no declared group is left with no fields in it', () => { + // An empty group renders as an empty section header — visible, and always + // the residue of a rename or a deleted field. + const bad: string[] = []; + for (const obj of objects) { + const declared = ((obj.fieldGroups ?? []) as AnyRec[]).map((g) => g.key); + if (declared.length === 0) continue; + const used = new Set( + Object.values((obj.fields ?? {}) as Record) + .map((f) => f?.group) + .filter((g): g is string => typeof g === 'string'), + ); + for (const key of declared) { + if (!used.has(key)) bad.push(`${obj.name}: group "${key}" has no fields`); + } + } + expect(bad, `empty fieldGroups:\n ${bad.join('\n ')}`).toEqual([]); + }); +}); diff --git a/test/lead-disqualification.test.ts b/test/lead-disqualification.test.ts new file mode 100644 index 00000000..6c87dc23 --- /dev/null +++ b/test/lead-disqualification.test.ts @@ -0,0 +1,136 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import stack from '../objectstack.config'; +import { CrmSeedData } from '../src/data/index'; + +/** + * `crm_lead.disqualification_reason` — promise vs enforcement (#575 A2). + * + * The field's own `description` has read **"Required when status is + * Unqualified"** since it was added, and nothing in the repo implemented it: + * no validation, no hook, and no form that even rendered the field. A lead + * could be parked in `unqualified` with no recorded reason, which is the one + * datum a disqualification review needs — and the seeded demo leads did + * exactly that. + * + * Enforcement now lives in the `disqualification_reason_required` script + * validation on `crm_lead`, modelled on `crm_case.escalation_reason_required`. + * A validation without a writer is worse than none, though: a form that lets a + * user pick "Unqualified" but never shows the reason field produces a save + * error the user has no way to clear. These tests pin all three halves — + * the rule, the forms that can trip it, and the seed rows that must satisfy it. + */ + +type AnyRec = Record; + +const objects: AnyRec[] = (stack as any).objects ?? []; +const views: AnyRec[] = (stack as any).views ?? []; + +const lead = objects.find((o) => o.name === 'crm_lead') as AnyRec | undefined; +// A view names its object on the default list, not at the top level (see +// `objectOf()` in metadata-references.test.ts). +const leadView = views.find( + (v) => (v.list?.data?.object ?? v.form?.data?.object ?? v.object) === 'crm_lead', +) as AnyRec | undefined; + +/** `P` compiles to `{ dialect: 'cel', source }`; older rules may be raw strings. */ +const celSource = (condition: unknown): string => + typeof condition === 'string' + ? condition + : String((condition as AnyRec | null)?.source ?? ''); + +const rule = ((lead?.validations ?? []) as AnyRec[]).find( + (v) => v.name === 'disqualification_reason_required', +); + +describe('crm_lead disqualification reason is enforced, not just documented', () => { + it('the object still carries the promise the rule exists to keep', () => { + // Guard the guard: if someone drops the description, this test would + // otherwise keep passing while the stated contract quietly disappeared. + const field = lead?.fields?.disqualification_reason; + expect(field, 'crm_lead.disqualification_reason missing').toBeTruthy(); + expect(String(field.description ?? '')).toMatch(/required when status is unqualified/i); + }); + + it('declares an error-severity script validation for it', () => { + expect(rule, 'crm_lead.disqualification_reason_required missing').toBeTruthy(); + expect(rule!.type).toBe('script'); + expect(rule!.severity).toBe('error'); + expect(String(rule!.message ?? '')).not.toBe(''); + }); + + it('the condition fires exactly on an unqualified lead with no reason', () => { + const source = celSource(rule?.condition); + // Both halves must be present: the status test and the blank test. A rule + // that only checked `isBlank(...)` would reject every non-unqualified lead. + expect(source).toMatch(/record\.status\s*==\s*"unqualified"/); + expect(source).toMatch(/isBlank\(record\.disqualification_reason\)/); + }); + + it('every lead form that can set status also offers the reason', () => { + // The writer half. A form exposing an editable `status` can put the record + // into `unqualified`; without `disqualification_reason` on the same form, + // the save fails with an error the user cannot clear. + const forms: [string, AnyRec][] = [ + ...(leadView?.form ? ([['form', leadView.form]] as [string, AnyRec][]) : []), + ...(Object.entries(leadView?.formViews ?? {}) as [string, AnyRec][]), + ]; + expect(forms.length, 'no crm_lead forms found — the walker stopped matching').toBeGreaterThan(3); + + /** Field names a form section renders, whether declared bare or as an object. */ + const fieldNames = (form: AnyRec): string[] => + ((form.sections ?? []) as AnyRec[]).flatMap((s) => + ((s.fields ?? []) as unknown[]).map((f) => + typeof f === 'string' ? f : String((f as AnyRec).field ?? ''), + ), + ); + /** Is `status` editable here, or pinned readonly (conversion wizard)? */ + const editableStatus = (form: AnyRec): boolean => + ((form.sections ?? []) as AnyRec[]).some((s) => + ((s.fields ?? []) as unknown[]).some((f) => + typeof f === 'string' + ? f === 'status' + : (f as AnyRec).field === 'status' && (f as AnyRec).readonly !== true, + ), + ); + + const bad = forms + .filter(([, form]) => editableStatus(form)) + .filter(([, form]) => !fieldNames(form).includes('disqualification_reason')) + .map(([name]) => name); + + expect( + bad, + `lead forms that can set status=unqualified but hide the reason:\n ${bad.join('\n ')}`, + ).toEqual([]); + }); +}); + +describe('seeded unqualified leads satisfy the rule they are shipped under', () => { + const leadSeed = CrmSeedData.find((s: any) => s.object === 'crm_lead') as + | { records: AnyRec[] } + | undefined; + const records = leadSeed?.records ?? []; + const unqualified = records.filter((r) => r.status === 'unqualified'); + + it('the demo dataset actually contains unqualified leads', () => { + // Otherwise the assertion below is vacuously true. + expect(unqualified.length, 'no unqualified lead seeds').toBeGreaterThan(0); + }); + + it('every one carries a reason drawn from the field vocabulary', () => { + const legal = new Set( + ((lead?.fields?.disqualification_reason?.options ?? []) as AnyRec[]).map((o) => o.value), + ); + expect(legal.size, 'disqualification_reason has no options').toBeGreaterThan(0); + + const bad = unqualified + .filter((r) => !legal.has(r.disqualification_reason)) + .map((r) => `${r.email ?? r.company}: ${String(r.disqualification_reason)}`); + expect( + bad, + `unqualified lead seeds rejected by disqualification_reason_required:\n ${bad.join('\n ')}`, + ).toEqual([]); + }); +}); diff --git a/test/priority-rank-parity.test.ts b/test/priority-rank-parity.test.ts new file mode 100644 index 00000000..88e91d68 --- /dev/null +++ b/test/priority-rank-parity.test.ts @@ -0,0 +1,122 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import stack from '../objectstack.config'; +import caseHooks from '../src/objects/case.hook'; +import taskHooks from '../src/objects/task.hook'; + +/** + * `priority_rank` parity between crm_case and crm_task (#575 A4). + * + * Both objects materialise a sortable urgency ordinal because sorting on the + * `priority` select itself compares raw strings and inverts urgency. The two + * rank maps CANNOT be shared: L2 hook bodies run body-only in the QuickJS + * sandbox, so a module constant resolves at authoring time and arrives as + * `undefined` (see `_line-item-price-fill.ts`). They are therefore written out + * twice, and drifted — `rank[p] ?? 1` on the case, `?? 2` on the task, with + * field defaults to match. The same unknown priority sorted differently on the + * two objects, and on each it was indistinguishable from a real priority + * (`low` / `normal` respectively). + * + * The convention is now a shared UNRANKED sentinel of `0`, below every real + * rank on the `priority_rank desc` queues. Since the duplication is forced, + * this file is the thing keeping the two copies honest. + */ + +type AnyRec = Record; +type Rec = Record; + +const UNRANKED = 0; + +const objects: AnyRec[] = (stack as any).objects ?? []; +const fieldOf = (object: string) => + objects.find((o) => o.name === object)?.fields?.priority_rank as AnyRec | undefined; + +const hookNamed = (hooks: AnyRec[], name: string): AnyRec => { + const hook = hooks.find((h) => h.name === name); + if (!hook) throw new Error(`hook "${name}" not found`); + return hook; +}; + +/** Minimal beforeInsert context — these hooks only touch `input` for ranking. */ +const ctxFor = (input: Rec): AnyRec => ({ + event: 'beforeInsert', + input, + previous: undefined, + user: { id: 'user_1' }, + api: undefined, +}); + +/** Stamp `priority` through a hook and read back the rank it materialised. */ +const rankFor = async (hook: AnyRec, priority: string): Promise => { + const input: Rec = { priority }; + await hook.handler(ctxFor(input)); + return input.priority_rank; +}; + +const RANKED = [ + { + object: 'crm_case', + hook: hookNamed(caseHooks as AnyRec[], 'case_sla_defaults'), + // Ascending urgency — the ordinal must follow this order, not the strings. + ladder: ['low', 'medium', 'high', 'critical'], + }, + { + object: 'crm_task', + hook: hookNamed(taskHooks as AnyRec[], 'task_completion'), + ladder: ['low', 'normal', 'high', 'urgent'], + }, +]; + +describe('priority_rank uses one unranked sentinel across objects', () => { + it.each(RANKED.map((r) => r.object))('%s defaults priority_rank to the sentinel', (object) => { + const field = fieldOf(object); + expect(field, `${object}.priority_rank missing`).toBeTruthy(); + expect(field!.type).toBe('number'); + expect(field!.defaultValue, `${object}.priority_rank defaultValue`).toBe(UNRANKED); + }); + + it('both objects agree on the default, so an unstamped row sorts the same way', () => { + // The assertion the per-object check cannot make on its own: it is the + // AGREEMENT that matters, and it is what drifted. + expect(fieldOf('crm_case')?.defaultValue).toBe(fieldOf('crm_task')?.defaultValue); + }); +}); + +describe.each(RANKED)('$object priority ranking', ({ hook, ladder }) => { + it('ranks the known priorities in ascending urgency', async () => { + const ranks = []; + for (const priority of ladder) ranks.push(await rankFor(hook, priority)); + expect(ranks).toEqual([1, 2, 3, 4]); + }); + + it('falls back to the unranked sentinel for an unrecognised priority', async () => { + expect(await rankFor(hook, 'blocker')).toBe(UNRANKED); + }); + + it('the sentinel sorts below every real rank on a `priority_rank desc` queue', async () => { + // A fallback of 1 or 2 (the previous behaviour) buried an unknown priority + // in the middle of the queue, wearing a real priority's ordinal. + const known = []; + for (const priority of ladder) known.push(Number(await rankFor(hook, priority))); + expect(Math.min(...known)).toBeGreaterThan(UNRANKED); + }); +}); + +describe('the two hand-copied rank maps stay in agreement', () => { + it('an unknown priority ranks identically on both objects', async () => { + const [caseRank, taskRank] = await Promise.all([ + rankFor(RANKED[0].hook, 'blocker'), + rankFor(RANKED[1].hook, 'blocker'), + ]); + expect(caseRank).toBe(taskRank); + }); + + it('each hook stamps the same rank its object defaults to when unknown', async () => { + for (const { object, hook } of RANKED) { + expect(await rankFor(hook, 'blocker'), `${object} hook vs field default`).toBe( + fieldOf(object)?.defaultValue, + ); + } + }); +});