feat/rpu-history
needs attention33484d7 · incrementalPR #324reviewed 2026-07-22 21:00 UTC0H · 2M · 3L · 3I- Purpose
- Add RPU contract history features: rate-tag chip showing tariff/service classification, per-chapter regime timeline drawer, per-bill tariff display (own-bill TARIFA_REG wins over contract fallback), RPU-union bill count across chapters, and a serviceName/address refresh from strictly-newer bill evidence.
- Goal
- Complete the RPU-agreements UX arc (closes #314/#319/#321/#323) with the remaining features from rpu-history-spec.md.
- Sub-goals
- SG-1: Rate-tag chip (tariff_code + Autoabasto tag) in Contratos
- SG-2: Per-bill regime display in Historial (bill-own TARIFA_REG wins)
- SG-3: RPU-union bill count across chapters
- SG-4: Regime-timeline drawer (per-chapter Sheet, deep-link ?detail=<id>)
- SG-5: RPU-complete exports (audit confirmed already correct)
- SG-6: Evidence display refresh (serviceName/address from strictly-newer bill evidence)
- What
- Commit 33484d74 fixes 4 negative-path tests in decideEnsureContractFromBills: the tests now correctly set contractFields.serviceName so the guard conditions (tie/older/absent date/same name) are actually exercised, rather than passing vacuously because contractFields was absent. Also tightens the INVARIANT comment in bill.shells.ts.
- Why
- Prior negative tests passed because contractFields was undefined — the refresh logic was never entered. With contractFields now present but blocked by the guard, these tests genuinely verify the strictness of the evidence-refresh invariant.
- Areas
- apps/platform/src/api+180−20apps/platform/src/app/[locale]/(dashboard)/bills+850−60domains/utility/src/bill+180−10domains/utility/src/utility-contract+350−20domains/utility/src/utility-contract-overview+280−5packages/api/src+120−5packages/database+420−0docs/development+280−0
- Blast
- 36 files, +2667/-125 across platform UI, utility domain, API schemas, and DB migration; this commit touches 2 files (+28/-11).
Findings · 8
correctness3
Negative tests use conditional assertion style — could pass vacuously on wrong _tag
domains/utility/src/utility-contract/__tests__/utility-contract.decisions.test.ts:642
All 4 modified tests (tie, older evidence, absent evidencePeriodEnd, same name) assert `result.value.refresh` inside `if (result.ok && result.value._tag === 'UseExistingContract')`. If the decision returned a different _tag (e.g. ReactivateContract), the expect is silently skipped and the test passes vacuously. Add an unconditional `expect(result.value._tag).toBe('UseExistingContract')` before the conditional, or use `expect(result.ok).toBe(true)` + narrow tag unconditionally. This mirrors the positive-case tests at lines 592–595.
Comment rewrite drops the 'per-bill titulars remain as history' note
domains/utility/src/bill/bill.shells.ts:822
The old 5-line comment included: 'per-bill titulars remain on each bill's NOMBRE line items as history.' The new 3-line rewrite omits this. Minor loss of context — the note explained WHERE evidence persists (bill NOMBRE line items) vs WHERE the canonical name lives (the contract). Not a bug, but the deleted sentence answered an implied question about observability.
Job-level serviceName negative test (line 615) is untouched and correctly covers contractFields-absent case
domains/utility/src/utility-contract/__tests__/utility-contract.decisions.test.ts:615
The 'should NOT refresh from the job/batch-level serviceName when the evidence bill carries no titular' test calls withEvidence('2026-07-01', 'NUEVA RAZON SOCIAL SA') with no contractFields — evidenceName is undefined → refresh never fires. This correctly models the job-level invariant and is untouched by this commit. No issue.
conventions2
INVARIANT prefix label is not a canonical project comment convention
domains/utility/src/bill/bill.shells.ts:822
The project's CLAUDE.md defines no prescribed prefix vocabulary (INVARIANT/SAFETY/NOTE). The comment explains the why correctly; the prefix is informal. Low risk — either standardise it if adopting broadly, or leave it as a one-off label.
Spread-then-override fixture pattern is idiomatic
domains/utility/src/utility-contract/__tests__/utility-contract.decisions.test.ts
{ ...withEvidence(...), contractFields: { serviceName: '...' } } is the standard fixture override pattern used throughout this codebase. No convention issue.
tests3
No negative-path tests for address-field refresh guard conditions
domains/utility/src/utility-contract/__tests__/utility-contract.decisions.test.ts:707
All 4 negative tests pass only `contractFields: { serviceName: ... }`. There is one positive test for address refresh (line 707) but no test confirming that `address` in `contractFields` is also blocked by a tie date, older evidence, or absent evidencePeriodEnd. The decision checks `evidenceAddress` independently (same guard block as serviceName). A single negative test — e.g. 'should NOT refresh address on older evidence' — would close the gap.
'Job Config Name' fixture value is accurate but abstract
domains/utility/src/utility-contract/__tests__/utility-contract.decisions.test.ts
Using 'Job Config Name' as the job-level serviceName clearly signals 'this is the batch-config name, not the bill evidence name', but it could be even more explicit (e.g. 'JOB_LEVEL_NAME' or a Spanish-flavored string like 'Nombre de Batch'). Minor readability nit — the semantics are correct.
'absent evidencePeriodEnd' test short-circuits before the date comparison, not after
domains/utility/src/utility-contract/__tests__/utility-contract.decisions.test.ts:662
When evidencePeriodEnd is undefined, the decision exits the refresh block via `input.evidencePeriodEnd !== undefined` at the outer guard (line 546) — it never reaches the date comparison. The other three negative tests exercise sub-conditions inside the guard. A comment on this test distinguishing 'short-circuit before comparison' from 'comparison fires but fails' would help future readers understand why this is a separate case.