Repository navigation
crm_account.renewal_owner / next_renewal_date are a second, dead renewal model — the live one reads crm_contract.end_date and notifies the contract owner #1181
Description
Activity
Claim: PM loop round 1
Session:session_01XeAz8tSjwAibjdSDn9niiZ
Branch:claude/issue-1181-account-renewal-model
Worktree:hotcrm-issue-1181
Domain:metadata
File surface:src/objects/account.object.ts,src/objects/account.hook.ts,src/views/account.view.ts,src/data/sales.seed.ts,src/flows/contract-renewal.flow.ts,src/translations/*(only therenewal_owner/next_renewal_datekeys),test/(stop on breach; explain in the report)
Container & model:M,mode:subagent,model: opus
Serial constraints cleared: no in-flight PR or claim touches these paths. #1182 (decorative-field sweep) also editssrc/objects/account.object.tsandsrc/views/account.view.ts— it is deliberately held to the next batch and will merge main before starting. Sibling #1180 (src/actions/) and #1183 (scripts/) are disjoint; neither pins behavior this card asserts.
Generated by Claude Code
- added a commit that references this issue
on Aug 17, 2026 { "issue": 1181, "status": "done", "branch": "claude/issue-1181-account-renewal-model", "pr": "https://github.com/objectstack-ai/hotcrm/pull/1189", "premise_still_valid": true, "summary": "Took the REMOVE branch: crm_account.renewal_owner and next_renewal_date are gone, leaving crm_contract.end_date + contract_renewal.flow.ts as the single renewal model. Enforcement was rejected on measurement, not preference — maintaining next_renewal_date honestly requires a min(end_date) rollup recomputed on contract insert/update/delete/status-change (feature design, the card's own stated signal for remove), and preferring account.renewal_owner in the flow needs a per-contract get_record plus a fallback that flow metadata has no coalesce operator for, i.e. two decision branches with duplicated create_record + notify pairs in the repo's most safety-critical loop. The fact that settled it: crm_contract ALREADY ships the renewals queue (renewal_calendar, a Renewals tab over end_date), so the account-level '🔄 Upcoming Renewals' view was a second queue over a field nothing maintained — removing it loses no capability. Scope reached further than the card listed: the whole renewals_due view and its switcher tab had to go (both its filter and its sort key were next_renewal_date), plus the at_risk_accounts renewal-owner column, the form section entries, 5 seeds, 2 field labels AND the _views.renewals_due label in each of 4 locale bundles, docs/STATUS.md's field count, the developer API field list, and the user docs in ALL THREE locales (the zh-Hans/zh-Hant account pages mirror the English row-for-row and an English-only grep would have missed them). Kept scoped to the two renewal fields so #1182's sweep merges on top.", "tests": "All green at final commit 5f57c342 (union run AFTER the last commit, at that sha). `pnpm verify` (= validate && typecheck && lint && lint:i18n-gate && hygiene && build && test) exit 0 — 'Test Files 107 passed (107) / Tests 2612 passed | 1 skipped (2613)', '✓ i18n lint gate: 0 i18n/missing-* issues', '✓ source hygiene clean', '✓ Build complete', 'Data: 18 Objects 345 Fields' (was 347). Plus the CI-only gate pnpm verify omits, per #1149: `node scripts/check-stackblitz-lock.mjs` → 'package-lock.json is in sync with package.json (v3)'. One gate went genuinely RED mid-work and was fixed, not worked around: test/docs-drift.test.ts caught docs/STATUS.md still claiming 347 fields ('page says 347, the stack registers 345'). NEW TEST test/account-renewal-model.test.ts (7 tests) pins both halves — fields absent everywhere they were read, AND renewal stays contract-level with {currentContract.owner_id} as recipient. REVERSE-VERIFIED with the fix committed first, then ablated, then restored via git checkout: re-adding renewal_owner to account.object.ts produced exactly the predicted direction — 2 failed / 5 passed, failing on \"expected [...] to not include 'renewal_owner'\" and \"renewal_owner still read by: object\", with the flow-side pins staying green. Not run locally, left to CI: codeql, docs-app, e2e (no e2e spec touches renewals or account views), link-check (.md only), changeset-check (this PR adds .changeset/account-renewal-model-removed.md, minor, carrying the FROM → TO migration).", "open_questions": [], "out_of_scope_findings": [ "filed as #1188: the hand-written '15 lookups on sys_user' figure in src/objects/event_attendee.object.ts and test/event-attendee-cascade.test.ts has no guard and had ALREADY drifted — main @ 2342811 actually held 16. This PR removes one of them, making the number accurate again by coincidence; the missing guard is the finding. Labelled `finding`, unassigned." ] }Notes for the PM beyond the JSON
- The maintainer ruling was quoted verbatim in the PR body as the authorization for removing published fields, as instructed.
- The PR body's signature footer was silently duplicated by the platform on creation (two copies of the session-URL footer). That is platform behaviour on the create path, not another actor editing the PR — recorded rather than "corrected".
- CI was still converging (
mergeable_state: unstable) when this report was filed, per the report-at-draft-PR-time contract. Gate results after that point are yours to read.
Generated by Claude Code
ACCEPT — PM review of #1189 (round 1). Verified against GitHub and
origin/main, not against the report's self-description.Branch taken: remove. Authorized by the maintainer ruling of 2026-08-17, quoted verbatim in both the PR body and the changeset.
What I re-verified independently:
- The load-bearing argument holds. The removal rests on "the capability already exists on the right object". Checked on
origin/main:src/views/contract.view.ts:39,47does ship arenewalstab overrenewal_calendar, andall_contractssorts onend_date. So the account-level queue was a second, less accurate copy — removing it loses no user capability. This was the claim most worth falsifying and it survives. - Scope discipline against Decorative-field sweep: enforce-or-remove every declared field with zero business consumers #1182.
account.object.tsandaccount.view.tsare touched only where the two renewal fields and their view reach; other inert fields on the same object (parent_account,health_score) are untouched, so the decorative-field sweep merges on top as planned. - The arithmetic in the
event_attendeecomment edit. Counted directly:mainholds 16sys_userlookups while the comment claimed 15 (already drifted), and this branch brings the real count to 15. The comment is accurate again, and the report says so plainly rather than presenting the coincidence as a fix — the missing guard is filed separately as The hand-written "15 lookups onsys_user" figure in twoevent_attendeecomments has no guard, and had already drifted to 16 #1188 (finding, unassigned, confirmed present). - Removal completeness. Field declarations, view + switcher tab,
at_risk_accountscolumn, form section entries, 5 seeds, all four locale bundles (both the field labels and the_views.renewals_dueentries),docs/STATUS.mdfield count, the developer API field list, and the user docs in all three locales. The two localized account pages mirror the English one row-for-row and would have been missed by an English-only grep — catching them was the right instinct. - Test quality.
test/account-renewal-model.test.tspins both halves — the fields being gone, and the surviving contract-level model still notifying{currentContract.owner_id}. The second half is what stops the same two-truths defect being rebuilt from the flow side, where no absence assertion would be watching. Reverse-verified by ablation with the predicted failure direction stated and matched. - Honest red.
test/docs-drift.test.tswent genuinely red mid-work on the stale 347-field count inSTATUS.mdand was fixed rather than worked around. Reported rather than buried.
Landing status: 8 of 9 checks are complete and green (CodeQL, Analyze Code, Quality Checks, Check Changeset, Playwright, link-check, Label PR, Vercel). The gate-bearing job
Build and Test (22.x)is stillin_progress— an honest reading, not a pass. The PR stays draft and unqueued until that job concludessuccess; I am holding a timed re-check rather than polling.
Generated by Claude Code
- The load-bearing argument holds. The removal rests on "the capability already exists on the right object". Checked on
Problem
Two renewal models coexist, and only one runs:
src/flows/contract-renewal.flow.ts— daily sweep overcrm_contract.end_date, opens renewal tasks/opportunities for the contract owner.crm_account.renewal_owner(lookup to sys_user) +crm_account.next_renewal_date(date). Measured on main @ 2342811: referenced only byaccount.object.ts(declaration),account.view.ts(render),sales.seed.ts(seed value), and one incidental comment inevent_attendee.object.ts. No hook writes them, no flow reads them, no dataset exposes them.So the account-level fields claim a renewal ownership the automation never consults: an admin can set
renewal_ownerand that person will never be notified of anything. Two truths for one process — the declared-but-unenforced half is worse than absent, because it looks wired.Ruled
Maintainer ruling, 2026-08-17, PM session chat (verbatim): 「逐个 enforce-or-remove（推荐）」. For this pair, decide one of:
contract-renewal.flow.tsprefersaccount.renewal_owner(fallback: contract owner) when creating the renewal task/opportunity, and something real maintainsnext_renewal_date(e.g. contract activation stamps min(end_date) onto the account) — do this only if the renewal-owner concept earns its keep;The implementing dev should state which branch was taken and why in the PR body. Premise re-check:
grep -rn "renewal_owner\|next_renewal_date" src/ --include=*.ts | grep -v views/ | grep -v translations/.