Skip to content

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

Merged
yinlianghui merged 1 commit into
mainfrom
claude/issue-514-line-item-fixes-fp0u7q
Jul 31, 2026
Merged

fix(line-items): F/P expression tags, null guard, shared price-fill (#514 items 8, 3, 15)#570
yinlianghui merged 1 commit into
mainfrom
claude/issue-514-line-item-fixes-fp0u7q

Conversation

@yinlianghui

Copy link
Copy Markdown
Collaborator

Description

Partial follow-up to #514, scoped deliberately to the two *_line_item objects (plus the one-line campaign_member tag fix that belongs to the same item). Each of the four defects was re-verified as still present on main at 5b88ad0 before being touched — the issue body predates several merged PRs.

This PR does not close #514; the remaining tier-3/tier-4 items are untouched.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring
  • Performance improvement
  • CI/CD update

Related Issues

Refs #514 — items 8, 3 (line-item half) and 15 (price-fill half) only. Not a fix-all; the issue stays open.

Changes Made

  • Item 8 — formula-on-formula. 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 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.
  • Item 3 (line-item half) — unguarded predicate. crm_opportunity_line_item's unit_price_positive compared record.unit_price < 0 with no null guard. Strict CEL aborts on null < 0 rather than 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 and no seed data changes; a blank price is still caught by the field's own required.
  • Item 8 — wrong expression tags. Formula fields on both line-item objects were authored with 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, and campaign_member.object.ts imports P.
  • Item 15 (price-fill half) — near-verbatim twins. opportunity_line_item.hook.ts and quote_line_item.hook.ts carried 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 from a new src/objects/_line-item-price-fill.ts. The sharing is authoring-time only: the handler body closes over nothing but its own ctx, so it still lowers to a body-only sandbox callable, verified against the build artifact (the two lowered body.source strings in dist/objectstack.json are byte-identical and mention neither factory parameter). The rollup hooks next door look alike but compute genuinely different totals, so they are left alone.

Testing

  • Unit tests pass (pnpm test — 22 files, 403 passed / 2 skipped)
  • Linting passes (pnpm lint)
  • Build succeeds (pnpm build)
  • pnpm verify end to end, exit code 0
  • New tests added

New guards live in test/line-item-conventions.test.ts — a new file on purpose, rather than more surface on test/metadata-references.test.ts, which is the repo's highest-churn test module and would conflict with parallel #514 work.

Written test-first: all six guards fail on main before the source changes, one per defect.

Guard Asserted against
Every Field.formula expression uses F source text of every *.object.ts
Every validation condition uses P (catches the raw {dialect:'cel'} literal too) source text of every *.object.ts
No line-item formula reads another formula field compiled metadata
Inlined quote total still yields 388.80 on the worked example compiled metadata
Both unit_price_positive rules are null-guarded compiled metadata
The two price-fill handlers are one implementation handler source + a shared behavioural scenario table run against each object

The tag guards have to read source text because F/P/cel produce byte-identical Expression envelopes — the distinction exists only in the authored source. The other guards assert against compiled metadata. Both tag guards run repo-wide over src/objects/*.object.ts and are clean after this change; the null-guard check is scoped to the two line-item objects because product.object.ts has the same defect and is tracked by the rest of #514 item 3.

The pre-existing behavioural price-fill tests in hooks-runtime.test.ts and hooks-runtime-sales.test.ts still pass unchanged, which is the independent evidence that the de-duplication preserved behaviour on both objects.

Screenshots

n/a — no UI surface changes.

Checklist

  • I have added a changeset.changeset/line-item-expressions-and-price-fill.md
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation — n/a, no documented behaviour changes
  • My changes generate no new warnings (the two campaign_enrollment expression warnings are pre-existing on main)
  • I have added tests that prove my fix is effective
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Additional Notes

The null guard on unit_price_positive is the one item here with a real behavioural footprint, and it is strictly narrowing: the rule fired on nothing before and now fires on a genuinely negative price. Worth a second look during review is the sandbox reasoning in _line-item-price-fill.ts — the factory is only safe as long as the handler body never reads a factory parameter, since that would resolve at authoring time and be undefined inside QuickJS. That constraint is stated in the module's doc comment.


Generated by Claude Code

Refs #514 (items 8, 3 and 15 — scoped to the two `*_line_item` objects).

- `quote_line_item.total_price` was formula-on-formula (`record.subtotal * …`),
  depending on the platform hydrating another computed field first. Recomposed
  from the same stored fields `subtotal` reads; arithmetic unchanged.
- `opportunity_line_item.unit_price_positive` compared `unit_price < 0` with no
  null guard. Strict CEL aborts on `null < 0`, so the rule was inert on a blank
  price — it now carries the `!= null &&` guard its quote-side twin has.
- Formula fields were authored with `P` and `campaign_member`'s validation with
  a raw `{ dialect: 'cel', source }` object (the file never imported `P`).
  `F`/`P`/`cel` are the same tagged template, so this only ever misled readers.
- The two price-fill hooks were near-verbatim copies differing in comments.
  Both now build from `_line-item-price-fill.ts`; the handler body closes over
  nothing but `ctx`, so it still lowers to a body-only sandbox callable and the
  two lowered bodies are byte-identical.

Guarded by a new `test/line-item-conventions.test.ts` (its own file, not more
surface on the high-churn `metadata-references.test.ts`). All six guards fail on
main before these changes. `pnpm verify` passes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RomdmGNvi3GfNfBW53HJQb
@vercel

vercel Bot commented Jul 31, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
hotcrm Ignored Ignored Jul 31, 2026 8:38am

Request Review

@yinlianghui
yinlianghui marked this pull request as ready for review July 31, 2026 08:51
@yinlianghui
yinlianghui merged commit 23facd7 into main Jul 31, 2026
10 checks passed
yinlianghui pushed a commit that referenced this pull request Jul 31, 2026
Resolves conflicts in the two `*_line_item.hook.ts` files. main's #570
extracted the two near-verbatim price-fill handlers into a shared
`_line-item-price-fill.ts`; that extraction wins, and the `where:` fix moves
into the shared module — which had carried the `filter:` along with it.

Widens the source scan in `test/hook-query-predicate.test.ts` from
`src/objects/*.hook.ts` to every `.ts` in that directory. The narrow glob
would have missed `_line-item-price-fill.ts` entirely: hook bodies get
factored into shared modules, and a dropped predicate riding along in one of
those is exactly as broken while matching no `*.hook.ts` pattern. The scan now
asserts it can see that file, so the coverage cannot silently narrow again.

The rollup fixes in both line-item hooks merged cleanly and are unchanged.

pnpm verify: 25 files, 480 passed | 2 skipped. Coverage thresholds hold.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R7h5XeGgNPBqNuV6LCRWpg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow-up to #496: behavior fixes and convention unification (tiers 3-4)

2 participants