From fa4318156241369c1a2bb376acd2081c2975e761 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 14:50:57 +0000 Subject: [PATCH] Enforce lead disqualification reason, group campaign/task fields, unify priority_rank MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refs #575 (A2, A3, A4). A2 — crm_lead.disqualification_reason promised "Required when status is Unqualified" in its own description and nothing enforced it: no validation, no hook, and no form that rendered the field. Adds the disqualification_reason_required script validation (same shape as crm_case.escalation_reason_required), puts the field on every crm_lead form that exposes an editable status (shown only when the status is unqualified) and on the lead detail page, and gives the generated unqualified seed leads a reason with a matching note. A3 — crm_campaign and crm_task were the last two detail-page business objects with zero fieldGroups, so both rendered as one flat grid. Both now declare groups mirroring the sections their forms already use, with every field assigned. Line items stay ungrouped: they are edited inline in the parent's grid and have no detail page to section. A4 — priority_rank's 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 was indistinguishable from a real priority. Both now use 0, an unranked sentinel below every real rank on the priority_rank desc queues; the known ranks 1-4 are unchanged. The two rank maps stay hand-copied because L2 hook bodies run body-only in the QuickJS sandbox, so the new parity test is what keeps the copies in agreement. New guards live in three new test files rather than being appended to test/metadata-references.test.ts. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KSmk2xtzFcv9TbW9PuYBUe --- ...ualification-field-groups-priority-rank.md | 50 +++++++ src/data/index.ts | 22 ++- src/objects/campaign.object.ts | 43 +++++- src/objects/case.hook.ts | 10 +- src/objects/case.object.ts | 8 +- src/objects/lead.object.ts | 13 ++ src/objects/task.hook.ts | 9 +- src/objects/task.object.ts | 48 ++++++- src/pages/lead_detail.page.ts | 6 +- src/views/lead.view.ts | 58 +++++++- test/field-groups-coverage.test.ts | 116 +++++++++++++++ test/lead-disqualification.test.ts | 136 ++++++++++++++++++ test/priority-rank-parity.test.ts | 122 ++++++++++++++++ 13 files changed, 632 insertions(+), 9 deletions(-) create mode 100644 .changeset/lead-disqualification-field-groups-priority-rank.md create mode 100644 test/field-groups-coverage.test.ts create mode 100644 test/lead-disqualification.test.ts create mode 100644 test/priority-rank-parity.test.ts 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, + ); + } + }); +});