diff --git a/.changeset/2077-closed-deal-lines.md b/.changeset/2077-closed-deal-lines.md new file mode 100644 index 00000000..def5a380 --- /dev/null +++ b/.changeset/2077-closed-deal-lines.md @@ -0,0 +1,19 @@ +--- +"hotcrm": patch +--- + +**A closed opportunity no longer takes new line items.** A closed-won or closed-lost deal was +already locked, but its line items were not: a rep could add a product line to it from the +Products panel, change the quantity or price of an existing line, or move a line onto it from an +open deal, and the save went through. The deal kept its closed Amount, so its lines no longer +added up to it. + +Adding a line to a closed deal, changing one of its lines, or moving a line onto or off a closed +deal is now refused with *"Opportunity Acme Renewal is closed (closed_won), so its line items can +no longer be added or changed."* (409, `RECORD_LOCKED`), the same lock the deal's own fields +have. An open deal takes line changes as before. Seed data and other system writes that carry no +signed-in user are not affected, and the one-time line-number back-fill +(`scripts/backfill-line-number.ts`) still numbers the lines of closed deals. + +Not changed: removing a line from a closed deal is still allowed, and deleting a closed deal still +deletes its lines. Whether a single line may be removed is an open decision on #2078. diff --git a/src/revenue/objects/opportunity_line_item.hook.ts b/src/revenue/objects/opportunity_line_item.hook.ts index d7863fb9..192c970e 100644 --- a/src/revenue/objects/opportunity_line_item.hook.ts +++ b/src/revenue/objects/opportunity_line_item.hook.ts @@ -45,6 +45,73 @@ const opportunityLineItemNumbering: Hook = createLineItemNumbering( 'opportunity_line_item_line_number', ); +/** + * A closed deal's LINES are frozen with it (#2077, the deal-side twin of the + * settled-quote line freeze in `quote_workflow`, #2060 item 1). + * + * Measured on 18.0.0-next.1 before this hook: a rep added a 3 × 500 line to + * their own closed-won deal (admitted), `opportunity_amount_rollup` rightly + * left the closed amount at 1,000, and the lines summed to 1,500. Changing a + * closed-lost deal's line and moving an open deal's line onto a closed one were + * admitted too. + * + * ⚠️ Not in `opportunity_lifecycle`, though that is the deal's own freeze: a + * hook may not attach to another package's object (ADR-0130 R4), and this + * object is `src/revenue`'s. The freeze is the same one, drawn the same way: + * "closed" is the lifecycle's own test (`closed_won` / `closed_lost`), the + * envelope is its `RECORD_LOCKED` / 409, and only USER writes are judged (a + * write with no `ctx.user` — the seed, a migration — passes). An update is + * judged by what it CHANGES, as the quote line freeze judges it, so the 4.0.0 + * line-number back-fill's `{ line_number: null }` touch still lands. Both + * parents are judged, so a line moves neither onto a closed deal nor off one. + * Priority 90 runs it ahead of the price fill and the numbering, so the + * caller's own payload is what is judged, not the keys those two inject. + * + * ⚠️ DELETE is not guarded: deleting a deal cascades to its lines as the + * deleting user, the same open question as quote lines (decision card #2078). + */ +const opportunityLineItemFreeze: Hook = { + name: 'opportunity_line_item_freeze', + object: 'crm_opportunity_line_item', + events: ['beforeInsert', 'beforeUpdate'], + priority: 90, + description: 'Refuse a user adding, changing or moving a line item of a closed opportunity.', + handler: async (ctx: HookContext) => { + // The refusal envelope (#1075). Mirrored from `src/sales/objects/_refusal.ts` + // because a lowered body has no module scope and `extractHookBody` THROWS on + // an import; `test/refusal-envelope.test.ts` pins every copy against it. + function refuse( + message: string, + code: string, + status: number, + userMessage: string = message, + ): Error { + const err = new Error(message) as Error & { + code: string; + status: number; + userMessage: string; + }; + err.code = code; + err.status = status; + err.userMessage = userMessage; + return err; + } + const { input, previous } = ctx; + const changed = Object.keys(input).some((k) => !['id', 'updated_at', 'updated_by', 'line_number'].includes(k) && input[k] !== previous?.[k]); + if (!changed || !ctx.user?.id) return; + for (const id of new Set([input.crm_opportunity, previous?.crm_opportunity])) { + const deal = typeof id === 'string' && id + ? await (ctx.api as HookApi | undefined)?.object('crm_opportunity').findOne({ where: { id }, fields: ['name', 'stage'] }) + : null; + if (deal?.stage === 'closed_won' || deal?.stage === 'closed_lost') { + // `crm_opportunity.nameField` is `name` — never the record id (#1243). + const name = typeof deal.name === 'string' ? deal.name.trim() : ''; + throw refuse(`${name ? `Opportunity ${name}` : 'Opportunity'} is closed (${deal.stage}), so its line items can no longer be added or changed.`, 'RECORD_LOCKED', 409); + } + } + }, +}; + const opportunityAmountRollup: Hook = { name: 'opportunity_amount_rollup', object: 'crm_opportunity_line_item', @@ -99,4 +166,4 @@ const opportunityAmountRollup: Hook = { }, }; -export default [opportunityLineItemPriceFill, opportunityLineItemNumbering, opportunityAmountRollup]; +export default [opportunityLineItemPriceFill, opportunityLineItemNumbering, opportunityAmountRollup, opportunityLineItemFreeze]; diff --git a/test/opportunity-line-freeze.test.ts b/test/opportunity-line-freeze.test.ts new file mode 100644 index 00000000..e7926b91 --- /dev/null +++ b/test/opportunity-line-freeze.test.ts @@ -0,0 +1,129 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, beforeAll, vi } from 'vitest'; +import type { VerifyStack } from '@objectstack/verify'; +import { REFUSAL_CODES } from '../src/sales/objects/_refusal'; +import { hookNamed } from './helpers/composed-stack'; +import { hotcrmStack, signUpPerson, systemUpdate, runShippedHook, type Person } from './helpers/verify-stack'; + +/** + * A closed deal's line items are frozen with it (#2077) — the deal-side twin of + * `test/quote-line-freeze.test.ts` (#2060 item 1). + * + * The defect, measured on 18.0.0-next.1 before the fix: a rep's line insert + * onto their own closed-won deal, a change to a closed-lost deal's line, and a + * move of an open deal's line onto a closed one were all admitted, while + * `opportunity_amount_rollup` rightly kept the closed amount — so the lines + * stopped summing to it (1,000 against 1,500). The opportunity doc says + * "Closed deals are locked" (`content/docs/sales/opportunities.mdx`). + * + * Pinned both ways: a user's insert, change or move is refused with the deal + * freeze's own envelope (`RECORD_LOCKED` / 409); an open deal still takes lines + * (the control), a system write still lands, and the line-number back-fill's + * touch still numbers a closed deal's line. Delete is decision card #2078. + */ + +type Rec = Record; + +let verify: VerifyStack; +let rep: Person; +let admin: string; +let k = 0; +beforeAll(async () => { + verify = await hotcrmStack(); + admin = await verify.signIn(); + rep = await signUpPerson(verify, 'rep@opportunity-line-freeze.test', { + name: 'Line Freeze Rep', positions: ['sales_rep'], permissionSets: ['sales_rep'], + }); +}, 180_000); + +const as = (p: Person | string) => ({ as: typeof p === 'string' ? p : p.token }); +const stored = async (object: string, id: string): Promise => (await verify.rows(object, { id }))[0]; +const CLOSE: Record = { closed_won: { win_reason: 'better_price' }, closed_lost: { loss_reason: 'price' } }; + +/** A rep's deal at `stage`, with one line written while it was open; the system closes it. */ +const dealWithLine = async (stage: string): Promise<{ deal: Rec; line: Rec; product: Rec }> => { + const [account] = await verify.seed('crm_account', [{ name: `Line Freeze Co ${++k}`, owner_id: rep.id }]); + const [product] = await verify.seed('crm_product', [{ name: `Line Freeze Product ${k}`, product_code: `LF-${k}`, is_active: true, list_price: 500 }]); + const [deal] = await verify.seed('crm_opportunity', [{ + name: `Line Freeze Deal ${k}`, amount: 1000, stage: 'negotiation', close_date: '2030-06-30', crm_account: account!.id, owner_id: rep.id, + }]); + const line = await verify.hooks.run('crm_opportunity_line_item', 'insert', { + crm_opportunity: deal!.id, crm_product: product!.id, quantity: 2, unit_price: 500, + }, as(rep)); + if (CLOSE[stage]) await systemUpdate(verify, 'crm_opportunity', { id: deal!.id, stage, ...CLOSE[stage] }); + return { deal: (await stored('crm_opportunity', deal!.id))!, line, product: product! }; +}; + +/** What the engine refused a write with, or null when it went through. */ +const refusalOf = (p: Promise): Promise => p.then(() => null, (e: Rec) => e); + +const sentence = (deal: Rec) => `Opportunity ${deal.name} is closed (${deal.stage}), so its line items can no longer be added or changed.`; + +/** The deal freeze's envelope, its wording, and the deal named by its name, never its id. */ +const expectFrozen = (err: Rec | null, deal: Rec): void => { + expect(err, 'the write went through').toBeTruthy(); + expect([err!.code, err!.status]).toEqual([REFUSAL_CODES.locked.code, REFUSAL_CODES.locked.status]); + expect(String(err!.message)).toContain(sentence(deal)); + expect(String(err!.message)).not.toContain(deal.id); +}; + +describe('a user cannot change the lines of a closed deal', () => { + it.each(['closed_won', 'closed_lost'])('refuses a rep adding a line to a %s deal, and stores nothing', async (stage) => { + const { deal, product } = await dealWithLine(stage); + expectFrozen(await refusalOf(verify.hooks.run('crm_opportunity_line_item', 'insert', { + crm_opportunity: deal.id, crm_product: product.id, quantity: 3, unit_price: 500, + }, as(rep))), deal); + expect(await verify.rows('crm_opportunity_line_item', { crm_opportunity: deal.id })).toHaveLength(1); + }); + + it('refuses a rep changing a line of a closed deal, and keeps the stored line', async () => { + const { deal, line } = await dealWithLine('closed_lost'); + expectFrozen(await refusalOf(verify.hooks.run('crm_opportunity_line_item', 'update', { id: line.id, quantity: 9 }, as(rep))), deal); + expect((await stored('crm_opportunity_line_item', line.id))!.quantity).toBe(2); + }); + + it('refuses moving a line off a closed deal, and onto one', async () => { + const closed = await dealWithLine('closed_won'); + const open = await dealWithLine('negotiation'); + expectFrozen(await refusalOf(verify.hooks.run('crm_opportunity_line_item', 'update', { id: closed.line.id, crm_opportunity: open.deal.id }, as(rep))), closed.deal); + expectFrozen(await refusalOf(verify.hooks.run('crm_opportunity_line_item', 'update', { id: open.line.id, crm_opportunity: closed.deal.id }, as(rep))), closed.deal); + expect((await stored('crm_opportunity_line_item', closed.line.id))!.crm_opportunity).toBe(closed.deal.id); + expect((await stored('crm_opportunity_line_item', open.line.id))!.crm_opportunity).toBe(open.deal.id); + }); + + it('refuses on the SHIPPED path too — the lowered body, in QuickJS', async () => { + const { deal, product } = await dealWithLine('closed_won'); + const err = await refusalOf(runShippedHook(verify, hookNamed('opportunity_line_item_freeze'), { + event: 'beforeInsert', input: { crm_opportunity: deal.id, crm_product: product.id, quantity: 1, unit_price: 500 }, as: rep.token, + })); + expect(err?.name).toBe('SandboxError'); + expect([err!.code, err!.status]).toEqual([REFUSAL_CODES.locked.code, REFUSAL_CODES.locked.status]); + expect(String(err!.userMessage)).toBe(sentence(deal)); + }); +}); + +describe('what the freeze lets through', () => { + it('CONTROL — an open deal still takes and changes lines, and its amount follows them', async () => { + const { deal, line, product } = await dealWithLine('negotiation'); + await verify.hooks.run('crm_opportunity_line_item', 'insert', { crm_opportunity: deal.id, crm_product: product.id, quantity: 1, unit_price: 50 }, as(rep)); + await verify.hooks.run('crm_opportunity_line_item', 'update', { id: line.id, quantity: 3 }, as(rep)); + // `opportunity_amount_rollup` is `async: true`, so its result is waited for. + await vi.waitFor(async () => expect((await stored('crm_opportunity', deal.id))!.amount).toBe(1550), { timeout: 10_000, interval: 50 }); // 3×500 + 1×50 + }); + + it('a system write onto a closed deal still lands (the freeze judges users only)', async () => { + const { deal, product } = await dealWithLine('closed_won'); + await verify.seed('crm_opportunity_line_item', [{ crm_opportunity: deal.id, crm_product: product.id, quantity: 1, unit_price: 10 }]); + expect(await verify.rows('crm_opportunity_line_item', { crm_opportunity: deal.id })).toHaveLength(2); + }); + + it('the line-number back-fill still numbers an unnumbered line of a closed deal', async () => { + const { line } = await dealWithLine('closed_lost'); + await systemUpdate(verify, 'crm_opportunity_line_item', { id: line.id, line_number: null }); + expect((await stored('crm_opportunity_line_item', line.id))!.line_number ?? null).toBeNull(); + // `scripts/backfill-line-number.ts`'s own write: an admin's REST touch. + await verify.hooks.run('crm_opportunity_line_item', 'update', { id: line.id, line_number: null }, as(admin)); + expect(typeof (await stored('crm_opportunity_line_item', line.id))!.line_number).toBe('number'); + }); +}); diff --git a/test/refusal-envelope.test.ts b/test/refusal-envelope.test.ts index d506e6b1..88530ad0 100644 --- a/test/refusal-envelope.test.ts +++ b/test/refusal-envelope.test.ts @@ -137,11 +137,12 @@ describe('every refusal names a code the platform will echo (#1075)', () => { // status-change gate to `opportunity_lifecycle`; 21 until REQ-0006 step 11 // added the 立项 (qualification) gate beside it; 22 until #2014 added the // refusal of an acceptance the deal's approval holds to `quote_workflow`; - // 23 until #2060 added the settled-quote line freeze to `quote_workflow`. + // 23 until #2060 added the settled-quote line freeze to `quote_workflow`; + // 24 until #2077 added the closed-deal line freeze. // The number is hand-maintained on purpose: a new refusal has to be a // deliberate edit here, so a guard that quietly stopped being swept cannot // hide behind a count that follows it. - expect(sites).toHaveLength(24); + expect(sites).toHaveLength(25); }); it('uses only members of the platform ErrorCode enum', () => { diff --git a/test/runtime-coverage.test.ts b/test/runtime-coverage.test.ts index 9f2d79d4..407ac8c2 100644 --- a/test/runtime-coverage.test.ts +++ b/test/runtime-coverage.test.ts @@ -105,6 +105,9 @@ const RUNTIME_TEST_FILES = [ // non-system strip, a caller-supplied one does not) measured on a real // ObjectQL with the strip's own control beside it, plus the lowered body. 'line-item-line-number.test.ts', + // #2077 — the closed-deal line freeze: the refusals, the open-deal and + // system-write controls, and the lowered body. + 'opportunity-line-freeze.test.ts', ]; /**