Skip to content

fix(imports): import raw balance records#1724

Merged
jjmata merged 3 commits into
we-promise:mainfrom
JSONbored:codex/fix-import-raw-balances
May 11, 2026
Merged

fix(imports): import raw balance records#1724
jjmata merged 3 commits into
we-promise:mainfrom
JSONbored:codex/fix-import-raw-balances

Conversation

@JSONbored
Copy link
Copy Markdown
Contributor

@JSONbored JSONbored commented May 10, 2026

Summary

  • Adds Sure NDJSON Balance import support to match existing raw balance export records.
  • Upserts imported balances by account, date, and currency so repeated imports remain idempotent.
  • Adds focused importer and export/import roundtrip coverage.

What changed

  • Family::DataImporter now accepts Balance records and imports them after accounts are remapped.
  • Balance imports remap account_id, default missing currency to the target account currency, and skip generated balance columns.
  • Imported balance history is considered when creating account opening anchors.
  • Tests cover raw balance import, idempotent re-import, opening-anchor date behavior, and full export/import roundtrip.

Why

Sure exports materialized raw balance history, but importer support previously reconstructed historical values only through valuation-style records. That left exported Balance rows without a matching restore path.

Validation

  • docker compose -f .devcontainer/docker-compose.yml exec -T app bash -lc 'cd /workspace && bin/rails test test/models/family/data_importer_test.rb test/models/family/data_exporter_test.rb'
  • docker compose -f .devcontainer/docker-compose.yml exec -T app bash -lc 'cd /workspace && bin/rubocop app/models/family/data_importer.rb test/models/family/data_importer_test.rb test/models/family/data_exporter_test.rb'
  • git diff --check

Notes

  • Draft PR for focused review.
  • This does not change balance export shape or public API behavior.

Summary by CodeRabbit

  • New Features
    • Import balance history during financial imports, including opening balances, cash flows, and related account metrics.
    • Balance import runs immediately after accounts so balances are available for downstream processing.
  • Bug Fixes
    • Duplicate balance records are handled idempotently; later imports update existing entries without erasing omitted components.
    • Earliest account dates now consider imported balances for correct ordering.
  • Tests
    • Added coverage for balance import, duplicates behavior, date ordering, and export/import round-trip.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2e343804-d8d4-428a-934d-12afe4301356

📥 Commits

Reviewing files that changed from the base of the PR and between 5b12f68 and 44f09d8.

📒 Files selected for processing (2)
  • app/models/family/data_importer.rb
  • test/models/family/data_importer_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
  • app/models/family/data_importer.rb
  • test/models/family/data_importer_test.rb

📝 Walkthrough

Walkthrough

This PR extends Family::DataImporter to recognize and import Balance NDJSON records. Balance records are parsed from NDJSON, have their account IDs remapped, and are upserted with numeric fields normalized to decimals. The import sequencing places Balance imports immediately after Accounts, and earliest-date tracking now includes Balance records to ensure account opening balances precede all subsequent activity.

Changes

Balance Record Import

Layer / File(s) Summary
Type Registration
app/models/family/data_importer.rb
SUPPORTED_TYPES updated to include "Balance" so Balance NDJSON records are recognized and routed.
Import and Conversion Helpers
app/models/family/data_importer.rb
import_balances remaps account_id, parses dates, validates required inputs, converts numeric fields to decimals (via optional_decimal), and normalizes flows_factor (balance_flows_factor_for) before upserting balances.
Import Sequencing
app/models/family/data_importer.rb
import! transaction now calls import_balances after import_accounts and before downstream record types.
Earliest Date Tracking
app/models/family/data_importer.rb
oldest_import_entry_dates_by_account now includes "Balance" when computing per-account earliest imported dates.
Test Coverage
test/models/family/data_importer_test.rb
Tests added for: single Balance import with numeric fields, idempotent duplicate handling by account/date/currency, preserving omitted fields on duplicates, opening-anchor date ordering, and full export/import round-trip for balance history.

Sequence Diagram(s)

sequenceDiagram
  participant Importer as Family::DataImporter
  participant AccountMap as Account ID Map
  participant Parser as Date Parser
  participant Validator as Field Validator
  participant Converter as Decimal Converter
  participant Balance as Balance Model

  Importer->>Importer: import_balances(records)
  loop per Balance record
    Importer->>AccountMap: remap account_id
    AccountMap-->>Importer: mapped_account_id
    Importer->>Parser: parse date string
    Parser-->>Importer: parsed_date
    Importer->>Validator: check mapped_account, date, balance present
    Validator-->>Importer: valid or skip
    Importer->>Converter: optional_decimal(cash fields)
    Converter-->>Importer: decimalized values
    Importer->>Converter: balance_flows_factor_for(value)
    Converter-->>Importer: normalized ±1 or 1
    Importer->>Balance: find_or_initialize_by(date, currency, account) + assign attrs
    Balance-->>Importer: upsert complete
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • we-promise/sure#1641: Adds Balance record exports from Family::DataExporter, which pairs with this PR's Balance import functionality.
  • we-promise/sure#1595: Modifies account opening balance and oldest-imported-date logic that this PR extends to include Balance records.
  • we-promise/sure#1643: Adds a new NDJSON record type and adjusts SUPPORTED_TYPES/import ordering in Family::DataImporter similar to this change.

Suggested reviewers

  • jjmata

Poem

🐰 I hopped through NDJSON, sniffed each balance line,
IDs remapped, decimals set, dates aligned just fine.
Upserts keep duplicates neat, omitted bits stay true,
Opening anchors placed before the history grew—
Round-trip danced back home, and I left a carrot or two. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% 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 and specifically describes the main change: adding support for importing raw balance records into the Family::DataImporter, which matches the primary objective and implementation details across both modified files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Collaborator

jjmata commented May 11, 2026

Clean fix — threading Balance into SUPPORTED_TYPES, the dependency ordering, and the idempotent find_or_initialize_by(date:, currency:) upsert all look correct.

One semantic note on imported_balance_attributes:

cash_inflows: decimal_or_default(data["cash_inflows"]),  # returns 0 when absent

decimal_or_default returns 0 for missing fields. On a re-import (same account + date + currency), any flow columns absent from the NDJSON will overwrite existing non-zero DB values with 0. If the intent is full-replace semantics that's fine — just worth confirming. If the intent is "only update what's explicitly provided", the flow fields would need optional_decimal (which returns nil, leaving the DB value unchanged) combined with filtering nils out of the attributes hash before assign_attributes.


Generated by Claude Code

@JSONbored JSONbored marked this pull request as ready for review May 11, 2026 06:17
@superagent-security superagent-security Bot added contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis. labels May 11, 2026
@jjmata jjmata merged commit c167818 into we-promise:main May 11, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis.

Development

Successfully merging this pull request may close these issues.

2 participants