#92 Historical Equity Rate: the account guard (references are done) — and it should test fx_method, not account type

[Anonymous]

Originally created by: grynn-in

What is the Historical Equity Rate doctype?

It's a narrow IAS-21 override used during currency translation in consolidation. When a foreign subsidiary's trial balance is translated into the group reporting currency, equity accounts (share capital, premium, pre-acquisition reserves) must not move with FX — they're frozen at the historical rate on the date the equity was established/acquired. One row = "for equity account X in entity Y of group Z, translate at frozen rate R." It is not a general FX table — it has no currency-pair fields.

Consumed by gold_consolidated_trial_balance.sql:214–220:

when accounting_currency = reporting_currency  → 1.0
when is_equity = 1 and historical_rate exists  → historical_equity_rate   ← this doctype
when is_balance_sheet                           → closing_rate
when is_pnl                                      → average_rate

This issue tracks 6 correctness/design defects found in that doctype and its dbt consumption.

Findings

#	Severity	Issue	Where	What's wrong	Fix direction
1	Critical	Cancel doesn't remove rate; drafts leak to ClickHouse	historical_equity_rate.py:22-29 → clickhouse.py:237-255	sync_doctype runs frappe.get_all with no docstatus filter, so the full TRUNCATE+INSERT re-syncs draft (0) and cancelled (2) docs. on_cancel re-inserts the just-cancelled row; drafts leak in on the next submit of any doc. Systemic — same pattern in Ownership Period, IC Balance, Consolidation Adjustment.	Add filters={"docstatus": 1} in sync_doctype for submittable doctypes
2	Critical	dbt lookup ignores period_date (wrong as-of)	gold_consolidated_trial_balance.sql:233-250	Comment says "latest rate_date ≤ period_date" but row_number() ... order by rate_date desc + rn = 1 just takes the most recent rate_date ever — no period correlation. Earlier periods get a future rate. Contrast the correct asof join used for ownership. The autoname includes rate_date precisely to allow multiple tranches, but only the latest is ever used.	Replace row_number/rn=1 with an ASOF join keyed on period_date
3	High	Free-text keys → silent join misses	historical_equity_rate.json:14-34	consolidation_group, data_area_id, main_account are Data, not Link. Any typo silently fails the exact-string dbt join → equity quietly falls back to closing rate (line 217), no error. Conflicts with the app's Link/referential-integrity convention; consolidation_group is a Link elsewhere.	Convert to Link fields
4	Medium	No validation	historical_equity_rate.py (no validate)	No check that main_account is an equity account — yet dbt applies the rate only when is_equity = 1 (line 216), so a misplaced rate is silently dropped. No duplicate or positive-rate guard. (Ownership Period has a validate(); this doesn't.)	Add validate(): equity-account check + duplicate/positive guards
5	Low	Permissions too narrow	historical_equity_rate.json:57-66	Only System Manager. Rest of the consolidation app grants EPM Admin / System Manager / Administrator; an EPM Admin can't manage historic rates.	Add EPM Admin + Administrator roles
6	Low	autoname length risk	historical_equity_rate.json:11	HER-{consolidation_group}-{data_area_id}-{main_account}-{rate_date} packs four free-text fields into Frappe's 140-char name → truncation/collision.	Hash or shorten the naming series

Notes

• #1 and [#2] corrupt consolidated numbers; #3 makes failures silent. These three should be prioritized.
• The existing dbt test assert_equity_uses_historical_rate.sql does not catch [#2] — it only checks that a historical rate was used, not the correct-period one. Add a period-specific assertion when fixing [#2].
• [#1] must also be fixed in sync_doctype itself so the planned manual Exchange Rate doctype (#91) doesn't inherit the same leak.

Repos affected

• konsol (Frappe app at docker/frappe/konsol): [#1] (sync_doctype, the doctype hooks), [#3], [#4], [#5], [#6].
• open_epm (this repo): [#2] (gold_consolidated_trial_balance.sql), the new dbt test.

Related: [#91] (FX configuration).

[Anonymous]

Originally posted by: grynn-in

Status audit against current main — 5 of 6 findings resolved, scoping down.

• ✅ #1 (docstatus leak) — clickhouse.py::sync_doctype now applies {"docstatus": 1} for submittable doctypes (keyed on is_submittable, so it fixes Ownership Period / IC Balance / Consolidation Adjustment / HER at once).
• ✅ #2 (wrong as-of) — replaced the row_number()/rn=1 with an ASOF join on period_date in gold_consolidated_trial_balance.sql.
• ⚠️ #4 (validation) — validate() rejects non-positive rates; duplicates blocked via collision-safe autoname(). Remaining: the is-equity-account check (a rate on a non-equity account is still silently dropped by the dbt is_equity=1 gate).
• ✅ #5 (permissions) — roles now include EPM Admin + Administrator.
• ✅ #6 (autoname length) — replaced the 4-field format: name with digest_name() (readable head + hash), no truncation/collision.

Still open: #3 — consolidation_group / data_area_id / main_account are still Data, not Link, so a typo silently misses the dbt join → equity quietly falls back to closing rate with no error. Fixing this needs the field-type change plus a migration patch to validate existing values resolve to real Links. Leaving this issue open scoped to #3 (+ the [#4] equity-account check).

[Anonymous]

Originally posted by: grynn-in

Finding [#3] addressed (konsol [#81], merged). A literal Data→Link conversion is infeasible — there's no Link target (Consolidation Group is named CG-{group}-{entity}, the dbt join keys on the bare code, and there's no Legal Entity / Main Account doctype). So referential integrity is now enforced in validate(): the group must be a known group and the entity a known member entity in the Consolidation Group registry (a typo previously missed the gold join silently → closing-rate fallback).

Checked independently, not as a (group, entity) pair — the Consolidation Group doctype currently diverges from the dbt seed gold joins on (#130), so a pair check would reject seed-correct keys. Pair enforcement lands once [#130] is resolved.

Remaining on this issue: finding [#4]'s is-equity-account check (a rate on a non-equity account is still silently dropped by the dbt is_equity=1 gate — needs a ClickHouse/account lookup). Keeping [#92] open scoped to that + pair-enforcement (pending [#130]).

[Anonymous]

Originally posted by: grynn-in

📋 Decision brief (options + trade-offs + recommendation): docs/developer-guide/decisions/konsolidat-92-historical-equity-rate.md — merged in [#133].

[Anonymous]

Originally posted by: grynn-in

Finding [#4] — reassessed after [#130]. The (group, entity) referential integrity is already enforced by assert_equity_rate_coverage (error-level), and #130 made it green (historical_equity_rates now matches the seed: GROUP_CORP×{USMF,DEMF,GBMF,JPMF} + AMG×{AMHQ,AMUS,AMDE}).

The one remaining bit — the is-equity-account guard — turns out to be low value as an interim dbt test: on the real D365 chart the HER demo accounts 3010/3100 are BalanceSheet/ProfitAndLoss (not Equity) and carry no DEMF activity, so the equity join is inert on real data (as assert_equity_rate_coverage already notes). A warn test would just re-state that known demo mismatch on every build.

Decision: defer the is-equity guard to its proper form — a doctype validate() against a synced equity-account registry — and fold it into the single-source-of-truth work in konsol [#82] (which also fixes the HER dual-writer). Keeping [#92] open, scoped to that one guard. Findings #1/#2/#3/#5/#6 are done; [#4]'s coverage half is done.

[Anonymous]

Originally posted by: grynn-in

Status check, 11 Sep 2026 — four of the six are fixed by later work. Narrowing this issue to what remains.

#	status	evidence
1 Critical — docstatus leak	fixed	clickhouse.resolve_sync_filters returns {"docstatus": 1} for any submittable doctype, on both write paths (document hooks and reconcile_all) since grynn-in/konsol#112
2 Critical — dbt lookup ignores period_date	fixed	gold_consolidated_trial_balance.sql:279 is an asof left join keyed on period_date, with a Nullable(Float64) cast so a miss falls back to the closing rate instead of 0
3 High — free-text keys	partly	data_area_id is a Link to Entity (F1). consolidation_group and main_account are still Data
4 Medium — no validation	fixed	validate() now calls _validate_positive_rate and _validate_references
5 Low — permissions too narrow	fixed	roles are System Manager, EPM Admin, Administrator
6 Low — autoname length risk	probably fixed	autoname is now empty rather than the four-field format string — worth a glance to confirm that is deliberate

Remaining: [#3] (two fields) and a glance at [#6].

One thing this issue did not anticipate, and which matters more than either: grynn-in/konsol#82. epm_staging.historical_equity_rates still has two writers, and on the live stack the table is at 0 rows — the doctype holds nothing, so every reconcile truncates whatever demo-data inserted. That is what makes assert_equity_rate_coverage one of the two remaining baseline build failures.

[Anonymous]

Originally posted by: grynn-in

Rescoped on 12 Sep 2026 after an issue review against main.

Done:

• data_area_id is a Link to Entity.
• EPM Admin can edit.
• Naming uses digest_name.
• Positive-rate and reference validations exist.

Remaining:

• consolidation_group and main_account are still Data fields.
• There is no check that the account is an equity account.
• The _validate_references docstring still refers to the deleted seed.

The dual-writer problem (grynn-in/konsol#82) went away with the demo removal. grynn-in/konsol#103 (one governed rate table) may later absorb this doctype.

[Anonymous]

Originally posted by: grynn-in

Verdict: PARTLY DONE — recommend narrowing the scope rather than closing.

The integrity half appears built. historical_equity_rate.py now has _validate_references, which refuses an unknown consolidation group ("Unknown consolidation group '…'") and an unknown entity ("Unknown entity '…'. It must be a member entity"), plus _validate_positive_rate.

What I could not find is the equity-account guard — nothing appears to refuse a main_account that is not an equity account.

Worth re-scoping rather than just finishing: since konsol#182 the chart declares fx_method per account, and gold_consolidated_trial_balance selects the historical rate on fx_method = 'historical', not on account type. So the correct guard is probably "the account must be declared fx_method = historical", not "the account must be Equity". On the current chart those differ sharply: 21 accounts are declared historical, of which only 6 are equity leaves — 14 assets and 1 liability are too.

Live state worth knowing: zero Historical Equity Rate records exist, while 5,481 rows in the consolidated trial balance sit on historical-method accounts translated at a rate other than 1 — every one of them falling back to the closing rate.

🤖 Triage against main — Claude Code · https://claude.ai/code/session_01P3Pf9835FeLeXjRrYTTZ1M

[Anonymous]

Originally posted by: grynn-in

Re-scoped — the integrity half is done, and the remaining half needs a different rule

Done: historical_equity_rate.py now carries _validate_references, refusing an unknown consolidation group ("Unknown consolidation group '…'") and an unknown entity ("Unknown entity '…'. It must be a member entity"), plus _validate_positive_rate.

What remains: the account guard. Nothing refuses a main_account that should not carry a historical rate.

But the rule this issue proposes has been overtaken. It asks for an equity-account guard. Since konsol#182 the chart declares fx_method per account, and gold_consolidated_trial_balance selects the historical rate on fx_method = 'historical' — not on account type. On the current chart the two differ sharply:

• 21 accounts are declared historical
• of those, only 6 are equity leaves; 14 assets and 1 liability are also declared historical
• and 3400 Noncontrolling interest is the one equity leaf not declared historical

So an equity-only guard would refuse 15 accounts the warehouse will happily translate at a historical rate, and permit one it will not. The guard should test the declaration: refuse a main_account whose chart row is not fx_method = 'historical'.

Live state worth knowing before this is built: zero Historical Equity Rate records exist, while 5,481 rows in the consolidated trial balance sit on historical-method accounts translated at a rate other than 1 — every one falling back to the closing rate, across 194 distinct rates. There is also an open question about whether declaring 14 assets historical is intended (that is the temporal method, not IAS 21.39 current-rate) — worth settling first, since it changes which accounts this guard should accept.

🤖 Re-scoped 15 Sep 2026 against main — Claude Code · https://claude.ai/code/session_01P3Pf9835FeLeXjRrYTTZ1M

[Anonymous]

Ticket changed by: grynn-in

• status: open --> closed

[Anonymous]

Ticket changed by: grynn-in

• status: closed --> open

[Anonymous]

Originally posted by: grynn-in

Reopening: I closed this on merge, and that was wrong. This issue was scoped to finding 3 (the free-text keys) plus the equity-account half of finding 4. Only finding 4 shipped.

Shipped (grynn-in/konsolidat#217, with grynn-in/konsol#241): the warehouse now names a historical declaration on a non-equity account, and a Historical Equity Rate keyed to an account the chart does not type as equity, instead of discarding it in silence.

Still open, and the reason this issue stays open: consolidation_group, data_area_id and main_account on Historical Equity Rate are Data, not Link. A typo therefore misses the dbt join and the balance falls back to the closing rate with nothing said. That matters more now than it did: the user decided on 16 Sep that the retained-earnings account may declare historical or closing, so customers will start entering these rates, and a silent miss becomes a wrong number rather than a theoretical one. Fixing it needs the field-type change plus a migration that proves existing values resolve.

[Anonymous]

Originally posted by: grynn-in

Closing: finding 3 is resolved as far as it can be, and the reasoning now lives in the code rather than here.

main_account is a Link to Main Account (grynn-in/konsol#244). That doctype is named by its own code, so the stored value is unchanged and the dbt join is untouched byte for byte.

consolidation_group and data_area_id stay as they are, deliberately. Consolidation Group is named CG-{group}-{entity} while the warehouse joins on the bare code, so a Link there would store a string the join cannot use — trading a silent miss for a certain one. Both keys are instead checked for existence in _validate_references, which also fires on the write paths Frappe's own link validation skips (a patch, an import, ignore_links), and the docstring explains the asymmetry.

A migrate patch rewrites existing account values to the stored spelling, or stops the migrate naming every value that does not resolve and the rates carrying it. Nothing is guessed.

The silent-drop class this issue was really about is now covered from both sides: konsol refuses the bad key at entry, and grynn-in/konsolidat#219 names any account and period that declares historical with no rate covering it.

[Anonymous]

Originally posted by: grynn-in

Fixed by grynn-in/konsol#244 (the account key and its migration) and grynn-in/konsolidat#217 (the warehouse naming its silent drops).

[Anonymous]

Ticket changed by: grynn-in

• status: open --> closed