fix: update header_results with corrected opening balance - #216
Merged
Merged
Conversation
_apply_opening_balance_correction() corrected checks_and_balances, lines_results, and the scalar, but did not update header_results. This caused the corrected opening balance to never reach parquet/SQLite, leading to false GAP flags in the GapReport VIEW. Changes: - Update header_results with corrected STD_OPENING_BALANCE - Remove redundant scalar assignment (derived from header_results at line 560 anyway) - Add diagnostic print on correction - Add test_correction_updates_header_results - Update existing tests to derive scalar from header_results Closes #215 Signed-off-by: Jason Farrar <stillwaters.technology@gmail.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The fix is narrowly scoped to the persistence pathway for corrected opening balances and is backed by targeted tests, with only minor non-blocking nits identified.
Pull request overview
This PR fixes an end-to-end data propagation bug where _apply_opening_balance_correction() corrected the opening balance in internal validation/line results but did not update header_results, so StatementHeads persisted the wrong STD_OPENING_BALANCE to Parquet/SQLite and caused false GAP flags (notably for Halifax statements).
Changes:
- Update
_apply_opening_balance_correction()to overwriteheader_results.STD_OPENING_BALANCEso downstream persistence uses the corrected value. - Remove the redundant
std_opening_balancescalar assignment (since it is derived fromheader_resultslater inStatement.__init__). - Add/extend tests to ensure
header_resultsand the derived scalar reflect the corrected opening balance.
File summaries
| File | Description |
|---|---|
| src/bank_statement_parser/modules/statements.py | Persist corrected opening balance into header_results so StatementHeads writes the corrected value to Parquet/SQLite; add a diagnostic print. |
| tests/test_opening_balance_correction.py | Extend helper and add a test to verify the correction updates header_results, and adjust scalar expectations to derive from header_results. |
Review details
Suppressed comments (1)
tests/test_opening_balance_correction.py:185
- This test comment references an exact line number in
Statement.__init__("line 560"), which will drift as the file changes and becomes misleading. Prefer describing the behavior without hard-coding line numbers.
# Simulate Statement.__init__ line 560: scalar derived from header_results
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses PR review feedback: remove line-number reference that will drift, replace with behavioral description. Signed-off-by: Jason Farrar <stillwaters.technology@gmail.com>
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.
Summary
_apply_opening_balance_correction()instatements.pycorrectly updatedchecks_and_balances,lines_results, and the scalarstd_opening_balance— but did not updateheader_results. SinceStatementHeadsreadsSTD_OPENING_BALANCEfromheader_resultsto write to parquet/SQLite, the corrected opening balance never reached the database.This caused the
GapReportVIEW to compare the uncorrected (wrong) opening balance against the previous statement's closing balance, producing false GAP flags for Halifax current account statements.Changes
statements.py: Updateheader_resultswith correctedSTD_OPENING_BALANCEin_apply_opening_balance_correction(). Remove redundant scalar assignment (it's derived fromheader_resultsat line 560). Add diagnostic print on correction.test_opening_balance_correction.py: Addtest_correction_updates_header_results. Addheader_resultsto test helper. Update existing tests to derive scalar fromheader_resultspost-correction.Closes
Closes #215