fix(line-items): F/P expression tags, null guard, shared price-fill (#514 items 8, 3, 15)

.changeset/line-item-expressions-and-price-fill.md

@@ -0,0 +1,46 @@
+---
+'hotcrm': patch
+---
+
+Fix the line-item expression authoring and de-duplicate the price-fill hook
+(#514 items 8, 3 and 15 — the two `*_line_item` objects only).
+
+`crm_quote_line_item.total_price` was `record.subtotal * (1 + tax_rate/100)`,
+and `subtotal` is itself a FORMULA — so the total depended on the platform
+hydrating another computed field first, the hazard written up at
+`lead.object.ts:61-64`. It is now composed from the same stored fields
+`subtotal` reads, with the tax multiplier applied on top. The arithmetic is
+unchanged (4 × 100 at 10% line discount and 8% tax is still 388.80); what
+changes is that it no longer depends on evaluation order.
+
+`crm_opportunity_line_item`'s `unit_price_positive` compared `record.unit_price
+< 0` with no null guard. Strict CEL ABORTS on `null < 0` instead of evaluating
+it false, so the rule was inert on a blank price — it never fired at all. It now
+carries the same `!= null &&` guard its quote-side twin has always had. The
+guard only narrows the predicate, so no previously-accepted record starts
+failing; a blank price is still caught by the field's own `required`.
+
+Both line-item objects and `crm_campaign_member` also had their expressions
+authored with the wrong tag: formula fields used `P` (the predicate alias) and
+`campaign_member`'s validation used a raw `{ dialect: 'cel', source }` object
+because the file never imported `P`. `F`, `P` and `cel` are all aliases of the
+same tagged template, so this was invisible at runtime and only ever misled
+readers. Formulas now use `F`, conditions use `P`.
+
+Finally, `opportunity_line_item.hook.ts` and `quote_line_item.hook.ts` carried
+near-verbatim copies of the same price-fill handler, differing only in comments
+— the shape that lets a fix land on one object and silently skip the other. Both
+now build their hook from `_line-item-price-fill.ts`. The sharing happens at
+authoring time only: the handler body closes over nothing but its own `ctx`, so
+it still lowers to a body-only sandbox callable (the two lowered bodies in
+`dist/objectstack.json` are byte-identical). The rollup hooks next door look
+alike but compute genuinely different totals, so they stay separate.
+
+Guarded by a new `test/line-item-conventions.test.ts` — deliberately its own
+file rather than more surface on the high-churn `metadata-references.test.ts`.
+It pins all four: formula fields must use `F` and conditions `P` across every
+`*.object.ts` (a source-text check, because the tags are runtime-identical), no
+line-item formula may read another formula field, both `unit_price_positive`
+rules must be null-guarded, and the two price-fill handlers must remain one
+implementation — asserted both on handler source and on a shared behavioural
+scenario table run against each object.

src/objects/_line-item-price-fill.ts

@@ -0,0 +1,59 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+import type { Hook, HookContext } from '@objectstack/spec/data';
+import type { HookApi } from './_hook-api';
+
+/**
+ * Shared line-item price fill.
+ *
+ * `list_price` on both line-item objects is documented as "auto-populated from
+ * product.list_price", and nothing implemented it until this hook. On write,
+ * when a product is part of THIS change, it stamps the catalog `list_price`;
+ * on INSERT it also defaults the negotiated `unit_price` to the list price when
+ * the rep left it blank (they can still override). On update the negotiated
+ * `unit_price` is left alone, so a re-synced catalog price never clobbers a
+ * price someone actually negotiated.
+ *
+ * Why it lives here rather than twice: `crm_opportunity_line_item` and
+ * `crm_quote_line_item` carried near-verbatim copies of this handler that
+ * differed only in comments — the shape that lets a fix land on one object and
+ * silently skip the other. Only the price fill is shared; the two ROLLUP hooks
+ * next door look alike but compute genuinely different totals, so they stay
+ * separate.
+ *
+ * Sandbox note: L2 hook bodies run BODY-ONLY in the QuickJS sandbox, so a
+ * handler cannot reach module scope at runtime. This factory is still safe
+ * because the sharing happens at authoring time — `objectName`/`hookName` are
+ * plain metadata on the returned Hook, and the handler body below closes over
+ * NOTHING but its own `ctx`. Never move a value the body reads into this
+ * factory's parameters; it would resolve here and be undefined in the sandbox.
+ */
+export function createLineItemPriceFill(objectName: string, hookName: string): Hook {
+  return {
+    name: hookName,
+    object: objectName,
+    events: ['beforeInsert', 'beforeUpdate'],
+    priority: 100,
+    description: 'Default list_price / unit_price from the chosen product.',
+    handler: async (ctx: HookContext) => {
+      const { event, input } = ctx;
+      const api = ctx.api as HookApi | undefined;
+      if (!api) return;
+      // Only act when a product reference is part of THIS write.
+      const productId = typeof input.crm_product === 'string' ? input.crm_product : undefined;
+      if (!productId) return;
+      const product = await api.object('crm_product').findOne({
+        filter: { id: productId }, fields: ['id', 'list_price'],
+      });
+      const listPrice = product && typeof product.list_price === 'number' ? product.list_price : undefined;
+      if (listPrice === undefined) return;
+      // Catalog reference always tracks the product.
+      input.list_price = listPrice;
+      // Negotiated price defaults to list on create when left blank; never
+      // overwritten on update (respect a rep's negotiated figure).
+      if (event === 'beforeInsert' && input.unit_price == null) {
+        input.unit_price = listPrice;
+      }
+    },
+  };
+}

src/objects/campaign_member.object.ts

@@ -1,6 +1,7 @@
 // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
 
 import { ObjectSchema, Field } from '@objectstack/spec/data';
+import { P } from '@objectstack/spec';
 
 /**
  * Campaign Member Object
@@ -121,7 +122,7 @@ export const CampaignMember = ObjectSchema.create({
       type: 'script',
       severity: 'error',
       message: 'A campaign member must reference either a Lead or a Contact',
-      condition: { dialect: 'cel', source: 'isBlank(record.crm_lead) && isBlank(record.crm_contact)' },
+      condition: P`isBlank(record.crm_lead) && isBlank(record.crm_contact)`,
     },
   ],
 });

src/objects/opportunity_line_item.hook.ts

@@ -2,6 +2,7 @@
 
 import type { Hook, HookContext } from '@objectstack/spec/data';
 import type { HookApi } from './_hook-api';
+import { createLineItemPriceFill } from './_line-item-price-fill';
 
 /**
  * Opportunity amount rollup.
@@ -30,42 +31,13 @@ import type { HookApi } from './_hook-api';
  *   old and new parents are recomputed.
  */
 /**
- * Line-item price fill.
- *
- * `list_price` was documented as "Auto-populated from product.list_price" but
- * nothing implemented it. On write, when a product is chosen this stamps the
- * catalog `list_price`, and on INSERT defaults the negotiated `unit_price` to
- * the list price when the rep left it blank (they can still override). On update
- * the negotiated `unit_price` is left alone so a re-synced catalog price never
- * clobbers a negotiated one.
+ * Line-item price fill — one implementation shared with the quote line item;
+ * see `_line-item-price-fill.ts` for the behaviour and the sandbox caveat.
  */
-const opportunityLineItemPriceFill: Hook = {
-  name: 'opportunity_line_item_price_fill',
-  object: 'crm_opportunity_line_item',
-  events: ['beforeInsert', 'beforeUpdate'],
-  priority: 100,
-  description: 'Default list_price / unit_price from the chosen product.',
-  handler: async (ctx: HookContext) => {
-    const { event, input } = ctx;
-    const api = ctx.api as HookApi | undefined;
-    if (!api) return;
-    // Only act when a product reference is part of THIS write.
-    const productId = typeof input.crm_product === 'string' ? input.crm_product : undefined;
-    if (!productId) return;
-    const product = await api.object('crm_product').findOne({
-      filter: { id: productId }, fields: ['id', 'list_price'],
-    });
-    const listPrice = product && typeof product.list_price === 'number' ? product.list_price : undefined;
-    if (listPrice === undefined) return;
-    // Catalog reference always tracks the product.
-    input.list_price = listPrice;
-    // Negotiated price defaults to list on create when left blank; never
-    // overwritten on update (respect a rep's negotiated figure).
-    if (event === 'beforeInsert' && input.unit_price == null) {
-      input.unit_price = listPrice;
-    }
-  },
-};
+const opportunityLineItemPriceFill: Hook = createLineItemPriceFill(
+  'crm_opportunity_line_item',
+  'opportunity_line_item_price_fill',
+);
 
 const opportunityAmountRollup: Hook = {
   name: 'opportunity_amount_rollup',

src/objects/opportunity_line_item.object.ts

@@ -1,7 +1,7 @@
 // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
 
 import { ObjectSchema, Field } from '@objectstack/spec/data';
-import { P } from '@objectstack/spec';
+import { F, P } from '@objectstack/spec';
 
 /**
  * Opportunity Line Item
@@ -88,7 +88,7 @@ export const OpportunityLineItem = ObjectSchema.create({
 
     total_price: Field.formula({
       label: 'Total',
-      expression: P`record.quantity * record.unit_price * (1 - record.discount / 100)`,
+      expression: F`record.quantity * record.unit_price * (1 - record.discount / 100)`,
     }),
 
     line_number: Field.number({
@@ -104,7 +104,13 @@ export const OpportunityLineItem = ObjectSchema.create({
       type: 'script',
       severity: 'error',
       message: 'Sales price cannot be negative',
-      condition: P`record.unit_price < 0`,
+      // The `!= null` guard is load-bearing, not defensive noise: strict CEL
+      // ABORTS on `null < 0` instead of evaluating it false, so the unguarded
+      // form left this rule inert on a blank price — it never fired at all,
+      // which is the opposite of what a validation is for. The hazard is
+      // written up at `account.object.ts:244-245`; the same-named rule in
+      // `quote_line_item.object.ts` has carried the guard all along.
+      condition: P`record.unit_price != null && record.unit_price < 0`,
     },
   ],
 });

src/objects/quote_line_item.hook.ts

@@ -2,6 +2,7 @@
 
 import type { Hook, HookContext } from '@objectstack/spec/data';
 import type { HookApi } from './_hook-api';
+import { createLineItemPriceFill } from './_line-item-price-fill';
 
 /**
  * Quote total rollup.
@@ -33,33 +34,13 @@ import type { HookApi } from './_hook-api';
  * - handles re-parenting (old + new quote).
  */
 /**
- * Line-item price fill (see opportunity_line_item.hook for the rationale).
- * Stamps catalog `list_price` from the chosen product and defaults the
- * negotiated `unit_price` to it on create when blank.
+ * Line-item price fill — one implementation shared with the opportunity line
+ * item; see `_line-item-price-fill.ts` for the behaviour and the sandbox caveat.
  */
-const quoteLineItemPriceFill: Hook = {
-  name: 'quote_line_item_price_fill',
-  object: 'crm_quote_line_item',
-  events: ['beforeInsert', 'beforeUpdate'],
-  priority: 100,
-  description: 'Default list_price / unit_price from the chosen product.',
-  handler: async (ctx: HookContext) => {
-    const { event, input } = ctx;
-    const api = ctx.api as HookApi | undefined;
-    if (!api) return;
-    const productId = typeof input.crm_product === 'string' ? input.crm_product : undefined;
-    if (!productId) return;
-    const product = await api.object('crm_product').findOne({
-      filter: { id: productId }, fields: ['id', 'list_price'],
-    });
-    const listPrice = product && typeof product.list_price === 'number' ? product.list_price : undefined;
-    if (listPrice === undefined) return;
-    input.list_price = listPrice;
-    if (event === 'beforeInsert' && input.unit_price == null) {
-      input.unit_price = listPrice;
-    }
-  },
-};
+const quoteLineItemPriceFill: Hook = createLineItemPriceFill(
+  'crm_quote_line_item',
+  'quote_line_item_price_fill',
+);
 
 const quoteTotalRollup: Hook = {
   name: 'quote_total_rollup',

src/objects/quote_line_item.object.ts

@@ -1,7 +1,7 @@
 // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
 
 import { ObjectSchema, Field } from '@objectstack/spec/data';
-import { P } from '@objectstack/spec';
+import { F, P } from '@objectstack/spec';
 
 /**
  * Quote Line Item
@@ -83,7 +83,7 @@ export const QuoteLineItem = ObjectSchema.create({
 
     subtotal: Field.formula({
       label: 'Subtotal',
-      expression: P`record.quantity * record.unit_price * (1 - record.discount / 100)`,
+      expression: F`record.quantity * record.unit_price * (1 - record.discount / 100)`,
     }),
 
     tax_rate: Field.percent({
@@ -94,9 +94,14 @@ export const QuoteLineItem = ObjectSchema.create({
       defaultValue: 0,
     }),
 
+    // Composed from the same STORED fields `subtotal` reads, not from
+    // `subtotal` itself — a formula reading another formula depends on the
+    // platform hydrating that field first, which is the hazard warned against
+    // at `lead.object.ts:61-64`. The arithmetic is unchanged: this is
+    // `subtotal`'s expression with the tax multiplier applied on top.
     total_price: Field.formula({
       label: 'Total',
-      expression: P`record.subtotal * (1 + record.tax_rate / 100)`,
+      expression: F`record.quantity * record.unit_price * (1 - record.discount / 100) * (1 + record.tax_rate / 100)`,
     }),
 
     line_number: Field.number({