Repository navigation
refactor(crm): enforce-or-remove every decorative field, one verdict each - #1195
Merged
Merged
Conversation
…each (#1182) Ten declared fields had no business consumer. Nine are removed; one is kept and given the roll-up it always claimed to have. Removed, with every reader that went with them — view columns, filters and sorts, form sections, seed values, all four locale bundles, the contact import mapping AND its shipped CSV template, the developer field lists and the user docs in all three locales: crm_product.quantity_on_hand / reorder_point (+ the whole low_stock view, its switcher tab, and a "Low Stock" filter that compared against a hardcoded 10 rather than the reorder point beside it) crm_product.is_taxable, billing_type, unit_of_measure (+ 26 seeded values) crm_case.customer_signature, parent_case crm_task.estimated_hours, actual_hours crm_contact.reports_to, birthdate crm_campaign.parent_campaign Kept and ENFORCED: crm_account.parent_account. The accounts docs promised "roll-up reports — annual revenue of the global parent sums all children" and nothing computed it. child_account_revenue is that promise: a Field.summary over the self-referencing lookup, measured on the real engine across insert, child update, re-parent and delete before being declared. The same docs claimed cascading sharing, which crm_account (private) has never done; that claim is now corrected rather than half-fixed. Removal of published fields is authorized by the maintainer ruling of 2026-08-17, 逐个 enforce-or-remove(推荐), quoted in full in the PR body. test/decorative-field-sweep.test.ts pins both halves: the removals stay removed across every surface, and the roll-up is re-measured against the booted stack so a platform regression cannot turn the enforced row back into a decoration. Claude-Session: https://claude.ai/code/session_01XeAz8tSjwAibjdSDn9niiZ Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
os-steve
marked this pull request as ready for review
August 17, 2026 04:16
This was referenced Aug 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1182
The authorization
Maintainer ruling, 2026-08-17, quoted verbatim:
Chosen from the option described as: per-field adjudication in one card — give the three parents one rollup each or drop them, and delete the product inventory/tax surface,
customer_signature, and the hour fields. Removing published fields is normally a maintainer-only decision; that ruling is that decision.The unit of adjudication is the field. Ten rows, ten verdicts, each justified on its own below.
The measurement that decided three of the rows
Before judging the parent hierarchies I measured whether the "one rollup" the ruling offers is actually available on this platform, because the answer changes which verdict is honest. It is — and the cost argument I expected to make does not survive it.
Field.summary()is engine-computed (summaryOperations), and the engine handles it on a self-referencing relationship, where parent and child are the same object and the summary index is keyed by child. Measured on the installed 17.0.0 against the app's own booted stack, on all four legs of the lifecycle rather than at insert alone:So "enforce a parent hierarchy" costs one declaration — no hook, no recompute code, no backfill. That is not the same situation #1189 faced with
next_renewal_date, where enforcement needed a cross-objectmin()over contract statuses and was rightly called feature design. With cost off the table, each parent row had to be decided on business need instead.Row-by-row verdicts
crm_product.quantity_on_hand,reorder_pointlow_stockview they fed was not the report it looked like: its filter wasquantity_on_hand <= 10, a hardcoded constant, not each product's own reorder point sitting in the next column. HotCRM sells from a catalog; a second copy of warehouse state is drift waiting to happen.crm_product.is_taxablecreateLineItemPriceFillstampslist_priceand nothing else, andcrm_quote_line_item.tax_ratedefaults to0and is typed per line. Clearing the flag on a zero-rated product changed no total anywhere.crm_product.unit_of_measure,billing_typecrm_case.customer_signaturecrm_task.estimated_hours,actual_hoursprogress_percent, which the views do read. A rollup could be declared, but nothing in this app asks what a case cost in hours.crm_contact.reports_tosrc/pages/account_detail.page.tscontains no contact component at all, and no skill reads the chain. Sharing derives from the mastercrm_account, never from this lookup.crm_contact.birthdatecrm_account.parent_accountchild_account_revenueis now that sentence, in one declaration, measured above. Importability matters here and is why this row differs from #9 and #10 — see below.crm_campaign.parent_campaignroiis a formula over each campaign's ownactual_cost/actual_revenue, so a rolled-up ROI puts two differently-scoped ROI numbers on one record and leaves every reader guessing which they are looking at. Two truths on one object is the defect #1189 removed, not a fix.crm_case.parent_casecount(child cases)rollup would be one declaration and would compute. It was still the wrong answer: nothing in the service model acts on case parenting — no mass close, no linked-case notification, no SLA inheritance — so the number would render on a form and change nothing. A computed number nobody acts on is the same decoration with arithmetic in it. Related cases that matter here already link throughresolved_by_article, which the deflection measures do read.What importability meant for #8, and why it did not save #7
parent_accountis reachable fromsrc/mappings/account_import.mapping.ts, so a customer's account file can populate the hierarchy today, and the import guide documents the column in three locales including the forward-reference behaviour. That is a write path with a real user behind it and no reader on the other side — data arriving into a column nothing consumes. Combined with the published rollup promise, that is the closest thing to measured pull any row in this card has, and it is what tipped #8 to enforce.birthdateis importable in the same sense and still went. The difference is what the write was for: no doc promises a birthday capability, so the import column was collecting personal data toward a feature nobody had specified. Volume of a declared write path is not pull; a stated purpose is.The removal takes the
Birthdatecolumn out of both halves of the contact template — the mapping andassets/import-templates/contacts.csv. The account template is untouched and still shipsParent Account.Premise re-check (rule 6)
premise_still_valid: truefor all ten rows — re-run per row onorigin/main@c83aa744, then widened, because the card flags its own scan as untrusted.The card's scan is
src/-scoped,*.ts-only and case-sensitive. Re-running it repo-wide over every file type found no consumer for any row, but it did surface three things the narrow form would have missed, and one of them was a real defect in this PR:assets/import-templates/contacts.csvcarries theBirthdatecolumn, outside everysrc/-scoped scan. I removed the mapping and the guide, missed the CSV, andtest/import-mappings.test.tswent red comparing the two. Fixed, not worked around, andtest/decorative-field-sweep.test.tsnow names that file so a future removal fails with a message that says which field regressed.content/docs/guides/importing-your-data.mdxspells the columnBirthdate(capitalised) and did not match a lowercase pattern; the zh-Hans/zh-Hant sweep by translated label (生日,直属上级,计费类型, …) surfaced four more affected pages —revenue/index,administration/setup,sales/activities,revenue/billing-handoff. The warned failure ran backwards this time: the localized pages caught the English page.crm_product.tax_rateis equally inert but never reached the card, because the token also matches lines inquote_line_item.object.ts— a different object's field. A false negative is invisible in a scan's output. Filed ascrm_product.tax_rateis inert, and the consumer scan cannot see it — a same-named field on another object masks it #1193 with the methodology fix; not touched here, since it is not one of the ten rows.Two view-side facts the
views/-excluding scan could not show, both handled:low_stockis a whole view whose columns, filter and sort key are all removed fields, so it went with them; and no view hierarchy mode, referential action orcontrolled_by_parentderivation depends on any of the three self-lookups (crm_accountandcrm_caseareprivate,crm_campaignispublic_read, andcrm_contactderives from its master-detailcrm_account, not fromreports_to).What went with the removals
Field declaration, view columns / filters / sorts, form section entries, seed values, all four locale bundles, import mapping and CSV template,
docs/developers/api_reference.md,docs/STATUS.md(345 → 334 fields), and everycontent/docspage that described a removed field in all three locales — 27 MDX files acrossrevenue/products,revenue/index,revenue/billing-handoff,sales/contacts,sales/accounts,sales/activities,service/cases,administration/setupandguides/importing-your-data.Three doc changes are judgement rather than deletion, so they are called out:
sales/accountspromised two things and delivered neither. The rollup is now real and documented precisely (direct children, one level, maintained on edit / re-parent / delete). The "cascading sharing" claim was simply false —crm_accountisprivate— and is now stated as the non-behaviour it is, pointing at the sharing rules that do the job. [finding] The churn report never readshealth_score— the one field a CSM hand-maintains for exactly this purpose #1186'shealth_scorewas left alone.sales/contactsloses the org-chart section and gains a short note saying the tree never existed and naming what does work (title / department, which reports and the Copilot really read). A removal that leaves the reader wondering where their feature went is only half done.service/casescarries exact field arithmetic ("16 of the object's 28", "the twelve that are not on the tab", "24 of the 28"). Recomputed against the object: 27 fields, 16 on the Details tab, 11 not, 7 of those on the form, 4 on no screen. This forced one correction beyond my rows: the page's field-group table claims to be "the one list that accounts for every field the object has" and was already missingresolved_by_article(28 stated vs 29 declared, predating this PR). Subtracting two would have made the page arithmetically false in a way a reader can check, so the missing entry is restored — the minimum edit that leaves the page true.Gates run
All green at
e16507d7, the final commit — the union was run after the last commit, at that sha.pnpm verify=validate && typecheck && lint && lint:i18n-gate && hygiene && hygiene:tokens && build && test— exit 0. 110 test files, 2680 passed, 1 skipped.node scripts/check-stackblitz-lock.mjs— theci.ymlgateverifydoes not include, thepnpm verifyis not the CI gate set: a green local verify cannot clear this repo, and AGENTS.md's platform-upgrade rule names only one of the two lockfiles #1149 gap refactor(account): remove the dead account-level renewal model #1189 found:package-lock.json is in sync with package.json (v3).grep -naP '[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]') — clean.Both gates the card named as friends did their job and neither was papered over:
test/docs-drift.test.tshelddocs/STATUS.mdto the field count, andtest/import-mappings.test.tscaught the CSV template described above.Source token ratchet — down in all three scopes
The re-anchor advisory did not fire — it triggers above 10% headroom and no scope reaches it — so no ceiling is lowered here. Worth knowing why the drop is modest: each removed declaration left a short comment saying why the field is absent, and the ratchet is comment-stripped by design, so the numbers move by the declarations only.
Not run locally, left to CI
codeql,e2e(Playwright — grep confirms no spec references any removed field or thelow_stockview),link-check,changeset-check(this PR adds.changeset/decorative-field-sweep.md,minor, with per-removal migration notes).docs-appdeserves a note: it is the only job that compilescontent/docs, and it triggers only onapps/docs/**— so this PR's 27 MDX files will not be compiled by CI. That gap is already filed as #1169 and is not mine to fix here. Since CI will not check them, every edited page was structurally validated locally (frontmatter present and closed, code fences balanced, no ragged markdown tables) and all internal links I introduced match link forms already in use on neighbouring pages.Scope
Held to the ten rows.
crm_account.health_scorewas not touched (#1186). #1180'stask_do_not_call_guardandevent_do_not_call_guardinsrc/objects/task.hook.ts/event.hook.tsare untouched —crm_taskwas edited only where the two hour fields reached.Filed out of scope, unassigned:
crm_product.tax_rateis inert, and the consumer scan cannot see it — a same-named field on another object masks it #1193 —crm_product.tax_rateis inert, and the consumer scan cannot see it: a same-named field on another object masks it. Includes the scan-methodology fix, which is the part with leverage.Tests
test/decorative-field-sweep.test.ts(new, 34 tests) pins both halves:low_stockand its switcher tab asserted together, since a tab pointing at a deleted view and a view unreachable from any tab are two different broken states;parent_accountstill has the consumer it was kept for, and that consumer still works — the four-leg lifecycle above, re-measured against the booted stack on every run.Half 2 is the half worth arguing for. A test that only asserted
child_account_revenueis declared would pass just as happily on the day the platform stopped computing it — which would leave the enforced row in exactly the state nine other fields were removed for being in. The enforce verdict is only true while the engine keeps its side, so the test measures rather than reads.Every absence assertion carries a positive control, and two of them earned their keep during this work: an accessor typo (
.fieldsfor.fieldMapping) made both import-mapping assertions vacuous, and the controls turned red rather than lettingexpect([]).not.toContain(...)pass over an empty array.Generated by Claude Code
Generated by Claude Code