Fix reverse balance opening boundary adjustments

[BeastOrange]

Summary

Fixes #2497.

Reverse balance sync now surfaces opening-boundary discrepancies as explicit balance adjustments instead of silently creating an unexplained cross-day jump after the opening anchor.

Changes

• Add an opening-boundary adjustment on opening_anchor_date + 1 during reverse balance calculation.
• Preserve the opening anchor as the starting point and keep the next day ending balance derived from provider/current/reconciliation data.
• Cover both calculator output and persisted balances table generated columns.

Verification

• bin/rails test test/models/balance/materializer_test.rb
• bin/rails test test/models/balance
• bin/rubocop app/models/balance/reverse_calculator.rb test/models/balance/reverse_calculator_test.rb test/models/balance/materializer_test.rb
• bin/rails test
• git diff --check

Summary by CodeRabbit

• Bug Fixes

  • Reverse balance syncing now records opening-boundary adjustments explicitly, avoiding silent balance jumps.
  • Opening-boundary adjustments now compute and propagate cash vs non-cash impacts consistently, including correct direction for asset vs liability-style accounts.
  • Reverse materialization now persists and carries opening/boundary/transaction balance changes accurately across dates.

• Refactor

  • Centralized flow-direction logic for balance calculations to ensure consistent sign handling.

• Tests

  • Added regression coverage for opening-boundary adjustment persistence and direction, and updated expectations for reconciliation scenarios.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bb9e1f30-7d2e-4372-9d11-78b3161023f9

📥 Commits

Reviewing files that changed from the base of the PR and between 9138f342a6650f43acb916366e1f1cf82edfec58 and e53ef25.

📒 Files selected for processing (5)

• app/models/balance/base_calculator.rb
• app/models/balance/forward_calculator.rb
• app/models/balance/reverse_calculator.rb
• test/models/balance/materializer_test.rb
• test/models/balance/reverse_calculator_test.rb

💤 Files with no reviewable changes (1)

• app/models/balance/forward_calculator.rb

🚧 Files skipped from review as they are similar to previous changes (3)

• test/models/balance/materializer_test.rb
• test/models/balance/reverse_calculator_test.rb
• app/models/balance/reverse_calculator.rb

---

📝 Walkthrough

Walkthrough

Reverse balance calculation now emits explicit opening-boundary cash and non-cash adjustments, and reverse materialization/tests now assert the adjusted persisted balances.

Changes

Reverse balance opening-boundary adjustment

Layer / File(s)	Summary
Shared flow sign helper app/models/balance/base_calculator.rb, app/models/balance/forward_calculator.rb	Centralizes persisted-balance flow sign handling in BaseCalculator and removes the forward calculator’s local override.
Opening-boundary handling app/models/balance/reverse_calculator.rb	Initializes per-day adjustment values, applies the opening-boundary branch, computes boundary adjustments, and passes them into build_balance.
Reverse balance tests test/models/balance/reverse_calculator_test.rb, test/models/balance/materializer_test.rb	Adds reverse and materializer coverage for opening-boundary adjustments, persisted rows, and updated balance expectations across affected scenarios.

Sequence Diagram(s)

sequenceDiagram
  participant BalanceMaterializer
  participant ReverseCalculator
  participant BalancesTable
  BalanceMaterializer->>ReverseCalculator: strategy :reverse
  ReverseCalculator->>ReverseCalculator: detect opening boundary day
  ReverseCalculator->>BalancesTable: persist cash_adjustments and non_cash_adjustments

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• we-promise/sure#1663: Also changes Balance::ReverseCalculator#calculate to special-case reverse balance values around reconciliation waypoints.
• we-promise/sure#2434: Also changes Balance::ReverseCalculator#calculate around reverse-sync dates near the opening anchor.

Suggested labels

contributor:verified, pr:verified

Suggested reviewers

• jjmata

Poem

A bunny hopped by balances bright,
and found the boundary set just right.
Cash and non-cash now speak their part,
so reverse days can add up smart.
🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: fixing reverse balance opening-boundary adjustments.
Linked Issues check	✅ Passed	The PR surfaces the opening-boundary discrepancy as an explicit adjustment, matching #2497's requirement to expose the plug and preserve articulation.
Out of Scope Changes check	✅ Passed	The supporting balance-calculator refactors stay tied to the opening-boundary fix and don't introduce unrelated changes.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details

{}

[coderabbitai]

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details

{}

[jjmata]

Good fix — surfacing the opening-boundary discrepancy as an explicit adjustment rather than a silent cross-day balance jump makes the history auditable and consistent with how other adjustments are handled.

Two things worth double-checking:

Liability account coverage. The flows_factor method correctly flips the sign for liabilities (-1), but all the updated test cases use depository/asset accounts. A test with a credit-card or loan account would confirm the sign convention is correct for the liability case, especially since the boundary adjustment formula (end_cash_balance - opening_cash_balance - cash_flows_total(flows)) is sign-sensitive.

Interaction with reconciliation waypoints. The PR updates the existing "reconciliation waypoint" test assertions to reflect that the day after the opening anchor now carries a cash_adjustments value. The old assertion was cash_adjustments: 0 and the new one is cash_adjustments: 2000 (etc.). That's the correct behaviour, but it's worth confirming in the description (or a comment) that this is intentional — i.e., the gap between the opening anchor and the first reconciliation waypoint is now explicitly attributed to the boundary date rather than silently absorbed by the waypoint logic.

---

Generated by Claude Code

[jjmata]

Review

Solid fix. Surfacing the opening-boundary discrepancy as an explicit cash_adjustments/non_cash_adjustments value on opening_anchor_date + 1 (instead of a silent cross-day jump) makes the reverse-sync history auditable and consistent with how other adjustments are handled.

I verified the adjustment math reproduces the DB generated columns (db/schema.rb end_cash_balance/end_non_cash_balance/end_balance): with the boundary day's start = opening and cash_adjustments = end_cash − opening_cash − (cash_inflows − cash_outflows)·flows_factor, the generated end_cash_balance collapses back exactly to the calculator's value. The CreditCard test confirms the liability sign path, and the investment cases correctly route the plug to cash while non-cash nets to 0. CI is green and I found no correctness bugs.

The change requests below are all minor / non-blocking cleanups — happy to approve once they're addressed or explicitly waved off.

Requested changes

1. Deduplicate the flows-factor expression. flows_factor in app/models/balance/reverse_calculator.rb re-implements account.classification == "asset" ? 1 : -1, which already exists in Balance::BaseCalculator#build_balance (base_calculator.rb:162). Consider lifting it to a single shared method on the base calculator so the convention lives in one place.

2. Consider reusing the existing adjustment helpers. base_calculator.rb already defines cash_adjustments_for_date / non_cash_adjustments_for_date (lines 57–67), which encode the same end − start − net_flows shape (with balance_type gating). Reusing them in opening_boundary_adjustment would keep the adjustment logic centralized and the non-cash gating consistent for edge account types (e.g. loans). Please double-check behavior parity before refactoring.

3. Add doc comments to the new helpers. The rest of reverse_calculator.rb documents each branch/method; the new helpers (opening_boundary_adjustment, opening_balance_components, cash_flows_total, non_cash_flows_total, flows_factor) are undocumented, which is also what triggers the CodeRabbit docstring-coverage warning. A one-line comment each would match the file's style.

---

Generated by Claude Code

[BeastOrange]

Addressed the review cleanup in e53ef25:

• Centralized flows_factor in Balance::BaseCalculator and removed the duplicate calculator implementations.
• Reused cash_adjustments_for_date for the opening-boundary cash adjustment.
• Added one-line comments for the new boundary helpers.
• Kept the opening-boundary non-cash calculation local and market-value-aware so investment account behavior stays unchanged; the base non-cash helper is gated to balance_type == :non_cash.

Verification:

• bin/rubocop app/models/balance/base_calculator.rb app/models/balance/forward_calculator.rb app/models/balance/reverse_calculator.rb test/models/balance/reverse_calculator_test.rb test/models/balance/materializer_test.rb
• bin/rails test test/models/balance/reverse_calculator_test.rb test/models/balance/materializer_test.rb
• git diff --check
• Remote checks are green.

[sure-design]

e53ef25 addresses all three items from the changes-requested review: flows_factor is now centralized in Balance::BaseCalculator (duplicate implementations removed), the opening-boundary cash adjustment reuses the existing cash_adjustments_for_date helper, and doc comments were added to the new boundary helpers. CI is green — this should be ready for a re-review to clear the changes-requested status.

---

Generated by Claude Code