Skip to content

Retire dead lead/opportunity rules, stamp first response, guard CPQ lifecycles (#575 group B) - #578

Merged
yinlianghui merged 1 commit into
mainfrom
claude/issue-575-group-b-mmgbq6
Jul 31, 2026
Merged

Retire dead lead/opportunity rules, stamp first response, guard CPQ lifecycles (#575 group B)#578
yinlianghui merged 1 commit into
mainfrom
claude/issue-575-group-b-mmgbq6

Conversation

@yinlianghui

Copy link
Copy Markdown
Collaborator

Description

Group B of #575, executed to the plan agreed in the issue's two comments ("B 组方案已定" + "更正 B2 的实现方案"). B3 is out of scope — .changeset/ and the release flow are untouched apart from this PR's own changeset.

Two of these remove rules that could never fire, one gives a shipped SLA metric its missing writer, and one constrains the two lifecycles that had no constraint at all.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Code refactoring

Related Issues

Related to #575

Changes Made

B1 — delete crm_lead's cannot_edit_converted validation, keep the hook.
The code described the pair as a deliberate division of labour: the validation for a friendly, recoverable error on the four identity fields, the hook for a hard stop on everything else. That division never existed at runtime — a beforeUpdate throw aborts the write before validations are evaluated, so the validation could not produce its friendlier error on any field, ever. Same shape as the revenue_positive rule deleted in #571. The hook is now the single guard, and the comments on both sides say so.

B2a — delete crm_opportunity.created_date.
A duplicate of the platform's created_at with no writer anywhere — not the seed loader (which legitimately backdates readonly fields), not a hook, not a flow — so it was null on every row while deal_timeline used it as startDateField, giving the timeline no start dates. The view now reads created_at, the spelling the lead activity calendar already used; all four locale packs and the API reference drop the entry. crm_case.created_date is a different field with a real writer and is untouched.

B2b — give crm_case.first_response_date a writer.
It was the only member of the case SLA family without one (sla_due_date and resolution_time_hours come from case.hook, is_sla_violated from case_sla_monitor), so the most standard service-desk metric was permanently null while the field, its four translations and its place on the case detail page all advertised otherwise. It is now stamped in the shared logActivityAction body on the first sys_activity a case receives — the industry definition (Salesforce FirstResponseDateTime, Zendesk first reply time) is when the customer first heard back, so a logged call or meeting is the event and a status change deliberately is not. Two details are load-bearing:

  • the field drops readonly, which would otherwise silently discard the write on a user-context write (#2948) — the same reason is_sla_violated and escalated_date are not readonly;
  • the body reads the stored row instead of trusting ctx.record, because list_item / record_related dispatch can hand it a projected record, and a missing key there would re-stamp on every log and turn "first response" into "last response".

A convention note in the body records that any future customer-facing path on a case (a reply-email action, an inbound portal reply) must stamp it too.

B4 — state_machine validations for crm_quote and crm_contract only.
Neither had a transition table or a status guard in its hook, so draft → accepted on a quote (binding numbers nobody reviewed or sent) and draft → activated on a contract (which stamps signed_date, promotes the account to customer, and starts the renewal clock) were both legal. Warning severity, matching the lead / opportunity / case machines. The tables agree with the automation that drives them: every unsettled quote status can still expire (the sweep filters on expiration_date alone), accepted / expired are terminal because quote_pricing_guard freezes them, and only an activated contract expires. crm_campaign and crm_task deliberately get nothing — their status is descriptive, not a controlled lifecycle — and a test pins that absence so it stays a decision rather than a gap.

Testing

  • Unit tests pass (pnpm test) — 33 files, 630 passed / 2 skipped
  • Linting passes (pnpm lint)
  • Build succeeds (pnpm build)
  • New tests added
  • Full pnpm verify green locally (validate → typecheck → lint → hygiene → build → test)

Four new test files, per the issue's convention of not appending to test/metadata-references.test.ts:

File Pins
test/converted-lead-guard.test.ts No second converted-lead rule exists (by name or by shape); the hook rejects all four identity fields, names the offending field, and still lets narrative fields and system writes through
test/case-first-response.test.ts Both activity twins stamp once and never move an existing stamp; the (data, options) update shape; the stored-value read; the field is not readonly
test/opportunity-creation-date.test.ts Field gone, crm_case.created_date intact, no locale still labels it, no opportunity view anchors on a date field that does not exist
test/status-state-machines.test.ts All five machines cover their field's full vocabulary on both sides of the arrow; the new tables agree with the flows and hooks; campaign/task stay ungoverned

The first-response tests run the shipped action body through the real QuickJS sandbox added in #575 A1, because the two things that can break this stamp only exist there: the api.read capability the new lookup needs, and the engine facade's update(data, options) signature — the one mass_update_stage gets wrong and silently never writes.

Checklist

  • I have added a changeset.changeset/lifecycle-guards-and-dead-fields.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 (docs/developers/api_reference.md)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective
  • New and existing unit tests pass locally with my changes

Additional Notes

Per the issue's conventions: the problems were re-verified against main at 28e87c9 (after #576 and #577 landed) before starting, and the commit message uses Refs #575 rather than Closes, since group B is only part of the issue.

Worth knowing for review: test/hooks-runtime-service.test.ts exercises contract_on_activation with a draft → activated fixture, which the new contract table now flags as a skipped-approval jump. The fixture is untouched — hooks bypass validations, so nothing breaks, and the hook's behaviour genuinely does not depend on the origin state — but it is the one place in the repo where the new rule and an existing test scenario disagree on intent.


Generated by Claude Code

…ifecycles

Group B of #575, following the plan agreed in the issue comments.

B1 — delete `crm_lead`'s `cannot_edit_converted` validation. It was described
as the friendly, recoverable half of a two-layer converted-lead lock, but the
`beforeUpdate` throw in lead.hook.ts aborts the write first, so it never
produced that error on any field. The hook is now the single guard.

B2 — delete `crm_opportunity.created_date`, a duplicate of the platform's
`created_at` with no writer: `deal_timeline` used it as `startDateField` and so
had no start dates. The view reads `created_at`; the four locale packs no
longer label a field that does not exist. `crm_case.created_date` is a
different field with a real writer and is untouched.

B2 — give `crm_case.first_response_date` the writer it never had, in the shared
`logActivityAction` body: the first `sys_activity` on a case is when the
customer first heard back (Salesforce `FirstResponseDateTime` / Zendesk first
reply time), so a logged call or meeting is the event, not a status change. The
field drops `readonly`, which would discard the write, and the body reads the
stored value so a projected record cannot turn "first response" into "last".

B4 — add `state_machine` validations to `crm_quote` and `crm_contract`, neither
of which had a transition table or a hook-level status guard: `draft →
accepted` and `draft → activated` were both legal. Warning severity, matching
the three existing machines. `crm_campaign` and `crm_task` deliberately get
none — their status is descriptive — and a test pins that absence.

Guards land in four new test files rather than metadata-references.test.ts;
the first-response tests run the shipped action body through the real QuickJS
sandbox from #575 A1.

Refs #575

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011FvY1DtcReVbyfSh5PvzCE
@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 3:17pm

Request Review

@github-actions github-actions Bot added documentation Improvements or additions to documentation ci/cd metadata backend labels Jul 31, 2026
@yinlianghui
yinlianghui marked this pull request as ready for review July 31, 2026 15:32
@yinlianghui
yinlianghui merged commit 26d7c8d into main Jul 31, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend ci/cd documentation Improvements or additions to documentation metadata

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants